Re: [PATCH 0/3] Collapse vimc into single monolithic driver

[Shuah Khan]

On 8/9/19 6: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
> > > ...
> > >
> > > > 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
> >
> > 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".

Yeah. I should have saved dot before my changes and compare. The goal is
to not change anything. I will make sure topology doesn't change.

Thanks again for testing these patches quickly.

thanks,
-- Shuah