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

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

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
> >
> >