Received-SPF: pass (google.com: domain of linux-kernel+bounces-117080-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 25 Mar 2024 13:02:08 +0000
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Umang Jain <umang.jain@ideasonboard.com>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for
 single-pad subdevs
Message-ID: <ZgF10EVLrfF7cl57@kekkonen.localdomain>
References: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>
 <20240325125055.GC23988@pendragon.ideasonboard.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240325125055.GC23988@pendragon.ideasonboard.com>

Moi,

Thanks for the patch.

On Mon, Mar 25, 2024 at 02:50:55PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Mar 25, 2024 at 02:43:23PM +0200, Tomi Valkeinen wrote:
> > Currently a subdevice with a single pad, e.g. a sensor subdevice, must
> > use the v4l2_subdev_video_ops.s_stream op, instead of
> > v4l2_subdev_pad_ops.enable/disable_streams. This is because the
> > enable/disable_streams machinery requires a routing table which a subdev
> > cannot have with a single pad.
> > 
> > Implement enable/disable_streams support for these single-pad subdevices
> > by assuming an implicit stream 0 when the subdevice has only one pad.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> > Even though I did send this patch, I'm not sure if this is necessary.
> > s_stream works fine for the subdevs with a single pad. With the upcoming
> > internal pads, adding an internal pad to the subdev will create a
> > routing table, and enable/disable_streams would get "fixed" that way.

I'd like to get rid of a redundant way to control streaming.

> > 
> > So perhaps the question is, do we want to support single-pad subdevs in
> > the future, in which case something like this patch is necessary, or
> > will all modern source subdev drivers have internal pads, in which
> > case this is not needed...
> 
> I think the latter would be best. I however can't guarantee we won't
> have valid use cases for (enable|disable)_streams on single-pad subdevs
> though, so you patch could still be interesting.

Instead of the number of pads, could we use instead the
V4L2_SUBDEV_FL_STREAMS flag or whether g_routing op is supported to
determine the need for this?

> 
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 105 ++++++++++++++++++++++------------
> >  include/media/v4l2-subdev.h           |   4 +-
> >  2 files changed, 72 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 4c6198c48dd6..ddc7ed69421c 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -2129,21 +2129,33 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >  	 * Verify that the requested streams exist and that they are not
> >  	 * already enabled.
> >  	 */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > -			continue;
> > -
> > -		found_streams |= BIT_ULL(cfg->stream);
> > -
> > -		if (cfg->enabled) {
> > +	if (sd->entity.num_pads == 1) {
> > +		if (sd->enabled_streams) {
> >  			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > +				0, sd->entity.name, pad);
> >  			ret = -EALREADY;
> >  			goto done;
> >  		}
> > +
> > +		found_streams = BIT_ULL(0);
> > +	} else {
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> > +
> > +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > +				continue;
> > +
> > +			found_streams |= BIT_ULL(cfg->stream);
> > +
> > +			if (cfg->enabled) {
> > +				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> > +					cfg->stream, sd->entity.name, pad);
> > +				ret = -EALREADY;
> > +				goto done;
> > +			}
> > +		}
> >  	}
> >  
> >  	if (found_streams != streams_mask) {
> > @@ -2164,13 +2176,17 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >  		goto done;
> >  	}
> >  
> > -	/* Mark the streams as enabled. */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > +	if (sd->entity.num_pads == 1) {
> > +		sd->enabled_streams |= streams_mask;
> > +	} else {
> > +		/* Mark the streams as enabled. */
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = true;
> > +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > +				cfg->enabled = true;
> > +		}
> >  	}
> >  
> >  done:
> > @@ -2246,21 +2262,32 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >  	 * Verify that the requested streams exist and that they are not
> >  	 * already disabled.
> >  	 */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > -
> > -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > -			continue;
> > -
> > -		found_streams |= BIT_ULL(cfg->stream);
> > -
> > -		if (!cfg->enabled) {
> > +	if (sd->entity.num_pads == 1) {
> > +		if (!sd->enabled_streams) {
> >  			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > +				0, sd->entity.name, pad);
> >  			ret = -EALREADY;
> >  			goto done;
> >  		}
> > +
> > +		found_streams = BIT_ULL(0);
> > +	} else {
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> > +
> > +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > +				continue;
> > +
> > +			found_streams |= BIT_ULL(cfg->stream);
> > +
> > +			if (!cfg->enabled) {
> > +				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> > +					cfg->stream, sd->entity.name, pad);
> > +				ret = -EALREADY;
> > +				goto done;
> > +			}
> > +		}
> >  	}
> >  
> >  	if (found_streams != streams_mask) {
> > @@ -2281,13 +2308,17 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >  		goto done;
> >  	}
> >  
> > -	/* Mark the streams as disabled. */
> > -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > -		struct v4l2_subdev_stream_config *cfg =
> > -			&state->stream_configs.configs[i];
> > +	if (sd->entity.num_pads == 1) {
> > +		sd->enabled_streams &= ~streams_mask;
> > +	} else {
> > +		/* Mark the streams as disabled. */
> > +		for (i = 0; i < state->stream_configs.num_configs; ++i) {
> > +			struct v4l2_subdev_stream_config *cfg =
> > +				&state->stream_configs.configs[i];
> >  
> > -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = false;
> > +			if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > +				cfg->enabled = false;
> > +		}
> >  	}
> >  
> >  done:
> > @@ -2325,8 +2356,12 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> >  	 */
> >  	state = v4l2_subdev_lock_and_get_active_state(sd);
> >  
> > -	for_each_active_route(&state->routing, route)
> > -		source_mask |= BIT_ULL(route->source_stream);
> > +	if (sd->entity.num_pads == 1) {
> > +		source_mask = BIT_ULL(0);
> > +	} else {
> > +		for_each_active_route(&state->routing, route)
> > +			source_mask |= BIT_ULL(route->source_stream);
> > +	}
> >  
> >  	v4l2_subdev_unlock_state(state);
> >  
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a9e6b8146279..39b230f7b3c8 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1041,8 +1041,8 @@ struct v4l2_subdev_platform_data {
> >   *		  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.
> > + *		     v4l2_subdev_disable_streams() helper functions. This is
> > + *		     for fallback cases and for subdevs with single pads.
> >   *
> >   * Each instance of a subdev driver should create this struct, either
> >   * stand-alone or embedded in a larger struct.
> > 
> > ---
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> > change-id: 20240325-single-pad-enable-streams-32a9a746ac5b
> 

-- 
Terveisin,

Sakari Ailus