Currently DRM framework expects that the HDMI connector driver supports
all infoframe types: it generates the data as required and calls into
the driver to program all of them, letting the driver to soft-fail if
the infoframe is unsupported. This has a major drawback on userspace
API: the framework also registers debugfs files for all Infoframe types,
possibly surprising the users when infoframe is visible in the debugfs
file, but it is not visible on the wire.
Let drivers declare that they support only a subset of infoframes,
creating a more consistent interface.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/display/drm_bridge_connector.c | 6 ++++
drivers/gpu/drm/display/drm_hdmi_state_helper.c | 37 ++++++++++++++++++++--
drivers/gpu/drm/drm_connector.c | 4 +++
drivers/gpu/drm/drm_debugfs.c | 16 +++++++---
drivers/gpu/drm/rockchip/inno_hdmi.c | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 1 +
drivers/gpu/drm/tests/drm_connector_test.c | 28 ++++++++++++++++
drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 8 +++++
drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++
include/drm/drm_bridge.h | 7 ++++
include/drm/drm_connector.h | 22 +++++++++++++
11 files changed, 128 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 8c915427d0538435661d771940efe38b462027a1..b94458d5faa9ae283889fc79496ae323bb4dc88c 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -781,6 +781,12 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
&drm_bridge_connector_hdmi_funcs,
connector_type, ddc,
supported_formats,
+ bridge->supported_infoframes ? :
+ DRM_CONNECTOR_INFOFRAME_AUDIO |
+ DRM_CONNECTOR_INFOFRAME_AVI |
+ DRM_CONNECTOR_INFOFRAME_DRM |
+ DRM_CONNECTOR_INFOFRAME_SPD |
+ DRM_CONNECTOR_INFOFRAME_VENDOR,
max_bpc);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index a561f124be99a0cd4259dbacf5f5f6651ff8a0ea..44100fba5e7465b39bce48a086bc3d012d951690 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -687,6 +687,9 @@ static int hdmi_generate_avi_infoframe(const struct drm_connector *connector,
infoframe->set = false;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_AVI))
+ return 0;
+
ret = drm_hdmi_avi_infoframe_from_display_mode(frame, connector, mode);
if (ret)
return ret;
@@ -718,6 +721,9 @@ static int hdmi_generate_spd_infoframe(const struct drm_connector *connector,
infoframe->set = false;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_SPD))
+ return 0;
+
ret = hdmi_spd_infoframe_init(frame,
connector->hdmi.vendor,
connector->hdmi.product);
@@ -742,6 +748,9 @@ static int hdmi_generate_hdr_infoframe(const struct drm_connector *connector,
infoframe->set = false;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_DRM))
+ return 0;
+
if (connector->max_bpc < 10)
return 0;
@@ -771,6 +780,9 @@ static int hdmi_generate_hdmi_vendor_infoframe(const struct drm_connector *conne
infoframe->set = false;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_VENDOR))
+ return 0;
+
if (!info->has_hdmi_infoframe)
return 0;
@@ -905,6 +917,11 @@ static int clear_device_infoframe(struct drm_connector *connector,
return 0;
}
+ if (!drm_hdmi_connector_supports_infoframe(connector, type)) {
+ drm_dbg_kms(dev, "Infoframe 0x%02x not supported, bailing.\n", type);
+ return 0;
+ }
+
ret = funcs->clear_infoframe(connector, type);
if (ret) {
drm_dbg_kms(dev, "Call failed: %d\n", ret);
@@ -930,23 +947,29 @@ static int write_device_infoframe(struct drm_connector *connector,
union hdmi_infoframe *frame)
{
const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
+ enum hdmi_infoframe_type type = frame->any.type;
struct drm_device *dev = connector->dev;
u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
int ret;
int len;
- drm_dbg_kms(dev, "Writing infoframe type %x\n", frame->any.type);
+ drm_dbg_kms(dev, "Writing infoframe type %x\n", type);
if (!funcs || !funcs->write_infoframe) {
drm_dbg_kms(dev, "Function not implemented, bailing.\n");
return -EINVAL;
}
+ if (!drm_hdmi_connector_supports_infoframe(connector, type)) {
+ drm_dbg_kms(dev, "Infoframe %d not supported, bailing.\n", type);
+ return 0;
+ }
+
len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
if (len < 0)
return len;
- ret = funcs->write_infoframe(connector, frame->any.type, buffer, len);
+ ret = funcs->write_infoframe(connector, type, buffer, len);
if (ret) {
drm_dbg_kms(dev, "Call failed: %d\n", ret);
return ret;
@@ -1067,6 +1090,11 @@ drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co
struct drm_display_info *info = &connector->display_info;
int ret;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_AUDIO)) {
+ drm_warn_once(connector->dev, "Audio Infoframe not supported, bailing.\n");
+ return -EOPNOTSUPP;
+ }
+
if (!info->is_hdmi)
return 0;
@@ -1102,6 +1130,11 @@ drm_atomic_helper_connector_hdmi_clear_audio_infoframe(struct drm_connector *con
struct drm_display_info *info = &connector->display_info;
int ret;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_AUDIO)) {
+ drm_warn_once(connector->dev, "Audio Infoframe not supported, bailing.\n");
+ return -EOPNOTSUPP;
+ }
+
if (!info->is_hdmi)
return 0;
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea4784e97ca894ec4d463beebf9fdbf0..e753de9fc80a26c30b9674c96083328711f32960 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -553,6 +553,7 @@ EXPORT_SYMBOL(drmm_connector_init);
* @connector_type: user visible type of the connector
* @ddc: optional pointer to the associated ddc adapter
* @supported_formats: Bitmask of @hdmi_colorspace listing supported output formats
+ * @supported_infoframes: Bitmask of @DRM_CONNECTOR_INFOFRAME listing supported Infoframes
* @max_bpc: Maximum bits per char the HDMI connector supports
*
* Initialises a preallocated HDMI connector. Connectors can be
@@ -576,6 +577,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
int connector_type,
struct i2c_adapter *ddc,
unsigned long supported_formats,
+ unsigned long supported_infoframes,
unsigned int max_bpc)
{
int ret;
@@ -623,6 +625,8 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
connector->hdmi.funcs = hdmi_funcs;
+ connector->hdmi.supported_infoframes = supported_infoframes;
+
return 0;
}
EXPORT_SYMBOL(drmm_connector_hdmi_init);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 365cf337529fa2a88b69516d57360d212419c126..248cb9ea1d8781674160cd8d454113c9422ec691 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -672,6 +672,9 @@ static int create_hdmi_audio_infoframe_file(struct drm_connector *connector,
{
struct dentry *file;
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_AUDIO))
+ return 0;
+
file = debugfs_create_file("audio", 0400, parent, connector, &audio_infoframe_fops);
if (IS_ERR(file))
return PTR_ERR(file);
@@ -679,7 +682,7 @@ static int create_hdmi_audio_infoframe_file(struct drm_connector *connector,
return 0;
}
-#define DEFINE_INFOFRAME_FILE(_f) \
+#define DEFINE_INFOFRAME_FILE(_f, _F) \
static ssize_t _f##_read_infoframe(struct file *filp, \
char __user *ubuf, \
size_t count, \
@@ -726,6 +729,9 @@ static int create_hdmi_## _f ## _infoframe_file(struct drm_connector *connector,
{ \
struct dentry *file; \
\
+ if (!drm_hdmi_connector_supports_infoframe(connector, HDMI_INFOFRAME_TYPE_ ## _F)) \
+ return 0; \
+ \
file = debugfs_create_file(#_f, 0400, parent, connector, &_f ## _infoframe_fops); \
if (IS_ERR(file)) \
return PTR_ERR(file); \
@@ -733,10 +739,10 @@ static int create_hdmi_## _f ## _infoframe_file(struct drm_connector *connector,
return 0; \
}
-DEFINE_INFOFRAME_FILE(avi);
-DEFINE_INFOFRAME_FILE(hdmi);
-DEFINE_INFOFRAME_FILE(hdr_drm);
-DEFINE_INFOFRAME_FILE(spd);
+DEFINE_INFOFRAME_FILE(avi, AVI);
+DEFINE_INFOFRAME_FILE(hdmi, VENDOR);
+DEFINE_INFOFRAME_FILE(hdr_drm, DRM);
+DEFINE_INFOFRAME_FILE(spd, SPD);
static int create_hdmi_infoframe_files(struct drm_connector *connector,
struct dentry *parent)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 1ab3ad4bde9ea7305021186ea221d2ff9057fdbb..65eed5ae23194200c145cb174acff4f252b3ef1f 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -1065,6 +1065,7 @@ static int inno_hdmi_register(struct drm_device *drm, struct inno_hdmi *hdmi)
DRM_MODE_CONNECTOR_HDMIA,
hdmi->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_INFOFRAME_AVI,
8);
drm_connector_attach_encoder(&hdmi->connector, encoder);
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index ab0938ba61f7d75dd0bec473807a04a20e1cffbd..0b931da4ea2d4eb58a6224476059b9205e8626b4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -640,6 +640,7 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
DRM_MODE_CONNECTOR_HDMIA,
hdmi->ddc_i2c,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_INFOFRAME_AVI,
8);
if (ret) {
dev_err(dev,
diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
index 22e2d959eb31459f9981fef488228904d67cb6f9..fd28ed2bf8bcecabaabc67f2f8f5ccc1f42525d3 100644
--- a/drivers/gpu/drm/tests/drm_connector_test.c
+++ b/drivers/gpu/drm/tests/drm_connector_test.c
@@ -641,6 +641,13 @@ static struct kunit_suite drm_connector_dynamic_register_test_suite = {
.test_cases = drm_connector_dynamic_register_tests,
};
+#define DRM_CONNECTOR_ALL_INFOFRAMES \
+ (DRM_CONNECTOR_INFOFRAME_AUDIO | \
+ DRM_CONNECTOR_INFOFRAME_AVI | \
+ DRM_CONNECTOR_INFOFRAME_DRM | \
+ DRM_CONNECTOR_INFOFRAME_SPD | \
+ DRM_CONNECTOR_INFOFRAME_VENDOR)
+
/*
* Test that the registration of a bog standard connector works as
* expected and doesn't report any error.
@@ -657,6 +664,7 @@ static void drm_test_connector_hdmi_init_valid(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
}
@@ -677,6 +685,7 @@ static void drm_test_connector_hdmi_init_null_ddc(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
NULL,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
}
@@ -697,6 +706,7 @@ static void drm_test_connector_hdmi_init_null_vendor(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -717,6 +727,7 @@ static void drm_test_connector_hdmi_init_null_product(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -743,6 +754,7 @@ static void drm_test_connector_hdmi_init_product_valid(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_MEMEQ(test,
@@ -776,6 +788,7 @@ static void drm_test_connector_hdmi_init_product_length_exact(struct kunit *test
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_MEMEQ(test,
@@ -803,6 +816,7 @@ static void drm_test_connector_hdmi_init_product_length_too_long(struct kunit *t
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -829,6 +843,7 @@ static void drm_test_connector_hdmi_init_vendor_valid(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_MEMEQ(test,
@@ -861,6 +876,7 @@ static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_MEMEQ(test,
@@ -888,6 +904,7 @@ static void drm_test_connector_hdmi_init_vendor_length_too_long(struct kunit *te
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -908,6 +925,7 @@ static void drm_test_connector_hdmi_init_bpc_invalid(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
9);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -928,6 +946,7 @@ static void drm_test_connector_hdmi_init_bpc_null(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
0);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -953,6 +972,7 @@ static void drm_test_connector_hdmi_init_bpc_8(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
@@ -994,6 +1014,7 @@ static void drm_test_connector_hdmi_init_bpc_10(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
10);
KUNIT_EXPECT_EQ(test, ret, 0);
@@ -1035,6 +1056,7 @@ static void drm_test_connector_hdmi_init_bpc_12(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
12);
KUNIT_EXPECT_EQ(test, ret, 0);
@@ -1071,6 +1093,7 @@ static void drm_test_connector_hdmi_init_formats_empty(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
0,
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -1091,6 +1114,7 @@ static void drm_test_connector_hdmi_init_formats_no_rgb(struct kunit *test)
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_YUV422),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -1149,6 +1173,7 @@ static void drm_test_connector_hdmi_init_formats_yuv420_allowed(struct kunit *te
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
params->supported_formats,
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, params->expected_result);
}
@@ -1170,6 +1195,7 @@ static void drm_test_connector_hdmi_init_type_valid(struct kunit *test)
connector_type,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
}
@@ -1205,6 +1231,7 @@ static void drm_test_connector_hdmi_init_type_invalid(struct kunit *test)
connector_type,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_LT(test, ret, 0);
}
@@ -1482,6 +1509,7 @@ static void drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector(
DRM_MODE_CONNECTOR_HDMIA,
&priv->ddc,
BIT(HDMI_COLORSPACE_RGB),
+ DRM_CONNECTOR_ALL_INFOFRAMES,
8);
KUNIT_EXPECT_EQ(test, ret, 0);
diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 8bd412735000cb18e66aeca21433b2ebbefe2b44..2901fcb6b12ee318a4a9c727a62d5290d7c9aa84 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -138,6 +138,13 @@ static const struct drm_connector_funcs dummy_connector_funcs = {
.reset = dummy_hdmi_connector_reset,
};
+#define DRM_CONNECTOR_ALL_INFOFRAMES \
+ (DRM_CONNECTOR_INFOFRAME_AUDIO | \
+ DRM_CONNECTOR_INFOFRAME_AVI | \
+ DRM_CONNECTOR_INFOFRAME_DRM | \
+ DRM_CONNECTOR_INFOFRAME_SPD | \
+ DRM_CONNECTOR_INFOFRAME_VENDOR)
+
static
struct drm_atomic_helper_connector_hdmi_priv *
__connector_hdmi_init(struct kunit *test,
@@ -192,6 +199,7 @@ __connector_hdmi_init(struct kunit *test,
DRM_MODE_CONNECTOR_HDMIA,
NULL,
formats,
+ DRM_CONNECTOR_ALL_INFOFRAMES,
max_bpc);
KUNIT_ASSERT_EQ(test, ret, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 07c91b450f93ab9e795d040d6f60f485ac71cfe8..2098d04c95e7e733307c90bb9ab5e2631f6f5df0 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -556,6 +556,11 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
BIT(HDMI_COLORSPACE_RGB) |
BIT(HDMI_COLORSPACE_YUV422) |
BIT(HDMI_COLORSPACE_YUV444),
+ DRM_CONNECTOR_INFOFRAME_AUDIO |
+ DRM_CONNECTOR_INFOFRAME_AVI |
+ DRM_CONNECTOR_INFOFRAME_DRM |
+ DRM_CONNECTOR_INFOFRAME_SPD |
+ DRM_CONNECTOR_INFOFRAME_VENDOR,
max_bpc);
if (ret)
return ret;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 620e119cc24c3491c2be5f08efaf51dfa8f708b3..529dcaca1d7924da12d9587170f96ec6a00ad126 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1153,6 +1153,13 @@ struct drm_bridge {
*/
unsigned int max_bpc;
+ /**
+ * @supported_infoframes: Bitmask of DRM_CONNECTOR_INFOFRAME values,
+ * listing supported infoframes. This is only relevant if
+ * @DRM_BRIDGE_OP_HDMI is set.
+ */
+ unsigned int supported_infoframes;
+
/**
* @hdmi_cec_dev: device to be used as a containing device for CEC
* functions.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d83dccd3e820a444fbf74fb6c16f2..7a92b4d75d25b355898b6c5d7cc45431187dc3b9 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1839,6 +1839,12 @@ struct drm_connector_hdmi {
*/
unsigned long supported_formats;
+ /**
+ * @supported_infoframes: Bitmask of infoframe types supported by the
+ * controller. See @DRM_CONNECTOR_INFOFRAME.
+ */
+ unsigned long supported_infoframes;
+
/**
* @funcs: HDMI connector Control Functions
*/
@@ -2336,6 +2342,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
int connector_type,
struct i2c_adapter *ddc,
unsigned long supported_formats,
+ unsigned long supported_infoframes,
unsigned int max_bpc);
void drm_connector_attach_edid_property(struct drm_connector *connector);
int drm_connector_register(struct drm_connector *connector);
@@ -2488,6 +2495,21 @@ void drm_connector_attach_privacy_screen_provider(
struct drm_connector *connector, struct drm_privacy_screen *priv);
void drm_connector_update_privacy_screen(const struct drm_connector_state *connector_state);
+#define DRM_CONNECTOR_INFOFRAME(type) BIT(type - 0x80)
+
+#define DRM_CONNECTOR_INFOFRAME_AUDIO DRM_CONNECTOR_INFOFRAME(HDMI_INFOFRAME_TYPE_AUDIO)
+#define DRM_CONNECTOR_INFOFRAME_AVI DRM_CONNECTOR_INFOFRAME(HDMI_INFOFRAME_TYPE_AVI)
+#define DRM_CONNECTOR_INFOFRAME_DRM DRM_CONNECTOR_INFOFRAME(HDMI_INFOFRAME_TYPE_DRM)
+#define DRM_CONNECTOR_INFOFRAME_SPD DRM_CONNECTOR_INFOFRAME(HDMI_INFOFRAME_TYPE_SPD)
+#define DRM_CONNECTOR_INFOFRAME_VENDOR DRM_CONNECTOR_INFOFRAME(HDMI_INFOFRAME_TYPE_VENDOR)
+
+static inline bool
+drm_hdmi_connector_supports_infoframe(const struct drm_connector *connector,
+ enum hdmi_infoframe_type type)
+{
+ return connector->hdmi.supported_infoframes & DRM_CONNECTOR_INFOFRAME(type);
+}
+
/**
* struct drm_tile_group - Tile group metadata
* @refcount: reference count
--
2.47.2Hi, On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > Currently DRM framework expects that the HDMI connector driver supports > all infoframe types: it generates the data as required and calls into > the driver to program all of them, letting the driver to soft-fail if > the infoframe is unsupported. This has a major drawback on userspace > API: the framework also registers debugfs files for all Infoframe types, > possibly surprising the users when infoframe is visible in the debugfs > file, but it is not visible on the wire. > > Let drivers declare that they support only a subset of infoframes, > creating a more consistent interface. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> I'm not really convinced. Infoframes aren't really something you should ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if audio support is enabled, DRM is mandatory if HDR is used. SPD is indeed optional though. So, it's really dynamic in essence, and not really something we should expect drivers to ignore. I do acknowledge that a lot of drivers just silently ignore the infoframes they don't support at the moment, which isn't great either. Maybe we should standardize and document what drivers should do when they don't support a given infoframe type? Something like return EOPNOTSUPP if you don't support it, and we warn in the core if we get one for a mandatory infoframe? Maxime
On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > Hi, > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > Currently DRM framework expects that the HDMI connector driver supports > > all infoframe types: it generates the data as required and calls into > > the driver to program all of them, letting the driver to soft-fail if > > the infoframe is unsupported. This has a major drawback on userspace > > API: the framework also registers debugfs files for all Infoframe types, > > possibly surprising the users when infoframe is visible in the debugfs > > file, but it is not visible on the wire. > > > > Let drivers declare that they support only a subset of infoframes, > > creating a more consistent interface. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > I'm not really convinced. Infoframes aren't really something you should > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > audio support is enabled, DRM is mandatory if HDR is used. Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers provide support only for the AVI infoframe. Some of them can be extended to support other infoframe kinds (e.g. ADV7511 has two spare infoframes which can be used for HDMI and SPD). > SPD is indeed optional though. > > So, it's really dynamic in essence, and not really something we should > expect drivers to ignore. > > I do acknowledge that a lot of drivers just silently ignore the > infoframes they don't support at the moment, which isn't great either. > > Maybe we should standardize and document what drivers should do when > they don't support a given infoframe type? The chips might be generating infoframes internally. This series was triggered by LT9611UXC, which does all HDMI work under the hood in the firmware. See [1]. The series I posted hooks HDMI audio directly into the bridge driver, but I'd really prefer to be able to use drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to implementing CEC support for it. ADV7511 likewise generates audio infoframe without Linux help (audio-related fields are programmed, but it's not the infoframe itself). Maybe I should change documentation for this field as 'support sending infoframes generated by the Linux kernel'. > Something like return EOPNOTSUPP if you don't support it, and we warn in > the core if we get one for a mandatory infoframe? [1] https://lore.kernel.org/dri-devel/20250803-lt9611uxc-hdmi-v1-2-cb9ce1793acf@oss.qualcomm.com/ -- With best wishes Dmitry
Hi, On Wed, Aug 20, 2025 at 12:52:44PM +0300, Dmitry Baryshkov wrote: > On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > > Hi, > > > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > > Currently DRM framework expects that the HDMI connector driver supports > > > all infoframe types: it generates the data as required and calls into > > > the driver to program all of them, letting the driver to soft-fail if > > > the infoframe is unsupported. This has a major drawback on userspace > > > API: the framework also registers debugfs files for all Infoframe types, > > > possibly surprising the users when infoframe is visible in the debugfs > > > file, but it is not visible on the wire. > > > > > > Let drivers declare that they support only a subset of infoframes, > > > creating a more consistent interface. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > > I'm not really convinced. Infoframes aren't really something you should > > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > > audio support is enabled, DRM is mandatory if HDR is used. > > Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers > provide support only for the AVI infoframe. Yes, but it's still something we shouldn't paper over. The spec mandates it, if drivers want to deviate from it it's something we should warn about, not silence. sun4i is a good example, to me at least since I have the doc. The hardware supports AVI, Audio, ACP, and SPD. HDR isn't supported, so DRM isn't either. The only missing one is HDMI, but the documentation isn't the best so it might still be supported. In short, it's a driver issue. adv7511 supports AVI, Audio, ACP, SPD, ACP, and looks to have a mechanism to send any infoframe as is. So, again, driver issue. I couldn't find the other datasheet, but I'd be very surprised if it wasn't the case for these too. > Some of them can be extended to support other infoframe kinds (e.g. > ADV7511 has two spare infoframes which can be used for HDMI and SPD). > > > SPD is indeed optional though. > > > > So, it's really dynamic in essence, and not really something we should > > expect drivers to ignore. > > > > I do acknowledge that a lot of drivers just silently ignore the > > infoframes they don't support at the moment, which isn't great either. > > > > Maybe we should standardize and document what drivers should do when > > they don't support a given infoframe type? > > The chips might be generating infoframes internally. This series was > triggered by LT9611UXC, which does all HDMI work under the hood in the > firmware. See [1]. The series I posted hooks HDMI audio directly into > the bridge driver, but I'd really prefer to be able to use > drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to > implementing CEC support for it. > > ADV7511 likewise generates audio infoframe without Linux > help (audio-related fields are programmed, but it's not the > infoframe itself). Implementing the write_infoframe hooks as a nop with a comment in those case is totally reasonable to me. I'd still like to document that drivers should only return 0 if they programmed the infoframe, and -ENOTSUPP (and the core logging a warning) otherwise. That way, we would be able to differentiate between the legimitate LT9611UXC case, and the "driver is broken" sun4i (and others) case. Maxime
On Wed, Aug 27, 2025 at 09:30:20AM +0200, Maxime Ripard wrote: > Hi, > > On Wed, Aug 20, 2025 at 12:52:44PM +0300, Dmitry Baryshkov wrote: > > On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > > > Hi, > > > > > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > > > Currently DRM framework expects that the HDMI connector driver supports > > > > all infoframe types: it generates the data as required and calls into > > > > the driver to program all of them, letting the driver to soft-fail if > > > > the infoframe is unsupported. This has a major drawback on userspace > > > > API: the framework also registers debugfs files for all Infoframe types, > > > > possibly surprising the users when infoframe is visible in the debugfs > > > > file, but it is not visible on the wire. > > > > > > > > Let drivers declare that they support only a subset of infoframes, > > > > creating a more consistent interface. > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > > > > I'm not really convinced. Infoframes aren't really something you should > > > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > > > audio support is enabled, DRM is mandatory if HDR is used. > > > > Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers > > provide support only for the AVI infoframe. > > Yes, but it's still something we shouldn't paper over. The spec mandates > it, if drivers want to deviate from it it's something we should warn > about, not silence. > > sun4i is a good example, to me at least since I have the doc. The > hardware supports AVI, Audio, ACP, and SPD. HDR isn't supported, so DRM > isn't either. The only missing one is HDMI, but the documentation isn't > the best so it might still be supported. In short, it's a driver issue. > > adv7511 supports AVI, Audio, ACP, SPD, ACP, and looks to have a > mechanism to send any infoframe as is. So, again, driver issue. I've send a patch, enabling SPD and VSI (HDMI) InfoFrames on ADV7511. > > I couldn't find the other datasheet, but I'd be very surprised if it > wasn't the case for these too. > > > Some of them can be extended to support other infoframe kinds (e.g. > > ADV7511 has two spare infoframes which can be used for HDMI and SPD). > > > > > SPD is indeed optional though. > > > > > > So, it's really dynamic in essence, and not really something we should > > > expect drivers to ignore. > > > > > > I do acknowledge that a lot of drivers just silently ignore the > > > infoframes they don't support at the moment, which isn't great either. > > > > > > Maybe we should standardize and document what drivers should do when > > > they don't support a given infoframe type? > > > > The chips might be generating infoframes internally. This series was > > triggered by LT9611UXC, which does all HDMI work under the hood in the > > firmware. See [1]. The series I posted hooks HDMI audio directly into > > the bridge driver, but I'd really prefer to be able to use > > drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to > > implementing CEC support for it. > > > > ADV7511 likewise generates audio infoframe without Linux > > help (audio-related fields are programmed, but it's not the > > infoframe itself). > > Implementing the write_infoframe hooks as a nop with a comment in those > case is totally reasonable to me. > > I'd still like to document that drivers should only return 0 if they > programmed the infoframe, and -ENOTSUPP (and the core logging a warning) > otherwise. > > That way, we would be able to differentiate between the legimitate > LT9611UXC case, and the "driver is broken" sun4i (and others) case. I don't want to end up in a sitation where userspace has a different idea of the InfoFrame being sent than the actual one being present on the wire. It seems, we need several states per the infoframe: - Not supported - Autogenerated - Generated by software E.g. in case of ADV7511 we can declare that Audio InfofFrame is autogenerated, AVI, HDMI and SPD as 'software-generated' and DRM (HDR) as unsupported. LT9611UXC will declare all (need to check) frame types as auto. This way we can implement the checks and still keep userspace from having irrelevant data in debugfs. I will update my patchset to implement this, but I have another question beforehand: should we just declare VSI support or should it be more exact, specifying that the driver support HVS (00:0c:03), HVFS (c4:5d:d8), etc? I'm asking, because e.g. MSM HDMI controller has hardware support for generating HVS frames (but only HVS, the OUI is not programmed, register format doesn't match 1:1 frame contents, etc). I instead ended up using GENERIC0, because it was more flexible (it's like SPARE packets on ADV7511, the contents is being sent as is). However if we ever need to send DRM infoframes, we might need to switch from GENERIC0 to HVS, for the price of being unable to send HVFS frames. WDYT? -- With best wishes Dmitry
On Wed, Aug 27, 2025 at 05:04:53PM +0300, Dmitry Baryshkov wrote: > On Wed, Aug 27, 2025 at 09:30:20AM +0200, Maxime Ripard wrote: > > On Wed, Aug 20, 2025 at 12:52:44PM +0300, Dmitry Baryshkov wrote: > > > On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > > > > Currently DRM framework expects that the HDMI connector driver supports > > > > > all infoframe types: it generates the data as required and calls into > > > > > the driver to program all of them, letting the driver to soft-fail if > > > > > the infoframe is unsupported. This has a major drawback on userspace > > > > > API: the framework also registers debugfs files for all Infoframe types, > > > > > possibly surprising the users when infoframe is visible in the debugfs > > > > > file, but it is not visible on the wire. > > > > > > > > > > Let drivers declare that they support only a subset of infoframes, > > > > > creating a more consistent interface. > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > > > > > > I'm not really convinced. Infoframes aren't really something you should > > > > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > > > > audio support is enabled, DRM is mandatory if HDR is used. > > > > > > Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers > > > provide support only for the AVI infoframe. > > > > Yes, but it's still something we shouldn't paper over. The spec mandates > > it, if drivers want to deviate from it it's something we should warn > > about, not silence. > > > > sun4i is a good example, to me at least since I have the doc. The > > hardware supports AVI, Audio, ACP, and SPD. HDR isn't supported, so DRM > > isn't either. The only missing one is HDMI, but the documentation isn't > > the best so it might still be supported. In short, it's a driver issue. > > > > adv7511 supports AVI, Audio, ACP, SPD, ACP, and looks to have a > > mechanism to send any infoframe as is. So, again, driver issue. > > I've send a patch, enabling SPD and VSI (HDMI) InfoFrames on ADV7511. > > > > > I couldn't find the other datasheet, but I'd be very surprised if it > > wasn't the case for these too. > > > > > Some of them can be extended to support other infoframe kinds (e.g. > > > ADV7511 has two spare infoframes which can be used for HDMI and SPD). > > > > > > > SPD is indeed optional though. > > > > > > > > So, it's really dynamic in essence, and not really something we should > > > > expect drivers to ignore. > > > > > > > > I do acknowledge that a lot of drivers just silently ignore the > > > > infoframes they don't support at the moment, which isn't great either. > > > > > > > > Maybe we should standardize and document what drivers should do when > > > > they don't support a given infoframe type? > > > > > > The chips might be generating infoframes internally. This series was > > > triggered by LT9611UXC, which does all HDMI work under the hood in the > > > firmware. See [1]. The series I posted hooks HDMI audio directly into > > > the bridge driver, but I'd really prefer to be able to use > > > drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to > > > implementing CEC support for it. > > > > > > ADV7511 likewise generates audio infoframe without Linux > > > help (audio-related fields are programmed, but it's not the > > > infoframe itself). > > > > Implementing the write_infoframe hooks as a nop with a comment in those > > case is totally reasonable to me. > > > > I'd still like to document that drivers should only return 0 if they > > programmed the infoframe, and -ENOTSUPP (and the core logging a warning) > > otherwise. > > > > That way, we would be able to differentiate between the legimitate > > LT9611UXC case, and the "driver is broken" sun4i (and others) case. > > I don't want to end up in a sitation where userspace has a different > idea of the InfoFrame being sent than the actual one being present on > the wire. It's not ideal, sure, but also, what's wrong with it? We're doing it *all the time*. Modes programmed by userspace are adjusted for the hardware, and thus the mode reported by the CRTC turns out different than the one actually used in hardware. Audio sampling rates might not match exactly what we're doing. The quirks infrastructure disables part of the EDID the userspace has access to, etc. And all those are under the userspace control, which the infoframes aren't. > It seems, we need several states per the infoframe: > > - Not supported Honestly, I'm not sure we need a state for that one. If that infoframe was set by the framework, then the driver must support it. And if it wasn't, then there's nothing in debugfs. > - Autogenerated Do we have any way to read them back on those? > - Generated by software > > E.g. in case of ADV7511 we can declare that Audio InfofFrame is > autogenerated, AVI, HDMI and SPD as 'software-generated' and DRM (HDR) > as unsupported. LT9611UXC will declare all (need to check) frame types > as auto. > > This way we can implement the checks and still keep userspace from > having irrelevant data in debugfs. If the only thing you're after is to prevent inconsistent data in userpace for devices that can generate it automatically, then I guess we could just implement an (optional) callback to read an infoframe from the hardware when reading from debugfs. Would that work? > I will update my patchset to implement this, but I have another question > beforehand: should we just declare VSI support or should it be more exact, > specifying that the driver support HVS (00:0c:03), HVFS (c4:5d:d8), etc? I guess you're talking about HDMI 1.4 Vendor specific Infoframe vs HDMI 2.0 HF-VSIF here? If so, the toggle should be HDMI 2.0 support. We'll need that toggle for other things anyway (scrambler, YUV420, etc.) > I'm asking, because e.g. MSM HDMI controller has hardware support for > generating HVS frames (but only HVS, the OUI is not programmed, register > format doesn't match 1:1 frame contents, etc). I instead ended up using > GENERIC0, because it was more flexible (it's like SPARE packets on > ADV7511, the contents is being sent as is). However if we ever need to > send DRM infoframes, we might need to switch from GENERIC0 to HVS, for > the price of being unable to send HVFS frames. Section 10.2 of the HDMI 2.0 states: Transmission of the HF-VSIF by Source Devices is optional unless one (or more) of the features listed in Table 10-1 is active 1. If such features are active, transmission of the HF-VSIF is mandatory. The features in question being 3d. So unless you're supporting 3d, suppporting VSI only seems ok to me. Maxime
On Mon, Sep 01, 2025 at 08:54:18AM +0200, Maxime Ripard wrote: > On Wed, Aug 27, 2025 at 05:04:53PM +0300, Dmitry Baryshkov wrote: > > On Wed, Aug 27, 2025 at 09:30:20AM +0200, Maxime Ripard wrote: > > > On Wed, Aug 20, 2025 at 12:52:44PM +0300, Dmitry Baryshkov wrote: > > > > On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > > > > > Hi, > > > > > > > > > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > > > > > Currently DRM framework expects that the HDMI connector driver supports > > > > > > all infoframe types: it generates the data as required and calls into > > > > > > the driver to program all of them, letting the driver to soft-fail if > > > > > > the infoframe is unsupported. This has a major drawback on userspace > > > > > > API: the framework also registers debugfs files for all Infoframe types, > > > > > > possibly surprising the users when infoframe is visible in the debugfs > > > > > > file, but it is not visible on the wire. > > > > > > > > > > > > Let drivers declare that they support only a subset of infoframes, > > > > > > creating a more consistent interface. > > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > > > > > > > > I'm not really convinced. Infoframes aren't really something you should > > > > > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > > > > > audio support is enabled, DRM is mandatory if HDR is used. > > > > > > > > Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers > > > > provide support only for the AVI infoframe. > > > > > > Yes, but it's still something we shouldn't paper over. The spec mandates > > > it, if drivers want to deviate from it it's something we should warn > > > about, not silence. > > > > > > sun4i is a good example, to me at least since I have the doc. The > > > hardware supports AVI, Audio, ACP, and SPD. HDR isn't supported, so DRM > > > isn't either. The only missing one is HDMI, but the documentation isn't > > > the best so it might still be supported. In short, it's a driver issue. > > > > > > adv7511 supports AVI, Audio, ACP, SPD, ACP, and looks to have a > > > mechanism to send any infoframe as is. So, again, driver issue. > > > > I've send a patch, enabling SPD and VSI (HDMI) InfoFrames on ADV7511. > > > > > > > > I couldn't find the other datasheet, but I'd be very surprised if it > > > wasn't the case for these too. > > > > > > > Some of them can be extended to support other infoframe kinds (e.g. > > > > ADV7511 has two spare infoframes which can be used for HDMI and SPD). > > > > > > > > > SPD is indeed optional though. > > > > > > > > > > So, it's really dynamic in essence, and not really something we should > > > > > expect drivers to ignore. > > > > > > > > > > I do acknowledge that a lot of drivers just silently ignore the > > > > > infoframes they don't support at the moment, which isn't great either. > > > > > > > > > > Maybe we should standardize and document what drivers should do when > > > > > they don't support a given infoframe type? > > > > > > > > The chips might be generating infoframes internally. This series was > > > > triggered by LT9611UXC, which does all HDMI work under the hood in the > > > > firmware. See [1]. The series I posted hooks HDMI audio directly into > > > > the bridge driver, but I'd really prefer to be able to use > > > > drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to > > > > implementing CEC support for it. > > > > > > > > ADV7511 likewise generates audio infoframe without Linux > > > > help (audio-related fields are programmed, but it's not the > > > > infoframe itself). > > > > > > Implementing the write_infoframe hooks as a nop with a comment in those > > > case is totally reasonable to me. > > > > > > I'd still like to document that drivers should only return 0 if they > > > programmed the infoframe, and -ENOTSUPP (and the core logging a warning) > > > otherwise. > > > > > > That way, we would be able to differentiate between the legimitate > > > LT9611UXC case, and the "driver is broken" sun4i (and others) case. > > > > I don't want to end up in a sitation where userspace has a different > > idea of the InfoFrame being sent than the actual one being present on > > the wire. > > It's not ideal, sure, but also, what's wrong with it? We're doing it > *all the time*. Modes programmed by userspace are adjusted for the > hardware, and thus the mode reported by the CRTC turns out different > than the one actually used in hardware. Audio sampling rates might not > match exactly what we're doing. The quirks infrastructure disables part > of the EDID the userspace has access to, etc. > > And all those are under the userspace control, which the infoframes > aren't. I think there is a differnece between 'change userspace input', 'knowingly mangle data' and 'lie to userspace because the driver doesn't care'. This is especially important e.g. if a user is trying to debug AV issues which can be caused by wrong information in the infoframe. > > > It seems, we need several states per the infoframe: > > > > - Not supported > > Honestly, I'm not sure we need a state for that one. If that infoframe > was set by the framework, then the driver must support it. And if it > wasn't, then there's nothing in debugfs. Yes, I ended up dropping this and having two separate flags. > > > - Autogenerated > > Do we have any way to read them back on those? Usually not. E.g. I don't think I can read back Audio InfoFrame on ADV7511. Nor can I read InfoFrames on LT9611UXC. > > > - Generated by software > > > > E.g. in case of ADV7511 we can declare that Audio InfofFrame is > > autogenerated, AVI, HDMI and SPD as 'software-generated' and DRM (HDR) > > as unsupported. LT9611UXC will declare all (need to check) frame types > > as auto. > > > > This way we can implement the checks and still keep userspace from > > having irrelevant data in debugfs. > > If the only thing you're after is to prevent inconsistent data in > userpace for devices that can generate it automatically, then I guess we > could just implement an (optional) callback to read an infoframe from > the hardware when reading from debugfs. Would that work? As I wrote, this is not always possible, so I'd skip this. > > > I will update my patchset to implement this, but I have another question > > beforehand: should we just declare VSI support or should it be more exact, > > specifying that the driver support HVS (00:0c:03), HVFS (c4:5d:d8), etc? > > I guess you're talking about HDMI 1.4 Vendor specific Infoframe vs HDMI > 2.0 HF-VSIF here? Yes. H14v-VSIF vs HF-VSIF. > > If so, the toggle should be HDMI 2.0 support. We'll need that toggle for > other things anyway (scrambler, YUV420, etc.) > > > I'm asking, because e.g. MSM HDMI controller has hardware support for > > generating HVS frames (but only HVS, the OUI is not programmed, register > > format doesn't match 1:1 frame contents, etc). I instead ended up using > > GENERIC0, because it was more flexible (it's like SPARE packets on > > ADV7511, the contents is being sent as is). However if we ever need to > > send DRM infoframes, we might need to switch from GENERIC0 to HVS, for > > the price of being unable to send HVFS frames. > > Section 10.2 of the HDMI 2.0 states: > > Transmission of the HF-VSIF by Source Devices is optional unless one (or > more) of the features listed in Table 10-1 is active 1. If such features > are active, transmission of the HF-VSIF is mandatory. > > The features in question being 3d. Or ALLM. It's not on my todo list though. > So unless you're supporting 3d, suppporting VSI only seems ok to me. MSM HDMI controllers can support some bits and pieces of 3D. Nobody bothered to implement that though. Maybe I should try getting a monitor which supports stereo output. -- With best wishes Dmitry
On Tue, Sep 02, 2025 at 06:12:39AM +0300, Dmitry Baryshkov wrote: > On Mon, Sep 01, 2025 at 08:54:18AM +0200, Maxime Ripard wrote: > > On Wed, Aug 27, 2025 at 05:04:53PM +0300, Dmitry Baryshkov wrote: > > > On Wed, Aug 27, 2025 at 09:30:20AM +0200, Maxime Ripard wrote: > > > > On Wed, Aug 20, 2025 at 12:52:44PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, Aug 20, 2025 at 09:15:36AM +0200, Maxime Ripard wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, Aug 19, 2025 at 09:57:30PM +0300, Dmitry Baryshkov wrote: > > > > > > > Currently DRM framework expects that the HDMI connector driver supports > > > > > > > all infoframe types: it generates the data as required and calls into > > > > > > > the driver to program all of them, letting the driver to soft-fail if > > > > > > > the infoframe is unsupported. This has a major drawback on userspace > > > > > > > API: the framework also registers debugfs files for all Infoframe types, > > > > > > > possibly surprising the users when infoframe is visible in the debugfs > > > > > > > file, but it is not visible on the wire. > > > > > > > > > > > > > > Let drivers declare that they support only a subset of infoframes, > > > > > > > creating a more consistent interface. > > > > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > > > > > > > > > > I'm not really convinced. Infoframes aren't really something you should > > > > > > ignore, AVI is effectively mandatory, HDMI kind of is too, AUDIO is if > > > > > > audio support is enabled, DRM is mandatory if HDR is used. > > > > > > > > > > Nevertheless, sun4i, innohdmi, adv7511, it6263 and rk3066 drivers > > > > > provide support only for the AVI infoframe. > > > > > > > > Yes, but it's still something we shouldn't paper over. The spec mandates > > > > it, if drivers want to deviate from it it's something we should warn > > > > about, not silence. > > > > > > > > sun4i is a good example, to me at least since I have the doc. The > > > > hardware supports AVI, Audio, ACP, and SPD. HDR isn't supported, so DRM > > > > isn't either. The only missing one is HDMI, but the documentation isn't > > > > the best so it might still be supported. In short, it's a driver issue. > > > > > > > > adv7511 supports AVI, Audio, ACP, SPD, ACP, and looks to have a > > > > mechanism to send any infoframe as is. So, again, driver issue. > > > > > > I've send a patch, enabling SPD and VSI (HDMI) InfoFrames on ADV7511. > > > > > > > > > > > I couldn't find the other datasheet, but I'd be very surprised if it > > > > wasn't the case for these too. > > > > > > > > > Some of them can be extended to support other infoframe kinds (e.g. > > > > > ADV7511 has two spare infoframes which can be used for HDMI and SPD). > > > > > > > > > > > SPD is indeed optional though. > > > > > > > > > > > > So, it's really dynamic in essence, and not really something we should > > > > > > expect drivers to ignore. > > > > > > > > > > > > I do acknowledge that a lot of drivers just silently ignore the > > > > > > infoframes they don't support at the moment, which isn't great either. > > > > > > > > > > > > Maybe we should standardize and document what drivers should do when > > > > > > they don't support a given infoframe type? > > > > > > > > > > The chips might be generating infoframes internally. This series was > > > > > triggered by LT9611UXC, which does all HDMI work under the hood in the > > > > > firmware. See [1]. The series I posted hooks HDMI audio directly into > > > > > the bridge driver, but I'd really prefer to be able to use > > > > > drm_atomic_helper_connector_hdmi_hotplug(), especially if I ever get to > > > > > implementing CEC support for it. > > > > > > > > > > ADV7511 likewise generates audio infoframe without Linux > > > > > help (audio-related fields are programmed, but it's not the > > > > > infoframe itself). > > > > > > > > Implementing the write_infoframe hooks as a nop with a comment in those > > > > case is totally reasonable to me. > > > > > > > > I'd still like to document that drivers should only return 0 if they > > > > programmed the infoframe, and -ENOTSUPP (and the core logging a warning) > > > > otherwise. > > > > > > > > That way, we would be able to differentiate between the legimitate > > > > LT9611UXC case, and the "driver is broken" sun4i (and others) case. > > > > > > I don't want to end up in a sitation where userspace has a different > > > idea of the InfoFrame being sent than the actual one being present on > > > the wire. > > > > It's not ideal, sure, but also, what's wrong with it? We're doing it > > *all the time*. Modes programmed by userspace are adjusted for the > > hardware, and thus the mode reported by the CRTC turns out different > > than the one actually used in hardware. Audio sampling rates might not > > match exactly what we're doing. The quirks infrastructure disables part > > of the EDID the userspace has access to, etc. > > > > And all those are under the userspace control, which the infoframes > > aren't. > > I think there is a differnece between 'change userspace input', > 'knowingly mangle data' and 'lie to userspace because the driver doesn't > care'. This is especially important e.g. if a user is trying to debug > AV issues which can be caused by wrong information in the infoframe. We can play semantics if you want, but the line is *very* fine between "mangling data" and "lying". If the mode has been mangled, and you don't report that it has been to userspace, are you lying? I guess you could argue both ways, but it's really the same than what we're discussing here. Except for the fact that one is a central part of the uapi, that the userspace depends on, and one is a debugfs file that *isn't* a uAPI, and the userspace really shouldn't depend on. > > > It seems, we need several states per the infoframe: > > > > > > - Not supported > > > > Honestly, I'm not sure we need a state for that one. If that infoframe > > was set by the framework, then the driver must support it. And if it > > wasn't, then there's nothing in debugfs. > > Yes, I ended up dropping this and having two separate flags. > > > > > > - Autogenerated > > > > Do we have any way to read them back on those? > > Usually not. E.g. I don't think I can read back Audio InfoFrame on > ADV7511. Nor can I read InfoFrames on LT9611UXC. Then if you don't have access to it, we can just not register the debugfs files. Maxime
On 08/19/2025, Dmitry Baryshkov wrote:
[...]
> @@ -930,23 +947,29 @@ static int write_device_infoframe(struct drm_connector *connector,
> union hdmi_infoframe *frame)
> {
> const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
> + enum hdmi_infoframe_type type = frame->any.type;
> struct drm_device *dev = connector->dev;
> u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
> int ret;
> int len;
>
> - drm_dbg_kms(dev, "Writing infoframe type %x\n", frame->any.type);
> + drm_dbg_kms(dev, "Writing infoframe type %x\n", type);
>
> if (!funcs || !funcs->write_infoframe) {
> drm_dbg_kms(dev, "Function not implemented, bailing.\n");
> return -EINVAL;
> }
>
> + if (!drm_hdmi_connector_supports_infoframe(connector, type)) {
> + drm_dbg_kms(dev, "Infoframe %d not supported, bailing.\n", type);
This '%d' should also be replaced with '0x%02x'.
> + return 0;
> + }
[...]
--
Regards,
Liu Ying
On 08/19/2025, Dmitry Baryshkov wrote: > Currently DRM framework expects that the HDMI connector driver supports > all infoframe types: it generates the data as required and calls into > the driver to program all of them, letting the driver to soft-fail if > the infoframe is unsupported. This has a major drawback on userspace > API: the framework also registers debugfs files for all Infoframe types, > possibly surprising the users when infoframe is visible in the debugfs > file, but it is not visible on the wire. > > Let drivers declare that they support only a subset of infoframes, > creating a more consistent interface. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > --- > drivers/gpu/drm/display/drm_bridge_connector.c | 6 ++++ > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 37 ++++++++++++++++++++-- > drivers/gpu/drm/drm_connector.c | 4 +++ > drivers/gpu/drm/drm_debugfs.c | 16 +++++++--- > drivers/gpu/drm/rockchip/inno_hdmi.c | 1 + > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 1 + > drivers/gpu/drm/tests/drm_connector_test.c | 28 ++++++++++++++++ > drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 8 +++++ > drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +++ > include/drm/drm_bridge.h | 7 ++++ > include/drm/drm_connector.h | 22 +++++++++++++ > 11 files changed, 128 insertions(+), 7 deletions(-) Acked-by: Liu Ying <victor.liu@nxp.com>
© 2016 - 2025 Red Hat, Inc.