Received-SPF: pass (google.com: domain of linux-kernel+bounces-120358-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f463dbb0-611d-4d86-b35d-bc19410c436a@ideasonboard.com>
Date: Wed, 27 Mar 2024 11:40:34 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] media: v4l2-subdev: Support enable/disable_streams for
 single-pad subdevs
Content-Language: en-US
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
 Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil
 <hverkuil@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
 Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>
From: Umang Jain <umang.jain@ideasonboard.com>
In-Reply-To: <20240325-single-pad-enable-streams-v1-1-142e19896a72@ideasonboard.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi Tomi,

On 25/03/24 6:13 pm, 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>

fwiw,

Tested-by: Umang Jain <umang.jain@ideasonboard.com>

with [1]

[1]: 
https://lore.kernel.org/linux-media/4bb01eb0-bf53-43f2-a488-7959aadacc3b@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...
> ---
>   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
>
> Best regards,