Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

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

Hi Frieder,

On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
> On 16.05.21 04:42, Laurent Pinchart wrote:
> > Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> > capture with an OV5640 sensor connected over CSI-2, showed that the
> > TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> > larger than 8 bits. Do so, even if the reference manual doesn't clearly
> > describe the effect of the field.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> For the ADV7280-M I also have the diffs below applied. Do you think
> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?

Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
convention.

> In the RM it mentions YUV422 in the description of
> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
> wrong.

That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
retest.

> I know this is not really related to this patch. I'm just wondering
> how to properly support my setup.

It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
fields are not well documented, and neither is the CSI_CR18
MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
documented, how they're integrated isn't described. So far, I can only
guess.

> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -346,6 +346,11 @@ struct csis_pix_format {
>  
>  static const struct csis_pix_format mipi_csis_formats[] = {
>         /* YUV formats. */
> +       {
> +               .code = MEDIA_BUS_FMT_UYVY8_2X8,
> +               .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> +               .width = 8,
> +       },
>         {
>                 .code = MEDIA_BUS_FMT_UYVY8_1X16,
>                 .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> 
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
>         /* Color format */
>         val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>         val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> -       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
> +       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
>         mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>  
>         /* Pixel resolution */
> 
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>                 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;
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> +                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;

I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
you tried setting neither ?

Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
driver is the width value passed to v4l2_get_link_freq(). With
MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
equal to half of the actual value, and thus a wrong Ths_settle value. It
shuold have no other influence though.

>                         break;
>                 }
>         }
> 
> > ---
> >  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index f85a2f5f1413..256b9aa978f0 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  	int width = out_pix->width;
> >  	u32 stride = 0;
> >  	u32 cr1, cr18;
> > +	u32 cr3 = 0;
> >  
> >  	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> >  
> > @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG10_1X10:
> >  		case MEDIA_BUS_FMT_SGRBG10_1X10:
> >  		case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> >  			break;
> >  		case MEDIA_BUS_FMT_Y12_1X12:
> > @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG12_1X12:
> >  		case MEDIA_BUS_FMT_SGRBG12_1X12:
> >  		case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> >  			break;
> >  		case MEDIA_BUS_FMT_Y14_1X14:
> > @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG14_1X14:
> >  		case MEDIA_BUS_FMT_SGRBG14_1X14:
> >  		case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> >  			break;
> >  		/*
> > @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  
> >  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> >  	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> > -	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> > +	imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> >  	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
> >  
> >  	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> > 

-- 
Regards,

Laurent Pinchart