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

[Jacopo Mondi]

Hi Tommaso,

On Fri, Jul 31, 2026 at 04:55:57PM +0200, Tommaso Merciai wrote:
> 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: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > > ---
> > > 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.
>

Ah sorry, I completely missed that.

I'll review that one.

> >
> > 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.

Thank you!

>
> >
> > In the meantime for this patch
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20260729105603.1160966-6-tommaso.merciai.xr@xxxxxxxxxxxxxx/
>
> 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
> > >
> > >