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

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

[Maxime Ripard]

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 <mripard@kernel.org>
---
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 <mripard@kernel.org>

[PATCH RFC 01/13] drm/connector: Introduce an HDMI connector

[Maxime Ripard]

A lot of the various HDMI drivers duplicate some logic that depends on
the HDMI spec itself and not really a particular hardware
implementation.

Output BPC or format selection, infoframe generation are good examples
of such areas.

This creates a lot of boilerplate, with a lot of variations, which makes
it hard for userspace to rely on, and makes it difficult to get it right
for drivers.

Let's create a new connector variant specifically dedicated to HDMI
controllers that will allow to abstract away the duplicated logic.

Hopefully, this will make drivers simpler to handle, and their behaviour
more consistent.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/Makefile             |  1 +
 drivers/gpu/drm/drm_hdmi_connector.c | 45 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h          | 16 +++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 7a09a89b493b..1520d4ccd3d7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -27,6 +27,7 @@ drm-y := \
 	drm_fourcc.o \
 	drm_framebuffer.o \
 	drm_gem.o \
+	drm_hdmi_connector.o \
 	drm_ioctl.o \
 	drm_lease.o \
 	drm_managed.o \
diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
new file mode 100644
index 000000000000..62f01dd2e6df
--- /dev/null
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <drm/drm_connector.h>
+#include <drm/drm_mode.h>
+
+#include <linux/export.h>
+
+/**
+ * drmm_hdmi_connector_init - Init a preallocated HDMI connector
+ * @dev: DRM device
+ * @hdmi_connector: A pointer to the HDMI connector to init
+ * @connector_type: user visible type of the connector
+ * @ddc: optional pointer to the associated ddc adapter
+ *
+ * Initialises a preallocated HDMI connector. Connectors can be
+ * subclassed as part of driver connector objects.
+ *
+ * Cleanup is automatically handled with a call to
+ * drm_connector_cleanup() in a DRM-managed action.
+ *
+ * The connector structure should be allocated with drmm_kzalloc().
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drmm_hdmi_connector_init(struct drm_device *dev,
+			     struct drm_hdmi_connector *hdmi_connector,
+			     const struct drm_connector_funcs *funcs,
+			     int connector_type,
+			     struct i2c_adapter *ddc)
+{
+	struct drm_connector *connector = &hdmi_connector->base;
+	int ret;
+
+	if (connector_type != DRM_MODE_CONNECTOR_HDMIA ||
+	    connector_type != DRM_MODE_CONNECTOR_HDMIB)
+		return -EINVAL;
+
+	ret = drmm_connector_init(dev, connector, funcs, connector_type, ddc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drmm_hdmi_connector_init);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index d300fde6c1a4..1859b74083f5 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2042,6 +2042,22 @@ 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);
 
+struct drm_hdmi_connector {
+	/**
+	 * @base: Base Connector
+	 */
+	struct drm_connector base;
+};
+
+#define connector_to_hdmi_connector(connector) \
+	container_of_const(connector, struct drm_hdmi_connector, base)
+
+int drmm_hdmi_connector_init(struct drm_device *dev,
+			     struct drm_hdmi_connector *hdmi_connector,
+			     const struct drm_connector_funcs *funcs,
+			     int connector_type,
+			     struct i2c_adapter *ddc);
+
 /**
  * struct drm_tile_group - Tile group metadata
  * @refcount: reference count

-- 
2.41.0

Re: [PATCH RFC 01/13] drm/connector: Introduce an HDMI connector

[Jani Nikula]

On Mon, 14 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote:
> A lot of the various HDMI drivers duplicate some logic that depends on
> the HDMI spec itself and not really a particular hardware
> implementation.
>
> Output BPC or format selection, infoframe generation are good examples
> of such areas.
>
> This creates a lot of boilerplate, with a lot of variations, which makes
> it hard for userspace to rely on, and makes it difficult to get it right
> for drivers.
>
> Let's create a new connector variant specifically dedicated to HDMI
> controllers that will allow to abstract away the duplicated logic.
>
> Hopefully, this will make drivers simpler to handle, and their behaviour
> more consistent.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/Makefile             |  1 +
>  drivers/gpu/drm/drm_hdmi_connector.c | 45 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h          | 16 +++++++++++++
>  3 files changed, 62 insertions(+)
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 7a09a89b493b..1520d4ccd3d7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -27,6 +27,7 @@ drm-y := \
>  	drm_fourcc.o \
>  	drm_framebuffer.o \
>  	drm_gem.o \
> +	drm_hdmi_connector.o \
>  	drm_ioctl.o \
>  	drm_lease.o \
>  	drm_managed.o \
> diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
> new file mode 100644
> index 000000000000..62f01dd2e6df
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_hdmi_connector.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_mode.h>
> +
> +#include <linux/export.h>
> +
> +/**
> + * drmm_hdmi_connector_init - Init a preallocated HDMI connector
> + * @dev: DRM device
> + * @hdmi_connector: A pointer to the HDMI connector to init
> + * @connector_type: user visible type of the connector
> + * @ddc: optional pointer to the associated ddc adapter
> + *
> + * Initialises a preallocated HDMI connector. Connectors can be
> + * subclassed as part of driver connector objects.
> + *
> + * Cleanup is automatically handled with a call to
> + * drm_connector_cleanup() in a DRM-managed action.
> + *
> + * The connector structure should be allocated with drmm_kzalloc().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drmm_hdmi_connector_init(struct drm_device *dev,
> +			     struct drm_hdmi_connector *hdmi_connector,
> +			     const struct drm_connector_funcs *funcs,
> +			     int connector_type,
> +			     struct i2c_adapter *ddc)
> +{
> +	struct drm_connector *connector = &hdmi_connector->base;
> +	int ret;
> +
> +	if (connector_type != DRM_MODE_CONNECTOR_HDMIA ||
> +	    connector_type != DRM_MODE_CONNECTOR_HDMIB)
> +		return -EINVAL;
> +
> +	ret = drmm_connector_init(dev, connector, funcs, connector_type, ddc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drmm_hdmi_connector_init);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index d300fde6c1a4..1859b74083f5 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2042,6 +2042,22 @@ 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);
>  
> +struct drm_hdmi_connector {
> +	/**
> +	 * @base: Base Connector
> +	 */
> +	struct drm_connector base;
> +};
> +
> +#define connector_to_hdmi_connector(connector) \
> +	container_of_const(connector, struct drm_hdmi_connector, base)
> +
> +int drmm_hdmi_connector_init(struct drm_device *dev,
> +			     struct drm_hdmi_connector *hdmi_connector,
> +			     const struct drm_connector_funcs *funcs,
> +			     int connector_type,
> +			     struct i2c_adapter *ddc);
> +

Pure bikeshedding, maybe add drm_hdmi_connector.h from the start so we
don't have to split this up later.

BR,
Jani.

>  /**
>   * struct drm_tile_group - Tile group metadata
>   * @refcount: reference count

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH RFC 02/13] drm/connector: hdmi: Create a custom state

[Maxime Ripard]

The next features we will need to share across drivers will need to
store some parameters for drivers to use, such as the selected output
format.

Let's create a new connector state dedicated to HDMI controllers, that
will eventually store everything we need.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 145 +++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h          |  26 +++++++
 2 files changed, 171 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 62f01dd2e6df..ff825c053b27 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -1,10 +1,155 @@
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_mode.h>
 
 #include <linux/export.h>
 
+/**
+ * __drm_atomic_helper_hdmi_connector_reset() - Initializes all @drm_hdmi_connector_state resources
+ * @hdmi_connector: the connector this state refers to
+ * @new_hdmi_state: the HDMI connector state to initialize
+ *
+ * Initializes all relevant resources from a @drm_hdmi_connector_state
+ * without actually allocating it. This is useful for drivers that
+ * subclass @drm_hdmi_connector_state.
+ */
+void __drm_atomic_helper_hdmi_connector_reset(struct drm_hdmi_connector *hdmi_connector,
+					      struct drm_hdmi_connector_state *new_hdmi_state)
+{
+	struct drm_connector *connector = &hdmi_connector->base;
+
+	__drm_atomic_helper_connector_reset(connector, &new_hdmi_state->base);
+}
+EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_reset);
+
+/**
+ * drm_atomic_helper_hdmi_connector_reset() - Create a @drm_hdmi_connector_state object
+ * @connector: the parent connector
+ *
+ * This helper is meant to be the default &drm_connector_funcs.reset
+ * implementation for @drm_hdmi_connector that don't subclass
+ * @drm_hdmi_connector_state.
+ */
+void drm_atomic_helper_hdmi_connector_reset(struct drm_connector *connector)
+{
+	struct drm_hdmi_connector *hdmi_connector =
+		connector_to_hdmi_connector(connector);
+	struct drm_connector_state *old_state = connector->state;
+	struct drm_hdmi_connector_state *old_hdmi_state =
+		connector_state_to_hdmi_connector_state(old_state);
+	struct drm_hdmi_connector_state *new_hdmi_state;
+
+	if (old_state)
+		__drm_atomic_helper_connector_destroy_state(old_state);
+
+	kfree(old_hdmi_state);
+
+	new_hdmi_state = kzalloc(sizeof(*new_hdmi_state), GFP_KERNEL);
+	if (!new_hdmi_state)
+		return;
+
+	__drm_atomic_helper_hdmi_connector_reset(hdmi_connector, new_hdmi_state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_reset);
+
+/**
+ * __drm_atomic_helper_hdmi_connector_duplicate_state() - Copies all @drm_hdmi_connector_state resources
+ * @hdmi_connector: the connector this state refers to
+ * @new_hdmi_state: the HDMI connector state to copy to
+ *
+ * Copies all relevant resources from a @drm_hdmi_connector_state to a
+ * new one without actually allocating it. This is useful for drivers
+ * that subclass @drm_hdmi_connector_state.
+ */
+void
+__drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hdmi_connector,
+						   struct drm_hdmi_connector_state *new_hdmi_state)
+{
+	struct drm_connector *connector = &hdmi_connector->base;
+
+	__drm_atomic_helper_connector_duplicate_state(connector, &new_hdmi_state->base);
+}
+EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_duplicate_state);
+
+/**
+ * drm_atomic_helper_hdmi_connector_duplicate_state() - Duplicate a @drm_hdmi_connector_state object
+ * @connector: the parent connector this state refers to
+ *
+ * This helper is meant to be the default
+ * &drm_connector_funcs.atomic_duplicate_state implementation for
+ * @drm_hdmi_connector that don't subclass @drm_hdmi_connector_state.
+ */
+struct drm_connector_state *
+drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_connector *connector)
+{
+	struct drm_hdmi_connector *hdmi_connector =
+		connector_to_hdmi_connector(connector);
+	struct drm_hdmi_connector_state *new_hdmi_state;
+
+	new_hdmi_state = kzalloc(sizeof(*new_hdmi_state), GFP_KERNEL);
+	if (!new_hdmi_state)
+		return NULL;
+
+	__drm_atomic_helper_hdmi_connector_duplicate_state(hdmi_connector, new_hdmi_state);
+
+	return &new_hdmi_state->base;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_duplicate_state);
+
+/**
+ * __drm_atomic_helper_hdmi_connector_destroy_state() - Releases all @drm_hdmi_connector_state resources
+ * @hdmi_state: the HDMI connector state to release
+ *
+ * Release all resources stored in @drm_hdmi_connector_state without
+ * actually freeing it. This is useful for drivers that subclass
+ * @drm_hdmi_connector_state.
+ */
+void __drm_atomic_helper_hdmi_connector_destroy_state(struct drm_hdmi_connector_state *hdmi_state)
+{
+	__drm_atomic_helper_connector_destroy_state(&hdmi_state->base);
+}
+EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_destroy_state);
+
+/**
+ * drm_atomic_helper_hdmi_connector_destroy_state() - Destroy a @drm_hdmi_connector_state object
+ * @connector: the parent connector this state refers to
+ * @state: the parent connector state to destroy
+ *
+ * Destroys an HDMI connector state previously created by
+ * &drm_atomic_helper_hdmi_connector_reset() or
+ * &drm_atomic_helper_hdmi_connector_duplicate_state().
+ *
+ * This helper is meant to be the default
+ * &drm_connector_funcs.atomic_destroy_state implementation for
+ * @drm_hdmi_connector that don't subclass @drm_hdmi_connector_state.
+ */
+void drm_atomic_helper_hdmi_connector_destroy_state(struct drm_connector *connector,
+						    struct drm_connector_state *state)
+{
+	struct drm_hdmi_connector_state *hdmi_state =
+		connector_state_to_hdmi_connector_state(state);
+
+	__drm_atomic_helper_hdmi_connector_destroy_state(hdmi_state);
+	kfree(hdmi_state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_destroy_state);
+
+/**
+ * drm_atomic_helper_hdmi_connector_print_state - Prints a @drm_hdmi_connector_state
+ * @p: output printer
+ * @state: Connector state to print
+ *
+ * Default implementation of @drm_connector_funcs.atomic_print_state for
+ * a @drm_hdmi_connector_state.
+ */
+void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
+						  const struct drm_connector_state *state)
+{
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
+
 /**
  * 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 1859b74083f5..0aa662e0a6ea 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2042,6 +2042,32 @@ 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);
 
+struct drm_hdmi_connector_state {
+	/**
+	 * @base: Base Connector State
+	 */
+	struct drm_connector_state base;
+};
+
+#define connector_state_to_hdmi_connector_state(state) \
+	container_of_const(state, struct drm_hdmi_connector_state, base)
+
+struct drm_hdmi_connector;
+
+void __drm_atomic_helper_hdmi_connector_reset(struct drm_hdmi_connector *hdmi_connector,
+					      struct drm_hdmi_connector_state *new_hdmi_state);
+void drm_atomic_helper_hdmi_connector_reset(struct drm_connector *connector);
+void
+__drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hdmi_connector,
+						   struct drm_hdmi_connector_state *new_hdmi_state);
+struct drm_connector_state *
+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);
+void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
+						  const struct drm_connector_state *state);
+
 struct drm_hdmi_connector {
 	/**
 	 * @base: Base Connector

-- 
2.41.0

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

[Maxime Ripard]

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 <mripard@kernel.org>
---
 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

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

[Simon Ser]

+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?

[PATCH RFC 04/13] drm/connector: hdmi: Add helper to get the RGB range

[Maxime Ripard]

HDMI controller drivers will need to figure out the RGB range they need
to configure based on a given atomic state.

Let's provide a helper to provide that information.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 41 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h          |  4 ++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 2b6b9d444774..1fe3f3ebf719 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -4,6 +4,7 @@
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_print.h>
 
@@ -217,6 +218,15 @@ int drm_atomic_helper_hdmi_connector_set_property(struct drm_connector *connecto
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_set_property);
 
+static const struct drm_display_mode *
+connector_state_get_adjusted_mode(const struct drm_connector_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc_state *crtc_state = crtc->state;
+
+	return &crtc_state->adjusted_mode;
+}
+
 /**
  * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
  * @connector: the parent connector this state refers to
@@ -259,6 +269,37 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_atomic_check);
 
+/**
+ * drm_atomic_helper_hdmi_connector_is_full_range() - Checks whether a state uses Full-Range RGB
+ * @hdmi_connector: the HDMI connector this state refers to
+ * @hdmi_state: the HDMI connector state to check
+ *
+ * RETURNS:
+ * True if @hdmi_state requires a Full range RGB output, False otherwise
+ */
+bool
+drm_atomic_helper_hdmi_connector_is_full_range(const struct drm_hdmi_connector *hdmi_connector,
+					       const struct drm_hdmi_connector_state *hdmi_state)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_connector_state *conn_state = &hdmi_state->base;
+	const struct drm_display_mode *mode =
+		connector_state_get_adjusted_mode(conn_state);
+	const struct drm_display_info *display = &connector->display_info;
+
+	if (hdmi_state->broadcast_rgb == DRM_HDMI_BROADCAST_RGB_FULL)
+		return true;
+
+	if (hdmi_state->broadcast_rgb == DRM_HDMI_BROADCAST_RGB_LIMITED)
+		return false;
+
+	if (!display->is_hdmi)
+		return true;
+
+	return drm_default_rgb_quant_range(mode);
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_is_full_range);
+
 static const struct drm_prop_enum_list broadcast_rgb_names[] = {
 	{ DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
 	{ DRM_HDMI_BROADCAST_RGB_FULL, "Full" },
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 24a0d33e5148..dff95e6e2b48 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2111,6 +2111,10 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
 						  const struct drm_connector_state *state);
 
+bool
+drm_atomic_helper_hdmi_connector_is_full_range(const struct drm_hdmi_connector *hdmi_connector,
+					       const struct drm_hdmi_connector_state *hdmi_state);
+
 struct drm_hdmi_connector {
 	/**
 	 * @base: Base Connector

-- 
2.41.0

[PATCH RFC 05/13] drm/connector: hdmi: Add output BPC to the connector state

[Maxime Ripard]

We'll add automatic selection of the output BPC in a following patch,
but let's add it to the HDMI connector state already.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 21 +++++++++++++++++++--
 include/drm/drm_connector.h          | 13 ++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 1fe3f3ebf719..6d5535e613c6 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -26,6 +26,9 @@ void __drm_atomic_helper_hdmi_connector_reset(struct drm_hdmi_connector *hdmi_co
 
 	__drm_atomic_helper_connector_reset(connector, &new_hdmi_state->base);
 
+	new_hdmi_state->base.max_bpc = 8;
+	new_hdmi_state->base.max_requested_bpc = 8;
+	new_hdmi_state->output_bpc = 8;
 	new_hdmi_state->broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_reset);
@@ -78,6 +81,7 @@ __drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hd
 	struct drm_hdmi_connector_state *old_hdmi_state =
 		connector_state_to_hdmi_connector_state(old_state);
 
+	new_hdmi_state->output_bpc = old_hdmi_state->output_bpc;
 	new_hdmi_state->broadcast_rgb = old_hdmi_state->broadcast_rgb;
 	__drm_atomic_helper_connector_duplicate_state(connector, &new_hdmi_state->base);
 }
@@ -254,7 +258,8 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 	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) {
+	if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||
+	    old_hdmi_state->output_bpc != new_hdmi_state->output_bpc) {
 		struct drm_crtc *crtc = new_state->crtc;
 		struct drm_crtc_state *crtc_state;
 
@@ -339,6 +344,7 @@ void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
 
 	drm_printf(p, "\tbroadcast_rgb=%s\n",
 		   drm_hdmi_connector_get_broadcast_rgb_name(hdmi_state->broadcast_rgb));
+	drm_printf(p, "\toutput_bpc=%u\n", hdmi_state->output_bpc);
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
 
@@ -348,6 +354,7 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
  * @hdmi_connector: A pointer to the HDMI connector to init
  * @connector_type: user visible type of the connector
  * @ddc: optional pointer to the associated ddc adapter
+ * @max_bpc: Maximum bits per char the HDMI connector supports
  *
  * Initialises a preallocated HDMI connector. Connectors can be
  * subclassed as part of driver connector objects.
@@ -364,7 +371,8 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
 			     const struct drm_connector_funcs *funcs,
 			     int connector_type,
-			     struct i2c_adapter *ddc)
+			     struct i2c_adapter *ddc,
+			     unsigned int max_bpc)
 {
 	struct drm_connector *connector = &hdmi_connector->base;
 	struct drm_property *prop;
@@ -393,6 +401,15 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
 	drm_object_attach_property(&connector->base, prop,
 				   DRM_HDMI_BROADCAST_RGB_AUTO);
 
+	if (max_bpc) {
+		if (!(max_bpc == 8 || max_bpc == 10 || max_bpc == 12))
+			return -EINVAL;
+
+		drm_connector_attach_hdr_output_metadata_property(connector);
+		drm_connector_attach_max_bpc_property(connector, 8, max_bpc);
+		hdmi_connector->max_bpc = max_bpc;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drmm_hdmi_connector_init);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index dff95e6e2b48..353aa8e5a117 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2075,6 +2075,11 @@ struct drm_hdmi_connector_state {
 	 */
 	struct drm_connector_state base;
 
+	/**
+	 * @output_bpc: Bits per character to output.
+	 */
+	unsigned int output_bpc;
+
 	/**
 	 * @broadcast_rgb: Connector property to pass the Broadcast RGB
 	 * selection value.
@@ -2121,6 +2126,11 @@ struct drm_hdmi_connector {
 	 */
 	struct drm_connector base;
 
+	/**
+	 * @max_bpc: Maximum bits per character the connector supports.
+	 */
+	unsigned int max_bpc;
+
 	/**
 	 * @broadcast_rgb_property: Connector property to set the
 	 * Broadcast RGB selection to output with.
@@ -2135,7 +2145,8 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
 			     const struct drm_connector_funcs *funcs,
 			     int connector_type,
-			     struct i2c_adapter *ddc);
+			     struct i2c_adapter *ddc,
+			     unsigned int max_bpc);
 
 /**
  * struct drm_tile_group - Tile group metadata

-- 
2.41.0

[PATCH RFC 06/13] drm/connector: hdmi: Add support for output format

[Maxime Ripard]

Just like BPC, we'll add support for automatic selection of the output
format for HDMI connectors.

Let's add the needed defaults and fields for now.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 29 +++++++++++++++++++++++++++++
 include/drm/drm_connector.h          |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 6d5535e613c6..55f685c0095b 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -29,6 +29,7 @@ void __drm_atomic_helper_hdmi_connector_reset(struct drm_hdmi_connector *hdmi_co
 	new_hdmi_state->base.max_bpc = 8;
 	new_hdmi_state->base.max_requested_bpc = 8;
 	new_hdmi_state->output_bpc = 8;
+	new_hdmi_state->output_format = HDMI_COLORSPACE_RGB;
 	new_hdmi_state->broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_hdmi_connector_reset);
@@ -82,6 +83,7 @@ __drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hd
 		connector_state_to_hdmi_connector_state(old_state);
 
 	new_hdmi_state->output_bpc = old_hdmi_state->output_bpc;
+	new_hdmi_state->output_format = old_hdmi_state->output_format;
 	new_hdmi_state->broadcast_rgb = old_hdmi_state->broadcast_rgb;
 	__drm_atomic_helper_connector_duplicate_state(connector, &new_hdmi_state->base);
 }
@@ -222,6 +224,30 @@ int drm_atomic_helper_hdmi_connector_set_property(struct drm_connector *connecto
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_set_property);
 
+static const char * const output_format_str[] = {
+	[HDMI_COLORSPACE_RGB]		= "RGB",
+	[HDMI_COLORSPACE_YUV420]	= "YUV 4:2:0",
+	[HDMI_COLORSPACE_YUV422]	= "YUV 4:2:2",
+	[HDMI_COLORSPACE_YUV444]	= "YUV 4:4:4",
+};
+
+/*
+ * drm_hdmi_connector_get_output_format_name() - Return a string for HDMI connector output format
+ * @fmt: Output format to compute name of
+ *
+ * Returns: the name of the output format, or NULL if the type is not
+ * valid.
+ */
+const char *
+drm_hdmi_connector_get_output_format_name(enum hdmi_colorspace fmt)
+{
+	if (fmt >= ARRAY_SIZE(output_format_str))
+		return NULL;
+
+	return output_format_str[fmt];
+}
+EXPORT_SYMBOL(drm_hdmi_connector_get_output_format_name);
+
 static const struct drm_display_mode *
 connector_state_get_adjusted_mode(const struct drm_connector_state *state)
 {
@@ -259,6 +285,7 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 		connector_state_to_hdmi_connector_state(new_state);
 
 	if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||
+	    old_hdmi_state->output_format != new_hdmi_state->output_format ||
 	    old_hdmi_state->output_bpc != new_hdmi_state->output_bpc) {
 		struct drm_crtc *crtc = new_state->crtc;
 		struct drm_crtc_state *crtc_state;
@@ -345,6 +372,8 @@ void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
 	drm_printf(p, "\tbroadcast_rgb=%s\n",
 		   drm_hdmi_connector_get_broadcast_rgb_name(hdmi_state->broadcast_rgb));
 	drm_printf(p, "\toutput_bpc=%u\n", hdmi_state->output_bpc);
+	drm_printf(p, "\toutput_format=%s\n",
+		   drm_hdmi_connector_get_output_format_name(hdmi_state->output_format));
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 353aa8e5a117..995700110a16 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2080,6 +2080,11 @@ struct drm_hdmi_connector_state {
 	 */
 	unsigned int output_bpc;
 
+	/**
+	 * @output_format: Pixel format to output in.
+	 */
+	enum hdmi_colorspace output_format;
+
 	/**
 	 * @broadcast_rgb: Connector property to pass the Broadcast RGB
 	 * selection value.

-- 
2.41.0

[PATCH RFC 07/13] drm/connector: hdmi: Calculate TMDS character rate

[Maxime Ripard]

Most HDMI drivers have some code to calculate the TMDS character rate,
usually to adjust an internal clock to match what the mode requires.

Since the TMDS character rates mostly depends on the resolution, whether
we need to repeat pixels or not, the bpc count and the format, we can
now derive it from the HDMI connector state that stores all those infos
and remove the duplication from drivers.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 77 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h          | 11 ++++++
 2 files changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 55f685c0095b..e49782a284a5 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -82,6 +82,7 @@ __drm_atomic_helper_hdmi_connector_duplicate_state(struct drm_hdmi_connector *hd
 	struct drm_hdmi_connector_state *old_hdmi_state =
 		connector_state_to_hdmi_connector_state(old_state);
 
+	new_hdmi_state->tmds_char_rate = old_hdmi_state->tmds_char_rate;
 	new_hdmi_state->output_bpc = old_hdmi_state->output_bpc;
 	new_hdmi_state->output_format = old_hdmi_state->output_format;
 	new_hdmi_state->broadcast_rgb = old_hdmi_state->broadcast_rgb;
@@ -257,6 +258,69 @@ connector_state_get_adjusted_mode(const struct drm_connector_state *state)
 	return &crtc_state->adjusted_mode;
 }
 
+static enum drm_mode_status
+drm_hdmi_connector_clock_valid(const struct drm_hdmi_connector *hdmi_connector,
+			       const struct drm_display_mode *mode,
+			       unsigned long long clock)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_display_info *info = &connector->display_info;
+
+	if (info->max_tmds_clock && clock > info->max_tmds_clock * 1000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+/**
+ * drm_hdmi_connector_compute_mode_clock() - Computes the TMDS Character Rate
+ * @mode: Display mode to compute the clock for
+ * @bpc: Bits per character
+ * @fmt: Output Pixel Format used
+ *
+ * Returns the TMDS Character Rate for a given mode, bpc count and output format.
+ *
+ * RETURNS:
+ * The TMDS Character Rate, in Hertz
+ */
+unsigned long long
+drm_hdmi_connector_compute_mode_clock(const struct drm_display_mode *mode,
+				      unsigned int bpc,
+				      enum hdmi_colorspace fmt)
+{
+	unsigned long long clock = mode->clock * 1000ULL;
+
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+		clock = clock * 2;
+
+	if (fmt == HDMI_COLORSPACE_YUV422)
+		bpc = 8;
+
+	clock = clock * bpc;
+	do_div(clock, 8);
+
+	return clock;
+}
+EXPORT_SYMBOL(drm_hdmi_connector_compute_mode_clock);
+
+static int
+drm_hdmi_connector_compute_clock(const struct drm_hdmi_connector *hdmi_connector,
+				 struct drm_hdmi_connector_state *hdmi_state,
+				 const struct drm_display_mode *mode,
+				 unsigned int bpc,
+				 enum hdmi_colorspace fmt)
+{
+	unsigned long long clock;
+
+	clock = drm_hdmi_connector_compute_mode_clock(mode, bpc, fmt);
+	if (!drm_hdmi_connector_clock_valid(hdmi_connector, mode, clock) != MODE_OK)
+		return -EINVAL;
+
+	hdmi_state->tmds_char_rate = clock;
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
  * @connector: the parent connector this state refers to
@@ -275,6 +339,8 @@ connector_state_get_adjusted_mode(const struct drm_connector_state *state)
 int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connector,
 						  struct drm_atomic_state *state)
 {
+	struct drm_hdmi_connector *hdmi_connector =
+		connector_to_hdmi_connector(connector);
 	struct drm_connector_state *old_state =
 		drm_atomic_get_old_connector_state(state, connector);
 	struct drm_hdmi_connector_state *old_hdmi_state =
@@ -283,6 +349,16 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 		drm_atomic_get_new_connector_state(state, connector);
 	struct drm_hdmi_connector_state *new_hdmi_state =
 		connector_state_to_hdmi_connector_state(new_state);
+	const struct drm_display_mode *mode =
+		connector_state_get_adjusted_mode(new_state);
+	int ret;
+
+	ret = drm_hdmi_connector_compute_clock(hdmi_connector, new_hdmi_state,
+					       mode,
+					       new_hdmi_state->output_bpc,
+					       new_hdmi_state->output_format);
+	if (!ret)
+		return ret;
 
 	if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||
 	    old_hdmi_state->output_format != new_hdmi_state->output_format ||
@@ -374,6 +450,7 @@ void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
 	drm_printf(p, "\toutput_bpc=%u\n", hdmi_state->output_bpc);
 	drm_printf(p, "\toutput_format=%s\n",
 		   drm_hdmi_connector_get_output_format_name(hdmi_state->output_format));
+	drm_printf(p, "\ttmds_char_rate=%llu\n", hdmi_state->tmds_char_rate);
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 995700110a16..03c5af34323d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -38,6 +38,7 @@ struct drm_connector_helper_funcs;
 struct drm_modeset_acquire_ctx;
 struct drm_device;
 struct drm_crtc;
+struct drm_display_mode;
 struct drm_encoder;
 struct drm_panel;
 struct drm_property;
@@ -2085,6 +2086,11 @@ struct drm_hdmi_connector_state {
 	 */
 	enum hdmi_colorspace output_format;
 
+	/**
+	 * @tmds_char_rate: TMDS Character Rate, in Hz.
+	 */
+	unsigned long long tmds_char_rate;
+
 	/**
 	 * @broadcast_rgb: Connector property to pass the Broadcast RGB
 	 * selection value.
@@ -2146,6 +2152,11 @@ struct drm_hdmi_connector {
 #define connector_to_hdmi_connector(connector) \
 	container_of_const(connector, struct drm_hdmi_connector, base)
 
+unsigned long long
+drm_hdmi_connector_compute_mode_clock(const struct drm_display_mode *mode,
+				      unsigned int bpc,
+				      enum hdmi_colorspace fmt);
+
 int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
 			     const struct drm_connector_funcs *funcs,

-- 
2.41.0

[PATCH RFC 08/13] drm/connector: hdmi: Add custom hook to filter TMDS character rate

[Maxime Ripard]

Most of the HDMI controllers have an upper TMDS character rate limit
they can't exceed. On "embedded"-grade display controllers, it will
typically be lower than what high-grade monitors can provide these days,
so drivers will filter the TMDS character rate based on the controller
capabilities.

To make that easier to handle for drivers, let's provide an optional
hook to be implemented by drivers so they can tell the HDMI controller
helpers if a given TMDS character rate is reachable for them or not.

This will then be useful to figure out the best format and bpc count for
a given mode.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c |  8 ++++++++
 include/drm/drm_connector.h          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index e49782a284a5..d94ceeb6a8ef 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -263,12 +263,16 @@ drm_hdmi_connector_clock_valid(const struct drm_hdmi_connector *hdmi_connector,
 			       const struct drm_display_mode *mode,
 			       unsigned long long clock)
 {
+	const struct drm_hdmi_connector_funcs *funcs = hdmi_connector->funcs;
 	const struct drm_connector *connector = &hdmi_connector->base;
 	const struct drm_display_info *info = &connector->display_info;
 
 	if (info->max_tmds_clock && clock > info->max_tmds_clock * 1000)
 		return MODE_CLOCK_HIGH;
 
+	if (funcs && funcs->tmds_char_rate_valid)
+		return funcs->tmds_char_rate_valid(hdmi_connector, mode, clock);
+
 	return MODE_OK;
 }
 
@@ -458,6 +462,7 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
  * drmm_hdmi_connector_init - Init a preallocated HDMI connector
  * @dev: DRM device
  * @hdmi_connector: A pointer to the HDMI connector to init
+ * @funcs: callbacks for this connector
  * @connector_type: user visible type of the connector
  * @ddc: optional pointer to the associated ddc adapter
  * @max_bpc: Maximum bits per char the HDMI connector supports
@@ -476,6 +481,7 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
 int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
 			     const struct drm_connector_funcs *funcs,
+			     const struct drm_hdmi_connector_funcs *hdmi_funcs,
 			     int connector_type,
 			     struct i2c_adapter *ddc,
 			     unsigned int max_bpc)
@@ -516,6 +522,8 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
 		hdmi_connector->max_bpc = max_bpc;
 	}
 
+	hdmi_connector->funcs = hdmi_funcs;
+
 	return 0;
 }
 EXPORT_SYMBOL(drmm_hdmi_connector_init);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 03c5af34323d..6e25a16420e4 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2131,12 +2131,41 @@ bool
 drm_atomic_helper_hdmi_connector_is_full_range(const struct drm_hdmi_connector *hdmi_connector,
 					       const struct drm_hdmi_connector_state *hdmi_state);
 
+/**
+ * struct drm_hdmi_connector_funcs - drm_hdmi_connector control functions
+ */
+struct drm_hdmi_connector_funcs {
+	/**
+	 * @tmds_char_rate_valid:
+	 *
+	 * This callback is invoked at atomic_check time to figure out
+	 * whether a particular TMDS character rate is supported by the
+	 * driver.
+	 *
+	 * The @tmds_char_rate_valid callback is optional.
+	 *
+	 * Returns:
+	 *
+	 * Either &drm_mode_status.MODE_OK or one of the failure reasons
+	 * in &enum drm_mode_status.
+	 */
+	enum drm_mode_status
+	(*tmds_char_rate_valid)(const struct drm_hdmi_connector *connector,
+				const struct drm_display_mode *mode,
+				unsigned long long tmds_rate);
+};
+
 struct drm_hdmi_connector {
 	/**
 	 * @base: Base Connector
 	 */
 	struct drm_connector base;
 
+	/**
+	 * @funcs: HDMI connector Control Functions
+	 */
+	const struct drm_hdmi_connector_funcs *funcs;
+
 	/**
 	 * @max_bpc: Maximum bits per character the connector supports.
 	 */
@@ -2160,6 +2189,7 @@ drm_hdmi_connector_compute_mode_clock(const struct drm_display_mode *mode,
 int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
 			     const struct drm_connector_funcs *funcs,
+			     const struct drm_hdmi_connector_funcs *hdmi_funcs,
 			     int connector_type,
 			     struct i2c_adapter *ddc,
 			     unsigned int max_bpc);

-- 
2.41.0

[PATCH RFC 09/13] drm/connector: hdmi: Compute bpc and format automatically

[Maxime Ripard]

Now that we have all the infrastructure needed, we can add some code
that will, for a given connector state and mode, compute the best output
format and bpc.

The algorithm is the same one than the one already found in i915 and
vc4.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 182 ++++++++++++++++++++++++++++++++++-
 1 file changed, 177 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index d94ceeb6a8ef..22c49906dfb5 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -258,6 +258,91 @@ connector_state_get_adjusted_mode(const struct drm_connector_state *state)
 	return &crtc_state->adjusted_mode;
 }
 
+static bool
+sink_supports_format_bpc(const struct drm_hdmi_connector *hdmi_connector,
+			 const struct drm_display_info *info,
+			 const struct drm_display_mode *mode,
+			 unsigned int format, unsigned int bpc)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_device *dev = connector->dev;
+	u8 vic = drm_match_cea_mode(mode);
+
+	if (vic == 1 && bpc != 8) {
+		drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
+		return false;
+	}
+
+	if (!info->is_hdmi &&
+	    (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
+		drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
+		return false;
+	}
+
+	switch (format) {
+	case HDMI_COLORSPACE_RGB:
+		drm_dbg(dev, "RGB Format, checking the constraints.\n");
+
+		if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
+			return false;
+
+		if (bpc == 10 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30)) {
+			drm_dbg(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
+			return false;
+		}
+
+		if (bpc == 12 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_36)) {
+			drm_dbg(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
+			return false;
+		}
+
+		drm_dbg(dev, "RGB format supported in that configuration.\n");
+
+		return true;
+
+	case HDMI_COLORSPACE_YUV422:
+		drm_dbg(dev, "YUV422 format, checking the constraints.\n");
+
+		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR422)) {
+			drm_dbg(dev, "Sink doesn't support YUV422.\n");
+			return false;
+		}
+
+		if (bpc != 12) {
+			drm_dbg(dev, "YUV422 only supports 12 bpc.\n");
+			return false;
+		}
+
+		drm_dbg(dev, "YUV422 format supported in that configuration.\n");
+
+		return true;
+
+	case HDMI_COLORSPACE_YUV444:
+		drm_dbg(dev, "YUV444 format, checking the constraints.\n");
+
+		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR444)) {
+			drm_dbg(dev, "Sink doesn't support YUV444.\n");
+			return false;
+		}
+
+		if (bpc == 10 && !(info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_30)) {
+			drm_dbg(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
+			return false;
+		}
+
+		if (bpc == 12 && !(info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_36)) {
+			drm_dbg(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
+			return false;
+		}
+
+		drm_dbg(dev, "YUV444 format supported in that configuration.\n");
+
+		return true;
+	}
+
+	return false;
+}
+
 static enum drm_mode_status
 drm_hdmi_connector_clock_valid(const struct drm_hdmi_connector *hdmi_connector,
 			       const struct drm_display_mode *mode,
@@ -325,6 +410,95 @@ drm_hdmi_connector_compute_clock(const struct drm_hdmi_connector *hdmi_connector
 	return 0;
 }
 
+static bool
+drm_hdmi_connector_try_format_bpc(const struct drm_hdmi_connector *hdmi_connector,
+				  struct drm_hdmi_connector_state *hdmi_state,
+				  const struct drm_display_mode *mode,
+				  unsigned int bpc, enum hdmi_colorspace fmt)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_display_info *info = &connector->display_info;
+	struct drm_device *dev = connector->dev;
+	int ret;
+
+	drm_dbg(dev, "Trying output format %s\n",
+		drm_hdmi_connector_get_output_format_name(fmt));
+
+	if (!sink_supports_format_bpc(hdmi_connector, info, mode, fmt, bpc))
+		return false;
+
+	ret = drm_hdmi_connector_compute_clock(hdmi_connector, hdmi_state,
+					       mode, bpc, fmt);
+	if (ret)
+		return false;
+
+	return true;
+}
+
+static int
+drm_hdmi_connector_compute_format(const struct drm_hdmi_connector *hdmi_connector,
+				  struct drm_hdmi_connector_state *hdmi_state,
+				  const struct drm_display_mode *mode,
+				  unsigned int bpc)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_device *dev = connector->dev;
+
+	if (drm_hdmi_connector_try_format_bpc(hdmi_connector, hdmi_state,
+					      mode, bpc, HDMI_COLORSPACE_RGB)) {
+		hdmi_state->output_format = HDMI_COLORSPACE_RGB;
+		return 0;
+	}
+
+	if (drm_hdmi_connector_try_format_bpc(hdmi_connector, hdmi_state,
+					      mode, bpc, HDMI_COLORSPACE_YUV422)) {
+		hdmi_state->output_format = HDMI_COLORSPACE_YUV422;
+		return 0;
+	}
+
+	drm_dbg(dev, "Failed. No Format Supported for that bpc count.\n");
+
+	return -EINVAL;
+}
+
+static int
+drm_hdmi_connector_compute_config(const struct drm_hdmi_connector *hdmi_connector,
+				  struct drm_hdmi_connector_state *hdmi_state,
+				  const struct drm_display_mode *mode)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_connector_state *conn_state = &hdmi_state->base;
+	struct drm_device *dev = connector->dev;
+	unsigned int max_bpc = clamp_t(unsigned int,
+				       conn_state->max_bpc,
+				       8, hdmi_connector->max_bpc);
+	unsigned int bpc;
+	int ret;
+
+	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
+		drm_dbg(dev, "Trying with a %d bpc output\n", bpc);
+
+		ret = drm_hdmi_connector_compute_format(hdmi_connector,
+							hdmi_state,
+							mode, bpc);
+		if (ret)
+			continue;
+
+		hdmi_state->output_bpc = bpc;
+
+		drm_dbg(dev,
+			"Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n",
+			mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode),
+			hdmi_state->output_bpc,
+			drm_hdmi_connector_get_output_format_name(hdmi_state->output_format),
+			hdmi_state->tmds_char_rate);
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
  * @connector: the parent connector this state refers to
@@ -357,11 +531,9 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 		connector_state_get_adjusted_mode(new_state);
 	int ret;
 
-	ret = drm_hdmi_connector_compute_clock(hdmi_connector, new_hdmi_state,
-					       mode,
-					       new_hdmi_state->output_bpc,
-					       new_hdmi_state->output_format);
-	if (!ret)
+	ret = drm_hdmi_connector_compute_config(hdmi_connector, new_hdmi_state,
+						mode);
+	if (ret)
 		return ret;
 
 	if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||

-- 
2.41.0

[PATCH RFC 10/13] drm/connector: hdmi: Add Infoframes generation

[Maxime Ripard]

Infoframes in KMS is usually handled by a bunch of low-level helpers
that require quite some boilerplate for drivers. This leads to
discrepancies with how drivers generate them, and which are actually
sent.

Now that we have everything needed to generate them in the HDMI
connector state, we can generate them in our common logic so that
drivers can simply reuse what we precomputed.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_hdmi_connector.c | 287 +++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h          | 100 ++++++++++++
 2 files changed, 387 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
index 22c49906dfb5..46cafb17def7 100644
--- a/drivers/gpu/drm/drm_hdmi_connector.c
+++ b/drivers/gpu/drm/drm_hdmi_connector.c
@@ -5,8 +5,10 @@
 #include <drm/drm_connector.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_print.h>
+#include <drm/display/drm_hdmi_helper.h>
 
 #include <linux/export.h>
 
@@ -499,6 +501,131 @@ drm_hdmi_connector_compute_config(const struct drm_hdmi_connector *hdmi_connecto
 	return -EINVAL;
 }
 
+static int
+drm_hdmi_connector_generate_avi_infoframe(const struct drm_hdmi_connector *hdmi_connector,
+					  struct drm_hdmi_connector_state *hdmi_state)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_connector_state *state = &hdmi_state->base;
+	const struct drm_display_mode *mode =
+		connector_state_get_adjusted_mode(state);
+	struct hdmi_avi_infoframe *frame = &hdmi_state->infoframes.avi;
+	bool is_lim_range =
+		drm_atomic_helper_hdmi_connector_is_full_range(hdmi_connector,
+							       hdmi_state);
+	enum hdmi_quantization_range rgb_quant_range =
+		is_lim_range ? HDMI_QUANTIZATION_RANGE_FULL : HDMI_QUANTIZATION_RANGE_LIMITED;
+	int ret;
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(frame, connector, mode);
+	if (ret)
+		return ret;
+
+	frame->colorspace = hdmi_state->output_format;
+
+	drm_hdmi_avi_infoframe_quant_range(frame, connector, mode, rgb_quant_range);
+	drm_hdmi_avi_infoframe_colorimetry(frame, state);
+	drm_hdmi_avi_infoframe_bars(frame, state);
+
+	return 0;
+}
+
+static int
+drm_hdmi_connector_generate_spd_infoframe(const struct drm_hdmi_connector *hdmi_connector,
+					  struct drm_hdmi_connector_state *hdmi_state)
+{
+	struct hdmi_spd_infoframe *frame = &hdmi_state->infoframes.spd;
+	int ret;
+
+	ret = hdmi_spd_infoframe_init(frame,
+				      hdmi_connector->vendor,
+				      hdmi_connector->product);
+	if (ret)
+		return ret;
+
+	frame->sdi = HDMI_SPD_SDI_PC;
+
+	return 0;
+}
+
+static int
+drm_hdmi_connector_generate_hdr_infoframe(const struct drm_hdmi_connector *hdmi_connector,
+					  struct drm_hdmi_connector_state *hdmi_state)
+{
+	const struct drm_connector_state *state = &hdmi_state->base;
+	struct hdmi_drm_infoframe *frame = &hdmi_state->infoframes.drm;
+	int ret;
+
+	if (hdmi_connector->max_bpc < 10)
+		return 0;
+
+	if (!state->hdr_output_metadata)
+		return 0;
+
+	ret = drm_hdmi_infoframe_set_hdr_metadata(frame, state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int
+drm_hdmi_connector_generate_vendor_infoframe(const struct drm_hdmi_connector *hdmi_connector,
+					     struct drm_hdmi_connector_state *hdmi_state)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_connector_state *state = &hdmi_state->base;
+	const struct drm_display_mode *mode =
+		connector_state_get_adjusted_mode(state);
+	struct hdmi_vendor_infoframe *frame = &hdmi_state->infoframes.vendor;
+	int ret;
+
+	ret = drm_hdmi_vendor_infoframe_from_display_mode(frame, connector, mode);
+	if (ret == -EINVAL)
+		return 0;
+	else
+		return ret;
+
+	return 0;
+}
+
+static int
+drm_hdmi_connector_generate_infoframes(const struct drm_hdmi_connector *hdmi_connector,
+				       struct drm_hdmi_connector_state *hdmi_state)
+{
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct drm_display_info *info = &connector->display_info;
+	int ret;
+
+	if (!info->is_hdmi)
+		return 0;
+
+	if (!info->has_hdmi_infoframe)
+		return 0;
+
+	ret = drm_hdmi_connector_generate_avi_infoframe(hdmi_connector, hdmi_state);
+	if (ret)
+		return ret;
+
+	ret = drm_hdmi_connector_generate_spd_infoframe(hdmi_connector, hdmi_state);
+	if (ret)
+		return ret;
+
+	/*
+	 * Audio Infoframes will be generated by ALSA.
+	 */
+
+	ret = drm_hdmi_connector_generate_hdr_infoframe(hdmi_connector, hdmi_state);
+	if (ret)
+		return ret;
+
+	ret = drm_hdmi_connector_generate_vendor_infoframe(hdmi_connector, hdmi_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
  * @connector: the parent connector this state refers to
@@ -536,6 +663,10 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 	if (ret)
 		return ret;
 
+	ret = drm_hdmi_connector_generate_infoframes(hdmi_connector, new_hdmi_state);
+	if (ret)
+		return ret;
+
 	if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||
 	    old_hdmi_state->output_format != new_hdmi_state->output_format ||
 	    old_hdmi_state->output_bpc != new_hdmi_state->output_bpc) {
@@ -553,6 +684,152 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 }
 EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_atomic_check);
 
+#define HDMI_MAX_INFOFRAME_SIZE		29
+
+static int write_infoframe(struct drm_hdmi_connector *hdmi_connector,
+			   union hdmi_infoframe *frame)
+{
+	const struct drm_hdmi_connector_funcs *funcs = hdmi_connector->funcs;
+	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
+	int len;
+
+	if (!funcs || !funcs->write_infoframe)
+		return -ENOSYS;
+
+	len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
+	if (len < 0)
+		return len;
+
+	return funcs->write_infoframe(hdmi_connector, frame->any.type, buffer, len);
+}
+
+static int update_infoframe(struct drm_hdmi_connector *hdmi_connector,
+			    union hdmi_infoframe *frame)
+{
+	int ret;
+
+	ret = write_infoframe(hdmi_connector, frame);
+	if (ret)
+		return ret;
+
+	switch (frame->any.type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		memcpy(&hdmi_connector->infoframes.avi, &frame->avi,
+		       sizeof(hdmi_connector->infoframes.avi));
+		break;
+	case HDMI_INFOFRAME_TYPE_DRM:
+		memcpy(&hdmi_connector->infoframes.drm, &frame->drm,
+		       sizeof(hdmi_connector->infoframes.drm));
+		break;
+	case HDMI_INFOFRAME_TYPE_SPD:
+		memcpy(&hdmi_connector->infoframes.spd, &frame->spd,
+		       sizeof(hdmi_connector->infoframes.spd));
+		break;
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		memcpy(&hdmi_connector->infoframes.vendor, &frame->vendor,
+		       sizeof(hdmi_connector->infoframes.vendor));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+#define UPDATE_INFOFRAME(c, i)					\
+	update_infoframe(c, (union hdmi_infoframe *)&(c)->infoframes.i)
+
+/**
+ * drm_atomic_helper_hdmi_connector_update_infoframes - Update the Infoframes
+ * @hdmi_connector: A pointer to the HDMI connector
+ * @hdmi_state: The HDMI connector state to generate the infoframe from
+ *
+ * This function is meant for HDMI connector drivers to write their
+ * infoframes. It will typically be used in a
+ * @drm_connector_helper_funcs.atomic_enable implementation.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_atomic_helper_hdmi_connector_update_infoframes(struct drm_hdmi_connector *hdmi_connector,
+						       struct drm_hdmi_connector_state *hdmi_state)
+{
+	struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_display_info *info = &connector->display_info;
+	int ret;
+
+	if (!info->is_hdmi)
+		return 0;
+
+	if (!info->has_hdmi_infoframe)
+		return 0;
+
+	mutex_lock(&hdmi_connector->infoframes.lock);
+
+	ret = UPDATE_INFOFRAME(hdmi_connector, avi);
+	if (ret)
+		goto out;
+
+	ret = UPDATE_INFOFRAME(hdmi_connector, audio);
+	if (ret)
+		goto out;
+
+	ret = UPDATE_INFOFRAME(hdmi_connector, drm);
+	if (ret)
+		goto out;
+
+	ret = UPDATE_INFOFRAME(hdmi_connector, spd);
+	if (ret)
+		goto out;
+
+	ret = UPDATE_INFOFRAME(hdmi_connector, vendor);
+	if (ret)
+		goto out;
+
+out:
+	mutex_unlock(&hdmi_connector->infoframes.lock);
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_update_infoframes);
+
+#undef UPDATE_INFOFRAME
+
+/**
+ * drm_atomic_helper_hdmi_connector_update_audio_infoframe - Update the Audio Infoframe
+ * @hdmi_connector: A pointer to the HDMI connector
+ * @frame: A pointer to the audio infoframe to write
+ *
+ * This function is meant for HDMI connector drivers to update their
+ * audio infoframe. It will typically be used in one of the ALSA hooks
+ * (most likely prepare).
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int
+drm_atomic_helper_hdmi_connector_update_audio_infoframe(struct drm_hdmi_connector *hdmi_connector,
+							struct hdmi_audio_infoframe *frame)
+{
+	struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_display_info *info = &connector->display_info;
+	int ret;
+
+	if (!info->is_hdmi)
+		return 0;
+
+	if (!info->has_hdmi_infoframe)
+		return 0;
+
+	mutex_lock(&hdmi_connector->infoframes.lock);
+
+	ret = update_infoframe(hdmi_connector, (union hdmi_infoframe *)frame);
+
+	mutex_unlock(&hdmi_connector->infoframes.lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_update_audio_infoframe);
+
 /**
  * drm_atomic_helper_hdmi_connector_is_full_range() - Checks whether a state uses Full-Range RGB
  * @hdmi_connector: the HDMI connector this state refers to
@@ -634,6 +911,8 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
  * drmm_hdmi_connector_init - Init a preallocated HDMI connector
  * @dev: DRM device
  * @hdmi_connector: A pointer to the HDMI connector to init
+ * @vendor: HDMI Controller Vendor name
+ * @product: HDMI Controller Product name
  * @funcs: callbacks for this connector
  * @connector_type: user visible type of the connector
  * @ddc: optional pointer to the associated ddc adapter
@@ -652,6 +931,7 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
  */
 int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
+			     const char *vendor, const char *product,
 			     const struct drm_connector_funcs *funcs,
 			     const struct drm_hdmi_connector_funcs *hdmi_funcs,
 			     int connector_type,
@@ -670,6 +950,13 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	strscpy(hdmi_connector->vendor, vendor, sizeof(hdmi_connector->vendor));
+	strscpy(hdmi_connector->product, product, sizeof(hdmi_connector->product));
+
+	ret = drmm_mutex_init(dev, &hdmi_connector->infoframes.lock);
+	if (ret)
+		return ret;
+
 	prop = hdmi_connector->broadcast_rgb_property;
 	if (!prop) {
 		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6e25a16420e4..21da6f428101 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2096,6 +2096,32 @@ struct drm_hdmi_connector_state {
 	 * selection value.
 	 */
 	enum drm_hdmi_broadcast_rgb broadcast_rgb;
+
+	/**
+	 * @infoframes: HDMI Infoframes matching that state
+	 */
+	struct {
+		/**
+		 * @avi: AVI Infoframes structure matching our state.
+		 */
+		struct hdmi_avi_infoframe avi;
+
+		/**
+		 * @drm: DRM Infoframes structure matching our state.
+		 */
+		struct hdmi_drm_infoframe drm;
+
+		/**
+		 * @spd: SPD Infoframes structure matching our state.
+		 */
+		struct hdmi_spd_infoframe spd;
+
+		/**
+		 * @vendor: Vendor Infoframes structure matching our
+		 * state.
+		 */
+		struct hdmi_vendor_infoframe vendor;
+	} infoframes;
 };
 
 #define connector_state_to_hdmi_connector_state(state) \
@@ -2127,6 +2153,11 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
 void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
 						  const struct drm_connector_state *state);
 
+int drm_atomic_helper_hdmi_connector_update_infoframes(struct drm_hdmi_connector *hdmi_connector,
+						       struct drm_hdmi_connector_state *hdmi_state);
+int drm_atomic_helper_hdmi_connector_update_audio_infoframe(struct drm_hdmi_connector *hdmi_connector,
+							    struct hdmi_audio_infoframe *frame);
+
 bool
 drm_atomic_helper_hdmi_connector_is_full_range(const struct drm_hdmi_connector *hdmi_connector,
 					       const struct drm_hdmi_connector_state *hdmi_state);
@@ -2153,6 +2184,23 @@ struct drm_hdmi_connector_funcs {
 	(*tmds_char_rate_valid)(const struct drm_hdmi_connector *connector,
 				const struct drm_display_mode *mode,
 				unsigned long long tmds_rate);
+
+	/**
+	 * @write_infoframe:
+	 *
+	 * This callback is invoked through
+	 * @drm_atomic_helper_hdmi_connector_update_infoframes during a
+	 * commit to program the infoframes into the hardware. It will
+	 * be called multiple times, once for every infoframe type.
+	 *
+	 * The @write_infoframe callback is mandatory.
+	 *
+	 * Returns:
+	 * 0 on success, a negative error code otherwise
+	 */
+	int (*write_infoframe)(struct drm_hdmi_connector *connector,
+			       enum hdmi_infoframe_type type,
+			       const u8 *buffer, size_t len);
 };
 
 struct drm_hdmi_connector {
@@ -2161,6 +2209,16 @@ struct drm_hdmi_connector {
 	 */
 	struct drm_connector base;
 
+	/**
+	 * @vendor: HDMI Controller Vendor Name
+	 */
+	char vendor[8];
+
+	/**
+	 * @product: HDMI Controller Product Name
+	 */
+	char product[16];
+
 	/**
 	 * @funcs: HDMI connector Control Functions
 	 */
@@ -2176,6 +2234,47 @@ struct drm_hdmi_connector {
 	 * Broadcast RGB selection to output with.
 	 */
 	struct drm_property *broadcast_rgb_property;
+
+	/**
+	 * @infoframes: Current Infoframes output by the connector
+	 */
+	struct {
+		/**
+		 * @lock: Mutex protecting against concurrent access to
+		 * the infoframes, most notably between KMS and ALSA.
+		 */
+		struct mutex lock;
+
+		/**
+		 * @audio: Current Audio Infoframes structure. Protected
+		 * by @lock.
+		 */
+		struct hdmi_audio_infoframe audio;
+
+		/**
+		 * @avi: Current AVI Infoframes structure. Protected by
+		 * @lock.
+		 */
+		struct hdmi_avi_infoframe avi;
+
+		/**
+		 * @drm: Current DRM Infoframes structure. Protected by
+		 * @lock.
+		 */
+		struct hdmi_drm_infoframe drm;
+
+		/**
+		 * @spd: Current SPD Infoframes structure. Protected by
+		 * @lock.
+		 */
+		struct hdmi_spd_infoframe spd;
+
+		/**
+		 * @vendor: Current Vendor Infoframes structure.
+		 * Protected by @lock.
+		 */
+		struct hdmi_vendor_infoframe vendor;
+	} infoframes;
 };
 
 #define connector_to_hdmi_connector(connector) \
@@ -2188,6 +2287,7 @@ drm_hdmi_connector_compute_mode_clock(const struct drm_display_mode *mode,
 
 int drmm_hdmi_connector_init(struct drm_device *dev,
 			     struct drm_hdmi_connector *hdmi_connector,
+			     const char *vendor, const char *product,
 			     const struct drm_connector_funcs *funcs,
 			     const struct drm_hdmi_connector_funcs *hdmi_funcs,
 			     int connector_type,

-- 
2.41.0

Re: [PATCH RFC 10/13] drm/connector: hdmi: Add Infoframes generation

[Dave Stevenson]

Hi Maxime

On Mon, 14 Aug 2023 at 14:56, Maxime Ripard <mripard@kernel.org> wrote:
>
> Infoframes in KMS is usually handled by a bunch of low-level helpers
> that require quite some boilerplate for drivers. This leads to
> discrepancies with how drivers generate them, and which are actually
> sent.
>
> Now that we have everything needed to generate them in the HDMI
> connector state, we can generate them in our common logic so that
> drivers can simply reuse what we precomputed.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_hdmi_connector.c | 287 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h          | 100 ++++++++++++
>  2 files changed, 387 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_hdmi_connector.c b/drivers/gpu/drm/drm_hdmi_connector.c
> index 22c49906dfb5..46cafb17def7 100644
> --- a/drivers/gpu/drm/drm_hdmi_connector.c
> +++ b/drivers/gpu/drm/drm_hdmi_connector.c
> @@ -5,8 +5,10 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_print.h>
> +#include <drm/display/drm_hdmi_helper.h>
>
>  #include <linux/export.h>
>
> @@ -499,6 +501,131 @@ drm_hdmi_connector_compute_config(const struct drm_hdmi_connector *hdmi_connecto
>         return -EINVAL;
>  }
>
> +static int
> +drm_hdmi_connector_generate_avi_infoframe(const struct drm_hdmi_connector *hdmi_connector,
> +                                         struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       const struct drm_connector *connector = &hdmi_connector->base;
> +       const struct drm_connector_state *state = &hdmi_state->base;
> +       const struct drm_display_mode *mode =
> +               connector_state_get_adjusted_mode(state);
> +       struct hdmi_avi_infoframe *frame = &hdmi_state->infoframes.avi;
> +       bool is_lim_range =
> +               drm_atomic_helper_hdmi_connector_is_full_range(hdmi_connector,
> +                                                              hdmi_state);
> +       enum hdmi_quantization_range rgb_quant_range =
> +               is_lim_range ? HDMI_QUANTIZATION_RANGE_FULL : HDMI_QUANTIZATION_RANGE_LIMITED;
> +       int ret;
> +
> +       ret = drm_hdmi_avi_infoframe_from_display_mode(frame, connector, mode);
> +       if (ret)
> +               return ret;
> +
> +       frame->colorspace = hdmi_state->output_format;
> +
> +       drm_hdmi_avi_infoframe_quant_range(frame, connector, mode, rgb_quant_range);
> +       drm_hdmi_avi_infoframe_colorimetry(frame, state);
> +       drm_hdmi_avi_infoframe_bars(frame, state);
> +
> +       return 0;
> +}
> +
> +static int
> +drm_hdmi_connector_generate_spd_infoframe(const struct drm_hdmi_connector *hdmi_connector,
> +                                         struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       struct hdmi_spd_infoframe *frame = &hdmi_state->infoframes.spd;
> +       int ret;
> +
> +       ret = hdmi_spd_infoframe_init(frame,
> +                                     hdmi_connector->vendor,
> +                                     hdmi_connector->product);
> +       if (ret)
> +               return ret;
> +
> +       frame->sdi = HDMI_SPD_SDI_PC;
> +
> +       return 0;
> +}
> +
> +static int
> +drm_hdmi_connector_generate_hdr_infoframe(const struct drm_hdmi_connector *hdmi_connector,
> +                                         struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       const struct drm_connector_state *state = &hdmi_state->base;
> +       struct hdmi_drm_infoframe *frame = &hdmi_state->infoframes.drm;
> +       int ret;
> +
> +       if (hdmi_connector->max_bpc < 10)
> +               return 0;
> +
> +       if (!state->hdr_output_metadata)
> +               return 0;

Minor issue here I think.

If bpc < 10 or hdr_output_metadata isn't defined then the infoframe
will be left at all 0's due to the state's kzalloc. However we will
still call update_infoframe and therefore write_infoframe asking for
the infoframe to be sent, but frame->any.type = 0. It is true that
frame type 0 isn't defined, but what is the driver expected to make of
that?

If frame->any.type is initialised appropriately (or type is passed
directly), then a length of 0 could be reasonably used to signal that
the infoframe should not be sent. Otherwise I don't think we have a
path which can stop sending the HDR infoframe if it has ever been
sent.

On vc4 I think it's also going to trip up as it has a buffer slot per
type, and slot 0 is used for the GCP.

Thanks
  Dave

> +
> +       ret = drm_hdmi_infoframe_set_hdr_metadata(frame, state);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int
> +drm_hdmi_connector_generate_vendor_infoframe(const struct drm_hdmi_connector *hdmi_connector,
> +                                            struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       const struct drm_connector *connector = &hdmi_connector->base;
> +       const struct drm_connector_state *state = &hdmi_state->base;
> +       const struct drm_display_mode *mode =
> +               connector_state_get_adjusted_mode(state);
> +       struct hdmi_vendor_infoframe *frame = &hdmi_state->infoframes.vendor;
> +       int ret;
> +
> +       ret = drm_hdmi_vendor_infoframe_from_display_mode(frame, connector, mode);
> +       if (ret == -EINVAL)
> +               return 0;
> +       else
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int
> +drm_hdmi_connector_generate_infoframes(const struct drm_hdmi_connector *hdmi_connector,
> +                                      struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       const struct drm_connector *connector = &hdmi_connector->base;
> +       const struct drm_display_info *info = &connector->display_info;
> +       int ret;
> +
> +       if (!info->is_hdmi)
> +               return 0;
> +
> +       if (!info->has_hdmi_infoframe)
> +               return 0;
> +
> +       ret = drm_hdmi_connector_generate_avi_infoframe(hdmi_connector, hdmi_state);
> +       if (ret)
> +               return ret;
> +
> +       ret = drm_hdmi_connector_generate_spd_infoframe(hdmi_connector, hdmi_state);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * Audio Infoframes will be generated by ALSA.
> +        */
> +
> +       ret = drm_hdmi_connector_generate_hdr_infoframe(hdmi_connector, hdmi_state);
> +       if (ret)
> +               return ret;
> +
> +       ret = drm_hdmi_connector_generate_vendor_infoframe(hdmi_connector, hdmi_state);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
>  /**
>   * drm_atomic_helper_hdmi_connector_atomic_check() - Helper to check HDMI connector atomic state
>   * @connector: the parent connector this state refers to
> @@ -536,6 +663,10 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
>         if (ret)
>                 return ret;
>
> +       ret = drm_hdmi_connector_generate_infoframes(hdmi_connector, new_hdmi_state);
> +       if (ret)
> +               return ret;
> +
>         if (old_hdmi_state->broadcast_rgb != new_hdmi_state->broadcast_rgb ||
>             old_hdmi_state->output_format != new_hdmi_state->output_format ||
>             old_hdmi_state->output_bpc != new_hdmi_state->output_bpc) {
> @@ -553,6 +684,152 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_atomic_check);
>
> +#define HDMI_MAX_INFOFRAME_SIZE                29
> +
> +static int write_infoframe(struct drm_hdmi_connector *hdmi_connector,
> +                          union hdmi_infoframe *frame)
> +{
> +       const struct drm_hdmi_connector_funcs *funcs = hdmi_connector->funcs;
> +       u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
> +       int len;
> +
> +       if (!funcs || !funcs->write_infoframe)
> +               return -ENOSYS;
> +
> +       len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
> +       if (len < 0)
> +               return len;
> +
> +       return funcs->write_infoframe(hdmi_connector, frame->any.type, buffer, len);
> +}
> +
> +static int update_infoframe(struct drm_hdmi_connector *hdmi_connector,
> +                           union hdmi_infoframe *frame)
> +{
> +       int ret;
> +
> +       ret = write_infoframe(hdmi_connector, frame);
> +       if (ret)
> +               return ret;
> +
> +       switch (frame->any.type) {
> +       case HDMI_INFOFRAME_TYPE_AVI:
> +               memcpy(&hdmi_connector->infoframes.avi, &frame->avi,
> +                      sizeof(hdmi_connector->infoframes.avi));
> +               break;
> +       case HDMI_INFOFRAME_TYPE_DRM:
> +               memcpy(&hdmi_connector->infoframes.drm, &frame->drm,
> +                      sizeof(hdmi_connector->infoframes.drm));
> +               break;
> +       case HDMI_INFOFRAME_TYPE_SPD:
> +               memcpy(&hdmi_connector->infoframes.spd, &frame->spd,
> +                      sizeof(hdmi_connector->infoframes.spd));
> +               break;
> +       case HDMI_INFOFRAME_TYPE_VENDOR:
> +               memcpy(&hdmi_connector->infoframes.vendor, &frame->vendor,
> +                      sizeof(hdmi_connector->infoframes.vendor));
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +#define UPDATE_INFOFRAME(c, i)                                 \
> +       update_infoframe(c, (union hdmi_infoframe *)&(c)->infoframes.i)
> +
> +/**
> + * drm_atomic_helper_hdmi_connector_update_infoframes - Update the Infoframes
> + * @hdmi_connector: A pointer to the HDMI connector
> + * @hdmi_state: The HDMI connector state to generate the infoframe from
> + *
> + * This function is meant for HDMI connector drivers to write their
> + * infoframes. It will typically be used in a
> + * @drm_connector_helper_funcs.atomic_enable implementation.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_atomic_helper_hdmi_connector_update_infoframes(struct drm_hdmi_connector *hdmi_connector,
> +                                                      struct drm_hdmi_connector_state *hdmi_state)
> +{
> +       struct drm_connector *connector = &hdmi_connector->base;
> +       struct drm_display_info *info = &connector->display_info;
> +       int ret;
> +
> +       if (!info->is_hdmi)
> +               return 0;
> +
> +       if (!info->has_hdmi_infoframe)
> +               return 0;
> +
> +       mutex_lock(&hdmi_connector->infoframes.lock);
> +
> +       ret = UPDATE_INFOFRAME(hdmi_connector, avi);
> +       if (ret)
> +               goto out;
> +
> +       ret = UPDATE_INFOFRAME(hdmi_connector, audio);
> +       if (ret)
> +               goto out;
> +
> +       ret = UPDATE_INFOFRAME(hdmi_connector, drm);
> +       if (ret)
> +               goto out;
> +
> +       ret = UPDATE_INFOFRAME(hdmi_connector, spd);
> +       if (ret)
> +               goto out;
> +
> +       ret = UPDATE_INFOFRAME(hdmi_connector, vendor);
> +       if (ret)
> +               goto out;
> +
> +out:
> +       mutex_unlock(&hdmi_connector->infoframes.lock);
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_update_infoframes);
> +
> +#undef UPDATE_INFOFRAME
> +
> +/**
> + * drm_atomic_helper_hdmi_connector_update_audio_infoframe - Update the Audio Infoframe
> + * @hdmi_connector: A pointer to the HDMI connector
> + * @frame: A pointer to the audio infoframe to write
> + *
> + * This function is meant for HDMI connector drivers to update their
> + * audio infoframe. It will typically be used in one of the ALSA hooks
> + * (most likely prepare).
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int
> +drm_atomic_helper_hdmi_connector_update_audio_infoframe(struct drm_hdmi_connector *hdmi_connector,
> +                                                       struct hdmi_audio_infoframe *frame)
> +{
> +       struct drm_connector *connector = &hdmi_connector->base;
> +       struct drm_display_info *info = &connector->display_info;
> +       int ret;
> +
> +       if (!info->is_hdmi)
> +               return 0;
> +
> +       if (!info->has_hdmi_infoframe)
> +               return 0;
> +
> +       mutex_lock(&hdmi_connector->infoframes.lock);
> +
> +       ret = update_infoframe(hdmi_connector, (union hdmi_infoframe *)frame);
> +
> +       mutex_unlock(&hdmi_connector->infoframes.lock);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_update_audio_infoframe);
> +
>  /**
>   * drm_atomic_helper_hdmi_connector_is_full_range() - Checks whether a state uses Full-Range RGB
>   * @hdmi_connector: the HDMI connector this state refers to
> @@ -634,6 +911,8 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
>   * drmm_hdmi_connector_init - Init a preallocated HDMI connector
>   * @dev: DRM device
>   * @hdmi_connector: A pointer to the HDMI connector to init
> + * @vendor: HDMI Controller Vendor name
> + * @product: HDMI Controller Product name
>   * @funcs: callbacks for this connector
>   * @connector_type: user visible type of the connector
>   * @ddc: optional pointer to the associated ddc adapter
> @@ -652,6 +931,7 @@ EXPORT_SYMBOL(drm_atomic_helper_hdmi_connector_print_state);
>   */
>  int drmm_hdmi_connector_init(struct drm_device *dev,
>                              struct drm_hdmi_connector *hdmi_connector,
> +                            const char *vendor, const char *product,
>                              const struct drm_connector_funcs *funcs,
>                              const struct drm_hdmi_connector_funcs *hdmi_funcs,
>                              int connector_type,
> @@ -670,6 +950,13 @@ int drmm_hdmi_connector_init(struct drm_device *dev,
>         if (ret)
>                 return ret;
>
> +       strscpy(hdmi_connector->vendor, vendor, sizeof(hdmi_connector->vendor));
> +       strscpy(hdmi_connector->product, product, sizeof(hdmi_connector->product));
> +
> +       ret = drmm_mutex_init(dev, &hdmi_connector->infoframes.lock);
> +       if (ret)
> +               return ret;
> +
>         prop = hdmi_connector->broadcast_rgb_property;
>         if (!prop) {
>                 prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6e25a16420e4..21da6f428101 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2096,6 +2096,32 @@ struct drm_hdmi_connector_state {
>          * selection value.
>          */
>         enum drm_hdmi_broadcast_rgb broadcast_rgb;
> +
> +       /**
> +        * @infoframes: HDMI Infoframes matching that state
> +        */
> +       struct {
> +               /**
> +                * @avi: AVI Infoframes structure matching our state.
> +                */
> +               struct hdmi_avi_infoframe avi;
> +
> +               /**
> +                * @drm: DRM Infoframes structure matching our state.
> +                */
> +               struct hdmi_drm_infoframe drm;
> +
> +               /**
> +                * @spd: SPD Infoframes structure matching our state.
> +                */
> +               struct hdmi_spd_infoframe spd;
> +
> +               /**
> +                * @vendor: Vendor Infoframes structure matching our
> +                * state.
> +                */
> +               struct hdmi_vendor_infoframe vendor;
> +       } infoframes;
>  };
>
>  #define connector_state_to_hdmi_connector_state(state) \
> @@ -2127,6 +2153,11 @@ int drm_atomic_helper_hdmi_connector_atomic_check(struct drm_connector *connecto
>  void drm_atomic_helper_hdmi_connector_print_state(struct drm_printer *p,
>                                                   const struct drm_connector_state *state);
>
> +int drm_atomic_helper_hdmi_connector_update_infoframes(struct drm_hdmi_connector *hdmi_connector,
> +                                                      struct drm_hdmi_connector_state *hdmi_state);
> +int drm_atomic_helper_hdmi_connector_update_audio_infoframe(struct drm_hdmi_connector *hdmi_connector,
> +                                                           struct hdmi_audio_infoframe *frame);
> +
>  bool
>  drm_atomic_helper_hdmi_connector_is_full_range(const struct drm_hdmi_connector *hdmi_connector,
>                                                const struct drm_hdmi_connector_state *hdmi_state);
> @@ -2153,6 +2184,23 @@ struct drm_hdmi_connector_funcs {
>         (*tmds_char_rate_valid)(const struct drm_hdmi_connector *connector,
>                                 const struct drm_display_mode *mode,
>                                 unsigned long long tmds_rate);
> +
> +       /**
> +        * @write_infoframe:
> +        *
> +        * This callback is invoked through
> +        * @drm_atomic_helper_hdmi_connector_update_infoframes during a
> +        * commit to program the infoframes into the hardware. It will
> +        * be called multiple times, once for every infoframe type.
> +        *
> +        * The @write_infoframe callback is mandatory.
> +        *
> +        * Returns:
> +        * 0 on success, a negative error code otherwise
> +        */
> +       int (*write_infoframe)(struct drm_hdmi_connector *connector,
> +                              enum hdmi_infoframe_type type,
> +                              const u8 *buffer, size_t len);
>  };
>
>  struct drm_hdmi_connector {
> @@ -2161,6 +2209,16 @@ struct drm_hdmi_connector {
>          */
>         struct drm_connector base;
>
> +       /**
> +        * @vendor: HDMI Controller Vendor Name
> +        */
> +       char vendor[8];
> +
> +       /**
> +        * @product: HDMI Controller Product Name
> +        */
> +       char product[16];
> +
>         /**
>          * @funcs: HDMI connector Control Functions
>          */
> @@ -2176,6 +2234,47 @@ struct drm_hdmi_connector {
>          * Broadcast RGB selection to output with.
>          */
>         struct drm_property *broadcast_rgb_property;
> +
> +       /**
> +        * @infoframes: Current Infoframes output by the connector
> +        */
> +       struct {
> +               /**
> +                * @lock: Mutex protecting against concurrent access to
> +                * the infoframes, most notably between KMS and ALSA.
> +                */
> +               struct mutex lock;
> +
> +               /**
> +                * @audio: Current Audio Infoframes structure. Protected
> +                * by @lock.
> +                */
> +               struct hdmi_audio_infoframe audio;
> +
> +               /**
> +                * @avi: Current AVI Infoframes structure. Protected by
> +                * @lock.
> +                */
> +               struct hdmi_avi_infoframe avi;
> +
> +               /**
> +                * @drm: Current DRM Infoframes structure. Protected by
> +                * @lock.
> +                */
> +               struct hdmi_drm_infoframe drm;
> +
> +               /**
> +                * @spd: Current SPD Infoframes structure. Protected by
> +                * @lock.
> +                */
> +               struct hdmi_spd_infoframe spd;
> +
> +               /**
> +                * @vendor: Current Vendor Infoframes structure.
> +                * Protected by @lock.
> +                */
> +               struct hdmi_vendor_infoframe vendor;
> +       } infoframes;
>  };
>
>  #define connector_to_hdmi_connector(connector) \
> @@ -2188,6 +2287,7 @@ drm_hdmi_connector_compute_mode_clock(const struct drm_display_mode *mode,
>
>  int drmm_hdmi_connector_init(struct drm_device *dev,
>                              struct drm_hdmi_connector *hdmi_connector,
> +                            const char *vendor, const char *product,
>                              const struct drm_connector_funcs *funcs,
>                              const struct drm_hdmi_connector_funcs *hdmi_funcs,
>                              int connector_type,
>
> --
> 2.41.0
>

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

[Maxime Ripard]

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 <mripard@kernel.org>
---
 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

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

[Maxime Ripard]

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 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

[PATCH RFC 13/13] drm/vc4: hdmi: Switch to HDMI connector

[Maxime Ripard]

The new HDMI connector infrastructure allows us to remove a lot of
boilerplate, so let's switch to it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c     | 730 +++++++------------------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h     |  37 +-
 drivers/gpu/drm/vc4/vc4_hdmi_phy.c |   4 +-
 3 files changed, 139 insertions(+), 632 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ac5debd47e99..9a17e5d64b4c 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -109,28 +109,10 @@
 
 #define HDMI_14_MAX_TMDS_CLK   (340 * 1000 * 1000)
 
-static const char * const output_format_str[] = {
-	[VC4_HDMI_OUTPUT_RGB]		= "RGB",
-	[VC4_HDMI_OUTPUT_YUV420]	= "YUV 4:2:0",
-	[VC4_HDMI_OUTPUT_YUV422]	= "YUV 4:2:2",
-	[VC4_HDMI_OUTPUT_YUV444]	= "YUV 4:4:4",
-};
-
-static const char *vc4_hdmi_output_fmt_str(enum vc4_hdmi_output_format fmt)
-{
-	if (fmt >= ARRAY_SIZE(output_format_str))
-		return "invalid";
-
-	return output_format_str[fmt];
-}
-
-static unsigned long long
-vc4_hdmi_encoder_compute_mode_clock(const struct drm_display_mode *mode,
-				    unsigned int bpc, enum vc4_hdmi_output_format fmt);
-
 static bool vc4_hdmi_supports_scrambling(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
+	struct drm_display_info *display = &connector->display_info;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
@@ -148,31 +130,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
 					   unsigned int bpc,
 					   enum vc4_hdmi_output_format fmt)
 {
-	unsigned long long clock = vc4_hdmi_encoder_compute_mode_clock(mode, bpc, fmt);
+	unsigned long long clock = drm_hdmi_connector_compute_mode_clock(mode, bpc, fmt);
 
 	return clock > HDMI_14_MAX_TMDS_CLK;
 }
 
-static bool vc4_hdmi_is_full_range(struct vc4_hdmi *vc4_hdmi,
-				   struct vc4_hdmi_connector_state *vc4_state)
-{
-	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
-
-	if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
-		return false;
-	else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
-		return true;
-
-	return !display->is_hdmi ||
-		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
-}
-
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 {
 	struct drm_debugfs_entry *entry = m->private;
 	struct vc4_hdmi *vc4_hdmi = entry->file.data;
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	struct drm_printer p = drm_seq_file_printer(m);
 	int idx;
 
@@ -195,7 +162,7 @@ static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 
 static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -228,7 +195,7 @@ static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
 
 static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -257,7 +224,7 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
 #ifdef CONFIG_DRM_VC4_HDMI_CEC
 static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long cec_rate;
 	unsigned long flags;
 	u16 clk_cnt;
@@ -410,7 +377,7 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 				    struct drm_modeset_acquire_ctx *ctx,
 				    enum drm_connector_status status)
 {
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct edid *edid;
 	int ret;
 
@@ -534,12 +501,8 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 {
 	struct drm_connector_state *old_state =
 		drm_atomic_get_old_connector_state(state, connector);
-	struct vc4_hdmi_connector_state *old_vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(old_state);
 	struct drm_connector_state *new_state =
 		drm_atomic_get_new_connector_state(state, connector);
-	struct vc4_hdmi_connector_state *new_vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(new_state);
 	struct drm_crtc *crtc = new_state->crtc;
 
 	if (!crtc)
@@ -571,9 +534,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 			return ret;
 	}
 
-	if (old_state->colorspace != new_state->colorspace ||
-	    old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
-	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+	if (old_state->colorspace != new_state->colorspace) {
 		struct drm_crtc_state *crtc_state;
 
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
@@ -583,112 +544,23 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
 		crtc_state->mode_changed = true;
 	}
 
-	return 0;
-}
-
-static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
-					   const struct drm_connector_state *state,
-					   struct drm_property *property,
-					   uint64_t *val)
-{
-	struct drm_device *drm = connector->dev;
-	struct vc4_hdmi *vc4_hdmi =
-		connector_to_vc4_hdmi(connector);
-	const struct vc4_hdmi_connector_state *vc4_conn_state =
-		conn_state_to_vc4_hdmi_conn_state(state);
-
-	if (property == vc4_hdmi->broadcast_rgb_property) {
-		*val = vc4_conn_state->broadcast_rgb;
-	} else {
-		drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
-			property->base.id, property->name);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
-					   struct drm_connector_state *state,
-					   struct drm_property *property,
-					   uint64_t val)
-{
-	struct drm_device *drm = connector->dev;
-	struct vc4_hdmi *vc4_hdmi =
-		connector_to_vc4_hdmi(connector);
-	struct vc4_hdmi_connector_state *vc4_conn_state =
-		conn_state_to_vc4_hdmi_conn_state(state);
-
-	if (property == vc4_hdmi->broadcast_rgb_property) {
-		vc4_conn_state->broadcast_rgb = val;
-		return 0;
-	}
-
-	drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
-		property->base.id, property->name);
-	return -EINVAL;
+	return drm_atomic_helper_hdmi_connector_atomic_check(connector, state);
 }
 
 static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 {
-	struct vc4_hdmi_connector_state *old_state =
-		conn_state_to_vc4_hdmi_conn_state(connector->state);
-	struct vc4_hdmi_connector_state *new_state =
-		kzalloc(sizeof(*new_state), GFP_KERNEL);
-
-	if (connector->state)
-		__drm_atomic_helper_connector_destroy_state(connector->state);
-
-	kfree(old_state);
-	__drm_atomic_helper_connector_reset(connector, &new_state->base);
-
-	if (!new_state)
-		return;
-
-	new_state->base.max_bpc = 8;
-	new_state->base.max_requested_bpc = 8;
-	new_state->output_format = VC4_HDMI_OUTPUT_RGB;
-	new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
+	drm_atomic_helper_hdmi_connector_reset(connector);
 	drm_atomic_helper_connector_tv_margins_reset(connector);
 }
 
-static struct drm_connector_state *
-vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
-{
-	struct drm_connector_state *conn_state = connector->state;
-	struct vc4_hdmi_connector_state *vc4_state = conn_state_to_vc4_hdmi_conn_state(conn_state);
-	struct vc4_hdmi_connector_state *new_state;
-
-	new_state = kzalloc(sizeof(*new_state), GFP_KERNEL);
-	if (!new_state)
-		return NULL;
-
-	new_state->tmds_char_rate = vc4_state->tmds_char_rate;
-	new_state->output_bpc = vc4_state->output_bpc;
-	new_state->output_format = vc4_state->output_format;
-	new_state->broadcast_rgb = vc4_state->broadcast_rgb;
-	__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
-
-	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 = vc4_hdmi_connector_destroy_state,
-	.atomic_get_property = vc4_hdmi_connector_get_property,
-	.atomic_set_property = vc4_hdmi_connector_set_property,
+	.atomic_duplicate_state = drm_atomic_helper_hdmi_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_hdmi_connector_destroy_state,
+	.atomic_get_property = drm_atomic_helper_hdmi_connector_get_property,
+	.atomic_set_property = drm_atomic_helper_hdmi_connector_set_property,
+	.debugfs_init = drm_helper_hdmi_connector_debugfs_init,
 };
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
@@ -697,44 +569,23 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
 	.atomic_check = vc4_hdmi_connector_atomic_check,
 };
 
-static const struct drm_prop_enum_list broadcast_rgb_names[] = {
-	{ VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
-	{ VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
-	{ VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
-};
-
-static void
-vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
-				       struct vc4_hdmi *vc4_hdmi)
-{
-	struct drm_property *prop = vc4_hdmi->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;
-
-		vc4_hdmi->broadcast_rgb_property = prop;
-	}
-
-	drm_object_attach_property(&vc4_hdmi->connector.base, prop,
-				   VC4_HDMI_BROADCAST_RGB_AUTO);
-}
+static const struct drm_hdmi_connector_funcs vc4_hdmi_hdmi_connector_funcs;
 
 static int vc4_hdmi_connector_init(struct drm_device *dev,
 				   struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &hdmi_connector->base;
 	struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
 	int ret;
 
-	ret = drmm_connector_init(dev, connector,
-				  &vc4_hdmi_connector_funcs,
-				  DRM_MODE_CONNECTOR_HDMIA,
-				  vc4_hdmi->ddc);
+	ret = drmm_hdmi_connector_init(dev, hdmi_connector,
+				       "Broadcom", "Videocore",
+				       &vc4_hdmi_connector_funcs,
+				       &vc4_hdmi_hdmi_connector_funcs,
+				       DRM_MODE_CONNECTOR_HDMIA,
+				       vc4_hdmi->ddc,
+				       12);
 	if (ret)
 		return ret;
 
@@ -758,7 +609,6 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
 
 	drm_connector_attach_colorspace_property(connector);
 	drm_connector_attach_tv_margin_properties(connector);
-	drm_connector_attach_max_bpc_property(connector, 8, 12);
 
 	connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
 			     DRM_CONNECTOR_POLL_DISCONNECT);
@@ -767,22 +617,16 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
 	connector->doublescan_allowed = 0;
 	connector->stereo_allowed = 1;
 
-	if (vc4_hdmi->variant->supports_hdr)
-		drm_connector_attach_hdr_output_metadata_property(connector);
-
-	vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
-
 	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
 }
 
-static int vc4_hdmi_stop_packet(struct drm_encoder *encoder,
+static int vc4_hdmi_stop_packet(struct vc4_hdmi *vc4_hdmi,
 				enum hdmi_infoframe_type type,
 				bool poll)
 {
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	u32 packet_id = type - 0x80;
 	unsigned long flags;
 	int ret = 0;
@@ -805,12 +649,13 @@ static int vc4_hdmi_stop_packet(struct drm_encoder *encoder,
 	return ret;
 }
 
-static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,
-				     union hdmi_infoframe *frame)
+static int vc4_hdmi_write_infoframe(struct drm_hdmi_connector *hdmi_connector,
+				    enum hdmi_infoframe_type type,
+				    const u8 *buffer, size_t len)
 {
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	u32 packet_id = frame->any.type - 0x80;
+	struct vc4_hdmi *vc4_hdmi = hdmi_connector_to_vc4_hdmi(hdmi_connector);
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
+	u32 packet_id = type - 0x80;
 	const struct vc4_hdmi_register *ram_packet_start =
 		&vc4_hdmi->variant->registers[HDMI_RAM_PACKET_START];
 	u32 packet_reg = ram_packet_start->offset + VC4_HDMI_PACKET_STRIDE * packet_id;
@@ -818,24 +663,19 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,
 		VC4_HDMI_PACKET_STRIDE * (packet_id + 1);
 	void __iomem *base = __vc4_hdmi_get_field_base(vc4_hdmi,
 						       ram_packet_start->reg);
-	uint8_t buffer[VC4_HDMI_PACKET_STRIDE] = {};
 	unsigned long flags;
-	ssize_t len, i;
+	ssize_t i;
 	int ret;
 	int idx;
 
 	if (!drm_dev_enter(drm, &idx))
-		return;
+		return 0;
 
 	WARN_ONCE(!(HDMI_READ(HDMI_RAM_PACKET_CONFIG) &
 		    VC4_HDMI_RAM_PACKET_ENABLE),
 		  "Packet RAM has to be on to store the packet.");
 
-	len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
-	if (len < 0)
-		goto out;
-
-	ret = vc4_hdmi_stop_packet(encoder, frame->any.type, true);
+	ret = vc4_hdmi_stop_packet(vc4_hdmi, type, true);
 	if (ret) {
 		DRM_ERROR("Failed to wait for infoframe to go idle: %d\n", ret);
 		goto out;
@@ -877,130 +717,7 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,
 
 out:
 	drm_dev_exit(idx);
-}
-
-static void vc4_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
-					      enum vc4_hdmi_output_format fmt)
-{
-	switch (fmt) {
-	case VC4_HDMI_OUTPUT_RGB:
-		frame->colorspace = HDMI_COLORSPACE_RGB;
-		break;
-
-	case VC4_HDMI_OUTPUT_YUV420:
-		frame->colorspace = HDMI_COLORSPACE_YUV420;
-		break;
-
-	case VC4_HDMI_OUTPUT_YUV422:
-		frame->colorspace = HDMI_COLORSPACE_YUV422;
-		break;
-
-	case VC4_HDMI_OUTPUT_YUV444:
-		frame->colorspace = HDMI_COLORSPACE_YUV444;
-		break;
-
-	default:
-		break;
-	}
-}
-
-static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_connector *connector = &vc4_hdmi->connector;
-	struct drm_connector_state *cstate = connector->state;
-	struct vc4_hdmi_connector_state *vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(cstate);
-	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	union hdmi_infoframe frame;
-	int ret;
-
-	lockdep_assert_held(&vc4_hdmi->mutex);
-
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						       connector, mode);
-	if (ret < 0) {
-		DRM_ERROR("couldn't fill AVI infoframe\n");
-		return;
-	}
-
-	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
-					   connector, mode,
-					   vc4_hdmi_is_full_range(vc4_hdmi, vc4_state) ?
-					   HDMI_QUANTIZATION_RANGE_FULL :
-					   HDMI_QUANTIZATION_RANGE_LIMITED);
-	drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
-	vc4_hdmi_avi_infoframe_colorspace(&frame.avi, vc4_state->output_format);
-	drm_hdmi_avi_infoframe_bars(&frame.avi, cstate);
-
-	vc4_hdmi_write_infoframe(encoder, &frame);
-}
-
-static void vc4_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
-{
-	union hdmi_infoframe frame;
-	int ret;
-
-	ret = hdmi_spd_infoframe_init(&frame.spd, "Broadcom", "Videocore");
-	if (ret < 0) {
-		DRM_ERROR("couldn't fill SPD infoframe\n");
-		return;
-	}
-
-	frame.spd.sdi = HDMI_SPD_SDI_PC;
-
-	vc4_hdmi_write_infoframe(encoder, &frame);
-}
-
-static void vc4_hdmi_set_audio_infoframe(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct hdmi_audio_infoframe *audio = &vc4_hdmi->audio.infoframe;
-	union hdmi_infoframe frame;
-
-	memcpy(&frame.audio, audio, sizeof(*audio));
-
-	if (vc4_hdmi->packet_ram_enabled)
-		vc4_hdmi_write_infoframe(encoder, &frame);
-}
-
-static void vc4_hdmi_set_hdr_infoframe(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_connector *connector = &vc4_hdmi->connector;
-	struct drm_connector_state *conn_state = connector->state;
-	union hdmi_infoframe frame;
-
-	lockdep_assert_held(&vc4_hdmi->mutex);
-
-	if (!vc4_hdmi->variant->supports_hdr)
-		return;
-
-	if (!conn_state->hdr_output_metadata)
-		return;
-
-	if (drm_hdmi_infoframe_set_hdr_metadata(&frame.drm, conn_state))
-		return;
-
-	vc4_hdmi_write_infoframe(encoder, &frame);
-}
-
-static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-
-	lockdep_assert_held(&vc4_hdmi->mutex);
-
-	vc4_hdmi_set_avi_infoframe(encoder);
-	vc4_hdmi_set_spd_infoframe(encoder);
-	/*
-	 * If audio was streaming, then we need to reenabled the audio
-	 * infoframe here during encoder_enable.
-	 */
-	if (vc4_hdmi->audio.streaming)
-		vc4_hdmi_set_audio_infoframe(encoder);
-
-	vc4_hdmi_set_hdr_infoframe(encoder);
+	return ret;
 }
 
 #define SCRAMBLING_POLLING_DELAY_MS	1000
@@ -1008,7 +725,7 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct drm_device *drm = connector->dev;
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	unsigned long flags;
@@ -1046,7 +763,7 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
 static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct drm_device *drm = connector->dev;
 	unsigned long flags;
 	int idx;
@@ -1080,7 +797,7 @@ static void vc4_hdmi_scrambling_wq(struct work_struct *work)
 	struct vc4_hdmi *vc4_hdmi = container_of(to_delayed_work(work),
 						 struct vc4_hdmi,
 						 scrambling_work);
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 
 	if (drm_scdc_get_scrambling_status(connector))
 		return;
@@ -1096,7 +813,7 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
 					       struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -1134,7 +851,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
 						 struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int ret;
 	int idx;
@@ -1169,9 +886,13 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 			       struct drm_connector_state *state,
 			       const struct drm_display_mode *mode)
 {
-	struct vc4_hdmi_connector_state *vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(state);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
+	struct drm_hdmi_connector_state *hdmi_state =
+		connector_state_to_hdmi_connector_state(state);
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
+	bool is_lim_range =
+		drm_atomic_helper_hdmi_connector_is_full_range(hdmi_connector,
+							       hdmi_state);
 	unsigned long flags;
 	u32 csc_ctl;
 	int idx;
@@ -1184,7 +905,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 	csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
 				VC4_HD_CSC_CTL_ORDER);
 
-	if (!vc4_hdmi_is_full_range(vc4_hdmi, vc4_state)) {
+	if (!is_lim_range) {
 		/* CEA VICs other than #1 requre limited range RGB
 		 * output unless overridden by an AVI infoframe.
 		 * Apply a colorspace conversion to squash 0-255 down
@@ -1406,10 +1127,13 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 			       struct drm_connector_state *state,
 			       const struct drm_display_mode *mode)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct vc4_hdmi_connector_state *vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(state);
-	unsigned int lim_range = vc4_hdmi_is_full_range(vc4_hdmi, vc4_state) ? 0 : 1;
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
+	struct drm_hdmi_connector_state *hdmi_state =
+		connector_state_to_hdmi_connector_state(state);
+	unsigned int lim_range =
+		drm_atomic_helper_hdmi_connector_is_full_range(hdmi_connector,
+							       hdmi_state) ? 0 : 1;
 	unsigned long flags;
 	const u16 (*csc)[4];
 	u32 if_cfg = 0;
@@ -1424,14 +1148,14 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
-	switch (vc4_state->output_format) {
-	case VC4_HDMI_OUTPUT_YUV444:
+	switch (hdmi_state->output_format) {
+	case HDMI_COLORSPACE_YUV444:
 		csc = vc5_hdmi_find_yuv_csc_coeffs(vc4_hdmi, state->colorspace, !!lim_range);
 
 		vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi, csc);
 		break;
 
-	case VC4_HDMI_OUTPUT_YUV422:
+	case HDMI_COLORSPACE_YUV422:
 		csc = vc5_hdmi_find_yuv_csc_coeffs(vc4_hdmi, state->colorspace, !!lim_range);
 
 		csc_ctl |= VC4_SET_FIELD(VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422_STANDARD,
@@ -1448,7 +1172,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
 		vc5_hdmi_set_csc_coeffs(vc4_hdmi, csc);
 		break;
 
-	case VC4_HDMI_OUTPUT_RGB:
+	case HDMI_COLORSPACE_RGB:
 		if_xbar = 0x354021;
 
 		vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_rgb[lim_range]);
@@ -1472,7 +1196,7 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 				 struct drm_connector_state *state,
 				 const struct drm_display_mode *mode)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
 	bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
@@ -1536,9 +1260,9 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 				 struct drm_connector_state *state,
 				 const struct drm_display_mode *mode)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	const struct vc4_hdmi_connector_state *vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(state);
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
+	const struct drm_hdmi_connector_state *hdmi_state =
+		connector_state_to_hdmi_connector_state(state);
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
 	bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
@@ -1590,7 +1314,7 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 	HDMI_WRITE(HDMI_VERTB0, vertb_even);
 	HDMI_WRITE(HDMI_VERTB1, vertb);
 
-	switch (vc4_state->output_bpc) {
+	switch (hdmi_state->output_bpc) {
 	case 12:
 		gcp = 6;
 		break;
@@ -1607,7 +1331,7 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 	 * YCC422 is always 36-bit and not considered deep colour so
 	 * doesn't signal in GCP.
 	 */
-	if (vc4_state->output_format == VC4_HDMI_OUTPUT_YUV422) {
+	if (hdmi_state->output_format == HDMI_COLORSPACE_YUV422) {
 		gcp = 0;
 	}
 
@@ -1643,7 +1367,7 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
 
 static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	u32 drift;
 	int ret;
@@ -1687,14 +1411,14 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 						struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
-	struct vc4_hdmi_connector_state *vc4_conn_state =
-		conn_state_to_vc4_hdmi_conn_state(conn_state);
+	struct drm_hdmi_connector_state *hdmi_conn_state =
+		connector_state_to_hdmi_connector_state(conn_state);
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	unsigned long tmds_char_rate = vc4_conn_state->tmds_char_rate;
+	unsigned long tmds_char_rate = hdmi_conn_state->tmds_char_rate;
 	unsigned long bvb_rate, hsm_rate;
 	unsigned long flags;
 	int ret;
@@ -1771,7 +1495,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	}
 
 	if (vc4_hdmi->variant->phy_init)
-		vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);
+		vc4_hdmi->variant->phy_init(vc4_hdmi, hdmi_conn_state);
 
 	spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
 
@@ -1806,8 +1530,8 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
 					     struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
+	struct drm_device *drm = connector->dev;
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
@@ -1836,9 +1560,15 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 					      struct drm_atomic_state *state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &hdmi_connector->base;
+	struct drm_connector_state *new_conn_state =
+		drm_atomic_get_new_connector_state(state, connector);
+	struct drm_hdmi_connector_state *new_hdmi_state =
+		connector_state_to_hdmi_connector_state(new_conn_state);
+	struct drm_device *drm = connector->dev;
 	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
+	struct drm_display_info *display = &connector->display_info;
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
 	unsigned long flags;
@@ -1901,10 +1631,11 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
 
 		spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 		vc4_hdmi->packet_ram_enabled = true;
-
-		vc4_hdmi_set_infoframes(encoder);
 	}
 
+	drm_atomic_helper_hdmi_connector_update_infoframes(hdmi_connector,
+							   new_hdmi_state);
+
 	vc4_hdmi_recenter_fifo(vc4_hdmi);
 	vc4_hdmi_enable_scrambling(encoder);
 
@@ -1919,109 +1650,25 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
 					     struct drm_connector_state *conn_state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct vc4_hdmi_connector_state *vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(conn_state);
+	const struct drm_hdmi_connector_state *hdmi_state =
+		connector_state_to_hdmi_connector_state(conn_state);
 
 	mutex_lock(&vc4_hdmi->mutex);
 	drm_mode_copy(&vc4_hdmi->saved_adjusted_mode,
 		      &crtc_state->adjusted_mode);
-	vc4_hdmi->output_bpc = vc4_state->output_bpc;
-	vc4_hdmi->output_format = vc4_state->output_format;
+	vc4_hdmi->output_bpc = hdmi_state->output_bpc;
+	vc4_hdmi->output_format = hdmi_state->output_format;
 	mutex_unlock(&vc4_hdmi->mutex);
 }
 
-static bool
-vc4_hdmi_sink_supports_format_bpc(const struct vc4_hdmi *vc4_hdmi,
-				  const struct drm_display_info *info,
-				  const struct drm_display_mode *mode,
-				  unsigned int format, unsigned int bpc)
-{
-	struct drm_device *dev = vc4_hdmi->connector.dev;
-	u8 vic = drm_match_cea_mode(mode);
-
-	if (vic == 1 && bpc != 8) {
-		drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
-		return false;
-	}
-
-	if (!info->is_hdmi &&
-	    (format != VC4_HDMI_OUTPUT_RGB || bpc != 8)) {
-		drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
-		return false;
-	}
-
-	switch (format) {
-	case VC4_HDMI_OUTPUT_RGB:
-		drm_dbg(dev, "RGB Format, checking the constraints.\n");
-
-		if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
-			return false;
-
-		if (bpc == 10 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30)) {
-			drm_dbg(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
-			return false;
-		}
-
-		if (bpc == 12 && !(info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_36)) {
-			drm_dbg(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
-			return false;
-		}
-
-		drm_dbg(dev, "RGB format supported in that configuration.\n");
-
-		return true;
-
-	case VC4_HDMI_OUTPUT_YUV422:
-		drm_dbg(dev, "YUV422 format, checking the constraints.\n");
-
-		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR422)) {
-			drm_dbg(dev, "Sink doesn't support YUV422.\n");
-			return false;
-		}
-
-		if (bpc != 12) {
-			drm_dbg(dev, "YUV422 only supports 12 bpc.\n");
-			return false;
-		}
-
-		drm_dbg(dev, "YUV422 format supported in that configuration.\n");
-
-		return true;
-
-	case VC4_HDMI_OUTPUT_YUV444:
-		drm_dbg(dev, "YUV444 format, checking the constraints.\n");
-
-		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR444)) {
-			drm_dbg(dev, "Sink doesn't support YUV444.\n");
-			return false;
-		}
-
-		if (bpc == 10 && !(info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_30)) {
-			drm_dbg(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
-			return false;
-		}
-
-		if (bpc == 12 && !(info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_36)) {
-			drm_dbg(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
-			return false;
-		}
-
-		drm_dbg(dev, "YUV444 format supported in that configuration.\n");
-
-		return true;
-	}
-
-	return false;
-}
-
 static enum drm_mode_status
-vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
-			     const struct drm_display_mode *mode,
-			     unsigned long long clock)
+vc4_hdmi_connector_clock_valid(const struct drm_hdmi_connector *hdmi_connector,
+			       const struct drm_display_mode *mode,
+			       unsigned long long clock)
 {
-	const struct drm_connector *connector = &vc4_hdmi->connector;
-	const struct drm_display_info *info = &connector->display_info;
-	struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
+	const struct drm_connector *connector = &hdmi_connector->base;
+	const struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
+	const struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
 
 	if (clock > vc4_hdmi->variant->max_pixel_clock)
 		return MODE_CLOCK_HIGH;
@@ -2035,125 +1682,13 @@ vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
 	    drm_mode_vrefresh(mode) >= 50)
 		return MODE_CLOCK_HIGH;
 
-	if (info->max_tmds_clock && clock > (info->max_tmds_clock * 1000))
-		return MODE_CLOCK_HIGH;
-
 	return MODE_OK;
 }
 
-static unsigned long long
-vc4_hdmi_encoder_compute_mode_clock(const struct drm_display_mode *mode,
-				    unsigned int bpc,
-				    enum vc4_hdmi_output_format fmt)
-{
-	unsigned long long clock = mode->clock * 1000ULL;
-
-	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
-		clock = clock * 2;
-
-	if (fmt == VC4_HDMI_OUTPUT_YUV422)
-		bpc = 8;
-
-	clock = clock * bpc;
-	do_div(clock, 8);
-
-	return clock;
-}
-
-static int
-vc4_hdmi_encoder_compute_clock(const struct vc4_hdmi *vc4_hdmi,
-			       struct vc4_hdmi_connector_state *vc4_state,
-			       const struct drm_display_mode *mode,
-			       unsigned int bpc, unsigned int fmt)
-{
-	unsigned long long clock;
-
-	clock = vc4_hdmi_encoder_compute_mode_clock(mode, bpc, fmt);
-	if (vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode, clock) != MODE_OK)
-		return -EINVAL;
-
-	vc4_state->tmds_char_rate = clock;
-
-	return 0;
-}
-
-static int
-vc4_hdmi_encoder_compute_format(const struct vc4_hdmi *vc4_hdmi,
-				struct vc4_hdmi_connector_state *vc4_state,
-				const struct drm_display_mode *mode,
-				unsigned int bpc)
-{
-	struct drm_device *dev = vc4_hdmi->connector.dev;
-	const struct drm_connector *connector = &vc4_hdmi->connector;
-	const struct drm_display_info *info = &connector->display_info;
-	unsigned int format;
-
-	drm_dbg(dev, "Trying with an RGB output\n");
-
-	format = VC4_HDMI_OUTPUT_RGB;
-	if (vc4_hdmi_sink_supports_format_bpc(vc4_hdmi, info, mode, format, bpc)) {
-		int ret;
-
-		ret = vc4_hdmi_encoder_compute_clock(vc4_hdmi, vc4_state,
-						     mode, bpc, format);
-		if (!ret) {
-			vc4_state->output_format = format;
-			return 0;
-		}
-	}
-
-	drm_dbg(dev, "Failed, Trying with an YUV422 output\n");
-
-	format = VC4_HDMI_OUTPUT_YUV422;
-	if (vc4_hdmi_sink_supports_format_bpc(vc4_hdmi, info, mode, format, bpc)) {
-		int ret;
-
-		ret = vc4_hdmi_encoder_compute_clock(vc4_hdmi, vc4_state,
-						     mode, bpc, format);
-		if (!ret) {
-			vc4_state->output_format = format;
-			return 0;
-		}
-	}
-
-	drm_dbg(dev, "Failed. No Format Supported for that bpc count.\n");
-
-	return -EINVAL;
-}
-
-static int
-vc4_hdmi_encoder_compute_config(const struct vc4_hdmi *vc4_hdmi,
-				struct vc4_hdmi_connector_state *vc4_state,
-				const struct drm_display_mode *mode)
-{
-	struct drm_device *dev = vc4_hdmi->connector.dev;
-	struct drm_connector_state *conn_state = &vc4_state->base;
-	unsigned int max_bpc = clamp_t(unsigned int, conn_state->max_bpc, 8, 12);
-	unsigned int bpc;
-	int ret;
-
-	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
-		drm_dbg(dev, "Trying with a %d bpc output\n", bpc);
-
-		ret = vc4_hdmi_encoder_compute_format(vc4_hdmi, vc4_state,
-						      mode, bpc);
-		if (ret)
-			continue;
-
-		vc4_state->output_bpc = bpc;
-
-		drm_dbg(dev,
-			"Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n",
-			mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode),
-			vc4_state->output_bpc,
-			vc4_hdmi_output_fmt_str(vc4_state->output_format),
-			vc4_state->tmds_char_rate);
-
-		break;
-	}
-
-	return ret;
-}
+static const struct drm_hdmi_connector_funcs vc4_hdmi_hdmi_connector_funcs = {
+	.tmds_char_rate_valid	= vc4_hdmi_connector_clock_valid,
+	.write_infoframe	= vc4_hdmi_write_infoframe,
+};
 
 #define WIFI_2_4GHz_CH1_MIN_FREQ	2400000000ULL
 #define WIFI_2_4GHz_CH1_MAX_FREQ	2422000000ULL
@@ -2163,16 +1698,9 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 					 struct drm_connector_state *conn_state)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
-	struct drm_connector *connector = &vc4_hdmi->connector;
-	struct drm_connector_state *old_conn_state =
-		drm_atomic_get_old_connector_state(conn_state->state, connector);
-	struct vc4_hdmi_connector_state *old_vc4_state =
-		conn_state_to_vc4_hdmi_conn_state(old_conn_state);
-	struct vc4_hdmi_connector_state *vc4_state = conn_state_to_vc4_hdmi_conn_state(conn_state);
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	unsigned long long tmds_char_rate = mode->clock * 1000;
 	unsigned long long tmds_bit_rate;
-	int ret;
 
 	if (vc4_hdmi->variant->unsupported_odd_h_timings) {
 		if (mode->flags & DRM_MODE_FLAG_DBLCLK) {
@@ -2208,15 +1736,6 @@ static int vc4_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
 		tmds_char_rate = mode->clock * 1000;
 	}
 
-	ret = vc4_hdmi_encoder_compute_config(vc4_hdmi, vc4_state, mode);
-	if (ret)
-		return ret;
-
-	/* vc4_hdmi_encoder_compute_config may have changed output_bpc and/or output_format */
-	if (vc4_state->output_bpc != old_vc4_state->output_bpc ||
-	    vc4_state->output_format != old_vc4_state->output_format)
-		crtc_state->mode_changed = true;
-
 	return 0;
 }
 
@@ -2225,6 +1744,7 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder,
 			    const struct drm_display_mode *mode)
 {
 	struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
 
 	if (vc4_hdmi->variant->unsupported_odd_h_timings &&
 	    !(mode->flags & DRM_MODE_FLAG_DBLCLK) &&
@@ -2232,7 +1752,7 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder,
 	     (mode->hsync_end % 2) || (mode->htotal % 2)))
 		return MODE_H_ILLEGAL;
 
-	return vc4_hdmi_encoder_clock_valid(vc4_hdmi, mode, mode->clock * 1000);
+	return vc4_hdmi_connector_clock_valid(hdmi_connector, mode, mode->clock * 1000);
 }
 
 static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
@@ -2283,7 +1803,7 @@ static u32 vc5_hdmi_channel_map(struct vc4_hdmi *vc4_hdmi, u32 channel_mask)
 
 static bool vc5_hdmi_hp_detect(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	u32 hotplug;
 	int idx;
@@ -2304,7 +1824,7 @@ static bool vc5_hdmi_hp_detect(struct vc4_hdmi *vc4_hdmi)
 static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi,
 					 unsigned int samplerate)
 {
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	u32 hsm_clock;
 	unsigned long flags;
 	unsigned long n, m;
@@ -2366,7 +1886,8 @@ static inline struct vc4_hdmi *dai_to_hdmi(struct snd_soc_dai *dai)
 
 static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_display_info *display = &vc4_hdmi->connector.display_info;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
+	struct drm_display_info *display = &connector->display_info;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
@@ -2383,7 +1904,7 @@ static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi)
 static int vc4_hdmi_audio_startup(struct device *dev, void *data)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int ret = 0;
 	int idx;
@@ -2424,7 +1945,6 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data)
 
 static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
 	struct device *dev = &vc4_hdmi->pdev->dev;
 	unsigned long flags;
 	int ret;
@@ -2432,7 +1952,7 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi)
 	lockdep_assert_held(&vc4_hdmi->mutex);
 
 	vc4_hdmi->audio.streaming = false;
-	ret = vc4_hdmi_stop_packet(encoder, HDMI_INFOFRAME_TYPE_AUDIO, false);
+	ret = vc4_hdmi_stop_packet(vc4_hdmi, HDMI_INFOFRAME_TYPE_AUDIO, false);
 	if (ret)
 		dev_err(dev, "Failed to stop audio infoframe: %d\n", ret);
 
@@ -2448,7 +1968,7 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi)
 static void vc4_hdmi_audio_shutdown(struct device *dev, void *data)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -2522,8 +2042,8 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data,
 				  struct hdmi_codec_params *params)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
-	struct drm_encoder *encoder = &vc4_hdmi->encoder.base;
+	struct drm_hdmi_connector *hdmi_connector = &vc4_hdmi->connector;
+	struct drm_device *drm = hdmi_connector->base.dev;
 	unsigned int sample_rate = params->sample_rate;
 	unsigned int channels = params->channels;
 	unsigned long flags;
@@ -2600,8 +2120,10 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data,
 
 	spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
 
-	memcpy(&vc4_hdmi->audio.infoframe, &params->cea, sizeof(params->cea));
-	vc4_hdmi_set_audio_infoframe(encoder);
+	ret = drm_atomic_helper_hdmi_connector_update_audio_infoframe(hdmi_connector,
+								      &params->cea);
+	if (ret)
+		goto out_dev_exit;
 
 out_dev_exit:
 	drm_dev_exit(idx);
@@ -2649,7 +2171,7 @@ static int vc4_hdmi_audio_get_eld(struct device *dev, void *data,
 				  uint8_t *buf, size_t len)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 
 	mutex_lock(&vc4_hdmi->mutex);
 	memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
@@ -2832,7 +2354,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
 static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv)
 {
 	struct vc4_hdmi *vc4_hdmi = priv;
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct drm_device *dev = connector->dev;
 
 	if (dev && dev->registered)
@@ -2843,7 +2365,7 @@ static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv)
 
 static int vc4_hdmi_hotplug_init(struct vc4_hdmi *vc4_hdmi)
 {
-	struct drm_connector *connector = &vc4_hdmi->connector;
+	struct drm_connector *connector = &vc4_hdmi->connector.base;
 	struct platform_device *pdev = vc4_hdmi->pdev;
 	int ret;
 
@@ -2916,7 +2438,7 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
 
 static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1)
 {
-	struct drm_device *dev = vc4_hdmi->connector.dev;
+	struct drm_device *dev = vc4_hdmi->connector.base.dev;
 	struct cec_msg *msg = &vc4_hdmi->cec_rx_msg;
 	unsigned int i;
 
@@ -3056,7 +2578,7 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
 static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	/* clock period in microseconds */
 	const u32 usecs = 1000000 / CEC_CLOCK_FREQ;
 	unsigned long flags;
@@ -3123,7 +2645,7 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -3167,7 +2689,7 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
 static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-	struct drm_device *drm = vc4_hdmi->connector.dev;
+	struct drm_device *drm = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	int idx;
 
@@ -3196,7 +2718,7 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 				      u32 signal_free_time, struct cec_msg *msg)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-	struct drm_device *dev = vc4_hdmi->connector.dev;
+	struct drm_device *dev = vc4_hdmi->connector.base.dev;
 	unsigned long flags;
 	u32 val;
 	unsigned int i;
@@ -3273,7 +2795,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
 	if (ret < 0)
 		return ret;
 
-	cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector);
+	cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector.base);
 	cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
 
 	if (vc4_hdmi->variant->external_irq_controller) {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 934d5d61485a..1e2752798bbb 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -76,7 +76,7 @@ struct vc4_hdmi_variant {
 
 	/* Callback to initialize the PHY according to the connector state */
 	void (*phy_init)(struct vc4_hdmi *vc4_hdmi,
-			 struct vc4_hdmi_connector_state *vc4_conn_state);
+			 struct drm_hdmi_connector_state *vc4_conn_state);
 
 	/* Callback to disable the PHY */
 	void (*phy_disable)(struct vc4_hdmi *vc4_hdmi);
@@ -117,12 +117,6 @@ enum vc4_hdmi_output_format {
 	VC4_HDMI_OUTPUT_YUV420,
 };
 
-enum vc4_hdmi_broadcast_rgb {
-	VC4_HDMI_BROADCAST_RGB_AUTO,
-	VC4_HDMI_BROADCAST_RGB_FULL,
-	VC4_HDMI_BROADCAST_RGB_LIMITED,
-};
-
 /* General HDMI hardware state. */
 struct vc4_hdmi {
 	struct vc4_hdmi_audio audio;
@@ -131,12 +125,10 @@ struct vc4_hdmi {
 	const struct vc4_hdmi_variant *variant;
 
 	struct vc4_encoder encoder;
-	struct drm_connector connector;
+	struct drm_hdmi_connector connector;
 
 	struct delayed_work scrambling_work;
 
-	struct drm_property *broadcast_rgb_property;
-
 	struct i2c_adapter *ddc;
 	void __iomem *hdmicore_regs;
 	void __iomem *hd_regs;
@@ -218,8 +210,8 @@ struct vc4_hdmi {
 	bool scdc_enabled;
 
 	/**
-	 * @output_bpc: Copy of @vc4_connector_state.output_bpc for use
-	 * outside of KMS hooks. Protected by @mutex.
+	 * @output_bpc: Copy of @drm_hdmi_connector_state.output_bpc for
+	 * use outside of KMS hooks. Protected by @mutex.
 	 */
 	unsigned int output_bpc;
 
@@ -230,9 +222,13 @@ struct vc4_hdmi {
 	enum vc4_hdmi_output_format output_format;
 };
 
-#define connector_to_vc4_hdmi(_connector)				\
+#define hdmi_connector_to_vc4_hdmi(_connector)				\
 	container_of_const(_connector, struct vc4_hdmi, connector)
 
+#define connector_to_vc4_hdmi(_connector)				\
+	container_of_const(_connector, struct vc4_hdmi, connector.base)
+
+
 static inline struct vc4_hdmi *
 encoder_to_vc4_hdmi(struct drm_encoder *encoder)
 {
@@ -240,25 +236,14 @@ encoder_to_vc4_hdmi(struct drm_encoder *encoder)
 	return container_of_const(_encoder, struct vc4_hdmi, encoder);
 }
 
-struct vc4_hdmi_connector_state {
-	struct drm_connector_state	base;
-	unsigned long long		tmds_char_rate;
-	unsigned int 			output_bpc;
-	enum vc4_hdmi_output_format	output_format;
-	enum vc4_hdmi_broadcast_rgb	broadcast_rgb;
-};
-
-#define conn_state_to_vc4_hdmi_conn_state(_state)			\
-	container_of_const(_state, struct vc4_hdmi_connector_state, base)
-
 void vc4_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
-		       struct vc4_hdmi_connector_state *vc4_conn_state);
+		       struct drm_hdmi_connector_state *conn_state);
 void vc4_hdmi_phy_disable(struct vc4_hdmi *vc4_hdmi);
 void vc4_hdmi_phy_rng_enable(struct vc4_hdmi *vc4_hdmi);
 void vc4_hdmi_phy_rng_disable(struct vc4_hdmi *vc4_hdmi);
 
 void vc5_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
-		       struct vc4_hdmi_connector_state *vc4_conn_state);
+		       struct drm_hdmi_connector_state *conn_state);
 void vc5_hdmi_phy_disable(struct vc4_hdmi *vc4_hdmi);
 void vc5_hdmi_phy_rng_enable(struct vc4_hdmi *vc4_hdmi);
 void vc5_hdmi_phy_rng_disable(struct vc4_hdmi *vc4_hdmi);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_phy.c b/drivers/gpu/drm/vc4/vc4_hdmi_phy.c
index ec24999bf96d..54deb30306df 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi_phy.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi_phy.c
@@ -128,7 +128,7 @@
 #define OSCILLATOR_FREQUENCY	54000000
 
 void vc4_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
-		       struct vc4_hdmi_connector_state *conn_state)
+		       struct drm_hdmi_connector_state *conn_state)
 {
 	unsigned long flags;
 
@@ -361,7 +361,7 @@ static void vc5_hdmi_reset_phy(struct vc4_hdmi *vc4_hdmi)
 }
 
 void vc5_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
-		       struct vc4_hdmi_connector_state *conn_state)
+		       struct drm_hdmi_connector_state *conn_state)
 {
 	const struct phy_lane_settings *chan0_settings, *chan1_settings, *chan2_settings, *clock_settings;
 	const struct vc4_hdmi_variant *variant = vc4_hdmi->variant;

-- 
2.41.0

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

[Hans Verkuil]

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 <mripard@kernel.org>
> ---
> 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,

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

[Daniel Vetter]

On Mon, Aug 14, 2023 at 03:56:12PM +0200, 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.
> 
> This entire series is only build-tested at the moment. Let me know what
> you think,
> Maxime

I think the idea overall makes sense, we we probably need it to roll out
actual hdmi support to all the hdmi drivers we have. But there's the
eternal issue of "C sucks at multiple inheritance".

Which means if you have a driver that subclasses drm_connector already for
it's driver needs it defacto cannot, or only under some serious pains, use
this. Which is kinda why in practice we tend to not subclass, but stuff
subclass fields into a name sub-structure. So essentially struct
drm_connector.hdmi and struct drm_connector_state.hdmi instead of
drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
set it all up would all still be the same roughly. It's less typesafe but
I think the gain in practical use (like you could make i915 use the
helpers probably, which with this approach here is practically
impossible).

The only other nit is that we probably want to put some of the hdmi
properties into struct drm_mode_config because there's no reason to have
per-connector valid values.

Also, it might be really good if you can find a co-conspirator who also
wants to use this in their driver, then with some i915 extracting we'd
have three, which should ensure the helper api is solid.

Cheers, Sima


> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> 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 <mripard@kernel.org>
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Maxime Ripard]

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

Hi,

On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote:
> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote:
> > 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,
>
> I think the idea overall makes sense, we we probably need it to roll out
> actual hdmi support to all the hdmi drivers we have. But there's the
> eternal issue of "C sucks at multiple inheritance".
> 
> Which means if you have a driver that subclasses drm_connector already for
> it's driver needs it defacto cannot, or only under some serious pains, use
> this.

That's what vc4 is doing, and it went fine I think? it was mostly a
matter of subclassing drm_hdmi_connector instead of drm_connector, and
adjusting the various pointers and accessors here and there.

It does create a fairly big diffstat, but nothing too painful.

> Which is kinda why in practice we tend to not subclass, but stuff
> subclass fields into a name sub-structure. So essentially struct
> drm_connector.hdmi and struct drm_connector_state.hdmi instead of
> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
> set it all up would all still be the same roughly. It's less typesafe but
> I think the gain in practical use (like you could make i915 use the
> helpers probably, which with this approach here is practically
> impossible).

Ack.

> The only other nit is that we probably want to put some of the hdmi
> properties into struct drm_mode_config because there's no reason to have
> per-connector valid values.

What property would you want to move?

> Also, it might be really good if you can find a co-conspirator who also
> wants to use this in their driver, then with some i915 extracting we'd
> have three, which should ensure the helper api is solid.

I can convert sunxi (old) HDMI driver if needed. I'm not sure how
helpful it would be since it doesn't support bpc > 8, but it could be a
nice showcase still for "simple" HDMI controllers.

Maxime

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

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

[Daniel Vetter]

On Tue, Aug 22, 2023 at 04:35:55PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote:
> > > 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,
> >
> > I think the idea overall makes sense, we we probably need it to roll out
> > actual hdmi support to all the hdmi drivers we have. But there's the
> > eternal issue of "C sucks at multiple inheritance".
> > 
> > Which means if you have a driver that subclasses drm_connector already for
> > it's driver needs it defacto cannot, or only under some serious pains, use
> > this.
> 
> That's what vc4 is doing, and it went fine I think? it was mostly a
> matter of subclassing drm_hdmi_connector instead of drm_connector, and
> adjusting the various pointers and accessors here and there.
> 
> It does create a fairly big diffstat, but nothing too painful.

Yeah it's the massive churn that's the pain for refactoring existing
bigger drivers.

Plus what do you do when you both need a hdmi connector and a dp connector
(or a writeback connector).

> > Which is kinda why in practice we tend to not subclass, but stuff
> > subclass fields into a name sub-structure. So essentially struct
> > drm_connector.hdmi and struct drm_connector_state.hdmi instead of
> > drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
> > set it all up would all still be the same roughly. It's less typesafe but
> > I think the gain in practical use (like you could make i915 use the
> > helpers probably, which with this approach here is practically
> > impossible).
> 
> Ack.
> 
> > The only other nit is that we probably want to put some of the hdmi
> > properties into struct drm_mode_config because there's no reason to have
> > per-connector valid values.
> 
> What property would you want to move?

The rgb broadcast property looked very much like it's connector invariant.
Just the one I noticed, I didn't check all the others.

> > Also, it might be really good if you can find a co-conspirator who also
> > wants to use this in their driver, then with some i915 extracting we'd
> > have three, which should ensure the helper api is solid.
> 
> I can convert sunxi (old) HDMI driver if needed. I'm not sure how
> helpful it would be since it doesn't support bpc > 8, but it could be a
> nice showcase still for "simple" HDMI controllers.

Yeah that might be good. Or perhaps poke Rob Clark whether msm is
interested and someone could do a conversion for dpu5 or so?

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Jani Nikula]

On Tue, 22 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote:
>> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote:
>> > 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,
>>
>> I think the idea overall makes sense, we we probably need it to roll out
>> actual hdmi support to all the hdmi drivers we have. But there's the
>> eternal issue of "C sucks at multiple inheritance".
>> 
>> Which means if you have a driver that subclasses drm_connector already for
>> it's driver needs it defacto cannot, or only under some serious pains, use
>> this.
>
> That's what vc4 is doing, and it went fine I think? it was mostly a
> matter of subclassing drm_hdmi_connector instead of drm_connector, and
> adjusting the various pointers and accessors here and there.
>
> It does create a fairly big diffstat, but nothing too painful.

The main pain point is not the diffstat per se, but that *all* casts to
subclass need to check what the connector type is before doing
so. You'll also get fun NULL conditions that you need to check and
handle if the type isn't what you'd like it to be.

Currently i915 can just assume all drm_connectors it encounters are
intel_connectors that it created, always.

Basically this has blocked the writeback connector stuff for a few years
now in i915, because writeback forces a different subclassing, and what
should be a small change in i915 turns into huge churn.

BR,
Jani.


>
>> Which is kinda why in practice we tend to not subclass, but stuff
>> subclass fields into a name sub-structure. So essentially struct
>> drm_connector.hdmi and struct drm_connector_state.hdmi instead of
>> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
>> set it all up would all still be the same roughly. It's less typesafe but
>> I think the gain in practical use (like you could make i915 use the
>> helpers probably, which with this approach here is practically
>> impossible).
>
> Ack.
>
>> The only other nit is that we probably want to put some of the hdmi
>> properties into struct drm_mode_config because there's no reason to have
>> per-connector valid values.
>
> What property would you want to move?
>
>> Also, it might be really good if you can find a co-conspirator who also
>> wants to use this in their driver, then with some i915 extracting we'd
>> have three, which should ensure the helper api is solid.
>
> I can convert sunxi (old) HDMI driver if needed. I'm not sure how
> helpful it would be since it doesn't support bpc > 8, but it could be a
> nice showcase still for "simple" HDMI controllers.
>
> Maxime

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Daniel Vetter]

On Tue, Aug 22, 2023 at 05:51:39PM +0300, Jani Nikula wrote:
> On Tue, 22 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote:
> > Hi,
> >
> > On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote:
> >> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote:
> >> > 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,
> >>
> >> I think the idea overall makes sense, we we probably need it to roll out
> >> actual hdmi support to all the hdmi drivers we have. But there's the
> >> eternal issue of "C sucks at multiple inheritance".
> >> 
> >> Which means if you have a driver that subclasses drm_connector already for
> >> it's driver needs it defacto cannot, or only under some serious pains, use
> >> this.
> >
> > That's what vc4 is doing, and it went fine I think? it was mostly a
> > matter of subclassing drm_hdmi_connector instead of drm_connector, and
> > adjusting the various pointers and accessors here and there.
> >
> > It does create a fairly big diffstat, but nothing too painful.
> 
> The main pain point is not the diffstat per se, but that *all* casts to
> subclass need to check what the connector type is before doing
> so. You'll also get fun NULL conditions that you need to check and
> handle if the type isn't what you'd like it to be.
> 
> Currently i915 can just assume all drm_connectors it encounters are
> intel_connectors that it created, always.
> 
> Basically this has blocked the writeback connector stuff for a few years
> now in i915, because writeback forces a different subclassing, and what
> should be a small change in i915 turns into huge churn.

Yeah after the writeback experience I'm heavily leaning towards "this was
a mistake".

For writeback we could refactor it I think by just moving it all (which I
hope isn't too much churn), and then removing the then empty types (which
is where the big churn kicks in, so maybe just add that to gpu/todo.rst).

Cheers, Sima

> 
> BR,
> Jani.
> 
> 
> >
> >> Which is kinda why in practice we tend to not subclass, but stuff
> >> subclass fields into a name sub-structure. So essentially struct
> >> drm_connector.hdmi and struct drm_connector_state.hdmi instead of
> >> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to
> >> set it all up would all still be the same roughly. It's less typesafe but
> >> I think the gain in practical use (like you could make i915 use the
> >> helpers probably, which with this approach here is practically
> >> impossible).
> >
> > Ack.
> >
> >> The only other nit is that we probably want to put some of the hdmi
> >> properties into struct drm_mode_config because there's no reason to have
> >> per-connector valid values.
> >
> > What property would you want to move?
> >
> >> Also, it might be really good if you can find a co-conspirator who also
> >> wants to use this in their driver, then with some i915 extracting we'd
> >> have three, which should ensure the helper api is solid.
> >
> > I can convert sunxi (old) HDMI driver if needed. I'm not sure how
> > helpful it would be since it doesn't support bpc > 8, but it could be a
> > nice showcase still for "simple" HDMI controllers.
> >
> > Maxime
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch