Hi Laurent
On Wed, Apr 30, 2025 at 02:53:19AM +0300, Laurent Pinchart wrote:
> When reading YUV formats from memory, the hardware requires the crop
> rectangle size and position to be aligned to multiples of two, depending
> on the horizontal and vertical subsampling factors. The driver doesn't
> enforce this, leading to incorrect operation.
>
> As the crop rectangle is implemented on the RPF subdev's sink pad,
> enforcing the constraint conditionally based on the subsampling factors
> is difficult, as those are only known by the RPF video device. We could
> perform the check at pipeline validation time, but that could lead to
> confusing -EPIPE errors. As there is very few use cases for odd crop
> offsets and sizes with non-subsampled YUV, take the easier and more
> user-friendly route of enforcing the constraint on all YUV formats.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> .../media/platform/renesas/vsp1/vsp1_rwpf.c | 41 ++++++++++++-------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> index 83ff2c266038..61f7e13ebeee 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> @@ -117,6 +117,17 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
> RWPF_MIN_WIDTH, rwpf->entity.max_width);
> format->height = clamp_t(unsigned int, fmt->format.height,
> RWPF_MIN_HEIGHT, rwpf->entity.max_height);
> +
> + /*
> + * For YUV formats on the RPF, restrict the size to multiples of 2 to
> + * avoid shifting the color plane.
> + */
> + if (rwpf->entity.type == VSP1_ENTITY_RPF &&
> + format->code == MEDIA_BUS_FMT_AYUV8_1X32) {
> + format->width = ALIGN(format->width, 2);
> + format->height = ALIGN(format->height, 2);
ALIGN aligns up right ? Is it ok or is it better to read 1 pixel less
than reading memory outside of the region the user asked for ?
> + }
> +
> format->field = V4L2_FIELD_NONE;
>
> format->colorspace = fmt->format.colorspace;
> @@ -231,23 +242,23 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev,
> /* Make sure the crop rectangle is entirely contained in the image. */
> 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.
> - */
> - 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);
> - }
> -
> 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.width = min_t(unsigned int, sel->r.width,
> - format->width - sel->r.left);
> - sel->r.height = min_t(unsigned int, sel->r.height,
> - format->height - sel->r.top);
> + sel->r.width = clamp_t(unsigned int, sel->r.width, RWPF_MIN_WIDTH,
> + format->width - sel->r.left);
> + sel->r.height = clamp_t(unsigned int, sel->r.height, RWPF_MIN_HEIGHT,
> + format->height - sel->r.top);
> +
> + /*
> + * 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 = round_down(sel->r.left, 2);
> + sel->r.top = round_down(sel->r.top, 2);
> + sel->r.width = ALIGN(sel->r.width, 2);
> + sel->r.height = ALIGN(sel->r.height, 2);
The existing code did
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);
is it intentional ?
> + }
>
> crop = v4l2_subdev_state_get_crop(state, RWPF_PAD_SINK);
> *crop = sel->r;
> --
> Regards,
>
> Laurent Pinchart
>
>
