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 Tue, May 25, 2021 at 11:59:40AM +0200, Frieder Schrempf wrote:
> On 19.05.21 02:16, Laurent Pinchart wrote:
> > 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.
> 
> I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But
> if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then
> maybe the ADV driver needs to be fixed.

That would be the ideal situation. We need to be careful about not
breaking backward compatibility, but apart from that,
MEDIA_BUS_FMT_UYVY8_1X16 is the way to go.

I started writing a documentation patch, and found it it's already
documented :-) Documentation/userspace-api/media/v4l/subdev-formats.rst
states:

    The media bus pixel codes document parallel formats. Should the
    pixel data be transported over a serial bus, the media bus pixel
    code that describes a parallel format that transfers a sample on a
    single clock cycle is used. For instance, both
    MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used on
    parallel busses for transferring an 8 bits per sample BGR data,
    whereas on serial busses the data in this format is only referred to
    using MEDIA_BUS_FMT_BGR888_1X24. This is because there is
    effectively only a single way to transport that format on the serial
    busses.

> >> 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 ?
> 
> Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors
> from the MIPI CSI2 controller and it doesn't work at all.

What PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT do exactly is not totally
clear, we'll have to iron it out to ensure drivers can pick the right
settings automatically.

> > 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.
> 
> The link frequency calculation doesn't work for me at the moment, as
> the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ
> or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle
> value.

-- 
Regards,

Laurent Pinchart