Received-SPF: pass (google.com: domain of linux-kernel+bounces-131488-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: Thu, 4 Apr 2024 16:08:13 +0300
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 4/4] media: subdev: Support enable/disable_streams for
 non-streams subdevs
Message-ID: <20240404130813.GS23803@pendragon.ideasonboard.com>
References: <20240404-enable-streams-impro-v1-0-1017a35bbe07@ideasonboard.com>
 <20240404-enable-streams-impro-v1-4-1017a35bbe07@ideasonboard.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240404-enable-streams-impro-v1-4-1017a35bbe07@ideasonboard.com>

Hi Tomi,

Thank you for the patch.

On Thu, Apr 04, 2024 at 01:50:03PM +0300, Tomi Valkeinen wrote:
> Currently a subdevice without streams support (V4L2_SUBDEV_FL_STREAMS),
> e.g. a sensor subdevice with a single source pad, 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 subdevs without streams
> support. As they don't support multiple streams, each pad must contain a
> single stream, with stream ID 0, and rather than tracking enabled
> streams, we can track enabled pads similarly to the
> enable/disable_streams_fallback by using the sd->enabled_pads field.

Could you explain in the commit message why this is desired ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 86 +++++++++++++++++++++++------------
>  1 file changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 91298bb84e6b..d86f930be2c4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2158,21 +2158,32 @@ 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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
> +		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;
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
>  
> -		found_streams |= BIT_ULL(cfg->stream);
> +			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);
> +			if (cfg->enabled) {
> +				dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
> +	} else {
> +		if (sd->enabled_pads & BIT_ULL(pad)) {
> +			dev_dbg(dev, "stream 0 already enabled on %s:%u\n",
> +				sd->entity.name, pad);
>  			ret = -EALREADY;
>  			goto done;
>  		}
> +
> +		found_streams = BIT_ULL(0);
>  	}
>  
>  	if (found_streams != streams_mask) {
> @@ -2194,12 +2205,16 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  	}
>  
>  	/* 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->flags & V4L2_SUBDEV_FL_STREAMS) {
> +		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;
> +		}
> +	} else {
> +		sd->enabled_pads |= BIT_ULL(pad);
>  	}
>  
>  done:
> @@ -2281,21 +2296,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 (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
> +		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;
> +			if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> +				continue;
>  
> -		found_streams |= BIT_ULL(cfg->stream);
> +			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);
> +			if (!cfg->enabled) {
> +				dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> +					cfg->stream, sd->entity.name, pad);
> +				ret = -EALREADY;
> +				goto done;
> +			}
> +		}
> +	} else {
> +		if (!(sd->enabled_pads & BIT_ULL(pad))) {
> +			dev_dbg(dev, "stream 0 already disabled on %s:%u\n",
> +				sd->entity.name, pad);
>  			ret = -EALREADY;
>  			goto done;
>  		}
> +
> +		found_streams = BIT_ULL(0);
>  	}
>  
>  	if (found_streams != streams_mask) {
> @@ -2317,12 +2343,16 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  	}
>  
>  	/* 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->flags & V4L2_SUBDEV_FL_STREAMS) {
> +		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;
> +		}
> +	} else {
> +		sd->enabled_pads &= ~BIT_ULL(pad);
>  	}
>  
>  done:

-- 
Regards,

Laurent Pinchart