Re: [PATCH v2 8/9] media: subdev: Refactor v4l2_subdev_enable/disable_streams()

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 10/04/2024 13:13, Sakari Ailus wrote:
> Moi,
>
> Thank you for working on this.
>
> On Fri, Apr 05, 2024 at 12:14:26PM +0300, Tomi Valkeinen wrote:
> > Add two internal helper functions, v4l2_subdev_collect_streams() and
> > v4l2_subdev_set_streams_enabled(), which allows us to refactor
> > v4l2_subdev_enable/disable_streams() functions.
> >
> > This (I think) makes the code a bit easier to read, and lets us more
> > easily add new functionality in the helper functions in the following
> > patch.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
> >   1 file changed, 62 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 015f2fb423c9..6c3c9069f1e2 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -2099,6 +2099,44 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
> >   }
> >   EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
> >   
> > +static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
> > +					struct v4l2_subdev_state *state,
> > +					u32 pad, u64 streams_mask,
> > +					u64 *found_streams,
> > +					u64 *enabled_streams)
> > +{
> > +	*found_streams = 0;
> > +	*enabled_streams = 0;
> > +
> > +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
> > +		const struct v4l2_subdev_stream_config *cfg;
> > +
> > +		cfg = &state->stream_configs.configs[i];
>
> You can do the assignment in declaration.

I can, but you want the lines to be split at 80, so that'll end up being 
in two lines, which, I think, looks messier than the one above.

Usually I think doing initialization when declaring the variable is best 
done if it fits into one line.

 Tomi

> Same for streams_enabled() below.
>
> > +
> > +		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
> > +			continue;
> > +
> > +		*found_streams |= BIT_ULL(cfg->stream);
> > +		if (cfg->enabled)
> > +			*enabled_streams |= BIT_ULL(cfg->stream);
> > +	}
> > +}
> > +
> > +static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
> > +					    struct v4l2_subdev_state *state,
> > +					    u32 pad, u64 streams_mask,
> > +					    bool enabled)
> > +{
> > +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
> > +		struct v4l2_subdev_stream_config *cfg;
> > +
> > +		cfg = &state->stream_configs.configs[i];
> > +
> > +		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > +			cfg->enabled = enabled;
> > +	}
> > +}
> > +
> >   static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
> >   					       u64 streams_mask)
> >   {
> > @@ -2150,8 +2188,8 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >   	struct device *dev = sd->entity.graph_obj.mdev->dev;
> >   	struct v4l2_subdev_state *state;
> >   	bool already_streaming;
> > -	u64 found_streams = 0;
> > -	unsigned int i;
> > +	u64 enabled_streams;
> > +	u64 found_streams;
> >   	int ret;
> >   
> >   	/* A few basic sanity checks first. */
> > @@ -2172,22 +2210,9 @@ 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) {
> > -			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > -			ret = -EALREADY;
> > -			goto done;
> > -		}
> > -	}
> > +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
> > +				    &found_streams, &enabled_streams);
> >   
> >   	if (found_streams != streams_mask) {
> >   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
> > @@ -2196,6 +2221,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >   		goto done;
> >   	}
> >   
> > +	if (enabled_streams) {
> > +		dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n",
> > +			enabled_streams, sd->entity.name, pad);
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> >   	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
> >   
> >   	already_streaming = v4l2_subdev_is_streaming(sd);
> > @@ -2213,13 +2245,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = true;
> > -	}
> > +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
> >   
> >   done:
> >   	if (ret && !already_streaming)
> > @@ -2281,8 +2307,8 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >   {
> >   	struct device *dev = sd->entity.graph_obj.mdev->dev;
> >   	struct v4l2_subdev_state *state;
> > -	u64 found_streams = 0;
> > -	unsigned int i;
> > +	u64 enabled_streams;
> > +	u64 found_streams;
> >   	int ret;
> >   
> >   	/* A few basic sanity checks first. */
> > @@ -2303,22 +2329,9 @@ 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) {
> > -			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
> > -				cfg->stream, sd->entity.name, pad);
> > -			ret = -EALREADY;
> > -			goto done;
> > -		}
> > -	}
> > +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
> > +				    &found_streams, &enabled_streams);
> >   
> >   	if (found_streams != streams_mask) {
> >   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
> > @@ -2327,6 +2340,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >   		goto done;
> >   	}
> >   
> > +	if (enabled_streams != streams_mask) {
> > +		dev_dbg(dev, "streams 0x%llx already disabled on %s:%u\n",
> > +			streams_mask & ~enabled_streams, sd->entity.name, pad);
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> >   	dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
> >   
> >   	/* Call the .disable_streams() operation. */
> > @@ -2338,14 +2358,7 @@ 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 (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
> > -			cfg->enabled = false;
> > -	}
> > +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
> >   
> >   done:
> >   	if (!v4l2_subdev_is_streaming(sd))