Messages in this thread | | | Subject | Re: [PATCH 0/3] Collapse vimc into single monolithic driver | From | Shuah Khan <> | Date | Mon, 12 Aug 2019 08:08:18 -0600 |
| |
On 8/9/19 9:51 PM, Helen Koike wrote: > Hi Andre, > > Thanks for testing this. > > On 8/9/19 9:24 PM, André Almeida wrote: >> On 8/9/19 9:17 PM, Shuah Khan wrote: >>> Hi Andre, >>> >>> On 8/9/19 5:52 PM, André Almeida wrote: >>>> Hello Shuah, >>>> >>>> Thanks for the patch, I did some comments below. >>>> >>>> On 8/9/19 6:45 PM, Shuah Khan wrote: >>>>> vimc uses Component API to split the driver into functional components. >>>>> The real hardware resembles a monolith structure than component and >>>>> component structure added a level of complexity making it hard to >>>>> maintain without adding any real benefit. >>>>> The sensor is one vimc component that would makes sense to be a >>>>> separate >>>>> module to closely align with the real hardware. It would be easier to >>>>> collapse vimc into single monolithic driver first and then split the >>>>> sensor off as a separate module. >>>>> >>>>> This patch series emoves the component API and makes minimal changes to >>>>> the code base preserving the functional division of the code structure. >>>>> Preserving the functional structure allows us to split the sensor off >>>>> as a separate module in the future. >>>>> >>>>> Major design elements in this change are: >>>>> - Use existing struct vimc_ent_config and struct >>>>> vimc_pipeline_config >>>>> to drive the initialization of the functional components. >>>>> - Make vimc_ent_config global by moving it to vimc.h >>>>> - Add two new hooks add and rm to initialize and register, >>>>> unregister >>>>> and free subdevs. >>>>> - All component API is now gone and bind and unbind hooks are >>>>> modified >>>>> to do "add" and "rm" with minimal changes to just add and rm >>>>> subdevs. >>>>> - vimc-core's bind and unbind are now register and unregister. >>>>> - vimc-core invokes "add" hooks from its vimc_register_devices(). >>>>> The "add" hooks remain the same and register subdevs. They don't >>>>> create platform devices of their own and use vimc's pdev.dev as >>>>> their reference device. The "add" hooks save their >>>>> vimc_ent_device(s) >>>>> in the corresponding vimc_ent_config. >>>>> - vimc-core invokes "rm" hooks from its unregister to unregister >>>>> subdevs >>>>> and cleanup. >>>>> - vimc-core invokes "add" and "rm" hooks with pointer to struct >>>>> vimc_device >>>>> and the corresponding struct vimc_ent_config pointer. >>>>> The following configure and stream test works on all devices. >>>>> media-ctl -d platform:vimc -V '"Sensor >>>>> A":0[fmt:SBGGR8_1X8/640x480]' >>>>> media-ctl -d platform:vimc -V '"Debayer >>>>> A":0[fmt:SBGGR8_1X8/640x480]' >>>>> media-ctl -d platform:vimc -V '"Sensor >>>>> B":0[fmt:SBGGR8_1X8/640x480]' >>>>> media-ctl -d platform:vimc -V '"Debayer >>>>> B":0[fmt:SBGGR8_1X8/640x480]' >>>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v >>>>> width=1920,height=1440 >>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81 >>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1 >>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2 >>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3 >>>>> >>>>> The third patch in the series fixes a general protection fault found >>>>> when rmmod is done while stream is active. >>>> >>>> I applied your patch on top of media_tree/master and I did some testing. >>>> Not sure if I did something wrong, but just adding and removing the >>>> module generated a kernel panic: >>> >>> Thanks for testing. >>> >>> Odd. I tested modprobe and rmmod both.I was working on Linux 5.3-rc2. >>> I will apply these to media latest and work from there. I have to >>> rebase these on top of the reverts from Lucas and Helen >> >> Ok, please let me know if I succeeded to reproduce. >> >>>> >>>> ~# modprobe vimc >>>> ~# rmmod vimc >>>> [ 16.452974] stack segment: 0000 [#1] SMP PTI >>>> [ 16.453688] CPU: 0 PID: 2038 Comm: rmmod Not tainted 5.3.0-rc2+ #36 >>>> [ 16.454678] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), >>>> BIOS 1.12.0-20181126_142135-anatol 04/01/2014 >>>> [ 16.456191] RIP: 0010:kfree+0x4d/0x240 >>>> >>>> <registers values...> >>>> >>>> [ 16.469188] Call Trace: >>>> [ 16.469666] vimc_remove+0x35/0x90 [vimc] >>>> [ 16.470436] platform_drv_remove+0x1f/0x40 >>>> [ 16.471233] device_release_driver_internal+0xd3/0x1b0 >>>> [ 16.472184] driver_detach+0x37/0x6b >>>> [ 16.472882] bus_remove_driver+0x50/0xc1 >>>> [ 16.473569] vimc_exit+0xc/0xca0 [vimc] >>>> [ 16.474231] __x64_sys_delete_module+0x18d/0x240 >>>> [ 16.475036] do_syscall_64+0x43/0x110 >>>> [ 16.475656] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>> [ 16.476504] RIP: 0033:0x7fceb8dafa4b >>>> >>>> <registers values...> >>>> >>>> [ 16.484853] Modules linked in: vimc(-) videobuf2_vmalloc >>>> videobuf2_memops v4l2_tpg videobuf2_v4l2 videobuf2_common >>>> [ 16.486187] ---[ end trace 91e5e0894e254d49 ]--- >>>> [ 16.486758] RIP: 0010:kfree+0x4d/0x240 >>>> >>>> <registers values...> >>>> >>>> fish: “rmmod vimc” terminated by signal SIGSEGV (Address boundary error) >>>> >>>> I just added the module after booting, no other action was made. Here is >>>> how my `git log --oneline` looks like: >>>> >>>> 897d708e922b media: vimc: Fix gpf in rmmod path when stream is active >>>> 2e4a5ad8ad6d media: vimc: Collapse component structure into a single >>>> monolithic driver >>>> 7c8da1687e92 media: vimc: move private defines to a common header >>>> 97299a303532 media: Remove dev_err() usage after platform_get_irq() >>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2 > > I couldn't reproduce the error, my tree looks the same: > > [I] koike@floko ~/m/o/linux> git log --oneline > e3345155c8ed (HEAD) media: vimc: Fix gpf in rmmod path when stream is active > 43e9e2fe761f media: vimc: Collapse component structure into a single monolithic driver > 8a6d0b9adde0 media: vimc: move private defines to a common header > 97299a303532 (media/master) media: Remove dev_err() usage after platform_get_irq() > 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2
Thanks Helen for trying to reproduce and sharing the result. > > André, is this deterministic? Or it just happens sometimes? > >>>> ... >>>> >>>>> >>>>> vimc_print_dot (--print-dot) topology after this change: >>>>> digraph board { >>>>> rankdir=TB >>>>> n00000001 [label="{{} | Sensor A\n/dev/v4l-subdev0 | {<port0> >>>>> 0}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000001:port0 -> n00000005:port0 [style=bold] >>>>> n00000001:port0 -> n0000000b [style=bold] >>>>> n00000003 [label="{{} | Sensor B\n/dev/v4l-subdev1 | {<port0> >>>>> 0}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000003:port0 -> n00000008:port0 [style=bold] >>>>> n00000003:port0 -> n0000000f [style=bold] >>>>> n00000005 [label="{{<port0> 0} | Debayer A\n/dev/v4l-subdev2 | >>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000005:port1 -> n00000015:port0 >>>>> n00000008 [label="{{<port0> 0} | Debayer B\n/dev/v4l-subdev3 | >>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000008:port1 -> n00000015:port0 [style=dashed] >>>>> n0000000b [label="Raw Capture 0\n/dev/video1", shape=box, >>>>> style=filled, fillcolor=yellow] >>>>> n0000000f [label="Raw Capture 1\n/dev/video2", shape=box, >>>>> style=filled, fillcolor=yellow] >>>>> n00000013 [label="{{} | RGB/YUV Input\n/dev/v4l-subdev4 | >>>>> {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000013:port0 -> n00000015:port0 [style=dashed] >>>>> n00000015 [label="{{<port0> 0} | Scaler\n/dev/v4l-subdev5 | >>>>> {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green] >>>>> n00000015:port1 -> n00000018 [style=bold] >>>>> n00000018 [label="RGB/YUV Capture\n/dev/video3", shape=box, >>>>> style=filled, fillcolor=yellow] >>>>> } >>>> >>>> Since the topology changed, it would be nice to change in the >>>> documentation as well. The current dot file can be found at >>>> `Documentation/media/v4l-drivers/vimc.dot` and it's rendered at this >>>> page: https://www.kernel.org/doc/html/latest/media/v4l-drivers/vimc.html >>>> >>>
Thanks Andre! Yes this is the one I am using as a reference.
>>> Topology shouldn't have changed. No changes to links or pads etc. >>> I will take a look to be sure. I agree that if topology changes >>> document should be updated. >> >> If you "diff" the current dot with the dot you generated, you will see >> some differences. The main difference is that "RGB/YUV Input" was a >> device "/dev/video2/", and now it a subdevice "/dev/v4l-subdev4". > > hmm, I just generated the topology for media/master, and it is > /dev/v4l-subdev4. As we don't have an implementation of the output device > yet, we used the sensor as a place holder and that is why it appears as > "/dev/v4l-subdev4", what is in the docs is the ideal version after we get > the output merged. We should update the docs in any case. >
Helen! Does the master match what I generated. In any case, I will do the diff of before and after my patches for sure.
I can update the document with what it should be.
thanks, -- Shuah
| |