[PATCH v4 0/6] media: max9286: Add multiplexed streams support

[PATCH v4 0/6] media: max9286: Add multiplexed streams support

[Jacopo Mondi]

Hello,
   this series breaks out from the larger
[PATCH v2 00/13] media: Add multiplexed support to R-Car CSI-2 and GMSL

and isolated changes to the max9286 driver to support multiplexed
streams.

Rebased on Tomi's v10 and tested on Eagle V3M with the following branch
https://git.sr.ht/~jmondi_/linux #multistream/tomba-v10/gmsl

Laurent has just sent me comments on the previous version which I didn't take
into account here as I wanted to first break the previous series down to
smaller chunks which can be easily digested.

I'll review Laurent's patches and possibly rebase on top of them, or if Tomi
beats me to that with a v11 I'll rebase on top of his work.

Thanks
   j

Jacopo Mondi (6):
  media: max9286: Add support for v4l2_subdev_state
  media: max9286: Implement set_routing
  media: max9286: Use enabled routes to calculate pixelrate
  media: max9286: Use routes to configure link order
  media: max9286: Move format to subdev state
  media: max9286: Implement get_frame_desc()

 drivers/media/i2c/max9286.c | 455 +++++++++++++++++++++++++-----------
 1 file changed, 324 insertions(+), 131 deletions(-)

--
2.33.1

[PATCH v4 1/6] media: max9286: Add support for v4l2_subdev_state

[Jacopo Mondi]

Create and initialize the v4l2_subdev_state for the max9286 driver in
order to prepare to support routing operations and multiplexed streams.

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_cfg() operation to guarantee the state is initialized
correctly with the default device format.

Remove the max9286_open() subdev internal operation as the format of the
file-handle state is now initialized by init_cfg().

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 89 +++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9e204aabf465..1b9ff537d08e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -214,6 +214,17 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
 	return container_of(sd, struct max9286_priv, sd);
 }
 
+static const struct v4l2_mbus_framefmt max9286_default_format = {
+	.width		= 1280,
+	.height		= 800,
+	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
+	.colorspace	= V4L2_COLORSPACE_SRGB,
+	.field		= V4L2_FIELD_NONE,
+	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+	.quantization	= V4L2_QUANTIZATION_DEFAULT,
+	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+};
+
 /* -----------------------------------------------------------------------------
  * I2C IO
  */
@@ -821,11 +832,45 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max9286_init_cfg(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state)
+{
+	struct v4l2_subdev_route routes[MAX9286_NUM_GMSL];
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_subdev_krouting routing;
+	struct max9286_source *source;
+	unsigned int num_routes = 0;
+	int ret;
+
+	/* Create a route for each enable source. */
+	for_each_source(priv, source) {
+		struct v4l2_subdev_route *route = &routes[num_routes++];
+		unsigned int idx = to_index(priv, source);
+
+		route->sink_pad = idx;
+		route->sink_stream = 0;
+		route->source_pad = MAX9286_SRC_PAD;
+		route->source_stream = idx;
+		route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
+	}
+
+	routing.num_routes = num_routes;
+	routing.routes = routes;
+
+	v4l2_subdev_lock_state(state);
+	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
+					       &max9286_default_format);
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static const struct v4l2_subdev_video_ops max9286_video_ops = {
 	.s_stream	= max9286_s_stream,
 };
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
+	.init_cfg	= max9286_init_cfg,
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= max9286_get_fmt,
 	.set_fmt	= max9286_set_fmt,
@@ -836,35 +881,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
 	.pad		= &max9286_pad_ops,
 };
 
-static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
-{
-	fmt->width		= 1280;
-	fmt->height		= 800;
-	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
-	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
-	fmt->field		= V4L2_FIELD_NONE;
-	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
-	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
-	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
-}
-
-static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
-{
-	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
-
-	for (i = 0; i < MAX9286_N_SINKS; i++) {
-		format = v4l2_subdev_get_try_format(subdev, fh->state, i);
-		max9286_init_format(format);
-	}
-
-	return 0;
-}
-
-static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
-	.open = max9286_open,
-};
-
 static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	switch (ctrl->id) {
@@ -896,11 +912,11 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	/* Configure V4L2 for the MAX9286 itself */
 
 	for (i = 0; i < MAX9286_N_SINKS; i++)
-		max9286_init_format(&priv->fmt[i]);
+		priv->fmt[i] = max9286_default_format;
 
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
-	priv->sd.internal_ops = &max9286_subdev_internal_ops;
-	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			  V4L2_SUBDEV_FL_MULTIPLEXED;
 
 	v4l2_ctrl_handler_init(&priv->ctrls, 1);
 	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
@@ -932,14 +948,20 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	}
 	priv->sd.fwnode = ep;
 
+	ret = v4l2_subdev_init_finalize(&priv->sd);
+	if (ret)
+		goto err_put_node;
+
 	ret = v4l2_async_register_subdev(&priv->sd);
 	if (ret < 0) {
 		dev_err(dev, "Unable to register subdevice\n");
-		goto err_put_node;
+		goto err_state_cleanup;
 	}
 
 	return 0;
 
+err_state_cleanup:
+	v4l2_subdev_cleanup(&priv->sd);
 err_put_node:
 	fwnode_handle_put(ep);
 err_async:
@@ -952,6 +974,7 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
 {
 	fwnode_handle_put(priv->sd.fwnode);
 	v4l2_async_unregister_subdev(&priv->sd);
+	v4l2_subdev_cleanup(&priv->sd);
 	max9286_v4l2_notifier_unregister(priv);
 }
 
-- 
2.33.1

Re: [PATCH v4 1/6] media: max9286: Add support for v4l2_subdev_state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:41PM +0100, Jacopo Mondi wrote:
> Create and initialize the v4l2_subdev_state for the max9286 driver in
> order to prepare to support routing operations and multiplexed streams.
> 
> Create the subdevice state with v4l2_subdev_init_finalize() and
> implement the init_cfg() operation to guarantee the state is initialized
> correctly with the default device format.
> 
> Remove the max9286_open() subdev internal operation as the format of the
> file-handle state is now initialized by init_cfg().
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/max9286.c | 89 +++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 9e204aabf465..1b9ff537d08e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -214,6 +214,17 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  	return container_of(sd, struct max9286_priv, sd);
>  }
>  
> +static const struct v4l2_mbus_framefmt max9286_default_format = {
> +	.width		= 1280,
> +	.height		= 800,
> +	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +	.colorspace	= V4L2_COLORSPACE_SRGB,
> +	.field		= V4L2_FIELD_NONE,
> +	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +	.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -821,11 +832,45 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int max9286_init_cfg(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_subdev_route routes[MAX9286_NUM_GMSL];
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	struct v4l2_subdev_krouting routing;
> +	struct max9286_source *source;
> +	unsigned int num_routes = 0;
> +	int ret;
> +
> +	/* Create a route for each enable source. */
> +	for_each_source(priv, source) {
> +		struct v4l2_subdev_route *route = &routes[num_routes++];
> +		unsigned int idx = to_index(priv, source);
> +
> +		route->sink_pad = idx;
> +		route->sink_stream = 0;
> +		route->source_pad = MAX9286_SRC_PAD;
> +		route->source_stream = idx;
> +		route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
> +	}
> +
> +	routing.num_routes = num_routes;
> +	routing.routes = routes;
> +
> +	v4l2_subdev_lock_state(state);
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> +					       &max9286_default_format);
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  	.s_stream	= max9286_s_stream,
>  };
>  
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> +	.init_cfg	= max9286_init_cfg,
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= max9286_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> @@ -836,35 +881,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
>  	.pad		= &max9286_pad_ops,
>  };
>  
> -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> -{
> -	fmt->width		= 1280;
> -	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
> -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> -	fmt->field		= V4L2_FIELD_NONE;
> -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> -}
> -
> -static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> -{
> -	struct v4l2_mbus_framefmt *format;
> -	unsigned int i;
> -
> -	for (i = 0; i < MAX9286_N_SINKS; i++) {
> -		format = v4l2_subdev_get_try_format(subdev, fh->state, i);
> -		max9286_init_format(format);
> -	}
> -
> -	return 0;
> -}
> -
> -static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> -	.open = max9286_open,
> -};
> -
>  static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	switch (ctrl->id) {
> @@ -896,11 +912,11 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	/* Configure V4L2 for the MAX9286 itself */
>  
>  	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		max9286_init_format(&priv->fmt[i]);
> +		priv->fmt[i] = max9286_default_format;
>  
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> -	priv->sd.internal_ops = &max9286_subdev_internal_ops;
> -	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			  V4L2_SUBDEV_FL_MULTIPLEXED;
>  
>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
>  	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> @@ -932,14 +948,20 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	}
>  	priv->sd.fwnode = ep;
>  
> +	ret = v4l2_subdev_init_finalize(&priv->sd);
> +	if (ret)
> +		goto err_put_node;
> +
>  	ret = v4l2_async_register_subdev(&priv->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to register subdevice\n");
> -		goto err_put_node;
> +		goto err_state_cleanup;
>  	}
>  
>  	return 0;
>  
> +err_state_cleanup:
> +	v4l2_subdev_cleanup(&priv->sd);
>  err_put_node:
>  	fwnode_handle_put(ep);
>  err_async:
> @@ -952,6 +974,7 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  {
>  	fwnode_handle_put(priv->sd.fwnode);
>  	v4l2_async_unregister_subdev(&priv->sd);
> +	v4l2_subdev_cleanup(&priv->sd);
>  	max9286_v4l2_notifier_unregister(priv);
>  }
>  
> -- 
> 2.33.1
> 

-- 
Regards,

Laurent Pinchart

[PATCH v4 2/6] media: max9286: Implement set_routing

[Jacopo Mondi]

Add the set_routing() subdev operation to allow userspace to configure
routing on the max9286 deserializer.

Implement route verification but do not take routing into consideration
when configuring the CSI-2 output and pixel rate yet.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 89 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1b9ff537d08e..eb76acdb2cd9 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -832,6 +832,91 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max9286_routing_validate(struct max9286_priv *priv,
+				    struct v4l2_subdev_krouting *routing)
+{
+	unsigned int i;
+	int ret;
+
+	ret = v4l2_subdev_routing_validate_1_to_1(routing);
+	if (ret)
+		return ret;
+
+	/*
+	 * Make sure all routes points to the single source pad which can have
+	 * up to 4 streams. All routes shall start from a sink pad and shall not
+	 * have more than one sink stream. The GMSL link for the sink has to be
+	 * enabled.
+	 */
+	for (i = 0; i < routing->num_routes; ++i) {
+		const struct v4l2_subdev_route *route = &routing->routes[i];
+		struct max9286_source *source = &priv->sources[i];
+
+		if (route->source_pad != MAX9286_SRC_PAD ||
+		    route->source_stream > 4) {
+			dev_err(&priv->client->dev,
+				"Invalid (%u,%u) source in route %u\n",
+				route->source_pad, route->source_stream, i);
+			return -EINVAL;
+		}
+
+		if (route->sink_pad >= MAX9286_N_SINKS ||
+		    route->sink_stream != 0) {
+			dev_err(&priv->client->dev,
+				"Invalid (%u,%u) sink in route %u\n",
+				route->sink_pad, route->sink_stream, i);
+			return -EINVAL;
+		}
+
+		source = &priv->sources[route->sink_pad];
+		if (!source->fwnode) {
+			dev_err(&priv->client->dev,
+				"Cannot set route for non-active source %u\n",
+				route->sink_pad);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int _max9286_set_routing(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_krouting *routing)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	int ret;
+
+	ret = max9286_routing_validate(priv, routing);
+	if (ret)
+		return ret;
+
+	/* Re-initialize the format on a routing change. */
+	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
+					       &max9286_default_format);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int max9286_set_routing(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_state *state,
+			       enum v4l2_subdev_format_whence which,
+			       struct v4l2_subdev_krouting *routing)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	unsigned int i;
+	int ret;
+
+	v4l2_subdev_lock_state(state);
+
+	ret = _max9286_set_routing(sd, state, routing);
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static int max9286_init_cfg(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *state)
 {
@@ -858,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
 	routing.routes = routes;
 
 	v4l2_subdev_lock_state(state);
-	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
-					       &max9286_default_format);
+	ret = _max9286_set_routing(sd, state, &routing);
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
@@ -874,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= max9286_get_fmt,
 	.set_fmt	= max9286_set_fmt,
+	.set_routing	= max9286_set_routing,
 };
 
 static const struct v4l2_subdev_ops max9286_subdev_ops = {
-- 
2.33.1

Re: [PATCH v4 2/6] media: max9286: Implement set_routing

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:42PM +0100, Jacopo Mondi wrote:
> Add the set_routing() subdev operation to allow userspace to configure
> routing on the max9286 deserializer.
> 
> Implement route verification but do not take routing into consideration
> when configuring the CSI-2 output and pixel rate yet.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 89 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1b9ff537d08e..eb76acdb2cd9 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -832,6 +832,91 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int max9286_routing_validate(struct max9286_priv *priv,
> +				    struct v4l2_subdev_krouting *routing)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	ret = v4l2_subdev_routing_validate_1_to_1(routing);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Make sure all routes points to the single source pad which can have
> +	 * up to 4 streams. All routes shall start from a sink pad and shall not
> +	 * have more than one sink stream. The GMSL link for the sink has to be
> +	 * enabled.
> +	 */
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		const struct v4l2_subdev_route *route = &routing->routes[i];
> +		struct max9286_source *source = &priv->sources[i];
> +
> +		if (route->source_pad != MAX9286_SRC_PAD ||
> +		    route->source_stream > 4) {
> +			dev_err(&priv->client->dev,
> +				"Invalid (%u,%u) source in route %u\n",
> +				route->source_pad, route->source_stream, i);
> +			return -EINVAL;
> +		}
> +
> +		if (route->sink_pad >= MAX9286_N_SINKS ||
> +		    route->sink_stream != 0) {
> +			dev_err(&priv->client->dev,
> +				"Invalid (%u,%u) sink in route %u\n",
> +				route->sink_pad, route->sink_stream, i);
> +			return -EINVAL;
> +		}
> +
> +		source = &priv->sources[route->sink_pad];
> +		if (!source->fwnode) {
> +			dev_err(&priv->client->dev,
> +				"Cannot set route for non-active source %u\n",
> +				route->sink_pad);
> +			return -EINVAL;
> +		}
> +	}

Apart from from possibly using the new helper I've submitted, this looks
fine.

> +
> +	return 0;
> +}
> +
> +static int _max9286_set_routing(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_krouting *routing)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	int ret;
> +
> +	ret = max9286_routing_validate(priv, routing);
> +	if (ret)
> +		return ret;
> +
> +	/* Re-initialize the format on a routing change. */
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
> +					       &max9286_default_format);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int max9286_set_routing(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *state,
> +			       enum v4l2_subdev_format_whence which,
> +			       struct v4l2_subdev_krouting *routing)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	unsigned int i;
> +	int ret;
> +
> +	v4l2_subdev_lock_state(state);
> +

I'd drop the blank line, or add one before the unlock call.

> +	ret = _max9286_set_routing(sd, state, routing);
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static int max9286_init_cfg(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *state)
>  {
> @@ -858,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
>  	routing.routes = routes;
>  
>  	v4l2_subdev_lock_state(state);
> -	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> -					       &max9286_default_format);
> +	ret = _max9286_set_routing(sd, state, &routing);
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;
> @@ -874,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= max9286_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> +	.set_routing	= max9286_set_routing,
>  };
>  
>  static const struct v4l2_subdev_ops max9286_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/6] media: max9286: Implement set_routing

[Laurent Pinchart]

Hi Jacopo,

Another comment.

On Fri, Dec 17, 2021 at 03:56:48AM +0200, Laurent Pinchart wrote:
> On Thu, Dec 16, 2021 at 06:47:42PM +0100, Jacopo Mondi wrote:
> > Add the set_routing() subdev operation to allow userspace to configure
> > routing on the max9286 deserializer.
> > 
> > Implement route verification but do not take routing into consideration
> > when configuring the CSI-2 output and pixel rate yet.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 89 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 1b9ff537d08e..eb76acdb2cd9 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -832,6 +832,91 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int max9286_routing_validate(struct max9286_priv *priv,
> > +				    struct v4l2_subdev_krouting *routing)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = v4l2_subdev_routing_validate_1_to_1(routing);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Make sure all routes points to the single source pad which can have
> > +	 * up to 4 streams. All routes shall start from a sink pad and shall not
> > +	 * have more than one sink stream. The GMSL link for the sink has to be
> > +	 * enabled.
> > +	 */
> > +	for (i = 0; i < routing->num_routes; ++i) {
> > +		const struct v4l2_subdev_route *route = &routing->routes[i];
> > +		struct max9286_source *source = &priv->sources[i];

There's no need to initialize source here.

> > +
> > +		if (route->source_pad != MAX9286_SRC_PAD ||
> > +		    route->source_stream > 4) {
> > +			dev_err(&priv->client->dev,
> > +				"Invalid (%u,%u) source in route %u\n",
> > +				route->source_pad, route->source_stream, i);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (route->sink_pad >= MAX9286_N_SINKS ||
> > +		    route->sink_stream != 0) {
> > +			dev_err(&priv->client->dev,
> > +				"Invalid (%u,%u) sink in route %u\n",
> > +				route->sink_pad, route->sink_stream, i);
> > +			return -EINVAL;
> > +		}
> > +
> > +		source = &priv->sources[route->sink_pad];
> > +		if (!source->fwnode) {
> > +			dev_err(&priv->client->dev,
> > +				"Cannot set route for non-active source %u\n",
> > +				route->sink_pad);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Apart from from possibly using the new helper I've submitted, this looks
> fine.

The helper should allow to drop the source_pad and sink_pad checks, but
the source_stream and sink_stream checks are still needed. This may be
possible to factorize in the helper too, let's see when we'll have more
use cases.

> > +
> > +	return 0;
> > +}
> > +
> > +static int _max9286_set_routing(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_krouting *routing)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +	int ret;
> > +
> > +	ret = max9286_routing_validate(priv, routing);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Re-initialize the format on a routing change. */
> > +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
> > +					       &max9286_default_format);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int max9286_set_routing(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_state *state,
> > +			       enum v4l2_subdev_format_whence which,
> > +			       struct v4l2_subdev_krouting *routing)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	v4l2_subdev_lock_state(state);
> > +
> 
> I'd drop the blank line, or add one before the unlock call.
> 
> > +	ret = _max9286_set_routing(sd, state, routing);
> > +	v4l2_subdev_unlock_state(state);
> > +
> > +	return ret;
> > +}
> > +
> >  static int max9286_init_cfg(struct v4l2_subdev *sd,
> >  			    struct v4l2_subdev_state *state)
> >  {
> > @@ -858,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
> >  	routing.routes = routes;
> >  
> >  	v4l2_subdev_lock_state(state);
> > -	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> > -					       &max9286_default_format);
> > +	ret = _max9286_set_routing(sd, state, &routing);
> >  	v4l2_subdev_unlock_state(state);
> >  
> >  	return ret;
> > @@ -874,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> >  	.enum_mbus_code = max9286_enum_mbus_code,
> >  	.get_fmt	= max9286_get_fmt,
> >  	.set_fmt	= max9286_set_fmt,
> > +	.set_routing	= max9286_set_routing,
> >  };
> >  
> >  static const struct v4l2_subdev_ops max9286_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/6] media: max9286: Implement set_routing

[Laurent Pinchart]

On Fri, Dec 17, 2021 at 04:18:51AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Another comment.
> 
> On Fri, Dec 17, 2021 at 03:56:48AM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 16, 2021 at 06:47:42PM +0100, Jacopo Mondi wrote:
> > > Add the set_routing() subdev operation to allow userspace to configure
> > > routing on the max9286 deserializer.
> > > 
> > > Implement route verification but do not take routing into consideration
> > > when configuring the CSI-2 output and pixel rate yet.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/max9286.c | 89 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 87 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 1b9ff537d08e..eb76acdb2cd9 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -832,6 +832,91 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >  
> > > +static int max9286_routing_validate(struct max9286_priv *priv,
> > > +				    struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	ret = v4l2_subdev_routing_validate_1_to_1(routing);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Make sure all routes points to the single source pad which can have
> > > +	 * up to 4 streams. All routes shall start from a sink pad and shall not
> > > +	 * have more than one sink stream. The GMSL link for the sink has to be
> > > +	 * enabled.
> > > +	 */
> > > +	for (i = 0; i < routing->num_routes; ++i) {
> > > +		const struct v4l2_subdev_route *route = &routing->routes[i];
> > > +		struct max9286_source *source = &priv->sources[i];
> 
> There's no need to initialize source here.
> 
> > > +
> > > +		if (route->source_pad != MAX9286_SRC_PAD ||
> > > +		    route->source_stream > 4) {
> > > +			dev_err(&priv->client->dev,
> > > +				"Invalid (%u,%u) source in route %u\n",
> > > +				route->source_pad, route->source_stream, i);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		if (route->sink_pad >= MAX9286_N_SINKS ||
> > > +		    route->sink_stream != 0) {
> > > +			dev_err(&priv->client->dev,
> > > +				"Invalid (%u,%u) sink in route %u\n",
> > > +				route->sink_pad, route->sink_stream, i);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		source = &priv->sources[route->sink_pad];
> > > +		if (!source->fwnode) {
> > > +			dev_err(&priv->client->dev,
> > > +				"Cannot set route for non-active source %u\n",
> > > +				route->sink_pad);
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > 
> > Apart from from possibly using the new helper I've submitted, this looks
> > fine.
> 
> The helper should allow to drop the source_pad and sink_pad checks, but
> the source_stream and sink_stream checks are still needed. This may be
> possible to factorize in the helper too, let's see when we'll have more
> use cases.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int _max9286_set_routing(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_state *state,
> > > +				struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +	int ret;
> > > +
> > > +	ret = max9286_routing_validate(priv, routing);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Re-initialize the format on a routing change. */
> > > +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
> > > +					       &max9286_default_format);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int max9286_set_routing(struct v4l2_subdev *sd,
> > > +			       struct v4l2_subdev_state *state,
> > > +			       enum v4l2_subdev_format_whence which,
> > > +			       struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +	unsigned int i;

priv and i are not used (they will be used in a patch later in the
series, so should be moved there).

> > > +	int ret;
> > > +
> > > +	v4l2_subdev_lock_state(state);
> > > +
> > 
> > I'd drop the blank line, or add one before the unlock call.
> > 
> > > +	ret = _max9286_set_routing(sd, state, routing);
> > > +	v4l2_subdev_unlock_state(state);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int max9286_init_cfg(struct v4l2_subdev *sd,
> > >  			    struct v4l2_subdev_state *state)
> > >  {
> > > @@ -858,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
> > >  	routing.routes = routes;
> > >  
> > >  	v4l2_subdev_lock_state(state);
> > > -	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> > > -					       &max9286_default_format);
> > > +	ret = _max9286_set_routing(sd, state, &routing);
> > >  	v4l2_subdev_unlock_state(state);

This is identical to max9286_set_routing(), you could call it directly,
and then merge _max9286_set_routing() and max9286_set_routing().

> > >  
> > >  	return ret;
> > > @@ -874,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > >  	.enum_mbus_code = max9286_enum_mbus_code,
> > >  	.get_fmt	= max9286_get_fmt,
> > >  	.set_fmt	= max9286_set_fmt,
> > > +	.set_routing	= max9286_set_routing,
> > >  };
> > >  
> > >  static const struct v4l2_subdev_ops max9286_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/6] media: max9286: Implement set_routing

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 04:24:09AM +0200, Laurent Pinchart wrote:
> On Fri, Dec 17, 2021 at 04:18:51AM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Another comment.
> >
> > On Fri, Dec 17, 2021 at 03:56:48AM +0200, Laurent Pinchart wrote:
> > > On Thu, Dec 16, 2021 at 06:47:42PM +0100, Jacopo Mondi wrote:
> > > > Add the set_routing() subdev operation to allow userspace to configure
> > > > routing on the max9286 deserializer.
> > > >
> > > > Implement route verification but do not take routing into consideration
> > > > when configuring the CSI-2 output and pixel rate yet.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 89 ++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 87 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index 1b9ff537d08e..eb76acdb2cd9 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -832,6 +832,91 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int max9286_routing_validate(struct max9286_priv *priv,
> > > > +				    struct v4l2_subdev_krouting *routing)
> > > > +{
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > > +	ret = v4l2_subdev_routing_validate_1_to_1(routing);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Make sure all routes points to the single source pad which can have
> > > > +	 * up to 4 streams. All routes shall start from a sink pad and shall not
> > > > +	 * have more than one sink stream. The GMSL link for the sink has to be
> > > > +	 * enabled.
> > > > +	 */
> > > > +	for (i = 0; i < routing->num_routes; ++i) {
> > > > +		const struct v4l2_subdev_route *route = &routing->routes[i];
> > > > +		struct max9286_source *source = &priv->sources[i];
> >
> > There's no need to initialize source here.
> >
> > > > +
> > > > +		if (route->source_pad != MAX9286_SRC_PAD ||
> > > > +		    route->source_stream > 4) {
> > > > +			dev_err(&priv->client->dev,
> > > > +				"Invalid (%u,%u) source in route %u\n",
> > > > +				route->source_pad, route->source_stream, i);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		if (route->sink_pad >= MAX9286_N_SINKS ||
> > > > +		    route->sink_stream != 0) {
> > > > +			dev_err(&priv->client->dev,
> > > > +				"Invalid (%u,%u) sink in route %u\n",
> > > > +				route->sink_pad, route->sink_stream, i);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		source = &priv->sources[route->sink_pad];
> > > > +		if (!source->fwnode) {
> > > > +			dev_err(&priv->client->dev,
> > > > +				"Cannot set route for non-active source %u\n",
> > > > +				route->sink_pad);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > >
> > > Apart from from possibly using the new helper I've submitted, this looks
> > > fine.
> >
> > The helper should allow to drop the source_pad and sink_pad checks, but
> > the source_stream and sink_stream checks are still needed. This may be
> > possible to factorize in the helper too, let's see when we'll have more
> > use cases.
> >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int _max9286_set_routing(struct v4l2_subdev *sd,
> > > > +				struct v4l2_subdev_state *state,
> > > > +				struct v4l2_subdev_krouting *routing)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +	int ret;
> > > > +
> > > > +	ret = max9286_routing_validate(priv, routing);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Re-initialize the format on a routing change. */
> > > > +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
> > > > +					       &max9286_default_format);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9286_set_routing(struct v4l2_subdev *sd,
> > > > +			       struct v4l2_subdev_state *state,
> > > > +			       enum v4l2_subdev_format_whence which,
> > > > +			       struct v4l2_subdev_krouting *routing)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +	unsigned int i;
>
> priv and i are not used (they will be used in a patch later in the
> series, so should be moved there).
>

Ah ups, I've not compiled this in isolation it seems

> > > > +	int ret;
> > > > +
> > > > +	v4l2_subdev_lock_state(state);
> > > > +
> > >
> > > I'd drop the blank line, or add one before the unlock call.
> > >
> > > > +	ret = _max9286_set_routing(sd, state, routing);
> > > > +	v4l2_subdev_unlock_state(state);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  static int max9286_init_cfg(struct v4l2_subdev *sd,
> > > >  			    struct v4l2_subdev_state *state)
> > > >  {
> > > > @@ -858,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
> > > >  	routing.routes = routes;
> > > >
> > > >  	v4l2_subdev_lock_state(state);
> > > > -	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> > > > -					       &max9286_default_format);
> > > > +	ret = _max9286_set_routing(sd, state, &routing);
> > > >  	v4l2_subdev_unlock_state(state);
>
> This is identical to max9286_set_routing(), you could call it directly,
> and then merge _max9286_set_routing() and max9286_set_routing().
>

meh, yes it is right now, but later max9286_set_routing() will also
configure the pixelrate. Also, max9286_set_routing() needs a whence,
something I don't have here.

I would keep it the way it is if that's ok for you.

Thanks
   j

> > > >
> > > >  	return ret;
> > > > @@ -874,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > >  	.enum_mbus_code = max9286_enum_mbus_code,
> > > >  	.get_fmt	= max9286_get_fmt,
> > > >  	.set_fmt	= max9286_set_fmt,
> > > > +	.set_routing	= max9286_set_routing,
> > > >  };
> > > >
> > > >  static const struct v4l2_subdev_ops max9286_subdev_ops = {
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v4 3/6] media: max9286: Use enabled routes to calculate pixelrate

[Jacopo Mondi]

Now that the device supports routing, use the enabled routes instead
of the connected sources to compute the output pixel rate.

To do so, update the route_mask after a set_routing() call with the
ACTIVE format, and re-calculate the pixel rate just after.

At the same time, start and stop only the enabled routes at s_stream()
time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 63 ++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index eb76acdb2cd9..1395eb783dc0 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -188,8 +188,11 @@ struct max9286_priv {
 	struct v4l2_async_notifier notifier;
 };
 
+#define to_index(priv, source) ((source) - &(priv)->sources[0])
+
 static struct max9286_source *next_source(struct max9286_priv *priv,
-					  struct max9286_source *source)
+					  struct max9286_source *source,
+					  bool routed)
 {
 	if (!source)
 		source = &priv->sources[0];
@@ -197,17 +200,27 @@ static struct max9286_source *next_source(struct max9286_priv *priv,
 		source++;
 
 	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
-		if (source->fwnode)
+		unsigned int index = to_index(priv, source);
+
+		if (!source->fwnode)
+			continue;
+
+		/*
+		 * Careful here! A very unfortunate call to set_routing() can
+		 * change priv->route_mask behind our back!
+		 */
+		if (!routed || priv->route_mask & BIT(index))
 			return source;
 	}
 
 	return NULL;
 }
 
-#define for_each_source(priv, source) \
-	for ((source) = NULL; ((source) = next_source((priv), (source))); )
+#define for_each_route(priv, source) \
+	for ((source) = NULL; ((source) = next_source((priv), (source), true)); )
 
-#define to_index(priv, source) ((source) - &(priv)->sources[0])
+#define for_each_source(priv, source) \
+	for ((source) = NULL; ((source) = next_source((priv), (source), false)); )
 
 static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
 {
@@ -409,7 +422,7 @@ static int max9286_check_video_links(struct max9286_priv *priv)
 		if (ret < 0)
 			return -EIO;
 
-		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
+		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->route_mask)
 			break;
 
 		usleep_range(350, 500);
@@ -493,9 +506,10 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 static int max9286_set_pixelrate(struct max9286_priv *priv)
 {
 	struct max9286_source *source = NULL;
+	unsigned int num_routes = 0;
 	u64 pixelrate = 0;
 
-	for_each_source(priv, source) {
+	for_each_route(priv, source) {
 		struct v4l2_ctrl *ctrl;
 		u64 source_rate = 0;
 
@@ -516,6 +530,8 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
 				"Unable to calculate pixel rate\n");
 			return -EINVAL;
 		}
+
+		num_routes++;
 	}
 
 	if (!pixelrate) {
@@ -529,7 +545,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
 	 * by the number of available sources.
 	 */
 	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
-				      pixelrate * priv->nsources);
+				      pixelrate * num_routes);
 }
 
 static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
@@ -691,8 +707,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		 */
 		max9286_i2c_mux_open(priv);
 
-		/* Start all cameras. */
-		for_each_source(priv, source) {
+		/* Start cameras. */
+		for_each_route(priv, source) {
 			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
 			if (ret)
 				return ret;
@@ -733,8 +749,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	} else {
 		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
 
-		/* Stop all cameras. */
-		for_each_source(priv, source)
+		/* Stop cameras. */
+		for_each_route(priv, source)
 			v4l2_subdev_call(source->sd, video, s_stream, 0);
 
 		max9286_i2c_mux_close(priv);
@@ -912,6 +928,29 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
 	v4l2_subdev_lock_state(state);
 
 	ret = _max9286_set_routing(sd, state, routing);
+	if (ret)
+		goto out;
+
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		goto out;
+
+	/*
+	 * Update the active routes mask and the pixel rate according to the
+	 * routed sources.
+	 */
+	priv->route_mask = 0;
+	for (i = 0; i < routing->num_routes; ++i) {
+		struct v4l2_subdev_route *route = &routing->routes[i];
+
+		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
+			continue;
+
+		priv->route_mask |= BIT(route->sink_pad);
+	}
+
+	max9286_set_pixelrate(priv);
+
+out:
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
-- 
2.33.1

Re: [PATCH v4 3/6] media: max9286: Use enabled routes to calculate pixelrate

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:43PM +0100, Jacopo Mondi wrote:
> Now that the device supports routing, use the enabled routes instead
> of the connected sources to compute the output pixel rate.
> 
> To do so, update the route_mask after a set_routing() call with the
> ACTIVE format, and re-calculate the pixel rate just after.
> 
> At the same time, start and stop only the enabled routes at s_stream()
> time.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 63 ++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index eb76acdb2cd9..1395eb783dc0 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -188,8 +188,11 @@ struct max9286_priv {
>  	struct v4l2_async_notifier notifier;
>  };
>  
> +#define to_index(priv, source) ((source) - &(priv)->sources[0])
> +
>  static struct max9286_source *next_source(struct max9286_priv *priv,
> -					  struct max9286_source *source)
> +					  struct max9286_source *source,
> +					  bool routed)
>  {
>  	if (!source)
>  		source = &priv->sources[0];
> @@ -197,17 +200,27 @@ static struct max9286_source *next_source(struct max9286_priv *priv,
>  		source++;
>  
>  	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
> -		if (source->fwnode)
> +		unsigned int index = to_index(priv, source);
> +
> +		if (!source->fwnode)
> +			continue;
> +
> +		/*
> +		 * Careful here! A very unfortunate call to set_routing() can
> +		 * change priv->route_mask behind our back!
> +		 */

This seems error-prone. I wonder if we could instead remove route_mask
completely, and compute it as .s_stream(1) time by walking the routes
from the state. That would avoid storing state information in
max9286_priv.

The downside is that we'll have to pass the route mask down to functions
called by max9286_s_stream(). At some point in the future, I'd like to
see support for subclassing the subdev state in subdev drivers, at which
point we will be able to cache the route_mask in the subclassed state
when configuring routing, and reusing the cached value when starting the
streams.

> +		if (!routed || priv->route_mask & BIT(index))
>  			return source;
>  	}
>  
>  	return NULL;
>  }
>  
> -#define for_each_source(priv, source) \
> -	for ((source) = NULL; ((source) = next_source((priv), (source))); )
> +#define for_each_route(priv, source) \
> +	for ((source) = NULL; ((source) = next_source((priv), (source), true)); )
>  
> -#define to_index(priv, source) ((source) - &(priv)->sources[0])
> +#define for_each_source(priv, source) \
> +	for ((source) = NULL; ((source) = next_source((priv), (source), false)); )
>  
>  static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  {
> @@ -409,7 +422,7 @@ static int max9286_check_video_links(struct max9286_priv *priv)
>  		if (ret < 0)
>  			return -EIO;
>  
> -		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
> +		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->route_mask)
>  			break;
>  
>  		usleep_range(350, 500);
> @@ -493,9 +506,10 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  static int max9286_set_pixelrate(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source = NULL;
> +	unsigned int num_routes = 0;
>  	u64 pixelrate = 0;
>  
> -	for_each_source(priv, source) {
> +	for_each_route(priv, source) {
>  		struct v4l2_ctrl *ctrl;
>  		u64 source_rate = 0;
>  
> @@ -516,6 +530,8 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
>  				"Unable to calculate pixel rate\n");
>  			return -EINVAL;
>  		}
> +
> +		num_routes++;
>  	}
>  
>  	if (!pixelrate) {
> @@ -529,7 +545,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
>  	 * by the number of available sources.
>  	 */
>  	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
> -				      pixelrate * priv->nsources);
> +				      pixelrate * num_routes);
>  }
>  
>  static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -691,8 +707,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		 */
>  		max9286_i2c_mux_open(priv);
>  
> -		/* Start all cameras. */
> -		for_each_source(priv, source) {
> +		/* Start cameras. */
> +		for_each_route(priv, source) {
>  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
>  			if (ret)
>  				return ret;
> @@ -733,8 +749,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	} else {
>  		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
>  
> -		/* Stop all cameras. */
> -		for_each_source(priv, source)
> +		/* Stop cameras. */
> +		for_each_route(priv, source)
>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>  
>  		max9286_i2c_mux_close(priv);
> @@ -912,6 +928,29 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
>  	v4l2_subdev_lock_state(state);
>  
>  	ret = _max9286_set_routing(sd, state, routing);
> +	if (ret)
> +		goto out;
> +
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		goto out;
> +
> +	/*
> +	 * Update the active routes mask and the pixel rate according to the
> +	 * routed sources.
> +	 */
> +	priv->route_mask = 0;
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		struct v4l2_subdev_route *route = &routing->routes[i];
> +
> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> +			continue;
> +
> +		priv->route_mask |= BIT(route->sink_pad);
> +	}
> +
> +	max9286_set_pixelrate(priv);
> +
> +out:
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/6] media: max9286: Use enabled routes to calculate pixelrate

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 04:29:50AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Dec 16, 2021 at 06:47:43PM +0100, Jacopo Mondi wrote:
> > Now that the device supports routing, use the enabled routes instead
> > of the connected sources to compute the output pixel rate.
> >
> > To do so, update the route_mask after a set_routing() call with the
> > ACTIVE format, and re-calculate the pixel rate just after.
> >
> > At the same time, start and stop only the enabled routes at s_stream()
> > time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 63 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 51 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index eb76acdb2cd9..1395eb783dc0 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -188,8 +188,11 @@ struct max9286_priv {
> >  	struct v4l2_async_notifier notifier;
> >  };
> >
> > +#define to_index(priv, source) ((source) - &(priv)->sources[0])
> > +
> >  static struct max9286_source *next_source(struct max9286_priv *priv,
> > -					  struct max9286_source *source)
> > +					  struct max9286_source *source,
> > +					  bool routed)
> >  {
> >  	if (!source)
> >  		source = &priv->sources[0];
> > @@ -197,17 +200,27 @@ static struct max9286_source *next_source(struct max9286_priv *priv,
> >  		source++;
> >
> >  	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
> > -		if (source->fwnode)
> > +		unsigned int index = to_index(priv, source);
> > +
> > +		if (!source->fwnode)
> > +			continue;
> > +
> > +		/*
> > +		 * Careful here! A very unfortunate call to set_routing() can
> > +		 * change priv->route_mask behind our back!
> > +		 */
>
> This seems error-prone. I wonder if we could instead remove route_mask
> completely, and compute it as .s_stream(1) time by walking the routes
> from the state. That would avoid storing state information in
> max9286_priv.

I wish I could, but I need an up-to-date pixelrate for the CSI-2
receiver, before it calls into s_stream(1) :(

The race window is very small (I know, if things can go wrong, they
will..) and I could protect it with a mutex ?

>
> The downside is that we'll have to pass the route mask down to functions
> called by max9286_s_stream(). At some point in the future, I'd like to
> see support for subclassing the subdev state in subdev drivers, at which
> point we will be able to cache the route_mask in the subclassed state
> when configuring routing, and reusing the cached value when starting the
> streams.
>

With the state being locked, this would prevent any race from
happening, that's right!

What about a mutex for the time being ?

> > +		if (!routed || priv->route_mask & BIT(index))
> >  			return source;
> >  	}
> >
> >  	return NULL;
> >  }
> >
> > -#define for_each_source(priv, source) \
> > -	for ((source) = NULL; ((source) = next_source((priv), (source))); )
> > +#define for_each_route(priv, source) \
> > +	for ((source) = NULL; ((source) = next_source((priv), (source), true)); )
> >
> > -#define to_index(priv, source) ((source) - &(priv)->sources[0])
> > +#define for_each_source(priv, source) \
> > +	for ((source) = NULL; ((source) = next_source((priv), (source), false)); )
> >
> >  static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
> >  {
> > @@ -409,7 +422,7 @@ static int max9286_check_video_links(struct max9286_priv *priv)
> >  		if (ret < 0)
> >  			return -EIO;
> >
> > -		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
> > +		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->route_mask)
> >  			break;
> >
> >  		usleep_range(350, 500);
> > @@ -493,9 +506,10 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> >  static int max9286_set_pixelrate(struct max9286_priv *priv)
> >  {
> >  	struct max9286_source *source = NULL;
> > +	unsigned int num_routes = 0;
> >  	u64 pixelrate = 0;
> >
> > -	for_each_source(priv, source) {
> > +	for_each_route(priv, source) {
> >  		struct v4l2_ctrl *ctrl;
> >  		u64 source_rate = 0;
> >
> > @@ -516,6 +530,8 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
> >  				"Unable to calculate pixel rate\n");
> >  			return -EINVAL;
> >  		}
> > +
> > +		num_routes++;
> >  	}
> >
> >  	if (!pixelrate) {
> > @@ -529,7 +545,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
> >  	 * by the number of available sources.
> >  	 */
> >  	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
> > -				      pixelrate * priv->nsources);
> > +				      pixelrate * num_routes);
> >  }
> >
> >  static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > @@ -691,8 +707,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  		 */
> >  		max9286_i2c_mux_open(priv);
> >
> > -		/* Start all cameras. */
> > -		for_each_source(priv, source) {
> > +		/* Start cameras. */
> > +		for_each_route(priv, source) {
> >  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
> >  			if (ret)
> >  				return ret;
> > @@ -733,8 +749,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	} else {
> >  		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> >
> > -		/* Stop all cameras. */
> > -		for_each_source(priv, source)
> > +		/* Stop cameras. */
> > +		for_each_route(priv, source)
> >  			v4l2_subdev_call(source->sd, video, s_stream, 0);
> >
> >  		max9286_i2c_mux_close(priv);
> > @@ -912,6 +928,29 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
> >  	v4l2_subdev_lock_state(state);
> >
> >  	ret = _max9286_set_routing(sd, state, routing);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> > +		goto out;
> > +
> > +	/*
> > +	 * Update the active routes mask and the pixel rate according to the
> > +	 * routed sources.
> > +	 */
> > +	priv->route_mask = 0;
> > +	for (i = 0; i < routing->num_routes; ++i) {
> > +		struct v4l2_subdev_route *route = &routing->routes[i];
> > +
> > +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > +			continue;
> > +
> > +		priv->route_mask |= BIT(route->sink_pad);
> > +	}
> > +
> > +	max9286_set_pixelrate(priv);
> > +
> > +out:
> >  	v4l2_subdev_unlock_state(state);
> >
> >  	return ret;
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v4 4/6] media: max9286: Use routes to configure link order

[Jacopo Mondi]

Use the routing table to configure the link output order and link
masking.

The link output order defines the CSI-2 virtual channel a GSML stream
is output on. Configure ordering at stream start time and at chip
setup time. This last step requires to move the chip initialization
function after the V4L2 setup phase as it requires the subdev state from
where the routing table is retrieved from to be initialized.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1395eb783dc0..5d728fa23f01 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -499,6 +499,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 	return 0;
 }
 
+/*
+ * Configure the links output order (which defines on which CSI-2 VC a
+ * link is output on) and configure link masking.
+ */
+static void max9286_config_links(struct max9286_priv *priv)
+{
+	const struct v4l2_subdev_krouting *routing;
+	struct v4l2_subdev_state *state;
+	u8 link_order = 0;
+	u8 vc_mask = 0xf;
+	unsigned int i;
+
+	state = v4l2_subdev_lock_active_state(&priv->sd);
+	routing = &state->routing;
+
+	for (i = 0; i < routing->num_routes; ++i) {
+		struct v4l2_subdev_route *route = &routing->routes[i];
+
+		if (!(priv->route_mask & BIT(i)))
+			continue;
+
+		/* Assign the CSI-2 VC using the source stream number. */
+		link_order |= route->source_stream << (2 * route->sink_pad);
+		vc_mask &= ~BIT(route->source_stream);
+	}
+
+	/*
+	 * This might look rather silly, but now that we have assigned a
+	 * VC to the enabled routes, we have to assign one to the disabled
+	 * routes as well, as there cannot be two sources with the same VC.
+	 */
+	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
+		unsigned int vc;
+
+		if (priv->route_mask & BIT(i))
+			continue;
+
+		/* ffs() counts from 1. */
+		vc = ffs(vc_mask) - 1;
+		link_order |= vc << (2 * i);
+		vc_mask &= ~BIT(vc);
+	}
+
+	/*
+	 * Use the enabled routes to enable GMSL links, configure the CSI-2
+	 * output order, mask unused links and autodetect link used as CSI
+	 * clock source.
+	 */
+	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
+	max9286_write(priv, 0x0b, link_order);
+	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
+
+	v4l2_subdev_unlock_state(state);
+}
+
 /* -----------------------------------------------------------------------------
  * V4L2 Subdev
  */
@@ -700,6 +755,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		max9286_config_links(priv);
+
 		/*
 		 * The frame sync between cameras is transmitted across the
 		 * reverse channel as GPIO. We must open all channels while
@@ -1108,32 +1165,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
 
 static int max9286_setup(struct max9286_priv *priv)
 {
-	/*
-	 * Link ordering values for all enabled links combinations. Orders must
-	 * be assigned sequentially from 0 to the number of enabled links
-	 * without leaving any hole for disabled links. We thus assign orders to
-	 * enabled links first, and use the remaining order values for disabled
-	 * links are all links must have a different order value;
-	 */
-	static const u8 link_order[] = {
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
-		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
-		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
-		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
-		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
-		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
-		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
-		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
-		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
-		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
-		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
-		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
-		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
-	};
-
 	/*
 	 * Set the I2C bus speed.
 	 *
@@ -1143,13 +1174,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_configure_i2c(priv, true);
 	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 
-	/*
-	 * Enable GMSL links, mask unused ones and autodetect link
-	 * used as CSI clock source.
-	 */
-	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
-	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
-	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
+	max9286_config_links(priv);
 
 	/*
 	 * Video format setup:
@@ -1321,12 +1346,6 @@ static int max9286_init(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = max9286_setup(priv);
-	if (ret) {
-		dev_err(dev, "Unable to setup max9286\n");
-		goto err_poc_disable;
-	}
-
 	/*
 	 * Register all V4L2 interactions for the MAX9286 and notifiers for
 	 * any subdevices connected.
@@ -1337,6 +1356,12 @@ static int max9286_init(struct device *dev)
 		goto err_poc_disable;
 	}
 
+	ret = max9286_setup(priv);
+	if (ret) {
+		dev_err(dev, "Unable to setup max9286\n");
+		goto err_poc_disable;
+	}
+
 	ret = max9286_i2c_mux_init(priv);
 	if (ret) {
 		dev_err(dev, "Unable to initialize I2C multiplexer\n");
-- 
2.33.1

Re: [PATCH v4 4/6] media: max9286: Use routes to configure link order

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:44PM +0100, Jacopo Mondi wrote:
> Use the routing table to configure the link output order and link
> masking.
> 
> The link output order defines the CSI-2 virtual channel a GSML stream
> is output on. Configure ordering at stream start time and at chip
> setup time. This last step requires to move the chip initialization
> function after the V4L2 setup phase as it requires the subdev state from
> where the routing table is retrieved from to be initialized.

I wonder if we could drop some of the configuration from
max9286_setup().

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1395eb783dc0..5d728fa23f01 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -499,6 +499,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>  
> +/*
> + * Configure the links output order (which defines on which CSI-2 VC a
> + * link is output on) and configure link masking.
> + */
> +static void max9286_config_links(struct max9286_priv *priv)
> +{
> +	const struct v4l2_subdev_krouting *routing;
> +	struct v4l2_subdev_state *state;
> +	u8 link_order = 0;
> +	u8 vc_mask = 0xf;
> +	unsigned int i;
> +
> +	state = v4l2_subdev_lock_active_state(&priv->sd);
> +	routing = &state->routing;
> +
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		struct v4l2_subdev_route *route = &routing->routes[i];
> +
> +		if (!(priv->route_mask & BIT(i)))
> +			continue;

Is this check needed ?

> +
> +		/* Assign the CSI-2 VC using the source stream number. */
> +		link_order |= route->source_stream << (2 * route->sink_pad);
> +		vc_mask &= ~BIT(route->source_stream);
> +	}
> +
> +	/*
> +	 * This might look rather silly, but now that we have assigned a
> +	 * VC to the enabled routes, we have to assign one to the disabled
> +	 * routes as well, as there cannot be two sources with the same VC.
> +	 */
> +	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
> +		unsigned int vc;
> +
> +		if (priv->route_mask & BIT(i))
> +			continue;
> +
> +		/* ffs() counts from 1. */
> +		vc = ffs(vc_mask) - 1;

You can use __ffs(), which counts from 0 (but has an undefined behaviour
when no bit is set).

> +		link_order |= vc << (2 * i);
> +		vc_mask &= ~BIT(vc);
> +	}
> +
> +	/*
> +	 * Use the enabled routes to enable GMSL links, configure the CSI-2
> +	 * output order, mask unused links and autodetect link used as CSI
> +	 * clock source.
> +	 */
> +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> +	max9286_write(priv, 0x0b, link_order);
> +	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
> +
> +	v4l2_subdev_unlock_state(state);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdev
>   */
> @@ -700,6 +755,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>  
>  	if (enable) {
> +		max9286_config_links(priv);
> +
>  		/*
>  		 * The frame sync between cameras is transmitted across the
>  		 * reverse channel as GPIO. We must open all channels while
> @@ -1108,32 +1165,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  
>  static int max9286_setup(struct max9286_priv *priv)
>  {
> -	/*
> -	 * Link ordering values for all enabled links combinations. Orders must
> -	 * be assigned sequentially from 0 to the number of enabled links
> -	 * without leaving any hole for disabled links. We thus assign orders to
> -	 * enabled links first, and use the remaining order values for disabled
> -	 * links are all links must have a different order value;
> -	 */
> -	static const u8 link_order[] = {
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
> -		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
> -		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
> -		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
> -		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
> -		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
> -		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
> -		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
> -		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
> -		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
> -		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
> -		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
> -		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
> -	};
> -
>  	/*
>  	 * Set the I2C bus speed.
>  	 *
> @@ -1143,13 +1174,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_configure_i2c(priv, true);
>  	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  
> -	/*
> -	 * Enable GMSL links, mask unused ones and autodetect link
> -	 * used as CSI clock source.
> -	 */
> -	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> -	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> -	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> +	max9286_config_links(priv);
>  
>  	/*
>  	 * Video format setup:
> @@ -1321,12 +1346,6 @@ static int max9286_init(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = max9286_setup(priv);
> -	if (ret) {
> -		dev_err(dev, "Unable to setup max9286\n");
> -		goto err_poc_disable;
> -	}
> -
>  	/*
>  	 * Register all V4L2 interactions for the MAX9286 and notifiers for
>  	 * any subdevices connected.
> @@ -1337,6 +1356,12 @@ static int max9286_init(struct device *dev)
>  		goto err_poc_disable;
>  	}
>  
> +	ret = max9286_setup(priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to setup max9286\n");
> +		goto err_poc_disable;
> +	}
> +
>  	ret = max9286_i2c_mux_init(priv);
>  	if (ret) {
>  		dev_err(dev, "Unable to initialize I2C multiplexer\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/6] media: max9286: Use routes to configure link order

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 04:35:19AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Dec 16, 2021 at 06:47:44PM +0100, Jacopo Mondi wrote:
> > Use the routing table to configure the link output order and link
> > masking.
> >
> > The link output order defines the CSI-2 virtual channel a GSML stream
> > is output on. Configure ordering at stream start time and at chip
> > setup time. This last step requires to move the chip initialization
> > function after the V4L2 setup phase as it requires the subdev state from
> > where the routing table is retrieved from to be initialized.
>
> I wonder if we could drop some of the configuration from
> max9286_setup().

Do you mean move it to a later time ?

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
> >  1 file changed, 64 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 1395eb783dc0..5d728fa23f01 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -499,6 +499,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> >  	return 0;
> >  }
> >
> > +/*
> > + * Configure the links output order (which defines on which CSI-2 VC a
> > + * link is output on) and configure link masking.
> > + */
> > +static void max9286_config_links(struct max9286_priv *priv)
> > +{
> > +	const struct v4l2_subdev_krouting *routing;
> > +	struct v4l2_subdev_state *state;
> > +	u8 link_order = 0;
> > +	u8 vc_mask = 0xf;
> > +	unsigned int i;
> > +
> > +	state = v4l2_subdev_lock_active_state(&priv->sd);
> > +	routing = &state->routing;
> > +
> > +	for (i = 0; i < routing->num_routes; ++i) {
> > +		struct v4l2_subdev_route *route = &routing->routes[i];
> > +
> > +		if (!(priv->route_mask & BIT(i)))
> > +			continue;
>
> Is this check needed ?
>

It is, I need to first assign a VC to the enabled routes.
I could replace it with for_each_active_route() probably!

> > +
> > +		/* Assign the CSI-2 VC using the source stream number. */
> > +		link_order |= route->source_stream << (2 * route->sink_pad);
> > +		vc_mask &= ~BIT(route->source_stream);
> > +	}
> > +
> > +	/*
> > +	 * This might look rather silly, but now that we have assigned a
> > +	 * VC to the enabled routes, we have to assign one to the disabled
> > +	 * routes as well, as there cannot be two sources with the same VC.
> > +	 */
> > +	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
> > +		unsigned int vc;
> > +
> > +		if (priv->route_mask & BIT(i))
> > +			continue;
> > +
> > +		/* ffs() counts from 1. */
> > +		vc = ffs(vc_mask) - 1;
>
> You can use __ffs(), which counts from 0 (but has an undefined behaviour
> when no bit is set).

That's why I didn't want to use it :)

>
> > +		link_order |= vc << (2 * i);
> > +		vc_mask &= ~BIT(vc);
> > +	}
> > +
> > +	/*
> > +	 * Use the enabled routes to enable GMSL links, configure the CSI-2
> > +	 * output order, mask unused links and autodetect link used as CSI
> > +	 * clock source.
> > +	 */
> > +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > +	max9286_write(priv, 0x0b, link_order);
> > +	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
> > +
> > +	v4l2_subdev_unlock_state(state);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 Subdev
> >   */
> > @@ -700,6 +755,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	int ret;
> >
> >  	if (enable) {
> > +		max9286_config_links(priv);
> > +
> >  		/*
> >  		 * The frame sync between cameras is transmitted across the
> >  		 * reverse channel as GPIO. We must open all channels while
> > @@ -1108,32 +1165,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
> >
> >  static int max9286_setup(struct max9286_priv *priv)
> >  {
> > -	/*
> > -	 * Link ordering values for all enabled links combinations. Orders must
> > -	 * be assigned sequentially from 0 to the number of enabled links
> > -	 * without leaving any hole for disabled links. We thus assign orders to
> > -	 * enabled links first, and use the remaining order values for disabled
> > -	 * links are all links must have a different order value;
> > -	 */
> > -	static const u8 link_order[] = {
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
> > -		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
> > -		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
> > -		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
> > -		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
> > -		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
> > -		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
> > -		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
> > -		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
> > -		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
> > -		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
> > -		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
> > -		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
> > -	};
> > -
> >  	/*
> >  	 * Set the I2C bus speed.
> >  	 *
> > @@ -1143,13 +1174,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	max9286_configure_i2c(priv, true);
> >  	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
> >
> > -	/*
> > -	 * Enable GMSL links, mask unused ones and autodetect link
> > -	 * used as CSI clock source.
> > -	 */
> > -	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > -	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> > -	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > +	max9286_config_links(priv);
> >
> >  	/*
> >  	 * Video format setup:
> > @@ -1321,12 +1346,6 @@ static int max9286_init(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = max9286_setup(priv);
> > -	if (ret) {
> > -		dev_err(dev, "Unable to setup max9286\n");
> > -		goto err_poc_disable;
> > -	}
> > -
> >  	/*
> >  	 * Register all V4L2 interactions for the MAX9286 and notifiers for
> >  	 * any subdevices connected.
> > @@ -1337,6 +1356,12 @@ static int max9286_init(struct device *dev)
> >  		goto err_poc_disable;
> >  	}
> >
> > +	ret = max9286_setup(priv);
> > +	if (ret) {
> > +		dev_err(dev, "Unable to setup max9286\n");
> > +		goto err_poc_disable;
> > +	}
> > +
> >  	ret = max9286_i2c_mux_init(priv);
> >  	if (ret) {
> >  		dev_err(dev, "Unable to initialize I2C multiplexer\n");
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v4 5/6] media: max9286: Move format to subdev state

[Jacopo Mondi]

Move format handling to the v4l2_subdev state and store it per
(pad, stream) combination.

Now that the image format is stored in the subdev state, it can be
accessed through v4l2_subdev_get_fmt() instead of open-coding it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 86 ++++++++++++-------------------------
 1 file changed, 27 insertions(+), 59 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 5d728fa23f01..aa7cb7c10fc0 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -174,8 +174,6 @@ struct max9286_priv {
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
 
-	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
-
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
 
@@ -828,28 +826,17 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-max9286_get_pad_format(struct max9286_priv *priv,
-		       struct v4l2_subdev_state *sd_state,
-		       unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &priv->fmt[pad];
-	default:
-		return NULL;
-	}
-}
-
 static int max9286_set_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
+			   struct v4l2_subdev_state *state,
 			   struct v4l2_subdev_format *format)
 {
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
 
+	/*
+	 * Refuse to set format on the multiplexed source pad.
+	 * Format is propagated from sinks streams to source streams.
+	 */
 	if (format->pad == MAX9286_SRC_PAD)
 		return -EINVAL;
 
@@ -865,44 +852,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 		break;
 	}
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
-					 format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	*cfg_fmt = format->format;
-	mutex_unlock(&priv->mutex);
-
-	return 0;
-}
-
-static int max9286_get_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
-			   struct v4l2_subdev_format *format)
-{
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
-	unsigned int pad = format->pad;
-
-	/*
-	 * Multiplexed Stream Support: Support link validation by returning the
-	 * format of the first bound link. All links must have the same format,
-	 * as we do not support mixing and matching of cameras connected to the
-	 * max9286.
-	 */
-	if (pad == MAX9286_SRC_PAD)
-		pad = __ffs(priv->bound_sources);
+	v4l2_subdev_lock_state(state);
+	fmt = v4l2_state_get_stream_format(state, format->pad,
+					   format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
+	/* Propagate format to the other end of the route. */
+	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
+							   format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-	mutex_lock(&priv->mutex);
-	format->format = *cfg_fmt;
-	mutex_unlock(&priv->mutex);
+out:
+	v4l2_subdev_unlock_state(state);
 
-	return 0;
+	return ret;
 }
 
 static int max9286_routing_validate(struct max9286_priv *priv,
@@ -1052,7 +1023,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.init_cfg	= max9286_init_cfg,
 	.enum_mbus_code = max9286_enum_mbus_code,
-	.get_fmt	= max9286_get_fmt,
+	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
 	.set_routing	= max9286_set_routing,
 };
@@ -1092,9 +1063,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 
 	/* Configure V4L2 for the MAX9286 itself */
 
-	for (i = 0; i < MAX9286_N_SINKS; i++)
-		priv->fmt[i] = max9286_default_format;
-
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 			  V4L2_SUBDEV_FL_MULTIPLEXED;
-- 
2.33.1

Re: [PATCH v4 5/6] media: max9286: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:45PM +0100, Jacopo Mondi wrote:
> Move format handling to the v4l2_subdev state and store it per
> (pad, stream) combination.
> 
> Now that the image format is stored in the subdev state, it can be
> accessed through v4l2_subdev_get_fmt() instead of open-coding it.

It could still be good to move this to the beginning of the series, in
order to merge the patch with 01/38 to 06/38 from the muxed streams
series (which will be submitted standalone in a v11).

This patch looks good to me otherwise.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 86 ++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 5d728fa23f01..aa7cb7c10fc0 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -174,8 +174,6 @@ struct max9286_priv {
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
>  
> -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> -
>  	/* Protects controls and fmt structures */
>  	struct mutex mutex;
>  
> @@ -828,28 +826,17 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -max9286_get_pad_format(struct max9286_priv *priv,
> -		       struct v4l2_subdev_state *sd_state,
> -		       unsigned int pad, u32 which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &priv->fmt[pad];
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int max9286_set_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_state *state,
>  			   struct v4l2_subdev_format *format)
>  {
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
>  
> +	/*
> +	 * Refuse to set format on the multiplexed source pad.
> +	 * Format is propagated from sinks streams to source streams.
> +	 */
>  	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>  
> @@ -865,44 +852,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	}
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> -					 format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	*cfg_fmt = format->format;
> -	mutex_unlock(&priv->mutex);
> -
> -	return 0;
> -}
> -
> -static int max9286_get_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> -			   struct v4l2_subdev_format *format)
> -{
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> -	unsigned int pad = format->pad;
> -
> -	/*
> -	 * Multiplexed Stream Support: Support link validation by returning the
> -	 * format of the first bound link. All links must have the same format,
> -	 * as we do not support mixing and matching of cameras connected to the
> -	 * max9286.
> -	 */
> -	if (pad == MAX9286_SRC_PAD)
> -		pad = __ffs(priv->bound_sources);
> +	v4l2_subdev_lock_state(state);
> +	fmt = v4l2_state_get_stream_format(state, format->pad,
> +					   format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> +	/* Propagate format to the other end of the route. */
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> +							   format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	mutex_lock(&priv->mutex);
> -	format->format = *cfg_fmt;
> -	mutex_unlock(&priv->mutex);
> +out:
> +	v4l2_subdev_unlock_state(state);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int max9286_routing_validate(struct max9286_priv *priv,
> @@ -1052,7 +1023,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.init_cfg	= max9286_init_cfg,
>  	.enum_mbus_code = max9286_enum_mbus_code,
> -	.get_fmt	= max9286_get_fmt,
> +	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
>  	.set_routing	= max9286_set_routing,
>  };
> @@ -1092,9 +1063,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  
>  	/* Configure V4L2 for the MAX9286 itself */
>  
> -	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		priv->fmt[i] = max9286_default_format;
> -
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  			  V4L2_SUBDEV_FL_MULTIPLEXED;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/6] media: max9286: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 04:40:26AM +0200, Laurent Pinchart wrote:
> On Thu, Dec 16, 2021 at 06:47:45PM +0100, Jacopo Mondi wrote:
> > Move format handling to the v4l2_subdev state and store it per
> > (pad, stream) combination.
> > 
> > Now that the image format is stored in the subdev state, it can be
> > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> 
> It could still be good to move this to the beginning of the series, in
> order to merge the patch with 01/38 to 06/38 from the muxed streams
> series (which will be submitted standalone in a v11).
> 
> This patch looks good to me otherwise.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Not quite yet actually :-)

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 86 ++++++++++++-------------------------
> >  1 file changed, 27 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 5d728fa23f01..aa7cb7c10fc0 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -174,8 +174,6 @@ struct max9286_priv {
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate;
> >  
> > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > -
> >  	/* Protects controls and fmt structures */
> >  	struct mutex mutex;
> >  
> > @@ -828,28 +826,17 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -static struct v4l2_mbus_framefmt *
> > -max9286_get_pad_format(struct max9286_priv *priv,
> > -		       struct v4l2_subdev_state *sd_state,
> > -		       unsigned int pad, u32 which)
> > -{
> > -	switch (which) {
> > -	case V4L2_SUBDEV_FORMAT_TRY:
> > -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > -		return &priv->fmt[pad];
> > -	default:
> > -		return NULL;
> > -	}
> > -}
> > -
> >  static int max9286_set_fmt(struct v4l2_subdev *sd,
> > -			   struct v4l2_subdev_state *sd_state,
> > +			   struct v4l2_subdev_state *state,
> >  			   struct v4l2_subdev_format *format)
> >  {
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > +	struct v4l2_mbus_framefmt *fmt;
> > +	int ret = 0;
> >  
> > +	/*
> > +	 * Refuse to set format on the multiplexed source pad.
> > +	 * Format is propagated from sinks streams to source streams.
> > +	 */
> >  	if (format->pad == MAX9286_SRC_PAD)
> >  		return -EINVAL;

We should return v4l2_subdev_get_fmt() now.

> >  
> > @@ -865,44 +852,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  		break;
> >  	}
> >  
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> > -					 format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&priv->mutex);
> > -	*cfg_fmt = format->format;
> > -	mutex_unlock(&priv->mutex);
> > -
> > -	return 0;
> > -}
> > -
> > -static int max9286_get_fmt(struct v4l2_subdev *sd,
> > -			   struct v4l2_subdev_state *sd_state,
> > -			   struct v4l2_subdev_format *format)
> > -{
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > -	unsigned int pad = format->pad;
> > -
> > -	/*
> > -	 * Multiplexed Stream Support: Support link validation by returning the
> > -	 * format of the first bound link. All links must have the same format,
> > -	 * as we do not support mixing and matching of cameras connected to the
> > -	 * max9286.
> > -	 */
> > -	if (pad == MAX9286_SRC_PAD)
> > -		pad = __ffs(priv->bound_sources);
> > +	v4l2_subdev_lock_state(state);
> > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > +					   format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> >  
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > +	/* Propagate format to the other end of the route. */
> > +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > +							   format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> >  
> > -	mutex_lock(&priv->mutex);
> > -	format->format = *cfg_fmt;
> > -	mutex_unlock(&priv->mutex);
> > +out:
> > +	v4l2_subdev_unlock_state(state);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int max9286_routing_validate(struct max9286_priv *priv,
> > @@ -1052,7 +1023,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> >  	.init_cfg	= max9286_init_cfg,
> >  	.enum_mbus_code = max9286_enum_mbus_code,
> > -	.get_fmt	= max9286_get_fmt,
> > +	.get_fmt	= v4l2_subdev_get_fmt,
> >  	.set_fmt	= max9286_set_fmt,
> >  	.set_routing	= max9286_set_routing,
> >  };
> > @@ -1092,9 +1063,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >  
> >  	/* Configure V4L2 for the MAX9286 itself */
> >  
> > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > -		priv->fmt[i] = max9286_default_format;
> > -
> >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> >  			  V4L2_SUBDEV_FL_MULTIPLEXED;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/6] media: max9286: Move format to subdev state

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 05:09:39AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Dec 17, 2021 at 04:40:26AM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 16, 2021 at 06:47:45PM +0100, Jacopo Mondi wrote:
> > > Move format handling to the v4l2_subdev state and store it per
> > > (pad, stream) combination.
> > >
> > > Now that the image format is stored in the subdev state, it can be
> > > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> >
> > It could still be good to move this to the beginning of the series, in
> > order to merge the patch with 01/38 to 06/38 from the muxed streams
> > series (which will be submitted standalone in a v11).
> >
> > This patch looks good to me otherwise.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Not quite yet actually :-)
>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/max9286.c | 86 ++++++++++++-------------------------
> > >  1 file changed, 27 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 5d728fa23f01..aa7cb7c10fc0 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -174,8 +174,6 @@ struct max9286_priv {
> > >  	struct v4l2_ctrl_handler ctrls;
> > >  	struct v4l2_ctrl *pixelrate;
> > >
> > > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > -
> > >  	/* Protects controls and fmt structures */
> > >  	struct mutex mutex;
> > >
> > > @@ -828,28 +826,17 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >
> > > -static struct v4l2_mbus_framefmt *
> > > -max9286_get_pad_format(struct max9286_priv *priv,
> > > -		       struct v4l2_subdev_state *sd_state,
> > > -		       unsigned int pad, u32 which)
> > > -{
> > > -	switch (which) {
> > > -	case V4L2_SUBDEV_FORMAT_TRY:
> > > -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> > > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > -		return &priv->fmt[pad];
> > > -	default:
> > > -		return NULL;
> > > -	}
> > > -}
> > > -
> > >  static int max9286_set_fmt(struct v4l2_subdev *sd,
> > > -			   struct v4l2_subdev_state *sd_state,
> > > +			   struct v4l2_subdev_state *state,
> > >  			   struct v4l2_subdev_format *format)
> > >  {
> > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > > +	struct v4l2_mbus_framefmt *fmt;
> > > +	int ret = 0;
> > >
> > > +	/*
> > > +	 * Refuse to set format on the multiplexed source pad.
> > > +	 * Format is propagated from sinks streams to source streams.
> > > +	 */
> > >  	if (format->pad == MAX9286_SRC_PAD)
> > >  		return -EINVAL;
>
> We should return v4l2_subdev_get_fmt() now.
>

I might have missed the reason.

The current documentation says

EINVAL
The struct v4l2_subdev_format pad references a non-existing pad,

Should this be changd to "non-existing or invalid pad" ?

> > >
> > > @@ -865,44 +852,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> > >  		break;
> > >  	}
> > >
> > > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> > > -					 format->which);
> > > -	if (!cfg_fmt)
> > > -		return -EINVAL;
> > > -
> > > -	mutex_lock(&priv->mutex);
> > > -	*cfg_fmt = format->format;
> > > -	mutex_unlock(&priv->mutex);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > -			   struct v4l2_subdev_state *sd_state,
> > > -			   struct v4l2_subdev_format *format)
> > > -{
> > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > > -	unsigned int pad = format->pad;
> > > -
> > > -	/*
> > > -	 * Multiplexed Stream Support: Support link validation by returning the
> > > -	 * format of the first bound link. All links must have the same format,
> > > -	 * as we do not support mixing and matching of cameras connected to the
> > > -	 * max9286.
> > > -	 */
> > > -	if (pad == MAX9286_SRC_PAD)
> > > -		pad = __ffs(priv->bound_sources);
> > > +	v4l2_subdev_lock_state(state);
> > > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > > +					   format->stream);
> > > +	if (!fmt) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +	*fmt = format->format;
> > >
> > > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> > > -	if (!cfg_fmt)
> > > -		return -EINVAL;
> > > +	/* Propagate format to the other end of the route. */
> > > +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > > +							   format->stream);
> > > +	if (!fmt) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +	*fmt = format->format;
> > >
> > > -	mutex_lock(&priv->mutex);
> > > -	format->format = *cfg_fmt;
> > > -	mutex_unlock(&priv->mutex);
> > > +out:
> > > +	v4l2_subdev_unlock_state(state);
> > >
> > > -	return 0;
> > > +	return ret;
> > >  }
> > >
> > >  static int max9286_routing_validate(struct max9286_priv *priv,
> > > @@ -1052,7 +1023,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > >  	.init_cfg	= max9286_init_cfg,
> > >  	.enum_mbus_code = max9286_enum_mbus_code,
> > > -	.get_fmt	= max9286_get_fmt,
> > > +	.get_fmt	= v4l2_subdev_get_fmt,
> > >  	.set_fmt	= max9286_set_fmt,
> > >  	.set_routing	= max9286_set_routing,
> > >  };
> > > @@ -1092,9 +1063,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> > >
> > >  	/* Configure V4L2 for the MAX9286 itself */
> > >
> > > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > > -		priv->fmt[i] = max9286_default_format;
> > > -
> > >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> > >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > >  			  V4L2_SUBDEV_FL_MULTIPLEXED;
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v4 5/6] media: max9286: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 03:11:28PM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 05:09:39AM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 17, 2021 at 04:40:26AM +0200, Laurent Pinchart wrote:
> > > On Thu, Dec 16, 2021 at 06:47:45PM +0100, Jacopo Mondi wrote:
> > > > Move format handling to the v4l2_subdev state and store it per
> > > > (pad, stream) combination.
> > > >
> > > > Now that the image format is stored in the subdev state, it can be
> > > > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> > >
> > > It could still be good to move this to the beginning of the series, in
> > > order to merge the patch with 01/38 to 06/38 from the muxed streams
> > > series (which will be submitted standalone in a v11).
> > >
> > > This patch looks good to me otherwise.
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Not quite yet actually :-)
> >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 86 ++++++++++++-------------------------
> > > >  1 file changed, 27 insertions(+), 59 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index 5d728fa23f01..aa7cb7c10fc0 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -174,8 +174,6 @@ struct max9286_priv {
> > > >  	struct v4l2_ctrl_handler ctrls;
> > > >  	struct v4l2_ctrl *pixelrate;
> > > >
> > > > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > > -
> > > >  	/* Protects controls and fmt structures */
> > > >  	struct mutex mutex;
> > > >
> > > > @@ -828,28 +826,17 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > > >  	return 0;
> > > >  }
> > > >
> > > > -static struct v4l2_mbus_framefmt *
> > > > -max9286_get_pad_format(struct max9286_priv *priv,
> > > > -		       struct v4l2_subdev_state *sd_state,
> > > > -		       unsigned int pad, u32 which)
> > > > -{
> > > > -	switch (which) {
> > > > -	case V4L2_SUBDEV_FORMAT_TRY:
> > > > -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> > > > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > > -		return &priv->fmt[pad];
> > > > -	default:
> > > > -		return NULL;
> > > > -	}
> > > > -}
> > > > -
> > > >  static int max9286_set_fmt(struct v4l2_subdev *sd,
> > > > -			   struct v4l2_subdev_state *sd_state,
> > > > +			   struct v4l2_subdev_state *state,
> > > >  			   struct v4l2_subdev_format *format)
> > > >  {
> > > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > > > +	struct v4l2_mbus_framefmt *fmt;
> > > > +	int ret = 0;
> > > >
> > > > +	/*
> > > > +	 * Refuse to set format on the multiplexed source pad.
> > > > +	 * Format is propagated from sinks streams to source streams.
> > > > +	 */
> > > >  	if (format->pad == MAX9286_SRC_PAD)
> > > >  		return -EINVAL;
> >
> > We should return v4l2_subdev_get_fmt() now.
> 
> I might have missed the reason.
> 
> The current documentation says
> 
> EINVAL
> The struct v4l2_subdev_format pad references a non-existing pad,
> 
> Should this be changd to "non-existing or invalid pad" ?

I don't think so. Setting the format is meant to be valid on any pad,
even pads that have a fixed format. "Non-existing" seems enough to me.
What would you envision "invalid pad" to cover ?

> > > >
> > > > @@ -865,44 +852,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> > > >  		break;
> > > >  	}
> > > >
> > > > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> > > > -					 format->which);
> > > > -	if (!cfg_fmt)
> > > > -		return -EINVAL;
> > > > -
> > > > -	mutex_lock(&priv->mutex);
> > > > -	*cfg_fmt = format->format;
> > > > -	mutex_unlock(&priv->mutex);
> > > > -
> > > > -	return 0;
> > > > -}
> > > > -
> > > > -static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > > -			   struct v4l2_subdev_state *sd_state,
> > > > -			   struct v4l2_subdev_format *format)
> > > > -{
> > > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > > > -	unsigned int pad = format->pad;
> > > > -
> > > > -	/*
> > > > -	 * Multiplexed Stream Support: Support link validation by returning the
> > > > -	 * format of the first bound link. All links must have the same format,
> > > > -	 * as we do not support mixing and matching of cameras connected to the
> > > > -	 * max9286.
> > > > -	 */
> > > > -	if (pad == MAX9286_SRC_PAD)
> > > > -		pad = __ffs(priv->bound_sources);
> > > > +	v4l2_subdev_lock_state(state);
> > > > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > > > +					   format->stream);
> > > > +	if (!fmt) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +	*fmt = format->format;
> > > >
> > > > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> > > > -	if (!cfg_fmt)
> > > > -		return -EINVAL;
> > > > +	/* Propagate format to the other end of the route. */
> > > > +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > > > +							   format->stream);
> > > > +	if (!fmt) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +	*fmt = format->format;
> > > >
> > > > -	mutex_lock(&priv->mutex);
> > > > -	format->format = *cfg_fmt;
> > > > -	mutex_unlock(&priv->mutex);
> > > > +out:
> > > > +	v4l2_subdev_unlock_state(state);
> > > >
> > > > -	return 0;
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static int max9286_routing_validate(struct max9286_priv *priv,
> > > > @@ -1052,7 +1023,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > >  	.init_cfg	= max9286_init_cfg,
> > > >  	.enum_mbus_code = max9286_enum_mbus_code,
> > > > -	.get_fmt	= max9286_get_fmt,
> > > > +	.get_fmt	= v4l2_subdev_get_fmt,
> > > >  	.set_fmt	= max9286_set_fmt,
> > > >  	.set_routing	= max9286_set_routing,
> > > >  };
> > > > @@ -1092,9 +1063,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> > > >
> > > >  	/* Configure V4L2 for the MAX9286 itself */
> > > >
> > > > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > > > -		priv->fmt[i] = max9286_default_format;
> > > > -
> > > >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> > > >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > >  			  V4L2_SUBDEV_FL_MULTIPLEXED;

-- 
Regards,

Laurent Pinchart

[PATCH v4 6/6] media: max9286: Implement get_frame_desc()

[Jacopo Mondi]

Implement the get_frame_desc pad operation to allow retrieving the
stream configuration of the max9286 subdevice.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 53 +++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index aa7cb7c10fc0..78988f2bdf91 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -876,6 +876,58 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int max9286_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				  struct v4l2_mbus_frame_desc *fd)
+{
+	struct v4l2_subdev_route *route;
+	struct v4l2_subdev_state *state;
+	int ret = 0;
+
+	if (pad != MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_active_state(sd);
+
+	memset(fd, 0, sizeof(*fd));
+
+	/* One stream entry per each connected route. */
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_frame_desc_entry *entry =
+						&fd->entry[fd->num_entries];
+		struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_state_get_stream_format(state, pad,
+						   route->source_stream);
+		if (!fmt) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/*
+		 * Assume a YUYV format (0x1e DT) and 16 bpp: we only support
+		 * these formats at the moment.
+		 */
+		entry->stream = fd->num_entries++;
+		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
+		entry->length = fmt->width * fmt->height * 16 / 8;
+		entry->pixelcode = fmt->code;
+
+		/*
+		 * The source stream id corresponds to the virtual channel a
+		 * stream is output on.
+		 */
+		entry->bus.csi2.vc = route->source_stream;
+		entry->bus.csi2.dt = 0x1e;
+	}
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static int max9286_routing_validate(struct max9286_priv *priv,
 				    struct v4l2_subdev_krouting *routing)
 {
@@ -1025,6 +1077,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
+	.get_frame_desc = max9286_get_frame_desc,
 	.set_routing	= max9286_set_routing,
 };
 
-- 
2.33.1

Re: [PATCH v4 6/6] media: max9286: Implement get_frame_desc()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 16, 2021 at 06:47:46PM +0100, Jacopo Mondi wrote:
> Implement the get_frame_desc pad operation to allow retrieving the
> stream configuration of the max9286 subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 53 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index aa7cb7c10fc0..78988f2bdf91 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -876,6 +876,58 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static int max9286_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct v4l2_subdev_route *route;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	if (pad != MAX9286_SRC_PAD)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_active_state(sd);
> +
> +	memset(fd, 0, sizeof(*fd));
> +
> +	/* One stream entry per each connected route. */
> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *entry =
> +						&fd->entry[fd->num_entries];
> +		struct v4l2_mbus_framefmt *fmt;

This can be const.

> +
> +		fmt = v4l2_state_get_stream_format(state, pad,
> +						   route->source_stream);
> +		if (!fmt) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		/*
> +		 * Assume a YUYV format (0x1e DT) and 16 bpp: we only support
> +		 * these formats at the moment.
> +		 */
> +		entry->stream = fd->num_entries++;
> +		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +		entry->length = fmt->width * fmt->height * 16 / 8;
> +		entry->pixelcode = fmt->code;
> +
> +		/*
> +		 * The source stream id corresponds to the virtual channel a
> +		 * stream is output on.
> +		 */
> +		entry->bus.csi2.vc = route->source_stream;
> +		entry->bus.csi2.dt = 0x1e;

A header file for the CSI-2 data types could be useful (out of scope for
this series, unless you insist :-)).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	}
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +out:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static int max9286_routing_validate(struct max9286_priv *priv,
>  				    struct v4l2_subdev_krouting *routing)
>  {
> @@ -1025,6 +1077,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> +	.get_frame_desc = max9286_get_frame_desc,
>  	.set_routing	= max9286_set_routing,
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 6/6] media: max9286: Implement get_frame_desc()

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 04:45:02AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Dec 16, 2021 at 06:47:46PM +0100, Jacopo Mondi wrote:
> > Implement the get_frame_desc pad operation to allow retrieving the
> > stream configuration of the max9286 subdevice.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 53 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index aa7cb7c10fc0..78988f2bdf91 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -876,6 +876,58 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  	return ret;
> >  }
> >
> > +static int max9286_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +				  struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct v4l2_subdev_route *route;
> > +	struct v4l2_subdev_state *state;
> > +	int ret = 0;
> > +
> > +	if (pad != MAX9286_SRC_PAD)
> > +		return -EINVAL;
> > +
> > +	state = v4l2_subdev_lock_active_state(sd);
> > +
> > +	memset(fd, 0, sizeof(*fd));
> > +
> > +	/* One stream entry per each connected route. */
> > +	for_each_active_route(&state->routing, route) {
> > +		struct v4l2_mbus_frame_desc_entry *entry =
> > +						&fd->entry[fd->num_entries];
> > +		struct v4l2_mbus_framefmt *fmt;
>
> This can be const.
>

Indeed!

> > +
> > +		fmt = v4l2_state_get_stream_format(state, pad,
> > +						   route->source_stream);
> > +		if (!fmt) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * Assume a YUYV format (0x1e DT) and 16 bpp: we only support
> > +		 * these formats at the moment.
> > +		 */
> > +		entry->stream = fd->num_entries++;
> > +		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> > +		entry->length = fmt->width * fmt->height * 16 / 8;
> > +		entry->pixelcode = fmt->code;
> > +
> > +		/*
> > +		 * The source stream id corresponds to the virtual channel a
> > +		 * stream is output on.
> > +		 */
> > +		entry->bus.csi2.vc = route->source_stream;
> > +		entry->bus.csi2.dt = 0x1e;
>
> A header file for the CSI-2 data types could be useful (out of scope for
> this series, unless you insist :-)).

Nice try!

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
   j

>
> > +	}
> > +
> > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > +
> > +out:
> > +	v4l2_subdev_unlock_state(state);
> > +
> > +	return ret;
> > +}
> > +
> >  static int max9286_routing_validate(struct max9286_priv *priv,
> >  				    struct v4l2_subdev_krouting *routing)
> >  {
> > @@ -1025,6 +1077,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> >  	.enum_mbus_code = max9286_enum_mbus_code,
> >  	.get_fmt	= v4l2_subdev_get_fmt,
> >  	.set_fmt	= max9286_set_fmt,
> > +	.get_frame_desc = max9286_get_frame_desc,
> >  	.set_routing	= max9286_set_routing,
> >  };
> >
>
> --
> Regards,
>
> Laurent Pinchart