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@xxxxxxxxxxxxxxxx>
> ---
> 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
