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);
> }
>
> - 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
>
>
