[PATCH 0/4] media: subdev: Improve stream enable/disable machinery

[PATCH 0/4] media: subdev: Improve stream enable/disable machinery

[Tomi Valkeinen]

This series works on the .s_stream, .enable_streams, .disable_streams
related code:

"media: subdev: Add checks for subdev features" adds check to verify
that the subdevs implement the proper mix of the above ops.

"media: subdev: Fix use of sd->enabled_streams in call_s_stream()" fixes
somewhat questionable use of sd->enabled_streams in call_s_stream().

"media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback"
drops the restriction of supporting only a single source pads, and
prepares the way for the following patch.

"media: subdev: Support enable/disable_streams for non-streams subdevs"
makes it possible for non-streams subdevs to use .enable_streams and
.disable_streams, deprecating the earlier patch "[PATCH] media:
v4l2-subdev: Support enable/disable_streams for single-pad subdevs".

I have tested this on RPi5 with multiple streams, but I still need to
figure out ways to test the other scenarios.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (4):
      media: subdev: Add checks for subdev features
      media: subdev: Fix use of sd->enabled_streams in call_s_stream()
      media: subdev: Improve v4l2_subdev_enable/disable_streams_fallback
      media: subdev: Support enable/disable_streams for non-streams subdevs

 drivers/media/v4l2-core/v4l2-subdev.c | 189 +++++++++++++++++++++++-----------
 include/media/v4l2-subdev.h           |  11 +-
 2 files changed, 133 insertions(+), 67 deletions(-)
---
base-commit: b82779648dfd3814df4e381f086326ec70fd791f
change-id: 20240404-enable-streams-impro-db8bcd898471

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 1/4] media: subdev: Add checks for subdev features

[Tomi Valkeinen]

Add some checks to verify that the subdev driver implements required
features.

A subdevice that supports streams (V4L2_SUBDEV_FL_STREAMS) must do one
of the following:

- Implement neither .enable/disable_streams() nor .s_stream(), if the
  subdev is part of a video driver that uses an internal method to
  enable the subdev.
- Implement only .enable/disable_streams(), if support for legacy
  sink-side subdevices is not needed.
- Implement .enable/disable_streams() and .s_stream(), if support for
  legacy sink-side subdevices is needed.

At the moment the framework doesn't check this requirement. Add the
check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4c6198c48dd6..b90b5185e87f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1519,6 +1519,33 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
 				struct lock_class_key *key)
 {
 	struct v4l2_subdev_state *state;
+	struct device *dev = sd->dev;
+	bool has_disable_streams;
+	bool has_enable_streams;
+	bool has_s_stream;
+
+	/* Check that the subdevice implements the required features */
+
+	has_s_stream = sd->ops && sd->ops->video && sd->ops->video->s_stream;
+	has_enable_streams = sd->ops && sd->ops->pad && sd->ops->pad->enable_streams;
+	has_disable_streams = sd->ops && sd->ops->pad && sd->ops->pad->disable_streams;
+
+	if (has_enable_streams != has_disable_streams) {
+		dev_err(dev,
+			"subdev '%s' must implement both or neither of .enable_streams() and .disable_streams()\n",
+			sd->name);
+		return -EINVAL;
+	}
+
+	if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
+		if (has_s_stream && !has_enable_streams) {
+			dev_err(dev,
+				"subdev '%s' must implement .enable/disable_streams()\n",
+				sd->name);
+
+			return -EINVAL;
+		}
+	}
 
 	state = __v4l2_subdev_state_alloc(sd, name, key);
 	if (IS_ERR(state))

-- 
2.34.1

Re: [PATCH 1/4] media: subdev: Add checks for subdev features

[Sakari Ailus]

Moi,

On Thu, Apr 04, 2024 at 01:50:00PM +0300, Tomi Valkeinen wrote:
> Add some checks to verify that the subdev driver implements required
> features.
> 
> A subdevice that supports streams (V4L2_SUBDEV_FL_STREAMS) must do one
> of the following:
> 
> - Implement neither .enable/disable_streams() nor .s_stream(), if the
>   subdev is part of a video driver that uses an internal method to
>   enable the subdev.
> - Implement only .enable/disable_streams(), if support for legacy
>   sink-side subdevices is not needed.
> - Implement .enable/disable_streams() and .s_stream(), if support for
>   legacy sink-side subdevices is needed.
> 
> At the moment the framework doesn't check this requirement. Add the
> check.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..b90b5185e87f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1519,6 +1519,33 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>  				struct lock_class_key *key)
>  {
>  	struct v4l2_subdev_state *state;
> +	struct device *dev = sd->dev;
> +	bool has_disable_streams;
> +	bool has_enable_streams;
> +	bool has_s_stream;
> +
> +	/* Check that the subdevice implements the required features */
> +
> +	has_s_stream = sd->ops && sd->ops->video && sd->ops->video->s_stream;
> +	has_enable_streams = sd->ops && sd->ops->pad && sd->ops->pad->enable_streams;
> +	has_disable_streams = sd->ops && sd->ops->pad && sd->ops->pad->disable_streams;

There is v4l2_subdev_has_op(). Please also run:

	./scripts/checkpatch.pl --strict --max-line-length=80

on this.

> +
> +	if (has_enable_streams != has_disable_streams) {
> +		dev_err(dev,
> +			"subdev '%s' must implement both or neither of .enable_streams() and .disable_streams()\n",
> +			sd->name);
> +		return -EINVAL;
> +	}
> +
> +	if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
> +		if (has_s_stream && !has_enable_streams) {

Is the s_stream callback allowed to be something else than NULL or
v4l2_subdev_s_stream_helper?

> +			dev_err(dev,
> +				"subdev '%s' must implement .enable/disable_streams()\n",
> +				sd->name);
> +
> +			return -EINVAL;
> +		}
> +	}
>  
>  	state = __v4l2_subdev_state_alloc(sd, name, key);
>  	if (IS_ERR(state))
> 

-- 
Terveisin,

Sakari Ailus

Re: [PATCH 1/4] media: subdev: Add checks for subdev features

[Tomi Valkeinen]

On 04/04/2024 14:09, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Apr 04, 2024 at 01:50:00PM +0300, Tomi Valkeinen wrote:
>> Add some checks to verify that the subdev driver implements required
>> features.
>>
>> A subdevice that supports streams (V4L2_SUBDEV_FL_STREAMS) must do one
>> of the following:
>>
>> - Implement neither .enable/disable_streams() nor .s_stream(), if the
>>    subdev is part of a video driver that uses an internal method to
>>    enable the subdev.
>> - Implement only .enable/disable_streams(), if support for legacy
>>    sink-side subdevices is not needed.
>> - Implement .enable/disable_streams() and .s_stream(), if support for
>>    legacy sink-side subdevices is needed.
>>
>> At the moment the framework doesn't check this requirement. Add the
>> check.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4c6198c48dd6..b90b5185e87f 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -1519,6 +1519,33 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>>   				struct lock_class_key *key)
>>   {
>>   	struct v4l2_subdev_state *state;
>> +	struct device *dev = sd->dev;
>> +	bool has_disable_streams;
>> +	bool has_enable_streams;
>> +	bool has_s_stream;
>> +
>> +	/* Check that the subdevice implements the required features */
>> +
>> +	has_s_stream = sd->ops && sd->ops->video && sd->ops->video->s_stream;
>> +	has_enable_streams = sd->ops && sd->ops->pad && sd->ops->pad->enable_streams;
>> +	has_disable_streams = sd->ops && sd->ops->pad && sd->ops->pad->disable_streams;
> 
> There is v4l2_subdev_has_op(). Please also run:
> 
> 	./scripts/checkpatch.pl --strict --max-line-length=80
> 
> on this.
> 
>> +
>> +	if (has_enable_streams != has_disable_streams) {
>> +		dev_err(dev,
>> +			"subdev '%s' must implement both or neither of .enable_streams() and .disable_streams()\n",
>> +			sd->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
>> +		if (has_s_stream && !has_enable_streams) {
> 
> Is the s_stream callback allowed to be something else than NULL or
> v4l2_subdev_s_stream_helper?

v4l2_subdev_s_stream_helper() only supports subdevs with a single source 
pad. The driver can implement its own version.

  Tomi

[PATCH 2/4] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Tomi Valkeinen]

call_s_stream() uses sd->enabled_streams to track whether streaming has
already been enabled. However,
v4l2_subdev_enable/disable_streams_fallback(), which was the original
user of this field, already uses it, and
v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().

This leads to a conflict as both functions set the field. Afaics, both
functions set the field to the same value, so it won't cause a runtime
bug, but it's still wrong and if we, e.g., change how
v4l2_subdev_enable/disable_streams_fallback() operates we might easily
cause bugs.

Fix this by adding a new field, 'streaming_enabled', for
call_s_stream().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
 include/media/v4l2-subdev.h           | 2 ++
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b90b5185e87f..3b3310bce5d4 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -404,12 +404,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 	 * The .s_stream() operation must never be called to start or stop an
 	 * already started or stopped subdev. Catch offenders but don't return
 	 * an error yet to avoid regressions.
-	 *
-	 * As .s_stream() is mutually exclusive with the .enable_streams() and
-	 * .disable_streams() operation, we can use the enabled_streams field
-	 * to store the subdev streaming state.
 	 */
-	if (WARN_ON(!!sd->enabled_streams == !!enable))
+	if (WARN_ON(!!sd->streaming_enabled == !!enable))
 		return 0;
 
 #if IS_REACHABLE(CONFIG_LEDS_CLASS)
@@ -429,7 +425,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	if (!ret)
-		sd->enabled_streams = enable ? BIT(0) : 0;
+		sd->streaming_enabled = !!enable;
 
 	return ret;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a9e6b8146279..8bd1e3c96d2b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1043,6 +1043,7 @@ struct v4l2_subdev_platform_data {
  *		     v4l2_subdev_enable_streams() and
  *		     v4l2_subdev_disable_streams() helper functions for fallback
  *		     cases.
+ * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
  *
  * Each instance of a subdev driver should create this struct, either
  * stand-alone or embedded in a larger struct.
@@ -1091,6 +1092,7 @@ struct v4l2_subdev {
 	 */
 	struct v4l2_subdev_state *active_state;
 	u64 enabled_streams;
+	bool streaming_enabled;
 };
 
 

-- 
2.34.1

Re: [PATCH 2/4] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Apr 04, 2024 at 01:50:01PM +0300, Tomi Valkeinen wrote:
> call_s_stream() uses sd->enabled_streams to track whether streaming has
> already been enabled. However,
> v4l2_subdev_enable/disable_streams_fallback(), which was the original
> user of this field, already uses it, and
> v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().
> 
> This leads to a conflict as both functions set the field. Afaics, both
> functions set the field to the same value, so it won't cause a runtime
> bug, but it's still wrong and if we, e.g., change how
> v4l2_subdev_enable/disable_streams_fallback() operates we might easily
> cause bugs.
> 
> Fix this by adding a new field, 'streaming_enabled', for
> call_s_stream().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>  include/media/v4l2-subdev.h           | 2 ++
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b90b5185e87f..3b3310bce5d4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -404,12 +404,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  	 * The .s_stream() operation must never be called to start or stop an
>  	 * already started or stopped subdev. Catch offenders but don't return
>  	 * an error yet to avoid regressions.
> -	 *
> -	 * As .s_stream() is mutually exclusive with the .enable_streams() and
> -	 * .disable_streams() operation, we can use the enabled_streams field
> -	 * to store the subdev streaming state.
>  	 */
> -	if (WARN_ON(!!sd->enabled_streams == !!enable))
> +	if (WARN_ON(!!sd->streaming_enabled == !!enable))
>  		return 0;
>  
>  #if IS_REACHABLE(CONFIG_LEDS_CLASS)
> @@ -429,7 +425,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	if (!ret)
> -		sd->enabled_streams = enable ? BIT(0) : 0;
> +		sd->streaming_enabled = !!enable;
>  
>  	return ret;
>  }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..8bd1e3c96d2b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1043,6 +1043,7 @@ struct v4l2_subdev_platform_data {
>   *		     v4l2_subdev_enable_streams() and
>   *		     v4l2_subdev_disable_streams() helper functions for fallback
>   *		     cases.
> + * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.

Could you extend this to indicate this field can't be used by anything
else than call_s_stream() ?

I'm also getting a bit concerned about having multiple fields with
similar purposes. Another option would be to call the .s_stream()
operation directly from the fallback handlers, instead of going through
call_s_stream(). That may be considered as a bit of a hack though.

>   *
>   * Each instance of a subdev driver should create this struct, either
>   * stand-alone or embedded in a larger struct.
> @@ -1091,6 +1092,7 @@ struct v4l2_subdev {
>  	 */
>  	struct v4l2_subdev_state *active_state;
>  	u64 enabled_streams;
> +	bool streaming_enabled;
>  };
>  
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: subdev: Fix use of sd->enabled_streams in call_s_stream()

[Tomi Valkeinen]

On 04/04/2024 16:00, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Apr 04, 2024 at 01:50:01PM +0300, Tomi Valkeinen wrote:
>> call_s_stream() uses sd->enabled_streams to track whether streaming has
>> already been enabled. However,
>> v4l2_subdev_enable/disable_streams_fallback(), which was the original
>> user of this field, already uses it, and
>> v4l2_subdev_enable/disable_streams_fallback() will call call_s_stream().
>>
>> This leads to a conflict as both functions set the field. Afaics, both
>> functions set the field to the same value, so it won't cause a runtime
>> bug, but it's still wrong and if we, e.g., change how
>> v4l2_subdev_enable/disable_streams_fallback() operates we might easily
>> cause bugs.
>>
>> Fix this by adding a new field, 'streaming_enabled', for
>> call_s_stream().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>>   include/media/v4l2-subdev.h           | 2 ++
>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index b90b5185e87f..3b3310bce5d4 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -404,12 +404,8 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>   	 * The .s_stream() operation must never be called to start or stop an
>>   	 * already started or stopped subdev. Catch offenders but don't return
>>   	 * an error yet to avoid regressions.
>> -	 *
>> -	 * As .s_stream() is mutually exclusive with the .enable_streams() and
>> -	 * .disable_streams() operation, we can use the enabled_streams field
>> -	 * to store the subdev streaming state.
>>   	 */
>> -	if (WARN_ON(!!sd->enabled_streams == !!enable))
>> +	if (WARN_ON(!!sd->streaming_enabled == !!enable))
>>   		return 0;
>>   
>>   #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> @@ -429,7 +425,7 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>   	}
>>   
>>   	if (!ret)
>> -		sd->enabled_streams = enable ? BIT(0) : 0;
>> +		sd->streaming_enabled = !!enable;
>>   
>>   	return ret;
>>   }
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index a9e6b8146279..8bd1e3c96d2b 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1043,6 +1043,7 @@ struct v4l2_subdev_platform_data {
>>    *		     v4l2_subdev_enable_streams() and
>>    *		     v4l2_subdev_disable_streams() helper functions for fallback
>>    *		     cases.
>> + * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
> 
> Could you extend this to indicate this field can't be used by anything
> else than call_s_stream() ?

Yes, I'll add that.

> I'm also getting a bit concerned about having multiple fields with
> similar purposes. Another option would be to call the .s_stream()

I agree, it's getting slightly complex. The reason is that we support 
many ways for the subdev to implement these things.

> operation directly from the fallback handlers, instead of going through
> call_s_stream(). That may be considered as a bit of a hack though.

I thought about the same thing, but I felt it might just make things 
more complex. However, with my new unsent series, I add the privacy_led 
code also to the v4l2_subdev_enable_streams() function, so it's even 
more similar to the call_s_stream.

Maybe I'll try it out and see how it looks like if I call the op directly.

  Tomi

>>    *
>>    * Each instance of a subdev driver should create this struct, either
>>    * stand-alone or embedded in a larger struct.
>> @@ -1091,6 +1092,7 @@ struct v4l2_subdev {
>>   	 */
>>   	struct v4l2_subdev_state *active_state;
>>   	u64 enabled_streams;
>> +	bool streaming_enabled;
>>   };
>>   
>>   
> 

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

[Tomi Valkeinen]

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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 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.
 	 */
 	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
 		return -EOPNOTSUPP;
 
-	for (i = 0; i < sd->entity.num_pads; ++i) {
-		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
-			return -EOPNOTSUPP;
-	}
+	/*
+	 * .s_stream() means there is no streams support, so only allowed stream
+	 * is the imlicit stream 0.
+	 */
+	if (streams_mask != BIT_ULL(0))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
+	 * with 64 pads or less can be supported.
+	 */
+	if (pad >= sizeof(sd->enabled_pads) * 8)
+		return -EOPNOTSUPP;
 
-	if (sd->enabled_streams & streams_mask) {
-		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
-			streams_mask, sd->entity.name, pad);
+	if (sd->enabled_pads & BIT_ULL(pad)) {
+		dev_dbg(dev, "pad %u already enabled on %s\n",
+			pad, sd->entity.name);
 		return -EALREADY;
 	}
 
-	/* Start streaming when the first streams are enabled. */
-	if (!sd->enabled_streams) {
+	/* Start streaming when the first pad is enabled. */
+	if (!sd->enabled_pads) {
 		ret = v4l2_subdev_call(sd, video, s_stream, 1);
 		if (ret)
 			return ret;
 	}
 
-	sd->enabled_streams |= streams_mask;
+	sd->enabled_pads |= BIT_ULL(pad);
 
 	return 0;
 }
@@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
 
 	/*
-	 * If 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.
+	 * If the subdev doesn't implement pad-based stream enable, fall back
+	 * on the .s_stream() operation.
 	 */
 	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
 		return -EOPNOTSUPP;
 
-	for (i = 0; i < sd->entity.num_pads; ++i) {
-		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
-			return -EOPNOTSUPP;
-	}
+	/*
+	 * .s_stream() means there is no streams support, so only allowed stream
+	 * is the imlicit stream 0.
+	 */
+	if (streams_mask != BIT_ULL(0))
+		return -EOPNOTSUPP;
+
+	/*
+	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
+	 * with 64 pads or less can be supported.
+	 */
+	if (pad >= sizeof(sd->enabled_pads) * 8)
+		return -EOPNOTSUPP;
 
-	if ((sd->enabled_streams & streams_mask) != streams_mask) {
-		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
-			streams_mask, sd->entity.name, pad);
+	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
+		dev_dbg(dev, "pad %u already disabled on %s\n",
+			pad, sd->entity.name);
 		return -EALREADY;
 	}
 
 	/* Stop streaming when the last streams are disabled. */
-	if (!(sd->enabled_streams & ~streams_mask)) {
+	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
 		ret = v4l2_subdev_call(sd, video, s_stream, 0);
 		if (ret)
 			return ret;
 	}
 
-	sd->enabled_streams &= ~streams_mask;
+	sd->enabled_pads &= ~BIT_ULL(pad);
 
 	return 0;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8bd1e3c96d2b..7077aec3176c 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
  * @active_state: Active state for the subdev (NULL for subdevs tracking the
  *		  state internally). Initialized by calling
  *		  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.
+ * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
+ *		  and v4l2_subdev_disable_streams() helper functions for
+ *		  fallback cases.
  * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
  *
  * Each instance of a subdev driver should create this struct, either
@@ -1091,7 +1090,7 @@ struct v4l2_subdev {
 	 * doesn't support it.
 	 */
 	struct v4l2_subdev_state *active_state;
-	u64 enabled_streams;
+	u64 enabled_pads;
 	bool streaming_enabled;
 };
 

-- 
2.34.1

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

[Sakari Ailus]

Moi,

Thanks for the patch.

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.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  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.
>  	 */
>  	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>  		return -EOPNOTSUPP;
>  
> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> -			return -EOPNOTSUPP;
> -	}
> +	/*
> +	 * .s_stream() means there is no streams support, so only allowed stream
> +	 * is the imlicit stream 0.
> +	 */
> +	if (streams_mask != BIT_ULL(0))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> +	 * with 64 pads or less can be supported.
> +	 */
> +	if (pad >= sizeof(sd->enabled_pads) * 8)

s/8/BITS_PER_BYTE/

> +		return -EOPNOTSUPP;
>  
> -	if (sd->enabled_streams & streams_mask) {
> -		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
> -			streams_mask, sd->entity.name, pad);
> +	if (sd->enabled_pads & BIT_ULL(pad)) {
> +		dev_dbg(dev, "pad %u already enabled on %s\n",
> +			pad, sd->entity.name);
>  		return -EALREADY;
>  	}
>  
> -	/* Start streaming when the first streams are enabled. */
> -	if (!sd->enabled_streams) {
> +	/* Start streaming when the first pad is enabled. */
> +	if (!sd->enabled_pads) {
>  		ret = v4l2_subdev_call(sd, video, s_stream, 1);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	sd->enabled_streams |= streams_mask;
> +	sd->enabled_pads |= BIT_ULL(pad);
>  
>  	return 0;
>  }
> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
>  
>  	/*
> -	 * If 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.
> +	 * If the subdev doesn't implement pad-based stream enable, fall back
> +	 * on the .s_stream() operation.
>  	 */
>  	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>  		return -EOPNOTSUPP;
>  
> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> -			return -EOPNOTSUPP;
> -	}
> +	/*
> +	 * .s_stream() means there is no streams support, so only allowed stream
> +	 * is the imlicit stream 0.
> +	 */
> +	if (streams_mask != BIT_ULL(0))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> +	 * with 64 pads or less can be supported.
> +	 */
> +	if (pad >= sizeof(sd->enabled_pads) * 8)

Ditto.

How much of the code of the two functions is the same? Could some of this
be put to a common function both would call? They look (almost) exactly the
same.

> +		return -EOPNOTSUPP;
>  
> -	if ((sd->enabled_streams & streams_mask) != streams_mask) {
> -		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
> -			streams_mask, sd->entity.name, pad);
> +	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
> +		dev_dbg(dev, "pad %u already disabled on %s\n",
> +			pad, sd->entity.name);
>  		return -EALREADY;
>  	}
>  
>  	/* Stop streaming when the last streams are disabled. */
> -	if (!(sd->enabled_streams & ~streams_mask)) {
> +	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
>  		ret = v4l2_subdev_call(sd, video, s_stream, 0);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	sd->enabled_streams &= ~streams_mask;
> +	sd->enabled_pads &= ~BIT_ULL(pad);
>  
>  	return 0;
>  }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8bd1e3c96d2b..7077aec3176c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
>   * @active_state: Active state for the subdev (NULL for subdevs tracking the
>   *		  state internally). Initialized by calling
>   *		  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.
> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
> + *		  and v4l2_subdev_disable_streams() helper functions for
> + *		  fallback cases.
>   * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
>   *
>   * Each instance of a subdev driver should create this struct, either
> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
>  	 * doesn't support it.
>  	 */
>  	struct v4l2_subdev_state *active_state;
> -	u64 enabled_streams;
> +	u64 enabled_pads;
>  	bool streaming_enabled;
>  };
>  
> 

-- 
Kind regards,

Sakari Ailus

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

[Tomi Valkeinen]

On 04/04/2024 15:18, Sakari Ailus wrote:
> Moi,
> 
> Thanks for the patch.
> 
> 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.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   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.
>>   	 */
>>   	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>>   		return -EOPNOTSUPP;
>>   
>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
>> -			return -EOPNOTSUPP;
>> -	}
>> +	/*
>> +	 * .s_stream() means there is no streams support, so only allowed stream
>> +	 * is the imlicit stream 0.
>> +	 */
>> +	if (streams_mask != BIT_ULL(0))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
>> +	 * with 64 pads or less can be supported.
>> +	 */
>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> 
> s/8/BITS_PER_BYTE/
> 
>> +		return -EOPNOTSUPP;
>>   
>> -	if (sd->enabled_streams & streams_mask) {
>> -		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
>> -			streams_mask, sd->entity.name, pad);
>> +	if (sd->enabled_pads & BIT_ULL(pad)) {
>> +		dev_dbg(dev, "pad %u already enabled on %s\n",
>> +			pad, sd->entity.name);
>>   		return -EALREADY;
>>   	}
>>   
>> -	/* Start streaming when the first streams are enabled. */
>> -	if (!sd->enabled_streams) {
>> +	/* Start streaming when the first pad is enabled. */
>> +	if (!sd->enabled_pads) {
>>   		ret = v4l2_subdev_call(sd, video, s_stream, 1);
>>   		if (ret)
>>   			return ret;
>>   	}
>>   
>> -	sd->enabled_streams |= streams_mask;
>> +	sd->enabled_pads |= BIT_ULL(pad);
>>   
>>   	return 0;
>>   }
>> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
>>   
>>   	/*
>> -	 * If 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.
>> +	 * If the subdev doesn't implement pad-based stream enable, fall back
>> +	 * on the .s_stream() operation.
>>   	 */
>>   	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>>   		return -EOPNOTSUPP;
>>   
>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
>> -			return -EOPNOTSUPP;
>> -	}
>> +	/*
>> +	 * .s_stream() means there is no streams support, so only allowed stream
>> +	 * is the imlicit stream 0.
>> +	 */
>> +	if (streams_mask != BIT_ULL(0))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
>> +	 * with 64 pads or less can be supported.
>> +	 */
>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> 
> Ditto.
> 
> How much of the code of the two functions is the same? Could some of this
> be put to a common function both would call? They look (almost) exactly the
> same.

v4l2_subdev_enable_streams_fallback and v4l2_subdev_enable_streams? They 
have similar parts, but I don't right away see how combining them or 
making some common functions would help.

  Tomi

> 
>> +		return -EOPNOTSUPP;
>>   
>> -	if ((sd->enabled_streams & streams_mask) != streams_mask) {
>> -		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
>> -			streams_mask, sd->entity.name, pad);
>> +	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
>> +		dev_dbg(dev, "pad %u already disabled on %s\n",
>> +			pad, sd->entity.name);
>>   		return -EALREADY;
>>   	}
>>   
>>   	/* Stop streaming when the last streams are disabled. */
>> -	if (!(sd->enabled_streams & ~streams_mask)) {
>> +	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
>>   		ret = v4l2_subdev_call(sd, video, s_stream, 0);
>>   		if (ret)
>>   			return ret;
>>   	}
>>   
>> -	sd->enabled_streams &= ~streams_mask;
>> +	sd->enabled_pads &= ~BIT_ULL(pad);
>>   
>>   	return 0;
>>   }
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 8bd1e3c96d2b..7077aec3176c 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
>>    * @active_state: Active state for the subdev (NULL for subdevs tracking the
>>    *		  state internally). Initialized by calling
>>    *		  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.
>> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
>> + *		  and v4l2_subdev_disable_streams() helper functions for
>> + *		  fallback cases.
>>    * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
>>    *
>>    * Each instance of a subdev driver should create this struct, either
>> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
>>   	 * doesn't support it.
>>   	 */
>>   	struct v4l2_subdev_state *active_state;
>> -	u64 enabled_streams;
>> +	u64 enabled_pads;
>>   	bool streaming_enabled;
>>   };
>>   
>>
> 

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

[Laurent Pinchart]

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@ideasonboard.com>
> >> ---
> >>   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/

Same below.

> >>   	 */
> >>   	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>   		return -EOPNOTSUPP;
> >>   
> >> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> >> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >> -			return -EOPNOTSUPP;
> >> -	}
> >> +	/*
> >> +	 * .s_stream() means there is no streams support, so only allowed stream
> >> +	 * is the imlicit stream 0.

s/imlicit/implicit/

Same below.

> >> +	 */
> >> +	if (streams_mask != BIT_ULL(0))
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	/*
> >> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >> +	 * with 64 pads or less can be supported.
> >> +	 */
> >> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> > 
> > s/8/BITS_PER_BYTE/
> > 
> >> +		return -EOPNOTSUPP;
> >>   
> >> -	if (sd->enabled_streams & streams_mask) {
> >> -		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
> >> -			streams_mask, sd->entity.name, pad);
> >> +	if (sd->enabled_pads & BIT_ULL(pad)) {
> >> +		dev_dbg(dev, "pad %u already enabled on %s\n",
> >> +			pad, sd->entity.name);
> >>   		return -EALREADY;
> >>   	}
> >>   
> >> -	/* Start streaming when the first streams are enabled. */
> >> -	if (!sd->enabled_streams) {
> >> +	/* Start streaming when the first pad is enabled. */
> >> +	if (!sd->enabled_pads) {
> >>   		ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >>   		if (ret)
> >>   			return ret;
> >>   	}
> >>   
> >> -	sd->enabled_streams |= streams_mask;
> >> +	sd->enabled_pads |= BIT_ULL(pad);
> >>   
> >>   	return 0;
> >>   }
> >> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
> >>   
> >>   	/*
> >> -	 * If 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.
> >> +	 * If the subdev doesn't implement pad-based stream enable, fall back
> >> +	 * on the .s_stream() operation.
> >>   	 */
> >>   	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>   		return -EOPNOTSUPP;
> >>   
> >> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> >> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >> -			return -EOPNOTSUPP;
> >> -	}
> >> +	/*
> >> +	 * .s_stream() means there is no streams support, so only allowed stream
> >> +	 * is the imlicit stream 0.
> >> +	 */
> >> +	if (streams_mask != BIT_ULL(0))
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	/*
> >> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >> +	 * with 64 pads or less can be supported.
> >> +	 */
> >> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> > 
> > Ditto.
> > 
> > How much of the code of the two functions is the same? Could some of this
> > be put to a common function both would call? They look (almost) exactly the
> > same.
> 
> v4l2_subdev_enable_streams_fallback and v4l2_subdev_enable_streams? They 
> have similar parts, but I don't right away see how combining them or 
> making some common functions would help.

You could move the three checks to a
v4l2_subdev_streams_fallback_check() function (bikeshedding the name is
allowed).

> >> +		return -EOPNOTSUPP;
> >>   
> >> -	if ((sd->enabled_streams & streams_mask) != streams_mask) {
> >> -		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
> >> -			streams_mask, sd->entity.name, pad);
> >> +	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {

	if (!(sd->enabled_pads & BIT_ULL(pad))) {

> >> +		dev_dbg(dev, "pad %u already disabled on %s\n",
> >> +			pad, sd->entity.name);
> >>   		return -EALREADY;
> >>   	}
> >>   
> >>   	/* Stop streaming when the last streams are disabled. */
> >> -	if (!(sd->enabled_streams & ~streams_mask)) {
> >> +	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
> >>   		ret = v4l2_subdev_call(sd, video, s_stream, 0);
> >>   		if (ret)
> >>   			return ret;
> >>   	}
> >>   
> >> -	sd->enabled_streams &= ~streams_mask;
> >> +	sd->enabled_pads &= ~BIT_ULL(pad);
> >>   
> >>   	return 0;
> >>   }
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 8bd1e3c96d2b..7077aec3176c 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
> >>    * @active_state: Active state for the subdev (NULL for subdevs tracking the
> >>    *		  state internally). Initialized by calling
> >>    *		  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.
> >> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
> >> + *		  and v4l2_subdev_disable_streams() helper functions for
> >> + *		  fallback cases.
> >>    * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
> >>    *
> >>    * Each instance of a subdev driver should create this struct, either
> >> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
> >>   	 * doesn't support it.
> >>   	 */
> >>   	struct v4l2_subdev_state *active_state;
> >> -	u64 enabled_streams;
> >> +	u64 enabled_pads;
> >>   	bool streaming_enabled;
> >>   };
> >>   

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

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@ideasonboard.com>
>>>> ---
>>>>    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)?

  Tomi


> 
> Same below.
> 
>>>>    	 */
>>>>    	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
>>>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
>>>> -			return -EOPNOTSUPP;
>>>> -	}
>>>> +	/*
>>>> +	 * .s_stream() means there is no streams support, so only allowed stream
>>>> +	 * is the imlicit stream 0.
> 
> s/imlicit/implicit/
> 
> Same below.
> 
>>>> +	 */
>>>> +	if (streams_mask != BIT_ULL(0))
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	/*
>>>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
>>>> +	 * with 64 pads or less can be supported.
>>>> +	 */
>>>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
>>>
>>> s/8/BITS_PER_BYTE/
>>>
>>>> +		return -EOPNOTSUPP;
>>>>    
>>>> -	if (sd->enabled_streams & streams_mask) {
>>>> -		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
>>>> -			streams_mask, sd->entity.name, pad);
>>>> +	if (sd->enabled_pads & BIT_ULL(pad)) {
>>>> +		dev_dbg(dev, "pad %u already enabled on %s\n",
>>>> +			pad, sd->entity.name);
>>>>    		return -EALREADY;
>>>>    	}
>>>>    
>>>> -	/* Start streaming when the first streams are enabled. */
>>>> -	if (!sd->enabled_streams) {
>>>> +	/* Start streaming when the first pad is enabled. */
>>>> +	if (!sd->enabled_pads) {
>>>>    		ret = v4l2_subdev_call(sd, video, s_stream, 1);
>>>>    		if (ret)
>>>>    			return ret;
>>>>    	}
>>>>    
>>>> -	sd->enabled_streams |= streams_mask;
>>>> +	sd->enabled_pads |= BIT_ULL(pad);
>>>>    
>>>>    	return 0;
>>>>    }
>>>> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
>>>>    
>>>>    	/*
>>>> -	 * If 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.
>>>> +	 * If the subdev doesn't implement pad-based stream enable, fall back
>>>> +	 * on the .s_stream() operation.
>>>>    	 */
>>>>    	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
>>>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
>>>> -			return -EOPNOTSUPP;
>>>> -	}
>>>> +	/*
>>>> +	 * .s_stream() means there is no streams support, so only allowed stream
>>>> +	 * is the imlicit stream 0.
>>>> +	 */
>>>> +	if (streams_mask != BIT_ULL(0))
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	/*
>>>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
>>>> +	 * with 64 pads or less can be supported.
>>>> +	 */
>>>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
>>>
>>> Ditto.
>>>
>>> How much of the code of the two functions is the same? Could some of this
>>> be put to a common function both would call? They look (almost) exactly the
>>> same.
>>
>> v4l2_subdev_enable_streams_fallback and v4l2_subdev_enable_streams? They
>> have similar parts, but I don't right away see how combining them or
>> making some common functions would help.
> 
> You could move the three checks to a
> v4l2_subdev_streams_fallback_check() function (bikeshedding the name is
> allowed).
> 
>>>> +		return -EOPNOTSUPP;
>>>>    
>>>> -	if ((sd->enabled_streams & streams_mask) != streams_mask) {
>>>> -		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
>>>> -			streams_mask, sd->entity.name, pad);
>>>> +	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
> 
> 	if (!(sd->enabled_pads & BIT_ULL(pad))) {
> 
>>>> +		dev_dbg(dev, "pad %u already disabled on %s\n",
>>>> +			pad, sd->entity.name);
>>>>    		return -EALREADY;
>>>>    	}
>>>>    
>>>>    	/* Stop streaming when the last streams are disabled. */
>>>> -	if (!(sd->enabled_streams & ~streams_mask)) {
>>>> +	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
>>>>    		ret = v4l2_subdev_call(sd, video, s_stream, 0);
>>>>    		if (ret)
>>>>    			return ret;
>>>>    	}
>>>>    
>>>> -	sd->enabled_streams &= ~streams_mask;
>>>> +	sd->enabled_pads &= ~BIT_ULL(pad);
>>>>    
>>>>    	return 0;
>>>>    }
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index 8bd1e3c96d2b..7077aec3176c 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
>>>>     * @active_state: Active state for the subdev (NULL for subdevs tracking the
>>>>     *		  state internally). Initialized by calling
>>>>     *		  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.
>>>> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
>>>> + *		  and v4l2_subdev_disable_streams() helper functions for
>>>> + *		  fallback cases.
>>>>     * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
>>>>     *
>>>>     * Each instance of a subdev driver should create this struct, either
>>>> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
>>>>    	 * doesn't support it.
>>>>    	 */
>>>>    	struct v4l2_subdev_state *active_state;
>>>> -	u64 enabled_streams;
>>>> +	u64 enabled_pads;
>>>>    	bool streaming_enabled;
>>>>    };
>>>>    
> 

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

[Laurent Pinchart]

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@ideasonboard.com>
> >>>> ---
> >>>>    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 :-)

> > Same below.
> > 
> >>>>    	 */
> >>>>    	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>>>    		return -EOPNOTSUPP;
> >>>>    
> >>>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> >>>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >>>> -			return -EOPNOTSUPP;
> >>>> -	}
> >>>> +	/*
> >>>> +	 * .s_stream() means there is no streams support, so only allowed stream
> >>>> +	 * is the imlicit stream 0.
> > 
> > s/imlicit/implicit/
> > 
> > Same below.
> > 
> >>>> +	 */
> >>>> +	if (streams_mask != BIT_ULL(0))
> >>>> +		return -EOPNOTSUPP;
> >>>> +
> >>>> +	/*
> >>>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >>>> +	 * with 64 pads or less can be supported.
> >>>> +	 */
> >>>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> >>>
> >>> s/8/BITS_PER_BYTE/
> >>>
> >>>> +		return -EOPNOTSUPP;
> >>>>    
> >>>> -	if (sd->enabled_streams & streams_mask) {
> >>>> -		dev_dbg(dev, "set of streams %#llx already enabled on %s:%u\n",
> >>>> -			streams_mask, sd->entity.name, pad);
> >>>> +	if (sd->enabled_pads & BIT_ULL(pad)) {
> >>>> +		dev_dbg(dev, "pad %u already enabled on %s\n",
> >>>> +			pad, sd->entity.name);
> >>>>    		return -EALREADY;
> >>>>    	}
> >>>>    
> >>>> -	/* Start streaming when the first streams are enabled. */
> >>>> -	if (!sd->enabled_streams) {
> >>>> +	/* Start streaming when the first pad is enabled. */
> >>>> +	if (!sd->enabled_pads) {
> >>>>    		ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >>>>    		if (ret)
> >>>>    			return ret;
> >>>>    	}
> >>>>    
> >>>> -	sd->enabled_streams |= streams_mask;
> >>>> +	sd->enabled_pads |= BIT_ULL(pad);
> >>>>    
> >>>>    	return 0;
> >>>>    }
> >>>> @@ -2207,37 +2213,43 @@ static int v4l2_subdev_disable_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;
> >>>>    
> >>>>    	/*
> >>>> -	 * If 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.
> >>>> +	 * If the subdev doesn't implement pad-based stream enable, fall back
> >>>> +	 * on the .s_stream() operation.
> >>>>    	 */
> >>>>    	if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE))
> >>>>    		return -EOPNOTSUPP;
> >>>>    
> >>>> -	for (i = 0; i < sd->entity.num_pads; ++i) {
> >>>> -		if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
> >>>> -			return -EOPNOTSUPP;
> >>>> -	}
> >>>> +	/*
> >>>> +	 * .s_stream() means there is no streams support, so only allowed stream
> >>>> +	 * is the imlicit stream 0.
> >>>> +	 */
> >>>> +	if (streams_mask != BIT_ULL(0))
> >>>> +		return -EOPNOTSUPP;
> >>>> +
> >>>> +	/*
> >>>> +	 * We use a 64-bit bitmask for tracking enabled pads, so only subdevices
> >>>> +	 * with 64 pads or less can be supported.
> >>>> +	 */
> >>>> +	if (pad >= sizeof(sd->enabled_pads) * 8)
> >>>
> >>> Ditto.
> >>>
> >>> How much of the code of the two functions is the same? Could some of this
> >>> be put to a common function both would call? They look (almost) exactly the
> >>> same.
> >>
> >> v4l2_subdev_enable_streams_fallback and v4l2_subdev_enable_streams? They
> >> have similar parts, but I don't right away see how combining them or
> >> making some common functions would help.
> > 
> > You could move the three checks to a
> > v4l2_subdev_streams_fallback_check() function (bikeshedding the name is
> > allowed).
> > 
> >>>> +		return -EOPNOTSUPP;
> >>>>    
> >>>> -	if ((sd->enabled_streams & streams_mask) != streams_mask) {
> >>>> -		dev_dbg(dev, "set of streams %#llx already disabled on %s:%u\n",
> >>>> -			streams_mask, sd->entity.name, pad);
> >>>> +	if ((sd->enabled_pads & BIT_ULL(pad)) != BIT_ULL(pad)) {
> > 
> > 	if (!(sd->enabled_pads & BIT_ULL(pad))) {
> > 
> >>>> +		dev_dbg(dev, "pad %u already disabled on %s\n",
> >>>> +			pad, sd->entity.name);
> >>>>    		return -EALREADY;
> >>>>    	}
> >>>>    
> >>>>    	/* Stop streaming when the last streams are disabled. */
> >>>> -	if (!(sd->enabled_streams & ~streams_mask)) {
> >>>> +	if (!(sd->enabled_pads & ~BIT_ULL(pad))) {
> >>>>    		ret = v4l2_subdev_call(sd, video, s_stream, 0);
> >>>>    		if (ret)
> >>>>    			return ret;
> >>>>    	}
> >>>>    
> >>>> -	sd->enabled_streams &= ~streams_mask;
> >>>> +	sd->enabled_pads &= ~BIT_ULL(pad);
> >>>>    
> >>>>    	return 0;
> >>>>    }
> >>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>> index 8bd1e3c96d2b..7077aec3176c 100644
> >>>> --- a/include/media/v4l2-subdev.h
> >>>> +++ b/include/media/v4l2-subdev.h
> >>>> @@ -1039,10 +1039,9 @@ struct v4l2_subdev_platform_data {
> >>>>     * @active_state: Active state for the subdev (NULL for subdevs tracking the
> >>>>     *		  state internally). Initialized by calling
> >>>>     *		  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.
> >>>> + * @enabled_pads: Bitmask of enabled pads used by v4l2_subdev_enable_streams()
> >>>> + *		  and v4l2_subdev_disable_streams() helper functions for
> >>>> + *		  fallback cases.
> >>>>     * @streaming_enabled: Tracks whether streaming has been enabled with s_stream.
> >>>>     *
> >>>>     * Each instance of a subdev driver should create this struct, either
> >>>> @@ -1091,7 +1090,7 @@ struct v4l2_subdev {
> >>>>    	 * doesn't support it.
> >>>>    	 */
> >>>>    	struct v4l2_subdev_state *active_state;
> >>>> -	u64 enabled_streams;
> >>>> +	u64 enabled_pads;
> >>>>    	bool streaming_enabled;
> >>>>    };
> >>>>    

-- 
Regards,

Laurent Pinchart

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@ideasonboard.com>
>>>>>> ---
>>>>>>     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

[PATCH 4/4] media: subdev: Support enable/disable_streams for non-streams subdevs

[Tomi Valkeinen]

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.

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:

-- 
2.34.1

Re: [PATCH 4/4] media: subdev: Support enable/disable_streams for non-streams subdevs

[Laurent Pinchart]

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