Add v4l2_fill_pixfmt_aligned(), a variant of v4l2_fill_pixfmt()
that accepts a stride_alignment parameter, mirroring the existing
v4l2_fill_pixfmt_mp() / v4l2_fill_pixfmt_mp_aligned() pair.
v4l2_fill_pixfmt() is refactored to call v4l2_fill_pixfmt_aligned()
with stride_alignment=1, preserving its existing behaviour.
The new helper is needed by drivers whose DMA engine requires the
line stride to be a multiple of a specific value, such as the
Renesas RZ/G3E CRU which requires 128-byte alignment.
Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
---
v2->v3:
- No changes, just moved to from PATCH 3/4 to PATCH 2/4
v1->v2:
- Move v4l2_fill_pixfmt() into v4l2-common.h as inline wrapper
- Add v4l2_fill_pixfmt_aligned() helper documentation.
drivers/media/v4l2-core/v4l2-common.c | 12 +++++----
include/media/v4l2-common.h | 38 +++++++++++++++++++++++++--
2 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 54995ba8c20d..2ce4f1c20fbc 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -537,8 +537,8 @@ int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
}
EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp_aligned);
-int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
- u32 width, u32 height)
+int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
+ u32 width, u32 height, u8 stride_alignment)
{
const struct v4l2_format_info *info;
int i;
@@ -554,15 +554,17 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
pixfmt->width = width;
pixfmt->height = height;
pixfmt->pixelformat = pixelformat;
- pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width, 1);
+ pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width,
+ stride_alignment);
pixfmt->sizeimage = 0;
for (i = 0; i < info->comp_planes; i++)
pixfmt->sizeimage +=
- v4l2_format_plane_size(info, i, width, height, 1);
+ v4l2_format_plane_size(info, i, width, height,
+ stride_alignment);
return 0;
}
-EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
+EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_aligned);
#ifdef CONFIG_MEDIA_CONTROLLER
static s64 v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 749fe38c134e..be4dd9762196 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -554,8 +554,42 @@ static inline bool v4l2_is_format_bayer(const struct v4l2_format_info *f)
const struct v4l2_format_info *v4l2_format_info(u32 format);
void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
const struct v4l2_frmsize_stepwise *frmsize);
-int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
- u32 width, u32 height);
+
+/**
+ * v4l2_fill_pixfmt_aligned - Fill in a &struct v4l2_pix_format with stride
+ * alignment requirements.
+ *
+ * @pixfmt: pointer to the &struct v4l2_pix_format to be filled
+ * @pixelformat: the V4L2 pixel format (V4L2_PIX_FMT_*)
+ * @width: image width in pixels
+ * @height: image height in pixels
+ * @stride_alignment: stride alignment in bytes, must be a power of 2
+ *
+ * Fills all fields of @pixfmt for the given pixel format, dimensions, and
+ * stride alignment. Only formats stored in a single memory plane are
+ * supported; returns -EINVAL for multi-memory-plane formats.
+ *
+ * @pixfmt->bytesperline is set to the stride of the primary (plane 0) plane,
+ * rounded up to a multiple of @stride_alignment. For formats that store
+ * multiple component planes in a single memory buffer (e.g. NV12), the
+ * alignment applied to each component plane's stride is scaled relative to
+ * @stride_alignment so that the chroma stride remains consistently derivable
+ * from the luma stride. @pixfmt->bytesperline therefore reflects only the
+ * primary plane stride.
+ *
+ * @pixfmt->sizeimage is set to the total size in bytes of all component planes.
+ *
+ * Return: 0 on success, -EINVAL if @pixelformat is unknown or uses multiple
+ * memory planes.
+ */
+int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
+ u32 width, u32 height, u8 stride_alignment);
+
+static inline int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt,
+ u32 pixelformat, u32 width, u32 height)
+{
+ return v4l2_fill_pixfmt_aligned(pixfmt, pixelformat, width, height, 1);
+}
/* @stride_alignment is a power of 2 value in bytes */
int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
--
2.54.0
Hi Tommaso
On Wed, Jul 08, 2026 at 06:14:03PM +0200, Tommaso Merciai wrote:
> Add v4l2_fill_pixfmt_aligned(), a variant of v4l2_fill_pixfmt()
> that accepts a stride_alignment parameter, mirroring the existing
> v4l2_fill_pixfmt_mp() / v4l2_fill_pixfmt_mp_aligned() pair.
>
> v4l2_fill_pixfmt() is refactored to call v4l2_fill_pixfmt_aligned()
> with stride_alignment=1, preserving its existing behaviour.
>
> The new helper is needed by drivers whose DMA engine requires the
> line stride to be a multiple of a specific value, such as the
> Renesas RZ/G3E CRU which requires 128-byte alignment.
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> ---
> v2->v3:
> - No changes, just moved to from PATCH 3/4 to PATCH 2/4
>
> v1->v2:
> - Move v4l2_fill_pixfmt() into v4l2-common.h as inline wrapper
> - Add v4l2_fill_pixfmt_aligned() helper documentation.
>
> drivers/media/v4l2-core/v4l2-common.c | 12 +++++----
> include/media/v4l2-common.h | 38 +++++++++++++++++++++++++--
> 2 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 54995ba8c20d..2ce4f1c20fbc 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -537,8 +537,8 @@ int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
> }
> EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp_aligned);
>
> -int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> - u32 width, u32 height)
> +int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> + u32 width, u32 height, u8 stride_alignment)
> {
> const struct v4l2_format_info *info;
> int i;
> @@ -554,15 +554,17 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> pixfmt->width = width;
> pixfmt->height = height;
> pixfmt->pixelformat = pixelformat;
> - pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width, 1);
> + pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width,
> + stride_alignment);
> pixfmt->sizeimage = 0;
>
> for (i = 0; i < info->comp_planes; i++)
> pixfmt->sizeimage +=
> - v4l2_format_plane_size(info, i, width, height, 1);
> + v4l2_format_plane_size(info, i, width, height,
> + stride_alignment);
> return 0;
> }
> -EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_aligned);
>
> #ifdef CONFIG_MEDIA_CONTROLLER
> static s64 v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 749fe38c134e..be4dd9762196 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -554,8 +554,42 @@ static inline bool v4l2_is_format_bayer(const struct v4l2_format_info *f)
> const struct v4l2_format_info *v4l2_format_info(u32 format);
> void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
> const struct v4l2_frmsize_stepwise *frmsize);
> -int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> - u32 width, u32 height);
> +
> +/**
> + * v4l2_fill_pixfmt_aligned - Fill in a &struct v4l2_pix_format with stride
> + * alignment requirements.
nit:
I was about to suggest "No '.' at the end of the function's brief to
match the existing style" but I see the devm_v4l2_sensor_clk_get_legacy
has it. However the majority of the other functions don't, so maybe
consider dropping it.
> + *
> + * @pixfmt: pointer to the &struct v4l2_pix_format to be filled
> + * @pixelformat: the V4L2 pixel format (V4L2_PIX_FMT_*)
> + * @width: image width in pixels
> + * @height: image height in pixels
> + * @stride_alignment: stride alignment in bytes, must be a power of 2
> + *
> + * Fills all fields of @pixfmt for the given pixel format, dimensions, and
> + * stride alignment. Only formats stored in a single memory plane are
> + * supported; returns -EINVAL for multi-memory-plane formats.
> + *
> + * @pixfmt->bytesperline is set to the stride of the primary (plane 0) plane,
> + * rounded up to a multiple of @stride_alignment. For formats that store
> + * multiple component planes in a single memory buffer (e.g. NV12), the
> + * alignment applied to each component plane's stride is scaled relative to
> + * @stride_alignment so that the chroma stride remains consistently derivable
Does this rather mean that
"For formats that store multiple component planes in a single memory
buffer (e.g. NV12), the alignment applied to each component plane is
the first plane @stride_alignment scaled by the plane's sub-sampling
ratio" or have I mis-read this ?
> + * from the luma stride. @pixfmt->bytesperline therefore reflects only the
> + * primary plane stride.
> + *
> + * @pixfmt->sizeimage is set to the total size in bytes of all component planes.
maybe s/component // ?
> + *
> + * Return: 0 on success, -EINVAL if @pixelformat is unknown or uses multiple
> + * memory planes.
Do you need this tab ?
> + */
> +int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> + u32 width, u32 height, u8 stride_alignment);
> +
> +static inline int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt,
> + u32 pixelformat, u32 width, u32 height)
> +{
> + return v4l2_fill_pixfmt_aligned(pixfmt, pixelformat, width, height, 1);
> +}
All minors or nit-picking
Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Thanks
j
>
> /* @stride_alignment is a power of 2 value in bytes */
> int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
> --
> 2.54.0
>
>
Hi Jacopo, On 7/9/26 11:35 AM, Jacopo Mondi wrote: > Hi Tommaso > > On Wed, Jul 08, 2026 at 06:14:03PM +0200, Tommaso Merciai wrote: > >> + * >> + * @pixfmt: pointer to the &struct v4l2_pix_format to be filled >> + * @pixelformat: the V4L2 pixel format (V4L2_PIX_FMT_*) >> + * @width: image width in pixels >> + * @height: image height in pixels >> + * @stride_alignment: stride alignment in bytes, must be a power of 2 >> + * >> + * Fills all fields of @pixfmt for the given pixel format, dimensions, and >> + * stride alignment. Only formats stored in a single memory plane are >> + * supported; returns -EINVAL for multi-memory-plane formats. >> + * >> + * @pixfmt->bytesperline is set to the stride of the primary (plane 0) plane, >> + * rounded up to a multiple of @stride_alignment. For formats that store >> + * multiple component planes in a single memory buffer (e.g. NV12), the >> + * alignment applied to each component plane's stride is scaled relative to >> + * @stride_alignment so that the chroma stride remains consistently derivable > Does this rather mean that > > "For formats that store multiple component planes in a single memory > buffer (e.g. NV12), the alignment applied to each component plane is > the first plane @stride_alignment scaled by the plane's sub-sampling > ratio" or have I mis-read this ? No, for the example of NV12, no stride will get scaled (although the sub-sampling of 4:2:0, resulting in a vdiv and hdiv of 2). 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 intention is that for non-mp (not ending with M) formats we might do the scaling (e.g. YUV420 will have it's Y component stride alignment scaled to not break the u and v stride alignments, but YUV420M not) Sincerely Sven
Hi Sven On Fri, Jul 10, 2026 at 10:36:53AM +0200, Sven Püschel wrote: > Hi Jacopo, > > On 7/9/26 11:35 AM, Jacopo Mondi wrote: > > Hi Tommaso > > > > On Wed, Jul 08, 2026 at 06:14:03PM +0200, Tommaso Merciai wrote: > > > > > + * > > > + * @pixfmt: pointer to the &struct v4l2_pix_format to be filled > > > + * @pixelformat: the V4L2 pixel format (V4L2_PIX_FMT_*) > > > + * @width: image width in pixels > > > + * @height: image height in pixels > > > + * @stride_alignment: stride alignment in bytes, must be a power of 2 > > > + * > > > + * Fills all fields of @pixfmt for the given pixel format, dimensions, and > > > + * stride alignment. Only formats stored in a single memory plane are > > > + * supported; returns -EINVAL for multi-memory-plane formats. > > > + * > > > + * @pixfmt->bytesperline is set to the stride of the primary (plane 0) plane, > > > + * rounded up to a multiple of @stride_alignment. For formats that store > > > + * multiple component planes in a single memory buffer (e.g. NV12), the > > > + * alignment applied to each component plane's stride is scaled relative to > > > + * @stride_alignment so that the chroma stride remains consistently derivable > > Does this rather mean that > > > > "For formats that store multiple component planes in a single memory > > buffer (e.g. NV12), the alignment applied to each component plane is > > the first plane @stride_alignment scaled by the plane's sub-sampling > > ratio" or have I mis-read this ? > > No, for the example of NV12, no stride will get scaled (although the > sub-sampling of 4:2:0, resulting in a vdiv and hdiv of 2). Ah, I had looked at v4l2_format_plane_stride() for the NV12 case where byte_alignment gets adjusted for the second plane as: byte_alignment *= DIV_ROUND_UP(info->bpp[1], info->hdiv * info->bpp[0]); which for NV12 resolves at *= 1 and I got confused Indeed this is not just "the first plane @stride_alignment scaled by the plane's sub-sampling ratio" I proposed > > 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 ? > intention is that for non-mp (not ending with M) formats we might do the > scaling (e.g. YUV420 will have it's Y component stride alignment scaled to > not break the u and v stride alignments, but YUV420M not) > > Sincerely > Sven > >
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) Maybe smth. like YUV420 but not YUV420M is a better example (could also be NV24 but not NV24M)? Sincerely Sven
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 > Maybe smth. like YUV420 but not YUV420M is a better example (could also be > NV24 but not NV24M)? > > Sincerely > Sven >
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. Sincerely Sven
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.
Kind Regards,
Tommaso
>
> Sincerely
> Sven
>
>
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
Hi Jacopo,
Thanks for your review.
On Thu, Jul 09, 2026 at 11:35:58AM +0200, Jacopo Mondi wrote:
> Hi Tommaso
>
> On Wed, Jul 08, 2026 at 06:14:03PM +0200, Tommaso Merciai wrote:
> > Add v4l2_fill_pixfmt_aligned(), a variant of v4l2_fill_pixfmt()
> > that accepts a stride_alignment parameter, mirroring the existing
> > v4l2_fill_pixfmt_mp() / v4l2_fill_pixfmt_mp_aligned() pair.
> >
> > v4l2_fill_pixfmt() is refactored to call v4l2_fill_pixfmt_aligned()
> > with stride_alignment=1, preserving its existing behaviour.
> >
> > The new helper is needed by drivers whose DMA engine requires the
> > line stride to be a multiple of a specific value, such as the
> > Renesas RZ/G3E CRU which requires 128-byte alignment.
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > ---
> > v2->v3:
> > - No changes, just moved to from PATCH 3/4 to PATCH 2/4
> >
> > v1->v2:
> > - Move v4l2_fill_pixfmt() into v4l2-common.h as inline wrapper
> > - Add v4l2_fill_pixfmt_aligned() helper documentation.
> >
> > drivers/media/v4l2-core/v4l2-common.c | 12 +++++----
> > include/media/v4l2-common.h | 38 +++++++++++++++++++++++++--
> > 2 files changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 54995ba8c20d..2ce4f1c20fbc 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -537,8 +537,8 @@ int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
> > }
> > EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_mp_aligned);
> >
> > -int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> > - u32 width, u32 height)
> > +int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> > + u32 width, u32 height, u8 stride_alignment)
> > {
> > const struct v4l2_format_info *info;
> > int i;
> > @@ -554,15 +554,17 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> > pixfmt->width = width;
> > pixfmt->height = height;
> > pixfmt->pixelformat = pixelformat;
> > - pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width, 1);
> > + pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width,
> > + stride_alignment);
> > pixfmt->sizeimage = 0;
> >
> > for (i = 0; i < info->comp_planes; i++)
> > pixfmt->sizeimage +=
> > - v4l2_format_plane_size(info, i, width, height, 1);
> > + v4l2_format_plane_size(info, i, width, height,
> > + stride_alignment);
> > return 0;
> > }
> > -EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> > +EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt_aligned);
> >
> > #ifdef CONFIG_MEDIA_CONTROLLER
> > static s64 v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 749fe38c134e..be4dd9762196 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -554,8 +554,42 @@ static inline bool v4l2_is_format_bayer(const struct v4l2_format_info *f)
> > const struct v4l2_format_info *v4l2_format_info(u32 format);
> > void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
> > const struct v4l2_frmsize_stepwise *frmsize);
> > -int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> > - u32 width, u32 height);
> > +
> > +/**
> > + * v4l2_fill_pixfmt_aligned - Fill in a &struct v4l2_pix_format with stride
> > + * alignment requirements.
>
> nit:
> I was about to suggest "No '.' at the end of the function's brief to
> match the existing style" but I see the devm_v4l2_sensor_clk_get_legacy
> has it. However the majority of the other functions don't, so maybe
> consider dropping it.
Ok, I will drop it in v4.
>
> > + *
> > + * @pixfmt: pointer to the &struct v4l2_pix_format to be filled
> > + * @pixelformat: the V4L2 pixel format (V4L2_PIX_FMT_*)
> > + * @width: image width in pixels
> > + * @height: image height in pixels
> > + * @stride_alignment: stride alignment in bytes, must be a power of 2
> > + *
> > + * Fills all fields of @pixfmt for the given pixel format, dimensions, and
> > + * stride alignment. Only formats stored in a single memory plane are
> > + * supported; returns -EINVAL for multi-memory-plane formats.
> > + *
> > + * @pixfmt->bytesperline is set to the stride of the primary (plane 0) plane,
> > + * rounded up to a multiple of @stride_alignment. For formats that store
> > + * multiple component planes in a single memory buffer (e.g. NV12), the
> > + * alignment applied to each component plane's stride is scaled relative to
> > + * @stride_alignment so that the chroma stride remains consistently derivable
>
> Does this rather mean that
>
> "For formats that store multiple component planes in a single memory
> buffer (e.g. NV12), the alignment applied to each component plane is
> the first plane @stride_alignment scaled by the plane's sub-sampling
> ratio" or have I mis-read this ?
Yes thanks.
I will update this part with you suggestion.
>
> > + * from the luma stride. @pixfmt->bytesperline therefore reflects only the
> > + * primary plane stride.
> > + *
> > + * @pixfmt->sizeimage is set to the total size in bytes of all component planes.
>
> maybe s/component // ?
Ok, will drop this in v4.
>
> > + *
> > + * Return: 0 on success, -EINVAL if @pixelformat is unknown or uses multiple
> > + * memory planes.
>
> Do you need this tab ?
Will drop this in v4.
>
> > + */
> > +int v4l2_fill_pixfmt_aligned(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> > + u32 width, u32 height, u8 stride_alignment);
> > +
> > +static inline int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt,
> > + u32 pixelformat, u32 width, u32 height)
> > +{
> > + return v4l2_fill_pixfmt_aligned(pixfmt, pixelformat, width, height, 1);
> > +}
>
> All minors or nit-picking
> Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
Kind Regards,
Tommaso
>
> Thanks
> j
>
> >
> > /* @stride_alignment is a power of 2 value in bytes */
> > int v4l2_fill_pixfmt_mp_aligned(struct v4l2_pix_format_mplane *pixfmt,
> > --
> > 2.54.0
> >
> >
© 2016 - 2026 Red Hat, Inc.