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

[Helen Koike]

Hi Shuah,

Thanks for the patches.

On 8/15/19 4:42 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 removes 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_device and vimc_ent_config global by moving them to
>       vimc-common.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 second patch in the series fixes a general protection fault found
> when rmmod is done while stream is active.
> 
> - rmmod while streaming returns vimc is in use
> - rmmod without active stream works correctly
> 
> Changes since v1:
> Patch 1 & 2: (patch 1 in this series)
> - Collapsed the two patches into one
> - Added common defines (vimc_device and vimc_ent_config) to vimc-common.h
>   based on our discussion.
> - Addressed review comments from Helen and Laurent
> - Use vimc-common.h instead of creating a new file.
> - Other minor comments from Helen on int vs. unsigned int and
>   not needing to initialize ret in vimc_add_subdevs()
> Patch 3 (patch 2 in this series):
> - The second patch is the fix for gpf. Updated the patch after looking
>   at the test results from Andre and Helen. This problem is in a common
>   code and impacts all subdevs. The fix addresses the core problem and
>   fixes it. Fix removes pads release from v4l2_device_unregister_subdev()
>   and pads are now released from the sd release handler with all other
>   resources.
> 
> Outstanding:
> - Update documentation with the correct topology.
> - There is one outstanding gpf remaining in the unbind path. I will
>   fix that in a separate patch. This is an existing problem and will
>   be easier to fix on top of this patch series.
> 
> vimc_print_dot (--print-dot) topology after this change: (no change
> compared to media master)
> 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]
> 
> Shuah Khan (2):
>   media: vimc: Collapse component structure into a single monolithic
>     driver
>   media: vimc: Fix gpf in rmmod path when stream is active

I couldn't apply those on top of media/master, I think they are
conflicting with the two "Reverts" commits in vimc.

Regards,
Helen

> 
>  drivers/media/platform/vimc/Makefile       |   7 +-
>  drivers/media/platform/vimc/vimc-capture.c |  75 ++-------
>  drivers/media/platform/vimc/vimc-common.c  |   3 +-
>  drivers/media/platform/vimc/vimc-common.h  |  54 ++++++
>  drivers/media/platform/vimc/vimc-core.c    | 182 +++++++--------------
>  drivers/media/platform/vimc/vimc-debayer.c |  69 ++------
>  drivers/media/platform/vimc/vimc-scaler.c  |  69 ++------
>  drivers/media/platform/vimc/vimc-sensor.c  |  70 ++------
>  8 files changed, 161 insertions(+), 368 deletions(-)
>