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(-)
>
