Re: [PATCH 12/19] media: max9286: Implement .get_frame_desc()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 30, 2024 at 12:39:48PM +0200, Jacopo Mondi wrote:
> Implement the .get_frame_desc() pad operation to allow the receiver
> to retrieve information on the multiplexed source pad.
> 
> Record in the max9286_format_info structure the MIPI CSI-2
> data type and use it to populate the frame_desc_entry.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 120 ++++++++++++++++++++++++++++--------
>  1 file changed, 95 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index f203e4527257..4b4f4c03c10a 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -23,6 +23,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -145,7 +146,12 @@
>  
>  struct max9286_format_info {
>  	u32 code;
> -	u8 datatype;
> +	/* The gmsl data format configuration. */
> +	u8 gmsl_dt;
> +	/* The format bpp, used for stride calculation. */
> +	u8 bpp;
> +	/* The Data Type identifier as defined by the MIPI CSI-2 standard. */
> +	u8 mipi_dt;
>  };
>  
>  struct max9286_i2c_speed {
> @@ -235,28 +241,44 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  static const struct max9286_format_info max9286_formats[] = {
>  	{
>  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> -		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +		.gmsl_dt = MAX9286_DATATYPE_YUV422_8BIT,
> +		.bpp = 16,
> +		.mipi_dt = MIPI_CSI2_DT_YUV422_8B,
>  	}, {
>  		.code = MEDIA_BUS_FMT_VYUY8_1X16,
> -		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +		.gmsl_dt = MAX9286_DATATYPE_YUV422_8BIT,
> +		.bpp = 16,
> +		.mipi_dt = MIPI_CSI2_DT_YUV422_8B,
>  	}, {
>  		.code = MEDIA_BUS_FMT_YUYV8_1X16,
> -		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +		.gmsl_dt = MAX9286_DATATYPE_YUV422_8BIT,
> +		.bpp = 16,
> +		.mipi_dt = MIPI_CSI2_DT_YUV422_8B,
>  	}, {
>  		.code = MEDIA_BUS_FMT_YVYU8_1X16,
> -		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +		.gmsl_dt = MAX9286_DATATYPE_YUV422_8BIT,
> +		.bpp = 16,
> +		.mipi_dt = MIPI_CSI2_DT_YUV422_8B,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
> -		.datatype = MAX9286_DATATYPE_RAW12,
> +		.gmsl_dt = MAX9286_DATATYPE_RAW12,
> +		.bpp = 12,
> +		.mipi_dt = MIPI_CSI2_DT_RAW12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
> -		.datatype = MAX9286_DATATYPE_RAW12,
> +		.gmsl_dt = MAX9286_DATATYPE_RAW12,
> +		.bpp = 12,
> +		.mipi_dt = MIPI_CSI2_DT_RAW12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
> -		.datatype = MAX9286_DATATYPE_RAW12,
> +		.gmsl_dt = MAX9286_DATATYPE_RAW12,
> +		.bpp = 12,
> +		.mipi_dt = MIPI_CSI2_DT_RAW12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> -		.datatype = MAX9286_DATATYPE_RAW12,
> +		.gmsl_dt = MAX9286_DATATYPE_RAW12,
> +		.bpp = 12,
> +		.mipi_dt = MIPI_CSI2_DT_RAW12,
>  	},
>  };
>  
> @@ -532,19 +554,23 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>  
> +static const struct max9286_format_info *
> +max9286_get_format_info(unsigned int code)
> +{
> +	for (unsigned int i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> +		if (max9286_formats[i].code == code)
> +			return &max9286_formats[i];
> +	}
> +
> +	return NULL;
> +}
> +
>  static void max9286_set_video_format(struct max9286_priv *priv,
>  				     const struct v4l2_mbus_framefmt *format)
>  {
>  	const struct max9286_format_info *info = NULL;
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> -		if (max9286_formats[i].code == format->code) {
> -			info = &max9286_formats[i];
> -			break;
> -		}
> -	}
>  
> +	info = max9286_get_format_info(format->code);
>  	if (WARN_ON(!info))
>  		return;
>  
> @@ -559,7 +585,7 @@ static void max9286_set_video_format(struct max9286_priv *priv,
>  	/* Enable CSI-2 Lane D0-D3 only, DBL mode. */
>  	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
>  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> -		      info->datatype);
> +		      info->gmsl_dt);
>  
>  	/*
>  	 * Enable HS/VS encoding, use HS as line valid source, use D14/15 for
> @@ -900,7 +926,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_state *state,
>  			   struct v4l2_subdev_format *format)
>  {
> -	unsigned int i;
> +	const struct max9286_format_info *info;
>  
>  	/*
>  	 * Disable setting format on the source pad: format is propagated
> @@ -910,12 +936,8 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  		return -EINVAL;
>  
>  	/* Validate the format. */
> -	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> -		if (max9286_formats[i].code == format->format.code)
> -			break;
> -	}
> -
> -	if (i == ARRAY_SIZE(max9286_formats))
> +	info = max9286_get_format_info(format->format.code);
> +	if (!info)
>  		format->format.code = max9286_formats[0].code;
>  
>  	*v4l2_subdev_state_get_format(state, format->pad, 0) = format->format;
> @@ -930,6 +952,53 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +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;
> +	unsigned int num_routes = 0;
> +	int ret = 0;
> +
> +	if (pad != MAX9286_SRC_PAD)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);

Add a blank line.

> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *entry;
> +		const struct max9286_format_info *info;
> +		struct v4l2_mbus_framefmt *fmt;

const

> +
> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
> +						   route->sink_stream);
> +		info = max9286_get_format_info(fmt->code);
> +		if (WARN_ON(!info)) {

I don't think this can happen. You can drop the check and the err_unlock
label.

> +			ret = -EINVAL;
> +			goto err_unlock;
> +		}
> +
> +		entry = &fd->entry[num_routes];
> +		entry->stream = num_routes;
> +		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +		entry->length = fmt->width * fmt->height * info->bpp / 8;

As explained in the review of 07/19, you can drop flags and length (and
thus the bpp field from max9286_format_info).

> +		entry->pixelcode = fmt->code;
> +
> +		/* VC is set according to link ordering, see register 0x15. */
> +		entry->bus.csi2.vc = route->sink_pad;
> +		entry->bus.csi2.dt = info->mipi_dt;
> +
> +		num_routes++;
> +	}
> +
> +	fd->num_entries = num_routes;
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +err_unlock:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  	.s_stream	= max9286_s_stream,
>  };
> @@ -940,6 +1009,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.set_fmt	= max9286_set_fmt,
>  	.get_frame_interval = v4l2_subdev_get_frame_interval,
>  	.set_frame_interval = max9286_set_frame_interval,
> +	.get_frame_desc	= max9286_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops max9286_subdev_ops = {

-- 
Regards,

Laurent Pinchart