Re: [PATCH 2/4] media: rcar-isp: ispcore: Update image data flush settings

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Barnabás,

Thanks for your feedback.

On 2026-10-20 12:19:54 +0200, Barnabás Pőcze wrote:
> Hi
> 
> 2026. 10. 19. 21:35 keltezéssel, Niklas Söderlund írta:
> > Later versions of the datasheet have an updated chapter on how the image
> > out flush settings depends on proper stride settings. Update the bytes
> > per line calculation and flush out settings to match the datasheet
> > clarification.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >   drivers/media/platform/renesas/rcar-isp/core-io.c | 28 ++++++++++++++++++++---
> >   drivers/media/platform/renesas/rcar-isp/core.c    |  2 +-
> >   2 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-isp/core-io.c b/drivers/media/platform/renesas/rcar-isp/core-io.c
> > index badde72352c1..48db37bad69c 100644
> > --- a/drivers/media/platform/renesas/rcar-isp/core-io.c
> > +++ b/drivers/media/platform/renesas/rcar-isp/core-io.c
> > @@ -733,6 +733,28 @@ static const struct v4l2_pix_format_mplane risp_io_capture_default_format = {
> >   	},
> >   };
> > +static unsigned int risp_io_calture_bytesperline(unsigned int width, unsigned int bpp)
> 
>   calture -> capture
> 
> 
> > +{
> > +	/*
> > +	 * The datasheet specifies strict alignment and padding rules to be able
> > +	 * to flush out all the pixels. Each line must be 256 byte aligned and
> > +	 * at the end of the frame at least one extra line of space must be
> > +	 * available.
> > +	 *
> > +	 * As we can't extend the buffer with one extra line we need to work
> > +	 * with stride setting to satisfy both constraints. Fortunately the
> > +	 * datasheet gives us a calculation for this.
> > +	 *
> > +	 *   FLUSH[pixels] = ALIGN(WIDTH[pixels], 256)
> > +	 *
> > +	 *   PADDING[bytes] = ALIGN(BPP[bytes] * WIDTH[pixels], 256) - BPP[bytes] * WIDTH[pixels] + BPP[bytes] * FLUSH[pixels]
> > +	 */
> > +	unsigned int data = bpp * width;
> > +	unsigned int padding = ALIGN(data, 256) - data + bpp * ALIGN(width, 256);
> 
> So this adds one line's worth of extra padding on top of what is needed to achieve 256-byte alignment
> of the actual data? With this change, for an output size of 2304x1296, I see
> 
>   ioctl(18</dev/video3>, VIDIOC_TRY_FMT, {
>     type=V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, fmt.pix_mp={
>       width=2304, height=1296,
>       pixelformat=v4l2_fourcc('X', 'R', '2', '4') /* V4L2_PIX_FMT_XBGR32 */,
>       field=V4L2_FIELD_NONE, colorspace=V4L2_COLORSPACE_DEFAULT,
>       plane_fmt=[], num_planes=0
>     }
>   } => {
>    fmt.pix_mp={
>      width=2304, height=1296,
>      pixelformat=v4l2_fourcc('X', 'R', '2', '4') /* V4L2_PIX_FMT_XBGR32 */,
>      field=V4L2_FIELD_NONE,
>      colorspace=V4L2_COLORSPACE_SRGB,
>      plane_fmt=[
>        { sizeimage=23887872, bytesperline=18432 }
>      ],
>      num_planes=1
>    }
>   }) = 0 <0.000074>
> 
> which means that the stride is `(2304 * 4) * 2 = 18432`, which is effectively
> twice what would be required for just the 256 byte alignment. So this
> in practice doubles the size of every buffer, or am I missing something?

No that is correct, check chapter 54.5.11 Notes of additional memory 
area of flush out (Image Output DMA).

I'm quiet sure we could be more clever add add the flush out after the 
image by sizeimage = bpp * (hight + 2) or something like that. I opted 
for the big hammer here as this large padding space is needed to support 
NV12 due to how the DMA-engine operates.

In future we could look at using this more clever way for NV16 and RGB 
which do not need the large padding are to operate properly.

> 
> 
> > +
> > +	return data + padding;
> > +}
> > +
> >   static void risp_io_capture_try_format(struct rcar_isp_core_io *io,
> >   				       struct v4l2_pix_format_mplane *pix)
> >   {
> > @@ -749,16 +771,16 @@ static void risp_io_capture_try_format(struct rcar_isp_core_io *io,
> >   	case V4L2_PIX_FMT_NV16M:
> >   		pix->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> >   		pix->num_planes = 2;
> > -		pix->plane_fmt[0].bytesperline = ALIGN(pix->width, 256);
> > +		pix->plane_fmt[0].bytesperline = risp_io_calture_bytesperline(pix->width, 1);
> >   		pix->plane_fmt[0].sizeimage = pix->plane_fmt[0].bytesperline * pix->height;
> > -		pix->plane_fmt[1].bytesperline = ALIGN(pix->width, 256);
> > +		pix->plane_fmt[1].bytesperline = pix->plane_fmt[0].bytesperline;
> >   		pix->plane_fmt[1].sizeimage = pix->plane_fmt[1].bytesperline * pix->height;
> >   		break;
> >   	default:
> >   		pix->pixelformat = V4L2_PIX_FMT_XBGR32;
> >   		pix->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >   		pix->num_planes = 1;
> > -		pix->plane_fmt[0].bytesperline = ALIGN(pix->width * 4, 256);
> > +		pix->plane_fmt[0].bytesperline = risp_io_calture_bytesperline(pix->width, 4);
> >   		pix->plane_fmt[0].sizeimage = pix->plane_fmt[0].bytesperline * pix->height;
> >   		break;
> >   	}
> > diff --git a/drivers/media/platform/renesas/rcar-isp/core.c b/drivers/media/platform/renesas/rcar-isp/core.c
> > index b5861d0cd0e8..3e135e84b75b 100644
> > --- a/drivers/media/platform/renesas/rcar-isp/core.c
> > +++ b/drivers/media/platform/renesas/rcar-isp/core.c
> > @@ -398,7 +398,7 @@ static int risp_core_config_output(struct rcar_isp_core *core,
> >   	/* Set image out flush EOF. */
> >   	risp_core_write(core, ISPCORE_DMA_IMAGE_FLUSH_OUT_REG(index),
> > -			pix->plane_fmt[0].bytesperline <<
> > +			ALIGN(pix->width, 256) <<
> 
> I'm probably mistaken, but does this not need a multiplication by bpp?
> Or was the previous version actually incorrect?

The whole DMA Flush operation was a tad missunderstood but is better 
documented now. The unit here is pixels not bytes. This register covers 
all component outputs and each component may have a different bpp.

> 
> 
> >   			ISPCORE_DMA_IMAGE_FLUSH_OUT_PADDING_PIXEL_EOF_SHIFT);
> >   	/* Enable DMA and set burst length. */
> > 
> 

-- 
Kind Regards,
Niklas Söderlund