Received-SPF: pass (google.com: domain of linux-kernel+bounces-117061-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 25 Mar 2024 14:50:55 +0200
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	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: <20240325125055.GC23988@pendragon.ideasonboard.com>
References: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>

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

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

-- 
Regards,

Laurent Pinchart