* [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes
@ 2026-10-01 8:51 Barnabás Pőcze
2026-10-01 10:34 ` Sakari Ailus
2026-10-04 9:14 ` Jacopo Mondi
0 siblings, 2 replies; 6+ messages in thread
From: Barnabás Pőcze @ 2026-10-01 8:51 UTC (permalink / raw)
To: Niklas Söderlund, Mauro Carvalho Chehab, Geert Uytterhoeven,
Magnus Damm
Cc: Jacopo Mondi, Sakari Ailus, linux-media, linux-renesas-soc,
linux-kernel, Barnabás Pőcze
`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.
Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE")
Signed-off-by: Barnabás Pőcze <barnabas.pocze+renesas@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
changes in v2:
* fix typo
v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@ideasonboard.com
---
.../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 */
+
#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,
+ 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes 2026-10-01 8:51 [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes Barnabás Pőcze @ 2026-10-01 10:34 ` Sakari Ailus 2026-10-04 9:14 ` Jacopo Mondi 1 sibling, 0 replies; 6+ messages in thread From: Sakari Ailus @ 2026-10-01 10:34 UTC (permalink / raw) To: Barnabás Pőcze Cc: Niklas Söderlund, Mauro Carvalho Chehab, Geert Uytterhoeven, Magnus Damm, Jacopo Mondi, linux-media, linux-renesas-soc, linux-kernel 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. > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE") > Signed-off-by: Barnabás Pőcze <barnabas.pocze+renesas@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> I already have v1 in my tree and I don't think there's a need to fix that tiny typo just for the typo itself. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes 2026-10-01 8:51 [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes Barnabás Pőcze 2026-10-01 10:34 ` Sakari Ailus @ 2026-10-04 9:14 ` Jacopo Mondi 2026-10-05 8:55 ` Barnabás Pőcze 1 sibling, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2026-10-04 9:14 UTC (permalink / raw) To: Barnabás Pőcze Cc: Niklas Söderlund, Mauro Carvalho Chehab, Geert Uytterhoeven, Magnus Damm, Jacopo Mondi, Sakari Ailus, linux-media, linux-renesas-soc, linux-kernel 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. > > Fixes: 2151350f60d1 ("media: rcar-isp: Add support for ISPCORE") > Signed-off-by: Barnabás Pőcze <barnabas.pocze+renesas@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > changes in v2: > * fix typo > > v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@ideasonboard.com > --- > .../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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes 2026-10-04 9:14 ` Jacopo Mondi @ 2026-10-05 8:55 ` Barnabás Pőcze 2026-10-05 9:24 ` Jacopo Mondi 0 siblings, 1 reply; 6+ messages in thread From: Barnabás Pőcze @ 2026-10-05 8:55 UTC (permalink / raw) To: Jacopo Mondi Cc: Niklas Söderlund, Mauro Carvalho Chehab, Geert Uytterhoeven, Magnus Damm, Jacopo Mondi, Sakari Ailus, linux-media, linux-renesas-soc, linux-kernel 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, 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@ideasonboard.com> >> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> --- >> changes in v2: >> * fix typo >> >> v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@ideasonboard.com >> --- >> .../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 >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes 2026-10-05 8:55 ` Barnabás Pőcze @ 2026-10-05 9:24 ` Jacopo Mondi 2026-10-05 16:08 ` Niklas Söderlund 0 siblings, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2026-10-05 9:24 UTC (permalink / raw) To: Barnabás Pőcze Cc: Jacopo Mondi, Niklas Söderlund, Mauro Carvalho Chehab, Geert Uytterhoeven, Magnus Damm, Jacopo Mondi, Sakari Ailus, linux-media, linux-renesas-soc, linux-kernel 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@ideasonboard.com> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > changes in v2: > > > * fix typo > > > > > > v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@ideasonboard.com > > > --- > > > .../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 > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes 2026-10-05 9:24 ` Jacopo Mondi @ 2026-10-05 16:08 ` Niklas Söderlund 0 siblings, 0 replies; 6+ messages in thread From: Niklas Söderlund @ 2026-10-05 16:08 UTC (permalink / raw) To: Jacopo Mondi Cc: Barnabás Pőcze, Mauro Carvalho Chehab, Geert Uytterhoeven, Magnus Damm, Jacopo Mondi, Sakari Ailus, linux-media, linux-renesas-soc, linux-kernel On 2026-10-05 11:24:14 +0200, Jacopo Mondi wrote: > 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 ? I think this was picked/discovered early when we where debuggin the issues in the DMA from the VSPX and never re-evaluated. I acked the packed as it do the correct thing, expose the limit we enforce in TRY_FORMAT to the enumeration of frame sizes which was inconsistent. > > > 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@ideasonboard.com> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > changes in v2: > > > > * fix typo > > > > > > > > v1: https://lore.kernel.org/linux-media/20260929133020.342677-2-barnabas.pocze+renesas@ideasonboard.com > > > > --- > > > > .../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 > > > > > > > > > > -- Kind Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-05 16:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-10-01 8:51 [PATCH v2] media: rcar-isp: ispcore: Fix inconsistent step sizes Barnabás Pőcze 2026-10-01 10:34 ` Sakari Ailus 2026-10-04 9:14 ` Jacopo Mondi 2026-10-05 8:55 ` Barnabás Pőcze 2026-10-05 9:24 ` Jacopo Mondi 2026-10-05 16:08 ` Niklas Söderlund
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®