Re: [PATCH v2 7/7] media: imx: imx-mipi-csis: Add output format

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

Thank you for the patch.

On Fri, Feb 18, 2022 at 07:34:21PM +0100, Jacopo Mondi wrote:
> Due to how pixel components are transmitted on the CSI-2 serial bus
> and how they are stored in memory by the CSI-2 receiver, the component
> ordering might change and the image formats on the sink and source pads
> of the receiver should reflect it.
> 
> For RGB24, in example, the component ordering on the wire as described by
> the CSI-2 specification matches the BGR888 format, while once stored in
> in memory by the CSIS receiver they match the RGB888 format.

The CSI-2 receiver doesn't store data in memory. The issue this patch
fixes is that the CSI-2 receiver deserializes data and outputs it on a
parallel bus, with a bit order that is specific to the receiver and may
not match the media bus code used to described the format on the CSI-2
bus.

> Add an additional .output field to struct csis_pix_format to allow
> propagating the correct format to the source pad after a format
> configuration on the sink.
> 
> The change is only relevant for RGB24 but paves the way for further
> format translations in future.
> 
> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> ---
>  drivers/media/platform/imx/imx-mipi-csis.c | 29 +++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> index fdf133f81c5b..128f4180d1e9 100644
> --- a/drivers/media/platform/imx/imx-mipi-csis.c
> +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> @@ -351,6 +351,7 @@ struct csis_pix_format {
>  	u32 code;
>  	u32 data_type;
>  	u8 width;
> +	u32 output;

I'd move this right after code, as they're related.

>  };
>  
>  static const struct csis_pix_format mipi_csis_formats[] = {
> @@ -359,95 +360,117 @@ static const struct csis_pix_format mipi_csis_formats[] = {
>  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
>  		.data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>  		.width = 16,
> +		.output = MEDIA_BUS_FMT_UYVY8_1X16,
>  	},
>  	/* RGB formats. */
>  	{
>  		.code = MEDIA_BUS_FMT_RGB565_1X16,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RGB565,
>  		.width = 16,
> +		.output = MEDIA_BUS_FMT_RGB565_1X16,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_BGR888_1X24,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RGB888,
>  		.width = 24,
> +		.output = MEDIA_BUS_FMT_RGB888_1X24,
>  	},
>  	/* RAW (Bayer and greyscale) formats. */
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW8,
>  		.width = 8,
> +		.output = MEDIA_BUS_FMT_SBGGR8_1X8,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW8,
>  		.width = 8,
> +		.output = MEDIA_BUS_FMT_SGBRG8_1X8,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW8,
>  		.width = 8,
> +		.output = MEDIA_BUS_FMT_SGRBG8_1X8,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW8,
>  		.width = 8,
> +		.output = MEDIA_BUS_FMT_SRGGB8_1X8,
>  	}, {
>  		.code = MEDIA_BUS_FMT_Y8_1X8,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW8,
>  		.width = 8,
> +		.output = MEDIA_BUS_FMT_Y8_1X8,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW10,
>  		.width = 10,
> +		.output = MEDIA_BUS_FMT_SBGGR10_1X10,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGBRG10_1X10,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW10,
>  		.width = 10,
> +		.output = MEDIA_BUS_FMT_SGBRG10_1X10,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW10,
>  		.width = 10,
> +		.output = MEDIA_BUS_FMT_SGRBG10_1X10,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW10,
>  		.width = 10,
> +		.output = MEDIA_BUS_FMT_SRGGB10_1X10,
>  	}, {
>  		.code = MEDIA_BUS_FMT_Y10_1X10,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW10,
>  		.width = 10,
> +		.output = MEDIA_BUS_FMT_Y10_1X10,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW12,
>  		.width = 12,
> +		.output = MEDIA_BUS_FMT_SBGGR12_1X12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW12,
>  		.width = 12,
> +		.output = MEDIA_BUS_FMT_SGBRG12_1X12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW12,
>  		.width = 12,
> +		.output = MEDIA_BUS_FMT_SGRBG12_1X12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW12,
>  		.width = 12,
> +		.output = MEDIA_BUS_FMT_SRGGB12_1X12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_Y12_1X12,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW12,
>  		.width = 12,
> +		.output = MEDIA_BUS_FMT_Y12_1X12,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SBGGR14_1X14,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW14,
>  		.width = 14,
> +		.output = MEDIA_BUS_FMT_SBGGR14_1X14,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGBRG14_1X14,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW14,
>  		.width = 14,
> +		.output = MEDIA_BUS_FMT_SGBRG14_1X14,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SGRBG14_1X14,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW14,
>  		.width = 14,
> +		.output = MEDIA_BUS_FMT_SGRBG14_1X14,
>  	}, {
>  		.code = MEDIA_BUS_FMT_SRGGB14_1X14,
>  		.data_type = MIPI_CSI2_DATA_TYPE_RAW14,
>  		.width = 14,
> +		.output = MEDIA_BUS_FMT_SRGGB14_1X14,
>  	}
>  };
>  
> @@ -1090,7 +1113,11 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	/* Propagate the format from sink to source. */
>  	fmt = mipi_csis_get_format(state, sd_state, sdformat->which,
>  				   CSIS_PAD_SOURCE);
> -	*fmt = sdformat->format;
> +
> +	/* The format on the source pad might change due to unpacking. */
> +	fmt->code = csis_fmt->output;
> +	fmt->width = sdformat->format.width;
> +	fmt->height = sdformat->format.height;

You need to also propagate colorspace. The simplest solution is

	*fmt = sdformat->format;

	/* The format on the source pad might change due to unpacking. */
	fmt->code = csis_fmt->output;

>  	/* Store the CSIS format descriptor for active formats. */
>  	if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux