[PATCH 0/6] media: renesas: vsp1: Add support for IIF

[PATCH 0/6] media: renesas: vsp1: Add support for IIF

[Jacopo Mondi]

The IIF (ISP InterFace) is specialized BRU version that reads data from
external memory using two RPF instances and feed it to the ISP.

The IIF support is modeled in the vsp1 driver as a new, simple, entity type.

IIF is part of VSPX, a version of the VSP2 IP specialized for ISP
interfacing. To prepare to support VSPX, support IIF first by
introducing a new entity and by adjusting the RPF/WPF drivers to
operate correctly when an IIF is present.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
Jacopo Mondi (6):
      media: vsp1: Add support IIF ISP Interface
      media: vsp1: Enable FREE interrupt
      media: vsp1: dl: Use singleshot DL for VSPX
      media: vsp1: rwpf: Break out format handling
      media: vsp1: rwpf: Support RAW Bayer and ISP config
      media: vsp1: rwpf: Support operations with IIF

 drivers/media/platform/renesas/vsp1/Makefile      |   2 +-
 drivers/media/platform/renesas/vsp1/vsp1.h        |   3 +
 drivers/media/platform/renesas/vsp1/vsp1_dl.c     |   7 +-
 drivers/media/platform/renesas/vsp1/vsp1_drv.c    |  14 ++-
 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    | 134 ++++++++++++++++++++++
 drivers/media/platform/renesas/vsp1/vsp1_iif.h    |  31 +++++
 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   |   7 ++
 drivers/media/platform/renesas/vsp1/vsp1_rpf.c    |  66 ++++++-----
 drivers/media/platform/renesas/vsp1/vsp1_rwpf.c   |  42 +++++--
 drivers/media/platform/renesas/vsp1/vsp1_wpf.c    |  23 +++-
 14 files changed, 293 insertions(+), 47 deletions(-)
---
base-commit: 94794b5ce4d90ab134b0b101a02fddf6e74c437d
change-id: 20250123-v4h-iif-a1dda640c95d

Best regards,
-- 
Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

[PATCH 1/6] media: vsp1: Add support IIF ISP Interface

[Jacopo Mondi]

The IIF (ISP InterFace) is a specialized BRU version 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 on the
BRU during configure_stream().

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.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    | 134 ++++++++++++++++++++++
 drivers/media/platform/renesas/vsp1/vsp1_iif.h    |  31 +++++
 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   |   7 ++
 10 files changed, 198 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/vsp1/Makefile b/drivers/media/platform/renesas/vsp1/Makefile
index 4bb4dcbef7b55be7a40231a04e14029911da9beb..e080377af01672451c3274118f74dbd132ec573f 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_lif.o vsp1_uif.o vsp1_iif.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..cbaad0ea0b73f90f3994bbdfb4304d2f71eabccd 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"
@@ -292,6 +293,16 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 		list_add_tail(&vsp1->bru->entity.list_dev, &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);
+	}
+
 	if (vsp1_feature(vsp1, VSP1_HAS_CLU)) {
 		vsp1->clu = vsp1_clu_create(vsp1);
 		if (IS_ERR(vsp1->clu)) {
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
index 8b8945bd8f108354f1b484530bc496dbac7d3d88..343fc8f7fd6ef37a1c2e10ceaca2b7818cbd752d 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.
+	 *
+	 * For VSPX the IIF (a specialized BRU) is enabled with the extra
+	 * IIFSEL bit.
 	 */
 	if (source->type == VSP1_ENTITY_BRS)
 		route |= VI6_DPR_ROUTE_BRSSEL;
+	else if (source->type == VSP1_ENTITY_IIF)
+		route |= VI6_DPR_BRU_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..ceb5e5988b02d86e9b444036ba70c7517d863850
--- /dev/null
+++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.c
@@ -0,0 +1,134 @@
+// 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_brx.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_iif *iif,
+				  struct vsp1_dl_body *dlb, u32 reg, u32 data)
+{
+	vsp1_dl_body_write(dlb, iif->base + reg, data);
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2 Subdevice Operations
+ */
+
+static const unsigned int iif_codes[] = {
+	MEDIA_BUS_FMT_SRGGB8_1X8,
+	MEDIA_BUS_FMT_SGRBG8_1X8,
+	MEDIA_BUS_FMT_SGBRG8_1X8,
+	MEDIA_BUS_FMT_SBGGR8_1X8,
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGBRG10_1X10,
+	MEDIA_BUS_FMT_SBGGR10_1X10,
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SGRBG12_1X12,
+	MEDIA_BUS_FMT_SGBRG12_1X12,
+	MEDIA_BUS_FMT_SBGGR12_1X12,
+	MEDIA_BUS_FMT_SRGGB16_1X16,
+	MEDIA_BUS_FMT_SGRBG16_1X16,
+	MEDIA_BUS_FMT_SGBRG16_1X16,
+	MEDIA_BUS_FMT_SBGGR16_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)
+{
+	struct vsp1_iif *iif = to_iif(&entity->subdev);
+
+	vsp1_iif_write(iif, 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 is a specialized BRU instance, uses the same register base. */
+	iif->base = VI6_BRU_BASE;
+	iif->entity.ops = &iif_entity_ops;
+	iif->entity.type = VSP1_ENTITY_IIF;
+
+	ret = vsp1_entity_init(vsp1, &iif->entity, "iif", 3, &iif_ops,
+			       MEDIA_ENT_F_PROC_VIDEO_COMPOSER);
+	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..2ec3b1e3aeaca943dab870ca997ff1a582ef8117
--- /dev/null
+++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.h
@@ -0,0 +1,31 @@
+/* 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/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+#include "vsp1_entity.h"
+
+#define IIF_PAD_SINK 0
+
+struct vsp1_iif {
+	struct vsp1_entity entity;
+	unsigned int base;
+};
+
+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..5d655785db85fe7cc7d7774fac168ca98891a0f5 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -252,6 +252,12 @@
 #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 +394,7 @@
 #define VI6_DPR_HST_ROUTE		0x2044
 #define VI6_DPR_HSI_ROUTE		0x2048
 #define VI6_DPR_BRU_ROUTE		0x204c
+#define VI6_DPR_BRU_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)

-- 
2.47.1

[PATCH 2/6] media: vsp1: Enable FREE interrupt

[Jacopo Mondi]

Enable the "FrameEnd" interrupt to support the VSPX operations.

The frame completion interrupt signals that the transfer of
the config buffer and image buffer to the ISP has completed.

Enable the interrupt source if the pipe has an IIF entity, such as
in the VSPX case.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_drv.c | 3 ++-
 drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index cbaad0ea0b73f90f3994bbdfb4304d2f71eabccd..5aa0751a896f8a58bd11128ccaa092c9596cdb5d 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -69,7 +69,8 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
 				i, wpf->entity.pipe->underrun_count);
 		}
 
-		if (status & VI6_WPF_IRQ_STA_DFE) {
+		if (status & VI6_WPF_IRQ_STA_DFE ||
+		    status & VI6_WPF_IRQ_STA_FRE) {
 			vsp1_pipeline_frame_end(wpf->entity.pipe);
 			ret = IRQ_HANDLED;
 		}
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
index f176750ccd9847fdb8d51f7f51a6bd5092b70197..93a663f58a5930a3c7c40a96a30888d0b8ccb2ed 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
@@ -239,6 +239,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	const struct v4l2_mbus_framefmt *source_format;
 	const struct v4l2_mbus_framefmt *sink_format;
 	unsigned int index = wpf->entity.index;
+	u32 irqmask = 0;
 	unsigned int i;
 	u32 outfmt = 0;
 	u32 srcrpf = 0;
@@ -312,9 +313,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
 
 	/* Enable interrupts. */
+	irqmask = VI6_WPF_IRQ_ENB_DFEE | (pipe->iif ? VI6_WPF_IRQ_ENB_FREE : 0);
 	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
-	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
-			   VI6_WPF_IRQ_ENB_DFEE);
+	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irqmask);
 
 	/*
 	 * Configure writeback for display pipelines (the wpf writeback flag is

-- 
2.47.1

[PATCH 3/6] media: vsp1: dl: Use singleshot DL for VSPX

[Jacopo Mondi]

The vsp1_dl library allows to program a display list and feed it
continuously to the VSP2. As an alternative operation mode, the library
allows to program the VSP2 in 'single shot' mode, where a display list
is submitted to the VSP on request only.

Currently the 'single shot' mode is only available when the VSP2 is
controlled by userspace, while it works in continuous mode when driven
by DRM, as frames have to be submitted to the display continuously.

For the VSPX use case, where there is no uapi support, we should however
work in single-shot mode as the ISP driver programs the VSPX on
request.

Initialize the display lists in single shot mode in case the VSP1 is
controlled by userspace or in case the pipeline features an IIF entity,
as in the VSPX case.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
@@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
 		return NULL;
 
 	dlm->index = index;
-	dlm->singleshot = vsp1->info->uapi;
+	/*
+	 * uapi = single shot mode;
+	 * DRM = continuous mode;
+	 * VSPX = single shot mode;
+	 */
+	dlm->singleshot = (vsp1->info->uapi || vsp1->iif);
 	dlm->vsp1 = vsp1;
 
 	spin_lock_init(&dlm->lock);

-- 
2.47.1

[PATCH 4/6] media: vsp1: rwpf: Break out format handling

[Jacopo Mondi]

The current implementation of the r/wpf format handling assumes
three formats to be supported in the RGB/YUV space.

With the forthcoming support for VSPX the r/wpf units will be
used to fetch from exteranal memory images in RAW Bayer format
and buffers of ISP configuration parameters.

Prepare for adding support for these new formats by breaking
out the list of supported media bus codes in the vsp1_rwpf.c
file.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_rwpf.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
index 9d38203e73d00b82a1a7db0353e2f0b5a94084f6..93b0ed5fd0da0c6a182dbbfe1e54eb8cfd66c493 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
@@ -16,6 +16,12 @@
 #define RWPF_MIN_WIDTH				1
 #define RWPF_MIN_HEIGHT				1
 
+static const u32 rwpf_mbus_codes[] = {
+	MEDIA_BUS_FMT_ARGB8888_1X32,
+	MEDIA_BUS_FMT_AHSV8888_1X32,
+	MEDIA_BUS_FMT_AYUV8_1X32,
+};
+
 /* -----------------------------------------------------------------------------
  * V4L2 Subdevice Operations
  */
@@ -24,16 +30,10 @@ static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev,
 				    struct v4l2_subdev_state *sd_state,
 				    struct v4l2_subdev_mbus_code_enum *code)
 {
-	static const unsigned int codes[] = {
-		MEDIA_BUS_FMT_ARGB8888_1X32,
-		MEDIA_BUS_FMT_AHSV8888_1X32,
-		MEDIA_BUS_FMT_AYUV8_1X32,
-	};
-
-	if (code->index >= ARRAY_SIZE(codes))
+	if (code->index >= ARRAY_SIZE(rwpf_mbus_codes))
 		return -EINVAL;
 
-	code->code = codes[code->index];
+	code->code = rwpf_mbus_codes[code->index];
 
 	return 0;
 }
@@ -57,6 +57,7 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
 	struct vsp1_rwpf *rwpf = to_rwpf(subdev);
 	struct v4l2_subdev_state *state;
 	struct v4l2_mbus_framefmt *format;
+	unsigned int i;
 	int ret = 0;
 
 	mutex_lock(&rwpf->entity.lock);
@@ -68,9 +69,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
 	}
 
 	/* Default to YUV if the requested format is not supported. */
-	if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 &&
-	    fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 &&
-	    fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
+	for (i = 0; i < ARRAY_SIZE(rwpf_mbus_codes); ++i) {
+		if (fmt->format.code == rwpf_mbus_codes[i])
+			break;
+	}
+	if (i == ARRAY_SIZE(rwpf_mbus_codes))
 		fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
 
 	format = v4l2_subdev_state_get_format(state, fmt->pad);

-- 
2.47.1

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

[Jacopo Mondi]

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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
index 93b0ed5fd0da0c6a182dbbfe1e54eb8cfd66c493..75dde92ec35585825eb70c0faa31ff4cb1d4a3d4 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
@@ -20,6 +20,23 @@ static const u32 rwpf_mbus_codes[] = {
 	MEDIA_BUS_FMT_ARGB8888_1X32,
 	MEDIA_BUS_FMT_AHSV8888_1X32,
 	MEDIA_BUS_FMT_AYUV8_1X32,
+	MEDIA_BUS_FMT_SRGGB8_1X8,
+	MEDIA_BUS_FMT_SGRBG8_1X8,
+	MEDIA_BUS_FMT_SGBRG8_1X8,
+	MEDIA_BUS_FMT_SBGGR8_1X8,
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGBRG10_1X10,
+	MEDIA_BUS_FMT_SBGGR10_1X10,
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SGRBG12_1X12,
+	MEDIA_BUS_FMT_SGBRG12_1X12,
+	MEDIA_BUS_FMT_SBGGR12_1X12,
+	MEDIA_BUS_FMT_SRGGB16_1X16,
+	MEDIA_BUS_FMT_SGRBG16_1X16,
+	MEDIA_BUS_FMT_SGBRG16_1X16,
+	MEDIA_BUS_FMT_SBGGR16_1X16,
+	MEDIA_BUS_FMT_METADATA_FIXED
 };
 
 /* -----------------------------------------------------------------------------

-- 
2.47.1

[PATCH 6/6] media: vsp1: rwpf: Support operations with IIF

[Jacopo Mondi]

When the RPF/WPF units are used for ISP interfacing through
the IIF, the set of accessible registers is limited compared to
the regular VSPD operations.

Support ISP interfacing in the rpf and wpf drivers by checking if
the pipe features an IIF instance and writing only the relevant
registers.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/vsp1/vsp1_rpf.c | 66 +++++++++++++++-----------
 drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 18 +++++--
 2 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
index 5c8b3ba1bd3c2c7b9289f05c9c2578e9717c23ff..e215491a3d962e2ae3c06b7835ca3b7654f04d10 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
@@ -60,6 +60,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 	const struct v4l2_mbus_framefmt *sink_format;
 	unsigned int left = 0;
 	unsigned int top = 0;
+	u32 alpha_sel = 0;
 	u32 pstride;
 	u32 infmt;
 
@@ -84,7 +85,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 	sink_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
 	source_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SOURCE);
 
-	infmt = VI6_RPF_INFMT_CIPM
+	infmt = (pipe->iif ? 0 : VI6_RPF_INFMT_CIPM)
 	      | (fmtinfo->hwfmt << VI6_RPF_INFMT_RDFMT_SHIFT);
 
 	if (fmtinfo->swap_yc)
@@ -98,7 +99,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 	vsp1_rpf_write(rpf, dlb, VI6_RPF_INFMT, infmt);
 	vsp1_rpf_write(rpf, dlb, VI6_RPF_DSWAP, fmtinfo->swap);
 
-	if (entity->vsp1->info->gen == 4) {
+	if (entity->vsp1->info->gen == 4 && !pipe->iif) {
 		u32 ext_infmt0;
 		u32 ext_infmt1;
 		u32 ext_infmt2;
@@ -150,23 +151,6 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 		vsp1_rpf_write(rpf, dlb, VI6_RPF_EXT_INFMT2, ext_infmt2);
 	}
 
-	/* Output location. */
-	if (pipe->brx) {
-		const struct v4l2_rect *compose;
-
-		compose = v4l2_subdev_state_get_compose(pipe->brx->state,
-							rpf->brx_input);
-		left = compose->left;
-		top = compose->top;
-	}
-
-	if (pipe->interlaced)
-		top /= 2;
-
-	vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
-		       (left << VI6_RPF_LOC_HCOORD_SHIFT) |
-		       (top << VI6_RPF_LOC_VCOORD_SHIFT));
-
 	/*
 	 * On Gen2 use the alpha channel (extended to 8 bits) when available or
 	 * a fixed alpha value set through the V4L2_CID_ALPHA_COMPONENT control
@@ -188,11 +172,35 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 	 * contains an alpha channel. On Gen2 the global alpha is ignored in
 	 * that case.
 	 *
-	 * In all cases, disable color keying.
+	 * In all the above cases, disable color keying.
+	 *
+	 * VSPX wants alpha_sel to be set to 0.
 	 */
-	vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, VI6_RPF_ALPH_SEL_AEXT_EXT |
-		       (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
-				       : VI6_RPF_ALPH_SEL_ASEL_FIXED));
+	alpha_sel = pipe->iif ? 0
+			      : VI6_RPF_ALPH_SEL_AEXT_EXT
+				| (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
+						  : VI6_RPF_ALPH_SEL_ASEL_FIXED);
+	vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, alpha_sel);
+
+	if (pipe->iif)
+		return;
+
+	/* Output location. */
+	if (pipe->brx) {
+		const struct v4l2_rect *compose;
+
+		compose = v4l2_subdev_state_get_compose(pipe->brx->state,
+							rpf->brx_input);
+		left = compose->left;
+		top = compose->top;
+	}
+
+	if (pipe->interlaced)
+		top /= 2;
+	vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
+		       (left << VI6_RPF_LOC_HCOORD_SHIFT) |
+		       (top << VI6_RPF_LOC_VCOORD_SHIFT));
+
 
 	if (entity->vsp1->info->gen >= 3) {
 		u32 mult;
@@ -338,11 +346,15 @@ static void rpf_configure_partition(struct vsp1_entity *entity,
 	 */
 	if (pipe->interlaced) {
 		vsp1_rpf_configure_autofld(rpf, dl);
-	} else {
-		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_Y, mem.addr[0]);
-		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C0, mem.addr[1]);
-		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C1, mem.addr[2]);
+		return;
 	}
+
+	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_Y, mem.addr[0]);
+	if (!mem.addr[1])
+		return;
+
+	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C0, mem.addr[1]);
+	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C1, mem.addr[2]);
 }
 
 static void rpf_partition(struct vsp1_entity *entity,
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
index 93a663f58a5930a3c7c40a96a30888d0b8ccb2ed..736f76389e07a7cc28ba098a0a0bdf350a0794f7 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
@@ -248,8 +248,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	sink_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
 	source_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SOURCE);
 
-	/* Format */
-	if (!pipe->lif || wpf->writeback) {
+	/*
+	 * Format configuration. Skip for IIF (VSPX) or if the pipe doesn't have
+	 * an active output.
+	 */
+	if (!pipe->iif && (!pipe->lif || wpf->writeback)) {
 		const struct v4l2_pix_format_mplane *format = &wpf->format;
 		const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 
@@ -292,7 +295,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	 * Sources. If the pipeline has a single input and BRx is not used,
 	 * configure it as the master layer. Otherwise configure all
 	 * inputs as sub-layers and select the virtual RPF as the master
-	 * layer.
+	 * layer. For VSPX configure the enabled sources as masters.
 	 */
 	for (i = 0; i < vsp1->info->rpf_count; ++i) {
 		struct vsp1_rwpf *input = pipe->inputs[i];
@@ -300,7 +303,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 		if (!input)
 			continue;
 
-		srcrpf |= (!pipe->brx && pipe->num_inputs == 1)
+		/* For VSPX we enable and use RPF0 only for now. */
+		if (pipe->iif && i > 0)
+			break;
+
+		srcrpf |= (pipe->iif || (!pipe->brx && pipe->num_inputs == 1))
 			? VI6_WPF_SRCRPF_RPF_ACT_MST(input->entity.index)
 			: VI6_WPF_SRCRPF_RPF_ACT_SUB(input->entity.index);
 	}
@@ -317,6 +324,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
 	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irqmask);
 
+	if (pipe->iif)
+		return;
+
 	/*
 	 * Configure writeback for display pipelines (the wpf writeback flag is
 	 * never set for memory-to-memory pipelines). Start by adding a chained

-- 
2.47.1

Re: [PATCH 0/6] media: renesas: vsp1: Add support for IIF

[Niklas Söderlund]

Hi Jacopo,

Thanks for your series.

On 2025-01-23 18:04:01 +0100, Jacopo Mondi wrote:
> The IIF (ISP InterFace) is specialized BRU version that reads data from
> external memory using two RPF instances and feed it to the ISP.
> 
> The IIF support is modeled in the vsp1 driver as a new, simple, entity type.
> 
> IIF is part of VSPX, a version of the VSP2 IP specialized for ISP
> interfacing. To prepare to support VSPX, support IIF first by
> introducing a new entity and by adjusting the RPF/WPF drivers to
> operate correctly when an IIF is present.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

I'm no expert on the VSP1, but the changes looks good. For the whole 
series,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Jacopo Mondi (6):
>       media: vsp1: Add support IIF ISP Interface
>       media: vsp1: Enable FREE interrupt
>       media: vsp1: dl: Use singleshot DL for VSPX
>       media: vsp1: rwpf: Break out format handling
>       media: vsp1: rwpf: Support RAW Bayer and ISP config
>       media: vsp1: rwpf: Support operations with IIF
> 
>  drivers/media/platform/renesas/vsp1/Makefile      |   2 +-
>  drivers/media/platform/renesas/vsp1/vsp1.h        |   3 +
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c     |   7 +-
>  drivers/media/platform/renesas/vsp1/vsp1_drv.c    |  14 ++-
>  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    | 134 ++++++++++++++++++++++
>  drivers/media/platform/renesas/vsp1/vsp1_iif.h    |  31 +++++
>  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   |   7 ++
>  drivers/media/platform/renesas/vsp1/vsp1_rpf.c    |  66 ++++++-----
>  drivers/media/platform/renesas/vsp1/vsp1_rwpf.c   |  42 +++++--
>  drivers/media/platform/renesas/vsp1/vsp1_wpf.c    |  23 +++-
>  14 files changed, 293 insertions(+), 47 deletions(-)
> ---
> base-commit: 94794b5ce4d90ab134b0b101a02fddf6e74c437d
> change-id: 20250123-v4h-iif-a1dda640c95d
> 
> Best regards,
> -- 
> Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 1/6] media: vsp1: Add support IIF ISP Interface

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:02PM +0100, Jacopo Mondi wrote:
> The IIF (ISP InterFace) is a specialized BRU version that transfers
> data to the ISP by reading from external memory through two RPF instances.

I'm not sure it's a BRU version, I think it's more of a separate entity
that happens to share the same DPR routing ID as the BRU (and therefore
the VI6_DPR_BRU_ROUTE register). I'd rewrite the commit message
accordingly.

> 
> Add support for it in the vsp1 driver by introducing a new entity
> type. The sole required operation is to enable the IIF function on the
> BRU during configure_stream().
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.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    | 134 ++++++++++++++++++++++
>  drivers/media/platform/renesas/vsp1/vsp1_iif.h    |  31 +++++
>  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   |   7 ++
>  10 files changed, 198 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/Makefile b/drivers/media/platform/renesas/vsp1/Makefile
> index 4bb4dcbef7b55be7a40231a04e14029911da9beb..e080377af01672451c3274118f74dbd132ec573f 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_lif.o vsp1_uif.o vsp1_iif.o

Alphabetical order please.

>  
>  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..cbaad0ea0b73f90f3994bbdfb4304d2f71eabccd 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"
> @@ -292,6 +293,16 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>  		list_add_tail(&vsp1->bru->entity.list_dev, &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);
> +	}
> +

Alphabetical order here too, move it after the HGT.

>  	if (vsp1_feature(vsp1, VSP1_HAS_CLU)) {
>  		vsp1->clu = vsp1_clu_create(vsp1);
>  		if (IS_ERR(vsp1->clu)) {
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> index 8b8945bd8f108354f1b484530bc496dbac7d3d88..343fc8f7fd6ef37a1c2e10ceaca2b7818cbd752d 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.
> +	 *
> +	 * For VSPX the IIF (a specialized BRU) is enabled with the extra
> +	 * IIFSEL bit.

I would also not mention here that the IIF is a specialized BRU, but use
a similar comment as above for ILV and BRS.

>  	 */
>  	if (source->type == VSP1_ENTITY_BRS)
>  		route |= VI6_DPR_ROUTE_BRSSEL;
> +	else if (source->type == VSP1_ENTITY_IIF)
> +		route |= VI6_DPR_BRU_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..ceb5e5988b02d86e9b444036ba70c7517d863850
> --- /dev/null
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.c
> @@ -0,0 +1,134 @@
> +// 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_brx.h"

This isn't needed.

> +#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_iif *iif,
> +				  struct vsp1_dl_body *dlb, u32 reg, u32 data)
> +{
> +	vsp1_dl_body_write(dlb, iif->base + reg, data);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * V4L2 Subdevice Operations
> + */
> +
> +static const unsigned int iif_codes[] = {
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SBGGR8_1X8,

Please sort the bayer patterns alphabetically here and below.

> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +	MEDIA_BUS_FMT_SGBRG12_1X12,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
> +	MEDIA_BUS_FMT_SRGGB16_1X16,
> +	MEDIA_BUS_FMT_SGRBG16_1X16,
> +	MEDIA_BUS_FMT_SGBRG16_1X16,
> +	MEDIA_BUS_FMT_SBGGR16_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)
> +{
> +	struct vsp1_iif *iif = to_iif(&entity->subdev);
> +
> +	vsp1_iif_write(iif, 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 is a specialized BRU instance, uses the same register base. */
> +	iif->base = VI6_BRU_BASE;

That seems really weird to me. VI6_BRU_BASE is defined as 0x2c00. You
use iif->base in vsp1_iif_write() as a register offset, when writing
VI6_IIF_CTRL. VI6_IIF_CTRL is defined as 0x0608, so the resulting
address is 0x3208, but the datasheet lists IIF_CTRL as being located at
0x0608. There's something to be investigated here, either the code is
wrong, or the datasheet is wrong.

While at it, if you believe the datasheet would be wrong, could you
inquire about different values being documented there from the register
(0x13 vs. 0x19) ? Getting answers may take time, so it's best to ask
early.

> +	iif->entity.ops = &iif_entity_ops;
> +	iif->entity.type = VSP1_ENTITY_IIF;
> +
> +	ret = vsp1_entity_init(vsp1, &iif->entity, "iif", 3, &iif_ops,
> +			       MEDIA_ENT_F_PROC_VIDEO_COMPOSER);

I'd use MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER as for the LIF as I think
both serve similar purposes. You could also copy the comment from there.

> +	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..2ec3b1e3aeaca943dab870ca997ff1a582ef8117
> --- /dev/null
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_iif.h
> @@ -0,0 +1,31 @@
> +/* 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/media-entity.h>
> +#include <media/v4l2-ctrls.h>

I think you can drop those two headers.

> +#include <media/v4l2-subdev.h>
> +
> +#include "vsp1_entity.h"
> +
> +#define IIF_PAD_SINK 0

This is never used.

> +
> +struct vsp1_iif {
> +	struct vsp1_entity entity;
> +	unsigned int base;

The reason why vsp1_brx has a base field is that a VSP can include
multiple BRU/BRS instances. There's only one IIF, so you can drop the
field.

> +};
> +
> +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..5d655785db85fe7cc7d7774fac168ca98891a0f5 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> @@ -252,6 +252,12 @@
>  #define VI6_RPF_BRDITH_CTRL_ODE		BIT(8)
>  #define VI6_RPF_BRDITH_CTRL_CBRM	BIT(0)
>  
> +/* -----------------------------------------------------------------------------
> + * IIF Control Registers
> + */

Missing blank line.

> +#define VI6_IIF_CTRL			0x0608
> +#define VI6_IIF_CTRL_CTRL		0x13
> +
>  /* -----------------------------------------------------------------------------
>   * WPF Control Registers
>   */
> @@ -388,6 +394,7 @@
>  #define VI6_DPR_HST_ROUTE		0x2044
>  #define VI6_DPR_HSI_ROUTE		0x2048
>  #define VI6_DPR_BRU_ROUTE		0x204c
> +#define VI6_DPR_BRU_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)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: vsp1: Enable FREE interrupt

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

s/FREE/FRE/ in the subject line, as FREE stands for "FRameEnd Enable",
so the interrupt is FRE.

On Thu, Jan 23, 2025 at 06:04:03PM +0100, Jacopo Mondi wrote:
> Enable the "FrameEnd" interrupt to support the VSPX operations.
> 
> The frame completion interrupt signals that the transfer of
> the config buffer and image buffer to the ISP has completed.
> 
> Enable the interrupt source if the pipe has an IIF entity, such as
> in the VSPX case.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_drv.c | 3 ++-
>  drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> index cbaad0ea0b73f90f3994bbdfb4304d2f71eabccd..5aa0751a896f8a58bd11128ccaa092c9596cdb5d 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> @@ -69,7 +69,8 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
>  				i, wpf->entity.pipe->underrun_count);
>  		}
>  
> -		if (status & VI6_WPF_IRQ_STA_DFE) {
> +		if (status & VI6_WPF_IRQ_STA_DFE ||
> +		    status & VI6_WPF_IRQ_STA_FRE) {
>  			vsp1_pipeline_frame_end(wpf->entity.pipe);
>  			ret = IRQ_HANDLED;
>  		}
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> index f176750ccd9847fdb8d51f7f51a6bd5092b70197..93a663f58a5930a3c7c40a96a30888d0b8ccb2ed 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> @@ -239,6 +239,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	const struct v4l2_mbus_framefmt *source_format;
>  	const struct v4l2_mbus_framefmt *sink_format;
>  	unsigned int index = wpf->entity.index;
> +	u32 irqmask = 0;

No need to initialize this to 0.

>  	unsigned int i;
>  	u32 outfmt = 0;
>  	u32 srcrpf = 0;
> @@ -312,9 +313,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
>  
>  	/* Enable interrupts. */
> +	irqmask = VI6_WPF_IRQ_ENB_DFEE | (pipe->iif ? VI6_WPF_IRQ_ENB_FREE : 0);

Do we need te DFE interrupt for VSPX ? If both DFE and FRE fire,
vsp1_pipeline_frame_end() will be called twice per frame, and that
doesn't sound like what you need. If DFE is not generated, I'd rather
not enable the interrupt, to be on the safe side.

Another option, which may be better, is to use the DFE interrupt. As far
as I understand, generation of the DFE interrupt is controlled by a bit
in the display list. Is there a reason you can't use that and need FRE ?

>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> -	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> -			   VI6_WPF_IRQ_ENB_DFEE);
> +	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irqmask);
>  
>  	/*
>  	 * Configure writeback for display pipelines (the wpf writeback flag is
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: vsp1: dl: Use singleshot DL for VSPX

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote:
> The vsp1_dl library allows to program a display list and feed it
> continuously to the VSP2. As an alternative operation mode, the library
> allows to program the VSP2 in 'single shot' mode, where a display list
> is submitted to the VSP on request only.
> 
> Currently the 'single shot' mode is only available when the VSP2 is
> controlled by userspace, while it works in continuous mode when driven
> by DRM, as frames have to be submitted to the display continuously.
> 
> For the VSPX use case, where there is no uapi support, we should however
> work in single-shot mode as the ISP driver programs the VSPX on
> request.
> 
> Initialize the display lists in single shot mode in case the VSP1 is
> controlled by userspace or in case the pipeline features an IIF entity,
> as in the VSPX case.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  		return NULL;
>  
>  	dlm->index = index;
> -	dlm->singleshot = vsp1->info->uapi;
> +	/*
> +	 * uapi = single shot mode;
> +	 * DRM = continuous mode;
> +	 * VSPX = single shot mode;
> +	 */
> +	dlm->singleshot = (vsp1->info->uapi || vsp1->iif);

I'm wondering if we should make this a bit more generic, and allow the
caller to select the mode of operation. It could be passed as a flag to
vsp1_dl_list_commit() and stored in the vsp1_dl_list.

There is however no use case at the moment to switch between singleshot
and continuous modes on a per display list basis. Implementing support
for that may be overkill, but on the other hand, the implementation
seems very simple, so it's not a big effort. The main and possibly only
reason why we may not want to do that now is because the display list
helpers haven't been tested to successfully switch between the modes, so
we may falsely advertise something as supported. Despite that, as no
caller would attempt switching, it wouldn't cause an issue.

What do you think ? What do you feel would be cleaner ?

>  	dlm->vsp1 = vsp1;
>  
>  	spin_lock_init(&dlm->lock);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: vsp1: rwpf: Break out format handling

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:05PM +0100, Jacopo Mondi wrote:
> The current implementation of the r/wpf format handling assumes
> three formats to be supported in the RGB/YUV space.
> 
> With the forthcoming support for VSPX the r/wpf units will be
> used to fetch from exteranal memory images in RAW Bayer format

s/exteranal/external/

> and buffers of ISP configuration parameters.
> 
> Prepare for adding support for these new formats by breaking
> out the list of supported media bus codes in the vsp1_rwpf.c
> file.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/renesas/vsp1/vsp1_rwpf.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> index 9d38203e73d00b82a1a7db0353e2f0b5a94084f6..93b0ed5fd0da0c6a182dbbfe1e54eb8cfd66c493 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> @@ -16,6 +16,12 @@
>  #define RWPF_MIN_WIDTH				1
>  #define RWPF_MIN_HEIGHT				1
>  
> +static const u32 rwpf_mbus_codes[] = {
> +	MEDIA_BUS_FMT_ARGB8888_1X32,
> +	MEDIA_BUS_FMT_AHSV8888_1X32,
> +	MEDIA_BUS_FMT_AYUV8_1X32,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdevice Operations
>   */
> @@ -24,16 +30,10 @@ static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev,
>  				    struct v4l2_subdev_state *sd_state,
>  				    struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	static const unsigned int codes[] = {
> -		MEDIA_BUS_FMT_ARGB8888_1X32,
> -		MEDIA_BUS_FMT_AHSV8888_1X32,
> -		MEDIA_BUS_FMT_AYUV8_1X32,
> -	};
> -
> -	if (code->index >= ARRAY_SIZE(codes))
> +	if (code->index >= ARRAY_SIZE(rwpf_mbus_codes))
>  		return -EINVAL;
>  
> -	code->code = codes[code->index];
> +	code->code = rwpf_mbus_codes[code->index];
>  
>  	return 0;
>  }
> @@ -57,6 +57,7 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  	struct vsp1_rwpf *rwpf = to_rwpf(subdev);
>  	struct v4l2_subdev_state *state;
>  	struct v4l2_mbus_framefmt *format;
> +	unsigned int i;
>  	int ret = 0;
>  
>  	mutex_lock(&rwpf->entity.lock);
> @@ -68,9 +69,11 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>  	}
>  
>  	/* Default to YUV if the requested format is not supported. */
> -	if (fmt->format.code != MEDIA_BUS_FMT_ARGB8888_1X32 &&
> -	    fmt->format.code != MEDIA_BUS_FMT_AHSV8888_1X32 &&
> -	    fmt->format.code != MEDIA_BUS_FMT_AYUV8_1X32)
> +	for (i = 0; i < ARRAY_SIZE(rwpf_mbus_codes); ++i) {
> +		if (fmt->format.code == rwpf_mbus_codes[i])
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(rwpf_mbus_codes))
>  		fmt->format.code = MEDIA_BUS_FMT_AYUV8_1X32;
>  
>  	format = v4l2_subdev_state_get_format(state, fmt->pad);
> 

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:06PM +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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> index 93b0ed5fd0da0c6a182dbbfe1e54eb8cfd66c493..75dde92ec35585825eb70c0faa31ff4cb1d4a3d4 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c
> @@ -20,6 +20,23 @@ static const u32 rwpf_mbus_codes[] = {
>  	MEDIA_BUS_FMT_ARGB8888_1X32,
>  	MEDIA_BUS_FMT_AHSV8888_1X32,
>  	MEDIA_BUS_FMT_AYUV8_1X32,
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SBGGR8_1X8,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +	MEDIA_BUS_FMT_SGBRG12_1X12,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
> +	MEDIA_BUS_FMT_SRGGB16_1X16,
> +	MEDIA_BUS_FMT_SGRBG16_1X16,
> +	MEDIA_BUS_FMT_SGBRG16_1X16,
> +	MEDIA_BUS_FMT_SBGGR16_1X16,
> +	MEDIA_BUS_FMT_METADATA_FIXED

Not all RPFs and WPFs should support those extra formats.

>  };
>  
>  /* -----------------------------------------------------------------------------
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: vsp1: rwpf: Support operations with IIF

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:07PM +0100, Jacopo Mondi wrote:
> When the RPF/WPF units are used for ISP interfacing through
> the IIF, the set of accessible registers is limited compared to
> the regular VSPD operations.
> 
> Support ISP interfacing in the rpf and wpf drivers by checking if
> the pipe features an IIF instance and writing only the relevant
> registers.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_rpf.c | 66 +++++++++++++++-----------
>  drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 18 +++++--
>  2 files changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> index 5c8b3ba1bd3c2c7b9289f05c9c2578e9717c23ff..e215491a3d962e2ae3c06b7835ca3b7654f04d10 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c
> @@ -60,6 +60,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  	const struct v4l2_mbus_framefmt *sink_format;
>  	unsigned int left = 0;
>  	unsigned int top = 0;
> +	u32 alpha_sel = 0;
>  	u32 pstride;
>  	u32 infmt;
>  
> @@ -84,7 +85,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  	sink_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
>  	source_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SOURCE);
>  
> -	infmt = VI6_RPF_INFMT_CIPM
> +	infmt = (pipe->iif ? 0 : VI6_RPF_INFMT_CIPM)
>  	      | (fmtinfo->hwfmt << VI6_RPF_INFMT_RDFMT_SHIFT);
>  
>  	if (fmtinfo->swap_yc)
> @@ -98,7 +99,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_rpf_write(rpf, dlb, VI6_RPF_INFMT, infmt);
>  	vsp1_rpf_write(rpf, dlb, VI6_RPF_DSWAP, fmtinfo->swap);
>  
> -	if (entity->vsp1->info->gen == 4) {
> +	if (entity->vsp1->info->gen == 4 && !pipe->iif) {
>  		u32 ext_infmt0;
>  		u32 ext_infmt1;
>  		u32 ext_infmt2;
> @@ -150,23 +151,6 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  		vsp1_rpf_write(rpf, dlb, VI6_RPF_EXT_INFMT2, ext_infmt2);
>  	}
>  
> -	/* Output location. */
> -	if (pipe->brx) {
> -		const struct v4l2_rect *compose;
> -
> -		compose = v4l2_subdev_state_get_compose(pipe->brx->state,
> -							rpf->brx_input);
> -		left = compose->left;
> -		top = compose->top;
> -	}
> -
> -	if (pipe->interlaced)
> -		top /= 2;
> -
> -	vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
> -		       (left << VI6_RPF_LOC_HCOORD_SHIFT) |
> -		       (top << VI6_RPF_LOC_VCOORD_SHIFT));
> -
>  	/*
>  	 * On Gen2 use the alpha channel (extended to 8 bits) when available or
>  	 * a fixed alpha value set through the V4L2_CID_ALPHA_COMPONENT control
> @@ -188,11 +172,35 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
>  	 * contains an alpha channel. On Gen2 the global alpha is ignored in
>  	 * that case.
>  	 *
> -	 * In all cases, disable color keying.
> +	 * In all the above cases, disable color keying.
> +	 *
> +	 * VSPX wants alpha_sel to be set to 0.
>  	 */
> -	vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, VI6_RPF_ALPH_SEL_AEXT_EXT |
> -		       (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
> -				       : VI6_RPF_ALPH_SEL_ASEL_FIXED));
> +	alpha_sel = pipe->iif ? 0
> +			      : VI6_RPF_ALPH_SEL_AEXT_EXT
> +				| (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
> +						  : VI6_RPF_ALPH_SEL_ASEL_FIXED);

	alpha_sel = pipe->iif ? 0
		  : VI6_RPF_ALPH_SEL_AEXT_EXT |
		    (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
				    : VI6_RPF_ALPH_SEL_ASEL_FIXED);

or

	if (!pipe->iif)
		alpha_sel |= VI6_RPF_ALPH_SEL_AEXT_EXT
			  |  (fmtinfo->alpha ? VI6_RPF_ALPH_SEL_ASEL_PACKED
					     : VI6_RPF_ALPH_SEL_ASEL_FIXED);

as alpha_sel is initialized to 0 when declared.

> +	vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, alpha_sel);
> +
> +	if (pipe->iif)
> +		return;

This becomes confusing, as you move the output location configuration in
the middle of the alpha handling block which is described by the big
comment above.

One option is to move the "output location" section after the whole
alpha handling (to the very end of this function). Another option is to
add

	if (pipe->iif) {
		vsp1_rpf_write(rpf, dlb, VI6_RPF_ALPH_SEL, 0);
		return;
	}

just above the "output location" section. That would avoid quite a few
changes from this patch.

> +
> +	/* Output location. */
> +	if (pipe->brx) {
> +		const struct v4l2_rect *compose;
> +
> +		compose = v4l2_subdev_state_get_compose(pipe->brx->state,
> +							rpf->brx_input);
> +		left = compose->left;
> +		top = compose->top;
> +	}
> +
> +	if (pipe->interlaced)
> +		top /= 2;
> +	vsp1_rpf_write(rpf, dlb, VI6_RPF_LOC,
> +		       (left << VI6_RPF_LOC_HCOORD_SHIFT) |
> +		       (top << VI6_RPF_LOC_VCOORD_SHIFT));
> +
>  
>  	if (entity->vsp1->info->gen >= 3) {
>  		u32 mult;
> @@ -338,11 +346,15 @@ static void rpf_configure_partition(struct vsp1_entity *entity,
>  	 */
>  	if (pipe->interlaced) {
>  		vsp1_rpf_configure_autofld(rpf, dl);
> -	} else {
> -		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_Y, mem.addr[0]);
> -		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C0, mem.addr[1]);
> -		vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C1, mem.addr[2]);
> +		return;
>  	}
> +
> +	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_Y, mem.addr[0]);
> +	if (!mem.addr[1])
> +		return;
> +
> +	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C0, mem.addr[1]);
> +	vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_ADDR_C1, mem.addr[2]);
>  }
>  
>  static void rpf_partition(struct vsp1_entity *entity,
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> index 93a663f58a5930a3c7c40a96a30888d0b8ccb2ed..736f76389e07a7cc28ba098a0a0bdf350a0794f7 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> @@ -248,8 +248,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	sink_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SINK);
>  	source_format = v4l2_subdev_state_get_format(state, RWPF_PAD_SOURCE);
>  
> -	/* Format */
> -	if (!pipe->lif || wpf->writeback) {
> +	/*
> +	 * Format configuration. Skip for IIF (VSPX) or if the pipe doesn't have
> +	 * an active output.

s/active output/active memory output/

or maybe better

"if the pipe doesn't write to memory."

> +	 */
> +	if (!pipe->iif && (!pipe->lif || wpf->writeback)) {
>  		const struct v4l2_pix_format_mplane *format = &wpf->format;
>  		const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
>  
> @@ -292,7 +295,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	 * Sources. If the pipeline has a single input and BRx is not used,
>  	 * configure it as the master layer. Otherwise configure all
>  	 * inputs as sub-layers and select the virtual RPF as the master
> -	 * layer.
> +	 * layer. For VSPX configure the enabled sources as masters.
>  	 */
>  	for (i = 0; i < vsp1->info->rpf_count; ++i) {
>  		struct vsp1_rwpf *input = pipe->inputs[i];
> @@ -300,7 +303,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  		if (!input)
>  			continue;
>  
> -		srcrpf |= (!pipe->brx && pipe->num_inputs == 1)
> +		/* For VSPX we enable and use RPF0 only for now. */
> +		if (pipe->iif && i > 0)
> +			break;

Is this needed ? I don't expect the pipeline to have more than one input
(for now) on the VSPX, so the continue statement above should take care
of this already.

> +
> +		srcrpf |= (pipe->iif || (!pipe->brx && pipe->num_inputs == 1))
>  			? VI6_WPF_SRCRPF_RPF_ACT_MST(input->entity.index)
>  			: VI6_WPF_SRCRPF_RPF_ACT_SUB(input->entity.index);
>  	}
> @@ -317,6 +324,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irqmask);
>  
> +	if (pipe->iif)
> +		return;
> +
>  	/*
>  	 * Configure writeback for display pipelines (the wpf writeback flag is
>  	 * never set for memory-to-memory pipelines). Start by adding a chained

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: vsp1: dl: Use singleshot DL for VSPX

[Jacopo Mondi]

Hi Laurent

On Thu, Jan 23, 2025 at 11:44:45PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote:
> > The vsp1_dl library allows to program a display list and feed it
> > continuously to the VSP2. As an alternative operation mode, the library
> > allows to program the VSP2 in 'single shot' mode, where a display list
> > is submitted to the VSP on request only.
> >
> > Currently the 'single shot' mode is only available when the VSP2 is
> > controlled by userspace, while it works in continuous mode when driven
> > by DRM, as frames have to be submitted to the display continuously.
> >
> > For the VSPX use case, where there is no uapi support, we should however
> > work in single-shot mode as the ISP driver programs the VSPX on
> > request.
> >
> > Initialize the display lists in single shot mode in case the VSP1 is
> > controlled by userspace or in case the pipeline features an IIF entity,
> > as in the VSPX case.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
> >  		return NULL;
> >
> >  	dlm->index = index;
> > -	dlm->singleshot = vsp1->info->uapi;
> > +	/*
> > +	 * uapi = single shot mode;
> > +	 * DRM = continuous mode;
> > +	 * VSPX = single shot mode;
> > +	 */
> > +	dlm->singleshot = (vsp1->info->uapi || vsp1->iif);
>
> I'm wondering if we should make this a bit more generic, and allow the
> caller to select the mode of operation. It could be passed as a flag to
> vsp1_dl_list_commit() and stored in the vsp1_dl_list.
>
> There is however no use case at the moment to switch between singleshot
> and continuous modes on a per display list basis. Implementing support
> for that may be overkill, but on the other hand, the implementation
> seems very simple, so it's not a big effort. The main and possibly only
> reason why we may not want to do that now is because the display list
> helpers haven't been tested to successfully switch between the modes, so
> we may falsely advertise something as supported. Despite that, as no
> caller would attempt switching, it wouldn't cause an issue.

I would simply avoid upstreaming code I can't test, and changing the
singleshot mode between commit might create contentions with
vsp1_dlm_irq_frame_end() which inspects dlm->singleshot.

>
> What do you think ? What do you feel would be cleaner ?
>
> >  	dlm->vsp1 = vsp1;
> >
> >  	spin_lock_init(&dlm->lock);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/6] media: vsp1: Enable FREE interrupt

[Jacopo Mondi]

Hi Laurent

On Thu, Jan 23, 2025 at 11:33:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> s/FREE/FRE/ in the subject line, as FREE stands for "FRameEnd Enable",
> so the interrupt is FRE.
>
> On Thu, Jan 23, 2025 at 06:04:03PM +0100, Jacopo Mondi wrote:
> > Enable the "FrameEnd" interrupt to support the VSPX operations.
> >
> > The frame completion interrupt signals that the transfer of
> > the config buffer and image buffer to the ISP has completed.
> >
> > Enable the interrupt source if the pipe has an IIF entity, such as
> > in the VSPX case.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/renesas/vsp1/vsp1_drv.c | 3 ++-
> >  drivers/media/platform/renesas/vsp1/vsp1_wpf.c | 5 +++--
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > index cbaad0ea0b73f90f3994bbdfb4304d2f71eabccd..5aa0751a896f8a58bd11128ccaa092c9596cdb5d 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > @@ -69,7 +69,8 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
> >  				i, wpf->entity.pipe->underrun_count);
> >  		}
> >
> > -		if (status & VI6_WPF_IRQ_STA_DFE) {
> > +		if (status & VI6_WPF_IRQ_STA_DFE ||
> > +		    status & VI6_WPF_IRQ_STA_FRE) {
> >  			vsp1_pipeline_frame_end(wpf->entity.pipe);
> >  			ret = IRQ_HANDLED;
> >  		}
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > index f176750ccd9847fdb8d51f7f51a6bd5092b70197..93a663f58a5930a3c7c40a96a30888d0b8ccb2ed 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c
> > @@ -239,6 +239,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  	const struct v4l2_mbus_framefmt *source_format;
> >  	const struct v4l2_mbus_framefmt *sink_format;
> >  	unsigned int index = wpf->entity.index;
> > +	u32 irqmask = 0;
>
> No need to initialize this to 0.
>
> >  	unsigned int i;
> >  	u32 outfmt = 0;
> >  	u32 srcrpf = 0;
> > @@ -312,9 +313,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
> >
> >  	/* Enable interrupts. */
> > +	irqmask = VI6_WPF_IRQ_ENB_DFEE | (pipe->iif ? VI6_WPF_IRQ_ENB_FREE : 0);
>
> Do we need te DFE interrupt for VSPX ? If both DFE and FRE fire,
> vsp1_pipeline_frame_end() will be called twice per frame, and that
> doesn't sound like what you need. If DFE is not generated, I'd rather
> not enable the interrupt, to be on the safe side.
>
> Another option, which may be better, is to use the DFE interrupt. As far
> as I understand, generation of the DFE interrupt is controlled by a bit
> in the display list. Is there a reason you can't use that and need FRE ?

Documentation mentions the FRE interrupt only for VSPX.

In example:

6. Waiting complete of one frame processing
ISP core and VSPX asserts end of frame interrupts. To notify complete
of one frame processing, wait both interrupts from ISP core and VSPX
(FRE interrupt).

in the ISP documentation and

Use frame end interrupt (VI6_WPF0_IRQ_ENB.FRE) for VSPX.

in the VSPX chapter.

I concur for VSPX I don't need DFE though. Actually, I currently get
two interrupts per transfer, I thought one was for the config DMA
buffer and one for image buffer, but it might actually be one DFE and
one FRE. I'll investigate.


>
> >  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> > -	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> > -			   VI6_WPF_IRQ_ENB_DFEE);
> > +	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index), irqmask);
> >
> >  	/*
> >  	 * Configure writeback for display pipelines (the wpf writeback flag is
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 3/6] media: vsp1: dl: Use singleshot DL for VSPX

[Laurent Pinchart]

On Fri, Jan 24, 2025 at 09:44:06AM +0100, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Thu, Jan 23, 2025 at 11:44:45PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote:
> > > The vsp1_dl library allows to program a display list and feed it
> > > continuously to the VSP2. As an alternative operation mode, the library
> > > allows to program the VSP2 in 'single shot' mode, where a display list
> > > is submitted to the VSP on request only.
> > >
> > > Currently the 'single shot' mode is only available when the VSP2 is
> > > controlled by userspace, while it works in continuous mode when driven
> > > by DRM, as frames have to be submitted to the display continuously.
> > >
> > > For the VSPX use case, where there is no uapi support, we should however
> > > work in single-shot mode as the ISP driver programs the VSPX on
> > > request.
> > >
> > > Initialize the display lists in single shot mode in case the VSP1 is
> > > controlled by userspace or in case the pipeline features an IIF entity,
> > > as in the VSPX case.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > > index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644
> > > --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> > > @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
> > >  		return NULL;
> > >
> > >  	dlm->index = index;
> > > -	dlm->singleshot = vsp1->info->uapi;
> > > +	/*
> > > +	 * uapi = single shot mode;
> > > +	 * DRM = continuous mode;
> > > +	 * VSPX = single shot mode;
> > > +	 */
> > > +	dlm->singleshot = (vsp1->info->uapi || vsp1->iif);
> >
> > I'm wondering if we should make this a bit more generic, and allow the
> > caller to select the mode of operation. It could be passed as a flag to
> > vsp1_dl_list_commit() and stored in the vsp1_dl_list.
> >
> > There is however no use case at the moment to switch between singleshot
> > and continuous modes on a per display list basis. Implementing support
> > for that may be overkill, but on the other hand, the implementation
> > seems very simple, so it's not a big effort. The main and possibly only
> > reason why we may not want to do that now is because the display list
> > helpers haven't been tested to successfully switch between the modes, so
> > we may falsely advertise something as supported. Despite that, as no
> > caller would attempt switching, it wouldn't cause an issue.
> 
> I would simply avoid upstreaming code I can't test, and changing the
> singleshot mode between commit might create contentions with
> vsp1_dlm_irq_frame_end() which inspects dlm->singleshot.

I had considered that when looking at the code. Moving the single shot
flag to the vsp1_dl_list, vsp1_dlm_irq_frame_end() would check the flag
from that structure instead of getting it from the dlm, so I don't think
it would be an issue. That's the reason why I'm in two minds about this,
I think it would be easy to do, with very low risk for our use cases,
but indeed the switch itself wouldn't be fully tested.

> > What do you think ? What do you feel would be cleaner ?
> >
> > >  	dlm->vsp1 = vsp1;
> > >
> > >  	spin_lock_init(&dlm->lock);

-- 
Regards,

Laurent Pinchart