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

[Tomi Valkeinen]

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 <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >     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