Re: [PATCH v2 5/6] media: vsp1: rwpf: Support RAW Bayer and ISP config

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

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 24, 2025 at 09:19:45PM +0100, Jacopo Mondi wrote:
> With the forthcoming support for VSPX the r/wpf unit will be used
> to perform memory access on the behalf of the ISP units.
> 
> Prepare to support reading from external memory images in RAW Bayer
> format and ISP configuration parameters by expanding the list
> of supported media bus codes.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_rwpf.c | 97 +++++++++++++++++++++++--
>  1 file changed, 92 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> index 93b0ed5fd0da0c6a182dbbfe1e54eb8cfd66c493..aef7b3d53a2171cda028a272f587641b4a8f85dc 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> @@ -10,18 +10,102 @@
>  #include <media/v4l2-subdev.h>
>  
>  #include "vsp1.h"
> +#include "vsp1_pipe.h"

I don't think this is needed.

>  #include "vsp1_rwpf.h"
>  #include "vsp1_video.h"
>  
>  #define RWPF_MIN_WIDTH				1
>  #define RWPF_MIN_HEIGHT				1
>  
> +struct vsp1_rwpf_codes {
> +	const u32 *codes;
> +	unsigned int num_codes;
> +};
> +
>  static const u32 rwpf_mbus_codes[] = {
>  	MEDIA_BUS_FMT_ARGB8888_1X32,
>  	MEDIA_BUS_FMT_AHSV8888_1X32,
>  	MEDIA_BUS_FMT_AYUV8_1X32,
>  };
>  
> +static const struct vsp1_rwpf_codes rwpf_codes = {
> +	.codes = rwpf_mbus_codes,
> +	.num_codes = ARRAY_SIZE(rwpf_mbus_codes),
> +};
> +
> +static const u32 vspx_rpf0_mbus_codes[] = {
> +	MEDIA_BUS_FMT_SBGGR8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
> +	MEDIA_BUS_FMT_SGBRG12_1X12,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SBGGR16_1X16,
> +	MEDIA_BUS_FMT_SGBRG16_1X16,
> +	MEDIA_BUS_FMT_SGRBG16_1X16,
> +	MEDIA_BUS_FMT_SRGGB16_1X16,

Same comment as in 1/6,

	MEDIA_BUS_FMT_Y8_1X8,
	MEDIA_BUS_FMT_Y10_1X10,
	MEDIA_BUS_FMT_Y12_1X12,
	MEDIA_BUS_FMT_Y16_1X16,

> +	MEDIA_BUS_FMT_METADATA_FIXED
> +};
> +
> +static const struct vsp1_rwpf_codes vspx_rpf0_codes = {
> +	.codes = vspx_rpf0_mbus_codes,
> +	.num_codes = ARRAY_SIZE(vspx_rpf0_mbus_codes),
> +};
> +
> +static const u32 vspx_rpf1_mbus_codes[] = {
> +	MEDIA_BUS_FMT_SBGGR8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
> +	MEDIA_BUS_FMT_SGBRG12_1X12,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SBGGR16_1X16,
> +	MEDIA_BUS_FMT_SGBRG16_1X16,
> +	MEDIA_BUS_FMT_SGRBG16_1X16,
> +	MEDIA_BUS_FMT_SRGGB16_1X16,

Here too.

> +};
> +
> +static const struct vsp1_rwpf_codes vspx_rpf1_codes = {
> +	.codes = vspx_rpf1_mbus_codes,
> +	.num_codes = ARRAY_SIZE(vspx_rpf1_mbus_codes),
> +};
> +
> +static const struct vsp1_rwpf_codes *vsp1_rwpf_codes(struct v4l2_subdev *sd)
> +{
> +	struct vsp1_rwpf *rwpf = to_rwpf(sd);
> +	struct vsp1_entity *ent = &rwpf->entity;
> +
> +	/* Only VSPX supports reading Bayer formats. */
> +	if (!vsp1_feature(ent->vsp1, VSP1_HAS_IIF))
> +		return &rwpf_codes;
> +
> +	if (ent->type == VSP1_ENTITY_RPF) {
> +		switch (ent->index) {
> +		case 0:
> +			/* VSPX RPF0 supports ISP config data too. */
> +			return &vspx_rpf0_codes;
> +		case 1:
> +			return &vspx_rpf1_codes;
> +		default:

This should never happen. See below for a proposal on how to handle it.

> +			return &rwpf_codes;
> +		}
> +	}
> +
> +	return &rwpf_codes;

You could lower indentation with

	if (ent->type != VSP1_ENTITY_RPF)
		return &rwpf_codes;

	/* Only VSPX supports reading Bayer formats. */
	if (!vsp1_feature(ent->vsp1, VSP1_HAS_IIF))
		return &rwpf_codes;

	switch (ent->index) {
	case 0:
		/* VSPX RPF0 supports ISP config data too. */
		return &vspx_rpf0_codes;
	case 1:
		return &vspx_rpf1_codes;
	default:
		return &rwpf_codes;
	}

> +}

Would it make sense to call this function at init time, and store the
const struct vsp1_rwpf_codes pointer in the vsp1_rwpf structure ? You
could rename vsp1_rwpf_init_ctrls() to vsp1_rwpf_init() (or add a
vsp1_rwpf_init() function that calls vsp1_rwpf_init_ctrls()) and handle
the format initialization there. If the RPF index is > 1,
vsp1_rwpf_init() should return an error (callers should be fixed to
handle the error, that's a patch that can be applied on its own
already).

> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdevice Operations
>   */
> @@ -30,10 +114,12 @@ 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_mbus_codes))
> +	const struct vsp1_rwpf_codes *codes = vsp1_rwpf_codes(subdev);
> +
> +	if (code->index >= codes->num_codes)
>  		return -EINVAL;
>  
> -	code->code = rwpf_mbus_codes[code->index];
> +	code->code = codes->codes[code->index];
>  
>  	return 0;
>  }
> @@ -54,6 +140,7 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  				struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_format *fmt)
>  {
> +	const struct vsp1_rwpf_codes *codes = vsp1_rwpf_codes(subdev);
>  	struct vsp1_rwpf *rwpf = to_rwpf(subdev);
>  	struct v4l2_subdev_state *state;
>  	struct v4l2_mbus_framefmt *format;
> @@ -69,11 +156,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  	}
>  
>  	/* Default to YUV if the requested format is not supported. */
> -	for (i = 0; i < ARRAY_SIZE(rwpf_mbus_codes); ++i) {
> -		if (fmt->format.code == rwpf_mbus_codes[i])
> +	for (i = 0; i < codes->num_codes; ++i) {
> +		if (fmt->format.code == codes->codes[i])
>  			break;
>  	}
> -	if (i == ARRAY_SIZE(rwpf_mbus_codes))
> +	if (i == codes->num_codes)
>  		fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;

This isn't a valid format for the VSP-X. I would pick codes->codes[0] as
a default.

>  
>  	format = v4l2_subdev_state_get_format(state, fmt->pad);
> 

-- 
Regards,

Laurent Pinchart