[v2] media: rcar-vin: Add support to select data pins for YCbCr422-8bit input

Commit Message

Prabhakar Aug. 3, 2020, 4:02 p.m. UTC

Select the data pins for YCbCr422-8bit input format depending on
bus_width and data_shift passed as part of DT.

Signed-off-by: Lad Prabhakar <[e-mail]>
Reviewed-by: Biju Das <[e-mail]>
---
Changes for v2:
* Dropped DT binding documentation patch
* Select the data pins depending on bus-width and data-shift

v1 -
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
---
 drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
 drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
 3 files changed, 17 insertions(+)

Comments

Niklas Söderlund Aug. 3, 2020, 6:06 p.m. UTC | #1

Hi Lad,

Thanks for your work.

On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> Select the data pins for YCbCr422-8bit input format depending on
> bus_width and data_shift passed as part of DT.
> 
> Signed-off-by: Lad Prabhakar <[e-mail]>
> Reviewed-by: Biju Das <[e-mail]>
> ---
> Changes for v2:
> * Dropped DT binding documentation patch
> * Select the data pins depending on bus-width and data-shift

I like this v2 much better then v1, nice work!

> 
> v1 -
> https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c8965d27..55005d86928d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  	vin->parallel = rvpe;
>  	vin->parallel->mbus_type = vep->bus_type;
>  
> +	/* select VInDATA[15:8] pins for YCbCr422-8bit format */
> +	if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> +	    vep->bus.parallel.data_shift == DATA_SHIFT_8)
> +		vin->parallel->ycbcr_8b_g = true;
> +

I would store the bus_width and bus_shift values in the struct 
rvin_parallel_entity and evaluate them in place rater then create a flag 
for this specific use-case..

Also according to the documentation is the check correct? Do we not wish 
to use the new mode when bus_width == 16 and bus_shift == 8. The check 
you have here seems to describe a 8 lane bus where 0 lanes are used.

I think you should also verify that bus_shift is either 0 or 8 as that 
is all the driver supports.

>  	switch (vin->parallel->mbus_type) {
>  	case V4L2_MBUS_PARALLEL:
>  		vin_dbg(vin, "Found PARALLEL media bus\n");
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd036371..5db483877d65 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -127,6 +127,8 @@
>  #define VNDMR2_FTEV		(1 << 17)
>  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
>  
> +#define VNDMR2_YDS		BIT(22)

This should be grouped with the other VNDMR2_* macros and not on its 
own. Also it should be sorted so it should be inserted between 
VNDMR2_CES and VNDMR2_FTEV.

Also I know BIT() is a nice macro but the rest of the driver uses (1 << 
22), please do the same for this one.

> +
>  /* Video n CSI2 Interface Mode Register (Gen3) */
>  #define VNCSI_IFMD_DES1		(1 << 26)
>  #define VNCSI_IFMD_DES0		(1 << 25)
> @@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
>  		/* Data Enable Polarity Select */
>  		if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
>  			dmr2 |= VNDMR2_CES;
> +
> +		if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
> +			dmr2 |= VNDMR2_YDS;
> +		else
> +			dmr2 &= ~VNDMR2_YDS;

dmr2 is already unitized and YDS is cleared, no need to clear it again 
if you don't wish to set it. Taking this and the comments above into 
account this would become something like (not tested),

    switch (vin->mbus_code) {
    case MEDIA_BUS_FMT_UYVY8_2X8:
        if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8)
            dmr2 |= VNDMR2_YDS;
        break;
    default:
        break;
    }

>  	}
>  
>  	/*
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c19d077ce1cb..3126fee9a89b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -87,6 +87,9 @@ struct rvin_video_format {
>  	u8 bpp;
>  };
>  
> +#define BUS_WIDTH_8	8
> +#define DATA_SHIFT_8	8

As pointed out by Geert, not so useful, use 8 in the code :-)

> +
>  /**
>   * struct rvin_parallel_entity - Parallel video input endpoint descriptor
>   * @asd:	sub-device descriptor for async framework
> @@ -95,6 +98,7 @@ struct rvin_video_format {
>   * @mbus_flags:	media bus configuration flags
>   * @source_pad:	source pad of remote subdevice
>   * @sink_pad:	sink pad of remote subdevice
> + * @ycbcr_8b_g:	select data pins for YCbCr422-8bit
>   *
>   */
>  struct rvin_parallel_entity {
> @@ -106,6 +110,7 @@ struct rvin_parallel_entity {
>  
>  	unsigned int source_pad;
>  	unsigned int sink_pad;
> +	bool ycbcr_8b_g;
>  };
>  
>  /**
> -- 
> 2.17.1
>

Geert Uytterhoeven Aug. 3, 2020, 7:15 p.m. UTC | #2

Hi Niklas,

On Mon, Aug 3, 2020 at 8:06 PM Niklas <[e-mail]> wrote:
> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > Select the data pins for YCbCr422-8bit input format depending on
> > bus_width and data_shift passed as part of DT.
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > Reviewed-by: Biju Das <[e-mail]>

> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >       vin->parallel = rvpe;
> >       vin->parallel->mbus_type = vep->bus_type;
> >
> > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > +             vin->parallel->ycbcr_8b_g = true;
> > +
>
> I would store the bus_width and bus_shift values in the struct
> rvin_parallel_entity and evaluate them in place rater then create a flag
> for this specific use-case..
>
> Also according to the documentation is the check correct? Do we not wish
> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> you have here seems to describe a 8 lane bus where 0 lanes are used.

The bus width used is 8.

Documentation/devicetree/bindings/media/video-interfaces.txt:

  - bus-width: number of data lines actively used, valid for the
parallel busses.
  - data-shift: on the parallel data busses, if bus-width is used to specify the
    number of data lines, data-shift can be used to specify which data lines are
    used, e.g. "bus-width=<8>; data-shift=<2>;" means, that lines 9:2 are used.

Gr{oetje,eeting}s,

                        Geert

Prabhakar Aug. 3, 2020, 7:17 p.m. UTC | #3

Hi Niklas,

Thank you for the review.

On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > Select the data pins for YCbCr422-8bit input format depending on
> > bus_width and data_shift passed as part of DT.
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > Reviewed-by: Biju Das <[e-mail]>
> > ---
> > Changes for v2:
> > * Dropped DT binding documentation patch
> > * Select the data pins depending on bus-width and data-shift
>
> I like this v2 much better then v1, nice work!
>
> >
> > v1 -
> > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
> >  drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
> >  3 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7440c8965d27..55005d86928d 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >       vin->parallel = rvpe;
> >       vin->parallel->mbus_type = vep->bus_type;
> >
> > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > +             vin->parallel->ycbcr_8b_g = true;
> > +
>
> I would store the bus_width and bus_shift values in the struct
> rvin_parallel_entity and evaluate them in place rater then create a flag
> for this specific use-case..
>
Ok will do that.

> Also according to the documentation is the check correct? Do we not wish
> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> you have here seems to describe a 8 lane bus where 0 lanes are used.
>
bus-width is the actual data lines used, so bus_width == 16 and
bus_shift == 8 would mean use lines 23:8, so just check for bus_width
== 8 and bus_shift == 8 should be sufficient.

> I think you should also verify that bus_shift is either 0 or 8 as that
> is all the driver supports.
>
Not sure if thats correct.In that case this patch wont make sense, I
believed we agreed upon we determine the YDS depending on both
bus-width and bus-shift.

On iWave G21D-Q7 for VI2 interface VI2_G* pins are connected to SoC
and for VIN3 interface Vi3_DATA* pins are connected. So in this case
the capture only works for VIN2 only if YDS bit is set for 8-bit 422,
and for VIN3 capture only works if YDS is 0

&vin2 {
    status = "okay";
    pinctrl-0 = <&vin2_pins>;
    pinctrl-names = "default";

    port {
        #address-cells = <1>;
        #size-cells = <0>;

        vin2ep: endpoint {
            remote-endpoint = <&ov7725_2>;
            bus-width = <8>;
            data-shift = <8>;
        };
    };
};

&vin3 {
    status = "okay";
    pinctrl-0 = <&vin3_pins>;
    pinctrl-names = "default";

    port {
        #address-cells = <1>;
        #size-cells = <0>;

        vin3ep: endpoint {
            remote-endpoint = <&ov7725_3>;
            bus-width = <8>;
        };
    };
};


> >       switch (vin->parallel->mbus_type) {
> >       case V4L2_MBUS_PARALLEL:
> >               vin_dbg(vin, "Found PARALLEL media bus\n");
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 1a30cd036371..5db483877d65 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -127,6 +127,8 @@
> >  #define VNDMR2_FTEV          (1 << 17)
> >  #define VNDMR2_VLV(n)                ((n & 0xf) << 12)
> >
> > +#define VNDMR2_YDS           BIT(22)
>
> This should be grouped with the other VNDMR2_* macros and not on its
> own. Also it should be sorted so it should be inserted between
> VNDMR2_CES and VNDMR2_FTEV.
>
> Also I know BIT() is a nice macro but the rest of the driver uses (1 <<
> 22), please do the same for this one.
>
Sure will take care of it.

> > +
> >  /* Video n CSI2 Interface Mode Register (Gen3) */
> >  #define VNCSI_IFMD_DES1              (1 << 26)
> >  #define VNCSI_IFMD_DES0              (1 << 25)
> > @@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
> >               /* Data Enable Polarity Select */
> >               if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
> >                       dmr2 |= VNDMR2_CES;
> > +
> > +             if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
> > +                     dmr2 |= VNDMR2_YDS;
> > +             else
> > +                     dmr2 &= ~VNDMR2_YDS;
>
> dmr2 is already unitized and YDS is cleared, no need to clear it again
> if you don't wish to set it. Taking this and the comments above into
> account this would become something like (not tested),
>
Agreed.

>     switch (vin->mbus_code) {
>     case MEDIA_BUS_FMT_UYVY8_2X8:
>         if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8)
>             dmr2 |= VNDMR2_YDS;
>         break;
>     default:
>         break;
>     }
>
> >       }
> >
> >       /*
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index c19d077ce1cb..3126fee9a89b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -87,6 +87,9 @@ struct rvin_video_format {
> >       u8 bpp;
> >  };
> >
> > +#define BUS_WIDTH_8  8
> > +#define DATA_SHIFT_8 8
>
> As pointed out by Geert, not so useful, use 8 in the code :-)
>
Agreed will drop it.

Cheers,
Prabhakar

Niklas Söderlund Aug. 3, 2020, 7:28 p.m. UTC | #4

Hi Lad,

On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> >
> > Hi Lad,
> >
> > Thanks for your work.
> >
> > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > Select the data pins for YCbCr422-8bit input format depending on
> > > bus_width and data_shift passed as part of DT.
> > >
> > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > Reviewed-by: Biju Das <[e-mail]>
> > > ---
> > > Changes for v2:
> > > * Dropped DT binding documentation patch
> > > * Select the data pins depending on bus-width and data-shift
> >
> > I like this v2 much better then v1, nice work!
> >
> > >
> > > v1 -
> > > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
> > >  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
> > >  drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
> > >  3 files changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 7440c8965d27..55005d86928d 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > >       vin->parallel = rvpe;
> > >       vin->parallel->mbus_type = vep->bus_type;
> > >
> > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > +             vin->parallel->ycbcr_8b_g = true;
> > > +
> >
> > I would store the bus_width and bus_shift values in the struct
> > rvin_parallel_entity and evaluate them in place rater then create a flag
> > for this specific use-case..
> >
> Ok will do that.
> 
> > Also according to the documentation is the check correct? Do we not wish
> > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > you have here seems to describe a 8 lane bus where 0 lanes are used.
> >
> bus-width is the actual data lines used, so bus_width == 16 and
> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> == 8 and bus_shift == 8 should be sufficient.

As you and Geert points out I was wrong, they should indeed both be 8.

> 
> > I think you should also verify that bus_shift is either 0 or 8 as that
> > is all the driver supports.
> >
> Not sure if thats correct.In that case this patch wont make sense, I
> believed we agreed upon we determine the YDS depending on both
> bus-width and bus-shift.

I'm sorry I think I lost you :-) The driver is not capable of supporting 
bus_width = 8 and bus_shift = 2 right? Maybe we are talking about 
different things.

What I tried to say (updated with the knowledge of that bus_width should 
indeed be 8 and not 16) was that would it make sens to with bus_width=8 
allow for a bus_shift value other then 0 or 8? What for example would 
the driver do if the value was 2?

> 
> On iWave G21D-Q7 for VI2 interface VI2_G* pins are connected to SoC
> and for VIN3 interface Vi3_DATA* pins are connected. So in this case
> the capture only works for VIN2 only if YDS bit is set for 8-bit 422,
> and for VIN3 capture only works if YDS is 0
> 
> &vin2 {
>     status = "okay";
>     pinctrl-0 = <&vin2_pins>;
>     pinctrl-names = "default";
> 
>     port {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         vin2ep: endpoint {
>             remote-endpoint = <&ov7725_2>;
>             bus-width = <8>;
>             data-shift = <8>;
>         };
>     };
> };
> 
> &vin3 {
>     status = "okay";
>     pinctrl-0 = <&vin3_pins>;
>     pinctrl-names = "default";
> 
>     port {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         vin3ep: endpoint {
>             remote-endpoint = <&ov7725_3>;
>             bus-width = <8>;
>         };
>     };
> };
> 
> 
> > >       switch (vin->parallel->mbus_type) {
> > >       case V4L2_MBUS_PARALLEL:
> > >               vin_dbg(vin, "Found PARALLEL media bus\n");
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 1a30cd036371..5db483877d65 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -127,6 +127,8 @@
> > >  #define VNDMR2_FTEV          (1 << 17)
> > >  #define VNDMR2_VLV(n)                ((n & 0xf) << 12)
> > >
> > > +#define VNDMR2_YDS           BIT(22)
> >
> > This should be grouped with the other VNDMR2_* macros and not on its
> > own. Also it should be sorted so it should be inserted between
> > VNDMR2_CES and VNDMR2_FTEV.
> >
> > Also I know BIT() is a nice macro but the rest of the driver uses (1 <<
> > 22), please do the same for this one.
> >
> Sure will take care of it.
> 
> > > +
> > >  /* Video n CSI2 Interface Mode Register (Gen3) */
> > >  #define VNCSI_IFMD_DES1              (1 << 26)
> > >  #define VNCSI_IFMD_DES0              (1 << 25)
> > > @@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
> > >               /* Data Enable Polarity Select */
> > >               if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
> > >                       dmr2 |= VNDMR2_CES;
> > > +
> > > +             if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
> > > +                     dmr2 |= VNDMR2_YDS;
> > > +             else
> > > +                     dmr2 &= ~VNDMR2_YDS;
> >
> > dmr2 is already unitized and YDS is cleared, no need to clear it again
> > if you don't wish to set it. Taking this and the comments above into
> > account this would become something like (not tested),
> >
> Agreed.
> 
> >     switch (vin->mbus_code) {
> >     case MEDIA_BUS_FMT_UYVY8_2X8:
> >         if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8)
> >             dmr2 |= VNDMR2_YDS;
> >         break;
> >     default:
> >         break;
> >     }
> >
> > >       }
> > >
> > >       /*
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index c19d077ce1cb..3126fee9a89b 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -87,6 +87,9 @@ struct rvin_video_format {
> > >       u8 bpp;
> > >  };
> > >
> > > +#define BUS_WIDTH_8  8
> > > +#define DATA_SHIFT_8 8
> >
> > As pointed out by Geert, not so useful, use 8 in the code :-)
> >
> Agreed will drop it.
> 
> Cheers,
> Prabhakar

Prabhakar Aug. 4, 2020, 8:04 a.m. UTC | #5

Hi Niklas,

On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
>
> Hi Lad,
>
> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > Thank you for the review.
> >
> > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > >
> > > Hi Lad,
> > >
> > > Thanks for your work.
> > >
> > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > bus_width and data_shift passed as part of DT.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > Reviewed-by: Biju Das <[e-mail]>
> > > > ---
> > > > Changes for v2:
> > > > * Dropped DT binding documentation patch
> > > > * Select the data pins depending on bus-width and data-shift
> > >
> > > I like this v2 much better then v1, nice work!
> > >
> > > >
> > > > v1 -
> > > > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
> > > > ---
> > > >  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
> > > >  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
> > > >  drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
> > > >  3 files changed, 17 insertions(+)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > index 7440c8965d27..55005d86928d 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > >       vin->parallel = rvpe;
> > > >       vin->parallel->mbus_type = vep->bus_type;
> > > >
> > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > +
> > >
> > > I would store the bus_width and bus_shift values in the struct
> > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > for this specific use-case..
> > >
> > Ok will do that.
> >
> > > Also according to the documentation is the check correct? Do we not wish
> > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > >
> > bus-width is the actual data lines used, so bus_width == 16 and
> > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > == 8 and bus_shift == 8 should be sufficient.
>
> As you and Geert points out I was wrong, they should indeed both be 8.
>
> >
> > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > is all the driver supports.
> > >
> > Not sure if thats correct.In that case this patch wont make sense, I
> > believed we agreed upon we determine the YDS depending on both
> > bus-width and bus-shift.
>
> I'm sorry I think I lost you :-) The driver is not capable of supporting
> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> different things.
>
> What I tried to say (updated with the knowledge of that bus_width should
> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> allow for a bus_shift value other then 0 or 8? What for example would
> the driver do if the value was 2?
>
I think this should be possible but I am not sure how this will work.
For example on iWave G21D-Q7 platform with 16-bit wired bus say we
connect a 8-bit camera as below:

bus-width = 8 and bus-shift = 2
VI1_G0_B        -> Not connected
VI1_G1_B        -> Not connected
VI1_G2_B_16        -> Connected
VI1_G3_B        -> Connected
VI1_G4_B        -> Connected
VI1_G5_B        -> Connected
VI1_G6_B        -> Connected
VI1_G7_B        -> Connected
VI1_DATA7_B/VI1_B7_B_16    -> Connected
VI1_DATA6_B/VI1_B6_B_16    -> Connected
VI1_DATA5_B/VI1_B5_B_16    -> Not connected
VI1_DATA4_B/VI1_B4_B_16    -> Not connected
VI1_DATA3_B/VI1_B3_B_16    -> Not connected
VI1_DATA2_B/VI1_B2_B_16    -> Not connected
VI1_DATA1_B/VI1_B1_B_16    -> Not connected
VI1_DATA0_B/VI1_B0_B_16    -> Not connected

So in this case for 8-bit YCbCr422 format should YDS be set I am not
sure. Or is this not a valid case at all ?

Cheers,
Prabhakar

> >
> > On iWave G21D-Q7 for VI2 interface VI2_G* pins are connected to SoC
> > and for VIN3 interface Vi3_DATA* pins are connected. So in this case
> > the capture only works for VIN2 only if YDS bit is set for 8-bit 422,
> > and for VIN3 capture only works if YDS is 0
> >
> > &vin2 {
> >     status = "okay";
> >     pinctrl-0 = <&vin2_pins>;
> >     pinctrl-names = "default";
> >
> >     port {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         vin2ep: endpoint {
> >             remote-endpoint = <&ov7725_2>;
> >             bus-width = <8>;
> >             data-shift = <8>;
> >         };
> >     };
> > };
> >
> > &vin3 {
> >     status = "okay";
> >     pinctrl-0 = <&vin3_pins>;
> >     pinctrl-names = "default";
> >
> >     port {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         vin3ep: endpoint {
> >             remote-endpoint = <&ov7725_3>;
> >             bus-width = <8>;
> >         };
> >     };
> > };
> >
> >
> > > >       switch (vin->parallel->mbus_type) {
> > > >       case V4L2_MBUS_PARALLEL:
> > > >               vin_dbg(vin, "Found PARALLEL media bus\n");
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > index 1a30cd036371..5db483877d65 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > @@ -127,6 +127,8 @@
> > > >  #define VNDMR2_FTEV          (1 << 17)
> > > >  #define VNDMR2_VLV(n)                ((n & 0xf) << 12)
> > > >
> > > > +#define VNDMR2_YDS           BIT(22)
> > >
> > > This should be grouped with the other VNDMR2_* macros and not on its
> > > own. Also it should be sorted so it should be inserted between
> > > VNDMR2_CES and VNDMR2_FTEV.
> > >
> > > Also I know BIT() is a nice macro but the rest of the driver uses (1 <<
> > > 22), please do the same for this one.
> > >
> > Sure will take care of it.
> >
> > > > +
> > > >  /* Video n CSI2 Interface Mode Register (Gen3) */
> > > >  #define VNCSI_IFMD_DES1              (1 << 26)
> > > >  #define VNCSI_IFMD_DES0              (1 << 25)
> > > > @@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
> > > >               /* Data Enable Polarity Select */
> > > >               if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
> > > >                       dmr2 |= VNDMR2_CES;
> > > > +
> > > > +             if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
> > > > +                     dmr2 |= VNDMR2_YDS;
> > > > +             else
> > > > +                     dmr2 &= ~VNDMR2_YDS;
> > >
> > > dmr2 is already unitized and YDS is cleared, no need to clear it again
> > > if you don't wish to set it. Taking this and the comments above into
> > > account this would become something like (not tested),
> > >
> > Agreed.
> >
> > >     switch (vin->mbus_code) {
> > >     case MEDIA_BUS_FMT_UYVY8_2X8:
> > >         if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8)
> > >             dmr2 |= VNDMR2_YDS;
> > >         break;
> > >     default:
> > >         break;
> > >     }
> > >
> > > >       }
> > > >
> > > >       /*
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > index c19d077ce1cb..3126fee9a89b 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > @@ -87,6 +87,9 @@ struct rvin_video_format {
> > > >       u8 bpp;
> > > >  };
> > > >
> > > > +#define BUS_WIDTH_8  8
> > > > +#define DATA_SHIFT_8 8
> > >
> > > As pointed out by Geert, not so useful, use 8 in the code :-)
> > >
> > Agreed will drop it.
> >
> > Cheers,
> > Prabhakar
>
> --
> Regards,
> Niklas Söderlund

Niklas Söderlund Aug. 4, 2020, 10:05 a.m. UTC | #6

Hi Lad,

On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> >
> > Hi Lad,
> >
> > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > Thank you for the review.
> > >
> > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > >
> > > > Hi Lad,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > bus_width and data_shift passed as part of DT.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > Reviewed-by: Biju Das <[e-mail]>
> > > > > ---
> > > > > Changes for v2:
> > > > > * Dropped DT binding documentation patch
> > > > > * Select the data pins depending on bus-width and data-shift
> > > >
> > > > I like this v2 much better then v1, nice work!
> > > >
> > > > >
> > > > > v1 -
> > > > > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799
> > > > > ---
> > > > >  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
> > > > >  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
> > > > >  drivers/media/platform/rcar-vin/rcar-vin.h  | 5 +++++
> > > > >  3 files changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > index 7440c8965d27..55005d86928d 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > >       vin->parallel = rvpe;
> > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > >
> > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > +
> > > >
> > > > I would store the bus_width and bus_shift values in the struct
> > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > for this specific use-case..
> > > >
> > > Ok will do that.
> > >
> > > > Also according to the documentation is the check correct? Do we not wish
> > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > >
> > > bus-width is the actual data lines used, so bus_width == 16 and
> > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > == 8 and bus_shift == 8 should be sufficient.
> >
> > As you and Geert points out I was wrong, they should indeed both be 8.
> >
> > >
> > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > is all the driver supports.
> > > >
> > > Not sure if thats correct.In that case this patch wont make sense, I
> > > believed we agreed upon we determine the YDS depending on both
> > > bus-width and bus-shift.
> >
> > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > different things.
> >
> > What I tried to say (updated with the knowledge of that bus_width should
> > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > allow for a bus_shift value other then 0 or 8? What for example would
> > the driver do if the value was 2?
> >
> I think this should be possible but I am not sure how this will work.
> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> connect a 8-bit camera as below:
> 
> bus-width = 8 and bus-shift = 2
> VI1_G0_B        -> Not connected
> VI1_G1_B        -> Not connected
> VI1_G2_B_16        -> Connected
> VI1_G3_B        -> Connected
> VI1_G4_B        -> Connected
> VI1_G5_B        -> Connected
> VI1_G6_B        -> Connected
> VI1_G7_B        -> Connected
> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> VI1_DATA0_B/VI1_B0_B_16    -> Not connected

I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to 
be wired.

> 
> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> sure. Or is this not a valid case at all ?

That is my question :-)

I can't find anything int the documentation that would allow is to do 
anything other then bus-width = 8 together with bus-shift = 0 (do not 
set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you 
check for this and print a warning if bus-shift is anything else :-)

But if you can figured out how we can do a bus-shift = 2 as in your 
example then of course the check is wrong. I have not read the docs 
carefully enough about this to rule it out as impossible.

> 
> Cheers,
> Prabhakar
> 
> > >
> > > On iWave G21D-Q7 for VI2 interface VI2_G* pins are connected to SoC
> > > and for VIN3 interface Vi3_DATA* pins are connected. So in this case
> > > the capture only works for VIN2 only if YDS bit is set for 8-bit 422,
> > > and for VIN3 capture only works if YDS is 0
> > >
> > > &vin2 {
> > >     status = "okay";
> > >     pinctrl-0 = <&vin2_pins>;
> > >     pinctrl-names = "default";
> > >
> > >     port {
> > >         #address-cells = <1>;
> > >         #size-cells = <0>;
> > >
> > >         vin2ep: endpoint {
> > >             remote-endpoint = <&ov7725_2>;
> > >             bus-width = <8>;
> > >             data-shift = <8>;
> > >         };
> > >     };
> > > };
> > >
> > > &vin3 {
> > >     status = "okay";
> > >     pinctrl-0 = <&vin3_pins>;
> > >     pinctrl-names = "default";
> > >
> > >     port {
> > >         #address-cells = <1>;
> > >         #size-cells = <0>;
> > >
> > >         vin3ep: endpoint {
> > >             remote-endpoint = <&ov7725_3>;
> > >             bus-width = <8>;
> > >         };
> > >     };
> > > };
> > >
> > >
> > > > >       switch (vin->parallel->mbus_type) {
> > > > >       case V4L2_MBUS_PARALLEL:
> > > > >               vin_dbg(vin, "Found PARALLEL media bus\n");
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > index 1a30cd036371..5db483877d65 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > @@ -127,6 +127,8 @@
> > > > >  #define VNDMR2_FTEV          (1 << 17)
> > > > >  #define VNDMR2_VLV(n)                ((n & 0xf) << 12)
> > > > >
> > > > > +#define VNDMR2_YDS           BIT(22)
> > > >
> > > > This should be grouped with the other VNDMR2_* macros and not on its
> > > > own. Also it should be sorted so it should be inserted between
> > > > VNDMR2_CES and VNDMR2_FTEV.
> > > >
> > > > Also I know BIT() is a nice macro but the rest of the driver uses (1 <<
> > > > 22), please do the same for this one.
> > > >
> > > Sure will take care of it.
> > >
> > > > > +
> > > > >  /* Video n CSI2 Interface Mode Register (Gen3) */
> > > > >  #define VNCSI_IFMD_DES1              (1 << 26)
> > > > >  #define VNCSI_IFMD_DES0              (1 << 25)
> > > > > @@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > >               /* Data Enable Polarity Select */
> > > > >               if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
> > > > >                       dmr2 |= VNDMR2_CES;
> > > > > +
> > > > > +             if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
> > > > > +                     dmr2 |= VNDMR2_YDS;
> > > > > +             else
> > > > > +                     dmr2 &= ~VNDMR2_YDS;
> > > >
> > > > dmr2 is already unitized and YDS is cleared, no need to clear it again
> > > > if you don't wish to set it. Taking this and the comments above into
> > > > account this would become something like (not tested),
> > > >
> > > Agreed.
> > >
> > > >     switch (vin->mbus_code) {
> > > >     case MEDIA_BUS_FMT_UYVY8_2X8:
> > > >         if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8)
> > > >             dmr2 |= VNDMR2_YDS;
> > > >         break;
> > > >     default:
> > > >         break;
> > > >     }
> > > >
> > > > >       }
> > > > >
> > > > >       /*
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > > index c19d077ce1cb..3126fee9a89b 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > > > @@ -87,6 +87,9 @@ struct rvin_video_format {
> > > > >       u8 bpp;
> > > > >  };
> > > > >
> > > > > +#define BUS_WIDTH_8  8
> > > > > +#define DATA_SHIFT_8 8
> > > >
> > > > As pointed out by Geert, not so useful, use 8 in the code :-)
> > > >
> > > Agreed will drop it.
> > >
> > > Cheers,
> > > Prabhakar
> >
> > --
> > Regards,
> > Niklas Söderlund

Geert Uytterhoeven Aug. 4, 2020, 10:17 a.m. UTC | #7

Hi Niklas, Prabhakar,

On Tue, Aug 4, 2020 at 12:05 PM Niklas <[e-mail]> wrote:
> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> > > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > > bus_width and data_shift passed as part of DT.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > Reviewed-by: Biju Das <[e-mail]>

> > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > > >       vin->parallel = rvpe;
> > > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > > >
> > > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > > +
> > > > >
> > > > > I would store the bus_width and bus_shift values in the struct
> > > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > > for this specific use-case..
> > > > >
> > > > Ok will do that.
> > > >
> > > > > Also according to the documentation is the check correct? Do we not wish
> > > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > >
> > > > bus-width is the actual data lines used, so bus_width == 16 and
> > > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > == 8 and bus_shift == 8 should be sufficient.
> > >
> > > As you and Geert points out I was wrong, they should indeed both be 8.
> > >
> > > >
> > > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > > is all the driver supports.
> > > > >
> > > > Not sure if thats correct.In that case this patch wont make sense, I
> > > > believed we agreed upon we determine the YDS depending on both
> > > > bus-width and bus-shift.
> > >
> > > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > different things.
> > >
> > > What I tried to say (updated with the knowledge of that bus_width should
> > > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > allow for a bus_shift value other then 0 or 8? What for example would
> > > the driver do if the value was 2?
> > >
> > I think this should be possible but I am not sure how this will work.
> > For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > connect a 8-bit camera as below:
> >
> > bus-width = 8 and bus-shift = 2
> > VI1_G0_B        -> Not connected
> > VI1_G1_B        -> Not connected
> > VI1_G2_B_16        -> Connected
> > VI1_G3_B        -> Connected
> > VI1_G4_B        -> Connected
> > VI1_G5_B        -> Connected
> > VI1_G6_B        -> Connected
> > VI1_G7_B        -> Connected
> > VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > VI1_DATA0_B/VI1_B0_B_16    -> Not connected
>
> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> be wired.
>
> >
> > So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > sure. Or is this not a valid case at all ?
>
> That is my question :-)
>
> I can't find anything int the documentation that would allow is to do
> anything other then bus-width = 8 together with bus-shift = 0 (do not
> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> check for this and print a warning if bus-shift is anything else :-)
>
> But if you can figured out how we can do a bus-shift = 2 as in your
> example then of course the check is wrong. I have not read the docs
> carefully enough about this to rule it out as impossible.

IIUIC, this is a completely different scenario than "low" or "high" wiring
of 8-bit YCbCr-422, hence YDS does not apply?

The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
bits unconnected?

Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).

Gr{oetje,eeting}s,

                        Geert

Prabhakar Aug. 4, 2020, 3:12 p.m. UTC | #8

Hi Geert,

On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Niklas, Prabhakar,
>
> On Tue, Aug 4, 2020 at 12:05 PM Niklas <[e-mail]> wrote:
> > On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> > > > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > > > bus_width and data_shift passed as part of DT.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > > Reviewed-by: Biju Das <[e-mail]>
>
> > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > > > >       vin->parallel = rvpe;
> > > > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > > > >
> > > > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > > > +
> > > > > >
> > > > > > I would store the bus_width and bus_shift values in the struct
> > > > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > > > for this specific use-case..
> > > > > >
> > > > > Ok will do that.
> > > > >
> > > > > > Also according to the documentation is the check correct? Do we not wish
> > > > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > > >
> > > > > bus-width is the actual data lines used, so bus_width == 16 and
> > > > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > > == 8 and bus_shift == 8 should be sufficient.
> > > >
> > > > As you and Geert points out I was wrong, they should indeed both be 8.
> > > >
> > > > >
> > > > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > > > is all the driver supports.
> > > > > >
> > > > > Not sure if thats correct.In that case this patch wont make sense, I
> > > > > believed we agreed upon we determine the YDS depending on both
> > > > > bus-width and bus-shift.
> > > >
> > > > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > > different things.
> > > >
> > > > What I tried to say (updated with the knowledge of that bus_width should
> > > > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > > allow for a bus_shift value other then 0 or 8? What for example would
> > > > the driver do if the value was 2?
> > > >
> > > I think this should be possible but I am not sure how this will work.
> > > For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > connect a 8-bit camera as below:
> > >
> > > bus-width = 8 and bus-shift = 2
> > > VI1_G0_B        -> Not connected
> > > VI1_G1_B        -> Not connected
> > > VI1_G2_B_16        -> Connected
> > > VI1_G3_B        -> Connected
> > > VI1_G4_B        -> Connected
> > > VI1_G5_B        -> Connected
> > > VI1_G6_B        -> Connected
> > > VI1_G7_B        -> Connected
> > > VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> >
> > I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > be wired.
> >
> > >
> > > So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > sure. Or is this not a valid case at all ?
> >
> > That is my question :-)
> >
> > I can't find anything int the documentation that would allow is to do
> > anything other then bus-width = 8 together with bus-shift = 0 (do not
> > set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > check for this and print a warning if bus-shift is anything else :-)
> >
> > But if you can figured out how we can do a bus-shift = 2 as in your
> > example then of course the check is wrong. I have not read the docs
> > carefully enough about this to rule it out as impossible.
>
> IIUIC, this is a completely different scenario than "low" or "high" wiring
> of 8-bit YCbCr-422, hence YDS does not apply?
>
I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
as done by this patch. (Although there isn't enough documentation to
prove it)

> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> bits unconnected?
>
B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0

> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
>
YDS mode ?

Chers,
Prabhakar

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Geert Uytterhoeven Aug. 4, 2020, 3:32 p.m. UTC | #9

Hi Prabhakar,

On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar
<[e-mail]> wrote:
> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven <[e-mail]> wrote:
> > On Tue, Aug 4, 2020 at 12:05 PM Niklas <[e-mail]> wrote:
> > > On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > > On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> > > > > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > > > > bus_width and data_shift passed as part of DT.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > > > Reviewed-by: Biju Das <[e-mail]>
> >
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > > > > >       vin->parallel = rvpe;
> > > > > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > > > > >
> > > > > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > > > > +
> > > > > > >
> > > > > > > I would store the bus_width and bus_shift values in the struct
> > > > > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > > > > for this specific use-case..
> > > > > > >
> > > > > > Ok will do that.
> > > > > >
> > > > > > > Also according to the documentation is the check correct? Do we not wish
> > > > > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > > > >
> > > > > > bus-width is the actual data lines used, so bus_width == 16 and
> > > > > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > > > == 8 and bus_shift == 8 should be sufficient.
> > > > >
> > > > > As you and Geert points out I was wrong, they should indeed both be 8.
> > > > >
> > > > > >
> > > > > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > > > > is all the driver supports.
> > > > > > >
> > > > > > Not sure if thats correct.In that case this patch wont make sense, I
> > > > > > believed we agreed upon we determine the YDS depending on both
> > > > > > bus-width and bus-shift.
> > > > >
> > > > > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > > > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > > > different things.
> > > > >
> > > > > What I tried to say (updated with the knowledge of that bus_width should
> > > > > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > > > allow for a bus_shift value other then 0 or 8? What for example would
> > > > > the driver do if the value was 2?
> > > > >
> > > > I think this should be possible but I am not sure how this will work.
> > > > For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > > connect a 8-bit camera as below:
> > > >
> > > > bus-width = 8 and bus-shift = 2
> > > > VI1_G0_B        -> Not connected
> > > > VI1_G1_B        -> Not connected
> > > > VI1_G2_B_16        -> Connected
> > > > VI1_G3_B        -> Connected
> > > > VI1_G4_B        -> Connected
> > > > VI1_G5_B        -> Connected
> > > > VI1_G6_B        -> Connected
> > > > VI1_G7_B        -> Connected
> > > > VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > > VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > > VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > > VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > > VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > > VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > > VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > > VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > >
> > > I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > be wired.
> > >
> > > > So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > > sure. Or is this not a valid case at all ?
> > >
> > > That is my question :-)
> > >
> > > I can't find anything int the documentation that would allow is to do
> > > anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > check for this and print a warning if bus-shift is anything else :-)
> > >
> > > But if you can figured out how we can do a bus-shift = 2 as in your
> > > example then of course the check is wrong. I have not read the docs
> > > carefully enough about this to rule it out as impossible.
> >
> > IIUIC, this is a completely different scenario than "low" or "high" wiring
> > of 8-bit YCbCr-422, hence YDS does not apply?
> >
> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> as done by this patch. (Although there isn't enough documentation to
> prove it)
>
> > The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > bits unconnected?
> >
> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
>
> > Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> >
> YDS mode ?

No, 10-bit YCbCr-422. But please forget my comment, I was looking at
the wrong table.

VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
using just bus-width and bus-shift properties?

Gr{oetje,eeting}s,

                        Geert

Prabhakar Aug. 5, 2020, 8 a.m. UTC | #10

Hi Geert and Niklas,

On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar
> <[e-mail]> wrote:
> > On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Tue, Aug 4, 2020 at 12:05 PM Niklas <[e-mail]> wrote:
> > > > On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > > > On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> > > > > > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > > > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > > > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > > > > > bus_width and data_shift passed as part of DT.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > > > > Reviewed-by: Biju Das <[e-mail]>
> > >
> > > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > > > > > >       vin->parallel = rvpe;
> > > > > > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > > > > > >
> > > > > > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > > > > > +
> > > > > > > >
> > > > > > > > I would store the bus_width and bus_shift values in the struct
> > > > > > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > > > > > for this specific use-case..
> > > > > > > >
> > > > > > > Ok will do that.
> > > > > > >
> > > > > > > > Also according to the documentation is the check correct? Do we not wish
> > > > > > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > > > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > > > > >
> > > > > > > bus-width is the actual data lines used, so bus_width == 16 and
> > > > > > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > > > > == 8 and bus_shift == 8 should be sufficient.
> > > > > >
> > > > > > As you and Geert points out I was wrong, they should indeed both be 8.
> > > > > >
> > > > > > >
> > > > > > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > > > > > is all the driver supports.
> > > > > > > >
> > > > > > > Not sure if thats correct.In that case this patch wont make sense, I
> > > > > > > believed we agreed upon we determine the YDS depending on both
> > > > > > > bus-width and bus-shift.
> > > > > >
> > > > > > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > > > > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > > > > different things.
> > > > > >
> > > > > > What I tried to say (updated with the knowledge of that bus_width should
> > > > > > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > > > > allow for a bus_shift value other then 0 or 8? What for example would
> > > > > > the driver do if the value was 2?
> > > > > >
> > > > > I think this should be possible but I am not sure how this will work.
> > > > > For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > > > connect a 8-bit camera as below:
> > > > >
> > > > > bus-width = 8 and bus-shift = 2
> > > > > VI1_G0_B        -> Not connected
> > > > > VI1_G1_B        -> Not connected
> > > > > VI1_G2_B_16        -> Connected
> > > > > VI1_G3_B        -> Connected
> > > > > VI1_G4_B        -> Connected
> > > > > VI1_G5_B        -> Connected
> > > > > VI1_G6_B        -> Connected
> > > > > VI1_G7_B        -> Connected
> > > > > VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > > > VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > > > VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > > > VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > > > VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > > > VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > > > VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > > > VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > > >
> > > > I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > > be wired.
> > > >
> > > > > So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > > > sure. Or is this not a valid case at all ?
> > > >
> > > > That is my question :-)
> > > >
> > > > I can't find anything int the documentation that would allow is to do
> > > > anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > > set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > > check for this and print a warning if bus-shift is anything else :-)
> > > >
> > > > But if you can figured out how we can do a bus-shift = 2 as in your
> > > > example then of course the check is wrong. I have not read the docs
> > > > carefully enough about this to rule it out as impossible.
> > >
> > > IIUIC, this is a completely different scenario than "low" or "high" wiring
> > > of 8-bit YCbCr-422, hence YDS does not apply?
> > >
> > I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > as done by this patch. (Although there isn't enough documentation to
> > prove it)
> >
> > > The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > > bits unconnected?
> > >
> > B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> >
> > > Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > > Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > >
> > YDS mode ?
>
> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> the wrong table.
>
> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> using just bus-width and bus-shift properties?
>
Yes and here is my explanation.

In Gen1 manual for YDS bit it says the below:
0: Vin_B[7:0] pins
1: Vin_G[7:0] pins

And in Gen2 manual it says,
0: Vin_DATA[7:0] pins
1: Vin_DATA[7:0] pins

On iwave platform for the VIN2 interface the following G pins are connected:

 VI2_G0_MARK, VI2_G1_MARK,
 VI2_G2_MARK, VI2_G3_MARK,
 VI2_G4_MARK, VI2_G5_MARK,
 VI2_G6_MARK, VI2_G7_MARK,

And for capture to work on this interface the YDS bit has to be set.

Now suppose some day we have a platform with 16 bit interface where G
and R pins are connected:

        VI2_G0_MARK, VI2_G1_MARK,
        VI2_G2_MARK, VI2_G3_MARK,
        VI2_G4_MARK, VI2_G5_MARK,
        VI2_G6_MARK, VI2_G7_MARK,
        /* R */
        VI2_R0_MARK, VI2_R1_MARK,
        VI2_R2_MARK, VI2_R3_MARK,
        VI2_R4_MARK, VI2_R5_MARK,
        VI2_R6_MARK, VI2_R7_MARK,

Scenarios
1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
1 for 8-bit YCbCr
2: Say we connect a 8-bit camera just with the R pins - YDS has to be
0 for 8-bit YCbCr
3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
camera - YDS has to be 1 for 8-bit camera

And looking at the Gen1 description of YDS bit, having a combination
of B and G is not a valid case.

So my vote is to have a property in the endpoint to say if YDS has to
be enabled as done in my first version of the patch.

Cheers,
Prabhakar

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Geert Uytterhoeven Aug. 5, 2020, 8:43 a.m. UTC | #11

Hi Prabhakar,

On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar
<[e-mail]> wrote:
> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven <[e-mail]> wrote:
> > On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar
> > <[e-mail]> wrote:
> > > On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > > On Tue, Aug 4, 2020 at 12:05 PM Niklas <[e-mail]> wrote:
> > > > > On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > > > > On Mon, Aug 3, 2020 at 8:28 PM Niklas <[e-mail]> wrote:
> > > > > > > On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > > > > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <[e-mail]> wrote:
> > > > > > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > > > > > > > Select the data pins for YCbCr422-8bit input format depending on
> > > > > > > > > > bus_width and data_shift passed as part of DT.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > > > > > > > Reviewed-by: Biju Das <[e-mail]>
> > > >
> > > > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > > > > > > >       vin->parallel = rvpe;
> > > > > > > > > >       vin->parallel->mbus_type = vep->bus_type;
> > > > > > > > > >
> > > > > > > > > > +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > > > > > > > +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > > > > > > > +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > > > > > > > +             vin->parallel->ycbcr_8b_g = true;
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > I would store the bus_width and bus_shift values in the struct
> > > > > > > > > rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > > > > > > for this specific use-case..
> > > > > > > > >
> > > > > > > > Ok will do that.
> > > > > > > >
> > > > > > > > > Also according to the documentation is the check correct? Do we not wish
> > > > > > > > > to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > > > > > > you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > > > > > >
> > > > > > > > bus-width is the actual data lines used, so bus_width == 16 and
> > > > > > > > bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > > > > > == 8 and bus_shift == 8 should be sufficient.
> > > > > > >
> > > > > > > As you and Geert points out I was wrong, they should indeed both be 8.
> > > > > > >
> > > > > > > >
> > > > > > > > > I think you should also verify that bus_shift is either 0 or 8 as that
> > > > > > > > > is all the driver supports.
> > > > > > > > >
> > > > > > > > Not sure if thats correct.In that case this patch wont make sense, I
> > > > > > > > believed we agreed upon we determine the YDS depending on both
> > > > > > > > bus-width and bus-shift.
> > > > > > >
> > > > > > > I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > > > > > bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > > > > > different things.
> > > > > > >
> > > > > > > What I tried to say (updated with the knowledge of that bus_width should
> > > > > > > indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > > > > > allow for a bus_shift value other then 0 or 8? What for example would
> > > > > > > the driver do if the value was 2?
> > > > > > >
> > > > > > I think this should be possible but I am not sure how this will work.
> > > > > > For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > > > > connect a 8-bit camera as below:
> > > > > >
> > > > > > bus-width = 8 and bus-shift = 2
> > > > > > VI1_G0_B        -> Not connected
> > > > > > VI1_G1_B        -> Not connected
> > > > > > VI1_G2_B_16        -> Connected
> > > > > > VI1_G3_B        -> Connected
> > > > > > VI1_G4_B        -> Connected
> > > > > > VI1_G5_B        -> Connected
> > > > > > VI1_G6_B        -> Connected
> > > > > > VI1_G7_B        -> Connected
> > > > > > VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > > > > VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > > > > VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > > > > VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > > > > VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > > > > VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > > > > VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > > > > VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > > > >
> > > > > I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > > > be wired.
> > > > >
> > > > > > So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > > > > sure. Or is this not a valid case at all ?
> > > > >
> > > > > That is my question :-)
> > > > >
> > > > > I can't find anything int the documentation that would allow is to do
> > > > > anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > > > set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > > > check for this and print a warning if bus-shift is anything else :-)
> > > > >
> > > > > But if you can figured out how we can do a bus-shift = 2 as in your
> > > > > example then of course the check is wrong. I have not read the docs
> > > > > carefully enough about this to rule it out as impossible.
> > > >
> > > > IIUIC, this is a completely different scenario than "low" or "high" wiring
> > > > of 8-bit YCbCr-422, hence YDS does not apply?
> > > >
> > > I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > > as done by this patch. (Although there isn't enough documentation to
> > > prove it)
> > >
> > > > The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > > > bits unconnected?
> > > >
> > > B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > >
> > > > Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > > > Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > > >
> > > YDS mode ?
> >
> > No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > the wrong table.
> >
> > VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > using just bus-width and bus-shift properties?
> >
> Yes and here is my explanation.
>
> In Gen1 manual for YDS bit it says the below:
> 0: Vin_B[7:0] pins
> 1: Vin_G[7:0] pins
>
> And in Gen2 manual it says,
> 0: Vin_DATA[7:0] pins
> 1: Vin_DATA[7:0] pins

Vin_DATA[15:8]

The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
while other SoCs use DATA[23:0], the latter presumably to avoid
confusion when using non-RGB input formats.
R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)

However, the underlying behavior is the same, which is clear from the
RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
component.

> On iwave platform for the VIN2 interface the following G pins are connected:
>
>  VI2_G0_MARK, VI2_G1_MARK,
>  VI2_G2_MARK, VI2_G3_MARK,
>  VI2_G4_MARK, VI2_G5_MARK,
>  VI2_G6_MARK, VI2_G7_MARK,
>
> And for capture to work on this interface the YDS bit has to be set.
>
> Now suppose some day we have a platform with 16 bit interface where G
> and R pins are connected:
>
>         VI2_G0_MARK, VI2_G1_MARK,
>         VI2_G2_MARK, VI2_G3_MARK,
>         VI2_G4_MARK, VI2_G5_MARK,
>         VI2_G6_MARK, VI2_G7_MARK,
>         /* R */
>         VI2_R0_MARK, VI2_R1_MARK,
>         VI2_R2_MARK, VI2_R3_MARK,
>         VI2_R4_MARK, VI2_R5_MARK,
>         VI2_R6_MARK, VI2_R7_MARK,
>
> Scenarios
> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> 1 for 8-bit YCbCr
> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> 0 for 8-bit YCbCr
> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> camera - YDS has to be 1 for 8-bit camera
>
> And looking at the Gen1 description of YDS bit, having a combination
> of B and G is not a valid case.

Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
could work when using 10-bit YCbCr.

> So my vote is to have a property in the endpoint to say if YDS has to
> be enabled as done in my first version of the patch.

"YDS" is not a generic property, "data-shift" is.

Now, how do you specify in DT that RGB-666 mode is to be used?
"bus-width = <18>"?
That won't work with the (so far theoretical) case where both contiguous
and sparse variants are supported by the hardware.  And "data-shift"
won't help here.

Am I overdesigning? ;-)

Gr{oetje,eeting}s,

                        Geert

Laurent Pinchart Aug. 5, 2020, 12:34 p.m. UTC | #12

Hi Geert,

On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> >> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> >>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> >>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> >>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> >>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> >>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> >>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> >>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> >>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> >>>>>>>>>> bus_width and data_shift passed as part of DT.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> >>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> >>>>>>>>>>
> >>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> >>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >>>>>>>>>>       vin->parallel = rvpe;
> >>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> >>>>>>>>>>
> >>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> >>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> >>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> >>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> I would store the bus_width and bus_shift values in the struct
> >>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> >>>>>>>>> for this specific use-case..
> >>>>>>>>>
> >>>>>>>> Ok will do that.
> >>>>>>>>
> >>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> >>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> >>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> >>>>>>>>>
> >>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> >>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> >>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> >>>>>>>
> >>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> >>>>>>>>> is all the driver supports.
> >>>>>>>>>
> >>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> >>>>>>>> believed we agreed upon we determine the YDS depending on both
> >>>>>>>> bus-width and bus-shift.
> >>>>>>>
> >>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> >>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> >>>>>>> different things.
> >>>>>>>
> >>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> >>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> >>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> >>>>>>> the driver do if the value was 2?
> >>>>>>>
> >>>>>> I think this should be possible but I am not sure how this will work.
> >>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> >>>>>> connect a 8-bit camera as below:
> >>>>>>
> >>>>>> bus-width = 8 and bus-shift = 2
> >>>>>> VI1_G0_B        -> Not connected
> >>>>>> VI1_G1_B        -> Not connected
> >>>>>> VI1_G2_B_16        -> Connected
> >>>>>> VI1_G3_B        -> Connected
> >>>>>> VI1_G4_B        -> Connected
> >>>>>> VI1_G5_B        -> Connected
> >>>>>> VI1_G6_B        -> Connected
> >>>>>> VI1_G7_B        -> Connected
> >>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> >>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> >>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> >>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> >>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> >>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> >>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> >>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> >>>>>
> >>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> >>>>> be wired.
> >>>>>
> >>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> >>>>>> sure. Or is this not a valid case at all ?
> >>>>>
> >>>>> That is my question :-)
> >>>>>
> >>>>> I can't find anything int the documentation that would allow is to do
> >>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> >>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> >>>>> check for this and print a warning if bus-shift is anything else :-)
> >>>>>
> >>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> >>>>> example then of course the check is wrong. I have not read the docs
> >>>>> carefully enough about this to rule it out as impossible.
> >>>>
> >>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> >>>> of 8-bit YCbCr-422, hence YDS does not apply?
> >>>>
> >>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> >>> as done by this patch. (Although there isn't enough documentation to
> >>> prove it)
> >>>
> >>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> >>>> bits unconnected?
> >>>>
> >>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> >>>
> >>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> >>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> >>>>
> >>> YDS mode ?
> >>
> >> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> >> the wrong table.
> >>
> >> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> >> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> >> using just bus-width and bus-shift properties?
> >>
> > Yes and here is my explanation.
> >
> > In Gen1 manual for YDS bit it says the below:
> > 0: Vin_B[7:0] pins
> > 1: Vin_G[7:0] pins
> >
> > And in Gen2 manual it says,
> > 0: Vin_DATA[7:0] pins
> > 1: Vin_DATA[7:0] pins
> 
> Vin_DATA[15:8]
> 
> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> while other SoCs use DATA[23:0], the latter presumably to avoid
> confusion when using non-RGB input formats.
> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> 
> However, the underlying behavior is the same, which is clear from the
> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> component.
> 
> > On iwave platform for the VIN2 interface the following G pins are connected:
> >
> >  VI2_G0_MARK, VI2_G1_MARK,
> >  VI2_G2_MARK, VI2_G3_MARK,
> >  VI2_G4_MARK, VI2_G5_MARK,
> >  VI2_G6_MARK, VI2_G7_MARK,
> >
> > And for capture to work on this interface the YDS bit has to be set.
> >
> > Now suppose some day we have a platform with 16 bit interface where G
> > and R pins are connected:
> >
> >         VI2_G0_MARK, VI2_G1_MARK,
> >         VI2_G2_MARK, VI2_G3_MARK,
> >         VI2_G4_MARK, VI2_G5_MARK,
> >         VI2_G6_MARK, VI2_G7_MARK,
> >         /* R */
> >         VI2_R0_MARK, VI2_R1_MARK,
> >         VI2_R2_MARK, VI2_R3_MARK,
> >         VI2_R4_MARK, VI2_R5_MARK,
> >         VI2_R6_MARK, VI2_R7_MARK,
> >
> > Scenarios
> > 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > 1 for 8-bit YCbCr
> > 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > 0 for 8-bit YCbCr
> > 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > camera - YDS has to be 1 for 8-bit camera
> >
> > And looking at the Gen1 description of YDS bit, having a combination
> > of B and G is not a valid case.
> 
> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> could work when using 10-bit YCbCr.
> 
> > So my vote is to have a property in the endpoint to say if YDS has to
> > be enabled as done in my first version of the patch.
> 
> "YDS" is not a generic property, "data-shift" is.

Agreed, we want something standard.

> Now, how do you specify in DT that RGB-666 mode is to be used?
> "bus-width = <18>"?
> That won't work with the (so far theoretical) case where both contiguous
> and sparse variants are supported by the hardware.  And "data-shift"
> won't help here.
> 
> Am I overdesigning? ;-)

If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
subset are used, as it doesn't support any other option. There's no need
for data-shift or any other property, as there's nothing to configure
:-) We could even set bus-width = <24>, it wouldn't make a difference.

Prabhakar Aug. 7, 2020, 8:20 p.m. UTC | #13

Hi Larent/Geert/Niklas,

On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart
<[e-mail]> wrote:
>
> Hi Geert,
>
> On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> > On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > > On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> > >> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> > >>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> > >>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> > >>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > >>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> > >>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > >>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> > >>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > >>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> > >>>>>>>>>> bus_width and data_shift passed as part of DT.
> > >>>>>>>>>>
> > >>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> > >>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> > >>>>>>>>>>
> > >>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > >>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > >>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > >>>>>>>>>>       vin->parallel = rvpe;
> > >>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> > >>>>>>>>>>
> > >>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > >>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > >>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > >>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> > >>>>>>>>>> +
> > >>>>>>>>>
> > >>>>>>>>> I would store the bus_width and bus_shift values in the struct
> > >>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> > >>>>>>>>> for this specific use-case..
> > >>>>>>>>>
> > >>>>>>>> Ok will do that.
> > >>>>>>>>
> > >>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> > >>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > >>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> > >>>>>>>>>
> > >>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> > >>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > >>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> > >>>>>>>
> > >>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> > >>>>>>>
> > >>>>>>>>
> > >>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> > >>>>>>>>> is all the driver supports.
> > >>>>>>>>>
> > >>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> > >>>>>>>> believed we agreed upon we determine the YDS depending on both
> > >>>>>>>> bus-width and bus-shift.
> > >>>>>>>
> > >>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> > >>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > >>>>>>> different things.
> > >>>>>>>
> > >>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> > >>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > >>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> > >>>>>>> the driver do if the value was 2?
> > >>>>>>>
> > >>>>>> I think this should be possible but I am not sure how this will work.
> > >>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > >>>>>> connect a 8-bit camera as below:
> > >>>>>>
> > >>>>>> bus-width = 8 and bus-shift = 2
> > >>>>>> VI1_G0_B        -> Not connected
> > >>>>>> VI1_G1_B        -> Not connected
> > >>>>>> VI1_G2_B_16        -> Connected
> > >>>>>> VI1_G3_B        -> Connected
> > >>>>>> VI1_G4_B        -> Connected
> > >>>>>> VI1_G5_B        -> Connected
> > >>>>>> VI1_G6_B        -> Connected
> > >>>>>> VI1_G7_B        -> Connected
> > >>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > >>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > >>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > >>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > >>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > >>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > >>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > >>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > >>>>>
> > >>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > >>>>> be wired.
> > >>>>>
> > >>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > >>>>>> sure. Or is this not a valid case at all ?
> > >>>>>
> > >>>>> That is my question :-)
> > >>>>>
> > >>>>> I can't find anything int the documentation that would allow is to do
> > >>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> > >>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > >>>>> check for this and print a warning if bus-shift is anything else :-)
> > >>>>>
> > >>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> > >>>>> example then of course the check is wrong. I have not read the docs
> > >>>>> carefully enough about this to rule it out as impossible.
> > >>>>
> > >>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> > >>>> of 8-bit YCbCr-422, hence YDS does not apply?
> > >>>>
> > >>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > >>> as done by this patch. (Although there isn't enough documentation to
> > >>> prove it)
> > >>>
> > >>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > >>>> bits unconnected?
> > >>>>
> > >>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > >>>
> > >>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > >>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > >>>>
> > >>> YDS mode ?
> > >>
> > >> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > >> the wrong table.
> > >>
> > >> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > >> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > >> using just bus-width and bus-shift properties?
> > >>
> > > Yes and here is my explanation.
> > >
> > > In Gen1 manual for YDS bit it says the below:
> > > 0: Vin_B[7:0] pins
> > > 1: Vin_G[7:0] pins
> > >
> > > And in Gen2 manual it says,
> > > 0: Vin_DATA[7:0] pins
> > > 1: Vin_DATA[7:0] pins
> >
> > Vin_DATA[15:8]
> >
> > The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> > while other SoCs use DATA[23:0], the latter presumably to avoid
> > confusion when using non-RGB input formats.
> > R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> >
> > However, the underlying behavior is the same, which is clear from the
> > RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> > DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> > component.
> >
> > > On iwave platform for the VIN2 interface the following G pins are connected:
> > >
> > >  VI2_G0_MARK, VI2_G1_MARK,
> > >  VI2_G2_MARK, VI2_G3_MARK,
> > >  VI2_G4_MARK, VI2_G5_MARK,
> > >  VI2_G6_MARK, VI2_G7_MARK,
> > >
> > > And for capture to work on this interface the YDS bit has to be set.
> > >
> > > Now suppose some day we have a platform with 16 bit interface where G
> > > and R pins are connected:
> > >
> > >         VI2_G0_MARK, VI2_G1_MARK,
> > >         VI2_G2_MARK, VI2_G3_MARK,
> > >         VI2_G4_MARK, VI2_G5_MARK,
> > >         VI2_G6_MARK, VI2_G7_MARK,
> > >         /* R */
> > >         VI2_R0_MARK, VI2_R1_MARK,
> > >         VI2_R2_MARK, VI2_R3_MARK,
> > >         VI2_R4_MARK, VI2_R5_MARK,
> > >         VI2_R6_MARK, VI2_R7_MARK,
> > >
> > > Scenarios
> > > 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > > 1 for 8-bit YCbCr
> > > 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > > 0 for 8-bit YCbCr
> > > 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > > camera - YDS has to be 1 for 8-bit camera
> > >
> > > And looking at the Gen1 description of YDS bit, having a combination
> > > of B and G is not a valid case.
> >
> > Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> > could work when using 10-bit YCbCr.
> >
> > > So my vote is to have a property in the endpoint to say if YDS has to
> > > be enabled as done in my first version of the patch.
> >
> > "YDS" is not a generic property, "data-shift" is.
>
> Agreed, we want something standard.
>
How do we proceed on this ?

> > Now, how do you specify in DT that RGB-666 mode is to be used?
> > "bus-width = <18>"?
> > That won't work with the (so far theoretical) case where both contiguous
> > and sparse variants are supported by the hardware.  And "data-shift"
> > won't help here.
> >
> > Am I overdesigning? ;-)
>
> If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
> subset are used, as it doesn't support any other option. There's no need
> for data-shift or any other property, as there's nothing to configure
> :-) We could even set bus-width = <24>, it wouldn't make a difference.
>
Thank you for jumping Laurent :)

Cheers,
Prabhakar

Laurent Pinchart Aug. 11, 2020, 11:41 a.m. UTC | #14

Hi Prabhakar,

On Fri, Aug 07, 2020 at 09:20:36PM +0100, Lad, Prabhakar wrote:
> On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart wrote:
> > On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> >>> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> >>>> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> >>>>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> >>>>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> >>>>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> >>>>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> >>>>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> >>>>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> >>>>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> >>>>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> >>>>>>>>>>>> bus_width and data_shift passed as part of DT.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> >>>>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> >>>>>>>>>>>>
> >>>>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >>>>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> >>>>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >>>>>>>>>>>>       vin->parallel = rvpe;
> >>>>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> >>>>>>>>>>>>
> >>>>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> >>>>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> >>>>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> >>>>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> >>>>>>>>>>>> +
> >>>>>>>>>>>
> >>>>>>>>>>> I would store the bus_width and bus_shift values in the struct
> >>>>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> >>>>>>>>>>> for this specific use-case..
> >>>>>>>>>>>
> >>>>>>>>>> Ok will do that.
> >>>>>>>>>>
> >>>>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> >>>>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> >>>>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> >>>>>>>>>>>
> >>>>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> >>>>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> >>>>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> >>>>>>>>>
> >>>>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> >>>>>>>>>>> is all the driver supports.
> >>>>>>>>>>>
> >>>>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> >>>>>>>>>> believed we agreed upon we determine the YDS depending on both
> >>>>>>>>>> bus-width and bus-shift.
> >>>>>>>>>
> >>>>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> >>>>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> >>>>>>>>> different things.
> >>>>>>>>>
> >>>>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> >>>>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> >>>>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> >>>>>>>>> the driver do if the value was 2?
> >>>>>>>>>
> >>>>>>>> I think this should be possible but I am not sure how this will work.
> >>>>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> >>>>>>>> connect a 8-bit camera as below:
> >>>>>>>>
> >>>>>>>> bus-width = 8 and bus-shift = 2
> >>>>>>>> VI1_G0_B        -> Not connected
> >>>>>>>> VI1_G1_B        -> Not connected
> >>>>>>>> VI1_G2_B_16        -> Connected
> >>>>>>>> VI1_G3_B        -> Connected
> >>>>>>>> VI1_G4_B        -> Connected
> >>>>>>>> VI1_G5_B        -> Connected
> >>>>>>>> VI1_G6_B        -> Connected
> >>>>>>>> VI1_G7_B        -> Connected
> >>>>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> >>>>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> >>>>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> >>>>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> >>>>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> >>>>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> >>>>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> >>>>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> >>>>>>>
> >>>>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> >>>>>>> be wired.
> >>>>>>>
> >>>>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> >>>>>>>> sure. Or is this not a valid case at all ?
> >>>>>>>
> >>>>>>> That is my question :-)
> >>>>>>>
> >>>>>>> I can't find anything int the documentation that would allow is to do
> >>>>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> >>>>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> >>>>>>> check for this and print a warning if bus-shift is anything else :-)
> >>>>>>>
> >>>>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> >>>>>>> example then of course the check is wrong. I have not read the docs
> >>>>>>> carefully enough about this to rule it out as impossible.
> >>>>>>
> >>>>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> >>>>>> of 8-bit YCbCr-422, hence YDS does not apply?
> >>>>>>
> >>>>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> >>>>> as done by this patch. (Although there isn't enough documentation to
> >>>>> prove it)
> >>>>>
> >>>>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> >>>>>> bits unconnected?
> >>>>>>
> >>>>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> >>>>>
> >>>>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> >>>>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> >>>>>>
> >>>>> YDS mode ?
> >>>>
> >>>> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> >>>> the wrong table.
> >>>>
> >>>> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> >>>> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> >>>> using just bus-width and bus-shift properties?
> >>>>
> >>> Yes and here is my explanation.
> >>>
> >>> In Gen1 manual for YDS bit it says the below:
> >>> 0: Vin_B[7:0] pins
> >>> 1: Vin_G[7:0] pins
> >>>
> >>> And in Gen2 manual it says,
> >>> 0: Vin_DATA[7:0] pins
> >>> 1: Vin_DATA[7:0] pins
> >>
> >> Vin_DATA[15:8]
> >>
> >> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> >> while other SoCs use DATA[23:0], the latter presumably to avoid
> >> confusion when using non-RGB input formats.
> >> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> >>
> >> However, the underlying behavior is the same, which is clear from the
> >> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> >> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> >> component.
> >>
> >>> On iwave platform for the VIN2 interface the following G pins are connected:
> >>>
> >>>  VI2_G0_MARK, VI2_G1_MARK,
> >>>  VI2_G2_MARK, VI2_G3_MARK,
> >>>  VI2_G4_MARK, VI2_G5_MARK,
> >>>  VI2_G6_MARK, VI2_G7_MARK,
> >>>
> >>> And for capture to work on this interface the YDS bit has to be set.
> >>>
> >>> Now suppose some day we have a platform with 16 bit interface where G
> >>> and R pins are connected:
> >>>
> >>>         VI2_G0_MARK, VI2_G1_MARK,
> >>>         VI2_G2_MARK, VI2_G3_MARK,
> >>>         VI2_G4_MARK, VI2_G5_MARK,
> >>>         VI2_G6_MARK, VI2_G7_MARK,
> >>>         /* R */
> >>>         VI2_R0_MARK, VI2_R1_MARK,
> >>>         VI2_R2_MARK, VI2_R3_MARK,
> >>>         VI2_R4_MARK, VI2_R5_MARK,
> >>>         VI2_R6_MARK, VI2_R7_MARK,
> >>>
> >>> Scenarios
> >>> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> >>> 1 for 8-bit YCbCr
> >>> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> >>> 0 for 8-bit YCbCr
> >>> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> >>> camera - YDS has to be 1 for 8-bit camera
> >>>
> >>> And looking at the Gen1 description of YDS bit, having a combination
> >>> of B and G is not a valid case.
> >>
> >> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> >> could work when using 10-bit YCbCr.
> >>
> >>> So my vote is to have a property in the endpoint to say if YDS has to
> >>> be enabled as done in my first version of the patch.
> >>
> >> "YDS" is not a generic property, "data-shift" is.
> >
> > Agreed, we want something standard.
>
> How do we proceed on this ?

I may have lost track of the discussion here. Would a bus-width of 18 or
24, like mentioned below, be enough to solve the problem, or do we need
something else ?

> >> Now, how do you specify in DT that RGB-666 mode is to be used?
> >> "bus-width = <18>"?
> >> That won't work with the (so far theoretical) case where both contiguous
> >> and sparse variants are supported by the hardware.  And "data-shift"
> >> won't help here.
> >>
> >> Am I overdesigning? ;-)
> >
> > If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
> > subset are used, as it doesn't support any other option. There's no need
> > for data-shift or any other property, as there's nothing to configure
> > :-) We could even set bus-width = <24>, it wouldn't make a difference.
>
> Thank you for jumping Laurent :)

Prabhakar Aug. 11, 2020, 5:40 p.m. UTC | #15

Hi Laurent,

On Tue, Aug 11, 2020 at 12:41 PM Laurent Pinchart
<[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Fri, Aug 07, 2020 at 09:20:36PM +0100, Lad, Prabhakar wrote:
> > On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart wrote:
> > > On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> > >> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > >>> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> > >>>> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> > >>>>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> > >>>>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> > >>>>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > >>>>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> > >>>>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > >>>>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> > >>>>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > >>>>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> > >>>>>>>>>>>> bus_width and data_shift passed as part of DT.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> > >>>>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > >>>>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > >>>>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > >>>>>>>>>>>>       vin->parallel = rvpe;
> > >>>>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > >>>>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > >>>>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > >>>>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> > >>>>>>>>>>>> +
> > >>>>>>>>>>>
> > >>>>>>>>>>> I would store the bus_width and bus_shift values in the struct
> > >>>>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> > >>>>>>>>>>> for this specific use-case..
> > >>>>>>>>>>>
> > >>>>>>>>>> Ok will do that.
> > >>>>>>>>>>
> > >>>>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> > >>>>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > >>>>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> > >>>>>>>>>>>
> > >>>>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> > >>>>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > >>>>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> > >>>>>>>>>
> > >>>>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> > >>>>>>>>>>> is all the driver supports.
> > >>>>>>>>>>>
> > >>>>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> > >>>>>>>>>> believed we agreed upon we determine the YDS depending on both
> > >>>>>>>>>> bus-width and bus-shift.
> > >>>>>>>>>
> > >>>>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> > >>>>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > >>>>>>>>> different things.
> > >>>>>>>>>
> > >>>>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> > >>>>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > >>>>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> > >>>>>>>>> the driver do if the value was 2?
> > >>>>>>>>>
> > >>>>>>>> I think this should be possible but I am not sure how this will work.
> > >>>>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > >>>>>>>> connect a 8-bit camera as below:
> > >>>>>>>>
> > >>>>>>>> bus-width = 8 and bus-shift = 2
> > >>>>>>>> VI1_G0_B        -> Not connected
> > >>>>>>>> VI1_G1_B        -> Not connected
> > >>>>>>>> VI1_G2_B_16        -> Connected
> > >>>>>>>> VI1_G3_B        -> Connected
> > >>>>>>>> VI1_G4_B        -> Connected
> > >>>>>>>> VI1_G5_B        -> Connected
> > >>>>>>>> VI1_G6_B        -> Connected
> > >>>>>>>> VI1_G7_B        -> Connected
> > >>>>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > >>>>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > >>>>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > >>>>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > >>>>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > >>>>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > >>>>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > >>>>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > >>>>>>>
> > >>>>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > >>>>>>> be wired.
> > >>>>>>>
> > >>>>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > >>>>>>>> sure. Or is this not a valid case at all ?
> > >>>>>>>
> > >>>>>>> That is my question :-)
> > >>>>>>>
> > >>>>>>> I can't find anything int the documentation that would allow is to do
> > >>>>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> > >>>>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > >>>>>>> check for this and print a warning if bus-shift is anything else :-)
> > >>>>>>>
> > >>>>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> > >>>>>>> example then of course the check is wrong. I have not read the docs
> > >>>>>>> carefully enough about this to rule it out as impossible.
> > >>>>>>
> > >>>>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> > >>>>>> of 8-bit YCbCr-422, hence YDS does not apply?
> > >>>>>>
> > >>>>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > >>>>> as done by this patch. (Although there isn't enough documentation to
> > >>>>> prove it)
> > >>>>>
> > >>>>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > >>>>>> bits unconnected?
> > >>>>>>
> > >>>>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > >>>>>
> > >>>>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > >>>>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > >>>>>>
> > >>>>> YDS mode ?
> > >>>>
> > >>>> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > >>>> the wrong table.
> > >>>>
> > >>>> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > >>>> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > >>>> using just bus-width and bus-shift properties?
> > >>>>
> > >>> Yes and here is my explanation.
> > >>>
> > >>> In Gen1 manual for YDS bit it says the below:
> > >>> 0: Vin_B[7:0] pins
> > >>> 1: Vin_G[7:0] pins
> > >>>
> > >>> And in Gen2 manual it says,
> > >>> 0: Vin_DATA[7:0] pins
> > >>> 1: Vin_DATA[7:0] pins
> > >>
> > >> Vin_DATA[15:8]
> > >>
> > >> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> > >> while other SoCs use DATA[23:0], the latter presumably to avoid
> > >> confusion when using non-RGB input formats.
> > >> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> > >>
> > >> However, the underlying behavior is the same, which is clear from the
> > >> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> > >> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> > >> component.
> > >>
> > >>> On iwave platform for the VIN2 interface the following G pins are connected:
> > >>>
> > >>>  VI2_G0_MARK, VI2_G1_MARK,
> > >>>  VI2_G2_MARK, VI2_G3_MARK,
> > >>>  VI2_G4_MARK, VI2_G5_MARK,
> > >>>  VI2_G6_MARK, VI2_G7_MARK,
> > >>>
> > >>> And for capture to work on this interface the YDS bit has to be set.
> > >>>
> > >>> Now suppose some day we have a platform with 16 bit interface where G
> > >>> and R pins are connected:
> > >>>
> > >>>         VI2_G0_MARK, VI2_G1_MARK,
> > >>>         VI2_G2_MARK, VI2_G3_MARK,
> > >>>         VI2_G4_MARK, VI2_G5_MARK,
> > >>>         VI2_G6_MARK, VI2_G7_MARK,
> > >>>         /* R */
> > >>>         VI2_R0_MARK, VI2_R1_MARK,
> > >>>         VI2_R2_MARK, VI2_R3_MARK,
> > >>>         VI2_R4_MARK, VI2_R5_MARK,
> > >>>         VI2_R6_MARK, VI2_R7_MARK,
> > >>>
> > >>> Scenarios
> > >>> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > >>> 1 for 8-bit YCbCr
> > >>> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > >>> 0 for 8-bit YCbCr
> > >>> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > >>> camera - YDS has to be 1 for 8-bit camera
> > >>>
> > >>> And looking at the Gen1 description of YDS bit, having a combination
> > >>> of B and G is not a valid case.
> > >>
> > >> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> > >> could work when using 10-bit YCbCr.
> > >>
> > >>> So my vote is to have a property in the endpoint to say if YDS has to
> > >>> be enabled as done in my first version of the patch.
> > >>
> > >> "YDS" is not a generic property, "data-shift" is.
> > >
> > > Agreed, we want something standard.
> >
> > How do we proceed on this ?
>
> I may have lost track of the discussion here. Would a bus-width of 18 or
> 24, like mentioned below, be enough to solve the problem, or do we need
> something else ?
>
Sorry maybe I miss-read your email, are you suggesting me to set the
YDS bit to1 if bus-width=18/24 ?

On the RZ/G1H parallel port for VIN2 interface the bus-width=8 since
just the VI2_Gx pins are used YDS has to be set to 1 for it to work.

Cheers,
Prabhakar

> > >> Now, how do you specify in DT that RGB-666 mode is to be used?
> > >> "bus-width = <18>"?
> > >> That won't work with the (so far theoretical) case where both contiguous
> > >> and sparse variants are supported by the hardware.  And "data-shift"
> > >> won't help here.
> > >>
> > >> Am I overdesigning? ;-)
> > >
> > > If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
> > > subset are used, as it doesn't support any other option. There's no need
> > > for data-shift or any other property, as there's nothing to configure
> > > :-) We could even set bus-width = <24>, it wouldn't make a difference.
> >
> > Thank you for jumping Laurent :)
>
> --
> Regards,
>
> Laurent Pinchart

Prabhakar Sept. 3, 2020, 2:54 p.m. UTC | #16

On Tue, Aug 11, 2020 at 6:40 PM Lad, Prabhakar
<[e-mail]> wrote:
>
> Hi Laurent,
>
> On Tue, Aug 11, 2020 at 12:41 PM Laurent Pinchart
> <[e-mail]> wrote:
> >
> > Hi Prabhakar,
> >
> > On Fri, Aug 07, 2020 at 09:20:36PM +0100, Lad, Prabhakar wrote:
> > > On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart wrote:
> > > > On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> > > >> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > > >>> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> > > >>>> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> > > >>>>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> > > >>>>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> > > >>>>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > >>>>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> > > >>>>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > >>>>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> > > >>>>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > >>>>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> > > >>>>>>>>>>>> bus_width and data_shift passed as part of DT.
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> > > >>>>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > >>>>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > >>>>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > >>>>>>>>>>>>       vin->parallel = rvpe;
> > > >>>>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > >>>>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > >>>>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > >>>>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> > > >>>>>>>>>>>> +
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> I would store the bus_width and bus_shift values in the struct
> > > >>>>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> > > >>>>>>>>>>> for this specific use-case..
> > > >>>>>>>>>>>
> > > >>>>>>>>>> Ok will do that.
> > > >>>>>>>>>>
> > > >>>>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> > > >>>>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > >>>>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > >>>>>>>>>>>
> > > >>>>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> > > >>>>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > >>>>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> > > >>>>>>>>>
> > > >>>>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> > > >>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> > > >>>>>>>>>>> is all the driver supports.
> > > >>>>>>>>>>>
> > > >>>>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> > > >>>>>>>>>> believed we agreed upon we determine the YDS depending on both
> > > >>>>>>>>>> bus-width and bus-shift.
> > > >>>>>>>>>
> > > >>>>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > >>>>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > >>>>>>>>> different things.
> > > >>>>>>>>>
> > > >>>>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> > > >>>>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > >>>>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> > > >>>>>>>>> the driver do if the value was 2?
> > > >>>>>>>>>
> > > >>>>>>>> I think this should be possible but I am not sure how this will work.
> > > >>>>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > >>>>>>>> connect a 8-bit camera as below:
> > > >>>>>>>>
> > > >>>>>>>> bus-width = 8 and bus-shift = 2
> > > >>>>>>>> VI1_G0_B        -> Not connected
> > > >>>>>>>> VI1_G1_B        -> Not connected
> > > >>>>>>>> VI1_G2_B_16        -> Connected
> > > >>>>>>>> VI1_G3_B        -> Connected
> > > >>>>>>>> VI1_G4_B        -> Connected
> > > >>>>>>>> VI1_G5_B        -> Connected
> > > >>>>>>>> VI1_G6_B        -> Connected
> > > >>>>>>>> VI1_G7_B        -> Connected
> > > >>>>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > >>>>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > >>>>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > > >>>>>>>
> > > >>>>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > >>>>>>> be wired.
> > > >>>>>>>
> > > >>>>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > >>>>>>>> sure. Or is this not a valid case at all ?
> > > >>>>>>>
> > > >>>>>>> That is my question :-)
> > > >>>>>>>
> > > >>>>>>> I can't find anything int the documentation that would allow is to do
> > > >>>>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > >>>>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > >>>>>>> check for this and print a warning if bus-shift is anything else :-)
> > > >>>>>>>
> > > >>>>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> > > >>>>>>> example then of course the check is wrong. I have not read the docs
> > > >>>>>>> carefully enough about this to rule it out as impossible.
> > > >>>>>>
> > > >>>>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> > > >>>>>> of 8-bit YCbCr-422, hence YDS does not apply?
> > > >>>>>>
> > > >>>>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > > >>>>> as done by this patch. (Although there isn't enough documentation to
> > > >>>>> prove it)
> > > >>>>>
> > > >>>>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > > >>>>>> bits unconnected?
> > > >>>>>>
> > > >>>>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > > >>>>>
> > > >>>>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > > >>>>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > > >>>>>>
> > > >>>>> YDS mode ?
> > > >>>>
> > > >>>> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > > >>>> the wrong table.
> > > >>>>
> > > >>>> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > > >>>> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > > >>>> using just bus-width and bus-shift properties?
> > > >>>>
> > > >>> Yes and here is my explanation.
> > > >>>
> > > >>> In Gen1 manual for YDS bit it says the below:
> > > >>> 0: Vin_B[7:0] pins
> > > >>> 1: Vin_G[7:0] pins
> > > >>>
> > > >>> And in Gen2 manual it says,
> > > >>> 0: Vin_DATA[7:0] pins
> > > >>> 1: Vin_DATA[7:0] pins
> > > >>
> > > >> Vin_DATA[15:8]
> > > >>
> > > >> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> > > >> while other SoCs use DATA[23:0], the latter presumably to avoid
> > > >> confusion when using non-RGB input formats.
> > > >> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> > > >>
> > > >> However, the underlying behavior is the same, which is clear from the
> > > >> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> > > >> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> > > >> component.
> > > >>
> > > >>> On iwave platform for the VIN2 interface the following G pins are connected:
> > > >>>
> > > >>>  VI2_G0_MARK, VI2_G1_MARK,
> > > >>>  VI2_G2_MARK, VI2_G3_MARK,
> > > >>>  VI2_G4_MARK, VI2_G5_MARK,
> > > >>>  VI2_G6_MARK, VI2_G7_MARK,
> > > >>>
> > > >>> And for capture to work on this interface the YDS bit has to be set.
> > > >>>
> > > >>> Now suppose some day we have a platform with 16 bit interface where G
> > > >>> and R pins are connected:
> > > >>>
> > > >>>         VI2_G0_MARK, VI2_G1_MARK,
> > > >>>         VI2_G2_MARK, VI2_G3_MARK,
> > > >>>         VI2_G4_MARK, VI2_G5_MARK,
> > > >>>         VI2_G6_MARK, VI2_G7_MARK,
> > > >>>         /* R */
> > > >>>         VI2_R0_MARK, VI2_R1_MARK,
> > > >>>         VI2_R2_MARK, VI2_R3_MARK,
> > > >>>         VI2_R4_MARK, VI2_R5_MARK,
> > > >>>         VI2_R6_MARK, VI2_R7_MARK,
> > > >>>
> > > >>> Scenarios
> > > >>> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > > >>> 1 for 8-bit YCbCr
> > > >>> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > > >>> 0 for 8-bit YCbCr
> > > >>> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > > >>> camera - YDS has to be 1 for 8-bit camera
> > > >>>
> > > >>> And looking at the Gen1 description of YDS bit, having a combination
> > > >>> of B and G is not a valid case.
> > > >>
> > > >> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> > > >> could work when using 10-bit YCbCr.
> > > >>
> > > >>> So my vote is to have a property in the endpoint to say if YDS has to
> > > >>> be enabled as done in my first version of the patch.
> > > >>
> > > >> "YDS" is not a generic property, "data-shift" is.
> > > >
> > > > Agreed, we want something standard.
> > >
> > > How do we proceed on this ?
> >
> > I may have lost track of the discussion here. Would a bus-width of 18 or
> > 24, like mentioned below, be enough to solve the problem, or do we need
> > something else ?
> >
> Sorry maybe I miss-read your email, are you suggesting me to set the
> YDS bit to1 if bus-width=18/24 ?
>
> On the RZ/G1H parallel port for VIN2 interface the bus-width=8 since
> just the VI2_Gx pins are used YDS has to be set to 1 for it to work.
>
Gentle ping.

Cheers,
Prabhakar
>
> > > >> Now, how do you specify in DT that RGB-666 mode is to be used?
> > > >> "bus-width = <18>"?
> > > >> That won't work with the (so far theoretical) case where both contiguous
> > > >> and sparse variants are supported by the hardware.  And "data-shift"
> > > >> won't help here.
> > > >>
> > > >> Am I overdesigning? ;-)
> > > >
> > > > If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
> > > > subset are used, as it doesn't support any other option. There's no need
> > > > for data-shift or any other property, as there's nothing to configure
> > > > :-) We could even set bus-width = <24>, it wouldn't make a difference.
> > >
> > > Thank you for jumping Laurent :)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Laurent Pinchart Sept. 4, 2020, 2:16 a.m. UTC | #17

Hi Prabhakar,

On Tue, Aug 11, 2020 at 06:40:58PM +0100, Lad, Prabhakar wrote:
> On Tue, Aug 11, 2020 at 12:41 PM Laurent Pinchart wrote:
> > On Fri, Aug 07, 2020 at 09:20:36PM +0100, Lad, Prabhakar wrote:
> > > On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart wrote:
> > > > On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> > > >> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > > >>> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> > > >>>> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> > > >>>>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> > > >>>>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> > > >>>>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > >>>>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> > > >>>>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > >>>>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> > > >>>>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > >>>>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> > > >>>>>>>>>>>> bus_width and data_shift passed as part of DT.
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> > > >>>>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > >>>>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > >>>>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > >>>>>>>>>>>>       vin->parallel = rvpe;
> > > >>>>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > >>>>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > >>>>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > >>>>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> > > >>>>>>>>>>>> +
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> I would store the bus_width and bus_shift values in the struct
> > > >>>>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> > > >>>>>>>>>>> for this specific use-case..
> > > >>>>>>>>>>>
> > > >>>>>>>>>> Ok will do that.
> > > >>>>>>>>>>
> > > >>>>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> > > >>>>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > >>>>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > >>>>>>>>>>>
> > > >>>>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> > > >>>>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > >>>>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> > > >>>>>>>>>
> > > >>>>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> > > >>>>>>>>>
> > > >>>>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> > > >>>>>>>>>>> is all the driver supports.
> > > >>>>>>>>>>>
> > > >>>>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> > > >>>>>>>>>> believed we agreed upon we determine the YDS depending on both
> > > >>>>>>>>>> bus-width and bus-shift.
> > > >>>>>>>>>
> > > >>>>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > >>>>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > >>>>>>>>> different things.
> > > >>>>>>>>>
> > > >>>>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> > > >>>>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > >>>>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> > > >>>>>>>>> the driver do if the value was 2?
> > > >>>>>>>>>
> > > >>>>>>>> I think this should be possible but I am not sure how this will work.
> > > >>>>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > >>>>>>>> connect a 8-bit camera as below:
> > > >>>>>>>>
> > > >>>>>>>> bus-width = 8 and bus-shift = 2
> > > >>>>>>>> VI1_G0_B        -> Not connected
> > > >>>>>>>> VI1_G1_B        -> Not connected
> > > >>>>>>>> VI1_G2_B_16        -> Connected
> > > >>>>>>>> VI1_G3_B        -> Connected
> > > >>>>>>>> VI1_G4_B        -> Connected
> > > >>>>>>>> VI1_G5_B        -> Connected
> > > >>>>>>>> VI1_G6_B        -> Connected
> > > >>>>>>>> VI1_G7_B        -> Connected
> > > >>>>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > >>>>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > >>>>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > >>>>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > > >>>>>>>
> > > >>>>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > >>>>>>> be wired.
> > > >>>>>>>
> > > >>>>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > >>>>>>>> sure. Or is this not a valid case at all ?
> > > >>>>>>>
> > > >>>>>>> That is my question :-)
> > > >>>>>>>
> > > >>>>>>> I can't find anything int the documentation that would allow is to do
> > > >>>>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > >>>>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > >>>>>>> check for this and print a warning if bus-shift is anything else :-)
> > > >>>>>>>
> > > >>>>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> > > >>>>>>> example then of course the check is wrong. I have not read the docs
> > > >>>>>>> carefully enough about this to rule it out as impossible.
> > > >>>>>>
> > > >>>>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> > > >>>>>> of 8-bit YCbCr-422, hence YDS does not apply?
> > > >>>>>>
> > > >>>>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > > >>>>> as done by this patch. (Although there isn't enough documentation to
> > > >>>>> prove it)
> > > >>>>>
> > > >>>>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > > >>>>>> bits unconnected?
> > > >>>>>
> > > >>>>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > > >>>>>
> > > >>>>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > > >>>>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > > >>>>>
> > > >>>>> YDS mode ?
> > > >>>>
> > > >>>> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > > >>>> the wrong table.
> > > >>>>
> > > >>>> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > > >>>> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > > >>>> using just bus-width and bus-shift properties?
> > > >>>>
> > > >>> Yes and here is my explanation.
> > > >>>
> > > >>> In Gen1 manual for YDS bit it says the below:
> > > >>> 0: Vin_B[7:0] pins
> > > >>> 1: Vin_G[7:0] pins
> > > >>>
> > > >>> And in Gen2 manual it says,
> > > >>> 0: Vin_DATA[7:0] pins
> > > >>> 1: Vin_DATA[7:0] pins
> > > >>
> > > >> Vin_DATA[15:8]
> > > >>
> > > >> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> > > >> while other SoCs use DATA[23:0], the latter presumably to avoid
> > > >> confusion when using non-RGB input formats.
> > > >> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> > > >>
> > > >> However, the underlying behavior is the same, which is clear from the
> > > >> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> > > >> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> > > >> component.
> > > >>
> > > >>> On iwave platform for the VIN2 interface the following G pins are connected:
> > > >>>
> > > >>>  VI2_G0_MARK, VI2_G1_MARK,
> > > >>>  VI2_G2_MARK, VI2_G3_MARK,
> > > >>>  VI2_G4_MARK, VI2_G5_MARK,
> > > >>>  VI2_G6_MARK, VI2_G7_MARK,
> > > >>>
> > > >>> And for capture to work on this interface the YDS bit has to be set.
> > > >>>
> > > >>> Now suppose some day we have a platform with 16 bit interface where G
> > > >>> and R pins are connected:
> > > >>>
> > > >>>         VI2_G0_MARK, VI2_G1_MARK,
> > > >>>         VI2_G2_MARK, VI2_G3_MARK,
> > > >>>         VI2_G4_MARK, VI2_G5_MARK,
> > > >>>         VI2_G6_MARK, VI2_G7_MARK,
> > > >>>         /* R */
> > > >>>         VI2_R0_MARK, VI2_R1_MARK,
> > > >>>         VI2_R2_MARK, VI2_R3_MARK,
> > > >>>         VI2_R4_MARK, VI2_R5_MARK,
> > > >>>         VI2_R6_MARK, VI2_R7_MARK,
> > > >>>
> > > >>> Scenarios
> > > >>> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > > >>> 1 for 8-bit YCbCr
> > > >>> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > > >>> 0 for 8-bit YCbCr
> > > >>> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > > >>> camera - YDS has to be 1 for 8-bit camera
> > > >>>
> > > >>> And looking at the Gen1 description of YDS bit, having a combination
> > > >>> of B and G is not a valid case.
> > > >>
> > > >> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> > > >> could work when using 10-bit YCbCr.
> > > >>
> > > >>> So my vote is to have a property in the endpoint to say if YDS has to
> > > >>> be enabled as done in my first version of the patch.
> > > >>
> > > >> "YDS" is not a generic property, "data-shift" is.
> > > >
> > > > Agreed, we want something standard.
> > >
> > > How do we proceed on this ?
> >
> > I may have lost track of the discussion here. Would a bus-width of 18 or
> > 24, like mentioned below, be enough to solve the problem, or do we need
> > something else ?
>
> Sorry maybe I miss-read your email, are you suggesting me to set the
> YDS bit to1 if bus-width=18/24 ?
> 
> On the RZ/G1H parallel port for VIN2 interface the bus-width=8 since
> just the VI2_Gx pins are used YDS has to be set to 1 for it to work.

Yes, sorry, I had misread the discussion.

I think bus-width and data-shift should be set to 8 and 8 in this case,
but that will likely not be enough. The issue is that we need to match
the two ends of the link. With bus-width set to 8, the VIN will assume a
8-bit format will be received. However, the sensor will report sending a
10-bit format, and link validation will fail.

We need either the VIN to expect a 10-bit media bus format, or the
sensor to advertise a 8-bit media bus format. The former isn't possible
with the properties we have in DT today. The later should work if we set
bus-width and data-shift to 10 and 2 on the sensor side. The sensor
driver would need to parse that, and adjust the media bus code
accordingly. That's more work for sensor drivers, which isn't very nice,
but I don't really see what other option we have today.

Would that work for your use case ? YDS would be set when data-shift is
8 on the VIN side.

> > > >> Now, how do you specify in DT that RGB-666 mode is to be used?
> > > >> "bus-width = <18>"?
> > > >> That won't work with the (so far theoretical) case where both contiguous
> > > >> and sparse variants are supported by the hardware.  And "data-shift"
> > > >> won't help here.
> > > >>
> > > >> Am I overdesigning? ;-)
> > > >
> > > > If bus-width = <18>, VIN would know know that the 6 MSBs of each 8-bit
> > > > subset are used, as it doesn't support any other option. There's no need
> > > > for data-shift or any other property, as there's nothing to configure
> > > > :-) We could even set bus-width = <24>, it wouldn't make a difference.
> > >
> > > Thank you for jumping Laurent :)

Prabhakar Sept. 6, 2020, 7 p.m. UTC | #18

Hi Laurent,

On Fri, Sep 4, 2020 at 3:16 AM Laurent Pinchart
<[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Aug 11, 2020 at 06:40:58PM +0100, Lad, Prabhakar wrote:
> > On Tue, Aug 11, 2020 at 12:41 PM Laurent Pinchart wrote:
> > > On Fri, Aug 07, 2020 at 09:20:36PM +0100, Lad, Prabhakar wrote:
> > > > On Wed, Aug 5, 2020 at 1:35 PM Laurent Pinchart wrote:
> > > > > On Wed, Aug 05, 2020 at 10:43:25AM +0200, Geert Uytterhoeven wrote:
> > > > >> On Wed, Aug 5, 2020 at 10:01 AM Lad, Prabhakar wrote:
> > > > >>> On Tue, Aug 4, 2020 at 4:32 PM Geert Uytterhoeven wrote:
> > > > >>>> On Tue, Aug 4, 2020 at 5:12 PM Lad, Prabhakar wrote:
> > > > >>>>> On Tue, Aug 4, 2020 at 11:17 AM Geert Uytterhoeven wrote:
> > > > >>>>>> On Tue, Aug 4, 2020 at 12:05 PM Niklas wrote:
> > > > >>>>>>> On 2020-08-04 09:04:25 +0100, Lad, Prabhakar wrote:
> > > > >>>>>>>> On Mon, Aug 3, 2020 at 8:28 PM Niklas wrote:
> > > > >>>>>>>>> On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote:
> > > > >>>>>>>>>> On Mon, Aug 3, 2020 at 7:06 PM Niklas wrote:
> > > > >>>>>>>>>>> On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote:
> > > > >>>>>>>>>>>> Select the data pins for YCbCr422-8bit input format depending on
> > > > >>>>>>>>>>>> bus_width and data_shift passed as part of DT.
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> Signed-off-by: Lad Prabhakar <[e-mail]>
> > > > >>>>>>>>>>>> Reviewed-by: Biju Das <[e-mail]>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > >>>>>>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > >>>>>>>>>>>> @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > > > >>>>>>>>>>>>       vin->parallel = rvpe;
> > > > >>>>>>>>>>>>       vin->parallel->mbus_type = vep->bus_type;
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> +     /* select VInDATA[15:8] pins for YCbCr422-8bit format */
> > > > >>>>>>>>>>>> +     if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
> > > > >>>>>>>>>>>> +         vep->bus.parallel.data_shift == DATA_SHIFT_8)
> > > > >>>>>>>>>>>> +             vin->parallel->ycbcr_8b_g = true;
> > > > >>>>>>>>>>>> +
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> I would store the bus_width and bus_shift values in the struct
> > > > >>>>>>>>>>> rvin_parallel_entity and evaluate them in place rater then create a flag
> > > > >>>>>>>>>>> for this specific use-case..
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>> Ok will do that.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>> Also according to the documentation is the check correct? Do we not wish
> > > > >>>>>>>>>>> to use the new mode when bus_width == 16 and bus_shift == 8. The check
> > > > >>>>>>>>>>> you have here seems to describe a 8 lane bus where 0 lanes are used.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>> bus-width is the actual data lines used, so bus_width == 16 and
> > > > >>>>>>>>>> bus_shift == 8 would mean use lines 23:8, so just check for bus_width
> > > > >>>>>>>>>> == 8 and bus_shift == 8 should be sufficient.
> > > > >>>>>>>>>
> > > > >>>>>>>>> As you and Geert points out I was wrong, they should indeed both be 8.
> > > > >>>>>>>>>
> > > > >>>>>>>>>>> I think you should also verify that bus_shift is either 0 or 8 as that
> > > > >>>>>>>>>>> is all the driver supports.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>> Not sure if thats correct.In that case this patch wont make sense, I
> > > > >>>>>>>>>> believed we agreed upon we determine the YDS depending on both
> > > > >>>>>>>>>> bus-width and bus-shift.
> > > > >>>>>>>>>
> > > > >>>>>>>>> I'm sorry I think I lost you :-) The driver is not capable of supporting
> > > > >>>>>>>>> bus_width = 8 and bus_shift = 2 right? Maybe we are talking about
> > > > >>>>>>>>> different things.
> > > > >>>>>>>>>
> > > > >>>>>>>>> What I tried to say (updated with the knowledge of that bus_width should
> > > > >>>>>>>>> indeed be 8 and not 16) was that would it make sens to with bus_width=8
> > > > >>>>>>>>> allow for a bus_shift value other then 0 or 8? What for example would
> > > > >>>>>>>>> the driver do if the value was 2?
> > > > >>>>>>>>>
> > > > >>>>>>>> I think this should be possible but I am not sure how this will work.
> > > > >>>>>>>> For example on iWave G21D-Q7 platform with 16-bit wired bus say we
> > > > >>>>>>>> connect a 8-bit camera as below:
> > > > >>>>>>>>
> > > > >>>>>>>> bus-width = 8 and bus-shift = 2
> > > > >>>>>>>> VI1_G0_B        -> Not connected
> > > > >>>>>>>> VI1_G1_B        -> Not connected
> > > > >>>>>>>> VI1_G2_B_16        -> Connected
> > > > >>>>>>>> VI1_G3_B        -> Connected
> > > > >>>>>>>> VI1_G4_B        -> Connected
> > > > >>>>>>>> VI1_G5_B        -> Connected
> > > > >>>>>>>> VI1_G6_B        -> Connected
> > > > >>>>>>>> VI1_G7_B        -> Connected
> > > > >>>>>>>> VI1_DATA7_B/VI1_B7_B_16    -> Connected
> > > > >>>>>>>> VI1_DATA6_B/VI1_B6_B_16    -> Connected
> > > > >>>>>>>> VI1_DATA5_B/VI1_B5_B_16    -> Not connected
> > > > >>>>>>>> VI1_DATA4_B/VI1_B4_B_16    -> Not connected
> > > > >>>>>>>> VI1_DATA3_B/VI1_B3_B_16    -> Not connected
> > > > >>>>>>>> VI1_DATA2_B/VI1_B2_B_16    -> Not connected
> > > > >>>>>>>> VI1_DATA1_B/VI1_B1_B_16    -> Not connected
> > > > >>>>>>>> VI1_DATA0_B/VI1_B0_B_16    -> Not connected
> > > > >>>>>>>
> > > > >>>>>>> I agree this is how I would imagine bus-width = 8 and bus-shift = 2 to
> > > > >>>>>>> be wired.
> > > > >>>>>>>
> > > > >>>>>>>> So in this case for 8-bit YCbCr422 format should YDS be set I am not
> > > > >>>>>>>> sure. Or is this not a valid case at all ?
> > > > >>>>>>>
> > > > >>>>>>> That is my question :-)
> > > > >>>>>>>
> > > > >>>>>>> I can't find anything int the documentation that would allow is to do
> > > > >>>>>>> anything other then bus-width = 8 together with bus-shift = 0 (do not
> > > > >>>>>>> set YDS) or bus-shift = 8 (set YDS). So that is why I suggested you
> > > > >>>>>>> check for this and print a warning if bus-shift is anything else :-)
> > > > >>>>>>>
> > > > >>>>>>> But if you can figured out how we can do a bus-shift = 2 as in your
> > > > >>>>>>> example then of course the check is wrong. I have not read the docs
> > > > >>>>>>> carefully enough about this to rule it out as impossible.
> > > > >>>>>>
> > > > >>>>>> IIUIC, this is a completely different scenario than "low" or "high" wiring
> > > > >>>>>> of 8-bit YCbCr-422, hence YDS does not apply?
> > > > >>>>>>
> > > > >>>>> I tend to agree. We only enable YDS if bus-width = 8 and bus-shift=8
> > > > >>>>> as done by this patch. (Although there isn't enough documentation to
> > > > >>>>> prove it)
> > > > >>>>>
> > > > >>>>>> The iWave G21D-Q7 wiring seems to be 10-bit YCbCr-422 with the 2 LSB
> > > > >>>>>> bits unconnected?
> > > > >>>>>
> > > > >>>>> B-8bit/ BG-16 bit for VI0 and  B-8bit/ BG-16 bit for VI0
> > > > >>>>>
> > > > >>>>>> Interestingly, that mode is supported on all RZ/G1 SoCs, on most R-Car
> > > > >>>>>> Gen3 SoCs, but only on a single R-Car Gen2 SoC (V2H).
> > > > >>>>>
> > > > >>>>> YDS mode ?
> > > > >>>>
> > > > >>>> No, 10-bit YCbCr-422. But please forget my comment, I was looking at
> > > > >>>> the wrong table.
> > > > >>>>
> > > > >>>> VI1_G[7:2] plus VI1_DATA[7:6] is not even a contiguous subset (I had
> > > > >>>> misread the used subset to be G[1:0] and B[7:2]), so it cannot be represented
> > > > >>>> using just bus-width and bus-shift properties?
> > > > >>>>
> > > > >>> Yes and here is my explanation.
> > > > >>>
> > > > >>> In Gen1 manual for YDS bit it says the below:
> > > > >>> 0: Vin_B[7:0] pins
> > > > >>> 1: Vin_G[7:0] pins
> > > > >>>
> > > > >>> And in Gen2 manual it says,
> > > > >>> 0: Vin_DATA[7:0] pins
> > > > >>> 1: Vin_DATA[7:0] pins
> > > > >>
> > > > >> Vin_DATA[15:8]
> > > > >>
> > > > >> The difference is due to some SoCs naming the signals R[7:0], G[7:0], B[7:0],
> > > > >> while other SoCs use DATA[23:0], the latter presumably to avoid
> > > > >> confusion when using non-RGB input formats.
> > > > >> R-Car V2H uses a mix: D[23:16]_R[7:0], D[15:8]_G[7:0], D[7:0]_B[7:0] ;-)
> > > > >>
> > > > >> However, the underlying behavior is the same, which is clear from the
> > > > >> RGB-666 mode, which is not using contiguous DATA[17:0], but sparse
> > > > >> DATA[23:18], DATA[15:10], DATA[7:2], i.e. the 6 MSB of each color
> > > > >> component.
> > > > >>
> > > > >>> On iwave platform for the VIN2 interface the following G pins are connected:
> > > > >>>
> > > > >>>  VI2_G0_MARK, VI2_G1_MARK,
> > > > >>>  VI2_G2_MARK, VI2_G3_MARK,
> > > > >>>  VI2_G4_MARK, VI2_G5_MARK,
> > > > >>>  VI2_G6_MARK, VI2_G7_MARK,
> > > > >>>
> > > > >>> And for capture to work on this interface the YDS bit has to be set.
> > > > >>>
> > > > >>> Now suppose some day we have a platform with 16 bit interface where G
> > > > >>> and R pins are connected:
> > > > >>>
> > > > >>>         VI2_G0_MARK, VI2_G1_MARK,
> > > > >>>         VI2_G2_MARK, VI2_G3_MARK,
> > > > >>>         VI2_G4_MARK, VI2_G5_MARK,
> > > > >>>         VI2_G6_MARK, VI2_G7_MARK,
> > > > >>>         /* R */
> > > > >>>         VI2_R0_MARK, VI2_R1_MARK,
> > > > >>>         VI2_R2_MARK, VI2_R3_MARK,
> > > > >>>         VI2_R4_MARK, VI2_R5_MARK,
> > > > >>>         VI2_R6_MARK, VI2_R7_MARK,
> > > > >>>
> > > > >>> Scenarios
> > > > >>> 1: Say we connect a 8-bit camera just  with the G pins - YDS has to be
> > > > >>> 1 for 8-bit YCbCr
> > > > >>> 2: Say we connect a 8-bit camera just with the R pins - YDS has to be
> > > > >>> 0 for 8-bit YCbCr
> > > > >>> 3: Now say we use G2-G7 along with R0 and R1 pins to connect a 8 bit
> > > > >>> camera - YDS has to be 1 for 8-bit camera
> > > > >>>
> > > > >>> And looking at the Gen1 description of YDS bit, having a combination
> > > > >>> of B and G is not a valid case.
> > > > >>
> > > > >> Scenario 3 is indeed not supported. But G[1:0] and B[7:2] (= DATA[9:2])
> > > > >> could work when using 10-bit YCbCr.
> > > > >>
> > > > >>> So my vote is to have a property in the endpoint to say if YDS has to
> > > > >>> be enabled as done in my first version of the patch.
> > > > >>
> > > > >> "YDS" is not a generic property, "data-shift" is.
> > > > >
> > > > > Agreed, we want something standard.
> > > >
> > > > How do we proceed on this ?
> > >
> > > I may have lost track of the discussion here. Would a bus-width of 18 or
> > > 24, like mentioned below, be enough to solve the problem, or do we need
> > > something else ?
> >
> > Sorry maybe I miss-read your email, are you suggesting me to set the
> > YDS bit to1 if bus-width=18/24 ?
> >
> > On the RZ/G1H parallel port for VIN2 interface the bus-width=8 since
> > just the VI2_Gx pins are used YDS has to be set to 1 for it to work.
>
> Yes, sorry, I had misread the discussion.
>
> I think bus-width and data-shift should be set to 8 and 8 in this case,
> but that will likely not be enough. The issue is that we need to match
> the two ends of the link. With bus-width set to 8, the VIN will assume a
> 8-bit format will be received. However, the sensor will report sending a
> 10-bit format, and link validation will fail.
>
On the VIN side when bus-width and data-shift is set to 8, even on
sensor end bus-width would be set to 8 if I am not wrong.

> We need either the VIN to expect a 10-bit media bus format, or the
> sensor to advertise a 8-bit media bus format. The former isn't possible
> with the properties we have in DT today. The later should work if we set
> bus-width and data-shift to 10 and 2 on the sensor side. The sensor
> driver would need to parse that, and adjust the media bus code
> accordingly. That's more work for sensor drivers, which isn't very nice,
> but I don't really see what other option we have today.
>
Agreed.

> Would that work for your use case ? YDS would be set when data-shift is
> 8 on the VIN side.
>
Looking again at the RZ/G2 manual, Table 26.8.1 " Channel 4 Data Pin
Connections" and Table 26.9.1 "Channel 5 Data Pin Connections "
YDS=1, data-shift=8 and bus-width=8.
Compared with RZ/G1 manual, Table 23.5  "Channel 0 Data Pin
Connections", Table 23.6 " Channel 1 Data Pin Connections" and Table
23.7 "Channel 2 Data Pin Connections" YDS=1, data-shift=8 and
bus-width=8.

This patch does the same where it checks for data-shift=8 and
bus-width=8 and the set YDS bit.

Cheers,
Prabhakar

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c8965d27..55005d86928d 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -624,6 +624,11 @@  static int rvin_parallel_parse_v4l2(struct device *dev,
 	vin->parallel = rvpe;
 	vin->parallel->mbus_type = vep->bus_type;
 
+	/* select VInDATA[15:8] pins for YCbCr422-8bit format */
+	if (vep->bus.parallel.bus_width == BUS_WIDTH_8 &&
+	    vep->bus.parallel.data_shift == DATA_SHIFT_8)
+		vin->parallel->ycbcr_8b_g = true;
+
 	switch (vin->parallel->mbus_type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd036371..5db483877d65 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -127,6 +127,8 @@ 
 #define VNDMR2_FTEV		(1 << 17)
 #define VNDMR2_VLV(n)		((n & 0xf) << 12)
 
+#define VNDMR2_YDS		BIT(22)
+
 /* Video n CSI2 Interface Mode Register (Gen3) */
 #define VNCSI_IFMD_DES1		(1 << 26)
 #define VNCSI_IFMD_DES0		(1 << 25)
@@ -698,6 +700,11 @@  static int rvin_setup(struct rvin_dev *vin)
 		/* Data Enable Polarity Select */
 		if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
 			dmr2 |= VNDMR2_CES;
+
+		if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
+			dmr2 |= VNDMR2_YDS;
+		else
+			dmr2 &= ~VNDMR2_YDS;
 	}
 
 	/*
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c19d077ce1cb..3126fee9a89b 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -87,6 +87,9 @@  struct rvin_video_format {
 	u8 bpp;
 };
 
+#define BUS_WIDTH_8	8
+#define DATA_SHIFT_8	8
+
 /**
  * struct rvin_parallel_entity - Parallel video input endpoint descriptor
  * @asd:	sub-device descriptor for async framework
@@ -95,6 +98,7 @@  struct rvin_video_format {
  * @mbus_flags:	media bus configuration flags
  * @source_pad:	source pad of remote subdevice
  * @sink_pad:	sink pad of remote subdevice
+ * @ycbcr_8b_g:	select data pins for YCbCr422-8bit
  *
  */
 struct rvin_parallel_entity {
@@ -106,6 +110,7 @@  struct rvin_parallel_entity {
 
 	unsigned int source_pad;
 	unsigned int sink_pad;
+	bool ycbcr_8b_g;
 };
 
 /**