Implement live video input format setting for ZynqMP DPSUB.
ZynqMP DPSUB can operate in 2 modes: DMA-based and live.
In the live mode, DPSUB receives a live video signal from FPGA-based CRTC.
DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune
into the incoming video signal, DPSUB should be programmed with the proper
media bus format. This patch series addresses this task.
Patch 1/4: Set the DPSUB layer mode of operation prior to enabling the
layer. Allows to use layer operational mode before its enablement.
Patch 2/4: Announce supported input media bus formats via
drm_bridge_funcs.atomic_get_input_bus_fmts callback.
Patch 3/4: Program DPSUB live video input format based on selected bus
config in the new atomic bridge state.
Patch 4/4: New optional CRTC atomic helper proposal that will allow CRTC
to participate in DRM bridge chain format negotiation and impose format
restrictions. Incorporate this callback into the DRM bridge format
negotiation process.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
Anatoliy Klymenko (4):
drm: xlnx: zynqmp_dpsub: Set layer mode during creation
drm: xlnx: zynqmp_dpsub: Anounce supported input formats
drm: xlnx: zynqmp_dpsub: Set input live format
drm/atomic-helper: Add select_output_bus_format callback
drivers/gpu/drm/drm_bridge.c | 19 +++++-
drivers/gpu/drm/xlnx/zynqmp_disp.c | 109 +++++++++++++++++++++++++++++--
drivers/gpu/drm/xlnx/zynqmp_disp.h | 25 +++----
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +--
drivers/gpu/drm/xlnx/zynqmp_dp.c | 16 +++--
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
include/drm/drm_modeset_helper_vtables.h | 31 +++++++++
7 files changed, 182 insertions(+), 28 deletions(-)
---
base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670
change-id: 20240226-dp-live-fmt-6415773b5a68
Best regards,
--
Anatoliy Klymenko <[email protected]>
Set layer mode of operation (live or dma-based) during layer creation.
Each DPSUB layer mode of operation is defined by corresponding DT node port
connection, so it is possible to assign it during layer object creation.
Previously it was set in layer enable functions, although it is too late
as setting layer format depends on layer mode, and should be done before
given layer enabled.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 ++++++++++++++++----
drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +------------
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..e6d26ef60e89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -64,6 +64,16 @@
#define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
+/**
+ * enum zynqmp_dpsub_layer_mode - Layer mode
+ * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
+ */
+enum zynqmp_dpsub_layer_mode {
+ ZYNQMP_DPSUB_LAYER_NONLIVE,
+ ZYNQMP_DPSUB_LAYER_LIVE,
+};
+
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
@@ -902,15 +912,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);
}
@@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
layer->id = i;
layer->disp = disp;
layer->info = &layer_info[i];
+ /* For now 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;
ret = zynqmp_disp_layer_request_dma(disp, layer);
if (ret)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..9b8b202224d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
ZYNQMP_DPSUB_LAYER_GFX,
};
-/**
- * enum zynqmp_dpsub_layer_mode - Layer mode
- * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
- * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
- */
-enum zynqmp_dpsub_layer_mode {
- ZYNQMP_DPSUB_LAYER_NONLIVE,
- ZYNQMP_DPSUB_LAYER_LIVE,
-};
-
void zynqmp_disp_enable(struct zynqmp_disp *disp);
void zynqmp_disp_disable(struct zynqmp_disp *disp);
int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
@@ -62,8 +52,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_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 1846c4971fd8..04b6bcac3b07 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
DPSUB in bridge mode supports multiple input media bus formats.
Announce the list of supported input media bus formats via
drm_bridge.atomic_get_input_bus_fmts callback.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 +++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++
drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
3 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index e6d26ef60e89..ee99aad915ba 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -18,6 +18,7 @@
#include <linux/dma/xilinx_dpdma.h>
#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
+#include <linux/media-bus-format.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
@@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: 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;
+ u32 bus_fmt;
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -364,6 +367,40 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* List of live video layer formats */
+static const struct zynqmp_disp_format avbuf_live_fmts[] = {
+ {
+ .drm_fmt = DRM_FORMAT_VYUY,
+ .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16,
+ .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ },
+};
+
+u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts)
+{
+ int i;
+ u32 *input_fmts;
+
+ input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts) {
+ *num_input_fmts = 0;
+ return input_fmts;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i)
+ input_fmts[i] = avbuf_live_fmts[i].bus_fmt;
+ *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts);
+
+ return input_fmts;
+}
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 9b8b202224d9..c2c8dd4896ba 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -27,6 +27,10 @@
struct device;
struct drm_format_info;
struct drm_plane_state;
+struct drm_bridge;
+struct drm_bridge_state;
+struct drm_connector_state;
+struct drm_crtc_state;
struct platform_device;
struct zynqmp_disp;
struct zynqmp_disp_layer;
@@ -52,6 +56,12 @@ 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);
+u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts);
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,
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 04b6bcac3b07..9cb7ac9f3097 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1580,6 +1580,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
.atomic_check = zynqmp_dp_bridge_atomic_check,
.detect = zynqmp_dp_bridge_detect,
.edid_read = zynqmp_dp_bridge_edid_read,
+ .atomic_get_input_bus_fmts = zynqmp_disp_get_input_bus_fmts,
};
/* -----------------------------------------------------------------------------
--
2.25.1
Program live video input format according to selected media bus format.
In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
almost certainly supports a single media bus format as its output. Expect
this to be delivered via the new bridge atomic state. Program DPSUB
registers accordingly.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 52 +++++++++++++++++++++++++++++++++
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 ++
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++---
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++++---
4 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index ee99aad915ba..1c3ffdee6b8e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -416,6 +416,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
@@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
zynqmp_disp_blend_layer_disable(layer->disp, layer);
}
+/**
+ * zynqmp_disp_layer_set_live_format - Set live layer input format
+ * @layer: The layer
+ * @info: Input media bus format
+ *
+ * Set the live @layer input bus format. The layer must be disabled.
+ */
+void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
+ u32 bus_format)
+{
+ int i;
+ const struct zynqmp_disp_format *fmt;
+
+ for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
+ fmt = &avbuf_live_fmts[i];
+ if (fmt->bus_fmt == bus_format) {
+ layer->disp_fmt = fmt;
+ layer->drm_fmt = drm_format_info(fmt->drm_fmt);
+ zynqmp_disp_avbuf_set_live_format(layer->disp, layer, fmt);
+ return;
+ }
+ }
+}
+
/**
* zynqmp_disp_layer_set_format - Set the layer format
* @layer: The layer
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index c2c8dd4896ba..f244b7d2346a 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -66,6 +66,8 @@ 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);
+void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
+ u32 bus_format);
int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
struct drm_plane_state *state);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
index f92a006d5070..fa3935384834 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 (0x0 << 4)
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4)
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4)
+#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4)
#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 9cb7ac9f3097..0d5dffd20ad1 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
{
enum zynqmp_dpsub_layer_id layer_id;
struct zynqmp_disp_layer *layer;
- const struct drm_format_info *info;
+ struct drm_bridge_state *bridge_state;
+ u32 bus_fmt;
if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
layer_id = ZYNQMP_DPSUB_LAYER_VID;
@@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
return;
layer = dp->dpsub->layers[layer_id];
+ bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
+ old_bridge_state->bridge);
+ if (bridge_state) {
+ bus_fmt = bridge_state->input_bus_cfg.format;
+ zynqmp_disp_layer_set_live_format(layer, bus_fmt);
+ } else
+ return;
- /* TODO: Make the format configurable. */
- info = drm_format_info(DRM_FORMAT_YUV422);
- zynqmp_disp_layer_set_format(layer, info);
zynqmp_disp_layer_enable(layer);
if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
--
2.25.1
Add select_output_bus_format to CRTC atomic helpers callbacks. This
callback Will allow CRTC to participate in media bus format negotiation
over connected DRM bridge chain and impose CRTC-specific restrictions.
A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will
most certainly support a single output media bus format, as supporting
multiple runtime options consumes extra FPGA resources. A variety of
options for FPGA are usually achieved by synthesizing IP with different
parameters.
Incorporate select_output_bus_format callback into the format negotiation
stage to fix the input bus format of the first DRM bridge in the chain.
Signed-off-by: Anatoliy Klymenko <[email protected]>
---
drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++--
include/drm/drm_modeset_helper_vtables.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 521a71c61b16..453ae3d174b4 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -32,6 +32,7 @@
#include <drm/drm_edid.h>
#include <drm/drm_encoder.h>
#include <drm/drm_file.h>
+#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_of.h>
#include <drm/drm_print.h>
@@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
unsigned int i, num_in_bus_fmts = 0;
struct drm_bridge_state *cur_state;
struct drm_bridge *prev_bridge;
- u32 *in_bus_fmts;
+ struct drm_crtc *crtc = crtc_state->crtc;
+ u32 *in_bus_fmts, in_fmt;
int ret;
prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
@@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
return -ENOMEM;
if (first_bridge == cur_bridge) {
- cur_state->input_bus_cfg.format = in_bus_fmts[0];
+ in_fmt = in_bus_fmts[0];
+ if (crtc->helper_private &&
+ crtc->helper_private->select_output_bus_format) {
+ in_fmt = crtc->helper_private->select_output_bus_format(
+ crtc,
+ crtc->state,
+ in_bus_fmts,
+ num_in_bus_fmts);
+ if (!in_fmt) {
+ kfree(in_bus_fmts);
+ return -ENOTSUPP;
+ }
+ }
+ cur_state->input_bus_cfg.format = in_fmt;
cur_state->output_bus_cfg.format = out_bus_fmt;
kfree(in_bus_fmts);
return 0;
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 881b03e4dc28..7c21ae1fe3ad 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
bool in_vblank_irq, int *vpos, int *hpos,
ktime_t *stime, ktime_t *etime,
const struct drm_display_mode *mode);
+
+ /**
+ * @select_output_bus_format
+ *
+ * Called by the first connected DRM bridge to negotiate input media
+ * bus format. CRTC is expected to pick preferable media formats from
+ * the list supported by the DRM bridge chain.
+ *
+ * This callback is optional.
+ *
+ * Parameters:
+ *
+ * crtc:
+ * The CRTC.
+ * crcs_state:
+ * New CRTC state.
+ * supported_fmts:
+ * List of input bus formats supported by the bridge.
+ * num_supported_fmts:
+ * Number of formats in the list.
+ *
+ * Returns:
+ *
+ * Preferred bus format from the list or 0 if CRTC doesn't support any
+ * from the provided list.
+ *
+ */
+ u32 (*select_output_bus_format)(struct drm_crtc *crtc,
+ struct drm_crtc_state *crtc_state,
+ const u32 *supported_fmts,
+ int num_supported_fmts);
};
/**
--
2.25.1
Hi Anatoliy,
Thank you for the patch.
On Mon, Feb 26, 2024 at 08:44:42PM -0800, Anatoliy Klymenko wrote:
> Set layer mode of operation (live or dma-based) during layer creation.
>
> Each DPSUB layer mode of operation is defined by corresponding DT node port
> connection, so it is possible to assign it during layer object creation.
> Previously it was set in layer enable functions, although it is too late
> as setting layer format depends on layer mode, and should be done before
> given layer enabled.
>
> Signed-off-by: Anatoliy Klymenko <[email protected]>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 ++++++++++++++++----
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +------------
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..e6d26ef60e89 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -64,6 +64,16 @@
>
> #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
>
> +/**
> + * enum zynqmp_dpsub_layer_mode - Layer mode
> + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> + */
> +enum zynqmp_dpsub_layer_mode {
> + ZYNQMP_DPSUB_LAYER_NONLIVE,
> + ZYNQMP_DPSUB_LAYER_LIVE,
> +};
> +
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> @@ -902,15 +912,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);
> }
> @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp)
> layer->id = i;
> layer->disp = disp;
> layer->info = &layer_info[i];
> + /* For now assume dpsub works in either live or non-live mode for both layers.
> + * Hybrid mode is not supported yet.
> + */
/*
* For now assume dpsub works in either live or non-live mode
* for both layers. Hybrid mode is not supported yet.
*/
Sounds like a reasonable restriction for now.
Reviewed-by: Laurent Pinchart <[email protected]>
> + layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE
> + : ZYNQMP_DPSUB_LAYER_LIVE;
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..9b8b202224d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> ZYNQMP_DPSUB_LAYER_GFX,
> };
>
> -/**
> - * enum zynqmp_dpsub_layer_mode - Layer mode
> - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> - */
> -enum zynqmp_dpsub_layer_mode {
> - ZYNQMP_DPSUB_LAYER_NONLIVE,
> - ZYNQMP_DPSUB_LAYER_LIVE,
> -};
> -
> void zynqmp_disp_enable(struct zynqmp_disp *disp);
> void zynqmp_disp_disable(struct zynqmp_disp *disp);
> int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> @@ -62,8 +52,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_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..04b6bcac3b07 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 = {
>
--
Regards,
Laurent Pinchart
Hi,
On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> Add select_output_bus_format to CRTC atomic helpers callbacks. This
> callback Will allow CRTC to participate in media bus format negotiation
> over connected DRM bridge chain and impose CRTC-specific restrictions.
> A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will
> most certainly support a single output media bus format, as supporting
> multiple runtime options consumes extra FPGA resources. A variety of
> options for FPGA are usually achieved by synthesizing IP with different
> parameters.
>
> Incorporate select_output_bus_format callback into the format negotiation
> stage to fix the input bus format of the first DRM bridge in the chain.
>
> Signed-off-by: Anatoliy Klymenko <[email protected]>
> ---
> drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++--
> include/drm/drm_modeset_helper_vtables.h | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 521a71c61b16..453ae3d174b4 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -32,6 +32,7 @@
> #include <drm/drm_edid.h>
> #include <drm/drm_encoder.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> #include <drm/drm_of.h>
> #include <drm/drm_print.h>
>
> @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> unsigned int i, num_in_bus_fmts = 0;
> struct drm_bridge_state *cur_state;
> struct drm_bridge *prev_bridge;
> - u32 *in_bus_fmts;
> + struct drm_crtc *crtc = crtc_state->crtc;
> + u32 *in_bus_fmts, in_fmt;
> int ret;
>
> prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> return -ENOMEM;
>
> if (first_bridge == cur_bridge) {
> - cur_state->input_bus_cfg.format = in_bus_fmts[0];
> + in_fmt = in_bus_fmts[0];
> + if (crtc->helper_private &&
> + crtc->helper_private->select_output_bus_format) {
> + in_fmt = crtc->helper_private->select_output_bus_format(
> + crtc,
> + crtc->state,
> + in_bus_fmts,
> + num_in_bus_fmts);
> + if (!in_fmt) {
> + kfree(in_bus_fmts);
> + return -ENOTSUPP;
> + }
> + }
> + cur_state->input_bus_cfg.format = in_fmt;
I don't think we should start poking at the CRTC internals, but we
should rather provide a helper here.
> cur_state->output_bus_cfg.format = out_bus_fmt;
> kfree(in_bus_fmts);
> return 0;
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 881b03e4dc28..7c21ae1fe3ad 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> bool in_vblank_irq, int *vpos, int *hpos,
> ktime_t *stime, ktime_t *etime,
> const struct drm_display_mode *mode);
> +
> + /**
> + * @select_output_bus_format
> + *
> + * Called by the first connected DRM bridge to negotiate input media
> + * bus format. CRTC is expected to pick preferable media formats from
> + * the list supported by the DRM bridge chain.
There's nothing restricting it to bridges here. Please rephrase this to
remove the bridge mention. The user is typically going to be the
encoder, and bridges are just an automagic implementation of an encoder.
And generally speaking, I'd really like to have an implementation
available before merging this.
Maxime
Hi Anatoliy,
Thank you for the patch.
On Mon, Feb 26, 2024 at 08:44:43PM -0800, Anatoliy Klymenko wrote:
> DPSUB in bridge mode supports multiple input media bus formats.
>
> Announce the list of supported input media bus formats via
> drm_bridge.atomic_get_input_bus_fmts callback.
>
> Signed-off-by: Anatoliy Klymenko <[email protected]>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
> 3 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index e6d26ef60e89..ee99aad915ba 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -18,6 +18,7 @@
> #include <linux/dma/xilinx_dpdma.h>
> #include <linux/dma-mapping.h>
> #include <linux/dmaengine.h>
> +#include <linux/media-bus-format.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> + * @bus_fmt: 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;
> + u32 bus_fmt;
> u32 buf_fmt;
> bool swap;
> const u32 *sf;
> @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
> },
> };
>
> +/* List of live video layer formats */
> +static const struct zynqmp_disp_format avbuf_live_fmts[] = {
> + {
> + .drm_fmt = DRM_FORMAT_VYUY,
> + .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> + .sf = scaling_factors_888,
Is there a reason to have a separate array, instead of populating
bus_fmt in the existing arrays for the formats that can be supported
with the live input, and only reporting those from
zynqmp_disp_get_input_bus_fmts() ?
> + },
> +};
> +
> +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts)
> +{
> + int i;
> + u32 *input_fmts;
> +
> + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), GFP_KERNEL);
> + if (!input_fmts) {
> + *num_input_fmts = 0;
> + return input_fmts;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i)
> + input_fmts[i] = avbuf_live_fmts[i].bus_fmt;
Extra space.
> + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts);
> +
> + return input_fmts;
> +}
> +
> static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
> {
> return readl(disp->avbuf.base + reg);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 9b8b202224d9..c2c8dd4896ba 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -27,6 +27,10 @@
> struct device;
> struct drm_format_info;
> struct drm_plane_state;
> +struct drm_bridge;
> +struct drm_bridge_state;
> +struct drm_connector_state;
> +struct drm_crtc_state;
> struct platform_device;
> struct zynqmp_disp;
> struct zynqmp_disp_layer;
> @@ -52,6 +56,12 @@ 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);
> +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts);
As this is a bridge operation, I think it would be better located in
zynqmp_dp.c. You can possibly expose the avbuf_live_fmts array in
zynqmp_disp.h, but that's not really nice as you'll be missing the size.
Another option would be to split the function in two, with the part that
handles the bridge API implemented in zynqmp_dp.c, and the part that
accesses the formats array in zynqmp_disp.c.
> 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,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..9cb7ac9f3097 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1580,6 +1580,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> .atomic_check = zynqmp_dp_bridge_atomic_check,
> .detect = zynqmp_dp_bridge_detect,
> .edid_read = zynqmp_dp_bridge_edid_read,
> + .atomic_get_input_bus_fmts = zynqmp_disp_get_input_bus_fmts,
> };
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
On Wed, Feb 28, 2024 at 04:29:33PM +0100, Maxime Ripard wrote:
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format negotiation
> > over connected DRM bridge chain and impose CRTC-specific restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will
> > most certainly support a single output media bus format, as supporting
> > multiple runtime options consumes extra FPGA resources. A variety of
> > options for FPGA are usually achieved by synthesizing IP with different
> > parameters.
> >
> > Incorporate select_output_bus_format callback into the format negotiation
> > stage to fix the input bus format of the first DRM bridge in the chain.
> >
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++--
> > include/drm/drm_modeset_helper_vtables.h | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> > #include <drm/drm_edid.h>
> > #include <drm/drm_encoder.h>
> > #include <drm/drm_file.h>
> > +#include <drm/drm_modeset_helper_vtables.h>
> > #include <drm/drm_of.h>
> > #include <drm/drm_print.h>
> >
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > - u32 *in_bus_fmts;
> > + struct drm_crtc *crtc = crtc_state->crtc;
> > + u32 *in_bus_fmts, in_fmt;
> > int ret;
> >
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
> > return -ENOMEM;
> >
> > if (first_bridge == cur_bridge) {
> > - cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > + in_fmt = in_bus_fmts[0];
> > + if (crtc->helper_private &&
> > + crtc->helper_private->select_output_bus_format) {
> > + in_fmt = crtc->helper_private->select_output_bus_format(
> > + crtc,
> > + crtc->state,
> > + in_bus_fmts,
> > + num_in_bus_fmts);
> > + if (!in_fmt) {
> > + kfree(in_bus_fmts);
> > + return -ENOTSUPP;
> > + }
> > + }
> > + cur_state->input_bus_cfg.format = in_fmt;
>
> I don't think we should start poking at the CRTC internals, but we
> should rather provide a helper here.
It would probably look cleaner, yes.
> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> > bool in_vblank_irq, int *vpos, int *hpos,
> > ktime_t *stime, ktime_t *etime,
> > const struct drm_display_mode *mode);
> > +
> > + /**
> > + * @select_output_bus_format
> > + *
> > + * Called by the first connected DRM bridge to negotiate input media
> > + * bus format. CRTC is expected to pick preferable media formats from
> > + * the list supported by the DRM bridge chain.
>
> There's nothing restricting it to bridges here. Please rephrase this to
> remove the bridge mention. The user is typically going to be the
> encoder, and bridges are just an automagic implementation of an encoder.
>
> And generally speaking, I'd really like to have an implementation
> available before merging this.
There's a downstream implementation in the Xilinx kernel, which would
indeed be nice to upstream. This shouldn't block patches 1/4 to 3/4
though, those can be merged once review comments are taken into account.
--
Regards,
Laurent Pinchart
Hi Anatoliy,
Thank you for the patch.
On Mon, Feb 26, 2024 at 08:44:44PM -0800, Anatoliy Klymenko wrote:
> Program live video input format according to selected media bus format.
>
> In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> almost certainly supports a single media bus format as its output. Expect
> this to be delivered via the new bridge atomic state. Program DPSUB
> registers accordingly.
>
> Signed-off-by: Anatoliy Klymenko <[email protected]>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 52 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 ++
> drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++---
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++++---
> 4 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index ee99aad915ba..1c3ffdee6b8e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -416,6 +416,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]);
> + }
This is identical to zynqmp_disp_avbuf_set_format(), you should avoid
duplicating code.
> + layer->disp_fmt = fmt;
> +}
> +
> /**
> * zynqmp_disp_avbuf_set_format - Set the input format for a layer
> * @disp: Display controller
> @@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
> zynqmp_disp_blend_layer_disable(layer->disp, layer);
> }
>
> +/**
> + * zynqmp_disp_layer_set_live_format - Set live layer input format
> + * @layer: The layer
> + * @info: Input media bus format
> + *
> + * Set the live @layer input bus format. The layer must be disabled.
> + */
> +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> + u32 bus_format)
I'd prefer reusing zynqmp_disp_layer_set_format(), and handling the
differences between live and non-live input there. There's already a
dma_enabled check in that function.
> +{
> + int i;
> + const struct zynqmp_disp_format *fmt;
> +
> + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
> + fmt = &avbuf_live_fmts[i];
> + if (fmt->bus_fmt == bus_format) {
> + layer->disp_fmt = fmt;
> + layer->drm_fmt = drm_format_info(fmt->drm_fmt);
> + zynqmp_disp_avbuf_set_live_format(layer->disp, layer, fmt);
> + return;
> + }
> + }
> +}
> +
> /**
> * zynqmp_disp_layer_set_format - Set the layer format
> * @layer: The layer
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index c2c8dd4896ba..f244b7d2346a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -66,6 +66,8 @@ 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);
> +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> + u32 bus_format);
> int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
> struct drm_plane_state *state);
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..fa3935384834 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 (0x0 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4)
This change isn't even mentioned in the commit message. It should be
split to a separate patch.
> #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 9cb7ac9f3097..0d5dffd20ad1 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> {
> enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> - const struct drm_format_info *info;
> + struct drm_bridge_state *bridge_state;
> + u32 bus_fmt;
>
> if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> layer_id = ZYNQMP_DPSUB_LAYER_VID;
> @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> return;
>
> layer = dp->dpsub->layers[layer_id];
> + bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
> + old_bridge_state->bridge);
> + if (bridge_state) {
> + bus_fmt = bridge_state->input_bus_cfg.format;
> + zynqmp_disp_layer_set_live_format(layer, bus_fmt);
> + } else
> + return;
if (!bridge_state)
return;
bus_fmt = bridge_state->input_bus_cfg.format;
zynqmp_disp_layer_set_live_format(layer, bus_fmt);
But more importantly, why would this fail ? If it does something is
seriously wrong and the display won't be working. I'd expect at least a
warning, but you should instead ensure it never fails.
>
> - /* TODO: Make the format configurable. */
> - info = drm_format_info(DRM_FORMAT_YUV422);
> - zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
>
--
Regards,
Laurent Pinchart
Hi Maxime,
Thanks for the review.
> -----Original Message-----
> From: Maxime Ripard <[email protected]>
> Sent: Wednesday, February 28, 2024 7:30 AM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Laurent Pinchart <[email protected]>; Maarten Lankhorst
> <[email protected]>; Thomas Zimmermann
> <[email protected]>; David Airlie <[email protected]>; Daniel Vetter
> <[email protected]>; Simek, Michal <[email protected]>; Andrzej Hajda
> <[email protected]>; Neil Armstrong <[email protected]>; Robert
> Foss <[email protected]>; Jonas Karlman <[email protected]>; Jernej Skrabec
> <[email protected]>; [email protected]; linux-arm-
> [email protected]; [email protected]
> Subject: Re: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format
> callback
>
> Hi,
>
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format
> > negotiation over connected DRM bridge chain and impose CRTC-specific
> restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC
> > will most certainly support a single output media bus format, as
> > supporting multiple runtime options consumes extra FPGA resources. A
> > variety of options for FPGA are usually achieved by synthesizing IP
> > with different parameters.
> >
> > Incorporate select_output_bus_format callback into the format
> > negotiation stage to fix the input bus format of the first DRM bridge in the
> chain.
> >
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++--
> > include/drm/drm_modeset_helper_vtables.h | 31
> > +++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c
> > b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> > #include <drm/drm_edid.h>
> > #include <drm/drm_encoder.h>
> > #include <drm/drm_file.h>
> > +#include <drm/drm_modeset_helper_vtables.h>
> > #include <drm/drm_of.h>
> > #include <drm/drm_print.h>
> >
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > - u32 *in_bus_fmts;
> > + struct drm_crtc *crtc = crtc_state->crtc;
> > + u32 *in_bus_fmts, in_fmt;
> > int ret;
> >
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > return -ENOMEM;
> >
> > if (first_bridge == cur_bridge) {
> > - cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > + in_fmt = in_bus_fmts[0];
> > + if (crtc->helper_private &&
> > + crtc->helper_private->select_output_bus_format) {
> > + in_fmt = crtc->helper_private-
> >select_output_bus_format(
> > + crtc,
> > + crtc->state,
> > + in_bus_fmts,
> > + num_in_bus_fmts);
> > + if (!in_fmt) {
> > + kfree(in_bus_fmts);
> > + return -ENOTSUPP;
> > + }
> > + }
> > + cur_state->input_bus_cfg.format = in_fmt;
>
> I don't think we should start poking at the CRTC internals, but we should rather
> provide a helper here.
Makes sense, thank you. ACK.
>
> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> > bool in_vblank_irq, int *vpos, int *hpos,
> > ktime_t *stime, ktime_t *etime,
> > const struct drm_display_mode *mode);
> > +
> > + /**
> > + * @select_output_bus_format
> > + *
> > + * Called by the first connected DRM bridge to negotiate input media
> > + * bus format. CRTC is expected to pick preferable media formats from
> > + * the list supported by the DRM bridge chain.
>
> There's nothing restricting it to bridges here. Please rephrase this to remove the
> bridge mention. The user is typically going to be the encoder, and bridges are just
> an automagic implementation of an encoder.
>
OK. I'll fix than in the next version.
> And generally speaking, I'd really like to have an implementation available before
> merging this.
>
Well, 2 instances of this callback implementations exist as drafts, as this is the new API. A little bit of a chicken and egg problem. I'll try to groom at least one of them into upstreamable shape and attach it to the patch set.
> Maxime
-Anatoliy
Hi Laurent,
Thanks a lot for the review.
> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Wednesday, February 28, 2024 8:08 AM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>; Maxime Ripard
> <[email protected]>; Thomas Zimmermann <[email protected]>; David
> Airlie <[email protected]>; Daniel Vetter <[email protected]>; Simek, Michal
> <[email protected]>; Andrzej Hajda <[email protected]>; Neil
> Armstrong <[email protected]>; Robert Foss <[email protected]>; Jonas
> Karlman <[email protected]>; Jernej Skrabec <[email protected]>; dri-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Mon, Feb 26, 2024 at 08:44:44PM -0800, Anatoliy Klymenko wrote:
> > Program live video input format according to selected media bus format.
> >
> > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> > almost certainly supports a single media bus format as its output.
> > Expect this to be delivered via the new bridge atomic state. Program
> > DPSUB registers accordingly.
> >
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 52
> +++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 ++
> > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++---
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++++---
> > 4 files changed, 67 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index ee99aad915ba..1c3ffdee6b8e 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -416,6 +416,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]);
> > + }
>
> This is identical to zynqmp_disp_avbuf_set_format(), you should avoid duplicating
> code.
>
Yeah, there are similarities - let me think on how to refactor this properly.
> > + layer->disp_fmt = fmt;
> > +}
> > +
> > /**
> > * zynqmp_disp_avbuf_set_format - Set the input format for a layer
> > * @disp: Display controller
> > @@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct
> zynqmp_disp_layer *layer)
> > zynqmp_disp_blend_layer_disable(layer->disp, layer); }
> >
> > +/**
> > + * zynqmp_disp_layer_set_live_format - Set live layer input format
> > + * @layer: The layer
> > + * @info: Input media bus format
> > + *
> > + * Set the live @layer input bus format. The layer must be disabled.
> > + */
> > +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> > + u32 bus_format)
>
> I'd prefer reusing zynqmp_disp_layer_set_format(), and handling the differences
> between live and non-live input there. There's already a dma_enabled check in
> that function.
>
There is a difference between setting format for dma-backed layer vs live input layer. In the first case we have memory layout in fourcc format, but in the second case we have video signal described by media bus format. Anyways, let me check if I can unify both cases.
> > +{
> > + int i;
> > + const struct zynqmp_disp_format *fmt;
> > +
> > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
> > + fmt = &avbuf_live_fmts[i];
> > + if (fmt->bus_fmt == bus_format) {
> > + layer->disp_fmt = fmt;
> > + layer->drm_fmt = drm_format_info(fmt->drm_fmt);
> > + zynqmp_disp_avbuf_set_live_format(layer->disp, layer, fmt);
> > + return;
> > + }
> > + }
> > +}
> > +
> > /**
> > * zynqmp_disp_layer_set_format - Set the layer format
> > * @layer: The layer
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > index c2c8dd4896ba..f244b7d2346a 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > @@ -66,6 +66,8 @@ 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);
> > +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> > + u32 bus_format);
> > int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
> > struct drm_plane_state *state);
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> > b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> > index f92a006d5070..fa3935384834 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 (0x0 << 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4)
>
> This change isn't even mentioned in the commit message. It should be split to a
> separate patch.
>
This patch is the first instance of these defines usage. Anyways, I'll split it. Thank you.
> > #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 9cb7ac9f3097..0d5dffd20ad1 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct
> > zynqmp_dp *dp, {
> > enum zynqmp_dpsub_layer_id layer_id;
> > struct zynqmp_disp_layer *layer;
> > - const struct drm_format_info *info;
> > + struct drm_bridge_state *bridge_state;
> > + u32 bus_fmt;
> >
> > if (dp->dpsub->connected_ports &
> BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> > layer_id = ZYNQMP_DPSUB_LAYER_VID; @@ -1291,10 +1292,14
> > @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> > return;
> >
> > layer = dp->dpsub->layers[layer_id];
> > + bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state-
> >base.state,
> > + old_bridge_state->bridge);
> > + if (bridge_state) {
> > + bus_fmt = bridge_state->input_bus_cfg.format;
> > + zynqmp_disp_layer_set_live_format(layer, bus_fmt);
> > + } else
> > + return;
>
> if (!bridge_state)
> return;
>
> bus_fmt = bridge_state->input_bus_cfg.format;
> zynqmp_disp_layer_set_live_format(layer, bus_fmt);
>
> But more importantly, why would this fail ? If it does something is seriously wrong
> and the display won't be working. I'd expect at least a warning, but you should
> instead ensure it never fails.
>
According to drm_atomic_get_new_bridge_state() doc, it is possible that bridge state is not a part of the global atomic state. But, most likely in this case bridge enable() callback will be omitted completely. I'll check this - thank you.
> >
> > - /* TODO: Make the format configurable. */
> > - info = drm_format_info(DRM_FORMAT_YUV422);
> > - zynqmp_disp_layer_set_format(layer, info);
> > zynqmp_disp_layer_enable(layer);
> >
> > if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> >
>
> --
> Regards,
>
> Laurent Pinchart
Thank you,
Anatoliy
Hi,
On Wed, Feb 28, 2024 at 10:00:19PM +0000, Klymenko, Anatoliy wrote:
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > b/include/drm/drm_modeset_helper_vtables.h
> > > index 881b03e4dc28..7c21ae1fe3ad 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> > > bool in_vblank_irq, int *vpos, int *hpos,
> > > ktime_t *stime, ktime_t *etime,
> > > const struct drm_display_mode *mode);
> > > +
> > > + /**
> > > + * @select_output_bus_format
> > > + *
> > > + * Called by the first connected DRM bridge to negotiate input media
> > > + * bus format. CRTC is expected to pick preferable media formats from
> > > + * the list supported by the DRM bridge chain.
> >
> > There's nothing restricting it to bridges here. Please rephrase this to remove the
> > bridge mention. The user is typically going to be the encoder, and bridges are just
> > an automagic implementation of an encoder.
> >
>
> OK. I'll fix than in the next version.
>
> > And generally speaking, I'd really like to have an implementation available before
> > merging this.
> >
>
> Well, 2 instances of this callback implementations exist as drafts, as
> this is the new API. A little bit of a chicken and egg problem. I'll
> try to groom at least one of them into upstreamable shape and attach
> it to the patch set.
That's totally what I meant :)
I basically don't want to have an interface that isn't used. If you
provide an implementation in the same series, it's totally reasonable
Maxime
Hi Laurent,
Thanks for the review.
> -----Original Message-----
> From: dri-devel <[email protected]> On Behalf Of Laurent
> Pinchart
> Sent: Wednesday, February 28, 2024 7:58 AM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>; Maxime Ripard
> <[email protected]>; Thomas Zimmermann <[email protected]>; David
> Airlie <[email protected]>; Daniel Vetter <[email protected]>; Simek, Michal
> <[email protected]>; Andrzej Hajda <[email protected]>; Neil
> Armstrong <[email protected]>; Robert Foss <[email protected]>; Jonas
> Karlman <[email protected]>; Jernej Skrabec <[email protected]>; dri-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input
> formats
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Mon, Feb 26, 2024 at 08:44:43PM -0800, Anatoliy Klymenko wrote:
> > DPSUB in bridge mode supports multiple input media bus formats.
> >
> > Announce the list of supported input media bus formats via
> > drm_bridge.atomic_get_input_bus_fmts callback.
> >
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 37
> > +++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
> > 3 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index e6d26ef60e89..ee99aad915ba 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -18,6 +18,7 @@
> > #include <linux/dma/xilinx_dpdma.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/dmaengine.h>
> > +#include <linux/media-bus-format.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > @@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
> > /**
> > * struct zynqmp_disp_format - Display subsystem format information
> > * @drm_fmt: DRM format (4CC)
> > + * @bus_fmt: 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;
> > + u32 bus_fmt;
> > u32 buf_fmt;
> > bool swap;
> > const u32 *sf;
> > @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format
> avbuf_gfx_fmts[] = {
> > },
> > };
> >
> > +/* List of live video layer formats */ static const struct
> > +zynqmp_disp_format avbuf_live_fmts[] = {
> > + {
> > + .drm_fmt = DRM_FORMAT_VYUY,
> > + .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16,
> > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> > + .sf = scaling_factors_888,
>
> Is there a reason to have a separate array, instead of populating .bus_fmt in the
> existing arrays for the formats that can be supported with the live input, and only
> reporting those from
> zynqmp_disp_get_input_bus_fmts() ?
>
There are multiple reasons for this: 1) those formats share some similarities, although they are different in nature, e.g. memory layout formats vs. video signal formats; 2) corresponding IP registers are different with incompatible layouts; 3) ZynqMP DPSUB documentation clearly distinguishes 3 sets of formats: live video, video packer and graphics packer, ref. https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Video-Formats.
I think, having separate format arrays will help to avoid ambiguity.
> > + },
> > +};
> > +
> > +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state,
> > + u32 output_fmt,
> > + unsigned int *num_input_fmts) {
> > + int i;
> > + u32 *input_fmts;
> > +
> > + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts),
> GFP_KERNEL);
> > + if (!input_fmts) {
> > + *num_input_fmts = 0;
> > + return input_fmts;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i)
> > + input_fmts[i] = avbuf_live_fmts[i].bus_fmt;
>
> Extra space.
>
ACK. Thank you.
> > + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts);
> > +
> > + return input_fmts;
> > +}
> > +
> > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
> > {
> > return readl(disp->avbuf.base + reg); diff --git
> > a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > index 9b8b202224d9..c2c8dd4896ba 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > @@ -27,6 +27,10 @@
> > struct device;
> > struct drm_format_info;
> > struct drm_plane_state;
> > +struct drm_bridge;
> > +struct drm_bridge_state;
> > +struct drm_connector_state;
> > +struct drm_crtc_state;
> > struct platform_device;
> > struct zynqmp_disp;
> > struct zynqmp_disp_layer;
> > @@ -52,6 +56,12 @@ 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);
> > +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state,
> > + u32 output_fmt,
> > + unsigned int *num_input_fmts);
>
> As this is a bridge operation, I think it would be better located in zynqmp_dp.c.
> You can possibly expose the avbuf_live_fmts array in zynqmp_disp.h, but that's
> not really nice as you'll be missing the size.
> Another option would be to split the function in two, with the part that handles
> the bridge API implemented in zynqmp_dp.c, and the part that accesses the
> formats array in zynqmp_disp.c.
>
Sure, I'll adjust this.
> > 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, diff
> > --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index 04b6bcac3b07..9cb7ac9f3097 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1580,6 +1580,7 @@ static const struct drm_bridge_funcs
> zynqmp_dp_bridge_funcs = {
> > .atomic_check = zynqmp_dp_bridge_atomic_check,
> > .detect = zynqmp_dp_bridge_detect,
> > .edid_read = zynqmp_dp_bridge_edid_read,
> > + .atomic_get_input_bus_fmts = zynqmp_disp_get_input_bus_fmts,
> > };
> >
> > /*
> > ----------------------------------------------------------------------
> > -------
>
> --
> Regards,
>
> Laurent Pinchart
Thank you,
Anatoliy
Hi Anatoliy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bfa4437fd3938ae2e186e7664b2db65bb8775670]
url: https://github.com/intel-lab-lkp/linux/commits/Anatoliy-Klymenko/drm-xlnx-zynqmp_dpsub-Set-layer-mode-during-creation/20240227-124631
base: bfa4437fd3938ae2e186e7664b2db65bb8775670
patch link: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-3-b78c3f69c9d8%40amd.com
patch subject: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20240304/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240304/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/gpu/drm/xlnx/zynqmp_disp.c:164: warning: Function parameter or struct member 'blend' not described in 'zynqmp_disp'
drivers/gpu/drm/xlnx/zynqmp_disp.c:164: warning: Function parameter or struct member 'avbuf' not described in 'zynqmp_disp'
drivers/gpu/drm/xlnx/zynqmp_disp.c:164: warning: Function parameter or struct member 'audio' not described in 'zynqmp_disp'
>> drivers/gpu/drm/xlnx/zynqmp_disp.c:1019: warning: Function parameter or struct member 'bus_format' not described in 'zynqmp_disp_layer_set_live_format'
>> drivers/gpu/drm/xlnx/zynqmp_disp.c:1019: warning: Excess function parameter 'info' description in 'zynqmp_disp_layer_set_live_format'
vim +1019 drivers/gpu/drm/xlnx/zynqmp_disp.c
1009
1010 /**
1011 * zynqmp_disp_layer_set_live_format - Set live layer input format
1012 * @layer: The layer
1013 * @info: Input media bus format
1014 *
1015 * Set the live @layer input bus format. The layer must be disabled.
1016 */
1017 void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
1018 u32 bus_format)
> 1019 {
1020 int i;
1021 const struct zynqmp_disp_format *fmt;
1022
1023 for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
1024 fmt = &avbuf_live_fmts[i];
1025 if (fmt->bus_fmt == bus_format) {
1026 layer->disp_fmt = fmt;
1027 layer->drm_fmt = drm_format_info(fmt->drm_fmt);
1028 zynqmp_disp_avbuf_set_live_format(layer->disp, layer, fmt);
1029 return;
1030 }
1031 }
1032 }
1033
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki