2023-08-14 14:45:31

by Maxime Ripard

[permalink] [raw]
Subject: [PATCH RFC 00/13] drm/connector: Create HDMI Connector infrastructure

Hi,

Here's a series that creates a subclass of drm_connector specifically
targeted at HDMI controllers.

The idea behind this series came from a recent discussion on IRC during
which we discussed infoframes generation of i915 vs everything else.

Infoframes generation code still requires some decent boilerplate, with
each driver doing some variation of it.

In parallel, while working on vc4, we ended up converting a lot of i915
logic (mostly around format / bpc selection, and scrambler setup) to
apply on top of a driver that relies only on helpers.

While currently sitting in the vc4 driver, none of that logic actually
relies on any driver or hardware-specific behaviour.

The only missing piec to make it shareable are a bunch of extra
variables stored in a state (current bpc, format, RGB range selection,
etc.).

Thus, I decided to create some generic subclass of drm_connector to
address HDMI connectors, with a bunch of helpers that will take care of
all the "HDMI Spec" related code. Scrambler setup is missing at the
moment but can easily be plugged in.

Last week, Hans Verkuil also expressed interest in retrieving the
infoframes generated from userspace to create an infoframe-decode tool.
This series thus leverages the infoframe generation code to expose it
through debugfs.

This entire series is only build-tested at the moment. Let me know what
you think,
Maxime

Signed-off-by: Maxime Ripard <[email protected]>
---
Maxime Ripard (13):
drm/connector: Introduce an HDMI connector
drm/connector: hdmi: Create a custom state
drm/connector: hdmi: Add Broadcast RGB property
drm/connector: hdmi: Add helper to get the RGB range
drm/connector: hdmi: Add output BPC to the connector state
drm/connector: hdmi: Add support for output format
drm/connector: hdmi: Calculate TMDS character rate
drm/connector: hdmi: Add custom hook to filter TMDS character rate
drm/connector: hdmi: Compute bpc and format automatically
drm/connector: hdmi: Add Infoframes generation
drm/connector: hdmi: Create Infoframe DebugFS entries
drm/vc4: hdmi: Create destroy state implementation
drm/vc4: hdmi: Switch to HDMI connector

drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_hdmi_connector.c | 1112 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_hdmi.c | 720 ++++------------------
drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +-
drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 4 +-
include/drm/drm_connector.h | 256 ++++++++
6 files changed, 1508 insertions(+), 622 deletions(-)
---
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
change-id: 20230814-kms-hdmi-connector-state-616787e67927

Best regards,
--
Maxime Ripard <[email protected]>



2023-08-14 14:47:41

by Maxime Ripard

[permalink] [raw]
Subject: [PATCH RFC 11/13] drm/connector: hdmi: Create Infoframe DebugFS entries

There has been some discussions recently about the infoframes sent by
drivers and if they were properly generated.

In parallel, there's been some interest in creating an infoframe-decode
tool similar to edid-decode.

Both would be much easier if we were to expose the infoframes programmed
in the hardware. It won't be perfect since we have no guarantee that
it's actually what goes through the wire, but it's the best we can do.

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/drm_hdmi_connector.c | 124 +++++++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 4 ++
2 files changed, 128 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 46cafb17def7..dcc45b1080f9 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -907,6 +907,130 @@ void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
}
EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);

+struct debugfs_wrapper {
+ struct drm_hdmi_connector *hdmi_connector;
+ union hdmi_infoframe *frame;
+};
+
+static ssize_t
+infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
+{
+ const struct debugfs_wrapper *wrapper = filp->private_data;
+ struct drm_hdmi_connector *hdmi_connector = wrapper->hdmi_connector;
+ union hdmi_infoframe *frame = wrapper->frame;
+ u8 buf[HDMI_MAX_INFOFRAME_SIZE];
+ ssize_t len;
+
+ len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
+ if (len < 0)
+ return len;
+
+ mutex_lock(&hdmi_connector->infoframes.lock);
+ len = simple_read_from_buffer(ubuf, count, ppos, buf, len);
+ mutex_unlock(&hdmi_connector->infoframes.lock);
+
+ return len;
+}
+
+static const struct file_operations infoframe_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = infoframe_read,
+};
+
+static int create_debugfs_infoframe_file(struct drm_hdmi_connector *hdmi_connector,
+ struct dentry *parent,
+ const char *filename,
+ union hdmi_infoframe *frame)
+{
+ struct drm_device *dev = hdmi_connector->base.dev;
+ struct debugfs_wrapper *wrapper;
+ struct dentry *file;
+
+ wrapper = drmm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
+ if (!wrapper)
+ return -ENOMEM;
+
+ wrapper->hdmi_connector = hdmi_connector;
+ wrapper->frame = frame;
+
+ file = debugfs_create_file(filename, 0400, parent, wrapper, &infoframe_fops);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ return 0;
+}
+
+#define CREATE_INFOFRAME_FILE(c, p, i) \
+ create_debugfs_infoframe_file(c, p, #i, (union hdmi_infoframe *)&(c)->infoframes.i)
+
+static int create_debugfs_infoframe_files(struct drm_hdmi_connector *hdmi_connector,
+ struct dentry *parent)
+{
+ int ret;
+
+ ret = CREATE_INFOFRAME_FILE(hdmi_connector, parent, audio);
+ if (ret)
+ return ret;
+
+ ret = CREATE_INFOFRAME_FILE(hdmi_connector, parent, avi);
+ if (ret)
+ return ret;
+
+ ret = CREATE_INFOFRAME_FILE(hdmi_connector, parent, drm);
+ if (ret)
+ return ret;
+
+ ret = CREATE_INFOFRAME_FILE(hdmi_connector, parent, spd);
+ if (ret)
+ return ret;
+
+ ret = CREATE_INFOFRAME_FILE(hdmi_connector, parent, vendor);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+#undef CREATE_INFOFRAME_FILE
+
+static void remove_debugfs_dir(struct drm_device *dev, void *data)
+{
+ struct dentry *dir = data;
+
+ debugfs_remove_recursive(dir);
+}
+
+/**
+ * drm_helper_hdmi_connector_debugfs_init - DebugFS init for HDMI connectors
+ * @connector: Parent Connector
+ * @dentry: DebugFS root dentry
+ *
+ * Provides a default implementation for
+ * @drm_connector_helper_funcs.debugfs_init that will create all the
+ * files relevant for a @drm_hdmi_connector.
+ */
+void drm_helper_hdmi_connector_debugfs_init(struct drm_connector *connector,
+ struct dentry *root)
+{
+ struct drm_hdmi_connector *hdmi_connector =
+ connector_to_hdmi_connector(connector);
+ struct drm_device *dev = hdmi_connector->base.dev;
+ struct dentry *dir;
+ int ret;
+
+ dir = debugfs_create_dir("infoframes", root);
+ if (IS_ERR(dir))
+ return;
+
+ ret = drmm_add_action_or_reset(dev, remove_debugfs_dir, dir);
+ if (ret)
+ return;
+
+ create_debugfs_infoframe_files(hdmi_connector, dir);
+}
+EXPORT_SYMBOL(drm_helper_hdmi_connector_debugfs_init);
+
/**
* drmm_hdmi_connector_init - Init a preallocated HDMI connector
* @dev: DRM device
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 21da6f428101..e5faaeb35a9d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2294,6 +2294,10 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
struct i2c_adapter *ddc,
unsigned int max_bpc);

+void drm_helper_hdmi_connector_debugfs_init(struct drm_connector *connector,
+ struct dentry *root);
+
+
/**
* struct drm_tile_group - Tile group metadata
* @refcount: reference count

--
2.41.0


2023-08-14 14:51:43

by Maxime Ripard

[permalink] [raw]
Subject: [PATCH RFC 12/13] drm/vc4: hdmi: Create destroy state implementation

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 5261526d286f..ac5debd47e99 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -672,11 +672,21 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
return &new_state->base;
}

+static void vc4_hdmi_connector_destroy_state(struct drm_connector *connector,
+ struct drm_connector_state *state)
+{
+ struct vc4_hdmi_connector_state *vc4_state =
+ conn_state_to_vc4_hdmi_conn_state(state);
+
+ __drm_atomic_helper_connector_destroy_state(state);
+ kfree(vc4_state);
+}
+
static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
.fill_modes = drm_helper_probe_single_connector_modes,
.reset = vc4_hdmi_connector_reset,
.atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .atomic_destroy_state = vc4_hdmi_connector_destroy_state,
.atomic_get_property = vc4_hdmi_connector_get_property,
.atomic_set_property = vc4_hdmi_connector_set_property,
};

--
2.41.0


2023-08-14 14:52:14

by Maxime Ripard

[permalink] [raw]
Subject: [PATCH RFC 03/13] drm/connector: hdmi: Add Broadcast RGB property

The i915 driver has a property to force the RGB range of an HDMI output.
The vc4 driver then implemented the same property with the same
semantics. KWin has support for it, and a PR for mutter is also there to
support it.

Both drivers implementing the same property with the same semantics,
plus the userspace having support for it, is proof enough that it's
pretty much a de-facto standard now and we can provide helpers for it.

Let's plumb it into the newly created HDMI connector.

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/drm_hdmi_connector.c | 167 +++++++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 49 ++++++++++
2 files changed, 216 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index ff825c053b27..2b6b9d444774 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -1,8 +1,11 @@
// SPDX-License-Identifier: GPL-2.0+

+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_state_helper.h>
#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
#include <drm/drm_mode.h>
+#include <drm/drm_print.h>

#include <linux/export.h>

@@ -21,6 +24,8 @@ void __drm_atomic_helper_hdmi_connector_reset(struct drm_hdmi_connector *hdmi_co
struct drm_connector *connector = &hdmi_connector->base;

__drm_atomic_helper_connector_reset(connector, &new_hdmi_state->base);
+
+ new_hdmi_state->broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
}
EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_reset);

@@ -68,7 +73,11 @@ __drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hd
struct drm_hdmi_connector_state *new_hdmi_state)
{
struct drm_connector *connector = &hdmi_connector->base;
+ struct drm_connector_state *old_state = connector->state;
+ struct drm_hdmi_connector_state *old_hdmi_state =
+ connector_state_to_hdmi_connector_state(old_state);

+ new_hdmi_state->broadcast_rgb = old_hdmi_state->broadcast_rgb;
__drm_atomic_helper_connector_duplicate_state(connector, &new_hdmi_state->base);
}
EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_duplicate_state);
@@ -136,6 +145,143 @@ void drm_atomic_helper_hdmi_connector_destroy_state(struct drm_connector *connec
}
EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_destroy_state);

+/**
+ * drm_atomic_helper_hdmi_connector_get_property() - Reads out HDMI connector properties
+ * @connector: the parent connector this state refers to
+ * @state: the parent connector state to destroy
+ * @property: Property instance being queried
+ * @val: Raw value of the property to read into
+ *
+ * Read out a @drm_connector_state property value.
+ *
+ * This helper is meant to be the default
+ * &drm_connector_funcs.atomic_get_property implementation for
+ * @drm_hdmi_connector.
+ */
+int drm_atomic_helper_hdmi_connector_get_property(struct drm_connector *connector,
+ const struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t *val)
+{
+ const struct drm_hdmi_connector *hdmi_connector =
+ connector_to_hdmi_connector(connector);
+ const struct drm_hdmi_connector_state *hdmi_state =
+ connector_state_to_hdmi_connector_state(state);
+ struct drm_device *drm = connector->dev;
+
+ if (property == hdmi_connector->broadcast_rgb_property) {
+ *val = hdmi_state->broadcast_rgb;
+ } else {
+ drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_get_property);
+
+/**
+ * drm_atomic_helper_hdmi_connector_set_property() - Decodes HDMI connector properties
+ * @connector: the parent connector this state refers to
+ * @state: the parent connector state to destroy
+ * @property: Property instance being queried
+ * @val: Raw value of the property to decode
+ *
+ * Decodes a property into an @drm_connector_state.
+ *
+ * This helper is meant to be the default
+ * &drm_connector_funcs.atomic_set_property implementation for
+ * @drm_hdmi_connector.
+ */
+int drm_atomic_helper_hdmi_connector_set_property(struct drm_connector *connector,
+ struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t val)
+{
+ const struct drm_hdmi_connector *hdmi_connector =
+ connector_to_hdmi_connector(connector);
+ struct drm_hdmi_connector_state *hdmi_state =
+ connector_state_to_hdmi_connector_state(state);
+ struct drm_device *drm = connector->dev;
+
+ if (property == hdmi_connector->broadcast_rgb_property) {
+ hdmi_state->broadcast_rgb = val;
+ } else {
+ drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_set_property);
+
+/**
+ * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
+ * @connector: the parent connector this state refers to
+ * @state: the parent connector state to check
+ *
+ * Provides a default connector state check handler for HDMI connectors.
+ * Checks that a desired connector update is valid, and updates various
+ * fields of derived state.
+ *
+ * Drivers that subclass @drm_hdmi_connector_state may still wish to
+ * call this function to avoid duplication of error checking code.
+ *
+ * RETURNS:
+ * Zero on success, or an errno code otherwise.
+ */
+int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector_state *old_state =
+ drm_atomic_get_old_connector_state(state, connector);
+ struct drm_hdmi_connector_state *old_hdmi_state =
+ connector_state_to_hdmi_connector_state(old_state);
+ struct drm_connector_state *new_state =
+ drm_atomic_get_new_connector_state(state, connector);
+ struct drm_hdmi_connector_state *new_hdmi_state =
+ connector_state_to_hdmi_connector_state(new_state);
+
+ if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb) {
+ struct drm_crtc *crtc = new_state->crtc;
+ struct drm_crtc_state *crtc_state;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ crtc_state->mode_changed = true;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_atomic_check);
+
+static const struct drm_prop_enum_list broadcast_rgb_names[] = {
+ { DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
+ { DRM_HDMI_BROADCAST_RGB_FULL, "Full" },
+ { DRM_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
+};
+
+/*
+ * drm_hdmi_connector_get_broadcast_rgb_name - Return a string for HDMI connector RGB broadcast selection
+ * @broadcast_rgb: Broadcast RGB selection to compute name of
+ *
+ * Returns: the name of the Broadcast RGB selection, or NULL if the type
+ * is not valid.
+ */
+const char *
+drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_rgb)
+{
+ if (broadcast_rgb > DRM_HDMI_BROADCAST_RGB_LIMITED)
+ return NULL;
+
+ return broadcast_rgb_names[broadcast_rgb].name;
+}
+EXPORT_SYMBOL(drm_hdmi_connector_get_broadcast_rgb_name);
+
/**
* drm_atomic_helper_hdmi_connector_print_state - Prints a @drm_hdmi_connector_state
* @p: output printer
@@ -147,6 +293,11 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_destroy_state);
void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
const struct drm_connector_state *state)
{
+ const struct drm_hdmi_connector_state *hdmi_state =
+ connector_state_to_hdmi_connector_state(state);
+
+ drm_printf(p, "\tbroadcast_rgb=%s\n",
+ drm_hdmi_connector_get_broadcast_rgb_name(hdmi_state->broadcast_rgb));
}
EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);

@@ -175,6 +326,7 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
struct i2c_adapter *ddc)
{
struct drm_connector *connector = &hdmi_connector->base;
+ struct drm_property *prop;
int ret;

if (connector_type != DRM_MODE_CONNECTOR_HDMIA ||
@@ -185,6 +337,21 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
if (ret)
return ret;

+ prop = hdmi_connector->broadcast_rgb_property;
+ if (!prop) {
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+ "Broadcast RGB",
+ broadcast_rgb_names,
+ ARRAY_SIZE(broadcast_rgb_names));
+ if (!prop)
+ return -EINVAL;
+
+ hdmi_connector->broadcast_rgb_property = prop;
+ }
+
+ drm_object_attach_property(&connector->base, prop,
+ DRM_HDMI_BROADCAST_RGB_AUTO);
+
return 0;
}
EXPORT_SYMBOL(drmm_hdmi_connector_init);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 0aa662e0a6ea..24a0d33e5148 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2042,11 +2042,44 @@ 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);

+/**
+ * enum drm_hdmi_broadcast_rgb - Broadcast RGB Selection for a @drm_hdmi_connector
+ *
+ * This enum is used to track broadcast RGB selection. There are no
+ * separate #defines for the uapi!
+ */
+enum drm_hdmi_broadcast_rgb {
+ /**
+ * @DRM_HDMI_BROADCAST_RGB_AUTO: The RGB range is selected
+ * automatically based on the mode.
+ */
+ DRM_HDMI_BROADCAST_RGB_AUTO,
+
+ /**
+ * @DRM_HDMI_BROADCAST_RGB_FULL: Full range RGB is forced.
+ */
+ DRM_HDMI_BROADCAST_RGB_FULL,
+
+ /**
+ * @DRM_HDMI_BROADCAST_RGB_FULL: Limited range RGB is forced.
+ */
+ DRM_HDMI_BROADCAST_RGB_LIMITED,
+};
+
+const char *
+drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_rgb);
+
struct drm_hdmi_connector_state {
/**
* @base: Base Connector State
*/
struct drm_connector_state base;
+
+ /**
+ * @broadcast_rgb: Connector property to pass the Broadcast RGB
+ * selection value.
+ */
+ enum drm_hdmi_broadcast_rgb broadcast_rgb;
};

#define connector_state_to_hdmi_connector_state(state) \
@@ -2065,6 +2098,16 @@ drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_connector *connector
void __drm_atomic_helper_hdmi_connector_destroy_state(struct drm_hdmi_connector_state *hdmi_state);
void drm_atomic_helper_hdmi_connector_destroy_state(struct drm_connector *connector,
struct drm_connector_state *state);
+int drm_atomic_helper_hdmi_connector_get_property(struct drm_connector *connector,
+ const struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t *val);
+int drm_atomic_helper_hdmi_connector_set_property(struct drm_connector *connector,
+ struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t val);
+int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state);
void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
const struct drm_connector_state *state);

@@ -2073,6 +2116,12 @@ struct drm_hdmi_connector {
* @base: Base Connector
*/
struct drm_connector base;
+
+ /**
+ * @broadcast_rgb_property: Connector property to set the
+ * Broadcast RGB selection to output with.
+ */
+ struct drm_property *broadcast_rgb_property;
};

#define connector_to_hdmi_connector(connector) \

--
2.41.0


2023-08-14 15:42:52

by Simon Ser

[permalink] [raw]
Subject: Re: [PATCH RFC 03/13] drm/connector: hdmi: Add Broadcast RGB property

+1 for a standardized property. wlroots is blocked on the kernel
making this property standard before starting using it.

Can docs be added in the "Standard Connector Properties" section?

2023-08-19 16:13:16

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH RFC 00/13] drm/connector: Create HDMI Connector infrastructure

On 14/08/2023 15:56, Maxime Ripard wrote:
> Hi,
>
> Here's a series that creates a subclass of drm_connector specifically
> targeted at HDMI controllers.
>
> The idea behind this series came from a recent discussion on IRC during
> which we discussed infoframes generation of i915 vs everything else.
>
> Infoframes generation code still requires some decent boilerplate, with
> each driver doing some variation of it.
>
> In parallel, while working on vc4, we ended up converting a lot of i915
> logic (mostly around format / bpc selection, and scrambler setup) to
> apply on top of a driver that relies only on helpers.
>
> While currently sitting in the vc4 driver, none of that logic actually
> relies on any driver or hardware-specific behaviour.
>
> The only missing piec to make it shareable are a bunch of extra
> variables stored in a state (current bpc, format, RGB range selection,
> etc.).
>
> Thus, I decided to create some generic subclass of drm_connector to
> address HDMI connectors, with a bunch of helpers that will take care of
> all the "HDMI Spec" related code. Scrambler setup is missing at the
> moment but can easily be plugged in.
>
> Last week, Hans Verkuil also expressed interest in retrieving the
> infoframes generated from userspace to create an infoframe-decode tool.
> This series thus leverages the infoframe generation code to expose it
> through debugfs.

Some background here: I maintain the edid-decode utility to parse and verify
EDIDs, and an infoframe-decode counterpart would be very nice. I can add
support for exposing infoframes to debugfs in HDMI receivers as well, and
that will help parse and verify received infoframes for correctness.

I added the linux-media mailinglist as well, since this will be of interest
for that subsystem as well.

Regards,

Hans

>
> This entire series is only build-tested at the moment. Let me know what
> you think,
> Maxime
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> Maxime Ripard (13):
> drm/connector: Introduce an HDMI connector
> drm/connector: hdmi: Create a custom state
> drm/connector: hdmi: Add Broadcast RGB property
> drm/connector: hdmi: Add helper to get the RGB range
> drm/connector: hdmi: Add output BPC to the connector state
> drm/connector: hdmi: Add support for output format
> drm/connector: hdmi: Calculate TMDS character rate
> drm/connector: hdmi: Add custom hook to filter TMDS character rate
> drm/connector: hdmi: Compute bpc and format automatically
> drm/connector: hdmi: Add Infoframes generation
> drm/connector: hdmi: Create Infoframe DebugFS entries
> drm/vc4: hdmi: Create destroy state implementation
> drm/vc4: hdmi: Switch to HDMI connector
>
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/drm_hdmi_connector.c | 1112 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/vc4/vc4_hdmi.c | 720 ++++------------------
> drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +-
> drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 4 +-
> include/drm/drm_connector.h | 256 ++++++++
> 6 files changed, 1508 insertions(+), 622 deletions(-)
> ---
> base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
> change-id: 20230814-kms-hdmi-connector-state-616787e67927
>
> Best regards,