[v2,05/11] media: adv748x-csi2: Implement enum_mbus_codes

  Define a list of supported mbus codes for the TXA and TXB CSI-2
transmitters and implement the enum_mbus_code operation.

The TXB transmitter only support YUV422 while the TXA one supports
multiple formats as reported by the chip's manual in section 9.7.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
  

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> Define a list of supported mbus codes for the TXA and TXB CSI-2
> transmitters and implement the enum_mbus_code operation.
> 
> The TXB transmitter only support YUV422 while the TXA one supports
> multiple formats as reported by the chip's manual in section 9.7.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 5b265b722394..4fd6d3a681d5 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -14,6 +14,18 @@
>  
>  #include "adv748x.h"
>  
> +static const unsigned int adv748x_csi2_txa_fmts[] = {
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_UYVY10_1X20,
> +	MEDIA_BUS_FMT_RGB565_1X16,
> +	MEDIA_BUS_FMT_RGB666_1X18,
> +	MEDIA_BUS_FMT_RGB888_1X24,
> +};
> +
> +static const unsigned int adv748x_csi2_txb_fmts[] = {
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +};
> +
>  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
>  {
>  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
>   * But we must support setting the pad formats for format propagation.
>   */
>  
> +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> +				       struct v4l2_subdev_state *sd_state,
> +				       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +	const unsigned int *codes = is_txa(tx) ?
> +				    adv748x_csi2_txa_fmts :
> +				    adv748x_csi2_txb_fmts;
> +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> +
> +	if (code->pad != ADV748X_CSI2_SOURCE)
> +		return -EINVAL;

Any reason to not support enumeration of formats on the sink pad ?

Is the CSI-2 TX pass-through regarding media bus formats ? That is, does
it modify the format between the sink and source pads ? If not, I think
this function should be implemented as

	if (code->pad == ADV748X_CSI2_SINK) {
		if (code->index >= num_fmts)
			return -EINVAL;

		code->code = codes[code->index];
	} else {
		const struct v4l2_msbu_framefmt *fmt;

		if (code->index > 0)
			return -EINVAL;

		/*
		 * The device doesn't modify formats, the same media bus code is
		 * used on the sink and source.
		 */
		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
		code->code = fmt->code;
	}

> +
> +	if (code->index >= num_fmts)
> +		return -EINVAL;
> +
> +	code->code = codes[code->index];
> +
> +	return 0;
> +}
> +
>  static struct v4l2_mbus_framefmt *
>  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *sd_state,
> @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
>  }
>  
>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
>  	.get_mbus_config = adv748x_csi2_get_mbus_config,
  

Hi Laurent

On Thu, May 09, 2024 at 03:42:49PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> > Define a list of supported mbus codes for the TXA and TXB CSI-2
> > transmitters and implement the enum_mbus_code operation.
> >
> > The TXB transmitter only support YUV422 while the TXA one supports
> > multiple formats as reported by the chip's manual in section 9.7.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 5b265b722394..4fd6d3a681d5 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -14,6 +14,18 @@
> >
> >  #include "adv748x.h"
> >
> > +static const unsigned int adv748x_csi2_txa_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > +	MEDIA_BUS_FMT_UYVY10_1X20,
> > +	MEDIA_BUS_FMT_RGB565_1X16,
> > +	MEDIA_BUS_FMT_RGB666_1X18,
> > +	MEDIA_BUS_FMT_RGB888_1X24,
> > +};
> > +
> > +static const unsigned int adv748x_csi2_txb_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > +};
> > +
> >  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
> >  {
> >  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> > @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
> >   * But we must support setting the pad formats for format propagation.
> >   */
> >
> > +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> > +				       struct v4l2_subdev_state *sd_state,
> > +				       struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	const unsigned int *codes = is_txa(tx) ?
> > +				    adv748x_csi2_txa_fmts :
> > +				    adv748x_csi2_txb_fmts;
> > +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> > +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> > +
> > +	if (code->pad != ADV748X_CSI2_SOURCE)
> > +		return -EINVAL;
>
> Any reason to not support enumeration of formats on the sink pad ?
>
> it modify the format between the sink and source pads ? If not, I think
> this function should be implemented as
>
> 	if (code->pad == ADV748X_CSI2_SINK) {
> 		if (code->index >= num_fmts)
> 			return -EINVAL;
>
> 		code->code = codes[code->index];

I don't think this is correct. The formats I have listed in
adv748x_csi2_txa_fmts and adv748x_csi2_txb_fmts are the CSI-2 output
formats, not the ones accepted on the sink side of the CSI-2 TX

The CSI-2 TX sink pads connects to either the HDMI or AFE subdevices.
The media link represents the internal processing pipeline between the
two frontends and the TXes. The formats accepted on the TX sinks are
then the formats that can be produced by the HDMI/Analog sources the
adv748x is connected to ?

> 	} else {
> 		const struct v4l2_msbu_framefmt *fmt;
>
> 		if (code->index > 0)
> 			return -EINVAL;
>
> 		/*
> 		 * The device doesn't modify formats, the same media bus code is

At the same time the device seems capable of performing format
conversion, but the driver configures it in pass-through mode.

Now, given this configuration, it seems that whatever format is
produced by the HDMI/Analog front-end is reproduced on the CSI-2 Tx
source side. However the two frontends only list

static int adv748x_hdmi_enum_mbus_code(struct v4l2_subdev *sd,
				  struct v4l2_subdev_state *sd_state,
				  struct v4l2_subdev_mbus_code_enum *code)
{
	if (code->index != 0)
		return -EINVAL;

	code->code = MEDIA_BUS_FMT_RGB888_1X24;

	return 0;
}


static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
				      struct v4l2_subdev_state *sd_state,
				      struct v4l2_subdev_mbus_code_enum *code)
{
	if (code->index != 0)
		return -EINVAL;

	code->code = MEDIA_BUS_FMT_UYVY8_2X8;

	return 0;
}

While I presume many more formats would be possible.

In facts (for analog):
The video standards supported by the video processor include PAL B/PAL
D/PAL I/PAL G/PAL H, PAL 60, PAL M, PAL N, PAL Nc, NTSC M/NTSC J, NTSC
4.43, and SECAM B/SECAM D/SECAM G/SECAM K/SECAM L. The ADV748x can
automatically detect the input video standard and process it
accordingly.

I presume the HDMI standard support more formats than just RGB888 ?

So, as I was not sure on how to handle this, and enumerating formats
on the sink pads (which represent an internal bus connection) was of
little value, I decided to only allow format enumeration on the CSI-2
source pads, as the supported formats are well described by the chip
manual.

What do you think ?

> 		 * used on the sink and source.
> 		 */
> 		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
> 		code->code = fmt->code;
> 	}
>
> > +
> > +	if (code->index >= num_fmts)
> > +		return -EINVAL;
> > +
> > +	code->code = codes[code->index];
> > +
> > +	return 0;
> > +}
> > +
> >  static struct v4l2_mbus_framefmt *
> >  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
> >  			    struct v4l2_subdev_state *sd_state,
> > @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> >  }
> >
> >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
> >  	.get_fmt = adv748x_csi2_get_format,
> >  	.set_fmt = adv748x_csi2_set_format,
> >  	.get_mbus_config = adv748x_csi2_get_mbus_config,
>
> --
> Regards,
>
> Laurent Pinchart
  

Hi Jacopo,

On Thu, May 09, 2024 at 03:44:02PM +0200, Jacopo Mondi wrote:
> On Thu, May 09, 2024 at 03:42:49PM GMT, Laurent Pinchart wrote:
> > On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> > > Define a list of supported mbus codes for the TXA and TXB CSI-2
> > > transmitters and implement the enum_mbus_code operation.
> > >
> > > The TXB transmitter only support YUV422 while the TXA one supports
> > > multiple formats as reported by the chip's manual in section 9.7.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > index 5b265b722394..4fd6d3a681d5 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > @@ -14,6 +14,18 @@
> > >
> > >  #include "adv748x.h"
> > >
> > > +static const unsigned int adv748x_csi2_txa_fmts[] = {
> > > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > > +	MEDIA_BUS_FMT_UYVY10_1X20,
> > > +	MEDIA_BUS_FMT_RGB565_1X16,
> > > +	MEDIA_BUS_FMT_RGB666_1X18,
> > > +	MEDIA_BUS_FMT_RGB888_1X24,
> > > +};
> > > +
> > > +static const unsigned int adv748x_csi2_txb_fmts[] = {
> > > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > > +};
> > > +
> > >  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
> > >  {
> > >  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> > > @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
> > >   * But we must support setting the pad formats for format propagation.
> > >   */
> > >
> > > +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> > > +				       struct v4l2_subdev_state *sd_state,
> > > +				       struct v4l2_subdev_mbus_code_enum *code)
> > > +{
> > > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > +	const unsigned int *codes = is_txa(tx) ?
> > > +				    adv748x_csi2_txa_fmts :
> > > +				    adv748x_csi2_txb_fmts;
> > > +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> > > +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> > > +
> > > +	if (code->pad != ADV748X_CSI2_SOURCE)
> > > +		return -EINVAL;
> >
> > Any reason to not support enumeration of formats on the sink pad ?
> >
> > it modify the format between the sink and source pads ? If not, I think
> > this function should be implemented as
> >
> > 	if (code->pad == ADV748X_CSI2_SINK) {
> > 		if (code->index >= num_fmts)
> > 			return -EINVAL;
> >
> > 		code->code = codes[code->index];
> 
> I don't think this is correct. The formats I have listed in
> adv748x_csi2_txa_fmts and adv748x_csi2_txb_fmts are the CSI-2 output
> formats, not the ones accepted on the sink side of the CSI-2 TX

Ah OK.

> The CSI-2 TX sink pads connects to either the HDMI or AFE subdevices.
> The media link represents the internal processing pipeline between the
> two frontends and the TXes. The formats accepted on the TX sinks are
> then the formats that can be produced by the HDMI/Analog sources the
> adv748x is connected to ?

So the CSI-2 TX performs format conversion ? How about RGB <-> YUV
conversion, is that handled in the source (AFE and HDMI), or in the
CSI-2 TX ?

Let's first discuss what each subdev does, and then we'll look at the
implementation.

> > 	} else {
> > 		const struct v4l2_msbu_framefmt *fmt;
> >
> > 		if (code->index > 0)
> > 			return -EINVAL;
> >
> > 		/*
> > 		 * The device doesn't modify formats, the same media bus code is
> 
> At the same time the device seems capable of performing format
> conversion, but the driver configures it in pass-through mode.
> 
> Now, given this configuration, it seems that whatever format is
> produced by the HDMI/Analog front-end is reproduced on the CSI-2 Tx
> source side. However the two frontends only list
> 
> static int adv748x_hdmi_enum_mbus_code(struct v4l2_subdev *sd,
> 				  struct v4l2_subdev_state *sd_state,
> 				  struct v4l2_subdev_mbus_code_enum *code)
> {
> 	if (code->index != 0)
> 		return -EINVAL;
> 
> 	code->code = MEDIA_BUS_FMT_RGB888_1X24;
> 
> 	return 0;
> }
> 
> 
> static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
> 				      struct v4l2_subdev_state *sd_state,
> 				      struct v4l2_subdev_mbus_code_enum *code)
> {
> 	if (code->index != 0)
> 		return -EINVAL;
> 
> 	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> 
> 	return 0;
> }
> 
> While I presume many more formats would be possible.
> 
> In facts (for analog):
> The video standards supported by the video processor include PAL B/PAL
> D/PAL I/PAL G/PAL H, PAL 60, PAL M, PAL N, PAL Nc, NTSC M/NTSC J, NTSC
> 4.43, and SECAM B/SECAM D/SECAM G/SECAM K/SECAM L. The ADV748x can
> automatically detect the input video standard and process it
> accordingly.
> 
> I presume the HDMI standard support more formats than just RGB888 ?

Probably. I haven't checked the documentation.

> So, as I was not sure on how to handle this, and enumerating formats
> on the sink pads (which represent an internal bus connection) was of
> little value, I decided to only allow format enumeration on the CSI-2
> source pads, as the supported formats are well described by the chip
> manual.
> 
> What do you think ?

From a CSI-2 TX subdev point of view, the correct thing to do API-wise
is it

- enumerate on the sink pad all the possible formats that can be
  configured on that pad, regardless of how the connected source subdev
  is connected ; and

- enumerate on the source pad all the possible formats that can be
  output using the current sink format (obtained from the state).

We thus need

- a list of all the possible formats the CSI-2 TX can accept on its
  input ; and

- a way to convert those formats to CSI-2 TX output formats, *if* the
  CSI-2 TX supports format conversion.

In case the CSI-2 TX supports format conversion, but the driver doesn't
implement that yet, it's fine enumerating a single format on the source
pad, identical to the current format on the sink pad for now. This can
be extended to other formats in the future when we implement format
conversion in the driver.

> > 		 * used on the sink and source.
> > 		 */
> > 		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
> > 		code->code = fmt->code;
> > 	}
> >
> > > +
> > > +	if (code->index >= num_fmts)
> > > +		return -EINVAL;
> > > +
> > > +	code->code = codes[code->index];
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static struct v4l2_mbus_framefmt *
> > >  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
> > >  			    struct v4l2_subdev_state *sd_state,
> > > @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> > >  }
> > >
> > >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > > +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
> > >  	.get_fmt = adv748x_csi2_get_format,
> > >  	.set_fmt = adv748x_csi2_set_format,
> > >  	.get_mbus_config = adv748x_csi2_get_mbus_config,