[PATCH v4 0/5] Collapse vimc into single monolithic driver

* [PATCH v4 0/5] Collapse vimc into single monolithic driver
@ 2019-09-07  2:42 Shuah Khan
  2019-09-07  2:42 ` [PATCH v4 1/5] media: vimc: Collapse component structure into a " Shuah Khan
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Shuah Khan @ 2019-09-07  2:42 UTC (permalink / raw)
  To: mchehab, helen.koike, skhan, andrealmeid, dafna.hirschfeld,
	hverkuil-cisco, davem, gregkh, nicolas.ferre
  Cc: linux-media, linux-kernel

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.

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

Changed since v3: (5 patches)
Patch 1:
- Main change is adding an array of struct vimc_ent_device(s) to save
  vimc_ent_device(s), subdevs create in their "add" hooks. These
  get used to create links and removing the subdevs. vimc-core allocates
  this array which sized to number of entries in the topology defined in
  the vimc_pipeline_config structure.
- Remove links when register fails before registering the media device.
  In this case, entity links need to be removed from error paths.
Patch 2: no change.
New patch 3:
- Cleaning up duplicated IS_SRC and IS_SINK defines and moving them to
  common header.
New patch 4:
- Documentation module parameter usage updates.
New patch 5:
- Lastly adding myself as a reviewers to MAINTAINERS file for vimc

Changes since v2:
- Rebase to media master on top of vimc reverts
- Rename "vent" variable to "vcfg" to reflect config and standout
  from "ved"
- Error handling when adding subdevs fails to remove already added
  subdevs, do other clean-up and bail out.

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/video0", shape=box, style=filled, fillcolor=yellow]
        n0000000f [label="Raw Capture 1\n/dev/video1", 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/video2", shape=box, style=filled, fillcolor=yellow]
}

Shuah Khan (5):
  media: vimc: Collapse component structure into a single monolithic
    driver
  media: vimc: Fix gpf in rmmod path when stream is active
  vimc: move duplicated IS_SRC and IS_SINK to common header
  doc: media: vimc: Update module parameter usage information
  MAINTAINERS: Add reviewer to vimc driver

 Documentation/media/v4l-drivers/vimc.rst   |  12 +-
 MAINTAINERS                                |   1 +
 drivers/media/platform/vimc/Makefile       |   7 +-
 drivers/media/platform/vimc/vimc-capture.c |  81 ++------
 drivers/media/platform/vimc/vimc-common.c  |   3 +-
 drivers/media/platform/vimc/vimc-common.h  |  58 ++++++
 drivers/media/platform/vimc/vimc-core.c    | 216 +++++++++------------
 drivers/media/platform/vimc/vimc-debayer.c |  81 ++------
 drivers/media/platform/vimc/vimc-scaler.c  |  82 ++------
 drivers/media/platform/vimc/vimc-sensor.c  |  74 ++-----
 10 files changed, 215 insertions(+), 400 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 15+ messages in thread