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

[Barnabás Pőcze]

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@xxxxxxxxxxxx>
> ---
>   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?


> +
> +	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?


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