On Fri, Jul 04, 2025 at 06:44:04PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 04, 2025 at 03:18:08AM +0300, Laurent Pinchart wrote:
> > The RPF doesn't enforces the alignment constraint on the sink pad
> > format, which could have an odd size, possibly down to 1x1. In that
> > case, the upper bounds for the left and top coordinates clamping would
> > become negative, cast to a very large positive value. Incorrect crop
> > rectangle coordinates would then be incorrectly accepted.
> >
> > A second issue can occur when the requested left and top coordinates are
> > negative. They are cast to a large unsigned value, clamped to the
> > maximum. While the calculation will produce valid values for the
> > hardware, this is not compliant with the V4L2 specification that
> > requires values to be adjusted to the closest valid value.
> >
> > Fix both issues by switching to signed clamping, with an explicit
> > minimum to adjust negative values, and adjusting the clamp bounds to
> > avoid negative upper bounds.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > .../media/platform/renesas/vsp1/vsp1_rwpf.c | 28 ++++++++++++++++---
> > 1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > index 56464875a6c5..ffc1b3ab54e2 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > @@ -201,6 +201,8 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_selection *sel)
> > {
> > + unsigned int min_width = RWPF_MIN_WIDTH;
> > + unsigned int min_height = RWPF_MIN_HEIGHT;
> > struct vsp1_rwpf *rwpf = to_rwpf(subdev);
> > struct v4l2_subdev_state *state;
> > struct v4l2_mbus_framefmt *format;
> > @@ -229,18 +231,36 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev,
> > format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
> >
> > /*
> > - * Restrict the crop rectangle coordinates to multiples of 2 to avoid
> > - * shifting the color plane.
> > + * For YUV formats, restrict the crop rectangle coordinates to multiples
> > + * of 2 to avoid shifting the color plane.
> > */
> > if (format->code == MEDIA_BUS_FMT_AYUV8_1X32) {
> > sel->r.left = ALIGN(sel->r.left, 2);
> > sel->r.top = ALIGN(sel->r.top, 2);
> > sel->r.width = round_down(sel->r.width, 2);
> > sel->r.height = round_down(sel->r.height, 2);
> > +
> > + /*
> > + * The RPF doesn't enforces the alignment constraint on the sink
> > + * pad format, which could have an odd size, possibly down to
> > + * 1x1. In that case, the minimum width and height would be
> > + * smaller than the sink pad format, leading to a negative upper
> > + * bound in the left and top clamping. Clamp the minimum width
> > + * and height to the format width and height to avoid this.
> > + *
> > + * In such a situation, odd values for the crop rectangle size
> > + * would be accepted when clamping the width and height below.
> > + * While that would create an invalid hardware configuration,
> > + * the video device enforces proper alignment of the pixel
> > + * format, and the mismatch will then result in link validation
> > + * failure. Incorrect operation of the hardware is not possible.
> > + */
> > + min_width = min(ALIGN(min_width, 2), format->width);
> > + min_height = min(ALIGN(min_height, 2), format->height);
>
> I wonder if initializing min_width (and height) to RWPF_MIN_WIDTH and
> then here unconditionally make it 2 (because of ALIGN) wouldn't just
> be better expressed as
>
> min_width = min(2, format->width);
The calculation will lead to the same result. I thought about expressing
it this way, but I think the explicit ALIGN() makes the intent clearer.
Testing both with a gcc 13.3.0-based arm64 cross-compiler, the generated
code is the same.
> > }
> >
> > - sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2);
> > - sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2);
> > + sel->r.left = clamp_t(int, sel->r.left, 0, format->width - min_width);
> > + sel->r.top = clamp_t(int, sel->r.top, 0, format->height - min_height);
>
> Regardless of the above
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> > sel->r.width = min_t(unsigned int, sel->r.width,
> > format->width - sel->r.left);
> > sel->r.height = min_t(unsigned int, sel->r.height,
--
Regards,
Laurent Pinchart
