Re: [PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

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

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote:
> The pixel sampling mode controls the size of data sampled from the CSI
> Rx queue. The supported sample size depends on the configuration of the
> preceding block in the capture pipeline and is then dependent on the SoC
> version the CSI peripheral is integrated on.
> 
> When capturing YUV422 data if dual sample mode is available use it.
> 
> This change is particularly relevant for the IMX8MM SoC which uses the
> CSIS CSI-2 receiver which operates in dual pixel mode.
> 
> Other SoCs should be unaffected by this change and should continue to
> operate as before.
> 
> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 112096774961..a8bdfb0bb0ee 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  {
>  	struct imx_media_video_dev *vdev = csi->vdev;
>  	struct v4l2_pix_format *out_pix = &vdev->fmt;
> +	struct imx_media_dev *imxmd = csi->imxmd;
>  	int width = out_pix->width;
>  	u32 stride = 0;
>  	u32 cr3 = BIT_FRMCNT_RST;
> @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  	cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK |
>  		  BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN |
>  		  BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
> -		  BIT_DEINTERLACE_EN);
> +		  BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT);
>  
>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>  		cr18 |= BIT_DEINTERLACE_EN;
> @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_YUYV8_2X8:
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> +
> +			/* If dual mode is supported use it. */
> +			if (imxmd->info->sample_modes & MODE_DUAL) {
> +				cr18 |= BIT_MIPI_DOUBLE_CMPNT;
> +				cr3 |= BIT_TWO_8BIT_SENSOR;
> +			}

I would implement this differently:

		case MEDIA_BUS_FMT_UYVY8_2X8:
		case MEDIA_BUS_FMT_YUYV8_2X8:
			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;

This would support either option here. What you will then need to change
is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow
the 2X8 and 1X16 variants based on the SoC. This is important for the
i.MX7, which has both a CSI-2 input and a parallel input. When using the
CSIS it can (and should) use double component mode, while when using the
parallel input it can work in 8-bit or 16-bit mode depending on how the
sensor is wired.

> +
>  			break;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart