Unnamed repository; edit this file 'description' to name the repository.
 help / color / mirror / Atom feed
From: Helen Koike <helen.koike@collabora.com>
To: Tomasz Figa <tfiga@chromium.org>,
	Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Ezequiel Garcia" <ezequiel@collabora.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	kernel@collabora.com, "Dafna Hirschfeld" <dafna3@gmail.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"open list:ARM/Rockchip SoC..."
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>
Subject: Re: [PATCH v4 0/5] media: add v4l2_pipeline_stream_{enable,disable}
Date: Fri, 29 May 2020 10:49:04 -0300
Message-ID: <7ce3e2cb-60ab-bf0f-6d0a-7bcf0c73d6b8@collabora.com> (raw)
In-Reply-To: <CAAFQd5Bcu+OP-2Uwkrq79C+0WAvFox-gxmcqYqkB6gpEPoABrw@mail.gmail.com>


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.


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

  reply	other threads:[~2020-05-29 13:49 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22  7:55 Dafna Hirschfeld
2020-05-22  7:55 ` [PATCH v4 1/5] media: mc-entity.c: add media_graph_walk_next_stream() Dafna Hirschfeld
2020-05-22  7:55 ` [PATCH v4 2/5] media: v4l2-common: add helper functions to call s_stream() callbacks Dafna Hirschfeld
2020-05-25  9:23   ` Sakari Ailus
2020-05-25  9:42     ` Dafna Hirschfeld
2020-05-25 10:03       ` Sakari Ailus
2020-05-25 10:45         ` Dafna Hirschfeld
2020-06-22  9:20           ` Sakari Ailus
2020-06-22  9:00   ` Hans Verkuil
2020-06-22 14:07     ` Dafna Hirschfeld
2020-05-22  7:55 ` [PATCH v4 3/5] media: staging: rkisp1: validate links before powering and streaming Dafna Hirschfeld
2020-06-10 17:00   ` Tomasz Figa
2020-05-22  7:55 ` [PATCH v4 4/5] media: staging: rkisp1: cap: use v4l2_pipeline_stream_{enable,disable} helpers Dafna Hirschfeld
2020-06-10 17:03   ` Tomasz Figa
2020-06-10 17:22     ` Dafna Hirschfeld
2020-06-10 17:28       ` Tomasz Figa
2020-05-22  7:55 ` [PATCH v4 5/5] media: vimc: " Dafna Hirschfeld
2020-05-22  9:06 ` [PATCH v4 0/5] media: add v4l2_pipeline_stream_{enable,disable} Helen Koike
2020-05-26 16:11   ` Tomasz Figa
2020-05-26 18:57     ` Laurent Pinchart
2020-05-28 16:21       ` Dafna Hirschfeld
2020-05-29 13:26         ` Tomasz Figa
2020-05-29 13:27           ` Tomasz Figa
2020-05-29 13:49             ` Helen Koike [this message]
2020-05-29 14:48               ` Tomasz Figa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7ce3e2cb-60ab-bf0f-6d0a-7bcf0c73d6b8@collabora.com \
    --to=helen.koike@collabora.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=dafna3@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=niklas.soderlund@ragnatech.se \
    --cc=sakari.ailus@linux.intel.com \
    --cc=skhan@linuxfoundation.org \
    --cc=tfiga@chromium.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Unnamed repository; edit this file 'description' to name the repository.

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://archive.lwn.net:8080/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ http://archive.lwn.net:8080/linux-media \
		linux-media@vger.kernel.org lwn-linux-media@archive.lwn.net
	public-inbox-index linux-media

Example config snippet for mirrors.
Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git