| Message ID | 20200522075522.6190-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: add v4l2_pipeline_stream_{enable,disable} | expand |
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(-) >
On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: > > 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. I certainly would see this reference counting done in generic code, because requiring every driver to do it simply adds to the endless amount of boiler plate that V4L2 currently requires from the drivers. :( I wonder if it wouldn't be possible to redesign the framework so that .s_stream() at the subdev level is only called when it's expected to either start or stop this particular subdev and driver's implementation can simply execute the requested action. Best regards, Tomasz
Hi Tomasz, On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: > On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: > > 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. > > I certainly would see this reference counting done in generic code, > because requiring every driver to do it simply adds to the endless > amount of boiler plate that V4L2 currently requires from the drivers. > :( > > I wonder if it wouldn't be possible to redesign the framework so that > .s_stream() at the subdev level is only called when it's expected to > either start or stop this particular subdev and driver's > implementation can simply execute the requested action. I'd very much like that. Note that I think a few drivers abuse the on parameter to the function to pass other values than 0 or 1. We'd have to fix those first (or maybe it has been done already, it's been a long time since I last checked).
Hi Tomasz, Helen, Laurent On 26.05.20 20:57, Laurent Pinchart wrote: > Hi Tomasz, > > On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: >> On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: >>> 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. >> >> I certainly would see this reference counting done in generic code, >> because requiring every driver to do it simply adds to the endless It is required only for drivers that support multistreaming. I don't know much about other driver except of the ones I am working on, is it a common case? >> amount of boiler plate that V4L2 currently requires from the drivers. >> :( >> >> I wonder if it wouldn't be possible to redesign the framework so that >> .s_stream() at the subdev level is only called when it's expected to >> either start or stop this particular subdev and driver's >> implementation can simply execute the requested action. You mean that a generic code similar to the helper functions in this patchset will be used for all drivers, so that drivers don't call s_stream for subdevices anymore? Anyway, this patchset just adds helper functions, it does not redesign the code. Maybe the stream_count can be updated in the v4l2_subdev_call macro ? This why it can be used not just for the helper functions. Thanks, Dafna > > I'd very much like that. Note that I think a few drivers abuse the on > parameter to the function to pass other values than 0 or 1. We'd have to > fix those first (or maybe it has been done already, it's been a long > time since I last checked). >
On Thu, May 28, 2020 at 6:21 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > Hi Tomasz, Helen, Laurent > > On 26.05.20 20:57, Laurent Pinchart wrote: > > Hi Tomasz, > > > > On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: > >> On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: > >>> 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. > >> > >> I certainly would see this reference counting done in generic code, > >> because requiring every driver to do it simply adds to the endless > > It is required only for drivers that support multistreaming. I don't know much > about other driver except of the ones I am working on, is it a common case? > I'm not very familiar with the older camera I/F drivers, but multiple streams isn't anything unusual for camera hardware. I recall the old Samsung FIMC already having support for separate preview and capture outputs. Also adding the reference counting on framework level probably wouldn't really hurt drivers which only have 1 stream anyway. > >> amount of boiler plate that V4L2 currently requires from the drivers. > >> :( > >> > >> I wonder if it wouldn't be possible to redesign the framework so that > >> .s_stream() at the subdev level is only called when it's expected to > >> either start or stop this particular subdev and driver's > >> implementation can simply execute the requested action. > > You mean that a generic code similar to the helper functions in this patchset > will be used for all drivers, so that drivers don't call s_stream for subdevices > anymore? > Anyway, this patchset just adds helper functions, it does not redesign the code. > Maybe the stream_count can be updated in the v4l2_subdev_call macro ? > This why it can be used not just for the helper functions. Sorry, just thinking out loud. Generally if we look at other kAPIs, such as the drm_crtc_helper_funcs [1] or regulator_ops [2], the driver provided implementation doesn't have to care about duplicate enable/disable requests. [1] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/drm/drm_modeset_helper_vtables.h#L61 [2] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/linux/regulator/driver.h#L144 If we could prohibit calling v4l2_subdev_ops::s_stream() by the drivers directly and instead add something like v4l2_subdev_s_stream(), the latter could do reference counting on its own and then only call v4l2_subdev_ops::s_stream() when the reference count changes between 0 and 1. One problem I see with this series is that I'm not sure if it's always guaranteed that all the drivers in the pipeline would actually use the generic helpers. If there is a driver in the pipeline which just calls v4l2_subdev_ops::s_stream() on some other subdev on its own, it wouldn't be aware of the reference count and bad things could happen (e.g. the subdev stopped despite the count being > 0). However, I'm afraid this is more of the kAPI design issue and could be require quite a significant effort to be straightened out. Best regards, Tomasz > > Thanks, > Dafna > > > > > I'd very much like that. Note that I think a few drivers abuse the on > > parameter to the function to pass other values than 0 or 1. We'd have to > > fix those first (or maybe it has been done already, it's been a long > > time since I last checked). > >
[Fixing Niklas's address.] On Fri, May 29, 2020 at 3:26 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Thu, May 28, 2020 at 6:21 PM Dafna Hirschfeld > <dafna.hirschfeld@collabora.com> wrote: > > > > Hi Tomasz, Helen, Laurent > > > > On 26.05.20 20:57, Laurent Pinchart wrote: > > > Hi Tomasz, > > > > > > On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: > > >> On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: > > >>> 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. > > >> > > >> I certainly would see this reference counting done in generic code, > > >> because requiring every driver to do it simply adds to the endless > > > > It is required only for drivers that support multistreaming. I don't know much > > about other driver except of the ones I am working on, is it a common case? > > > > I'm not very familiar with the older camera I/F drivers, but multiple > streams isn't anything unusual for camera hardware. I recall the old > Samsung FIMC already having support for separate preview and capture > outputs. > > Also adding the reference counting on framework level probably > wouldn't really hurt drivers which only have 1 stream anyway. > > > >> amount of boiler plate that V4L2 currently requires from the drivers. > > >> :( > > >> > > >> I wonder if it wouldn't be possible to redesign the framework so that > > >> .s_stream() at the subdev level is only called when it's expected to > > >> either start or stop this particular subdev and driver's > > >> implementation can simply execute the requested action. > > > > You mean that a generic code similar to the helper functions in this patchset > > will be used for all drivers, so that drivers don't call s_stream for subdevices > > anymore? > > Anyway, this patchset just adds helper functions, it does not redesign the code. > > Maybe the stream_count can be updated in the v4l2_subdev_call macro ? > > This why it can be used not just for the helper functions. > > Sorry, just thinking out loud. Generally if we look at other kAPIs, > such as the drm_crtc_helper_funcs [1] or regulator_ops [2], the driver > provided implementation doesn't have to care about duplicate > enable/disable requests. > > [1] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/drm/drm_modeset_helper_vtables.h#L61 > [2] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/linux/regulator/driver.h#L144 > > If we could prohibit calling v4l2_subdev_ops::s_stream() by the > drivers directly and instead add something like > v4l2_subdev_s_stream(), the latter could do reference counting on its > own and then only call v4l2_subdev_ops::s_stream() when the reference > count changes between 0 and 1. > > One problem I see with this series is that I'm not sure if it's always > guaranteed that all the drivers in the pipeline would actually use the > generic helpers. If there is a driver in the pipeline which just calls > v4l2_subdev_ops::s_stream() on some other subdev on its own, it > wouldn't be aware of the reference count and bad things could happen > (e.g. the subdev stopped despite the count being > 0). > > However, I'm afraid this is more of the kAPI design issue and could be > require quite a significant effort to be straightened out. > > Best regards, > Tomasz > > > > > Thanks, > > Dafna > > > > > > > > I'd very much like that. Note that I think a few drivers abuse the on > > > parameter to the function to pass other values than 0 or 1. We'd have to > > > fix those first (or maybe it has been done already, it's been a long > > > time since I last checked). > > >
Hi, On 5/29/20 10:27 AM, Tomasz Figa wrote: > [Fixing Niklas's address.] > > On Fri, May 29, 2020 at 3:26 PM Tomasz Figa <tfiga@chromium.org> wrote: >> >> On Thu, May 28, 2020 at 6:21 PM Dafna Hirschfeld >> <dafna.hirschfeld@collabora.com> wrote: >>> >>> Hi Tomasz, Helen, Laurent >>> >>> On 26.05.20 20:57, Laurent Pinchart wrote: >>>> Hi Tomasz, >>>> >>>> On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: >>>>> On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: >>>>>> 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. >>>>> >>>>> I certainly would see this reference counting done in generic code, >>>>> because requiring every driver to do it simply adds to the endless >>> >>> It is required only for drivers that support multistreaming. I don't know much >>> about other driver except of the ones I am working on, is it a common case? >>> >> >> I'm not very familiar with the older camera I/F drivers, but multiple >> streams isn't anything unusual for camera hardware. I recall the old >> Samsung FIMC already having support for separate preview and capture >> outputs. >> >> Also adding the reference counting on framework level probably >> wouldn't really hurt drivers which only have 1 stream anyway. >> >>>>> amount of boiler plate that V4L2 currently requires from the drivers. >>>>> :( >>>>> >>>>> I wonder if it wouldn't be possible to redesign the framework so that >>>>> .s_stream() at the subdev level is only called when it's expected to >>>>> either start or stop this particular subdev and driver's >>>>> implementation can simply execute the requested action. >>> >>> You mean that a generic code similar to the helper functions in this patchset >>> will be used for all drivers, so that drivers don't call s_stream for subdevices >>> anymore? >>> Anyway, this patchset just adds helper functions, it does not redesign the code. >>> Maybe the stream_count can be updated in the v4l2_subdev_call macro ? >>> This why it can be used not just for the helper functions. >> >> Sorry, just thinking out loud. Generally if we look at other kAPIs, >> such as the drm_crtc_helper_funcs [1] or regulator_ops [2], the driver >> provided implementation doesn't have to care about duplicate >> enable/disable requests. Thanks for this pointer. >> >> [1] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/drm/drm_modeset_helper_vtables.h#L61 >> [2] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/linux/regulator/driver.h#L144 >> >> If we could prohibit calling v4l2_subdev_ops::s_stream() by the >> drivers directly and instead add something like >> v4l2_subdev_s_stream(), the latter could do reference counting on its >> own and then only call v4l2_subdev_ops::s_stream() when the reference >> count changes between 0 and 1. This is basically how v3 was implemented https://patchwork.kernel.org/patch/11489583/ And the main concern (from what I understood) was to add a stream_count under struct v4l2_subdev, that is only touched by the helpers, so this stream_count field would be useless for drivers not using the helpers. which, imho, it is not a big problem. I think we gain more with a common implementation. >> >> One problem I see with this series is that I'm not sure if it's always >> guaranteed that all the drivers in the pipeline would actually use the >> generic helpers. I'm not sure we should always guarantee usage of generic helpers, since drivers may want to initialize subdevices in a specific order. >> If there is a driver in the pipeline which just calls >> v4l2_subdev_ops::s_stream() on some other subdev on its own, it >> wouldn't be aware of the reference count and bad things could happen >> (e.g. the subdev stopped despite the count being > 0). I don't think this is a problem, since v4l2_subdev_ops::s_stream() are usually triggered by a STREAM_ON on a video node. So or the video node driver uses the helpers, or it calls v4l2_subdev_ops::s_stream() on subdevices directly. Unless if, we could have a case where we have multiple video nodes in the same topology that is implemented by different drivers, which seems odd to me. Regards, Helen >> >> However, I'm afraid this is more of the kAPI design issue and could be >> require quite a significant effort to be straightened out. >> >> Best regards, >> Tomasz >> >>> >>> Thanks, >>> Dafna >>> >>>> >>>> I'd very much like that. Note that I think a few drivers abuse the on >>>> parameter to the function to pass other values than 0 or 1. We'd have to >>>> fix those first (or maybe it has been done already, it's been a long >>>> time since I last checked). >>>>
On Fri, May 29, 2020 at 3:49 PM Helen Koike <helen.koike@collabora.com> wrote: > > Hi, > > On 5/29/20 10:27 AM, Tomasz Figa wrote: > > [Fixing Niklas's address.] > > > > On Fri, May 29, 2020 at 3:26 PM Tomasz Figa <tfiga@chromium.org> wrote: > >> > >> On Thu, May 28, 2020 at 6:21 PM Dafna Hirschfeld > >> <dafna.hirschfeld@collabora.com> wrote: > >>> > >>> Hi Tomasz, Helen, Laurent > >>> > >>> On 26.05.20 20:57, Laurent Pinchart wrote: > >>>> Hi Tomasz, > >>>> > >>>> On Tue, May 26, 2020 at 06:11:00PM +0200, Tomasz Figa wrote: > >>>>> On Fri, May 22, 2020 at 11:06 AM Helen Koike <helen.koike@collabora.com> wrote: > >>>>>> 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. > >>>>> > >>>>> I certainly would see this reference counting done in generic code, > >>>>> because requiring every driver to do it simply adds to the endless > >>> > >>> It is required only for drivers that support multistreaming. I don't know much > >>> about other driver except of the ones I am working on, is it a common case? > >>> > >> > >> I'm not very familiar with the older camera I/F drivers, but multiple > >> streams isn't anything unusual for camera hardware. I recall the old > >> Samsung FIMC already having support for separate preview and capture > >> outputs. > >> > >> Also adding the reference counting on framework level probably > >> wouldn't really hurt drivers which only have 1 stream anyway. > >> > >>>>> amount of boiler plate that V4L2 currently requires from the drivers. > >>>>> :( > >>>>> > >>>>> I wonder if it wouldn't be possible to redesign the framework so that > >>>>> .s_stream() at the subdev level is only called when it's expected to > >>>>> either start or stop this particular subdev and driver's > >>>>> implementation can simply execute the requested action. > >>> > >>> You mean that a generic code similar to the helper functions in this patchset > >>> will be used for all drivers, so that drivers don't call s_stream for subdevices > >>> anymore? > >>> Anyway, this patchset just adds helper functions, it does not redesign the code. > >>> Maybe the stream_count can be updated in the v4l2_subdev_call macro ? > >>> This why it can be used not just for the helper functions. > >> > >> Sorry, just thinking out loud. Generally if we look at other kAPIs, > >> such as the drm_crtc_helper_funcs [1] or regulator_ops [2], the driver > >> provided implementation doesn't have to care about duplicate > >> enable/disable requests. > > Thanks for this pointer. > > >> > >> [1] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/drm/drm_modeset_helper_vtables.h#L61 > >> [2] https://elixir.bootlin.com/linux/v5.7-rc7/source/include/linux/regulator/driver.h#L144 > >> > >> If we could prohibit calling v4l2_subdev_ops::s_stream() by the > >> drivers directly and instead add something like > >> v4l2_subdev_s_stream(), the latter could do reference counting on its > >> own and then only call v4l2_subdev_ops::s_stream() when the reference > >> count changes between 0 and 1. > > This is basically how v3 was implemented https://patchwork.kernel.org/patch/11489583/ Not exactly. There are 2 separate problems being addressed here: 1) Iterating over the pipeline and starting streaming on all the entities, 2) Preventing duplicate calls to s_stream(). v3 attempted to address problem 2) by the way of addressing problem 1). However problem 2) can also occur on its own, outside of the pipeline start/stop, because s_stream is a standalone subdev operation, exposed both to the userspace via UAPI and to the drivers via the pseudo-kAPI (v4l2_subdev_call()). If 2) was solved on the level of the kAPI, i.e. removing v4l2_subdev_call() and replacing it with dedicated functions for operations like s_stream, refcounting could be implemented inside of such functions, without the need to do it in the pipeline iteration helpers. > > And the main concern (from what I understood) was to add a stream_count > under struct v4l2_subdev, that is only touched by the helpers, so this > stream_count field would be useless for drivers not using the helpers. > which, imho, it is not a big problem. I believe that's exactly because the two problems I mentioned above are actually separate problems and a solution for 1) can't solve 2) fully. > > I think we gain more with a common implementation. > > >> > >> One problem I see with this series is that I'm not sure if it's always > >> guaranteed that all the drivers in the pipeline would actually use the > >> generic helpers. > > I'm not sure we should always guarantee usage of generic helpers, since > drivers may want to initialize subdevices in a specific order. > If we treat the 2 problems separately, the helpers for 1) could be still optional, but the new calls for 2) would be mandatory. > >> If there is a driver in the pipeline which just calls > >> v4l2_subdev_ops::s_stream() on some other subdev on its own, it > >> wouldn't be aware of the reference count and bad things could happen > >> (e.g. the subdev stopped despite the count being > 0). > > I don't think this is a problem, since v4l2_subdev_ops::s_stream() are > usually triggered by a STREAM_ON on a video node. So or the video node driver > uses the helpers, or it calls v4l2_subdev_ops::s_stream() on subdevices directly. > That's not a kAPI guarantee. Any driver is free to call v4l2_subdev_call(..., s_stream) whenever it wants. > Unless if, we could have a case where we have multiple video nodes in the > same topology that is implemented by different drivers, which seems odd > to me. That's not the only case. There are some devices, such as the adv748x CSI-2 transmitter which manage the rest of the pipeline on their own, e.g. https://elixir.bootlin.com/linux/v5.7-rc7/source/drivers/media/i2c/adv748x/adv748x-csi2.c#L116 In this case, the generic helpers invoked by the ISP driver would compete with the explicit configuration done by the driver. However, if we solve the problem 2) at the kAPI level, that wouldn't be an issue anymore, because the generic helpers and explicit calls would simply grab additional reference counts. Best regards, Tomasz > > Regards, > Helen > > >> > >> However, I'm afraid this is more of the kAPI design issue and could be > >> require quite a significant effort to be straightened out. > >> > >> Best regards, > >> Tomasz > >> > >>> > >>> Thanks, > >>> Dafna > >>> > >>>> > >>>> I'd very much like that. Note that I think a few drivers abuse the on > >>>> parameter to the function to pass other values than 0 or 1. We'd have to > >>>> fix those first (or maybe it has been done already, it's been a long > >>>> time since I last checked). > >>>>
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. 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. - 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(-)