* [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®