Re: [PATCH 4/7] media: renesas: vsp1: Fix RPF sink alignment for YUV formats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Wed, May 28, 2025 at 04:49:50PM +0200, Jacopo Mondi wrote:
> 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@ideasonboard.com>
> > ---
> >  .../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 ?

This shouldn't actually matter. Link validation will ensure that the
format on the RPF subdev sink pad matches the format on the connected
video device. If this function rounds the value in a way that does not
match the format on the video device, link validation will fail.

I've checked the implementation on the video device, and the driver does

	/* Align the width and height for YUV 4:2:2 and 4:2:0 formats. */
	width = round_down(width, info->hsub);
	height = round_down(height, info->vsub);

	/* Clamp the width and height. */
	pix->width = clamp(width, info->hsub, VSP1_VIDEO_MAX_WIDTH);
	pix->height = clamp(height, info->vsub, VSP1_VIDEO_MAX_HEIGHT);

It seems I can drop the above code, as the video device correctly
handles the hardware requirement for the input size.

> > +	}
> > +
> >  	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 ?

If width == left + 1 (which the clamping code allows), rounding left up
and width down will cause width to be 0. Going in the other direction
ensures we won't have an empty crop rectangle. But the width can then
exceed the sink format width, if I drop the rounding there. I'll rework
this patch.

> > +	}
> >
> >  	crop = v4l2_subdev_state_get_crop(state, RWPF_PAD_SINK);
> >  	*crop = sel->r;

-- 
Regards,

Laurent Pinchart