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

HI

Thank you for measuring it -- that settles it, and you're right. The
byte-exact A/B on both the landscape and portrait shapes that reach
the ALIGN is conclusive: the hunk makes no difference to the decoded
output.

Tracing it back through the registers after your mail confirms your
explanation. The luma and reference strides are programmed as
bytesperline / 16 (reg018/019 and the VP9 lastf/golden/altref
hor_virstride fields), so the hardware only needs a 16-byte-aligned
stride, and width * 5 / 4 on a 64-aligned width is always a multiple
of 16. step_width = 64 already guarantees what the hardware asks for.
My "corrupted without this" was reasoned from the ALIGN constant
rather than measured against this tree with the pitch fix in place --
that was my mistake.

You're right that the real fix is taking the MV and reference pitch
from bytesperline instead of the bitstream width -- that's what makes
the register value and the buffer layout agree, and it's already in
the "Add VP9 support" patch ("Real buffer stride, not
display-width-derived"). I don't have a vector that fails with that in
place and the ALIGN removed.

So this patch should be dropped in v2; the pitch derivation carries
it. And thanks for the catch on v4l2_fill_pixfmt_mp_aligned() --
you're right it isn't in 7.2-rc5, so that suggestion doesn't apply
either.

Igor


On Mon, Jul 27, 2026 at 7:43 PM Francesco Saverio Pavone
<[email protected]> wrote:
>
> 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
> <[email protected]> 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
> > <[email protected]> wrote:
> > >
> > > Hi !
> > >
> > > On Sunday, 26 July 2026 16:25:37 EDT Venkata Atchuta Bheemeswara Sarma Darbha
> > > wrote:
> > > > From: Igor Paunovic <[email protected]>
> > > >
> > > > Ensure bytesperline is 64-byte aligned and scale sizeimage accordingly.
> > > >
> > > > Signed-off-by: Igor Paunovic <[email protected]>
> > > > Signed-off-by: Francesco Saverio Pavone <[email protected]>
> > > > Signed-off-by: Venkata Atchuta Bheemeswara Sarma Darbha
> > > > <[email protected]> ---
> > > >  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.
> > >
> > >
> > >

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip