[PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Niklas Söderlund]

Earlier versions of the datasheet where unclear about the stride setting
for RAW8 capture formats. Later datasheets clarifies that the stride
only process in this mode for non-image data. For image data the full
stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".

Remove the special case from pixel formats that carry image data and
treat it as any other image format.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index f8394be8a922..fdf0f86c801f 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 
 	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
 	stride = vin->format.bytesperline / fmt->bpp;
-
-	/* For RAW8 format bpp is 1, but the hardware process RAW8
-	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
-	 */
-	switch (vin->format.pixelformat) {
-	case V4L2_PIX_FMT_SBGGR8:
-	case V4L2_PIX_FMT_SGBRG8:
-	case V4L2_PIX_FMT_SGRBG8:
-	case V4L2_PIX_FMT_SRGGB8:
-	case V4L2_PIX_FMT_GREY:
-		stride /= 2;
-		break;
-	default:
-		break;
-	}
-
 	rvin_write(vin, stride, VNIS_REG);
 }
 
-- 
2.49.0

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Apr 02, 2025 at 08:33:02PM +0200, Niklas Söderlund wrote:
> Earlier versions of the datasheet where unclear about the stride setting
> for RAW8 capture formats. Later datasheets clarifies that the stride
> only process in this mode for non-image data. For image data the full
> stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".

I have a hard time telling anything from the documentation. Assuming
you've tested this to be correct,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Remove the special case from pixel formats that carry image data and
> treat it as any other image format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index f8394be8a922..fdf0f86c801f 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  
>  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
>  	stride = vin->format.bytesperline / fmt->bpp;
> -
> -	/* For RAW8 format bpp is 1, but the hardware process RAW8
> -	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> -	 */
> -	switch (vin->format.pixelformat) {
> -	case V4L2_PIX_FMT_SBGGR8:
> -	case V4L2_PIX_FMT_SGBRG8:
> -	case V4L2_PIX_FMT_SGRBG8:
> -	case V4L2_PIX_FMT_SRGGB8:
> -	case V4L2_PIX_FMT_GREY:
> -		stride /= 2;
> -		break;
> -	default:
> -		break;
> -	}
> -
>  	rvin_write(vin, stride, VNIS_REG);
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Tomi Valkeinen]

Hi,

On 02/04/2025 21:33, Niklas Söderlund wrote:
> Earlier versions of the datasheet where unclear about the stride setting
> for RAW8 capture formats. Later datasheets clarifies that the stride
> only process in this mode for non-image data. For image data the full
> stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".
> 
> Remove the special case from pixel formats that carry image data and
> treat it as any other image format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index f8394be8a922..fdf0f86c801f 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  
>  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
>  	stride = vin->format.bytesperline / fmt->bpp;
> -
> -	/* For RAW8 format bpp is 1, but the hardware process RAW8
> -	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> -	 */
> -	switch (vin->format.pixelformat) {
> -	case V4L2_PIX_FMT_SBGGR8:
> -	case V4L2_PIX_FMT_SGBRG8:
> -	case V4L2_PIX_FMT_SGRBG8:
> -	case V4L2_PIX_FMT_SRGGB8:
> -	case V4L2_PIX_FMT_GREY:
> -		stride /= 2;
> -		break;
> -	default:
> -		break;
> -	}
> -
>  	rvin_write(vin, stride, VNIS_REG);
>  }
>  

This one breaks RAW8 for me (V4H, streams, GMSL2 + imx219 sensors). How
did you test this?

 Tomi

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Tomi Valkeinen]

Hi,

On 02/04/2025 21:33, Niklas Söderlund wrote:
> Earlier versions of the datasheet where unclear about the stride setting
> for RAW8 capture formats. Later datasheets clarifies that the stride
> only process in this mode for non-image data. For image data the full
> stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".

Looking at the V4H manual, I see:

"
RAW: 8 Bits and Embedded 8-Bit Non-Image Data, User Defined 8-bit Data

The 8-bit RAW image data format, Embedded 8-bit non image data and User
Defined 8-bit data format are shown below. Notes: 1. These data are
transferred by 2-Byte unit.
"

and

"
RAW: 8 Bits (16 Bits/Pixel) Data

The 8-bit RAW (16 bits/pixel) image data format is shown below.
Notes: 1. These data are transferred by 2-Byte unit.
"

I don't see "RAW: 8 Bits". In both cases above the text says "2-byte unit".

 Tomi

> 
> Remove the special case from pixel formats that carry image data and
> treat it as any other image format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index f8394be8a922..fdf0f86c801f 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  
>  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
>  	stride = vin->format.bytesperline / fmt->bpp;
> -
> -	/* For RAW8 format bpp is 1, but the hardware process RAW8
> -	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> -	 */
> -	switch (vin->format.pixelformat) {
> -	case V4L2_PIX_FMT_SBGGR8:
> -	case V4L2_PIX_FMT_SGBRG8:
> -	case V4L2_PIX_FMT_SGRBG8:
> -	case V4L2_PIX_FMT_SRGGB8:
> -	case V4L2_PIX_FMT_GREY:
> -		stride /= 2;
> -		break;
> -	default:
> -		break;
> -	}
> -
>  	rvin_write(vin, stride, VNIS_REG);
>  }
>  

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Niklas Söderlund]

Hi Tomi,

On 2025-05-30 14:58:17 +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/04/2025 21:33, Niklas Söderlund wrote:
> > Earlier versions of the datasheet where unclear about the stride setting
> > for RAW8 capture formats. Later datasheets clarifies that the stride
> > only process in this mode for non-image data. For image data the full
> > stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> > Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".
> > 
> > Remove the special case from pixel formats that carry image data and
> > treat it as any other image format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
> >  1 file changed, 16 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index f8394be8a922..fdf0f86c801f 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  
> >  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> >  	stride = vin->format.bytesperline / fmt->bpp;
> > -
> > -	/* For RAW8 format bpp is 1, but the hardware process RAW8
> > -	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> > -	 */
> > -	switch (vin->format.pixelformat) {
> > -	case V4L2_PIX_FMT_SBGGR8:
> > -	case V4L2_PIX_FMT_SGBRG8:
> > -	case V4L2_PIX_FMT_SGRBG8:
> > -	case V4L2_PIX_FMT_SRGGB8:
> > -	case V4L2_PIX_FMT_GREY:
> > -		stride /= 2;
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -
> >  	rvin_write(vin, stride, VNIS_REG);
> >  }
> >  
> 
> This one breaks RAW8 for me (V4H, streams, GMSL2 + imx219 sensors). How
> did you test this?

I tested this with V4H and IMX219 and IMX462.

As you point out in your other reply the documentation is a tad unclear.  
I don't have access to it atm, but from memory there where two 8-bit 
modes. One operated in 1-byte units, the other in 2-byte units. This fix 
addresses the 1-byt use-case.

I will get back to you when I'm back in the office with better 
references to the documentation. Just wanted to let you know how this 
was tested.

> 
>  Tomi
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Geert Uytterhoeven]

Hi Tomi,

On Mon, 2 Jun 2025 at 09:12, Tomi Valkeinen
<tomi.valkeinen+renesas@ideasonboard.com> wrote:
> On 02/04/2025 21:33, Niklas Söderlund wrote:
> > Earlier versions of the datasheet where unclear about the stride setting
> > for RAW8 capture formats. Later datasheets clarifies that the stride
> > only process in this mode for non-image data. For image data the full
> > stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> > Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".
> >
> > Remove the special case from pixel formats that carry image data and
> > treat it as any other image format.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
> >  1 file changed, 16 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index f8394be8a922..fdf0f86c801f 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >
> >       fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> >       stride = vin->format.bytesperline / fmt->bpp;
> > -
> > -     /* For RAW8 format bpp is 1, but the hardware process RAW8
> > -      * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> > -      */
> > -     switch (vin->format.pixelformat) {
> > -     case V4L2_PIX_FMT_SBGGR8:
> > -     case V4L2_PIX_FMT_SGBRG8:
> > -     case V4L2_PIX_FMT_SGRBG8:
> > -     case V4L2_PIX_FMT_SRGGB8:
> > -     case V4L2_PIX_FMT_GREY:
> > -             stride /= 2;
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -
> >       rvin_write(vin, stride, VNIS_REG);
> >  }
> >
>
> This one breaks RAW8 for me (V4H, streams, GMSL2 + imx219 sensors). How
> did you test this?

Note that this is being backported right now:
https://lore.kernel.org/all/20250601234012.3516352-39-sashal@kernel.org/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] media: rcar-vin: Fix stride setting for RAW8 formats

[Niklas Söderlund]

Hi Tomi,

On 2025-05-30 15:08:56 +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/04/2025 21:33, Niklas Söderlund wrote:
> > Earlier versions of the datasheet where unclear about the stride setting
> > for RAW8 capture formats. Later datasheets clarifies that the stride
> > only process in this mode for non-image data. For image data the full
> > stride shall be used. Compare section "RAW: 8 Bits and Embedded 8-Bit
> > Non-Image Data, User Defined 8-bit Data" vs "RAW: 8 Bits".
> 
> Looking at the V4H manual, I see:
> 
> "
> RAW: 8 Bits and Embedded 8-Bit Non-Image Data, User Defined 8-bit Data
> 
> The 8-bit RAW image data format, Embedded 8-bit non image data and User
> Defined 8-bit data format are shown below. Notes: 1. These data are
> transferred by 2-Byte unit.
> "
> 
> and
> 
> "
> RAW: 8 Bits (16 Bits/Pixel) Data
> 
> The 8-bit RAW (16 bits/pixel) image data format is shown below.
> Notes: 1. These data are transferred by 2-Byte unit.
> "
> 
> I don't see "RAW: 8 Bits". In both cases above the text says "2-byte unit".

I agree this is all priorly documented. My understanding and this fix 
comes from the two diagrams attached to the two different sections you 
point out.

In the first case "(12) RAW: 8 Bits and Embedded 8-Bit Non-Image Data, 
User Defined 8-bit Data" the diagram (for even number of pixels) shows 
two pixels being transferred in a 16-bit space, concatenated one after 
the other.

In the second case "(21) RAW: 8 Bits (16 Bits/Pixel) Data" the diagram 
shows one pixel being transferred in a 16-bit space, and the upper 8 
bits being set to 0.

Without this fix the 8-bit raw frames I can grab from IMX219 and IMX462 
on V4H are jumbled. I suspect maybe we need to teach the VIN driver to 
support both modes as this have an effect on your setup. How do your 
output images look with this fix?

What I can't figure out is what is different at the VIN stage. All RAW 
8-bit media bus formats sets VnMC.INF to b100 (Embedded 8-bit non Image 
Data or RAW8 input) which would suggest the first case is always used.  
The only case where the second case could be used is for the RAW10 
formats where VnMC.INF is set to b111 (RAW8 (16 bits/pixel) or RAW10 or 
RAW12 or RAW14 or RAW16 or RGB565).

The only source I can think of is that our sensors somehow transmit 
8-bit data differently, but I'm not sure how we can check that theory.  
But if we can then we could use different media bus codes to address 
both scenarios. What do you think?

> 
>  Tomi
> 
> > 
> > Remove the special case from pixel formats that carry image data and
> > treat it as any other image format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 ----------------
> >  1 file changed, 16 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index f8394be8a922..fdf0f86c801f 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -680,22 +680,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  
> >  	fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> >  	stride = vin->format.bytesperline / fmt->bpp;
> > -
> > -	/* For RAW8 format bpp is 1, but the hardware process RAW8
> > -	 * format in 2 pixel unit hence configure VNIS_REG as stride / 2.
> > -	 */
> > -	switch (vin->format.pixelformat) {
> > -	case V4L2_PIX_FMT_SBGGR8:
> > -	case V4L2_PIX_FMT_SGBRG8:
> > -	case V4L2_PIX_FMT_SGRBG8:
> > -	case V4L2_PIX_FMT_SRGGB8:
> > -	case V4L2_PIX_FMT_GREY:
> > -		stride /= 2;
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -
> >  	rvin_write(vin, stride, VNIS_REG);
> >  }
> >  
> 

-- 
Kind Regards,
Niklas Söderlund