Hi Frieder,

On Tue, May 25, 2021 at 11:59:40AM +0200, Frieder Schrempf wrote:

On 19.05.21 02:16, Laurent Pinchart wrote:

quoted

On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:

quoted

On 16.05.21 04:42, Laurent Pinchart wrote:

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

--- 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.

quoted

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