Re: [PATCH v3 2/4] media: v4l2-common: Add v4l2_fill_pixfmt_aligned() helper

[Sven Püschel]

Hi Tommaso,

On 7/10/26 3:45 PM, Tommaso Merciai wrote:
> Hi Jacopo, Sven,
> Thanks for your comments:
>
> On Fri, Jul 10, 2026 at 02:26:23PM +0200, Sven Püschel wrote:
> > Hi Jacopo,
> >
> > On 7/10/26 2:15 PM, Jacopo Mondi wrote:
> > > Hi Sven
> > >
> > > On Fri, Jul 10, 2026 at 01:54:06PM +0200, Sven Püschel wrote:
> > > > Hi Jacopo,
> > > >
> > > > On 7/10/26 11:38 AM, Jacopo Mondi wrote:
> > > > > > This is due to the fact, that while we have a hdiv of 2 we also interleave
> > > > > > the cb and cr parts in a single plane, which results in the stride being the
> > > > > > same number of bytes as for the y plane (and vdiv isn't relevant for the
> > > > > > stride).
> > > > > >
> > > > > > Therefore the stride scaling also respects the bits per plane (bpp) value to
> > > > > > determine the scaling.
> > > > > >
> > > > > > @Tommaso : While the sentence looks ok, the NV12 example is misguided. The
> > > > > I guess the usage of NV12 was as example of a "formats that store
> > > > > multiple component planes in a single memory"
> > > > >
> > > > > NV24/42 works the same, but being 444 it needs the chroma plane stride to
> > > > > be a multiple of the fist plane stride and might prove as a better
> > > > > example ?
> > > > My potential concern is that NV as an example misguides the reader into one
> > > > of the following:
> > > >
> > > > - It's only for formats which interleave cb/cr into one plane (whereas
> > > > YUV420 also gets scaled)
> > > > - NV24 in the example being though of including the NV24M variant (whereas
> > > > latter won't be affected)
> > > M variants are not supported by the single-planar APIs
> > > https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html
> > >
> > > Some planar formats allow planes to be placed in independent memory
> > > locations. They are identified by an ‘M’ suffix in their name (such as
> > > in V4L2_PIX_FMT_NV12M). Those formats are intended to be used only in
> > > drivers and applications that support the multi-planar API,
> > >
> > > And here we're dealing with single-planar API only if I'm not mistaken
> > Oh, sorry. Assumed that the added description of both functions would be
> > similar/identical, which isn't the case.
> >
> > Given this, I'm fine with the wording and agree to just change the example
> > to smth. else than NV12.
> So if I'm not wrong we can then use YUV420 instead of NV12.
> NV12 is an unlucky example where the alignment scaling factor is 1,
> whereas YUV420 has a scaling factor of 2:
>
> # NV12
> (Y - luma)      bpp[0] = 1
> (CbCr - chroma) bpp[1] = 2
>                  hdiv = 2
>
> # YUV420
> (Y - luma)    bpp[0] = 1
> (Cb - chroma) bpp[1] = 1
>                hdiv = 2
>
> For plane = 0 (single memory-plane formats):
>
> factor = DIV_ROUND_UP(hdiv * bpp[0], bpp[1])
>
> 	NV12:    factor = DIV_ROUND_UP(2 * 1, 2) = 1
> 	YUV420:  factor = DIV_ROUND_UP(2 * 1, 1) = 2
>
> Then I will leave the wording as is and changing only parenthesis part
> like Sven suggested:
>
> 	(e.g NV12) --> (e.g. YUV420)
>
> Please correct me if I'm wrong.
> If for you is ok I will fix this in v4.

lgtm

Sincerely
    Sven