Re: [PATCH v5 1/5] media: rzg2l-cru: Align bytesperline to hardware DMA stride requirement

Hi Jacopo,
Thanks for your review.

On Thu, Jul 30, 2026 at 09:50:35PM +0200, Jacopo Mondi wrote:
> Hi Tommaso
> 
> On Wed, Jul 29, 2026 at 12:55:46PM +0200, Tommaso Merciai wrote:
> > The RZ/G3E CRU programs the line stride via the AMnIS register, whose
> > IS field encodes the value in units of 128 bytes. If bytesperline is
> > not a multiple of 128, the division truncates and the hardware uses a
> > wrong stride, causing horizontal banding.
> >
> > Commit ace92ccef0c9 ("media: platform: rzg2l-cru: Use v4l2_fill_pixfmt()")
> > replaced the open-coded aligned calculation with v4l2_fill_pixfmt(),
> > which sets no alignment, reintroducing the issue.
> >
> > Round bytesperline up to RZG2L_CRU_STRIDE_ALIGN and recompute
> > sizeimage when info->has_stride is set. RZ/G2L has no AMnIS register
> > and keeps the values from v4l2_fill_pixfmt() unchanged.
> >
> > Fixes: ace92ccef0c9 ("media: platform: rzg2l-cru: Use v4l2_fill_pixfmt()")
> > Cc: [email protected]
> > Signed-off-by: Tommaso Merciai <[email protected]>
> > ---
> > v4->v5:
> >  - New patch.
> >
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 5185a547461d..91eda5034248 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -851,6 +851,11 @@ static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
> >
> >  	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
> >
> > +	if (info->has_stride) {
> > +		pix->bytesperline = ALIGN(pix->bytesperline, RZG2L_CRU_STRIDE_ALIGN);
> > +		pix->sizeimage = pix->bytesperline * pix->height;
> > +	}
> > +
> 
> Ack! I think this does what is expected for the single planar formats
> the CRU supports!
> 
> I think after this cycle you could send a patch to replace the above
> back-portable change with the usage of v4l2_fill_pixfmt_aligned().

Ack but this is already done on PATCH 5/5 of this series ([1])
Please correct me if I'm wrong.

> 
> When you do so, could you consider, replacing "has_stride" with a per
> struct rzg2l_cru_info .stride field ?
> 
> I think it would look nicer as you could here unconditionally align
> (assuming the new .stride will be set to 1 for G2L which currently has
> .has_stride = 0).
> 
> Also, RZG2L_CRU_STRIDE_ALIGN is a bit of a mis-nomer, as only G3E and
> V2H has a AMnIS register and alignments constraints as far as I see.
> 
> The only other user of 'has_stride' I see is:
> 
>  rzg2l_cru_initialize_axi()
> 
> 	if (info->has_stride) {
> 		u32 stride = cru->format.bytesperline;
> 		u32 amnis;
> 
> 		stride /= RZG2L_CRU_STRIDE_ALIGN;
> 		amnis = rzg2l_cru_read(cru, AMnIS) & ~AMnIS_IS_MASK;
> 		rzg2l_cru_write(cru, AMnIS, amnis | AMnIS_IS(stride));
> 	}
> 
> the G2L has no register AMnIS, so we can't write it unconditionally
> but rather replace the check with:
> 
>         if (info->stride > 1)
> 
> As a bonus, if any other variant will get a different stride, it will
> be automatically supported.
> 
> What do you think ?

Fully agree!
I will post patches for this after this cycle, thanks.

> 
> In the meantime for this patch
> Reviewed-by: Jacopo Mondi <[email protected]>

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/

Kind Regards,
Tommaso

> 
> Thanks
>   j
> 
> 
> >  	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
> >  		pix->width, pix->height, pix->bytesperline, pix->sizeimage);
> >  }
> > --
> > 2.54.0
> >
> >