diff mbox series

[07/11] media: adv748x-csi2: Validate the image format

Message ID 20240503155127.105235-8-jacopo.mondi@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: renesas: rcar-csi2: Use the subdev active state | expand

Commit Message

Jacopo Mondi May 3, 2024, 3:51 p.m. UTC
The adv748x-csi2 driver configures the CSI-2 transmitter to
automatically infer the image stream format from the connected
frontend (HDMI or AFE).

Setting a new format on the subdevice hence does not actually control
the CSI-2 output format, but it's only there for the purpose of
pipeline validation.

However, there is currently no validation that the supplied media bus
code is valid and supported by the device.

With the introduction of enum_mbus_codes a list of supported format is
now available, use it to validate that the supplied format is correct
and use the default YUYV8 one if that's not the case.

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

Comments

Laurent Pinchart May 5, 2024, 9:09 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Fri, May 03, 2024 at 05:51:22PM +0200, Jacopo Mondi wrote:
> The adv748x-csi2 driver configures the CSI-2 transmitter to
> automatically infer the image stream format from the connected
> frontend (HDMI or AFE).
> 
> Setting a new format on the subdevice hence does not actually control
> the CSI-2 output format, but it's only there for the purpose of
> pipeline validation.
> 
> However, there is currently no validation that the supplied media bus
> code is valid and supported by the device.
> 
> With the introduction of enum_mbus_codes a list of supported format is
> now available, use it to validate that the supplied format is correct
> and use the default YUYV8 one if that's not the case.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 219417b319a6..1aae6467ca62 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> +					 unsigned int code)
> +{
> +	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);
> +
> +	for (unsigned int i = 0; i < num_fmts; i++) {
> +		if (codes[i] == code)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *sd_state,
>  				   struct v4l2_subdev_format *sdformat)
> @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	struct adv748x_state *state = tx->state;
>  	struct v4l2_mbus_framefmt *mbusformat;
> -	int ret = 0;
> +	int ret;
> +
> +	/*
> +	 * Make sure the format is supported, if not default it to
> +	 * YUYV8 as it's supported by both TXes.

As indicated in the review if 06/11, I think this should be UYVY8

> +	 */
> +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> +	if (ret)
> +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
>  
>  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
>  						 sdformat->which);
Niklas Söderlund May 6, 2024, 11:37 a.m. UTC | #2
Hi Jacopo,

Thanks for your work.

On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> The adv748x-csi2 driver configures the CSI-2 transmitter to
> automatically infer the image stream format from the connected
> frontend (HDMI or AFE).
> 
> Setting a new format on the subdevice hence does not actually control
> the CSI-2 output format, but it's only there for the purpose of
> pipeline validation.
> 
> However, there is currently no validation that the supplied media bus
> code is valid and supported by the device.
> 
> With the introduction of enum_mbus_codes a list of supported format is
> now available, use it to validate that the supplied format is correct
> and use the default YUYV8 one if that's not the case.

With the update tests for the change in patch 4 I hit multiple issues 
with this patch for CVBS capture.

> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 219417b319a6..1aae6467ca62 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> +					 unsigned int code)
> +{
> +	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);
> +
> +	for (unsigned int i = 0; i < num_fmts; i++) {
> +		if (codes[i] == code)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *sd_state,
>  				   struct v4l2_subdev_format *sdformat)
> @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	struct adv748x_state *state = tx->state;
>  	struct v4l2_mbus_framefmt *mbusformat;
> -	int ret = 0;
> +	int ret;
> +
> +	/*
> +	 * Make sure the format is supported, if not default it to
> +	 * YUYV8 as it's supported by both TXes.
> +	 */
> +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> +	if (ret)
> +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;

If adv748x_csi2_is_fmt_supported() returns non-zero you default to 
MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is 
propagated at the end of this function and to user-space falling the 
IOCTL.

Fixing that I hit another issue that kind of shows we need this format 
validation ;-)

The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE 
entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation 
in place it becomes impossible to connect AFE to TXB and breaking CBVS 
capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to 
TXB and I can again capture CVBS with patch 1-8 applied.

>  
>  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
>  						 sdformat->which);
> -- 
> 2.44.0
>
Jacopo Mondi May 6, 2024, 1:21 p.m. UTC | #3
Hi Niklas

On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > automatically infer the image stream format from the connected
> > frontend (HDMI or AFE).
> >
> > Setting a new format on the subdevice hence does not actually control
> > the CSI-2 output format, but it's only there for the purpose of
> > pipeline validation.
> >
> > However, there is currently no validation that the supplied media bus
> > code is valid and supported by the device.
> >
> > With the introduction of enum_mbus_codes a list of supported format is
> > now available, use it to validate that the supplied format is correct
> > and use the default YUYV8 one if that's not the case.
>
> With the update tests for the change in patch 4 I hit multiple issues
> with this patch for CVBS capture.
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 219417b319a6..1aae6467ca62 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > +					 unsigned int code)
> > +{
> > +	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);
> > +
> > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > +		if (codes[i] == code)
> > +			return 0;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> >  				   struct v4l2_subdev_state *sd_state,
> >  				   struct v4l2_subdev_format *sdformat)
> > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >  	struct adv748x_state *state = tx->state;
> >  	struct v4l2_mbus_framefmt *mbusformat;
> > -	int ret = 0;
> > +	int ret;
> > +
> > +	/*
> > +	 * Make sure the format is supported, if not default it to
> > +	 * YUYV8 as it's supported by both TXes.
> > +	 */
> > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > +	if (ret)
> > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
>
> If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> propagated at the end of this function and to user-space falling the
> IOCTL.

Ouch, I think in my tests the error message got ignored

>
> Fixing that I hit another issue that kind of shows we need this format
> validation ;-)
>
> The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> in place it becomes impossible to connect AFE to TXB and breaking CBVS
> capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> TXB and I can again capture CVBS with patch 1-8 applied.

Thanks for testing analog capture, I don't have a setup to easily do
so.

Should we make the AFE front-end produce 1X16 instead ? The format is
only used between the AFE and TXs after all, changing it shouldn't
have any implication on the interoperability with other devices.
>
> >
> >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> >  						 sdformat->which);
> > --
> > 2.44.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund
Niklas Söderlund May 6, 2024, 2:12 p.m. UTC | #4
Hi Jacopo,

On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> Hi Niklas
> 
> On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > automatically infer the image stream format from the connected
> > > frontend (HDMI or AFE).
> > >
> > > Setting a new format on the subdevice hence does not actually control
> > > the CSI-2 output format, but it's only there for the purpose of
> > > pipeline validation.
> > >
> > > However, there is currently no validation that the supplied media bus
> > > code is valid and supported by the device.
> > >
> > > With the introduction of enum_mbus_codes a list of supported format is
> > > now available, use it to validate that the supplied format is correct
> > > and use the default YUYV8 one if that's not the case.
> >
> > With the update tests for the change in patch 4 I hit multiple issues
> > with this patch for CVBS capture.
> >
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > index 219417b319a6..1aae6467ca62 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >
> > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > +					 unsigned int code)
> > > +{
> > > +	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);
> > > +
> > > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > > +		if (codes[i] == code)
> > > +			return 0;
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > >  				   struct v4l2_subdev_state *sd_state,
> > >  				   struct v4l2_subdev_format *sdformat)
> > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > >  	struct adv748x_state *state = tx->state;
> > >  	struct v4l2_mbus_framefmt *mbusformat;
> > > -	int ret = 0;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Make sure the format is supported, if not default it to
> > > +	 * YUYV8 as it's supported by both TXes.
> > > +	 */
> > > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > +	if (ret)
> > > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> >
> > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > propagated at the end of this function and to user-space falling the
> > IOCTL.
> 
> Ouch, I think in my tests the error message got ignored
> 
> >
> > Fixing that I hit another issue that kind of shows we need this format
> > validation ;-)
> >
> > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > TXB and I can again capture CVBS with patch 1-8 applied.
> 
> Thanks for testing analog capture, I don't have a setup to easily do
> so.

You can test it with the pattern generator on any Gen3 board.

> 
> Should we make the AFE front-end produce 1X16 instead ? The format is
> only used between the AFE and TXs after all, changing it shouldn't
> have any implication on the interoperability with other devices.

Not sure, the list of formats supported by each entity in the ADV748x is 
added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this 
series. Where did the list come from? What formats do the AFE support?  
Why is the formats supported different between TXA and TXB ?

> >
> > >
> > >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > >  						 sdformat->which);
> > > --
> > > 2.44.0
> > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund
Jacopo Mondi May 6, 2024, 2:36 p.m. UTC | #5
Hi Niklas

On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > Hi Niklas
> >
> > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > > automatically infer the image stream format from the connected
> > > > frontend (HDMI or AFE).
> > > >
> > > > Setting a new format on the subdevice hence does not actually control
> > > > the CSI-2 output format, but it's only there for the purpose of
> > > > pipeline validation.
> > > >
> > > > However, there is currently no validation that the supplied media bus
> > > > code is valid and supported by the device.
> > > >
> > > > With the introduction of enum_mbus_codes a list of supported format is
> > > > now available, use it to validate that the supplied format is correct
> > > > and use the default YUYV8 one if that's not the case.
> > >
> > > With the update tests for the change in patch 4 I hit multiple issues
> > > with this patch for CVBS capture.
> > >
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > index 219417b319a6..1aae6467ca62 100644
> > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > > +					 unsigned int code)
> > > > +{
> > > > +	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);
> > > > +
> > > > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > > > +		if (codes[i] == code)
> > > > +			return 0;
> > > > +	}
> > > > +
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > >  				   struct v4l2_subdev_state *sd_state,
> > > >  				   struct v4l2_subdev_format *sdformat)
> > > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > >  	struct adv748x_state *state = tx->state;
> > > >  	struct v4l2_mbus_framefmt *mbusformat;
> > > > -	int ret = 0;
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * Make sure the format is supported, if not default it to
> > > > +	 * YUYV8 as it's supported by both TXes.
> > > > +	 */
> > > > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > > +	if (ret)
> > > > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> > >
> > > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > > propagated at the end of this function and to user-space falling the
> > > IOCTL.
> >
> > Ouch, I think in my tests the error message got ignored
> >
> > >
> > > Fixing that I hit another issue that kind of shows we need this format
> > > validation ;-)
> > >
> > > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > > TXB and I can again capture CVBS with patch 1-8 applied.
> >
> > Thanks for testing analog capture, I don't have a setup to easily do
> > so.
>
> You can test it with the pattern generator on any Gen3 board.
>

ah well

> >
> > Should we make the AFE front-end produce 1X16 instead ? The format is
> > only used between the AFE and TXs after all, changing it shouldn't
> > have any implication on the interoperability with other devices.
>
> Not sure, the list of formats supported by each entity in the ADV748x is
> added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this
> series.

> Where did the list come from?

From the chip datasheet ?
Section 9.7 "MIPI Ouput format"

>What formats do the AFE support?

The AFE doesn't really "support" any format in my understanding. It
connects to one of the two TXes with an internal processing pipeline,
and the TX transmits the received video stream on the serial bus.

> Why is the formats supported different between TXA and TXB ?

You should ask the chip producer :)

>
> > >
> > > >
> > > >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > >  						 sdformat->which);
> > > > --
> > > > 2.44.0
> > > >
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
>
> --
> Kind Regards,
> Niklas Söderlund
>
Niklas Söderlund May 6, 2024, 2:58 p.m. UTC | #6
Hello Jacopo,

On 2024-05-06 16:36:01 +0200, Jacopo Mondi wrote:
> Hi Niklas
> 
> On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > > Hi Niklas
> > >
> > > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > > > automatically infer the image stream format from the connected
> > > > > frontend (HDMI or AFE).
> > > > >
> > > > > Setting a new format on the subdevice hence does not actually control
> > > > > the CSI-2 output format, but it's only there for the purpose of
> > > > > pipeline validation.
> > > > >
> > > > > However, there is currently no validation that the supplied media bus
> > > > > code is valid and supported by the device.
> > > > >
> > > > > With the introduction of enum_mbus_codes a list of supported format is
> > > > > now available, use it to validate that the supplied format is correct
> > > > > and use the default YUYV8 one if that's not the case.
> > > >
> > > > With the update tests for the change in patch 4 I hit multiple issues
> > > > with this patch for CVBS capture.
> > > >
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > index 219417b319a6..1aae6467ca62 100644
> > > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > > > +					 unsigned int code)
> > > > > +{
> > > > > +	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);
> > > > > +
> > > > > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > > > > +		if (codes[i] == code)
> > > > > +			return 0;
> > > > > +	}
> > > > > +
> > > > > +	return -EINVAL;
> > > > > +}
> > > > > +
> > > > >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > >  				   struct v4l2_subdev_state *sd_state,
> > > > >  				   struct v4l2_subdev_format *sdformat)
> > > > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > > >  	struct adv748x_state *state = tx->state;
> > > > >  	struct v4l2_mbus_framefmt *mbusformat;
> > > > > -	int ret = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	/*
> > > > > +	 * Make sure the format is supported, if not default it to
> > > > > +	 * YUYV8 as it's supported by both TXes.
> > > > > +	 */
> > > > > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > > > +	if (ret)
> > > > > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> > > >
> > > > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > > > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > > > propagated at the end of this function and to user-space falling the
> > > > IOCTL.
> > >
> > > Ouch, I think in my tests the error message got ignored
> > >
> > > >
> > > > Fixing that I hit another issue that kind of shows we need this format
> > > > validation ;-)
> > > >
> > > > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > > > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > > > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > > > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > > > TXB and I can again capture CVBS with patch 1-8 applied.
> > >
> > > Thanks for testing analog capture, I don't have a setup to easily do
> > > so.
> >
> > You can test it with the pattern generator on any Gen3 board.
> >
> 
> ah well
> 
> > >
> > > Should we make the AFE front-end produce 1X16 instead ? The format is
> > > only used between the AFE and TXs after all, changing it shouldn't
> > > have any implication on the interoperability with other devices.
> >
> > Not sure, the list of formats supported by each entity in the ADV748x is
> > added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this
> > series.
> 
> > Where did the list come from?
> 
> From the chip datasheet ?
> Section 9.7 "MIPI Ouput format"

Thanks I found it now, maybe add that to the commit message of that 
patch? I was hunting for register values in the register control manual 
and found nothing ;-)
> 
> >What formats do the AFE support?
> 
> The AFE doesn't really "support" any format in my understanding. It
> connects to one of the two TXes with an internal processing pipeline,
> and the TX transmits the received video stream on the serial bus.

Ahh! I think we found the root of the issue we talked about the other 
day in the VIN format handling about 1X16 vs 2x8 and CSI-2 ;-) That 
likely came from this setting.

Yes, with the information from the datahseet I do think we should change 
adv748x_afe_enum_mbus_code() to report MEDIA_BUS_FMT_YUYV8_1X16 instead 
of MEDIA_BUS_FMT_UYVY8_2X8.

> 
> > Why is the formats supported different between TXA and TXB ?
> 
> You should ask the chip producer :)

If only we could. Imagine how much time we save if we could talk to them 
and have datasheets instead of guessing half the time ;-)

> 
> >
> > > >
> > > > >
> > > > >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > > >  						 sdformat->which);
> > > > > --
> > > > > 2.44.0
> > > > >
> > > >
> > > > --
> > > > Kind Regards,
> > > > Niklas Söderlund
> >
> > --
> > Kind Regards,
> > Niklas Söderlund
> >
Jacopo Mondi May 6, 2024, 3:04 p.m. UTC | #7
Hi Niklas

On Mon, May 06, 2024 at 04:58:30PM GMT, Niklas Söderlund wrote:
> Hello Jacopo,
>
> On 2024-05-06 16:36:01 +0200, Jacopo Mondi wrote:
> > Hi Niklas
> >
> > On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > > > Hi Niklas
> > > >
> > > > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > > > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > > > > automatically infer the image stream format from the connected
> > > > > > frontend (HDMI or AFE).
> > > > > >
> > > > > > Setting a new format on the subdevice hence does not actually control
> > > > > > the CSI-2 output format, but it's only there for the purpose of
> > > > > > pipeline validation.
> > > > > >
> > > > > > However, there is currently no validation that the supplied media bus
> > > > > > code is valid and supported by the device.
> > > > > >
> > > > > > With the introduction of enum_mbus_codes a list of supported format is
> > > > > > now available, use it to validate that the supplied format is correct
> > > > > > and use the default YUYV8 one if that's not the case.
> > > > >
> > > > > With the update tests for the change in patch 4 I hit multiple issues
> > > > > with this patch for CVBS capture.
> > > > >
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > > > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > index 219417b319a6..1aae6467ca62 100644
> > > > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > > > > +					 unsigned int code)
> > > > > > +{
> > > > > > +	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);
> > > > > > +
> > > > > > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > > > > > +		if (codes[i] == code)
> > > > > > +			return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > > >  				   struct v4l2_subdev_state *sd_state,
> > > > > >  				   struct v4l2_subdev_format *sdformat)
> > > > > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > > >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > > > >  	struct adv748x_state *state = tx->state;
> > > > > >  	struct v4l2_mbus_framefmt *mbusformat;
> > > > > > -	int ret = 0;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Make sure the format is supported, if not default it to
> > > > > > +	 * YUYV8 as it's supported by both TXes.
> > > > > > +	 */
> > > > > > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > > > > +	if (ret)
> > > > > > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> > > > >
> > > > > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > > > > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > > > > propagated at the end of this function and to user-space falling the
> > > > > IOCTL.
> > > >
> > > > Ouch, I think in my tests the error message got ignored
> > > >
> > > > >
> > > > > Fixing that I hit another issue that kind of shows we need this format
> > > > > validation ;-)
> > > > >
> > > > > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > > > > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > > > > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > > > > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > > > > TXB and I can again capture CVBS with patch 1-8 applied.
> > > >
> > > > Thanks for testing analog capture, I don't have a setup to easily do
> > > > so.
> > >
> > > You can test it with the pattern generator on any Gen3 board.
> > >
> >
> > ah well
> >
> > > >
> > > > Should we make the AFE front-end produce 1X16 instead ? The format is
> > > > only used between the AFE and TXs after all, changing it shouldn't
> > > > have any implication on the interoperability with other devices.
> > >
> > > Not sure, the list of formats supported by each entity in the ADV748x is
> > > added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this
> > > series.
> >
> > > Where did the list come from?
> >
> > From the chip datasheet ?
> > Section 9.7 "MIPI Ouput format"
>
> Thanks I found it now, maybe add that to the commit message of that
> patch? I was hunting for register values in the register control manual
> and found nothing ;-)

ok..

> >
> > >What formats do the AFE support?
> >
> > The AFE doesn't really "support" any format in my understanding. It
> > connects to one of the two TXes with an internal processing pipeline,
> > and the TX transmits the received video stream on the serial bus.
>
> Ahh! I think we found the root of the issue we talked about the other
> day in the VIN format handling about 1X16 vs 2x8 and CSI-2 ;-) That
> likely came from this setting.
>
> Yes, with the information from the datahseet I do think we should change
> adv748x_afe_enum_mbus_code() to report MEDIA_BUS_FMT_YUYV8_1X16 instead

nit: MEDIA_BUS_FMT_UYVY8_1X16

> of MEDIA_BUS_FMT_UYVY8_2X8.
>
> >
> > > Why is the formats supported different between TXA and TXB ?
> >
> > You should ask the chip producer :)
>
> If only we could. Imagine how much time we save if we could talk to them
> and have datasheets instead of guessing half the time ;-)
>

:')

> >
> > >
> > > > >
> > > > > >
> > > > > >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > > > >  						 sdformat->which);
> > > > > > --
> > > > > > 2.44.0
> > > > > >
> > > > >
> > > > > --
> > > > > Kind Regards,
> > > > > Niklas Söderlund
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
> > >
>
> --
> Kind Regards,
> Niklas Söderlund
Laurent Pinchart May 6, 2024, 7:02 p.m. UTC | #8
On Mon, May 06, 2024 at 05:04:52PM +0200, Jacopo Mondi wrote:
> On Mon, May 06, 2024 at 04:58:30PM GMT, Niklas Söderlund wrote:
> > On 2024-05-06 16:36:01 +0200, Jacopo Mondi wrote:
> > > On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> > > > On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > > > > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > > > > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > > > > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > > > > > automatically infer the image stream format from the connected
> > > > > > > frontend (HDMI or AFE).
> > > > > > >
> > > > > > > Setting a new format on the subdevice hence does not actually control
> > > > > > > the CSI-2 output format, but it's only there for the purpose of
> > > > > > > pipeline validation.
> > > > > > >
> > > > > > > However, there is currently no validation that the supplied media bus
> > > > > > > code is valid and supported by the device.
> > > > > > >
> > > > > > > With the introduction of enum_mbus_codes a list of supported format is
> > > > > > > now available, use it to validate that the supplied format is correct
> > > > > > > and use the default YUYV8 one if that's not the case.
> > > > > >
> > > > > > With the update tests for the change in patch 4 I hit multiple issues
> > > > > > with this patch for CVBS capture.
> > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > > > > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > > index 219417b319a6..1aae6467ca62 100644
> > > > > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > > > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > > > > > +					 unsigned int code)
> > > > > > > +{
> > > > > > > +	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);
> > > > > > > +
> > > > > > > +	for (unsigned int i = 0; i < num_fmts; i++) {
> > > > > > > +		if (codes[i] == code)
> > > > > > > +			return 0;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return -EINVAL;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > > > >  				   struct v4l2_subdev_state *sd_state,
> > > > > > >  				   struct v4l2_subdev_format *sdformat)
> > > > > > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > > > >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > > > > >  	struct adv748x_state *state = tx->state;
> > > > > > >  	struct v4l2_mbus_framefmt *mbusformat;
> > > > > > > -	int ret = 0;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Make sure the format is supported, if not default it to
> > > > > > > +	 * YUYV8 as it's supported by both TXes.
> > > > > > > +	 */
> > > > > > > +	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > > > > > +	if (ret)
> > > > > > > +		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> > > > > >
> > > > > > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > > > > > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > > > > > propagated at the end of this function and to user-space falling the
> > > > > > IOCTL.
> > > > >
> > > > > Ouch, I think in my tests the error message got ignored
> > > > >
> > > > > > Fixing that I hit another issue that kind of shows we need this format
> > > > > > validation ;-)
> > > > > >
> > > > > > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > > > > > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > > > > > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > > > > > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > > > > > TXB and I can again capture CVBS with patch 1-8 applied.
> > > > >
> > > > > Thanks for testing analog capture, I don't have a setup to easily do
> > > > > so.
> > > >
> > > > You can test it with the pattern generator on any Gen3 board.
> > >
> > > ah well
> > >
> > > > > Should we make the AFE front-end produce 1X16 instead ? The format is
> > > > > only used between the AFE and TXs after all, changing it shouldn't
> > > > > have any implication on the interoperability with other devices.
> > > >
> > > > Not sure, the list of formats supported by each entity in the ADV748x is
> > > > added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this
> > > > series.
> > >
> > > > Where did the list come from?
> > >
> > > From the chip datasheet ?
> > > Section 9.7 "MIPI Ouput format"
> >
> > Thanks I found it now, maybe add that to the commit message of that
> > patch? I was hunting for register values in the register control manual
> > and found nothing ;-)
> 
> ok..
> 
> > > > What formats do the AFE support?
> > >
> > > The AFE doesn't really "support" any format in my understanding. It
> > > connects to one of the two TXes with an internal processing pipeline,
> > > and the TX transmits the received video stream on the serial bus.
> >
> > Ahh! I think we found the root of the issue we talked about the other
> > day in the VIN format handling about 1X16 vs 2x8 and CSI-2 ;-) That
> > likely came from this setting.
> >
> > Yes, with the information from the datahseet I do think we should change
> > adv748x_afe_enum_mbus_code() to report MEDIA_BUS_FMT_YUYV8_1X16 instead
> 
> nit: MEDIA_BUS_FMT_UYVY8_1X16

A general note about internal formats: in the absence of information
telling the exact format used in internal buses, and actually even when
that information is available but the exact format doesn't affect
anything, you're free to pick whatever seems logical enough and makes
life the easiest for userspace and kernel drivers. In this specific
case, MEDIA_BUS_FMT_YUYV8_1X16 would work, but I think
MEDIA_BUS_FMT_UYVY8_1X16 is better as it will match the CSI-2 TX output,
saving us from writing conversion code in the CSI-2 TX subdev.

> > of MEDIA_BUS_FMT_UYVY8_2X8.
> >
> > > > Why is the formats supported different between TXA and TXB ?
> > >
> > > You should ask the chip producer :)
> >
> > If only we could. Imagine how much time we save if we could talk to them
> > and have datasheets instead of guessing half the time ;-)
> 
> :')
> 
> > > > > > >
> > > > > > >  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > > > > >  						 sdformat->which);
diff mbox series

Patch

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 219417b319a6..1aae6467ca62 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -215,6 +215,23 @@  static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
+					 unsigned int code)
+{
+	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);
+
+	for (unsigned int i = 0; i < num_fmts; i++) {
+		if (codes[i] == code)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_format *sdformat)
@@ -222,7 +239,15 @@  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
 	struct adv748x_state *state = tx->state;
 	struct v4l2_mbus_framefmt *mbusformat;
-	int ret = 0;
+	int ret;
+
+	/*
+	 * Make sure the format is supported, if not default it to
+	 * YUYV8 as it's supported by both TXes.
+	 */
+	ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
+	if (ret)
+		sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
 
 	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
 						 sdformat->which);