Re: [PATCH 5/7] media: renesas: vsp1: Fix RWPF media bus code and frame size enumeration

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

On Wed, May 28, 2025 at 04:58:45PM +0200, Jacopo Mondi wrote:
> On Wed, Apr 30, 2025 at 02:53:20AM +0300, Laurent Pinchart wrote:
> > The RWPF can't freely convert between all input and output formats. They
> > support RGB <-> YUV conversion, but HSV formats can't be converted. Fix
> > the media bus code and frame size enumeration to take this into account
> > on the source pad.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  .../media/platform/renesas/vsp1/vsp1_rwpf.c   | 80 +++++++++++++++++--
> >  1 file changed, 74 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > index 61f7e13ebeee..bd97fc75eb5b 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> > @@ -21,29 +21,97 @@
> >   * V4L2 Subdevice Operations
> >   */
> >
> > +/* Keep HSV last. */
> >  static const unsigned int rwpf_codes[] = {
> > +	MEDIA_BUS_FMT_AYUV8_1X32,
> >  	MEDIA_BUS_FMT_ARGB8888_1X32,
> >  	MEDIA_BUS_FMT_AHSV8888_1X32,
> > -	MEDIA_BUS_FMT_AYUV8_1X32,
> >  };
> >
> >  static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev,
> >  				    struct v4l2_subdev_state *sd_state,
> >  				    struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -	if (code->index >= ARRAY_SIZE(rwpf_codes))
> > +	struct vsp1_entity *entity = to_vsp1_entity(subdev);
> > +	struct v4l2_subdev_state *state;
> > +	struct v4l2_mbus_framefmt *format;
> 
> minor nit: could you move this one line up for ... reverse xmas tree
> ordering :)

state goes first because in my head you get the state and then access
the format from it. As the line are nearly the same length, the breach
of the reverse xmas tree rule didn't shock me. We're really getting into
nitpicking territory :-)

This will go away when switching to the V4L2 subdev active state API. I
have a patch series for that, but need to fix a lockdep issue.

> > +
> > +	if (code->pad == RWPF_PAD_SINK)
> > +		return vsp1_subdev_enum_mbus_code(subdev, sd_state, code);
> > +
> > +	state = vsp1_entity_get_state(entity, sd_state, code->which);
> > +	if (!state)
> >  		return -EINVAL;
> >
> > -	code->code = rwpf_codes[code->index];
> > +	format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
> >
> > -	if (code->pad == RWPF_PAD_SOURCE &&
> > -	    code->code == MEDIA_BUS_FMT_AYUV8_1X32)
> > +	guard(mutex)(&entity->lock);
> > +
> > +	/*
> > +	 * The RWPF supports conversion between RGB and YUV formats, but HSV
> > +	 * formats can't be converted.
> > +	 */
> > +	if (format->code == MEDIA_BUS_FMT_AHSV8888_1X32) {
> > +		if (code->index)
> > +			return -EINVAL;
> > +
> > +		code->code = MEDIA_BUS_FMT_AHSV8888_1X32;
> > +	} else {
> > +		if (code->index >= ARRAY_SIZE(rwpf_codes) - 1)
> > +			return -EINVAL;
> > +
> > +		code->code = rwpf_codes[code->index];
> > +	}
> > +
> > +	if (code->code == MEDIA_BUS_FMT_AYUV8_1X32)
> >  		code->flags = V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC
> >  			    | V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
> >
> >  	return 0;
> >  }
> >
> > +static int vsp1_rwpf_enum_frame_size(struct v4l2_subdev *subdev,
> > +				     struct v4l2_subdev_state *sd_state,
> > +				     struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +	struct vsp1_entity *entity = to_vsp1_entity(subdev);
> > +	struct v4l2_subdev_state *state;
> > +	struct v4l2_mbus_framefmt *format;
> > +
> > +	if (fse->pad == RWPF_PAD_SINK)
> > +		return vsp1_subdev_enum_frame_size(subdev, sd_state, fse);
> > +
> > +	if (fse->index)
> > +		return -EINVAL;
> > +
> > +	state = vsp1_entity_get_state(entity, sd_state, fse->which);
> > +	if (!state)
> > +		return -EINVAL;
> > +
> > +	format = v4l2_subdev_state_get_format(state, 0);
> 
> Could you use RWPF_PAD_SINK ?

Will do.

> > +
> > +	guard(mutex)(&entity->lock);
> 
> As a general question, shouldn't we use the state lock ?

The series I mentioned above will handle this. The driver doesn't
currently use the active statue API.

> > +
> > +	/*
> > +	 * The RWPF supports conversion between RGB and YUV formats, but
> > +	 * HSV formats can't be converted.
> > +	 */
> > +	if ((format->code == MEDIA_BUS_FMT_AHSV8888_1X32) !=
> > +	    (fse->code == MEDIA_BUS_FMT_AHSV8888_1X32))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The size on the source pad is fixed and always identical to
> > +	 * the sink pad.
> > +	 */
> > +	fse->min_width = format->width;
> > +	fse->max_width = format->width;
> > +	fse->min_height = format->height;
> > +	fse->max_height = format->height;
> > +
> > +	return 0;
> > +}
> 
> All minors
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> > +
> >  static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
> >  				struct v4l2_subdev_state *sd_state,
> >  				struct v4l2_subdev_format *fmt)
> > @@ -275,7 +343,7 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev,
> >
> >  static const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = {
> >  	.enum_mbus_code = vsp1_rwpf_enum_mbus_code,
> > -	.enum_frame_size = vsp1_subdev_enum_frame_size,
> > +	.enum_frame_size = vsp1_rwpf_enum_frame_size,
> >  	.get_fmt = vsp1_subdev_get_pad_format,
> >  	.set_fmt = vsp1_rwpf_set_format,
> >  	.get_selection = vsp1_rwpf_get_selection,

-- 
Regards,

Laurent Pinchart