Re: [PATCH v2 3/7] media: imx: imx7-media-csi: Use dual sampling for YUV 1X16

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

Hi Jacopo,

Thank you for the patch.

On Fri, Feb 18, 2022 at 07:34:17PM +0100, Jacopo Mondi wrote:
> The CSI bridge should operate in dual pixel sampling mode when it is

s/dual pixel sampling mode/dual component mode/

> connected to a pixel transmitter that transfers two pixel samples (16
> bits) at the time in YUYV formats.

It's actually one pixel per clock. Without BIT_TWO_8BIT_SENSOR and
BIT_MIPI_DOUBLE_CMPNT, the CSI bridge expects 8-bit data, which requires
two clock cycles to transfer one pixel. When setting those bits, it
expects 16-bit data, with one clock cycle per pixel. The
MIPI_DOUBLE_CMPNT documentation states this clearly:

20 MIPI_DOUBLE_CMPNT
Double component per clock cycle in YUV422 formats.
0 Single component per clock cycle
(half pixel per clock cycle)
1 Double component per clock cycle
(a pixel per clock cycle)

> Use the image format variants to determine if single or dual pixel mode

"dual pixel" is a concept of the CSIS, not the CSI bridge. This should
mention single or double component mode.

> should be used.
> 
> Add a note to the TODO file to record that the list of supported formats
> should be restricted to the SoC model the CSI bridge is integrated on
> to avoid potential pipeline mis-configurations.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/TODO             | 26 ++++++++++++++++++++++
>  drivers/staging/media/imx/imx7-media-csi.c |  8 +++++--
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> index 06c94f20ecf8..e15eba32cc94 100644
> --- a/drivers/staging/media/imx/TODO
> +++ b/drivers/staging/media/imx/TODO
> @@ -27,3 +27,29 @@
>  - i.MX7: all of the above, since it uses the imx media core
> 
>  - i.MX7: use Frame Interval Monitor
> +
> +- imx7-media-csi: Restrict the supported formats list to the SoC version.
> +
> +  The imx7 CSI bridge can be configured to sample pixel components from the Rx
> +  queue in single  (8bpp) or double (16bpp) modes. Image format variations with
> +  different sample sizes (ie YUYV_2X8 vs YUYV_1X16) determine the sampling size
> +  (see imx7_csi_configure()).
> +
> +  As the imx7 CSI bridge can be interfaced with different IP blocks depending on
> +  the SoC model it is integrated on, the Rx queue sampling size should match
> +  the size of samples transferred by the transmitting IP block.
> +
> +  To avoid mis-configurations of the capture pipeline, the enumeration of the
> +  supported formats should be restricted to match the pixel source
> +  transmitting mode.
> +
> +  Examples: i.MX8MM SoC integrates the CSI bridge with the Samsung CSIS CSI-2
> +  receiver which operates in dual pixel sampling mode. The CSI bridge should
> +  only expose the 1X16 formats variant which instructs it to operate in dual
> +  pixel sampling mode. When the CSI bridge is instead integrated on an i.MX8MQ
> +  SoC, which features a CSI-2 receiver that operates in single sampling mode, it
> +  should only expose the 2X8 formats variant which instruct it to operate in
> +  single sampling mode.
> +
> +  This currently only applies to YUYV formats, but other formats might need
> +  to be treated in the same way.
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 32311fc0e2a4..108360ae3710 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -503,11 +503,15 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		 * all of them comply. Support both variants.
>  		 */

You can drop this comment, as the two variants are not related to the
CSI-2 bus format (which should always be UYVY8_1X16) but to the format
output by the CSI-2 RX.

I would add another comment to explain this clearly:

		/*
		 * The CSI bridge has a 16-bit input bus. Depending on
		 * the connected source, data may be transmitted with 8
		 * or 10 bits per clock sample (in bits [9:2] or [9:0]
		 * respectively) or with 16 bits per clock sample (in
		 * bits [15:0]). The data is then packed into a 32-bit
		 * FIFO (as shown in figure 13-11 of the i.MX8MM
		 * reference manual rev. 3).
		 *
		 * The data packing in a 32-bit FIFO input workd is
		 * controlled by the CR3 TWO_8BIT_SENSOR field (also
		 * known as SENSOR_16BITS in the i.MX8MM reference
		 * manual). When set to 0, data packing groups four
		 * 8-bit input samples (bits [9:2]). When set to 1, data
		 * packing groups two 16-bit input samples (bits
		 * [15:0]).
		 *
		 * The register field CR18 MIPI_DOUBLE_CMPNT also needs
		 * to be configured according to the input format for
		 * YUV 4:2:2 data. The field controls the gasket between
		 * the CSI-2 receiver and the CSI bridge. On i.MX7 and
		 * i.MX8MM, the field must be set to 1 when the CSIS
		 * outputs 16-bit samples. On i.MX8MQ, the gasket
		 * ignores the MIPI_DOUBLE_CMPNT bit and YUV 4:2:2
		 * always uses 16-bit samples. Setting MIPI_DOUBLE_CMPNT
		 * in that case has no effect, but doesn't cause any
		 * issue.
		 */

With this, someone trying to figure out what those bits do will
hopefully be able to get it right :-)

With these changes,

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

>  		case MEDIA_BUS_FMT_UYVY8_2X8:
> -		case MEDIA_BUS_FMT_UYVY8_1X16:
>  		case MEDIA_BUS_FMT_YUYV8_2X8:
> -		case MEDIA_BUS_FMT_YUYV8_1X16:
>  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>  			break;
> +		case MEDIA_BUS_FMT_UYVY8_1X16:
> +		case MEDIA_BUS_FMT_YUYV8_1X16:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
> +			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B |
> +				BIT_MIPI_DOUBLE_CMPNT;
> +			break;
>  		}
>  	}
> 

-- 
Regards,

Laurent Pinchart