| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250302134536.GL18557@pendragon.ideasonboard.com>
Date: Sun, 2 Mar 2025 15:45:36 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
Niklas Söderlund <niklas.soderlund@...natech.se>,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v2 1/6] media: vsp1: Add support IIF ISP Interface
On Sun, Mar 02, 2025 at 03:42:11PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Feb 24, 2025 at 09:19:41PM +0100, Jacopo Mondi wrote:
> > The IIF (ISP InterFace) is a VSP2 function that transfers data
> > to the ISP by reading from external memory through two RPF
> > instances.
> >
> > Add support for it in the vsp1 driver by introducing a new entity
> > type. The sole required operation is to enable the IIF function
> > during configure_stream().
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>
> > ---
> > drivers/media/platform/renesas/vsp1/Makefile | 2 +-
> > drivers/media/platform/renesas/vsp1/vsp1.h | 3 +
> > drivers/media/platform/renesas/vsp1/vsp1_drv.c | 11 ++
> > drivers/media/platform/renesas/vsp1/vsp1_entity.c | 8 ++
> > drivers/media/platform/renesas/vsp1/vsp1_entity.h | 1 +
> > drivers/media/platform/renesas/vsp1/vsp1_iif.c | 133 ++++++++++++++++++++++
> > drivers/media/platform/renesas/vsp1/vsp1_iif.h | 26 +++++
> > drivers/media/platform/renesas/vsp1/vsp1_pipe.c | 1 +
> > drivers/media/platform/renesas/vsp1/vsp1_pipe.h | 1 +
> > drivers/media/platform/renesas/vsp1/vsp1_regs.h | 8 ++
> > drivers/media/platform/renesas/vsp1/vsp1_rpf.c | 7 ++
> > 11 files changed, 200 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/Makefile b/drivers/media/platform/renesas/vsp1/Makefile
> > index 4bb4dcbef7b55be7a40231a04e14029911da9beb..de8c802e1d1a16aabe1326fc8c27be33ad2b2e10 100644
> > --- a/drivers/media/platform/renesas/vsp1/Makefile
> > +++ b/drivers/media/platform/renesas/vsp1/Makefile
> > @@ -5,6 +5,6 @@ vsp1-y += vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o
> > vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o
> > vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o
> > vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o
> > -vsp1-y += vsp1_lif.o vsp1_uif.o
> > +vsp1-y += vsp1_iif.o vsp1_lif.o vsp1_uif.o
> >
> > obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
> > index 2f6f0c6ae55514d312e48232a5f8c8673f69ba13..263024639dd2a7564fc70ad52dbe17f9a5279f45 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1.h
> > @@ -32,6 +32,7 @@ struct vsp1_clu;
> > struct vsp1_hgo;
> > struct vsp1_hgt;
> > struct vsp1_hsit;
> > +struct vsp1_iif;
> > struct vsp1_lif;
> > struct vsp1_lut;
> > struct vsp1_rwpf;
> > @@ -56,6 +57,7 @@ struct vsp1_uif;
> > #define VSP1_HAS_BRS BIT(9)
> > #define VSP1_HAS_EXT_DL BIT(10)
> > #define VSP1_HAS_NON_ZERO_LBA BIT(11)
> > +#define VSP1_HAS_IIF BIT(12)
> >
> > struct vsp1_device_info {
> > u32 version;
> > @@ -91,6 +93,7 @@ struct vsp1_device {
> > struct vsp1_hgt *hgt;
> > struct vsp1_hsit *hsi;
> > struct vsp1_hsit *hst;
> > + struct vsp1_iif *iif;
> > struct vsp1_lif *lif[VSP1_MAX_LIF];
> > struct vsp1_lut *lut;
> > struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > index 9fc6bf624a520ae38e9c5f30dfa4dfa412eec38e..d13e9b31aa7cff8610ea196ae4de88ab22e44e0f 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > @@ -29,6 +29,7 @@
> > #include "vsp1_hgo.h"
> > #include "vsp1_hgt.h"
> > #include "vsp1_hsit.h"
> > +#include "vsp1_iif.h"
> > #include "vsp1_lif.h"
> > #include "vsp1_lut.h"
> > #include "vsp1_pipe.h"
> > @@ -340,6 +341,16 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
> > &vsp1->entities);
> > }
> >
> > + if (vsp1_feature(vsp1, VSP1_HAS_IIF)) {
> > + vsp1->iif = vsp1_iif_create(vsp1);
> > + if (IS_ERR(vsp1->iif)) {
> > + ret = PTR_ERR(vsp1->iif);
> > + goto done;
> > + }
> > +
> > + list_add_tail(&vsp1->iif->entity.list_dev, &vsp1->entities);
> > + }
> > +
> > /*
> > * The LIFs are only supported when used in conjunction with the DU, in
> > * which case the userspace API is disabled. If the userspace API is
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > index 8b8945bd8f108354f1b484530bc496dbac7d3d88..2096a09a12780ce64d9e8690aa4363bdaef3521e 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > @@ -63,9 +63,14 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
> > /*
> > * The ILV and BRS share the same data path route. The extra BRSSEL bit
> > * selects between the ILV and BRS.
> > + *
> > + * The BRU and IIF share the same data path route. The extra IIFSEL bit
> > + * selects between the IIF and BRU.
> > */
> > if (source->type == VSP1_ENTITY_BRS)
> > route |= VI6_DPR_ROUTE_BRSSEL;
> > + else if (source->type == VSP1_ENTITY_IIF)
> > + route |= VI6_DPR_ROUTE_IIFSEL;
> > vsp1_dl_body_write(dlb, source->route->reg, route);
> > }
> >
> > @@ -528,6 +533,9 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad)
> > { VI6_DPR_NODE_WPF(idx) }, VI6_DPR_NODE_WPF(idx) }
> >
> > static const struct vsp1_route vsp1_routes[] = {
> > + { VSP1_ENTITY_IIF, 0, VI6_DPR_BRU_ROUTE,
> > + { VI6_DPR_NODE_BRU_IN(0), VI6_DPR_NODE_BRU_IN(1),
> > + VI6_DPR_NODE_BRU_IN(3) }, VI6_DPR_NODE_WPF(0) },
> > { VSP1_ENTITY_BRS, 0, VI6_DPR_ILV_BRS_ROUTE,
> > { VI6_DPR_NODE_BRS_IN(0), VI6_DPR_NODE_BRS_IN(1) }, 0 },
> > { VSP1_ENTITY_BRU, 0, VI6_DPR_BRU_ROUTE,
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.h b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > index 1bcc9e27dfdc7385e91987c84c46fc3725f3b90e..bdcb780a79dafccda3843bd65d337b64d6e12eb3 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.h
> > @@ -28,6 +28,7 @@ enum vsp1_entity_type {
> > VSP1_ENTITY_HGT,
> > VSP1_ENTITY_HSI,
> > VSP1_ENTITY_HST,
> > + VSP1_ENTITY_IIF,
> > VSP1_ENTITY_LIF,
> > VSP1_ENTITY_LUT,
> > VSP1_ENTITY_RPF,
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_iif.c b/drivers/media/platform/renesas/vsp1/vsp1_iif.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..12889885b14f8d61ee5e416b4111d2eced7a0715
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * vsp1_iif.c -- R-Car VSP1 IIF (ISP Interface)
> > + *
> > + * Copyright (C) 2025 Ideas On Board Oy
> > + * Copyright (C) 2025 Renesas Corporation
> > + */
> > +
> > +#include "vsp1.h"
> > +#include "vsp1_dl.h"
> > +#include "vsp1_iif.h"
> > +
> > +#define IIF_MIN_WIDTH 128U
> > +#define IIF_MIN_HEIGHT 32U
> > +#define IIF_MAX_WIDTH 5120U
> > +#define IIF_MAX_HEIGHT 4096U
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Device Access
> > + */
> > +
> > +static inline void vsp1_iif_write(struct vsp1_dl_body *dlb, u32 reg, u32 data)
> > +{
> > + vsp1_dl_body_write(dlb, reg, data);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 Subdevice Operations
> > + */
> > +
> > +static const unsigned int iif_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,
>
> As the VSP1 doesn't care about the bayer pattern, what would you think
> about simplifying this to
>
> 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 int iif_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + return vsp1_subdev_enum_mbus_code(subdev, sd_state, code, iif_codes,
> > + ARRAY_SIZE(iif_codes));
> > +}
> > +
> > +static int iif_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + return vsp1_subdev_enum_frame_size(subdev, sd_state, fse,
> > + IIF_MIN_WIDTH, IIF_MIN_HEIGHT,
> > + IIF_MAX_WIDTH, IIF_MAX_HEIGHT);
> > +}
> > +
> > +static int iif_set_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + return vsp1_subdev_set_pad_format(subdev, sd_state, fmt, iif_codes,
> > + ARRAY_SIZE(iif_codes),
> > + IIF_MIN_WIDTH, IIF_MIN_HEIGHT,
> > + IIF_MAX_WIDTH, IIF_MAX_HEIGHT);
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops iif_pad_ops = {
> > + .enum_mbus_code = iif_enum_mbus_code,
> > + .enum_frame_size = iif_enum_frame_size,
> > + .get_fmt = vsp1_subdev_get_pad_format,
> > + .set_fmt = iif_set_format,
> > +};
> > +
> > +static const struct v4l2_subdev_ops iif_ops = {
> > + .pad = &iif_pad_ops,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * VSP1 Entity Operations
> > + */
> > +
> > +static void iif_configure_stream(struct vsp1_entity *entity,
> > + struct v4l2_subdev_state *state,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_dl_list *dl,
> > + struct vsp1_dl_body *dlb)
> > +{
> > + vsp1_iif_write(dlb, VI6_IIF_CTRL, VI6_IIF_CTRL_CTRL);
> > +}
> > +
> > +static const struct vsp1_entity_operations iif_entity_ops = {
> > + .configure_stream = iif_configure_stream,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Initialization and Cleanup
> > + */
> > +
> > +struct vsp1_iif *vsp1_iif_create(struct vsp1_device *vsp1)
> > +{
> > + struct vsp1_iif *iif;
> > + int ret;
> > +
> > + iif = devm_kzalloc(vsp1->dev, sizeof(*iif), GFP_KERNEL);
> > + if (iif == NULL)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + iif->entity.ops = &iif_entity_ops;
> > + iif->entity.type = VSP1_ENTITY_IIF;
> > +
> > + /*
> > + * The IIF is never exposed to userspace, but media entity registration
> > + * requires a function to be set. Use PROC_VIDEO_PIXEL_FORMATTER just to
> > + * avoid triggering a WARN_ON(), the value won't be seen anywhere.
> > + */
> > + ret = vsp1_entity_init(vsp1, &iif->entity, "iif", 3, &iif_ops,
> > + MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
> > +
> > + return iif;
> > +}
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_iif.h b/drivers/media/platform/renesas/vsp1/vsp1_iif.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..165996a822c1baab1fc40636884c9e7a67e4db07
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * vsp1_iif.h -- R-Car VSP1 IIF (ISP Interface)
> > + *
> > + * Copyright (C) 2025 Ideas On Board Oy
> > + * Copyright (C) 2025 Renesas Corporation
> > + */
> > +#ifndef __VSP1_IIF_H__
> > +#define __VSP1_IIF_H__
> > +
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include "vsp1_entity.h"
> > +
> > +struct vsp1_iif {
> > + struct vsp1_entity entity;
> > +};
> > +
> > +static inline struct vsp1_iif *to_iif(struct v4l2_subdev *subdev)
> > +{
> > + return container_of(subdev, struct vsp1_iif, entity.subdev);
> > +}
> > +
> > +struct vsp1_iif *vsp1_iif_create(struct vsp1_device *vsp1);
> > +
> > +#endif /* __VSP1_IIF_H__ */
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > index bb0739f684f39e23326a4d8fdb9f43e020bc23c8..8e9be3ec1b4dbdad1cbe35ae3a88952f46e41343 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
> > @@ -286,6 +286,7 @@ void vsp1_pipeline_reset(struct vsp1_pipeline *pipe)
> > pipe->brx = NULL;
> > pipe->hgo = NULL;
> > pipe->hgt = NULL;
> > + pipe->iif = NULL;
> > pipe->lif = NULL;
> > pipe->uds = NULL;
> > }
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > index 1ba7bdbad5a845da0a4d71888e193e46d62bed90..1655a820da102003d3d7da82a7cdd64e01c29ac6 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.h
> > @@ -119,6 +119,7 @@ struct vsp1_pipeline {
> > struct vsp1_entity *brx;
> > struct vsp1_entity *hgo;
> > struct vsp1_entity *hgt;
> > + struct vsp1_entity *iif;
> > struct vsp1_entity *lif;
> > struct vsp1_entity *uds;
> > struct vsp1_entity *uds_input;
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > index 7eca82e0ba7ec5e02a5f3b9a30ccdcb48db39ed2..86e47c2d991fb4e0719b63c4ccb29340610ac24f 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > @@ -252,6 +252,13 @@
> > #define VI6_RPF_BRDITH_CTRL_ODE BIT(8)
> > #define VI6_RPF_BRDITH_CTRL_CBRM BIT(0)
> >
> > +/* -----------------------------------------------------------------------------
> > + * IIF Control Registers
> > + */
> > +
> > +#define VI6_IIF_CTRL 0x0608
> > +#define VI6_IIF_CTRL_CTRL 0x13
> > +
> > /* -----------------------------------------------------------------------------
> > * WPF Control Registers
> > */
> > @@ -388,6 +395,7 @@
> > #define VI6_DPR_HST_ROUTE 0x2044
> > #define VI6_DPR_HSI_ROUTE 0x2048
> > #define VI6_DPR_BRU_ROUTE 0x204c
> > +#define VI6_DPR_ROUTE_IIFSEL BIT(28)
> > #define VI6_DPR_ILV_BRS_ROUTE 0x2050
> > #define VI6_DPR_ROUTE_BRSSEL BIT(28)
> > #define VI6_DPR_ROUTE_FXA_MASK (0xff << 16)
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > index 5c8b3ba1bd3c2c7b9289f05c9c2578e9717c23ff..e10ed836153633c4fe27224d0df39c77d4c8ab89 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> > @@ -163,6 +163,13 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
> > if (pipe->interlaced)
> > top /= 2;
> >
> > + /* No further configuration for VSPX. */
> > + if (pipe->iif) {
> > + /* VSPX wants alpha_sel to be set to 0. */
> > + vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, 0);
> > + return;
> > + }
> > +
>
> I think this belongs to a separate patch where you adjust the RPF and
> WPF code to support the VSP-X.
>
> With those small issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
I meant
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> > vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
> > (left << VI6_RPF_LOC_HCOORD_SHIFT) |
> > (top << VI6_RPF_LOC_VCOORD_SHIFT));
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists