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

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