Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes

[Jacopo Mondi]

Hi Barnabás

On Mon, Oct 05, 2026 at 10:55:00AM +0200, Barnabás Pőcze wrote:
> 2026. 10. 04. 11:14 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, Oct 01, 2026 at 10:51:30AM +0200, Barnabás Pőcze wrote:
> > > `risp_io_{input,capture}_enum_framesizes()` sets the horizontal and vertical
> > > step size to 2. However, `risp_io_{input,capture}_try_format()` passes 2
> > > to `v4l_bound_align_image()`, which corresponds to a step size of 2^2 = 4.
> > >
> > > So move these constants into macros to avoid the repetition, and with that,
> > > use 4 as the step size everywhere.
> >
> > Why 4 ?
> >
> > I can't find any such limit in documentation.
> >
> > For input I don't find alignment constraints nor in CS or VSPX
> > documentation.
> >
> > For output I only see a requirement that the stride is 256 bits
> > aligned, something that is enforced by risp_io_capture_try_format()
> > already.
> >
> > I see Niklas has reviewed the patch, so he might have found somewhere
> > in the long documentation where the alignment for both input and
> > capture nodes is described.
>
> My primary goal was to remove the inconsistency between the reported and applied
> step sizes. So I went with 4 because that was the applied step size, so with that,

Let's first clarify if the 4 was intentional or is the API of
v4l_bound_align_image() that confused us.

Niklas: any recollection here ?

> it's quite certain that nothing that has worked will break.
>
> Admittedly I do not know if that is the correct number, but I think that can
> be considered a separate issue.
>
>
>
> >
> > >
> > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze+renesas@xxxxxxxxxxxxxxxx>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > ---
> > > changes in v2:
> > >    * fix typo
> > >
> > > v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@xxxxxxxxxxxxxxxx
> > > ---
> > >   .../media/platform/renesas/rcar-isp/core-io.c | 40 +++++++++++--------
> > >   1 file changed, 24 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-isp/core-io.c b/drivers/media/platform/renesas/rcar-isp/core-io.c
> > > index 820af506f896c..b60b91f43d42d 100644
> > > --- a/drivers/media/platform/renesas/rcar-isp/core-io.c
> > > +++ b/drivers/media/platform/renesas/rcar-isp/core-io.c
> > > @@ -15,6 +15,12 @@
> > >
> > >   #include "risp-core.h"
> > >
> > > +#define RISP_MIN_WIDTH 128
> > > +#define RISP_MAX_WIDTH 5120
> > > +#define RISP_MIN_HEIGHT 128
> > > +#define RISP_MAX_HEIGHT 4096
> > > +#define RISP_SIZE_ALIGNMENT 2 /* 2^2 = 4 */
> >
> > The fact that 2 means 2^2 is only because of how
> > v4l_bound_align_image() is implemented
> >
> > > +
> > >   #define risp_io_err(d, fmt, arg...)         dev_err((d)->core->dev, fmt, ##arg)
> > >
> > >   static struct risp_buffer *risp_io_vb2buf(struct vb2_v4l2_buffer *vb)
> > > @@ -330,8 +336,9 @@ static void risp_io_input_try_format(struct rcar_isp_core_io *io,
> > >   {
> > >   	unsigned int bpp = 0;
> > >
> > > -	v4l_bound_align_image(&pix->width, 128, 5120, 2,
> > > -			      &pix->height, 128, 4096, 2, 0);
> > > +	v4l_bound_align_image(&pix->width, RISP_MIN_WIDTH, RISP_MAX_WIDTH, RISP_SIZE_ALIGNMENT,
> > > +			      &pix->height, RISP_MIN_HEIGHT, RISP_MAX_HEIGHT, RISP_SIZE_ALIGNMENT,
> >
> > Should we maybe use 1 here, unless it is documented that 4 is a
> > requirement ?
> >
> > My suspicion is that we originally meant '2' everywhere, but
> > v4l_bound_align_image() is weird and if you pass '2' in it means
> > '2^2' and this went unnoticed.
> >
> > > +			      0);
> > >
> > >   	for (unsigned int i = 0; i < ARRAY_SIZE(risp_io_input_formats); i++) {
> > >   		if (risp_io_input_formats[i].fourcc == pix->pixelformat) {
> > > @@ -423,13 +430,13 @@ static int risp_io_input_enum_framesizes(struct file *file, void *fh,
> > >
> > >   	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > >
> > > -	fsize->stepwise.min_width = 128;
> > > -	fsize->stepwise.max_width = 5120;
> > > -	fsize->stepwise.step_width = 2;
> > > +	fsize->stepwise.min_width = RISP_MIN_WIDTH;
> > > +	fsize->stepwise.max_width = RISP_MAX_WIDTH;
> > > +	fsize->stepwise.step_width = 1u << RISP_SIZE_ALIGNMENT;
> > >
> > > -	fsize->stepwise.min_height = 128;
> > > -	fsize->stepwise.max_height = 4096;
> > > -	fsize->stepwise.step_height = 2;
> > > +	fsize->stepwise.min_height = RISP_MIN_HEIGHT;
> > > +	fsize->stepwise.max_height = RISP_MAX_HEIGHT;
> > > +	fsize->stepwise.step_height = 1u << RISP_SIZE_ALIGNMENT;
> > >
> > >   	return 0;
> > >   }
> > > @@ -720,8 +727,9 @@ static const struct v4l2_pix_format_mplane risp_io_capture_default_format = {
> > >   static void risp_io_capture_try_format(struct rcar_isp_core_io *io,
> > >   				       struct v4l2_pix_format_mplane *pix)
> > >   {
> > > -	v4l_bound_align_image(&pix->width, 128, 5120, 2,
> > > -			      &pix->height, 128, 4096, 2, 0);
> > > +	v4l_bound_align_image(&pix->width, RISP_MIN_WIDTH, RISP_MAX_WIDTH, RISP_SIZE_ALIGNMENT,
> > > +			      &pix->height, RISP_MIN_HEIGHT, RISP_MAX_HEIGHT, RISP_SIZE_ALIGNMENT,
> > > +			      0);
> > >
> > >   	pix->field = V4L2_FIELD_NONE;
> > >   	pix->colorspace = V4L2_COLORSPACE_SRGB;
> > > @@ -824,13 +832,13 @@ static int risp_io_capture_enum_framesizes(struct file *file, void *fh,
> > >
> > >   	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > >
> > > -	fsize->stepwise.min_width = 128;
> > > -	fsize->stepwise.max_width = 5120;
> > > -	fsize->stepwise.step_width = 2;
> > > +	fsize->stepwise.min_width = RISP_MIN_WIDTH;
> > > +	fsize->stepwise.max_width = RISP_MAX_WIDTH;
> > > +	fsize->stepwise.step_width = 1u << RISP_SIZE_ALIGNMENT;
> > >
> > > -	fsize->stepwise.min_height = 128;
> > > -	fsize->stepwise.max_height = 4096;
> > > -	fsize->stepwise.step_height = 2;
> > > +	fsize->stepwise.min_height = RISP_MIN_HEIGHT;
> > > +	fsize->stepwise.max_height = RISP_MAX_HEIGHT;
> > > +	fsize->stepwise.step_height = 1u << RISP_SIZE_ALIGNMENT;
> > >
> > >   	return 0;
> > >   }
> > > --
> > > 2.55.0
> > >
> > >
>