Hi Dafna, Thanks for working on this. On 5/22/20 4:55 AM, Dafna Hirschfeld wrote: > Hi, > This is v4 of the patchset that was sent by Helen Koike. > > Media drivers need to iterate through the pipeline and call .s_stream() > callbacks in the subdevices. > > Instead of repeating code, add helpers for this. > > These helpers will go walk through the pipeline only visiting entities > that participates in the stream, i.e. it follows links from sink to source > (and not the opposite). > For example, in a topology like this https://bit.ly/3b2MxjI > calling v4l2_pipeline_stream_enable() from rkisp1_mainpath won't call > .s_stream(true) for rkisp1_resizer_selfpath. > > stream_count variable was added in v4l2_subdevice to handle nested calls > to the helpers. > This is useful when the driver allows streaming from more then one > capture device sharing subdevices. If I understand correctly, this isn't true anymore right? Nested calls aren't possible anymore since this version doesn't contain stream_count in struct v4l2_subdevice. Documentation of v4l2_pipeline_stream_*() should also be updated. Just to be clear, without the nested call, vimc will require to add its own counters, patch https://patchwork.kernel.org/patch/10948833/ will be required again to allow multi streaming. Also, patch "media: staging: rkisp1: cap: use v4l2_pipeline_stream_{enable,disable}" is cleaner in the previous version (with stream_count in struct v4l2_subdevice). All drivers that allows multi streaming will need to implement some special handling. Adding stream_count in struct v4l2_subdevice still seems cleaner to me. I'd like to hear what others think. > > This patchset was tested on rkisp1 and vimc drivers. > > Other cleanup might be possible (but I won't add in this patchset as I > don't have the hw to test): > https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/qcom/camss/camss-video.c#n430 > https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/omap3isp/isp.c#n697 > https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/stm32/stm32-dcmi.c#n680 > https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/xilinx/xilinx-dma.c#n97 > > Testing: > -------- > > SEN_DEV=/dev/v4l-subdev3 > ISP_DEV=/dev/v4l-subdev0 > RSZ_SP_DEV=/dev/v4l-subdev2 > RSZ_MP_DEV=/dev/v4l-subdev1 > CAP_SP_DEV=/dev/video1 > CAP_MP_DEV=/dev/video0 > > WIDTH=1920 > HEIGHT=1080 > RAW_CODE=SRGGB10_1X10 > YUV_CODE=YUYV8_2X8 > > v4l2-ctl --set-subdev-fmt pad=0,width=$WIDTH,height=$HEIGHT,code=$RAW_CODE -d $SEN_DEV > > v4l2-ctl --set-subdev-fmt pad=0,width=$WIDTH,height=$HEIGHT,code=$RAW_CODE -d $ISP_DEV > v4l2-ctl --set-subdev-selection pad=0,target=crop,top=0,left=0,width=$WIDTH,height=$HEIGHT -d $ISP_DEV > > v4l2-ctl --set-subdev-selection pad=2,target=crop,top=0,left=0,width=$WIDTH,height=$HEIGHT -d $ISP_DEV > v4l2-ctl --set-subdev-fmt pad=2,width=$WIDTH,height=$HEIGHT,code=$YUV_CODE -d $ISP_DEV > > v4l2-ctl --set-subdev-fmt pad=0,width=$WIDTH,height=$HEIGHT,code=$YUV_CODE -d $RSZ_SP_DEV > v4l2-ctl --set-subdev-fmt pad=1,width=$WIDTH,height=$HEIGHT,code=$YUV_CODE -d $RSZ_SP_DEV > > v4l2-ctl --set-subdev-selection pad=0,target=crop,top=0,left=0,width=$WIDTH,height=$HEIGHT -d $RSZ_SP_DEV > > v4l2-ctl --set-subdev-fmt pad=0,width=$WIDTH,height=$HEIGHT,code=$YUV_CODE -d $RSZ_MP_DEV > v4l2-ctl --set-subdev-fmt pad=1,width=$WIDTH,height=$HEIGHT,code=$YUV_CODE -d $RSZ_MP_DEV > > v4l2-ctl --set-subdev-selection pad=0,target=crop,top=0,left=0,width=$WIDTH,height=$HEIGHT -d $RSZ_MP_DEV > > v4l2-ctl -v width=$WIDTH,height=$HEIGHT,pixelformat=NV12 -d $CAP_SP_DEV > v4l2-ctl -v width=$WIDTH,height=$HEIGHT,pixelformat=NV12 -d $CAP_MP_DEV > > sleep 1 > > v4l2-ctl --stream-mmap --stream-count=100 -d $CAP_SP_DEV --stream-to=/tmp/test_sp.raw & > v4l2-ctl --stream-mmap --stream-count=100 -d $CAP_MP_DEV --stream-to=/tmp/test_mp.raw & > > wait > echo "Completed" > > > Changes in v4: > ============== > patch 1: fix coding style issues > > patch 2: > - in function v4l2_pipeline_subdevs_get, use a local media_graph to walk on the topology so a lock is not needed > and also the pipe parameter is not needed. You are assuming that media_pipeline_start() will always be called before v4l2_pipeline_stream_enable(). I think this is fine, but it should reflect in the docs. Regards, Helen > - adding a function v4l2_subdevs_stream_disable to avoid code duplication > - change v4l2_pipeline_stream_disable to return an error code if failed > - don't add a new field to subdevice "stream_counter" when calling s_stream, since this counter is updated only in > the helper functions, and might be confusing that it is generally not an indication of the number of calls to s_stream. > Also, except of rkisp1, and vimc, it seems that the other drivers that might use the new helpers don't use a counter. > > new added - patch 3: the call to media_pipeline_start should be called before calling s_stream on the subdevices in order > to validate the links and prevents them from changing, this patch fixes it. > > patch 4: (use the helpers in rkisp1). The helpers now don't have a counter for the number of calls to s_stream, so rkisp1 > should check if the other capture is streaming and in that case call s_stream only for its resizer. > > patch 5: - (use the helpers in vimc) > - test the return value of v4l2_pipeline_stream_disable > - the call to the helerps now doesn't need the pipe parameter. > > Overview of patches in V3: > -------------------------- > > Patch 1/5 adds a new iterator function in media-controller to follow links from sink to > source only. > > Patch 2/5 adds the helpers in v4l2-common.c, > > Patch 3/5 calles media_pipeline_start before calling s_stream on the subdevices > > Patch 4/5 cleanup rkisp1 driver to use the helpers. > > Patch 5/5 cleanup vimc driver to use the helpers. > > Changes in V3: > ==================== > Following up Niklas' comments in V2 https://patchwork.kernel.org/patch/11473681/#23270823 > > * I removed the limitation in topologies with entities with multiple enabled > links to its sink pads in the topology. > Now it enables all subdevs in the pipeline that have an enabled link going > from sink to source while walking from the video device, so it can be > also useful for rcar-vin driver. > > To implement this, I added back in the series the patch from v1: > "media: mc-entity.c: add media_graph_walk_next_stream()" > > * "size" was renamed to "max_size" in function v4l2_pipeline_subdevs_get() > to reflect the maximum number of elements that can fit in the subdevs array, > with proper documentation. > > * v4l2_pipeline_subdevs_get() returns a negative number for error, instead > of returning 0 and printing a warning. > > * I also add if defined(CONFIG_MEDIA_CONTROLLER) around helpers to avoid > compiling errors. > > Overview of patches in V3: > -------------------------- > > Patch 1/4 adds a new iterator function in media-controller to follow links from sink to > source only. > > Patch 2/4 adds the helpers in v4l2-common.c, allowing nested calls by > adding stream_count in the subdevice struct. > > Patch 3/4 cleanup rkisp1 driver to use the helpers. > > Patch 4/4 cleanup vimc driver to use the helpers. > > Changes in V2: > ==================== > The first version was calling the s_stream() callbacks from sensor to > capture. > > This was generating errors in the Scarlet Chromebook, when the sensor > was being enabled before the ISP. > > It make sense to enable subdevices from capture to sensor instead (which > is what most drivers do already). > > This v2 drops the changes from mc-entity.c, and re-implement helpers in > v4l2-common.c > > Overview of patches in V2: > -------------------------- > > Path 1/3 adds the helpers in v4l2-common.c, allowing nested calls by > adding stream_count in the subdevice struct. > > Patch 2/3 cleanup rkisp1 driver to use the helpers. > > Patch 3/3 cleanup vimc driver to use the helpers. > > Dafna Hirschfeld (1): > media: staging: rkisp1: validate links before powering and streaming > > Helen Koike (4): > media: mc-entity.c: add media_graph_walk_next_stream() > media: v4l2-common: add helper functions to call s_stream() callbacks > media: staging: rkisp1: cap: use v4l2_pipeline_stream_{enable,disable} > helpers > media: vimc: use v4l2_pipeline_stream_{enable,disable} helpers > > drivers/media/mc/mc-entity.c | 34 ++++- > .../media/test-drivers/vimc/vimc-capture.c | 31 +++-- > .../media/test-drivers/vimc/vimc-streamer.c | 49 +------ > drivers/media/v4l2-core/v4l2-common.c | 99 ++++++++++++++ > drivers/staging/media/rkisp1/rkisp1-capture.c | 125 ++++++------------ > include/media/media-entity.h | 15 +++ > include/media/v4l2-common.h | 39 ++++++ > 7 files changed, 253 insertions(+), 139 deletions(-) >
