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
Assign device of node to bridge prior registering it. This will
make said bridge discoverable by separate crtc driver.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
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
Expect external video timing in live video input mode, program
DPSUB accordingly.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
Tomi
> Signed-off-by: Anatoliy Klymenko <[email protected]>
> ---
> 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;
>
> /*
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 <[email protected]>
> ---
> 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 <[email protected]>
Tomi
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
Agreed. With a better commit message,
Reviewed-by: Laurent Pinchart <[email protected]>
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
> 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 = {
Hi Laurent,
Thank you for the reply.
> Date: Wed, 17 Jan 2024 16:24:20 +0200
> From: Laurent Pinchart <[email protected]>
> To: Tomi Valkeinen <[email protected]>
> Cc: Anatoliy Klymenko <[email protected]>,
> [email protected], [email protected],
> [email protected], [email protected], [email protected],
> [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge
> discoverable
> Message-ID: <[email protected]>
> Content-Type: text/plain; charset=utf-8
>
> 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 <[email protected]>
>
> Agreed. With a better commit message,
>
Sure, I will update commit message in the next version.
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> > > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > > ---
> > > 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
>
>
Thank you,
Anatoliy
Hi Anatoliy,
On Fri, Jan 19, 2024 at 05:53:30AM +0000, Klymenko, Anatoliy wrote:
> On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote:
> > 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?
>
> Yes, I should make patch comment more consistent and elaborate. I will fix it in the next version. Thank you.
>
> >
> > > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > > ---
> > > 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.
>
> The sole purpose of zynqmp_dp_write() is to clear status register. The
> sooner we do it the better (after reading status in the local variable
> of course).
No disagreement about that. Tomi's point is that it's a good change, but
it should be done in a separate patch, by itself, not bundled with other
changes. The usual rule in the kernel is "one change, one commit".
> We can also reuse status variable for in-place filtering
> against the interrupt mask, which makes this change dependent on
> zynqmp_dp_write() reordering. I will add a comment explaining this. Is
> it ok?
--
Regards,
Laurent Pinchart
Hi Laurent,
> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Friday, January 19, 2024 4:30 AM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Simek, Michal <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live
> mode
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> On Fri, Jan 19, 2024 at 05:53:30AM +0000, Klymenko, Anatoliy wrote:
> > On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote:
> > > 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?
> >
> > Yes, I should make patch comment more consistent and elaborate. I will fix it in
> the next version. Thank you.
> >
> > >
> > > > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > > > ---
> > > > 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.
> >
> > The sole purpose of zynqmp_dp_write() is to clear status register. The
> > sooner we do it the better (after reading status in the local variable
> > of course).
>
> No disagreement about that. Tomi's point is that it's a good change, but it should
> be done in a separate patch, by itself, not bundled with other changes. The usual
> rule in the kernel is "one change, one commit".
>
Sure, I will move this into a separate commit in the next version. Thank you.
> > We can also reuse status variable for in-place filtering against the
> > interrupt mask, which makes this change dependent on
> > zynqmp_dp_write() reordering. I will add a comment explaining this. Is
> > it ok?
>
> --
> Regards,
>
> Laurent Pinchart
Thank you,
Anatoliy
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
> -----Original Message-----
> From: Maxime Ripard <[email protected]>
> Sent: Friday, January 26, 2024 4:26 AM
> To: Laurent Pinchart <[email protected]>
> Cc: Klymenko, Anatoliy <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>; dri-
> [email protected]; [email protected]; linux-
> [email protected]
> 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
On 19/01/2024 07:54, Klymenko, Anatoliy wrote:
>>> 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.
>>
>
> It is an embedded bit shift that corresponds to DPSUB live video / gfx format register layout. Original values are technically correct but would require extra bit shifts to operate with. The current patch is the first instance of actual use of those defines. Do you think it's worth to factor those changes out into a separate patch?
The value for the defines should then be something like (0x3 << 4), to
make it clearer that it's shifted to the right position.
Tomi
On Fri, Jan 26, 2024 at 11:18:30PM +0000, Klymenko, Anatoliy wrote:
>
>
> > -----Original Message-----
> > From: Maxime Ripard <[email protected]>
> > Sent: Friday, January 26, 2024 4:26 AM
> > To: Laurent Pinchart <[email protected]>
> > Cc: Klymenko, Anatoliy <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Simek, Michal <[email protected]>; dri-
> > [email protected]; [email protected]; linux-
> > [email protected]
> > 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
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
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