Re: [PATCH 4/4] media: rkvdec: align bytesperline to 64 bytes in rkvdec_fill_decoded_pixfmt()

[Francesco Saverio Pavone]

Hi,

> The 10-bit capture format is NV15, whose luma stride is width * 5 / 4
> [...] while the VDPU381 needs the luma stride 64-byte aligned (2432).
> [...] so NV15 comes out unaligned and the decoded frame is corrupted
> without this. I can show the before/after on a 10-bit vector if that's
> useful.

The arithmetic is right, the hardware requirement does not reproduce here.
I built the same tree twice, changing only this hunk, and measured on a
Rock 5B+ (RK3588, VDPU381):
test with ALIGN without ALIGN libvpx
1920x1080 VP9 Profile 2, 10-bit 8f40faddd32e 8f40faddd32e 8f40faddd32e
1080x1920 VP9 Profile 2, 10-bit 68400b913caa 68400b913caa 68400b913caa
fluster VP9-TEST-VECTORS 224/305 224/305 -
Both clips are the shapes that reach the ALIGN: 1920 gives an NV15 stride
of 2400 (2400 % 64 = 32) and the portrait clip rounds to a capture width of
1088, stride 1360 (1360 % 64 = 16). md5 is of the decoded frames converted
to I420, same pipeline for hardware and software. The 8-bit control on the
same pipeline is byte-identical too, so the comparison is not hiding a
conversion artefact.
That matches what the registers ask for. The stride goes into the hardware
as bytesperline / 16 (y_hor_virstride, uv_hor_virstride, and the VP9
reference ones), and width * 5 / 4 with a 64-aligned width is always a
multiple of 16, so step_width = 64 already guarantees the alignment the
hardware needs. Rounding to 64 only pads the stride reported to userspace,
for every variant and every coded format, not just the 10-bit case.
If you did see corruption without this, I suspect the other hunk in the
same patch was missing too. Taking the MV and reference pitch from
bytesperline instead of the bitstream width is the part that makes the
register value and the buffer layout agree, and it is the one I would keep.
My A/B has that hunk on both sides, so what I varied is only the ALIGN. If
you have a vector that fails with the pitch fix in place and the ALIGN
removed, I would like to run it.
One small thing on the v2 suggestion: v4l2_fill_pixfmt_mp_aligned() does
not exist in 7.2-rc5. v4l2-common.h exports v4l2_fill_pixfmt() and
v4l2_fill_pixfmt_mp() only, so that would be a new helper to add rather
than a drop-in replacement.

Sav

Il giorno lun 27 lug 2026 alle ore 19:07 Igor Paunovic
<royalnet026@xxxxxxxxx> ha scritto:
>
> Hi
>
> > This shouldn't be needed. As you set step_width to 64,
> > v4l2_apply_frmsize_constraints() should already do the alignment.
> > Do you have more context as to when width is not aligned ?
>
> You're right that the width is aligned -- step_width = 64 handles that.
> This isn't the width though, it's the stride, and it only bites on the
> 10-bit (Profile 2) capture format.
>
> For 8-bit NV12 the luma stride equals the width, so once the width is
> 64-aligned the stride is too and this block is a no-op.
>
> The 10-bit capture format is NV15, whose luma stride is width * 5 / 4
> (bpp {5, 10}, bpp_div {4, 4} in v4l2-common.c). That isn't a multiple
> of 64 even when the width is: a 1920-wide Profile 2 frame gives a
> 2400-byte stride, while the VDPU381 needs the luma stride 64-byte
> aligned (2432). v4l2_apply_frmsize_constraints() only rounds the width
> and height, and v4l2_fill_pixfmt_mp() fills the stride with alignment 1,
> so NV15 comes out unaligned and the decoded frame is corrupted without
> this. I can show the before/after on a 10-bit vector if that's useful.
>
> If you'd rather express it through the framework than with a manual
> ALIGN() here, v4l2_fill_pixfmt_mp_aligned(pixfmt, pixelformat, width,
> height, 64) produces the same bytesperline and sizeimage -- happy to
> switch to that in v2.
>
> Igor
>
>
> On Mon, Jul 27, 2026 at 5:58 PM Detlev Casanova
> <detlev.casanova@xxxxxxxxxxxxx> wrote:
> >
> > Hi !
> >
> > On Sunday, 26 July 2026 16:25:37 EDT Venkata Atchuta Bheemeswara Sarma Darbha
> > wrote:
> > > From: Igor Paunovic <royalnet026@xxxxxxxxx>
> > >
> > > Ensure bytesperline is 64-byte aligned and scale sizeimage accordingly.
> > >
> > > Signed-off-by: Igor Paunovic <royalnet026@xxxxxxxxx>
> > > Signed-off-by: Francesco Saverio Pavone <pavone.lawyer@xxxxxxxxx>
> > > Signed-off-by: Venkata Atchuta Bheemeswara Sarma Darbha
> > > <vdarbha0473@xxxxxxxxx> ---
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec.c index
> > > 436bb460f04e..6ab6530b1cc9 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > @@ -102,12 +102,20 @@ static void rkvdec_fill_decoded_pixfmt(struct
> > > rkvdec_ctx *ctx, struct v4l2_pix_format_mplane *pix_mp)
> > >  {
> > >       const struct rkvdec_variant *variant = ctx->dev->variant;
> > > +     struct v4l2_plane_pix_format *plane = &pix_mp->plane_fmt[0];
> > > +     u32 aligned_bpl;
> > >
> > >       v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, pix_mp->width,
> > > pix_mp->height);
> > >
> > > -     ctx->colmv_offset = pix_mp->plane_fmt[0].sizeimage;
> > > +     /* ensure bpl is 64 byte aligned and scale sizeimage accordingly.
> > */
> > > +     aligned_bpl = ALIGN(plane->bytesperline, 64);
> > > +     if (aligned_bpl != plane->bytesperline) {
> > > +             plane->sizeimage = plane->sizeimage / plane-
> > >bytesperline * aligned_bpl;
> > > +             plane->bytesperline = aligned_bpl;
> > > +     }
> >
> > This shouldn't be needed. As you set step_width to 64,
> > v4l2_apply_frmsize_constraints() should already do the alignment.
> >
> > Do you have more context as to when width is not aligned ?
> >
> > >
> > > -     pix_mp->plane_fmt[0].sizeimage += variant->ops->colmv_size(pix_mp-
> > >width,
> > > pix_mp->height); +    ctx->colmv_offset = plane->sizeimage;
> > > +     plane->sizeimage += variant->ops->colmv_size(pix_mp->width,
> > > pix_mp->height); }
> > >
> > >  static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, struct v4l2_format *f,
> >
> > Regards,
> > Detlev.
> >
> >
> >