[PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Anatoliy Klymenko]

Patches 1/4,2/4,3/4 are minor fixes.

DPSUB requires input live video format to be configured.
Patch 4/4: The DP Subsystem requires the input live video format to be
configured. In this patch we are assuming that the CRTC's bus format is fixed
and comes from the device tree. This is a proposed solution, as there are no api
to query CRTC output bus format.

Is this a good approach to go with?

Anatoliy Klymenko (4):
  drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable
  drm: xlnx: zynqmp_dpsub: Fix timing for live mode
  drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
  drm: xlnx: zynqmp_dpsub: Set live video in format

 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 111 +++++++++++++++++++++---
 drivers/gpu/drm/xlnx/zynqmp_disp.h      |   3 +-
 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |   8 +-
 drivers/gpu/drm/xlnx/zynqmp_dp.c        |  14 ++-
 drivers/gpu/drm/xlnx/zynqmp_kms.c       |   2 +-
 5 files changed, 118 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable

[Anatoliy Klymenko]

Assign device of node to bridge prior registering it. This will
make said bridge discoverable by separate crtc driver.

Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..d60b7431603f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
 		    | DRM_BRIDGE_OP_HPD;
 	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
+	bridge->of_node = dp->dev->of_node;
 	dpsub->bridge = bridge;
 
 	/*
-- 
2.25.1

Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable

[Tomi Valkeinen]

On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> Assign device of node to bridge prior registering it. This will
> make said bridge discoverable by separate crtc driver.

I think a few words on why this is needed (and why it wasn't needed 
before) would be nice.

Other than that:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..d60b7431603f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
>   		    | DRM_BRIDGE_OP_HPD;
>   	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> +	bridge->of_node = dp->dev->of_node;
>   	dpsub->bridge = bridge;
>   
>   	/*

Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable

[Laurent Pinchart]

On Wed, Jan 17, 2024 at 04:06:31PM +0200, Tomi Valkeinen wrote:
> On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> > Assign device of node to bridge prior registering it. This will
> > make said bridge discoverable by separate crtc driver.
> 
> I think a few words on why this is needed (and why it wasn't needed 
> before) would be nice.
> 
> Other than that:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Agreed. With a better commit message,

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

> > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> > ---
> >   drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index a0606fab0e22..d60b7431603f 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> >   	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
> >   		    | DRM_BRIDGE_OP_HPD;
> >   	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> > +	bridge->of_node = dp->dev->of_node;
> >   	dpsub->bridge = bridge;
> >   
> >   	/*

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] drm: xlnx: zynqmp_dpsub: Fix timing for live mode

[Anatoliy Klymenko]

Expect external video timing in live video input mode, program
DPSUB accordingly.

Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
---
 drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..8a39b3accce5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -1166,7 +1166,7 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
 	/* Choose clock source based on the DT clock handle. */
 	zynqmp_disp_avbuf_set_clocks_sources(disp, disp->dpsub->vid_clk_from_ps,
 					     disp->dpsub->aud_clk_from_ps,
-					     true);
+					     disp->dpsub->vid_clk_from_ps);
 	zynqmp_disp_avbuf_enable_channels(disp);
 	zynqmp_disp_avbuf_enable_audio(disp);
 
-- 
2.25.1

Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Fix timing for live mode

[Tomi Valkeinen]

On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> Expect external video timing in live video input mode, program
> DPSUB accordingly.
> 
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 407bc07cec69..8a39b3accce5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -1166,7 +1166,7 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
>   	/* Choose clock source based on the DT clock handle. */
>   	zynqmp_disp_avbuf_set_clocks_sources(disp, disp->dpsub->vid_clk_from_ps,
>   					     disp->dpsub->aud_clk_from_ps,
> -					     true);
> +					     disp->dpsub->vid_clk_from_ps);
>   	zynqmp_disp_avbuf_enable_channels(disp);
>   	zynqmp_disp_avbuf_enable_audio(disp);
>   

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

[PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode

[Anatoliy Klymenko]

Filter out status register against interrupts' mask.
Some events are being reported via DP status register, even if
corresponding interrupts have been disabled. Avoid processing
of such events in interrupt handler context.

Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index d60b7431603f..571c5dbc97e5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	u32 status, mask;
 
 	status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS);
+	zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
 	mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK);
-	if (!(status & ~mask))
+
+	/*
+	 * Status register may report some events, which corresponding interrupts
+	 * have been disabled. Filter out those events against interrupts' mask.
+	 */
+	status &= ~mask;
+
+	if (!status)
 		return IRQ_NONE;
 
 	/* dbg for diagnostic, but not much that the driver can do */
@@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK)
 		dev_dbg_ratelimited(dp->dev, "overflow interrupt\n");
 
-	zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
 
 	if (status & ZYNQMP_DP_INT_VBLANK_START)
 		zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
-- 
2.25.1

Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode

[Tomi Valkeinen]

On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> Filter out status register against interrupts' mask.
> Some events are being reported via DP status register, even if
> corresponding interrupts have been disabled. Avoid processing
> of such events in interrupt handler context.

The subject talks about vblank and live mode, the the description 
doesn't. Can you elaborate in the desc a bit about when this is an issue 
and why it wasn't before?

> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index d60b7431603f..571c5dbc97e5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>   	u32 status, mask;
>   
>   	status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS);
> +	zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
>   	mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK);
> -	if (!(status & ~mask))
> +
> +	/*
> +	 * Status register may report some events, which corresponding interrupts
> +	 * have been disabled. Filter out those events against interrupts' mask.
> +	 */
> +	status &= ~mask;
> +
> +	if (!status)
>   		return IRQ_NONE;
>   
>   	/* dbg for diagnostic, but not much that the driver can do */
> @@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
>   	if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK)
>   		dev_dbg_ratelimited(dp->dev, "overflow interrupt\n");
>   
> -	zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
>   
>   	if (status & ZYNQMP_DP_INT_VBLANK_START)
>   		zynqmp_dpsub_drm_handle_vblank(dp->dpsub);

Moving the zynqmp_dp_write() is not related to this fix, is it? I think 
it should be in a separate patch.

  Tomi

[PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format

[Anatoliy Klymenko]

Live video input format is expected to be set as
"bus-format" property in connected remote endpoint.
Program live video input format DPSUB registers.
Set display layer mode in layer creation context.

Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
---
 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 109 ++++++++++++++++++++++--
 drivers/gpu/drm/xlnx/zynqmp_disp.h      |   3 +-
 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |   8 +-
 drivers/gpu/drm/xlnx/zynqmp_dp.c        |   2 +-
 drivers/gpu/drm/xlnx/zynqmp_kms.c       |   2 +-
 5 files changed, 107 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..83af3ad9cdb5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -20,8 +20,10 @@
 #include <linux/dmaengine.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/media-bus-format.h>
 
 #include "zynqmp_disp.h"
 #include "zynqmp_disp_regs.h"
@@ -67,12 +69,16 @@
 /**
  * struct zynqmp_disp_format - Display subsystem format information
  * @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Live video media bus format
  * @buf_fmt: AV buffer format
  * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
  * @sf: Scaling factors for color components
  */
 struct zynqmp_disp_format {
-	u32 drm_fmt;
+	union {
+		u32 drm_fmt;
+		u32 bus_fmt;
+	};
 	u32 buf_fmt;
 	bool swap;
 	const u32 *sf;
@@ -354,6 +360,16 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
 	},
 };
 
+/* TODO: add support for different formats */
+static const struct zynqmp_disp_format avbuf_live_vid_fmts[] = {
+	{
+		.bus_fmt	= MEDIA_BUS_FMT_UYVY8_1X16,
+		.buf_fmt	= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+				  ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+		.sf		= scaling_factors_888,
+	}
+};
+
 static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
 {
 	return readl(disp->avbuf.base + reg);
@@ -369,6 +385,34 @@ static bool zynqmp_disp_layer_is_video(const struct zynqmp_disp_layer *layer)
 	return layer->id == ZYNQMP_DPSUB_LAYER_VID;
 }
 
+/**
+ * zynqmp_disp_avbuf_set_live_format - Set live input format for a layer
+ * @disp: Display controller
+ * @layer: The layer
+ * @fmt: The format information
+ *
+ * Set the live video input format for @layer to @fmt.
+ */
+static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
+					      struct zynqmp_disp_layer *layer,
+					      const struct zynqmp_disp_format *fmt)
+{
+	u32 reg, i;
+
+	reg = zynqmp_disp_layer_is_video(layer)
+	    ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+	    : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+	zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
+
+	for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
+		reg = zynqmp_disp_layer_is_video(layer)
+		    ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
+		    : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
+		zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
+	}
+	layer->disp_fmt = fmt;
+}
+
 /**
  * zynqmp_disp_avbuf_set_format - Set the input format for a layer
  * @disp: Display controller
@@ -902,15 +946,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
 /**
  * zynqmp_disp_layer_enable - Enable a layer
  * @layer: The layer
- * @mode: Operating mode of layer
  *
  * Enable the @layer in the audio/video buffer manager and the blender. DMA
  * channels are started separately by zynqmp_disp_layer_update().
  */
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
-			      enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
 {
-	layer->mode = mode;
 	zynqmp_disp_avbuf_enable_video(layer->disp, layer);
 	zynqmp_disp_blend_layer_enable(layer->disp, layer);
 }
@@ -950,11 +991,12 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
 	layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
 	layer->drm_fmt = info;
 
-	zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
-
-	if (!layer->disp->dpsub->dma_enabled)
+	/* Live format set during layer creation */
+	if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
 		return;
 
+	zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
+
 	/*
 	 * Set pconfig for each DMA channel to indicate they're part of a
 	 * video group.
@@ -1083,7 +1125,7 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
 	unsigned int i;
 	int ret;
 
-	if (!disp->dpsub->dma_enabled)
+	if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
 		return 0;
 
 	for (i = 0; i < layer->info->num_channels; i++) {
@@ -1104,6 +1146,43 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
 	return 0;
 }
 
+/**
+ * zynqmp_disp_get_live_fmt - Get live video format
+ * @disp: Display controller
+ * @layer: Display layer
+ *
+ * Parse connected remote endpoint and retrieve configured bus-format
+ *
+ * Return: live format pointer on success, NULL otherwise
+ */
+static const struct zynqmp_disp_format *zynqmp_disp_get_live_fmt(struct zynqmp_disp *disp,
+								 struct zynqmp_disp_layer *layer)
+{
+	struct device_node *local, *remote, *dpsub = disp->dev->of_node;
+	int rc, i;
+	u32 fmt;
+
+	local = of_graph_get_endpoint_by_regs(dpsub, layer->id, -1);
+	if (!local)
+		return NULL;
+
+	remote = of_graph_get_remote_endpoint(local);
+	of_node_put(local);
+	if (!remote)
+		return NULL;
+
+	rc = of_property_read_u32_index(remote, "bus-format", 0, &fmt);
+	of_node_put(remote);
+	if (rc)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(avbuf_live_vid_fmts); ++i)
+		if (avbuf_live_vid_fmts[i].bus_fmt == fmt)
+			return &avbuf_live_vid_fmts[i];
+
+	return NULL;
+}
+
 /**
  * zynqmp_disp_create_layers - Create and initialize all layers
  * @disp: Display controller
@@ -1130,9 +1209,15 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
 
 	for (i = 0; i < ARRAY_SIZE(disp->layers); i++) {
 		struct zynqmp_disp_layer *layer = &disp->layers[i];
+		const struct zynqmp_disp_format *disp_fmt;
 
 		layer->id = i;
 		layer->disp = disp;
+		/* For now we assume dpsub works in either live or non-live mode for both layers.
+		 * Hybrid mode is not supported yet.
+		 */
+		layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE
+						       : ZYNQMP_DPSUB_LAYER_LIVE;
 		layer->info = &layer_info[i];
 
 		ret = zynqmp_disp_layer_request_dma(disp, layer);
@@ -1140,6 +1225,12 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
 			goto err;
 
 		disp->dpsub->layers[i] = layer;
+
+		if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) {
+			disp_fmt = zynqmp_disp_get_live_fmt(disp, layer);
+			if (disp_fmt)
+				zynqmp_disp_avbuf_set_live_format(disp, layer, disp_fmt);
+		}
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..f3357b2d5c09 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -62,8 +62,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
 
 u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
 				   unsigned int *num_formats);
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
-			      enum zynqmp_dpsub_layer_mode mode);
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
 void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
 void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
 				  const struct drm_format_info *info);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
index f92a006d5070..926e07c255bb 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
@@ -165,10 +165,10 @@
 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10		0x2
 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12		0x3
 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK		GENMASK(2, 0)
-#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB		0x0
-#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444	0x1
-#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422	0x2
-#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY	0x3
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB		0x00
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444	0x10
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422	0x20
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY	0x30
 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK		GENMASK(5, 4)
 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST		BIT(8)
 #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY		0x400
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 571c5dbc97e5..59616ed1c3d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
 	/* TODO: Make the format configurable. */
 	info = drm_format_info(DRM_FORMAT_YUV422);
 	zynqmp_disp_layer_set_format(layer, info);
-	zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
+	zynqmp_disp_layer_enable(layer);
 
 	if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
 		zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index db3bb4afbfc4..43bf416b33d5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -122,7 +122,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
 
 	/* Enable or re-enable the plane if the format has changed. */
 	if (format_changed)
-		zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_NONLIVE);
+		zynqmp_disp_layer_enable(layer);
 }
 
 static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
-- 
2.25.1

Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format

[Tomi Valkeinen]

Hi Anatoliy,

On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> Live video input format is expected to be set as
> "bus-format" property in connected remote endpoint.
> Program live video input format DPSUB registers.
> Set display layer mode in layer creation context.

Some comments inline below. But one thing to improve is the commit desc.

I think this needs more explanation on what's the issue here. So 
basically something like what's the feature in question, why it's not 
working or what's missing, and what does this patch do to get it working.

And while often it's reasonable to expect some level of understanding of 
the HW in question, it doesn't hurt to give some clarifications on the 
names used (here the "live video input").

> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_disp.c      | 109 ++++++++++++++++++++++--
>   drivers/gpu/drm/xlnx/zynqmp_disp.h      |   3 +-
>   drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |   8 +-
>   drivers/gpu/drm/xlnx/zynqmp_dp.c        |   2 +-
>   drivers/gpu/drm/xlnx/zynqmp_kms.c       |   2 +-
>   5 files changed, 107 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..83af3ad9cdb5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -20,8 +20,10 @@
>   #include <linux/dmaengine.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_graph.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
> +#include <linux/media-bus-format.h>

Alphabetical order, please.

>   #include "zynqmp_disp.h"
>   #include "zynqmp_disp_regs.h"
> @@ -67,12 +69,16 @@
>   /**
>    * struct zynqmp_disp_format - Display subsystem format information
>    * @drm_fmt: DRM format (4CC)
> + * @bus_fmt: Live video media bus format
>    * @buf_fmt: AV buffer format
>    * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
>    * @sf: Scaling factors for color components
>    */
>   struct zynqmp_disp_format {
> -	u32 drm_fmt;
> +	union {
> +		u32 drm_fmt;
> +		u32 bus_fmt;
> +	};
>   	u32 buf_fmt;
>   	bool swap;
>   	const u32 *sf;
> @@ -354,6 +360,16 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
>   	},
>   };
>   
> +/* TODO: add support for different formats */
> +static const struct zynqmp_disp_format avbuf_live_vid_fmts[] = {
> +	{
> +		.bus_fmt	= MEDIA_BUS_FMT_UYVY8_1X16,
> +		.buf_fmt	= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> +				  ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> +		.sf		= scaling_factors_888,
> +	}
> +};
> +
>   static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
>   {
>   	return readl(disp->avbuf.base + reg);
> @@ -369,6 +385,34 @@ static bool zynqmp_disp_layer_is_video(const struct zynqmp_disp_layer *layer)
>   	return layer->id == ZYNQMP_DPSUB_LAYER_VID;
>   }
>   
> +/**
> + * zynqmp_disp_avbuf_set_live_format - Set live input format for a layer
> + * @disp: Display controller
> + * @layer: The layer
> + * @fmt: The format information
> + *
> + * Set the live video input format for @layer to @fmt.
> + */
> +static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
> +					      struct zynqmp_disp_layer *layer,
> +					      const struct zynqmp_disp_format *fmt)
> +{
> +	u32 reg, i;
> +
> +	reg = zynqmp_disp_layer_is_video(layer)
> +	    ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> +	    : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> +	zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
> +
> +	for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
> +		reg = zynqmp_disp_layer_is_video(layer)
> +		    ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
> +		    : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
> +		zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
> +	}
> +	layer->disp_fmt = fmt;
> +}
> +
>   /**
>    * zynqmp_disp_avbuf_set_format - Set the input format for a layer
>    * @disp: Display controller
> @@ -902,15 +946,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
>   /**
>    * zynqmp_disp_layer_enable - Enable a layer
>    * @layer: The layer
> - * @mode: Operating mode of layer
>    *
>    * Enable the @layer in the audio/video buffer manager and the blender. DMA
>    * channels are started separately by zynqmp_disp_layer_update().
>    */
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> -			      enum zynqmp_dpsub_layer_mode mode)
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>   {
> -	layer->mode = mode;
>   	zynqmp_disp_avbuf_enable_video(layer->disp, layer);
>   	zynqmp_disp_blend_layer_enable(layer->disp, layer);
>   }
> @@ -950,11 +991,12 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
>   	layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
>   	layer->drm_fmt = info;
>   
> -	zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
> -
> -	if (!layer->disp->dpsub->dma_enabled)
> +	/* Live format set during layer creation */
> +	if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
>   		return;
>   
> +	zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
> +
>   	/*
>   	 * Set pconfig for each DMA channel to indicate they're part of a
>   	 * video group.
> @@ -1083,7 +1125,7 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
>   	unsigned int i;
>   	int ret;
>   
> -	if (!disp->dpsub->dma_enabled)
> +	if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
>   		return 0;
>   
>   	for (i = 0; i < layer->info->num_channels; i++) {
> @@ -1104,6 +1146,43 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
>   	return 0;
>   }
>   
> +/**
> + * zynqmp_disp_get_live_fmt - Get live video format
> + * @disp: Display controller
> + * @layer: Display layer
> + *
> + * Parse connected remote endpoint and retrieve configured bus-format
> + *
> + * Return: live format pointer on success, NULL otherwise
> + */
> +static const struct zynqmp_disp_format *zynqmp_disp_get_live_fmt(struct zynqmp_disp *disp,
> +								 struct zynqmp_disp_layer *layer)
> +{
> +	struct device_node *local, *remote, *dpsub = disp->dev->of_node;
> +	int rc, i;
> +	u32 fmt;
> +
> +	local = of_graph_get_endpoint_by_regs(dpsub, layer->id, -1);
> +	if (!local)
> +		return NULL;
> +
> +	remote = of_graph_get_remote_endpoint(local);

The "remote" here is some PL component that provides the live stream? 
I'm not sure if there's a rule for these, but I think usually a driver 
should only read its own properties. I would add 'bus-format' to dp 
endpoint's DT data.

> +	of_node_put(local);
> +	if (!remote)
> +		return NULL;
> +
> +	rc = of_property_read_u32_index(remote, "bus-format", 0, &fmt);

Does this require a change to the DT bindings?

Why is this not of_property_read_u32()?

> +	of_node_put(remote);
> +	if (rc)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(avbuf_live_vid_fmts); ++i)
> +		if (avbuf_live_vid_fmts[i].bus_fmt == fmt)
> +			return &avbuf_live_vid_fmts[i];
> +
> +	return NULL;
> +}
> +
>   /**
>    * zynqmp_disp_create_layers - Create and initialize all layers
>    * @disp: Display controller
> @@ -1130,9 +1209,15 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
>   
>   	for (i = 0; i < ARRAY_SIZE(disp->layers); i++) {
>   		struct zynqmp_disp_layer *layer = &disp->layers[i];
> +		const struct zynqmp_disp_format *disp_fmt;
>   
>   		layer->id = i;
>   		layer->disp = disp;
> +		/* For now we assume dpsub works in either live or non-live mode for both layers.
> +		 * Hybrid mode is not supported yet.
> +		 */
> +		layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE
> +						       : ZYNQMP_DPSUB_LAYER_LIVE;
>   		layer->info = &layer_info[i];
>   
>   		ret = zynqmp_disp_layer_request_dma(disp, layer);
> @@ -1140,6 +1225,12 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
>   			goto err;
>   
>   		disp->dpsub->layers[i] = layer;
> +
> +		if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) {
> +			disp_fmt = zynqmp_disp_get_live_fmt(disp, layer);
> +			if (disp_fmt)
> +				zynqmp_disp_avbuf_set_live_format(disp, layer, disp_fmt);
> +		}
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..f3357b2d5c09 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -62,8 +62,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
>   
>   u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
>   				   unsigned int *num_formats);
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> -			      enum zynqmp_dpsub_layer_mode mode);
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
>   void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
>   void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
>   				  const struct drm_format_info *info);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..926e07c255bb 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -165,10 +165,10 @@
>   #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10		0x2
>   #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12		0x3
>   #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK		GENMASK(2, 0)
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB		0x0
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444	0x1
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422	0x2
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY	0x3
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB		0x00
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444	0x10
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422	0x20
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY	0x30

What's this about? Were these wrong before? Sounds like a separate patch 
needed for these.

>   #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK		GENMASK(5, 4)
>   #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST		BIT(8)
>   #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY		0x400
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 571c5dbc97e5..59616ed1c3d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
>   	/* TODO: Make the format configurable. */
>   	info = drm_format_info(DRM_FORMAT_YUV422);
>   	zynqmp_disp_layer_set_format(layer, info);
> -	zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
> +	zynqmp_disp_layer_enable(layer);
>   
>   	if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
>   		zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index db3bb4afbfc4..43bf416b33d5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -122,7 +122,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
>   
>   	/* Enable or re-enable the plane if the format has changed. */
>   	if (format_changed)
> -		zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_NONLIVE);
> +		zynqmp_disp_layer_enable(layer);
>   }
>   
>   static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {

Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> Patches 1/4,2/4,3/4 are minor fixes.
> 
> DPSUB requires input live video format to be configured.
> Patch 4/4: The DP Subsystem requires the input live video format to be
> configured. In this patch we are assuming that the CRTC's bus format is fixed
> and comes from the device tree. This is a proposed solution, as there are no api
> to query CRTC output bus format.
> 
> Is this a good approach to go with?

I guess you would need to expand a bit on what "live video input" is? Is
it some kind of mechanism to bypass memory and take your pixels straight
from a FIFO from another device, or something else?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Laurent Pinchart]

On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > Patches 1/4,2/4,3/4 are minor fixes.
> > 
> > DPSUB requires input live video format to be configured.
> > Patch 4/4: The DP Subsystem requires the input live video format to be
> > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > and comes from the device tree. This is a proposed solution, as there are no api
> > to query CRTC output bus format.
> > 
> > Is this a good approach to go with?
> 
> I guess you would need to expand a bit on what "live video input" is? Is
> it some kind of mechanism to bypass memory and take your pixels straight
> from a FIFO from another device, or something else?

Yes and no.

The DPSUB integrates DMA engines, a blending engine (two planes), and a
DP encoder. The dpsub driver supports all of this, and creates a DRM
device. The DP encoder hardware always takes its input data from the
output of the blending engine.

The blending engine can optionally take input data from a bus connected
to the FPGA fabric, instead of taking it from the DPSUB internal DMA
engines. When operating in that mode, the dpsub driver exposes the DP
encoder as a bridge, and internally programs the blending engine to
disable blending. Typically, the FPGA fabric will then contain a CRTC of
some sort, with a driver that will acquire the DP encoder bridge as
usually done.

In this mode of operation, it is typical for the IP cores in FPGA fabric
to be synthesized with a fixed format (as that saves resources), while
the DPSUB supports multiple input formats. Bridge drivers in the
upstream kernel work the other way around, with the bridge hardware
supporting a limited set of formats, and the CRTC then being programmed
with whatever the bridges chain needs. Here, the negotiation needs to go
the other way around, as the CRTC is the limiting factor, not the
bridge.

Is this explanation clear ?

-- 
Regards,

Laurent Pinchart

Re: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > Patches 1/4,2/4,3/4 are minor fixes.
> > > 
> > > DPSUB requires input live video format to be configured.
> > > Patch 4/4: The DP Subsystem requires the input live video format to be
> > > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > > and comes from the device tree. This is a proposed solution, as there are no api
> > > to query CRTC output bus format.
> > > 
> > > Is this a good approach to go with?
> > 
> > I guess you would need to expand a bit on what "live video input" is? Is
> > it some kind of mechanism to bypass memory and take your pixels straight
> > from a FIFO from another device, or something else?
> 
> Yes and no.
> 
> The DPSUB integrates DMA engines, a blending engine (two planes), and a
> DP encoder. The dpsub driver supports all of this, and creates a DRM
> device. The DP encoder hardware always takes its input data from the
> output of the blending engine.
> 
> The blending engine can optionally take input data from a bus connected
> to the FPGA fabric, instead of taking it from the DPSUB internal DMA
> engines. When operating in that mode, the dpsub driver exposes the DP
> encoder as a bridge, and internally programs the blending engine to
> disable blending. Typically, the FPGA fabric will then contain a CRTC of
> some sort, with a driver that will acquire the DP encoder bridge as
> usually done.
> 
> In this mode of operation, it is typical for the IP cores in FPGA fabric
> to be synthesized with a fixed format (as that saves resources), while
> the DPSUB supports multiple input formats.

Where is that CRTC driver? It's not clear to me why the format would
need to be in the device tree at all. Format negociation between the
CRTC and whatever comes next is already done in a number of drivers so
it would be useful to have that kind of API outside of the bridge
support.

> Bridge drivers in the upstream kernel work the other way around, with
> the bridge hardware supporting a limited set of formats, and the CRTC
> then being programmed with whatever the bridges chain needs. Here, the
> negotiation needs to go the other way around, as the CRTC is the
> limiting factor, not the bridge.

Sounds like there's something to rework in the API then?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Klymenko, Anatoliy]

> -----Original Message-----
> From: Maxime Ripard <mripard@kernel.org>
> Sent: Friday, January 26, 2024 4:26 AM
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>;
> maarten.lankhorst@linux.intel.com; tzimmermann@suse.de; airlied@gmail.com;
> daniel@ffwll.ch; Simek, Michal <michal.simek@amd.com>; dri-
> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
> 
> On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > > Patches 1/4,2/4,3/4 are minor fixes.
> > > >
> > > > DPSUB requires input live video format to be configured.
> > > > Patch 4/4: The DP Subsystem requires the input live video format to be
> > > > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > > > and comes from the device tree. This is a proposed solution, as there are no
> api
> > > > to query CRTC output bus format.
> > > >
> > > > Is this a good approach to go with?
> > >
> > > I guess you would need to expand a bit on what "live video input" is? Is
> > > it some kind of mechanism to bypass memory and take your pixels straight
> > > from a FIFO from another device, or something else?
> >
> > Yes and no.
> >
> > The DPSUB integrates DMA engines, a blending engine (two planes), and a
> > DP encoder. The dpsub driver supports all of this, and creates a DRM
> > device. The DP encoder hardware always takes its input data from the
> > output of the blending engine.
> >
> > The blending engine can optionally take input data from a bus connected
> > to the FPGA fabric, instead of taking it from the DPSUB internal DMA
> > engines. When operating in that mode, the dpsub driver exposes the DP
> > encoder as a bridge, and internally programs the blending engine to
> > disable blending. Typically, the FPGA fabric will then contain a CRTC of
> > some sort, with a driver that will acquire the DP encoder bridge as
> > usually done.
> >
> > In this mode of operation, it is typical for the IP cores in FPGA fabric
> > to be synthesized with a fixed format (as that saves resources), while
> > the DPSUB supports multiple input formats.
> 
> Where is that CRTC driver? It's not clear to me why the format would
> need to be in the device tree at all. Format negociation between the
> CRTC and whatever comes next is already done in a number of drivers so
> it would be useful to have that kind of API outside of the bridge
> support.
> 
One example of such CRTC driver: https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c
It's not upstreamed yet. Bus format negotiations here are handled by utilizing Xilinx-specific bridge framework. Ideally, it would be nice to rework this to comply with the upstream DRM bridge framework.

> > Bridge drivers in the upstream kernel work the other way around, with
> > the bridge hardware supporting a limited set of formats, and the CRTC
> > then being programmed with whatever the bridges chain needs. Here, the
> > negotiation needs to go the other way around, as the CRTC is the
> > limiting factor, not the bridge.
> 
> Sounds like there's something to rework in the API then?
> 
Adding an optional CRTC callback imposing CRTC specific bus format restrictions, which may be called from here https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935 would solve the problem.

> Maxime

--

Regards,
Anatoliy

Re: RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4090 bytes --]

On Fri, Jan 26, 2024 at 11:18:30PM +0000, Klymenko, Anatoliy wrote:
> 
> 
> > -----Original Message-----
> > From: Maxime Ripard <mripard@kernel.org>
> > Sent: Friday, January 26, 2024 4:26 AM
> > To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>;
> > maarten.lankhorst@linux.intel.com; tzimmermann@suse.de; airlied@gmail.com;
> > daniel@ffwll.ch; Simek, Michal <michal.simek@amd.com>; dri-
> > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
> > 
> > On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote:
> > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > > > Patches 1/4,2/4,3/4 are minor fixes.
> > > > >
> > > > > DPSUB requires input live video format to be configured.
> > > > > Patch 4/4: The DP Subsystem requires the input live video format to be
> > > > > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > > > > and comes from the device tree. This is a proposed solution, as there are no
> > api
> > > > > to query CRTC output bus format.
> > > > >
> > > > > Is this a good approach to go with?
> > > >
> > > > I guess you would need to expand a bit on what "live video input" is? Is
> > > > it some kind of mechanism to bypass memory and take your pixels straight
> > > > from a FIFO from another device, or something else?
> > >
> > > Yes and no.
> > >
> > > The DPSUB integrates DMA engines, a blending engine (two planes), and a
> > > DP encoder. The dpsub driver supports all of this, and creates a DRM
> > > device. The DP encoder hardware always takes its input data from the
> > > output of the blending engine.
> > >
> > > The blending engine can optionally take input data from a bus connected
> > > to the FPGA fabric, instead of taking it from the DPSUB internal DMA
> > > engines. When operating in that mode, the dpsub driver exposes the DP
> > > encoder as a bridge, and internally programs the blending engine to
> > > disable blending. Typically, the FPGA fabric will then contain a CRTC of
> > > some sort, with a driver that will acquire the DP encoder bridge as
> > > usually done.
> > >
> > > In this mode of operation, it is typical for the IP cores in FPGA fabric
> > > to be synthesized with a fixed format (as that saves resources), while
> > > the DPSUB supports multiple input formats.
> > 
> > Where is that CRTC driver? It's not clear to me why the format would
> > need to be in the device tree at all. Format negociation between the
> > CRTC and whatever comes next is already done in a number of drivers so
> > it would be useful to have that kind of API outside of the bridge
> > support.
>
> One example of such CRTC driver:
> https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c It's not
> upstreamed yet. Bus format negotiations here are handled by utilizing Xilinx-specific bridge
> framework. Ideally, it would be nice to rework this to comply with the upstream DRM bridge
> framework.
>
> > > Bridge drivers in the upstream kernel work the other way around, with
> > > the bridge hardware supporting a limited set of formats, and the CRTC
> > > then being programmed with whatever the bridges chain needs. Here, the
> > > negotiation needs to go the other way around, as the CRTC is the
> > > limiting factor, not the bridge.
> > 
> > Sounds like there's something to rework in the API then?
> > 
> Adding an optional CRTC callback imposing CRTC specific bus format restrictions, which may be
> called from here https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935
> would solve the problem.

CRTCs and bridges are orthogonal. If anything, I'd expect that callback
to be set at the CRTC, encoder and connector levels and filled by the
drm_bridge code if relevant.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Laurent Pinchart]

On Thu, Feb 01, 2024 at 06:01:01PM +0100, Maxime Ripard wrote:
> On Fri, Jan 26, 2024 at 11:18:30PM +0000, Klymenko, Anatoliy wrote:
> > On Friday, January 26, 2024 4:26 AM, Maxime Ripard wrote:
> > > On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > > > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > > > > Patches 1/4,2/4,3/4 are minor fixes.
> > > > > >
> > > > > > DPSUB requires input live video format to be configured.
> > > > > > Patch 4/4: The DP Subsystem requires the input live video format to be
> > > > > > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > > > > > and comes from the device tree. This is a proposed solution, as there are no api
> > > > > > to query CRTC output bus format.
> > > > > >
> > > > > > Is this a good approach to go with?
> > > > >
> > > > > I guess you would need to expand a bit on what "live video input" is? Is
> > > > > it some kind of mechanism to bypass memory and take your pixels straight
> > > > > from a FIFO from another device, or something else?
> > > >
> > > > Yes and no.
> > > >
> > > > The DPSUB integrates DMA engines, a blending engine (two planes), and a
> > > > DP encoder. The dpsub driver supports all of this, and creates a DRM
> > > > device. The DP encoder hardware always takes its input data from the
> > > > output of the blending engine.
> > > >
> > > > The blending engine can optionally take input data from a bus connected
> > > > to the FPGA fabric, instead of taking it from the DPSUB internal DMA
> > > > engines. When operating in that mode, the dpsub driver exposes the DP
> > > > encoder as a bridge, and internally programs the blending engine to
> > > > disable blending. Typically, the FPGA fabric will then contain a CRTC of
> > > > some sort, with a driver that will acquire the DP encoder bridge as
> > > > usually done.
> > > >
> > > > In this mode of operation, it is typical for the IP cores in FPGA fabric
> > > > to be synthesized with a fixed format (as that saves resources), while
> > > > the DPSUB supports multiple input formats.
> > > 
> > > Where is that CRTC driver? It's not clear to me why the format would
> > > need to be in the device tree at all. Format negociation between the
> > > CRTC and whatever comes next is already done in a number of drivers so
> > > it would be useful to have that kind of API outside of the bridge
> > > support.
> >
> > One example of such CRTC driver:
> > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c It's not
> > upstreamed yet. Bus format negotiations here are handled by utilizing Xilinx-specific bridge
> > framework. Ideally, it would be nice to rework this to comply with the upstream DRM bridge
> > framework.
> >
> > > > Bridge drivers in the upstream kernel work the other way around, with
> > > > the bridge hardware supporting a limited set of formats, and the CRTC
> > > > then being programmed with whatever the bridges chain needs. Here, the
> > > > negotiation needs to go the other way around, as the CRTC is the
> > > > limiting factor, not the bridge.
> > > 
> > > Sounds like there's something to rework in the API then?
> > > 
> > Adding an optional CRTC callback imposing CRTC specific bus format restrictions, which may be
> > called from here https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935
> > would solve the problem.
> 
> CRTCs and bridges are orthogonal. If anything, I'd expect that callback
> to be set at the CRTC, encoder and connector levels and filled by the
> drm_bridge code if relevant.

I'm thinking about a new CRTC operation that would be called by the
bridge chain format negotiation helper
drm_atomic_bridge_chain_select_bus_fmts() (or one of the functions it
calls), to filter the list of formats supported by the chain based on
what the CRTC supports, or possibly to pick a format in that list. This
needs to be prototyped

-- 
Regards,

Laurent Pinchart

Re: Re: RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4416 bytes --]

On Sun, Feb 04, 2024 at 11:56:18AM +0200, Laurent Pinchart wrote:
> On Thu, Feb 01, 2024 at 06:01:01PM +0100, Maxime Ripard wrote:
> > On Fri, Jan 26, 2024 at 11:18:30PM +0000, Klymenko, Anatoliy wrote:
> > > On Friday, January 26, 2024 4:26 AM, Maxime Ripard wrote:
> > > > On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > > > > > Patches 1/4,2/4,3/4 are minor fixes.
> > > > > > >
> > > > > > > DPSUB requires input live video format to be configured.
> > > > > > > Patch 4/4: The DP Subsystem requires the input live video format to be
> > > > > > > configured. In this patch we are assuming that the CRTC's bus format is fixed
> > > > > > > and comes from the device tree. This is a proposed solution, as there are no api
> > > > > > > to query CRTC output bus format.
> > > > > > >
> > > > > > > Is this a good approach to go with?
> > > > > >
> > > > > > I guess you would need to expand a bit on what "live video input" is? Is
> > > > > > it some kind of mechanism to bypass memory and take your pixels straight
> > > > > > from a FIFO from another device, or something else?
> > > > >
> > > > > Yes and no.
> > > > >
> > > > > The DPSUB integrates DMA engines, a blending engine (two planes), and a
> > > > > DP encoder. The dpsub driver supports all of this, and creates a DRM
> > > > > device. The DP encoder hardware always takes its input data from the
> > > > > output of the blending engine.
> > > > >
> > > > > The blending engine can optionally take input data from a bus connected
> > > > > to the FPGA fabric, instead of taking it from the DPSUB internal DMA
> > > > > engines. When operating in that mode, the dpsub driver exposes the DP
> > > > > encoder as a bridge, and internally programs the blending engine to
> > > > > disable blending. Typically, the FPGA fabric will then contain a CRTC of
> > > > > some sort, with a driver that will acquire the DP encoder bridge as
> > > > > usually done.
> > > > >
> > > > > In this mode of operation, it is typical for the IP cores in FPGA fabric
> > > > > to be synthesized with a fixed format (as that saves resources), while
> > > > > the DPSUB supports multiple input formats.
> > > > 
> > > > Where is that CRTC driver? It's not clear to me why the format would
> > > > need to be in the device tree at all. Format negociation between the
> > > > CRTC and whatever comes next is already done in a number of drivers so
> > > > it would be useful to have that kind of API outside of the bridge
> > > > support.
> > >
> > > One example of such CRTC driver:
> > > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c It's not
> > > upstreamed yet. Bus format negotiations here are handled by utilizing Xilinx-specific bridge
> > > framework. Ideally, it would be nice to rework this to comply with the upstream DRM bridge
> > > framework.
> > >
> > > > > Bridge drivers in the upstream kernel work the other way around, with
> > > > > the bridge hardware supporting a limited set of formats, and the CRTC
> > > > > then being programmed with whatever the bridges chain needs. Here, the
> > > > > negotiation needs to go the other way around, as the CRTC is the
> > > > > limiting factor, not the bridge.
> > > > 
> > > > Sounds like there's something to rework in the API then?
> > > > 
> > > Adding an optional CRTC callback imposing CRTC specific bus format restrictions, which may be
> > > called from here https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935
> > > would solve the problem.
> > 
> > CRTCs and bridges are orthogonal. If anything, I'd expect that callback
> > to be set at the CRTC, encoder and connector levels and filled by the
> > drm_bridge code if relevant.
> 
> I'm thinking about a new CRTC operation that would be called by the
> bridge chain format negotiation helper
> drm_atomic_bridge_chain_select_bus_fmts() (or one of the functions it
> calls), to filter the list of formats supported by the chain based on
> what the CRTC supports, or possibly to pick a format in that list. This
> needs to be prototyped

As long as we come up with something that works for regular encoders,
I'm fine with that.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB

[Klymenko, Anatoliy]

Hi Laurent and Maxime,

Laurent, thank you very much for clear and comprehensive description of the "live video input" feature.

Maxime, sure, I will elaborate more in the next version of cover letter.

> Date: Wed, 17 Jan 2024 16:23:43 +0200
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Maxime Ripard <mripard@kernel.org>
> Cc: Anatoliy Klymenko <anatoliy.klymenko@amd.com>,
>         maarten.lankhorst@linux.intel.com, tzimmermann@suse.de,
>         airlied@gmail.com, daniel@ffwll.ch, michal.simek@amd.com,
>         dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org,
>         linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
> Message-ID: <20240117142343.GD17920@pendragon.ideasonboard.com>
> Content-Type: text/plain; charset=utf-8
> 
> On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote:
> > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote:
> > > Patches 1/4,2/4,3/4 are minor fixes.
> > >
> > > DPSUB requires input live video format to be configured.
> > > Patch 4/4: The DP Subsystem requires the input live video format to
> > > be configured. In this patch we are assuming that the CRTC's bus
> > > format is fixed and comes from the device tree. This is a proposed
> > > solution, as there are no api to query CRTC output bus format.
> > >
> > > Is this a good approach to go with?
> >
> > I guess you would need to expand a bit on what "live video input" is?
> > Is it some kind of mechanism to bypass memory and take your pixels
> > straight from a FIFO from another device, or something else?
> 
> Yes and no.
> 
> The DPSUB integrates DMA engines, a blending engine (two planes), and a DP
> encoder. The dpsub driver supports all of this, and creates a DRM device. The DP
> encoder hardware always takes its input data from the output of the blending
> engine.
> 
> The blending engine can optionally take input data from a bus connected to the
> FPGA fabric, instead of taking it from the DPSUB internal DMA engines. When
> operating in that mode, the dpsub driver exposes the DP encoder as a bridge, and
> internally programs the blending engine to disable blending. Typically, the FPGA
> fabric will then contain a CRTC of some sort, with a driver that will acquire the DP
> encoder bridge as usually done.
> 
> In this mode of operation, it is typical for the IP cores in FPGA fabric to be
> synthesized with a fixed format (as that saves resources), while the DPSUB
> supports multiple input formats. Bridge drivers in the upstream kernel work the
> other way around, with the bridge hardware supporting a limited set of formats,
> and the CRTC then being programmed with whatever the bridges chain needs.
> Here, the negotiation needs to go the other way around, as the CRTC is the
> limiting factor, not the bridge.
> 
> Is this explanation clear ?
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 

Thank you,
Anatoliy