by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH 3/4] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback

On Thu, Apr 04, 2024 at 04:47:41PM +0300, Tomi Valkeinen wrote:
> On 04/04/2024 16:06, Laurent Pinchart wrote:
> > On Thu, Apr 04, 2024 at 03:38:45PM +0300, Tomi Valkeinen wrote:
> >> On 04/04/2024 15:18, Sakari Ailus wrote:
> >>> On Thu, Apr 04, 2024 at 01:50:02PM +0300, Tomi Valkeinen wrote:
> >>>> v4l2_subdev_enable/disable_streams_fallback() supports falling back to
> >>>> .s_stream() for subdevs with a single source pad. It also tracks the
> >>>> enabled streams for that one pad in the sd->enabled_streams field.
> >>>>
> >>>> Tracking the enabled streams with sd->enabled_streams does not make
> >>>> sense, as with .s_stream() there can only be a single stream per pad.
> >>>> Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports
> >>>> a single source pad, all we really need is a boolean which tells whether
> >>>> streaming has been enabled on this pad or not.
> >>>>
> >>>> However, as we only need a true/false state for a pad (instead of
> >>>> tracking which streams have been enabled for a pad), we can easily
> >>>> extend the fallback mechanism to support multiple source pads as we only
> >>>> need to keep track of which pads have been enabled.
> >>>>
> >>>> Change the sd->enabled_streams field to sd->enabled_pads, which is a
> >>>> 64-bit bitmask tracking the enabled source pads. With this change we can
> >>>> remove the restriction that
> >>>> v4l2_subdev_enable/disable_streams_fallback() only supports a single
> >>>> soruce pad.
> >
> > s/soruce/source/
> >
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <[email protected]>
> >>>> ---
> >>>> drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++---------------
> >>>> include/media/v4l2-subdev.h | 9 +++--
> >>>> 2 files changed, 44 insertions(+), 33 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> index 3b3310bce5d4..91298bb84e6b 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> @@ -2090,37 +2090,43 @@ static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
> >>>> u64 streams_mask)
> >>>> {
> >>>> struct device *dev = sd->entity.graph_obj.mdev->dev;
> >>>> - unsigned int i;
> >>>> int ret;
> >>>>
> >>>> /*
> >>>> * The subdev doesn't implement pad-based stream enable, fall back
> >>>> - * on the .s_stream() operation. This can only be done for subdevs that
> >>>> - * have a single source pad, as sd->enabled_streams is global to the
> >>>> - * subdev.
> >>>> + * on the .s_stream() operation.
> >
> > While at it, s/on/to/
>
> Actually, now that we support multiple pads here... Should the comment
> and the if below, checking whether the pad is a source pad, be removed?
> Is there any reason to require a source pad here (but not in the
> non-fallback case)?

Mostly the lack of test platforms where we handle stream start/stop from
source to sink, calling the operations on sink pads. I'm sure there will
be unforeseen issues :-)

> > Same below.
> >
> >>>> */
> >>>> if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>>> return -EOPNOTSUPP;
> >>>>
> >>>> - for (i = 0; i < sd->entity.num_pads; ++i) {
> >>>> - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >>>> - return -EOPNOTSUPP;
> >>>> - }
> >>>> + /*
> >>>> + * .s_stream() means there is no streams support, so only allowed stream
> >>>> + * is the imlicit stream 0.
> >
> > s/imlicit/implicit/
> >
> > Same below.
> >
> >>>> + */
> >>>> + if (streams_mask != BIT_ULL(0))
> >>>> + return -EOPNOTSUPP;
> >>>> +
> >>>> + /*
> >>>> + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >>>> + * with 64 pads or less can be supported.
> >>>> + */
> >>>> + if (pad >= sizeof(sd->enabled_pads) * 8)
> >>>
> >>> s/8/BITS_PER_BYTE/
> >>>
> >>>> + return -EOPNOTSUPP;
> >>>>
> >>>> - if (sd->enabled_streams & streams_mask) {
> >>>> - dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
> >>>> - streams_mask, sd->entity.name, pad);
> >>>> + if (sd->enabled_pads & BIT_ULL(pad)) {
> >>>> + dev_dbg(dev, "pad %u already enabled on %s\n",
> >>>> + pad, sd->entity.name);
> >>>> return -EALREADY;
> >>>> }
> >>>>
> >>>> - /* Start streaming when the first streams are enabled. */
> >>>> - if (!sd->enabled_streams) {
> >>>> + /* Start streaming when the first pad is enabled. */
> >>>> + if (!sd->enabled_pads) {
> >>>> ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >>>> if (ret)
> >>>> return ret;
> >>>> }
> >>>>
> >>>> - sd->enabled_streams |= streams_mask;
> >>>> + sd->enabled_pads |= BIT_ULL(pad);
> >>>>
> >>>> return 0;
> >>>> }
> >>>> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
> >>>> u64 streams_mask)
> >>>> {
> >>>> struct device *dev = sd->entity.graph_obj.mdev->dev;
> >>>> - unsigned int i;
> >>>> int ret;
> >>>>
> >>>> /*
> >>>> - * If the subdev doesn't implement pad-based stream enable, fall back
> >>>> - * on the .s_stream() operation. This can only be done for subdevs that
> >>>> - * have a single source pad, as sd->enabled_streams is global to the
> >>>> - * subdev.
> >>>> + * If the subdev doesn't implement pad-based stream enable, fall back
> >>>> + * on the .s_stream() operation.
> >>>> */
> >>>> if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>>> return -EOPNOTSUPP;
> >>>>
> >>>> - for (i = 0; i < sd->entity.num_pads; ++i) {
> >>>> - if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >>>> - return -EOPNOTSUPP;
> >>>> - }
> >>>> + /*
> >>>> + * .s_stream() means there is no streams support, so only allowed stream
> >>>> + * is the imlicit stream 0.
> >>>> + */
> >>>> + if (streams_mask != BIT_ULL(0))
> >>>> + return -EOPNOTSUPP;
> >>>> +
> >>>> + /*
> >>>> + * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >>>> + * with 64 pads or less can be supported.
> >>>> + */
> >>>> + if (pad >= sizeof(sd->enabled_pads) * 8)
> >>>
> >>> Ditto.
> >>>
> >>> How much of the code of the two functions is the same? Could some of this
> >>> be put to a common function both would call? They look (almost) exactly the
> >>> same.
> >>
> >> v4l2_subdev_enable_streams_fallback and v4l2_subdev_enable_streams? They
> >> have similar parts, but I don't right away see how combining them or
> >> making some common functions would help.
> >
> > You could move the three checks to a
> > v4l2_subdev_streams_fallback_check() function (bikeshedding the name is
> > allowed).
> >
> >>>> + return -EOPNOTSUPP;
> >>>>
> >>>> - if ((sd->enabled_streams & streams_mask) != streams_mask) {
> >>>> - dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
> >>>> - streams_mask, sd->entity.name, pad);
> >>>> + if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
> >
> > if (!(sd->enabled_pads & BIT_ULL(pad))) {
> >
> >>>> + dev_dbg(dev, "pad %u already disabled on %s\n",
> >>>> + pad, sd->entity.name);
> >>>> return -EALREADY;
> >>>> }
> >>>>
> >>>> /* Stop streaming when the last streams are disabled. */
> >>>> - if (!(sd->enabled_streams & ~streams_mask)) {
> >>>> + if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
> >>>> ret = v4l2_subdev_call(sd, video, s_stream, 0);
> >>>> if (ret)
> >>>> return ret;
> >>>> }
> >>>>
> >>>> - sd->enabled_streams &= ~streams_mask;
> >>>> + sd->enabled_pads &= ~BIT_ULL(pad);
> >>>>
> >>>> return 0;
> >>>> }
> >>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>> index 8bd1e3c96d2b..7077aec3176c 100644
> >>>> --- a/include/media/v4l2-subdev.h
> >>>> +++ b/include/media/v4l2-subdev.h
> >>>> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
> >>>> * @active_state: Active state for the subdev (NULL for subdevs tracking the
> >>>> * state internally). Initialized by calling
> >>>> * v4l2_subdev_init_finalize().
> >>>> - * @enabled_streams: Bitmask of enabled streams used by
> >>>> - * v4l2_subdev_enable_streams() and
> >>>> - * v4l2_subdev_disable_streams() helper functions for fallback
> >>>> - * cases.
> >>>> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
> >>>> + * and v4l2_subdev_disable_streams() helper functions for
> >>>> + * fallback cases.
> >>>> * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
> >>>> *
> >>>> * Each instance of a subdev driver should create this struct, either
> >>>> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
> >>>> * doesn't support it.
> >>>> */
> >>>> struct v4l2_subdev_state *active_state;
> >>>> - u64 enabled_streams;
> >>>> + u64 enabled_pads;
> >>>> bool streaming_enabled;
> >>>> };
> >>>>

--
Regards,

Laurent Pinchart

2024-04-05 09:14:25

by Tomi Valkeinen

[permalink] [raw]

Subject: Re: [PATCH 3/4] media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback

On 04/04/2024 17:25, Laurent Pinchart wrote:
> On Thu, Apr 04, 2024 at 04:47:41PM +0300, Tomi Valkeinen wrote:
>> On 04/04/2024 16:06, Laurent Pinchart wrote:
>>> On Thu, Apr 04, 2024 at 03:38:45PM +0300, Tomi Valkeinen wrote:
>>>> On 04/04/2024 15:18, Sakari Ailus wrote:
>>>>> On Thu, Apr 04, 2024 at 01:50:02PM +0300, Tomi Valkeinen wrote:
>>>>>> v4l2_subdev_enable/disable_streams_fallback() supports falling back to
>>>>>> .s_stream() for subdevs with a single source pad. It also tracks the
>>>>>> enabled streams for that one pad in the sd->enabled_streams field.
>>>>>>
>>>>>> Tracking the enabled streams with sd->enabled_streams does not make
>>>>>> sense, as with .s_stream() there can only be a single stream per pad.
>>>>>> Thus, as the v4l2_subdev_enable/disable_streams_fallback() only supports
>>>>>> a single source pad, all we really need is a boolean which tells whether
>>>>>> streaming has been enabled on this pad or not.
>>>>>>
>>>>>> However, as we only need a true/false state for a pad (instead of
>>>>>> tracking which streams have been enabled for a pad), we can easily
>>>>>> extend the fallback mechanism to support multiple source pads as we only
>>>>>> need to keep track of which pads have been enabled.
>>>>>>
>>>>>> Change the sd->enabled_streams field to sd->enabled_pads, which is a
>>>>>> 64-bit bitmask tracking the enabled source pads. With this change we can
>>>>>> remove the restriction that
>>>>>> v4l2_subdev_enable/disable_streams_fallback() only supports a single
>>>>>> soruce pad.
>>>
>>> s/soruce/source/
>>>
>>>>>>
>>>>>> Signed-off-by: Tomi Valkeinen <[email protected]>
>>>>>> ---
>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 68 ++++++++++++++++++++---------------
>>>>>> include/media/v4l2-subdev.h | 9 +++--
>>>>>> 2 files changed, 44 insertions(+), 33 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> index 3b3310bce5d4..91298bb84e6b 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -2090,37 +2090,43 @@ static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
>>>>>> u64 streams_mask)
>>>>>> {
>>>>>> struct device *dev = sd->entity.graph_obj.mdev->dev;
>>>>>> - unsigned int i;
>>>>>> int ret;
>>>>>>
>>>>>> /*
>>>>>> * The subdev doesn't implement pad-based stream enable, fall back
>>>>>> - * on the .s_stream() operation. This can only be done for subdevs that
>>>>>> - * have a single source pad, as sd->enabled_streams is global to the
>>>>>> - * subdev.
>>>>>> + * on the .s_stream() operation.
>>>
>>> While at it, s/on/to/
>>
>> Actually, now that we support multiple pads here... Should the comment
>> and the if below, checking whether the pad is a source pad, be removed?
>> Is there any reason to require a source pad here (but not in the
>> non-fallback case)?
>
> Mostly the lack of test platforms where we handle stream start/stop from
> source to sink, calling the operations on sink pads. I'm sure there will
> be unforeseen issues :-)

Have we tested that for the full streams version?

In the v2 I'll send shortly I have extended this test to cover also the
full streams version. We can discuss there if this test is ok, or should
it be dropped or only limited to the fallback case.

Tomi