[PATCH RFC 0/5] drm: add DRM HDMI Codec framework

[PATCH RFC 0/5] drm: add DRM HDMI Codec framework

[Dmitry Baryshkov]

While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector
framework, I stumbled upon an issue while handling the Audio InfoFrames.
The HDMI codec callbacks weren't receiving the drm_atomic_state, so
there was no simple way to get the drm_connector that stayed at the end
of the bridge chain. At the same point the drm_hdmi_connector functions
expected to get drm_connector instance.

While looking for a way to solve the issue, I stumbled upon several
deficiencies in existing hdmi_codec_ops implementations. Only few of the
implementations were able to handle codec's 'plugged' callback. One
third of the drivers didn't implement the get_eld() callback.

Most of the issues can be solved if drm_connector handles
hdmi-audio-codec on its own, delegating functionality to the actual
implementation, be it a driver that implements drm_connector or
drm_bridge.

Implement such high-level framework, adding proper support for Audio
InfoFrame generation to the LT9611 driver.

Several design decisions to be kept in mind:

- drm_connector_hdmi_codec is kept as simple as possible. It implements
  generic functionality (ELD, hotplug, registration).

- drm_hdmi_connector allocates drm_connector_hdmi_codec if the connector
  is setup correspondingly (either I2S or S/PDIF is marked as
  supported).

- drm_bridge_connector provides a way to link HDMI audio codec
  funcionality in the drm_bridge with the drm_connector_hdmi_codec
  framework.

- The drm_bridge didn't implemnent support for no_capture_mute flag. Nor
  there is support for no_i2s_playback/_capture and no_spdif_playback /
  _capture flags. I think it's worth moving no_capture_mute to the
  hdmi_codec_pdata, together with the rest of the codec data. This will
  allow us to keep single implementation of ops in the
  drm_bridge_connector.
  At the same time it might be worth reverting the
  no_i2s_capture / no_spdif_capture bits. Only TDA889x driver sets them,
  while it's safe to assume that most of HDMI / DP devices do not
  support ARC / capture. I think the drivers should opt-in capture
  support rather than having to opt-out of it.

- The bridge's driver has the unbalanced call to
  drmm_connector_hdmi_codec_free(). This is because of the difference in
  lifetime cycles. The drm_connector is tied to the lifetime of the DRM
  device, by using the drmm_ calls. However the bridge can be removed,
  while the DRM device is still lingering on its path to destruction.

This series is in the RFC stage, so some bits are underdocumented.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (5):
      drm/bridge: lt9611: use HDMI Connector helper to set InfoFrames
      ASoC: hdmi-codec: pass data to get_dai_id too
      drm/connector: implement generic HDMI codec helpers
      drm/bridge: connector: add support for HDMI codec framework
      drm/bridge: lt9611: switch to using the DRM HDMI codec framework

 drivers/gpu/drm/Makefile                           |   1 +
 drivers/gpu/drm/bridge/Kconfig                     |   2 +
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c     |   3 +-
 drivers/gpu/drm/bridge/analogix/anx7625.c          |   3 +-
 drivers/gpu/drm/bridge/lontium-lt9611.c            | 347 ++++++++++++---------
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c         |   3 +-
 drivers/gpu/drm/bridge/sii902x.c                   |   3 +-
 .../gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c    |   3 +-
 drivers/gpu/drm/drm_bridge_connector.c             | 130 +++++++-
 drivers/gpu/drm/drm_connector.c                    |   8 +
 drivers/gpu/drm/drm_connector_hdmi_codec.c         | 157 ++++++++++
 include/drm/drm_bridge.h                           |  46 +++
 include/drm/drm_connector.h                        |  33 ++
 include/sound/hdmi-codec.h                         |   3 +-
 sound/soc/codecs/hdmi-codec.c                      |   2 +-
 15 files changed, 594 insertions(+), 150 deletions(-)
---
base-commit: f83b272021ad706ff952bc53b707a3a8657eef71
change-id: 20240530-drm-bridge-hdmi-connector-9b0f6725973e

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH RFC 1/5] drm/bridge: lt9611: use HDMI Connector helper to set InfoFrames

[Dmitry Baryshkov]

Use new HDMI Connector helpers in the Lontium LT9611 bridge driver.
Program InfoFrames using the helper's callbacks. Also use TMDS char rate
validation callback to filter out modes, instead of hardcoding 4k@30.

The Audio InfoFrame isn't yet handled by these helpers, it requires
additional drm_bridge interface changes.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/Kconfig          |   2 +
 drivers/gpu/drm/bridge/lontium-lt9611.c | 173 ++++++++++++++++++++++----------
 2 files changed, 120 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index c621be1a99a8..b27b8a072101 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -140,6 +140,8 @@ config DRM_LONTIUM_LT9611
 	select DRM_PANEL_BRIDGE
 	select DRM_KMS_HELPER
 	select DRM_MIPI_DSI
+	select DRM_DISPLAY_HELPER
+	select DRM_DISPLAY_HDMI_STATE_HELPER
 	select REGMAP_I2C
 	help
 	  Driver for Lontium LT9611 DSI to HDMI bridge
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 73983f9b50cb..1b31fdebe164 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -23,6 +23,8 @@
 #include <drm/drm_of.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 
 #define EDID_SEG_SIZE	256
 #define EDID_LEN	32
@@ -333,49 +335,6 @@ static int lt9611_video_check(struct lt9611 *lt9611)
 	return temp;
 }
 
-static void lt9611_hdmi_set_infoframes(struct lt9611 *lt9611,
-				       struct drm_connector *connector,
-				       struct drm_display_mode *mode)
-{
-	union hdmi_infoframe infoframe;
-	ssize_t len;
-	u8 iframes = 0x0a; /* UD1 infoframe */
-	u8 buf[32];
-	int ret;
-	int i;
-
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&infoframe.avi,
-						       connector,
-						       mode);
-	if (ret < 0)
-		goto out;
-
-	len = hdmi_infoframe_pack(&infoframe, buf, sizeof(buf));
-	if (len < 0)
-		goto out;
-
-	for (i = 0; i < len; i++)
-		regmap_write(lt9611->regmap, 0x8440 + i, buf[i]);
-
-	ret = drm_hdmi_vendor_infoframe_from_display_mode(&infoframe.vendor.hdmi,
-							  connector,
-							  mode);
-	if (ret < 0)
-		goto out;
-
-	len = hdmi_infoframe_pack(&infoframe, buf, sizeof(buf));
-	if (len < 0)
-		goto out;
-
-	for (i = 0; i < len; i++)
-		regmap_write(lt9611->regmap, 0x8474 + i, buf[i]);
-
-	iframes |= 0x20;
-
-out:
-	regmap_write(lt9611->regmap, 0x843d, iframes); /* UD1 infoframe */
-}
-
 static void lt9611_hdmi_tx_digital(struct lt9611 *lt9611, bool is_hdmi)
 {
 	if (is_hdmi)
@@ -719,7 +678,7 @@ lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
 	}
 
 	lt9611_mipi_input_analog(lt9611);
-	lt9611_hdmi_set_infoframes(lt9611, connector, mode);
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
 	lt9611_hdmi_tx_digital(lt9611, connector->display_info.is_hdmi);
 	lt9611_hdmi_tx_phy(lt9611);
 
@@ -798,22 +757,25 @@ static enum drm_mode_status lt9611_bridge_mode_valid(struct drm_bridge *bridge,
 						     const struct drm_display_mode *mode)
 {
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+	unsigned long long rate;
 
 	if (mode->hdisplay > 3840)
 		return MODE_BAD_HVALUE;
 
-	if (mode->vdisplay > 2160)
-		return MODE_BAD_VVALUE;
-
-	if (mode->hdisplay == 3840 &&
-	    mode->vdisplay == 2160 &&
-	    drm_mode_vrefresh(mode) > 30)
-		return MODE_CLOCK_HIGH;
-
 	if (mode->hdisplay > 2000 && !lt9611->dsi1_node)
 		return MODE_PANEL;
-	else
-		return MODE_OK;
+
+	rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
+	return bridge->funcs->hdmi_tmds_char_rate_valid(bridge, mode, rate);
+}
+
+static int lt9611_bridge_atomic_check(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state)
+{
+	return drm_atomic_helper_connector_hdmi_check(conn_state->connector,
+						      conn_state->state);
 }
 
 static void lt9611_bridge_atomic_pre_enable(struct drm_bridge *bridge,
@@ -887,6 +849,99 @@ lt9611_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+/*
+ * Other working frames:
+ * - 0x01, 0x84df
+ * - 0x04, 0x84c0
+ */
+#define LT9611_INFOFRAME_AUDIO	0x02
+#define LT9611_INFOFRAME_AVI	0x08
+#define LT9611_INFOFRAME_SPD	0x10
+#define LT9611_INFOFRAME_VENDOR	0x20
+
+static int lt9611_hdmi_clear_infoframe(struct drm_bridge *bridge,
+				       enum hdmi_infoframe_type type)
+{
+	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+	unsigned int mask;
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		mask = LT9611_INFOFRAME_AVI;
+		break;
+
+	case HDMI_INFOFRAME_TYPE_SPD:
+		mask = LT9611_INFOFRAME_SPD;
+		break;
+
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		mask = LT9611_INFOFRAME_VENDOR;
+		break;
+
+	default:
+		drm_dbg_driver(lt9611->bridge.dev, "Unsupported HDMI InfoFrame %x\n", type);
+		mask = 0;
+		break;
+	}
+
+	if (mask)
+		regmap_update_bits(lt9611->regmap, 0x843d, mask, 0);
+
+	return 0;
+}
+
+static int lt9611_hdmi_write_infoframe(struct drm_bridge *bridge,
+				       enum hdmi_infoframe_type type,
+				       const u8 *buffer, size_t len)
+{
+	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+	unsigned int mask, addr;
+	int i;
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		mask = LT9611_INFOFRAME_AVI;
+		addr = 0x8440;
+		break;
+
+	case HDMI_INFOFRAME_TYPE_SPD:
+		mask = LT9611_INFOFRAME_SPD;
+		addr = 0x8493;
+		break;
+
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		mask = LT9611_INFOFRAME_VENDOR;
+		addr = 0x8474;
+		break;
+
+	default:
+		drm_dbg_driver(lt9611->bridge.dev, "Unsupported HDMI InfoFrame %x\n", type);
+		mask = 0;
+		break;
+	}
+
+	if (mask) {
+		for (i = 0; i < len; i++)
+			regmap_write(lt9611->regmap, addr + i, buffer[i]);
+
+		regmap_update_bits(lt9611->regmap, 0x843d, mask, mask);
+	}
+
+	return 0;
+}
+
+static enum drm_mode_status
+lt9611_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+				 const struct drm_display_mode *mode,
+				 unsigned long long tmds_rate)
+{
+	/* 297 MHz for 4k@30 mode */
+	if (tmds_rate > 297000000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
 static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.attach = lt9611_bridge_attach,
 	.mode_valid = lt9611_bridge_mode_valid,
@@ -894,6 +949,7 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.edid_read = lt9611_bridge_edid_read,
 	.hpd_enable = lt9611_bridge_hpd_enable,
 
+	.atomic_check = lt9611_bridge_atomic_check,
 	.atomic_pre_enable = lt9611_bridge_atomic_pre_enable,
 	.atomic_enable = lt9611_bridge_atomic_enable,
 	.atomic_disable = lt9611_bridge_atomic_disable,
@@ -902,6 +958,10 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.atomic_get_input_bus_fmts = lt9611_atomic_get_input_bus_fmts,
+
+	.hdmi_tmds_char_rate_valid = lt9611_hdmi_tmds_char_rate_valid,
+	.hdmi_write_infoframe = lt9611_hdmi_write_infoframe,
+	.hdmi_clear_infoframe = lt9611_hdmi_clear_infoframe,
 };
 
 static int lt9611_parse_dt(struct device *dev,
@@ -1116,8 +1176,11 @@ static int lt9611_probe(struct i2c_client *client)
 	lt9611->bridge.funcs = &lt9611_bridge_funcs;
 	lt9611->bridge.of_node = client->dev.of_node;
 	lt9611->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
-			     DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES;
+			     DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES |
+			     DRM_BRIDGE_OP_HDMI;
 	lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
+	lt9611->bridge.vendor = "Lontium";
+	lt9611->bridge.product = "LT9611";
 
 	drm_bridge_add(&lt9611->bridge);
 

-- 
2.39.2

[PATCH RFC 2/5] ASoC: hdmi-codec: pass data to get_dai_id too

[Dmitry Baryshkov]

The upcoming DRM connector HDMI codec implementation is going to use
codec-specific data in the .get_dai_id to get drm_connector. Pass data
to the callback, as it is done with other hdmi_codec_ops callbacks.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c      | 3 ++-
 drivers/gpu/drm/bridge/analogix/anx7625.c           | 3 ++-
 drivers/gpu/drm/bridge/lontium-lt9611.c             | 3 ++-
 drivers/gpu/drm/bridge/lontium-lt9611uxc.c          | 3 ++-
 drivers/gpu/drm/bridge/sii902x.c                    | 3 ++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 3 ++-
 include/sound/hdmi-codec.h                          | 3 ++-
 sound/soc/codecs/hdmi-codec.c                       | 2 +-
 8 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
index 61f4a38e7d2b..51fb9a574b4e 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
@@ -204,7 +204,8 @@ static void audio_shutdown(struct device *dev, void *data)
 }
 
 static int adv7511_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-					struct device_node *endpoint)
+					struct device_node *endpoint,
+					void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 88e4aa5830f3..f18cf79a292b 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -1962,7 +1962,8 @@ static void anx7625_audio_shutdown(struct device *dev, void *data)
 }
 
 static int anx7625_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-				       struct device_node *endpoint)
+				       struct device_node *endpoint,
+				       void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 1b31fdebe164..4fa0dfc5539a 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -1059,7 +1059,8 @@ static void lt9611_audio_shutdown(struct device *dev, void *data)
 }
 
 static int lt9611_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-				      struct device_node *endpoint)
+				      struct device_node *endpoint,
+				      void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index 4e802b54a1cb..54c528fa168e 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -601,7 +601,8 @@ static void lt9611uxc_audio_shutdown(struct device *dev, void *data)
 }
 
 static int lt9611uxc_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-					 struct device_node *endpoint)
+					 struct device_node *endpoint,
+					 void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 7f91b0db161e..1883df5fd5c1 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -798,7 +798,8 @@ static int sii902x_audio_get_eld(struct device *dev, void *data,
 }
 
 static int sii902x_audio_get_dai_id(struct snd_soc_component *component,
-				    struct device_node *endpoint)
+				    struct device_node *endpoint,
+				    void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 26c187d20d97..86c412e9cbc8 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -148,7 +148,8 @@ static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
 }
 
 static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-				  struct device_node *endpoint)
+				  struct device_node *endpoint,
+				  void *data)
 {
 	struct of_endpoint of_ep;
 	int ret;
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 5e1a9eafd10f..b3407b47b4a7 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -105,7 +105,8 @@ struct hdmi_codec_ops {
 	 * Optional
 	 */
 	int (*get_dai_id)(struct snd_soc_component *comment,
-			  struct device_node *endpoint);
+			  struct device_node *endpoint,
+			  void *data);
 
 	/*
 	 * Hook callback function to handle connector plug event.
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index d3abb7ce2153..afad402122cb 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -981,7 +981,7 @@ static int hdmi_of_xlate_dai_id(struct snd_soc_component *component,
 	int ret = -ENOTSUPP; /* see snd_soc_get_dai_id() */
 
 	if (hcp->hcd.ops->get_dai_id)
-		ret = hcp->hcd.ops->get_dai_id(component, endpoint);
+		ret = hcp->hcd.ops->get_dai_id(component, endpoint, hcp->hcd.data);
 
 	return ret;
 }

-- 
2.39.2

[PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

Several DRM drivers implement HDMI codec support (despite its name it
applies to both HDMI and DisplayPort drivers). Implement generic
framework to be used by these drivers. This removes a requirement to
implement get_eld() callback and provides default implementation for
codec's plug handling.

The framework is integrated with the DRM HDMI Connector framework, but
can be used by DisplayPort drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/Makefile                   |   1 +
 drivers/gpu/drm/drm_connector.c            |   8 ++
 drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
 include/drm/drm_connector.h                |  33 ++++++
 4 files changed, 199 insertions(+)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 68cc9258ffc4..e113a6eade23 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -45,6 +45,7 @@ drm-y := \
 	drm_client_modeset.o \
 	drm_color_mgmt.o \
 	drm_connector.o \
+	drm_connector_hdmi_codec.o \
 	drm_crtc.o \
 	drm_displayid.o \
 	drm_drv.o \
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3d73a981004c..66d6e9487339 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
 	mutex_init(&connector->mutex);
 	mutex_init(&connector->edid_override_mutex);
 	mutex_init(&connector->hdmi.infoframes.lock);
+	mutex_init(&connector->hdmi_codec.lock);
 	connector->edid_blob_ptr = NULL;
 	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
@@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
 
 	connector->hdmi.funcs = hdmi_funcs;
 
+	if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
+		ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drmm_connector_hdmi_init);
@@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->funcs->atomic_destroy_state(connector,
 						       connector->state);
 
+	mutex_destroy(&connector->hdmi_codec.lock);
 	mutex_destroy(&connector->hdmi.infoframes.lock);
 	mutex_destroy(&connector->mutex);
 
diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
new file mode 100644
index 000000000000..a3a7ad117f6f
--- /dev/null
+++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
@@ -0,0 +1,157 @@
+/*
+ * Copyright (c) 2024 Linaro Ltd
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission.  The copyright holders make no representations
+ * about the suitability of this software for any purpose.  It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_managed.h>
+
+#include <sound/hdmi-codec.h>
+
+static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
+					    uint8_t *buf, size_t len)
+{
+	struct drm_connector *connector = data;
+
+	//  FIXME: locking against drm_edid_to_eld ?
+	memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
+
+	return 0;
+}
+
+static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
+						    void *data,
+						    hdmi_codec_plugged_cb fn,
+						    struct device *codec_dev)
+{
+	struct drm_connector *connector = data;
+
+	mutex_lock(&connector->hdmi_codec.lock);
+
+	connector->hdmi_codec.plugged_cb = fn;
+	connector->hdmi_codec.plugged_cb_dev = codec_dev;
+
+	fn(codec_dev, connector->hdmi_codec.last_state);
+
+	mutex_unlock(&connector->hdmi_codec.lock);
+
+	return 0;
+}
+
+void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
+					     bool plugged)
+{
+	mutex_lock(&connector->hdmi_codec.lock);
+
+	connector->hdmi_codec.last_state = plugged;
+
+	if (connector->hdmi_codec.plugged_cb &&
+	    connector->hdmi_codec.plugged_cb_dev)
+		connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
+						 connector->hdmi_codec.last_state);
+
+	mutex_unlock(&connector->hdmi_codec.lock);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
+
+static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
+						    void *ptr)
+{
+	struct platform_device *pdev = ptr;
+
+	platform_device_unregister(pdev);
+}
+
+/**
+ * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
+ * @dev: DRM device
+ * @connector: A pointer to the connector to allocate codec for
+ * @ops: callbacks for this connector
+ *
+ * Create a HDMI codec device to be used with the specified connector.
+ *
+ * Cleanup is automatically handled with in a DRM-managed action.
+ *
+ * The connector structure should be allocated with drmm_kzalloc().
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
+				    struct drm_connector *connector,
+				    const struct hdmi_codec_ops *base_ops)
+{
+	struct hdmi_codec_pdata codec_pdata = {};
+	struct platform_device *pdev;
+	struct hdmi_codec_ops *ops;
+	int ret;
+
+	ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
+	*ops = *base_ops;
+
+	ops->get_eld = drm_connector_hdmi_codec_get_eld;
+	ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
+
+	codec_pdata.ops = ops;
+	codec_pdata.i2s = connector->hdmi_codec.i2s,
+	codec_pdata.spdif = connector->hdmi_codec.spdif,
+	codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
+	codec_pdata.data = connector;
+
+	pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
+					     HDMI_CODEC_DRV_NAME,
+					     PLATFORM_DEVID_AUTO,
+					     &codec_pdata, sizeof(codec_pdata));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
+	if (ret)
+		return ret;
+
+	connector->hdmi_codec.codec_pdev = pdev;
+
+	return 0;
+}
+EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
+
+/**
+ * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
+ * @dev: DRM device
+ * @hdmi_codec: A pointer to the HDMI codec data
+ *
+ * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
+ * While this function should not be necessary for a typical driver, DRM bridge
+ * drivers have to call it from the remove callback if the bridge uses
+ * Connector's HDMI Codec interface.
+ */
+void drmm_connector_hdmi_codec_free(struct drm_device *dev,
+				    struct drm_connector_hdmi_codec *hdmi_codec)
+{
+	drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
+			    hdmi_codec->codec_pdev);
+}
+EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index f750765d8fbc..0eb8d8ed9495 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -46,6 +46,7 @@ struct drm_property_blob;
 struct drm_printer;
 struct drm_privacy_screen;
 struct edid;
+struct hdmi_codec_ops;
 struct i2c_adapter;
 
 enum drm_connector_force {
@@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
 	int (*write_infoframe)(struct drm_connector *connector,
 			       enum hdmi_infoframe_type type,
 			       const u8 *buffer, size_t len);
+
+	const struct hdmi_codec_ops *codec_ops;
 };
 
 /**
@@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
 	} infoframes;
 };
 
+struct drm_connector_hdmi_codec {
+	struct device *parent_dev;
+	struct platform_device *codec_pdev;
+
+	const struct drm_connector_hdmi_codec_funcs *funcs;
+
+	struct mutex lock; /* protects last_state and plugged_cb */
+	void (*plugged_cb)(struct device *dev, bool plugged);
+	struct device *plugged_cb_dev;
+	bool last_state;
+
+	int max_i2s_channels;
+	uint i2s: 1;
+	uint spdif: 1;
+};
+
 /**
  * struct drm_connector - central DRM connector control structure
  *
@@ -2119,6 +2138,12 @@ struct drm_connector {
 	 * @hdmi: HDMI-related variable and properties.
 	 */
 	struct drm_connector_hdmi hdmi;
+
+	/**
+	 * @hdmi_codec: HDMI codec properties and variables. Also might be used
+	 * for DisplayPort audio.
+	 */
+	struct drm_connector_hdmi_codec hdmi_codec;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
@@ -2152,6 +2177,14 @@ void drm_connector_unregister(struct drm_connector *connector);
 int drm_connector_attach_encoder(struct drm_connector *connector,
 				      struct drm_encoder *encoder);
 
+int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
+				    struct drm_connector *connector,
+				    const struct hdmi_codec_ops *ops);
+void drmm_connector_hdmi_codec_free(struct drm_device *dev,
+				    struct drm_connector_hdmi_codec *codec);
+void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
+					     bool plugged);
+
 void drm_connector_cleanup(struct drm_connector *connector);
 
 static inline unsigned int drm_connector_index(const struct drm_connector *connector)

-- 
2.39.2

[PATCH RFC 4/5] drm/bridge: connector: add support for HDMI codec framework

[Dmitry Baryshkov]

Add necessary glue code to be able to use new HDMI codec framework from
the DRM bridge drivers. The drm_bridge implements a limited set of the
hdmi_codec_ops interface, with the functions accepting both
drm_connector and drm_bridge instead of just a generic void pointer.

This framework is integrated with the DRM HDMI Connector framework, but
can also be used for DisplayPort connectors.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge_connector.c | 130 ++++++++++++++++++++++++++++++++-
 include/drm/drm_bridge.h               |  46 ++++++++++++
 2 files changed, 174 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 0869b663f17e..71d6fdc2391f 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -20,6 +20,8 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
+#include <sound/hdmi-codec.h>
+
 /**
  * DOC: overview
  *
@@ -95,6 +97,14 @@ struct drm_bridge_connector {
 	 * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
 	 */
 	struct drm_bridge *bridge_hdmi;
+	/**
+	 * @bridge_hdmi_codec:
+	 *
+	 * The bridge in the chain that implements necessary support for the
+	 * HDMI Audio Codec infrastructure, if any (see
+	 * &DRM_BRIDGE_OP_HDMI_CODEC).
+	 */
+	struct drm_bridge *bridge_hdmi_codec;
 };
 
 #define to_drm_bridge_connector(x) \
@@ -343,10 +353,104 @@ static int drm_bridge_connector_write_infoframe(struct drm_connector *connector,
 	return bridge->funcs->hdmi_write_infoframe(bridge, type, buffer, len);
 }
 
+static int drm_bridge_connector_audio_startup(struct device *dev, void *data)
+{
+	struct drm_connector *connector = data;
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi_codec;
+	if (!bridge)
+		return -EINVAL;
+
+	if (bridge->funcs->hdmi_codec_audio_startup)
+		return bridge->funcs->hdmi_codec_audio_startup(dev, connector, bridge);
+	else
+		return 0;
+}
+
+static int drm_bridge_connector_prepare(struct device *dev, void *data,
+					struct hdmi_codec_daifmt *fmt,
+					struct hdmi_codec_params *hparms)
+{
+	struct drm_connector *connector = data;
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi_codec;
+	if (!bridge)
+		return -EINVAL;
+
+	return bridge->funcs->hdmi_codec_prepare(dev, connector, bridge, fmt, hparms);
+}
+
+static void drm_bridge_connector_audio_shutdown(struct device *dev, void *data)
+{
+	struct drm_connector *connector = data;
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi_codec;
+	if (!bridge)
+		return;
+
+	bridge->funcs->hdmi_codec_audio_shutdown(dev, connector, bridge);
+}
+
+static int drm_bridge_connector_mute_stream(struct device *dev, void *data,
+					    bool enable, int direction)
+{
+	struct drm_connector *connector = data;
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi_codec;
+	if (!bridge)
+		return -EINVAL;
+
+	if (bridge->funcs->hdmi_codec_mute_stream)
+		return bridge->funcs->hdmi_codec_mute_stream(dev, connector, bridge,
+							     enable, direction);
+	else
+		return -ENOTSUPP;
+}
+
+static int drm_bridge_connector_get_dai_id(struct snd_soc_component *comment,
+					   struct device_node *endpoint,
+					   void *data)
+{
+	struct drm_connector *connector = data;
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi_codec;
+	if (!bridge)
+		return -EINVAL;
+
+	if (bridge->funcs->hdmi_codec_get_dai_id)
+		return bridge->funcs->hdmi_codec_get_dai_id(connector, bridge, endpoint);
+	else
+		return -ENOTSUPP;
+}
+
+static const struct hdmi_codec_ops drm_bridge_connector_hdmi_codec_ops = {
+	.audio_startup = drm_bridge_connector_audio_startup,
+	.prepare = drm_bridge_connector_prepare,
+	.audio_shutdown = drm_bridge_connector_audio_shutdown,
+	.mute_stream = drm_bridge_connector_mute_stream,
+	.get_dai_id = drm_bridge_connector_get_dai_id,
+};
+
 static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
 	.tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
 	.clear_infoframe = drm_bridge_connector_clear_infoframe,
 	.write_infoframe = drm_bridge_connector_write_infoframe,
+	.codec_ops = &drm_bridge_connector_hdmi_codec_ops,
 };
 
 /* -----------------------------------------------------------------------------
@@ -427,6 +531,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 				max_bpc = bridge->max_bpc;
 		}
 
+		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CODEC) {
+			if (bridge_connector->bridge_hdmi_codec)
+				return ERR_PTR(-EBUSY);
+			if (!bridge->funcs->hdmi_codec_prepare ||
+			    !bridge->funcs->hdmi_codec_audio_shutdown)
+				return ERR_PTR(-EINVAL);
+
+			bridge_connector->bridge_hdmi_codec = bridge;
+
+			connector->hdmi_codec.parent_dev = bridge->parent;
+			connector->hdmi_codec.i2s = bridge->hdmi_codec_i2s;
+			connector->hdmi_codec.spdif = bridge->hdmi_codec_spdif;
+			connector->hdmi_codec.max_i2s_channels = bridge->max_i2s_channels;
+
+			bridge->hdmi_codec = &connector->hdmi_codec;
+		}
+
 		if (!drm_bridge_get_next_bridge(bridge))
 			connector_type = bridge->type;
 
@@ -448,7 +569,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (bridge_connector->bridge_hdmi)
+	if (bridge_connector->bridge_hdmi) {
 		ret = drmm_connector_hdmi_init(drm, connector,
 					       bridge_connector->bridge_hdmi->vendor,
 					       bridge_connector->bridge_hdmi->product,
@@ -457,10 +578,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 					       connector_type, ddc,
 					       supported_formats,
 					       max_bpc);
-	else
+	} else {
 		ret = drmm_connector_init(drm, connector,
 					  &drm_bridge_connector_funcs,
 					  connector_type, ddc);
+		if (!ret && bridge_connector->bridge_hdmi_codec) {
+			ret = drmm_connector_hdmi_codec_alloc(drm, connector,
+							      &drm_bridge_connector_hdmi_codec_ops);
+		}
+	}
 	if (ret) {
 		kfree(bridge_connector);
 		return ERR_PTR(ret);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 75019d16be64..c4a95c489b00 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -41,6 +41,8 @@ struct drm_display_info;
 struct drm_minor;
 struct drm_panel;
 struct edid;
+struct hdmi_codec_daifmt;
+struct hdmi_codec_params;
 struct i2c_adapter;
 
 /**
@@ -676,6 +678,29 @@ struct drm_bridge_funcs {
 				    enum hdmi_infoframe_type type,
 				    const u8 *buffer, size_t len);
 
+	int (*hdmi_codec_audio_startup)(struct device *dev,
+					struct drm_connector *connector,
+					struct drm_bridge *bridge);
+
+	int (*hdmi_codec_prepare)(struct device *dev,
+				  struct drm_connector *connector,
+				  struct drm_bridge *bridge,
+				  struct hdmi_codec_daifmt *fmt,
+				  struct hdmi_codec_params *hparms);
+
+	void (*hdmi_codec_audio_shutdown)(struct device *dev,
+					  struct drm_connector *connector,
+					  struct drm_bridge *bridge);
+
+	int (*hdmi_codec_mute_stream)(struct device *dev,
+				      struct drm_connector *connector,
+				      struct drm_bridge *bridge,
+				      bool enable, int direction);
+
+	int (*hdmi_codec_get_dai_id)(struct drm_connector *connector,
+				     struct drm_bridge *bridge,
+				     struct device_node *endpoint);
+
 	/**
 	 * @debugfs_init:
 	 *
@@ -761,6 +786,20 @@ enum drm_bridge_ops {
 	 * drivers.
 	 */
 	DRM_BRIDGE_OP_HDMI = BIT(4),
+	/**
+	 * @DRM_BRIDGE_OP_HDMI_CODEC: The bridge provides HDMI Audio Codec
+	 * operations. Bridges that set this flag must implement the
+	 * &drm_bridge_funcs->hdmi_codec_prepare and
+	 * &drm_bridge_funcs->hdmi_codec_audio_shutdown callbacks and set the
+	 * relevant field in the &drm_bridge structure.
+	 *
+	 * This framework can be used by both HDMI and DisplayPort bridges.
+	 *
+	 * Note: currently there can be at most one bridge in a chain that sets
+	 * this bit. This is to simplify corresponding glue code in connector
+	 * drivers.
+	 */
+	DRM_BRIDGE_OP_HDMI_CODEC = BIT(5),
 };
 
 /**
@@ -771,6 +810,8 @@ struct drm_bridge {
 	struct drm_private_obj base;
 	/** @dev: DRM device this bridge belongs to */
 	struct drm_device *dev;
+	/** @parent: device corresponding to the bridge, required only for HDMI codec */
+	struct device *parent;
 	/** @encoder: encoder to which this bridge is connected */
 	struct drm_encoder *encoder;
 	/** @chain_node: used to form a bridge chain */
@@ -854,6 +895,11 @@ struct drm_bridge {
 	 * @DRM_BRIDGE_OP_HDMI is set.
 	 */
 	unsigned int max_bpc;
+
+	int max_i2s_channels;
+	unsigned int hdmi_codec_i2s : 1;
+	unsigned int hdmi_codec_spdif : 1;
+	struct drm_connector_hdmi_codec *hdmi_codec;
 };
 
 static inline struct drm_bridge *

-- 
2.39.2

[PATCH RFC 5/5] drm/bridge: lt9611: switch to using the DRM HDMI codec framework

[Dmitry Baryshkov]

Make the Lontium LT9611 DSI-to-HDMI bridge driver use the DRM HDMI Codec
framework. This enables programming of Audio InfoFrames using the HDMI
Connector interface and also enables support for the missing features,
including the ELD retrieval and better hotplug support.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/lontium-lt9611.c | 207 ++++++++++++++++----------------
 1 file changed, 104 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 4fa0dfc5539a..02953468cb76 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -45,7 +45,6 @@ struct lt9611 {
 	struct device_node *dsi1_node;
 	struct mipi_dsi_device *dsi0;
 	struct mipi_dsi_device *dsi1;
-	struct platform_device *audio_pdev;
 
 	bool ac_mode;
 
@@ -688,15 +687,22 @@ lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	/* Enable HDMI output */
 	regmap_write(lt9611->regmap, 0x8130, 0xea);
+
+	drm_connector_hdmi_codec_plugged_notify(connector, true);
 }
 
 static void
 lt9611_bridge_atomic_disable(struct drm_bridge *bridge,
 			     struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	struct drm_connector *connector;
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
 	int ret;
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	drm_connector_hdmi_codec_plugged_notify(connector, false);
+
 	/* Disable HDMI output */
 	ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
 	if (ret) {
@@ -866,6 +872,10 @@ static int lt9611_hdmi_clear_infoframe(struct drm_bridge *bridge,
 	unsigned int mask;
 
 	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		mask = LT9611_INFOFRAME_AUDIO;
+		break;
+
 	case HDMI_INFOFRAME_TYPE_AVI:
 		mask = LT9611_INFOFRAME_AVI;
 		break;
@@ -899,6 +909,11 @@ static int lt9611_hdmi_write_infoframe(struct drm_bridge *bridge,
 	int i;
 
 	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		mask = LT9611_INFOFRAME_AUDIO;
+		addr = 0x84b2;
+		break;
+
 	case HDMI_INFOFRAME_TYPE_AVI:
 		mask = LT9611_INFOFRAME_AVI;
 		addr = 0x8440;
@@ -942,6 +957,79 @@ lt9611_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static int lt9611_hdmi_codec_prepare(struct device *dev,
+				     struct drm_connector *connector,
+				     struct drm_bridge *bridge,
+				     struct hdmi_codec_daifmt *fmt,
+				     struct hdmi_codec_params *hparms)
+{
+	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+	if (hparms->sample_rate == 48000)
+		regmap_write(lt9611->regmap, 0x840f, 0x2b);
+	else if (hparms->sample_rate == 96000)
+		regmap_write(lt9611->regmap, 0x840f, 0xab);
+	else
+		return -EINVAL;
+
+	regmap_write(lt9611->regmap, 0x8435, 0x00);
+	regmap_write(lt9611->regmap, 0x8436, 0x18);
+	regmap_write(lt9611->regmap, 0x8437, 0x00);
+
+	return drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector,
+								       &hparms->cea);
+}
+
+static int lt9611_hdmi_codec_audio_startup(struct device *dev,
+					   struct drm_connector *connector,
+					   struct drm_bridge *bridge)
+{
+	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+	regmap_write(lt9611->regmap, 0x82d6, 0x8c);
+	regmap_write(lt9611->regmap, 0x82d7, 0x04);
+
+	regmap_write(lt9611->regmap, 0x8406, 0x08);
+	regmap_write(lt9611->regmap, 0x8407, 0x10);
+
+	regmap_write(lt9611->regmap, 0x8434, 0xd5);
+
+	return 0;
+}
+
+static void lt9611_hdmi_codec_audio_shutdown(struct device *dev,
+					     struct drm_connector *connector,
+					     struct drm_bridge *bridge)
+{
+	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+	drm_atomic_helper_connector_hdmi_disable_audio_infoframe(connector);
+
+	regmap_write(lt9611->regmap, 0x8406, 0x00);
+	regmap_write(lt9611->regmap, 0x8407, 0x00);
+}
+
+static int lt9611_hdmi_codec_get_dai_id(struct drm_connector *connector,
+					struct drm_bridge *bridge,
+					struct device_node *endpoint)
+{
+	struct of_endpoint of_ep;
+	int ret;
+
+	ret = of_graph_parse_endpoint(endpoint, &of_ep);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * HDMI sound should be located as reg = <2>
+	 * Then, it is sound port 0
+	 */
+	if (of_ep.port == 2)
+		return 0;
+
+	return -EINVAL;
+}
+
 static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.attach = lt9611_bridge_attach,
 	.mode_valid = lt9611_bridge_mode_valid,
@@ -962,6 +1050,11 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.hdmi_tmds_char_rate_valid = lt9611_hdmi_tmds_char_rate_valid,
 	.hdmi_write_infoframe = lt9611_hdmi_write_infoframe,
 	.hdmi_clear_infoframe = lt9611_hdmi_clear_infoframe,
+
+	.hdmi_codec_audio_startup = lt9611_hdmi_codec_audio_startup,
+	.hdmi_codec_prepare = lt9611_hdmi_codec_prepare,
+	.hdmi_codec_audio_shutdown = lt9611_hdmi_codec_audio_shutdown,
+	.hdmi_codec_get_dai_id = lt9611_hdmi_codec_get_dai_id,
 };
 
 static int lt9611_parse_dt(struct device *dev,
@@ -1015,102 +1108,6 @@ static int lt9611_read_device_rev(struct lt9611 *lt9611)
 	return ret;
 }
 
-static int lt9611_hdmi_hw_params(struct device *dev, void *data,
-				 struct hdmi_codec_daifmt *fmt,
-				 struct hdmi_codec_params *hparms)
-{
-	struct lt9611 *lt9611 = data;
-
-	if (hparms->sample_rate == 48000)
-		regmap_write(lt9611->regmap, 0x840f, 0x2b);
-	else if (hparms->sample_rate == 96000)
-		regmap_write(lt9611->regmap, 0x840f, 0xab);
-	else
-		return -EINVAL;
-
-	regmap_write(lt9611->regmap, 0x8435, 0x00);
-	regmap_write(lt9611->regmap, 0x8436, 0x18);
-	regmap_write(lt9611->regmap, 0x8437, 0x00);
-
-	return 0;
-}
-
-static int lt9611_audio_startup(struct device *dev, void *data)
-{
-	struct lt9611 *lt9611 = data;
-
-	regmap_write(lt9611->regmap, 0x82d6, 0x8c);
-	regmap_write(lt9611->regmap, 0x82d7, 0x04);
-
-	regmap_write(lt9611->regmap, 0x8406, 0x08);
-	regmap_write(lt9611->regmap, 0x8407, 0x10);
-
-	regmap_write(lt9611->regmap, 0x8434, 0xd5);
-
-	return 0;
-}
-
-static void lt9611_audio_shutdown(struct device *dev, void *data)
-{
-	struct lt9611 *lt9611 = data;
-
-	regmap_write(lt9611->regmap, 0x8406, 0x00);
-	regmap_write(lt9611->regmap, 0x8407, 0x00);
-}
-
-static int lt9611_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
-				      struct device_node *endpoint,
-				      void *data)
-{
-	struct of_endpoint of_ep;
-	int ret;
-
-	ret = of_graph_parse_endpoint(endpoint, &of_ep);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * HDMI sound should be located as reg = <2>
-	 * Then, it is sound port 0
-	 */
-	if (of_ep.port == 2)
-		return 0;
-
-	return -EINVAL;
-}
-
-static const struct hdmi_codec_ops lt9611_codec_ops = {
-	.hw_params	= lt9611_hdmi_hw_params,
-	.audio_shutdown = lt9611_audio_shutdown,
-	.audio_startup	= lt9611_audio_startup,
-	.get_dai_id	= lt9611_hdmi_i2s_get_dai_id,
-};
-
-static struct hdmi_codec_pdata codec_data = {
-	.ops = &lt9611_codec_ops,
-	.max_i2s_channels = 8,
-	.i2s = 1,
-};
-
-static int lt9611_audio_init(struct device *dev, struct lt9611 *lt9611)
-{
-	codec_data.data = lt9611;
-	lt9611->audio_pdev =
-		platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
-					      PLATFORM_DEVID_AUTO,
-					      &codec_data, sizeof(codec_data));
-
-	return PTR_ERR_OR_ZERO(lt9611->audio_pdev);
-}
-
-static void lt9611_audio_exit(struct lt9611 *lt9611)
-{
-	if (lt9611->audio_pdev) {
-		platform_device_unregister(lt9611->audio_pdev);
-		lt9611->audio_pdev = NULL;
-	}
-}
-
 static int lt9611_probe(struct i2c_client *client)
 {
 	struct lt9611 *lt9611;
@@ -1174,14 +1171,20 @@ static int lt9611_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, lt9611);
 
+	/* Disable Audio InfoFrame, enabled by default */
+	regmap_update_bits(lt9611->regmap, 0x843d, LT9611_INFOFRAME_AUDIO, 0);
+
 	lt9611->bridge.funcs = &lt9611_bridge_funcs;
 	lt9611->bridge.of_node = client->dev.of_node;
 	lt9611->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
 			     DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES |
-			     DRM_BRIDGE_OP_HDMI;
+			     DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_HDMI_CODEC;
 	lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
 	lt9611->bridge.vendor = "Lontium";
 	lt9611->bridge.product = "LT9611";
+	lt9611->bridge.parent = dev;
+	lt9611->bridge.max_i2s_channels = 8;
+	lt9611->bridge.hdmi_codec_i2s = 1;
 
 	drm_bridge_add(&lt9611->bridge);
 
@@ -1203,10 +1206,6 @@ static int lt9611_probe(struct i2c_client *client)
 
 	lt9611_enable_hpd_interrupts(lt9611);
 
-	ret = lt9611_audio_init(dev, lt9611);
-	if (ret)
-		goto err_remove_bridge;
-
 	return 0;
 
 err_remove_bridge:
@@ -1227,7 +1226,9 @@ static void lt9611_remove(struct i2c_client *client)
 	struct lt9611 *lt9611 = i2c_get_clientdata(client);
 
 	disable_irq(client->irq);
-	lt9611_audio_exit(lt9611);
+	if (lt9611->bridge.dev)
+		drmm_connector_hdmi_codec_free(lt9611->bridge.dev,
+					       lt9611->bridge.hdmi_codec);
 	drm_bridge_remove(&lt9611->bridge);
 
 	regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);

-- 
2.39.2

Re: [PATCH RFC 2/5] ASoC: hdmi-codec: pass data to get_dai_id too

[Mark Brown]

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

On Sat, Jun 15, 2024 at 08:53:31PM +0300, Dmitry Baryshkov wrote:
> The upcoming DRM connector HDMI codec implementation is going to use
> codec-specific data in the .get_dai_id to get drm_connector. Pass data
> to the callback, as it is done with other hdmi_codec_ops callbacks.

Acked-by: Mark Brown <broonie@kernel.org>

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

Re: [PATCH RFC 5/5] drm/bridge: lt9611: switch to using the DRM HDMI codec framework

[Dmitry Baryshkov]

On Sat, Jun 15, 2024 at 08:53:34PM GMT, Dmitry Baryshkov wrote:
> Make the Lontium LT9611 DSI-to-HDMI bridge driver use the DRM HDMI Codec
> framework. This enables programming of Audio InfoFrames using the HDMI
> Connector interface and also enables support for the missing features,
> including the ELD retrieval and better hotplug support.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/bridge/lontium-lt9611.c | 207 ++++++++++++++++----------------
>  1 file changed, 104 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index 4fa0dfc5539a..02953468cb76 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -45,7 +45,6 @@ struct lt9611 {
>  	struct device_node *dsi1_node;
>  	struct mipi_dsi_device *dsi0;
>  	struct mipi_dsi_device *dsi1;
> -	struct platform_device *audio_pdev;
>  
>  	bool ac_mode;
>  
> @@ -688,15 +687,22 @@ lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  	/* Enable HDMI output */
>  	regmap_write(lt9611->regmap, 0x8130, 0xea);
> +
> +	drm_connector_hdmi_codec_plugged_notify(connector, true);
>  }
>  
>  static void
>  lt9611_bridge_atomic_disable(struct drm_bridge *bridge,
>  			     struct drm_bridge_state *old_bridge_state)
>  {
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	struct drm_connector *connector;
>  	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
>  	int ret;
>  
> +	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);

Of course this should have been
drm_atomic_get_old_connector_for_encoder(), otherwise it crashes because
connector is NULL.

> +	drm_connector_hdmi_codec_plugged_notify(connector, false);
> +
>  	/* Disable HDMI output */
>  	ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
>  	if (ret) {

-- 
With best wishes
Dmitry

Re: [PATCH RFC 1/5] drm/bridge: lt9611: use HDMI Connector helper to set InfoFrames

[Maxime Ripard]

On Sat, 15 Jun 2024 20:53:30 +0300, Dmitry Baryshkov wrote:
> Use new HDMI Connector helpers in the Lontium LT9611 bridge driver.
> Program InfoFrames using the helper's callbacks. Also use TMDS char rate
> validation callback to filter out modes, instead of hardcoding 4k@30.
> 
> The Audio InfoFrame isn't yet handled by these helpers, it requires
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard]

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

Hi,

Sorry for taking some time to review this series.

On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> Several DRM drivers implement HDMI codec support (despite its name it
> applies to both HDMI and DisplayPort drivers). Implement generic
> framework to be used by these drivers. This removes a requirement to
> implement get_eld() callback and provides default implementation for
> codec's plug handling.
> 
> The framework is integrated with the DRM HDMI Connector framework, but
> can be used by DisplayPort drivers.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/Makefile                   |   1 +
>  drivers/gpu/drm/drm_connector.c            |   8 ++
>  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
>  include/drm/drm_connector.h                |  33 ++++++
>  4 files changed, 199 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 68cc9258ffc4..e113a6eade23 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -45,6 +45,7 @@ drm-y := \
>  	drm_client_modeset.o \
>  	drm_color_mgmt.o \
>  	drm_connector.o \
> +	drm_connector_hdmi_codec.o \
>  	drm_crtc.o \
>  	drm_displayid.o \
>  	drm_drv.o \
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3d73a981004c..66d6e9487339 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
>  	mutex_init(&connector->mutex);
>  	mutex_init(&connector->edid_override_mutex);
>  	mutex_init(&connector->hdmi.infoframes.lock);
> +	mutex_init(&connector->hdmi_codec.lock);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  
>  	connector->hdmi.funcs = hdmi_funcs;
>  
> +	if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> +		ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		connector->funcs->atomic_destroy_state(connector,
>  						       connector->state);
>  
> +	mutex_destroy(&connector->hdmi_codec.lock);
>  	mutex_destroy(&connector->hdmi.infoframes.lock);
>  	mutex_destroy(&connector->mutex);
>  
> diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> new file mode 100644
> index 000000000000..a3a7ad117f6f
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright (c) 2024 Linaro Ltd
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_managed.h>
> +
> +#include <sound/hdmi-codec.h>
> +
> +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> +					    uint8_t *buf, size_t len)
> +{
> +	struct drm_connector *connector = data;
> +
> +	//  FIXME: locking against drm_edid_to_eld ?
> +	memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> +
> +	return 0;
> +}
> +
> +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> +						    void *data,
> +						    hdmi_codec_plugged_cb fn,
> +						    struct device *codec_dev)
> +{
> +	struct drm_connector *connector = data;
> +
> +	mutex_lock(&connector->hdmi_codec.lock);
> +
> +	connector->hdmi_codec.plugged_cb = fn;
> +	connector->hdmi_codec.plugged_cb_dev = codec_dev;
> +
> +	fn(codec_dev, connector->hdmi_codec.last_state);
> +
> +	mutex_unlock(&connector->hdmi_codec.lock);
> +
> +	return 0;
> +}
> +
> +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> +					     bool plugged)
> +{
> +	mutex_lock(&connector->hdmi_codec.lock);
> +
> +	connector->hdmi_codec.last_state = plugged;
> +
> +	if (connector->hdmi_codec.plugged_cb &&
> +	    connector->hdmi_codec.plugged_cb_dev)
> +		connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> +						 connector->hdmi_codec.last_state);
> +
> +	mutex_unlock(&connector->hdmi_codec.lock);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);

I think we should do this the other way around, or rather, like we do
for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
things (CEC, HDMI 2.0, audio), so it would be best to have a single
function to call from drivers, that will perform whatever is needed
depending on the driver's capabilities.

So something like drm_connector_hdmi_handle_hotplug, which would then do
the above if there's audio support.

> +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> +						    void *ptr)
> +{
> +	struct platform_device *pdev = ptr;
> +
> +	platform_device_unregister(pdev);
> +}
> +
> +/**
> + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> + * @dev: DRM device
> + * @connector: A pointer to the connector to allocate codec for
> + * @ops: callbacks for this connector
> + *
> + * Create a HDMI codec device to be used with the specified connector.
> + *
> + * Cleanup is automatically handled with in a DRM-managed action.
> + *
> + * The connector structure should be allocated with drmm_kzalloc().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> +				    struct drm_connector *connector,
> +				    const struct hdmi_codec_ops *base_ops)
> +{
> +	struct hdmi_codec_pdata codec_pdata = {};
> +	struct platform_device *pdev;
> +	struct hdmi_codec_ops *ops;
> +	int ret;
> +
> +	ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
> +		return -ENOMEM;

Do we actually need to allocate a new structure here?

> +	*ops = *base_ops;
> +
> +	ops->get_eld = drm_connector_hdmi_codec_get_eld;
> +	ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> +
> +	codec_pdata.ops = ops;
> +	codec_pdata.i2s = connector->hdmi_codec.i2s,
> +	codec_pdata.spdif = connector->hdmi_codec.spdif,
> +	codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> +	codec_pdata.data = connector;
> +
> +	pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> +					     HDMI_CODEC_DRV_NAME,
> +					     PLATFORM_DEVID_AUTO,
> +					     &codec_pdata, sizeof(codec_pdata));

I think parent_dev should be setup by drm_connector_hdmi_init. I guess
what I'm trying to say is that the reason HDMI support has been so
heterogenous is precisely because of the proliferation of functions they
needed to call, and so most drivers were doing the bare minimum until it
worked (or they encountered a bug).

What I was trying to do with the HDMI connector stuff was to make the
easiest approach the one that works according to the spec, for
everything.

Audio is optional, so it should be a togglable thing (either by an
additional function or parameter), but the drivers shouldn't have to set
everything more than what the function requires.

Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
structure as the device data and overwrite whatever we put there.

We worked around it in vc4 by making sure that snd_soc_card was right at
the start of the driver structure and thus both pointers would be equal,
but we have to deal with it here too.

> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> +	if (ret)
> +		return ret;
> +
> +	connector->hdmi_codec.codec_pdev = pdev;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> +
> +/**
> + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> + * @dev: DRM device
> + * @hdmi_codec: A pointer to the HDMI codec data
> + *
> + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> + * While this function should not be necessary for a typical driver, DRM bridge
> + * drivers have to call it from the remove callback if the bridge uses
> + * Connector's HDMI Codec interface.
> + */
> +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> +				    struct drm_connector_hdmi_codec *hdmi_codec)
> +{
> +	drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> +			    hdmi_codec->codec_pdev);
> +}

What would it be useful for?

> +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index f750765d8fbc..0eb8d8ed9495 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -46,6 +46,7 @@ struct drm_property_blob;
>  struct drm_printer;
>  struct drm_privacy_screen;
>  struct edid;
> +struct hdmi_codec_ops;
>  struct i2c_adapter;
>  
>  enum drm_connector_force {
> @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
>  	int (*write_infoframe)(struct drm_connector *connector,
>  			       enum hdmi_infoframe_type type,
>  			       const u8 *buffer, size_t len);
> +
> +	const struct hdmi_codec_ops *codec_ops;

I think I'd rather have the HDMI connector framework provide the ASoC
hooks, and make the needed pointer casts / lookups to provide a
consistent API to drivers using it.

This will probably also solve the issue mentioned above.

>  };
>  
>  /**
> @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
>  	} infoframes;
>  };
>  
> +struct drm_connector_hdmi_codec {
> +	struct device *parent_dev;
> +	struct platform_device *codec_pdev;
> +
> +	const struct drm_connector_hdmi_codec_funcs *funcs;
> +
> +	struct mutex lock; /* protects last_state and plugged_cb */
> +	void (*plugged_cb)(struct device *dev, bool plugged);
> +	struct device *plugged_cb_dev;
> +	bool last_state;
> +
> +	int max_i2s_channels;
> +	uint i2s: 1;
> +	uint spdif: 1;
> +};

It would be great to have some documentation on what those are,
last_state and the mutex especially raise attention :)


>  /**
>   * struct drm_connector - central DRM connector control structure
>   *
> @@ -2119,6 +2138,12 @@ struct drm_connector {
>  	 * @hdmi: HDMI-related variable and properties.
>  	 */
>  	struct drm_connector_hdmi hdmi;
> +
> +	/**
> +	 * @hdmi_codec: HDMI codec properties and variables. Also might be used
> +	 * for DisplayPort audio.
> +	 */
> +	struct drm_connector_hdmi_codec hdmi_codec;

I'd rather make this part of drm_connector_hdmi, it cannot work without it.

Maxime

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

Re: [PATCH RFC 4/5] drm/bridge: connector: add support for HDMI codec framework

[Maxime Ripard]

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

On Sat, Jun 15, 2024 at 08:53:33PM GMT, Dmitry Baryshkov wrote:
> Add necessary glue code to be able to use new HDMI codec framework from
> the DRM bridge drivers. The drm_bridge implements a limited set of the
> hdmi_codec_ops interface, with the functions accepting both
> drm_connector and drm_bridge instead of just a generic void pointer.
> 
> This framework is integrated with the DRM HDMI Connector framework, but
> can also be used for DisplayPort connectors.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_bridge_connector.c | 130 ++++++++++++++++++++++++++++++++-
>  include/drm/drm_bridge.h               |  46 ++++++++++++
>  2 files changed, 174 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 0869b663f17e..71d6fdc2391f 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -20,6 +20,8 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/display/drm_hdmi_state_helper.h>
>  
> +#include <sound/hdmi-codec.h>
> +
>  /**
>   * DOC: overview
>   *
> @@ -95,6 +97,14 @@ struct drm_bridge_connector {
>  	 * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
>  	 */
>  	struct drm_bridge *bridge_hdmi;
> +	/**
> +	 * @bridge_hdmi_codec:
> +	 *
> +	 * The bridge in the chain that implements necessary support for the
> +	 * HDMI Audio Codec infrastructure, if any (see
> +	 * &DRM_BRIDGE_OP_HDMI_CODEC).
> +	 */
> +	struct drm_bridge *bridge_hdmi_codec;

Can we have a setup where one bridge would support the video stream and
another one the audio?

I think for now I'd rather make them both provided by the same bridge,
and we can always change that later on if we need to.

>  };
>  
>  #define to_drm_bridge_connector(x) \
> @@ -343,10 +353,104 @@ static int drm_bridge_connector_write_infoframe(struct drm_connector *connector,
>  	return bridge->funcs->hdmi_write_infoframe(bridge, type, buffer, len);
>  }
>  
> +static int drm_bridge_connector_audio_startup(struct device *dev, void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_audio_startup)
> +		return bridge->funcs->hdmi_codec_audio_startup(dev, connector, bridge);
> +	else
> +		return 0;
> +}
> +
> +static int drm_bridge_connector_prepare(struct device *dev, void *data,
> +					struct hdmi_codec_daifmt *fmt,
> +					struct hdmi_codec_params *hparms)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	return bridge->funcs->hdmi_codec_prepare(dev, connector, bridge, fmt, hparms);
> +}
> +
> +static void drm_bridge_connector_audio_shutdown(struct device *dev, void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return;
> +
> +	bridge->funcs->hdmi_codec_audio_shutdown(dev, connector, bridge);
> +}
> +
> +static int drm_bridge_connector_mute_stream(struct device *dev, void *data,
> +					    bool enable, int direction)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_mute_stream)
> +		return bridge->funcs->hdmi_codec_mute_stream(dev, connector, bridge,
> +							     enable, direction);
> +	else
> +		return -ENOTSUPP;
> +}
> +
> +static int drm_bridge_connector_get_dai_id(struct snd_soc_component *comment,
> +					   struct device_node *endpoint,
> +					   void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_get_dai_id)
> +		return bridge->funcs->hdmi_codec_get_dai_id(connector, bridge, endpoint);
> +	else
> +		return -ENOTSUPP;
> +}
> +
> +static const struct hdmi_codec_ops drm_bridge_connector_hdmi_codec_ops = {
> +	.audio_startup = drm_bridge_connector_audio_startup,
> +	.prepare = drm_bridge_connector_prepare,
> +	.audio_shutdown = drm_bridge_connector_audio_shutdown,
> +	.mute_stream = drm_bridge_connector_mute_stream,
> +	.get_dai_id = drm_bridge_connector_get_dai_id,
> +};
> +
>  static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
>  	.tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
>  	.clear_infoframe = drm_bridge_connector_clear_infoframe,
>  	.write_infoframe = drm_bridge_connector_write_infoframe,
> +	.codec_ops = &drm_bridge_connector_hdmi_codec_ops,
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -427,6 +531,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  				max_bpc = bridge->max_bpc;
>  		}
>  
> +		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CODEC) {
> +			if (bridge_connector->bridge_hdmi_codec)
> +				return ERR_PTR(-EBUSY);
> +			if (!bridge->funcs->hdmi_codec_prepare ||
> +			    !bridge->funcs->hdmi_codec_audio_shutdown)
> +				return ERR_PTR(-EINVAL);
> +
> +			bridge_connector->bridge_hdmi_codec = bridge;
> +
> +			connector->hdmi_codec.parent_dev = bridge->parent;
> +			connector->hdmi_codec.i2s = bridge->hdmi_codec_i2s;
> +			connector->hdmi_codec.spdif = bridge->hdmi_codec_spdif;
> +			connector->hdmi_codec.max_i2s_channels = bridge->max_i2s_channels;
> +
> +			bridge->hdmi_codec = &connector->hdmi_codec;
> +		}
> +
>  		if (!drm_bridge_get_next_bridge(bridge))
>  			connector_type = bridge->type;
>  
> @@ -448,7 +569,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if (bridge_connector->bridge_hdmi)
> +	if (bridge_connector->bridge_hdmi) {
>  		ret = drmm_connector_hdmi_init(drm, connector,
>  					       bridge_connector->bridge_hdmi->vendor,
>  					       bridge_connector->bridge_hdmi->product,
> @@ -457,10 +578,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  					       connector_type, ddc,
>  					       supported_formats,
>  					       max_bpc);
> -	else
> +	} else {
>  		ret = drmm_connector_init(drm, connector,
>  					  &drm_bridge_connector_funcs,
>  					  connector_type, ddc);
> +		if (!ret && bridge_connector->bridge_hdmi_codec) {
> +			ret = drmm_connector_hdmi_codec_alloc(drm, connector,
> +							      &drm_bridge_connector_hdmi_codec_ops);
> +		}
> +	}
>  	if (ret) {
>  		kfree(bridge_connector);
>  		return ERR_PTR(ret);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 75019d16be64..c4a95c489b00 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -41,6 +41,8 @@ struct drm_display_info;
>  struct drm_minor;
>  struct drm_panel;
>  struct edid;
> +struct hdmi_codec_daifmt;
> +struct hdmi_codec_params;
>  struct i2c_adapter;
>  
>  /**
> @@ -676,6 +678,29 @@ struct drm_bridge_funcs {
>  				    enum hdmi_infoframe_type type,
>  				    const u8 *buffer, size_t len);
>  
> +	int (*hdmi_codec_audio_startup)(struct device *dev,
> +					struct drm_connector *connector,
> +					struct drm_bridge *bridge);
> +
> +	int (*hdmi_codec_prepare)(struct device *dev,
> +				  struct drm_connector *connector,
> +				  struct drm_bridge *bridge,
> +				  struct hdmi_codec_daifmt *fmt,
> +				  struct hdmi_codec_params *hparms);
> +
> +	void (*hdmi_codec_audio_shutdown)(struct device *dev,
> +					  struct drm_connector *connector,
> +					  struct drm_bridge *bridge);
> +
> +	int (*hdmi_codec_mute_stream)(struct device *dev,
> +				      struct drm_connector *connector,
> +				      struct drm_bridge *bridge,
> +				      bool enable, int direction);
> +
> +	int (*hdmi_codec_get_dai_id)(struct drm_connector *connector,
> +				     struct drm_bridge *bridge,
> +				     struct device_node *endpoint);
> +
>  	/**
>  	 * @debugfs_init:
>  	 *
> @@ -761,6 +786,20 @@ enum drm_bridge_ops {
>  	 * drivers.
>  	 */
>  	DRM_BRIDGE_OP_HDMI = BIT(4),
> +	/**
> +	 * @DRM_BRIDGE_OP_HDMI_CODEC: The bridge provides HDMI Audio Codec
> +	 * operations. Bridges that set this flag must implement the
> +	 * &drm_bridge_funcs->hdmi_codec_prepare and
> +	 * &drm_bridge_funcs->hdmi_codec_audio_shutdown callbacks and set the
> +	 * relevant field in the &drm_bridge structure.
> +	 *
> +	 * This framework can be used by both HDMI and DisplayPort bridges.
> +	 *
> +	 * Note: currently there can be at most one bridge in a chain that sets
> +	 * this bit. This is to simplify corresponding glue code in connector
> +	 * drivers.
> +	 */
> +	DRM_BRIDGE_OP_HDMI_CODEC = BIT(5),

I think I'd go and make it one step further and make HDMI_CODEC imply HDMI.

Maxime

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

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> Sorry for taking some time to review this series.

No problem, that's not long.

>
> On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > Several DRM drivers implement HDMI codec support (despite its name it
> > applies to both HDMI and DisplayPort drivers). Implement generic
> > framework to be used by these drivers. This removes a requirement to
> > implement get_eld() callback and provides default implementation for
> > codec's plug handling.
> >
> > The framework is integrated with the DRM HDMI Connector framework, but
> > can be used by DisplayPort drivers.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/Makefile                   |   1 +
> >  drivers/gpu/drm/drm_connector.c            |   8 ++
> >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h                |  33 ++++++
> >  4 files changed, 199 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 68cc9258ffc4..e113a6eade23 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -45,6 +45,7 @@ drm-y := \
> >       drm_client_modeset.o \
> >       drm_color_mgmt.o \
> >       drm_connector.o \
> > +     drm_connector_hdmi_codec.o \
> >       drm_crtc.o \
> >       drm_displayid.o \
> >       drm_drv.o \
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 3d73a981004c..66d6e9487339 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> >       mutex_init(&connector->mutex);
> >       mutex_init(&connector->edid_override_mutex);
> >       mutex_init(&connector->hdmi.infoframes.lock);
> > +     mutex_init(&connector->hdmi_codec.lock);
> >       connector->edid_blob_ptr = NULL;
> >       connector->epoch_counter = 0;
> >       connector->tile_blob_ptr = NULL;
> > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >
> >       connector->hdmi.funcs = hdmi_funcs;
> >
> > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> >               connector->funcs->atomic_destroy_state(connector,
> >                                                      connector->state);
> >
> > +     mutex_destroy(&connector->hdmi_codec.lock);
> >       mutex_destroy(&connector->hdmi.infoframes.lock);
> >       mutex_destroy(&connector->mutex);
> >
> > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > new file mode 100644
> > index 000000000000..a3a7ad117f6f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > @@ -0,0 +1,157 @@
> > +/*
> > + * Copyright (c) 2024 Linaro Ltd
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission.  The copyright holders make no representations
> > + * about the suitability of this software for any purpose.  It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_managed.h>
> > +
> > +#include <sound/hdmi-codec.h>
> > +
> > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > +                                         uint8_t *buf, size_t len)
> > +{
> > +     struct drm_connector *connector = data;
> > +
> > +     //  FIXME: locking against drm_edid_to_eld ?
> > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > +
> > +     return 0;
> > +}
> > +
> > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > +                                                 void *data,
> > +                                                 hdmi_codec_plugged_cb fn,
> > +                                                 struct device *codec_dev)
> > +{
> > +     struct drm_connector *connector = data;
> > +
> > +     mutex_lock(&connector->hdmi_codec.lock);
> > +
> > +     connector->hdmi_codec.plugged_cb = fn;
> > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > +
> > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > +
> > +     mutex_unlock(&connector->hdmi_codec.lock);
> > +
> > +     return 0;
> > +}
> > +
> > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > +                                          bool plugged)
> > +{
> > +     mutex_lock(&connector->hdmi_codec.lock);
> > +
> > +     connector->hdmi_codec.last_state = plugged;
> > +
> > +     if (connector->hdmi_codec.plugged_cb &&
> > +         connector->hdmi_codec.plugged_cb_dev)
> > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > +                                              connector->hdmi_codec.last_state);
> > +
> > +     mutex_unlock(&connector->hdmi_codec.lock);
> > +}
> > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
>
> I think we should do this the other way around, or rather, like we do
> for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> things (CEC, HDMI 2.0, audio), so it would be best to have a single
> function to call from drivers, that will perform whatever is needed
> depending on the driver's capabilities.

I see, this API is probably misnamed. The hdmi_codec_ops use the
'plugged' term, but most of the drivers notify the ASoC / codec during
atomic_enable / atomic_disable path, because usually the audio path
can not work with the video path being disabled. I'll rename this
function to something like ..hdmi_codec_enable. or ...
hdmi_codec_set_enabled.

>
> So something like drm_connector_hdmi_handle_hotplug, which would then do
> the above if there's audio support.
>
> > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > +                                                 void *ptr)
> > +{
> > +     struct platform_device *pdev = ptr;
> > +
> > +     platform_device_unregister(pdev);
> > +}
> > +
> > +/**
> > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > + * @dev: DRM device
> > + * @connector: A pointer to the connector to allocate codec for
> > + * @ops: callbacks for this connector
> > + *
> > + * Create a HDMI codec device to be used with the specified connector.
> > + *
> > + * Cleanup is automatically handled with in a DRM-managed action.
> > + *
> > + * The connector structure should be allocated with drmm_kzalloc().
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > +                                 struct drm_connector *connector,
> > +                                 const struct hdmi_codec_ops *base_ops)
> > +{
> > +     struct hdmi_codec_pdata codec_pdata = {};
> > +     struct platform_device *pdev;
> > +     struct hdmi_codec_ops *ops;
> > +     int ret;
> > +
> > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > +     if (!ops)
> > +             return -ENOMEM;
>
> Do we actually need to allocate a new structure here?

I didn't want to change the hdmi-codec's logic too much. But maybe
it's really better to have generic ops implementation here that calls
into the driver-specific callbacks.

> > +     *ops = *base_ops;
> > +
> > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > +
> > +     codec_pdata.ops = ops;
> > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > +     codec_pdata.data = connector;
> > +
> > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > +                                          HDMI_CODEC_DRV_NAME,
> > +                                          PLATFORM_DEVID_AUTO,
> > +                                          &codec_pdata, sizeof(codec_pdata));
>
> I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> what I'm trying to say is that the reason HDMI support has been so
> heterogenous is precisely because of the proliferation of functions they
> needed to call, and so most drivers were doing the bare minimum until it
> worked (or they encountered a bug).
>
> What I was trying to do with the HDMI connector stuff was to make the
> easiest approach the one that works according to the spec, for
> everything.
>
> Audio is optional, so it should be a togglable thing (either by an
> additional function or parameter), but the drivers shouldn't have to set
> everything more than what the function requires.

I'll see what I can do. I had more or less the same goals, being hit
by the lack of the plugged_cb and get_eld support in the bridge's
implementation.

> Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> structure as the device data and overwrite whatever we put there.

It registers driver_data for the created device, it doesn't touch parent_dev.

>
> We worked around it in vc4 by making sure that snd_soc_card was right at
> the start of the driver structure and thus both pointers would be equal,
> but we have to deal with it here too.

Hmm, maybe I'm missing something. The snd_soc_card is a different
story. The bridges just provide the hdmi_codec_ops, the card itself is
handled by the other driver.

>
> > +     if (IS_ERR(pdev))
> > +             return PTR_ERR(pdev);
> > +
> > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     connector->hdmi_codec.codec_pdev = pdev;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > +
> > +/**
> > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > + * @dev: DRM device
> > + * @hdmi_codec: A pointer to the HDMI codec data
> > + *
> > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > + * While this function should not be necessary for a typical driver, DRM bridge
> > + * drivers have to call it from the remove callback if the bridge uses
> > + * Connector's HDMI Codec interface.
> > + */
> > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > +{
> > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > +                         hdmi_codec->codec_pdev);
> > +}
>
> What would it be useful for?

See the last patch,
https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/

if the bridge driver gets unbound, we should also unregister the codec
device. The codec infrastructure uses drmm to allocate data and a drmm
action to unregister the codec device. However the bridge drivers are
not bound by the drmm lifecycle. So we have to do that manually.

>
> > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index f750765d8fbc..0eb8d8ed9495 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -46,6 +46,7 @@ struct drm_property_blob;
> >  struct drm_printer;
> >  struct drm_privacy_screen;
> >  struct edid;
> > +struct hdmi_codec_ops;
> >  struct i2c_adapter;
> >
> >  enum drm_connector_force {
> > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> >       int (*write_infoframe)(struct drm_connector *connector,
> >                              enum hdmi_infoframe_type type,
> >                              const u8 *buffer, size_t len);
> > +
> > +     const struct hdmi_codec_ops *codec_ops;
>
> I think I'd rather have the HDMI connector framework provide the ASoC
> hooks, and make the needed pointer casts / lookups to provide a
> consistent API to drivers using it.
>
> This will probably also solve the issue mentioned above.

Ack.

>
> >  };
> >
> >  /**
> > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> >       } infoframes;
> >  };
> >
> > +struct drm_connector_hdmi_codec {
> > +     struct device *parent_dev;
> > +     struct platform_device *codec_pdev;
> > +
> > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > +
> > +     struct mutex lock; /* protects last_state and plugged_cb */
> > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > +     struct device *plugged_cb_dev;
> > +     bool last_state;
> > +
> > +     int max_i2s_channels;
> > +     uint i2s: 1;
> > +     uint spdif: 1;
> > +};
>
> It would be great to have some documentation on what those are,
> last_state and the mutex especially raise attention :)

Yep, as I wrote in the cover letter, underdocumented.

>
>
> >  /**
> >   * struct drm_connector - central DRM connector control structure
> >   *
> > @@ -2119,6 +2138,12 @@ struct drm_connector {
> >        * @hdmi: HDMI-related variable and properties.
> >        */
> >       struct drm_connector_hdmi hdmi;
> > +
> > +     /**
> > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > +      * for DisplayPort audio.
> > +      */
> > +     struct drm_connector_hdmi_codec hdmi_codec;
>
> I'd rather make this part of drm_connector_hdmi, it cannot work without it.

It can. DisplayPort drivers also use hdmi_codec_ops. They should be
able to benefit from this implementation.



-- 
With best wishes
Dmitry

Re: [PATCH RFC 4/5] drm/bridge: connector: add support for HDMI codec framework

[Dmitry Baryshkov]

On Fri, 21 Jun 2024 at 12:30, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Sat, Jun 15, 2024 at 08:53:33PM GMT, Dmitry Baryshkov wrote:
> > Add necessary glue code to be able to use new HDMI codec framework from
> > the DRM bridge drivers. The drm_bridge implements a limited set of the
> > hdmi_codec_ops interface, with the functions accepting both
> > drm_connector and drm_bridge instead of just a generic void pointer.
> >
> > This framework is integrated with the DRM HDMI Connector framework, but
> > can also be used for DisplayPort connectors.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_bridge_connector.c | 130 ++++++++++++++++++++++++++++++++-
> >  include/drm/drm_bridge.h               |  46 ++++++++++++
> >  2 files changed, 174 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > index 0869b663f17e..71d6fdc2391f 100644
> > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > @@ -20,6 +20,8 @@
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/display/drm_hdmi_state_helper.h>
> >
> > +#include <sound/hdmi-codec.h>
> > +
> >  /**
> >   * DOC: overview
> >   *
> > @@ -95,6 +97,14 @@ struct drm_bridge_connector {
> >        * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
> >        */
> >       struct drm_bridge *bridge_hdmi;
> > +     /**
> > +      * @bridge_hdmi_codec:
> > +      *
> > +      * The bridge in the chain that implements necessary support for the
> > +      * HDMI Audio Codec infrastructure, if any (see
> > +      * &DRM_BRIDGE_OP_HDMI_CODEC).
> > +      */
> > +     struct drm_bridge *bridge_hdmi_codec;
>
> Can we have a setup where one bridge would support the video stream and
> another one the audio?
>
> I think for now I'd rather make them both provided by the same bridge,
> and we can always change that later on if we need to.

The same point here (and for your second comment): DisplayPort audio support.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard]

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

On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > Sorry for taking some time to review this series.
> 
> No problem, that's not long.
> 
> >
> > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > Several DRM drivers implement HDMI codec support (despite its name it
> > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > framework to be used by these drivers. This removes a requirement to
> > > implement get_eld() callback and provides default implementation for
> > > codec's plug handling.
> > >
> > > The framework is integrated with the DRM HDMI Connector framework, but
> > > can be used by DisplayPort drivers.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/Makefile                   |   1 +
> > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h                |  33 ++++++
> > >  4 files changed, 199 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 68cc9258ffc4..e113a6eade23 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -45,6 +45,7 @@ drm-y := \
> > >       drm_client_modeset.o \
> > >       drm_color_mgmt.o \
> > >       drm_connector.o \
> > > +     drm_connector_hdmi_codec.o \
> > >       drm_crtc.o \
> > >       drm_displayid.o \
> > >       drm_drv.o \
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 3d73a981004c..66d6e9487339 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > >       mutex_init(&connector->mutex);
> > >       mutex_init(&connector->edid_override_mutex);
> > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > +     mutex_init(&connector->hdmi_codec.lock);
> > >       connector->edid_blob_ptr = NULL;
> > >       connector->epoch_counter = 0;
> > >       connector->tile_blob_ptr = NULL;
> > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > >
> > >       connector->hdmi.funcs = hdmi_funcs;
> > >
> > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > >               connector->funcs->atomic_destroy_state(connector,
> > >                                                      connector->state);
> > >
> > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > >       mutex_destroy(&connector->mutex);
> > >
> > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > new file mode 100644
> > > index 000000000000..a3a7ad117f6f
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > @@ -0,0 +1,157 @@
> > > +/*
> > > + * Copyright (c) 2024 Linaro Ltd
> > > + *
> > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > + * documentation for any purpose is hereby granted without fee, provided that
> > > + * the above copyright notice appear in all copies and that both that copyright
> > > + * notice and this permission notice appear in supporting documentation, and
> > > + * that the name of the copyright holders not be used in advertising or
> > > + * publicity pertaining to distribution of the software without specific,
> > > + * written prior permission.  The copyright holders make no representations
> > > + * about the suitability of this software for any purpose.  It is provided "as
> > > + * is" without express or implied warranty.
> > > + *
> > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > + * OF THIS SOFTWARE.
> > > + */
> > > +
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/drm_managed.h>
> > > +
> > > +#include <sound/hdmi-codec.h>
> > > +
> > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > +                                         uint8_t *buf, size_t len)
> > > +{
> > > +     struct drm_connector *connector = data;
> > > +
> > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > +                                                 void *data,
> > > +                                                 hdmi_codec_plugged_cb fn,
> > > +                                                 struct device *codec_dev)
> > > +{
> > > +     struct drm_connector *connector = data;
> > > +
> > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > +
> > > +     connector->hdmi_codec.plugged_cb = fn;
> > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > +
> > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > +
> > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > +                                          bool plugged)
> > > +{
> > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > +
> > > +     connector->hdmi_codec.last_state = plugged;
> > > +
> > > +     if (connector->hdmi_codec.plugged_cb &&
> > > +         connector->hdmi_codec.plugged_cb_dev)
> > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > +                                              connector->hdmi_codec.last_state);
> > > +
> > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> >
> > I think we should do this the other way around, or rather, like we do
> > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > function to call from drivers, that will perform whatever is needed
> > depending on the driver's capabilities.
> 
> I see, this API is probably misnamed. The hdmi_codec_ops use the
> 'plugged' term,

Is it misnamed?

It's documented as:

  Hook callback function to handle connector plug event. Optional.

> but most of the drivers notify the ASoC / codec during atomic_enable /
> atomic_disable path, because usually the audio path can not work with
> the video path being disabled.

That's not clear to me either:

  - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
    enable/disable

  - anx7625, mtk_hdmi and mtk_dp calls it in detect

  - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
    exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.

So it doesn't look like there's a majority we can align with, and
neither should we: we need to figure out what we *need* to do and when,
and do that.

From the documentation and quickly through the code though, handling it
in detect looks like the right call.

> I'll rename this function to something like ..hdmi_codec_enable. or
> ... hdmi_codec_set_enabled.
> 
> >
> > So something like drm_connector_hdmi_handle_hotplug, which would then do
> > the above if there's audio support.
> >
> > > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > > +                                                 void *ptr)
> > > +{
> > > +     struct platform_device *pdev = ptr;
> > > +
> > > +     platform_device_unregister(pdev);
> > > +}
> > > +
> > > +/**
> > > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > > + * @dev: DRM device
> > > + * @connector: A pointer to the connector to allocate codec for
> > > + * @ops: callbacks for this connector
> > > + *
> > > + * Create a HDMI codec device to be used with the specified connector.
> > > + *
> > > + * Cleanup is automatically handled with in a DRM-managed action.
> > > + *
> > > + * The connector structure should be allocated with drmm_kzalloc().
> > > + *
> > > + * Returns:
> > > + * Zero on success, error code on failure.
> > > + */
> > > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > > +                                 struct drm_connector *connector,
> > > +                                 const struct hdmi_codec_ops *base_ops)
> > > +{
> > > +     struct hdmi_codec_pdata codec_pdata = {};
> > > +     struct platform_device *pdev;
> > > +     struct hdmi_codec_ops *ops;
> > > +     int ret;
> > > +
> > > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > +     if (!ops)
> > > +             return -ENOMEM;
> >
> > Do we actually need to allocate a new structure here?
> 
> I didn't want to change the hdmi-codec's logic too much. But maybe
> it's really better to have generic ops implementation here that calls
> into the driver-specific callbacks.
> 
> > > +     *ops = *base_ops;
> > > +
> > > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > > +
> > > +     codec_pdata.ops = ops;
> > > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > > +     codec_pdata.data = connector;
> > > +
> > > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > > +                                          HDMI_CODEC_DRV_NAME,
> > > +                                          PLATFORM_DEVID_AUTO,
> > > +                                          &codec_pdata, sizeof(codec_pdata));
> >
> > I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> > what I'm trying to say is that the reason HDMI support has been so
> > heterogenous is precisely because of the proliferation of functions they
> > needed to call, and so most drivers were doing the bare minimum until it
> > worked (or they encountered a bug).
> >
> > What I was trying to do with the HDMI connector stuff was to make the
> > easiest approach the one that works according to the spec, for
> > everything.
> >
> > Audio is optional, so it should be a togglable thing (either by an
> > additional function or parameter), but the drivers shouldn't have to set
> > everything more than what the function requires.
> 
> I'll see what I can do. I had more or less the same goals, being hit
> by the lack of the plugged_cb and get_eld support in the bridge's
> implementation.
> 
> > Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> > structure as the device data and overwrite whatever we put there.
> 
> It registers driver_data for the created device, it doesn't touch parent_dev.
> 
> >
> > We worked around it in vc4 by making sure that snd_soc_card was right at
> > the start of the driver structure and thus both pointers would be equal,
> > but we have to deal with it here too.
> 
> Hmm, maybe I'm missing something. The snd_soc_card is a different
> story. The bridges just provide the hdmi_codec_ops, the card itself is
> handled by the other driver.

For bridges, sure. For full blown controllers, it might be handled by
the driver directly if there's no external controllers involved.

> >
> > > +     if (IS_ERR(pdev))
> > > +             return PTR_ERR(pdev);
> > > +
> > > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     connector->hdmi_codec.codec_pdev = pdev;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > > +
> > > +/**
> > > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > > + * @dev: DRM device
> > > + * @hdmi_codec: A pointer to the HDMI codec data
> > > + *
> > > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > > + * While this function should not be necessary for a typical driver, DRM bridge
> > > + * drivers have to call it from the remove callback if the bridge uses
> > > + * Connector's HDMI Codec interface.
> > > + */
> > > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > > +{
> > > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > > +                         hdmi_codec->codec_pdev);
> > > +}
> >
> > What would it be useful for?
> 
> See the last patch,
> https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/
> 
> if the bridge driver gets unbound, we should also unregister the codec
> device. The codec infrastructure uses drmm to allocate data and a drmm
> action to unregister the codec device. However the bridge drivers are
> not bound by the drmm lifecycle. So we have to do that manually.

Bridge lifetimes in general are a mess, but why do we need to involve
drmm if it's manual then?

It's typically something that shouldn't be done by drivers anyway. Most
of them will get it wrong.

> >
> > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index f750765d8fbc..0eb8d8ed9495 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > >  struct drm_printer;
> > >  struct drm_privacy_screen;
> > >  struct edid;
> > > +struct hdmi_codec_ops;
> > >  struct i2c_adapter;
> > >
> > >  enum drm_connector_force {
> > > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> > >       int (*write_infoframe)(struct drm_connector *connector,
> > >                              enum hdmi_infoframe_type type,
> > >                              const u8 *buffer, size_t len);
> > > +
> > > +     const struct hdmi_codec_ops *codec_ops;
> >
> > I think I'd rather have the HDMI connector framework provide the ASoC
> > hooks, and make the needed pointer casts / lookups to provide a
> > consistent API to drivers using it.
> >
> > This will probably also solve the issue mentioned above.
> 
> Ack.
> 
> >
> > >  };
> > >
> > >  /**
> > > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> > >       } infoframes;
> > >  };
> > >
> > > +struct drm_connector_hdmi_codec {
> > > +     struct device *parent_dev;
> > > +     struct platform_device *codec_pdev;
> > > +
> > > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > > +
> > > +     struct mutex lock; /* protects last_state and plugged_cb */
> > > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > > +     struct device *plugged_cb_dev;
> > > +     bool last_state;
> > > +
> > > +     int max_i2s_channels;
> > > +     uint i2s: 1;
> > > +     uint spdif: 1;
> > > +};
> >
> > It would be great to have some documentation on what those are,
> > last_state and the mutex especially raise attention :)
> 
> Yep, as I wrote in the cover letter, underdocumented.
> 
> >
> >
> > >  /**
> > >   * struct drm_connector - central DRM connector control structure
> > >   *
> > > @@ -2119,6 +2138,12 @@ struct drm_connector {
> > >        * @hdmi: HDMI-related variable and properties.
> > >        */
> > >       struct drm_connector_hdmi hdmi;
> > > +
> > > +     /**
> > > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > > +      * for DisplayPort audio.
> > > +      */
> > > +     struct drm_connector_hdmi_codec hdmi_codec;
> >
> > I'd rather make this part of drm_connector_hdmi, it cannot work without it.
> 
> It can. DisplayPort drivers also use hdmi_codec_ops. They should be
> able to benefit from this implementation.

That's totally doable if we create a structure (and functions) that are
embedded in both drm_connector_hdmi and the future drm_connector_dp

Maxime

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

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dave Stevenson]

Hi Maxime and Dmitry

On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > Sorry for taking some time to review this series.
> >
> > No problem, that's not long.
> >
> > >
> > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > framework to be used by these drivers. This removes a requirement to
> > > > implement get_eld() callback and provides default implementation for
> > > > codec's plug handling.
> > > >
> > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > can be used by DisplayPort drivers.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h                |  33 ++++++
> > > >  4 files changed, 199 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 68cc9258ffc4..e113a6eade23 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -45,6 +45,7 @@ drm-y := \
> > > >       drm_client_modeset.o \
> > > >       drm_color_mgmt.o \
> > > >       drm_connector.o \
> > > > +     drm_connector_hdmi_codec.o \
> > > >       drm_crtc.o \
> > > >       drm_displayid.o \
> > > >       drm_drv.o \
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index 3d73a981004c..66d6e9487339 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > > >       mutex_init(&connector->mutex);
> > > >       mutex_init(&connector->edid_override_mutex);
> > > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > > +     mutex_init(&connector->hdmi_codec.lock);
> > > >       connector->edid_blob_ptr = NULL;
> > > >       connector->epoch_counter = 0;
> > > >       connector->tile_blob_ptr = NULL;
> > > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > > >
> > > >       connector->hdmi.funcs = hdmi_funcs;
> > > >
> > > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > >               connector->funcs->atomic_destroy_state(connector,
> > > >                                                      connector->state);
> > > >
> > > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > > >       mutex_destroy(&connector->mutex);
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > new file mode 100644
> > > > index 000000000000..a3a7ad117f6f
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > @@ -0,0 +1,157 @@
> > > > +/*
> > > > + * Copyright (c) 2024 Linaro Ltd
> > > > + *
> > > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > > + * documentation for any purpose is hereby granted without fee, provided that
> > > > + * the above copyright notice appear in all copies and that both that copyright
> > > > + * notice and this permission notice appear in supporting documentation, and
> > > > + * that the name of the copyright holders not be used in advertising or
> > > > + * publicity pertaining to distribution of the software without specific,
> > > > + * written prior permission.  The copyright holders make no representations
> > > > + * about the suitability of this software for any purpose.  It is provided "as
> > > > + * is" without express or implied warranty.
> > > > + *
> > > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > + * OF THIS SOFTWARE.
> > > > + */
> > > > +
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/drm_managed.h>
> > > > +
> > > > +#include <sound/hdmi-codec.h>
> > > > +
> > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > +                                         uint8_t *buf, size_t len)
> > > > +{
> > > > +     struct drm_connector *connector = data;
> > > > +
> > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > +                                                 void *data,
> > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > +                                                 struct device *codec_dev)
> > > > +{
> > > > +     struct drm_connector *connector = data;
> > > > +
> > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > +
> > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > +
> > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > +                                          bool plugged)
> > > > +{
> > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     connector->hdmi_codec.last_state = plugged;
> > > > +
> > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > +                                              connector->hdmi_codec.last_state);
> > > > +
> > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > >
> > > I think we should do this the other way around, or rather, like we do
> > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > function to call from drivers, that will perform whatever is needed
> > > depending on the driver's capabilities.
> >
> > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > 'plugged' term,
>
> Is it misnamed?
>
> It's documented as:
>
>   Hook callback function to handle connector plug event. Optional.
>
> > but most of the drivers notify the ASoC / codec during atomic_enable /
> > atomic_disable path, because usually the audio path can not work with
> > the video path being disabled.
>
> That's not clear to me either:
>
>   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
>     enable/disable
>
>   - anx7625, mtk_hdmi and mtk_dp calls it in detect
>
>   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
>     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.

FWIW I have a patch in the next set that adds the call to vc4. The
downstream version of the patch is at [1].

> So it doesn't look like there's a majority we can align with, and
> neither should we: we need to figure out what we *need* to do and when,
> and do that.
>
> From the documentation and quickly through the code though, handling it
> in detect looks like the right call.

We concluded that hotplug detect appeared to be the right place as well.

  Dave

[1] https://github.com/raspberrypi/linux/commit/051392bfdc6dc54563ed9909cc1164e8d734af43

> > I'll rename this function to something like ..hdmi_codec_enable. or
> > ... hdmi_codec_set_enabled.
> >
> > >
> > > So something like drm_connector_hdmi_handle_hotplug, which would then do
> > > the above if there's audio support.
> > >
> > > > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > > > +                                                 void *ptr)
> > > > +{
> > > > +     struct platform_device *pdev = ptr;
> > > > +
> > > > +     platform_device_unregister(pdev);
> > > > +}
> > > > +
> > > > +/**
> > > > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > > > + * @dev: DRM device
> > > > + * @connector: A pointer to the connector to allocate codec for
> > > > + * @ops: callbacks for this connector
> > > > + *
> > > > + * Create a HDMI codec device to be used with the specified connector.
> > > > + *
> > > > + * Cleanup is automatically handled with in a DRM-managed action.
> > > > + *
> > > > + * The connector structure should be allocated with drmm_kzalloc().
> > > > + *
> > > > + * Returns:
> > > > + * Zero on success, error code on failure.
> > > > + */
> > > > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > > > +                                 struct drm_connector *connector,
> > > > +                                 const struct hdmi_codec_ops *base_ops)
> > > > +{
> > > > +     struct hdmi_codec_pdata codec_pdata = {};
> > > > +     struct platform_device *pdev;
> > > > +     struct hdmi_codec_ops *ops;
> > > > +     int ret;
> > > > +
> > > > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > > +     if (!ops)
> > > > +             return -ENOMEM;
> > >
> > > Do we actually need to allocate a new structure here?
> >
> > I didn't want to change the hdmi-codec's logic too much. But maybe
> > it's really better to have generic ops implementation here that calls
> > into the driver-specific callbacks.
> >
> > > > +     *ops = *base_ops;
> > > > +
> > > > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > > > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > > > +
> > > > +     codec_pdata.ops = ops;
> > > > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > > > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > > > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > > > +     codec_pdata.data = connector;
> > > > +
> > > > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > > > +                                          HDMI_CODEC_DRV_NAME,
> > > > +                                          PLATFORM_DEVID_AUTO,
> > > > +                                          &codec_pdata, sizeof(codec_pdata));
> > >
> > > I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> > > what I'm trying to say is that the reason HDMI support has been so
> > > heterogenous is precisely because of the proliferation of functions they
> > > needed to call, and so most drivers were doing the bare minimum until it
> > > worked (or they encountered a bug).
> > >
> > > What I was trying to do with the HDMI connector stuff was to make the
> > > easiest approach the one that works according to the spec, for
> > > everything.
> > >
> > > Audio is optional, so it should be a togglable thing (either by an
> > > additional function or parameter), but the drivers shouldn't have to set
> > > everything more than what the function requires.
> >
> > I'll see what I can do. I had more or less the same goals, being hit
> > by the lack of the plugged_cb and get_eld support in the bridge's
> > implementation.
> >
> > > Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> > > structure as the device data and overwrite whatever we put there.
> >
> > It registers driver_data for the created device, it doesn't touch parent_dev.
> >
> > >
> > > We worked around it in vc4 by making sure that snd_soc_card was right at
> > > the start of the driver structure and thus both pointers would be equal,
> > > but we have to deal with it here too.
> >
> > Hmm, maybe I'm missing something. The snd_soc_card is a different
> > story. The bridges just provide the hdmi_codec_ops, the card itself is
> > handled by the other driver.
>
> For bridges, sure. For full blown controllers, it might be handled by
> the driver directly if there's no external controllers involved.
>
> > >
> > > > +     if (IS_ERR(pdev))
> > > > +             return PTR_ERR(pdev);
> > > > +
> > > > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     connector->hdmi_codec.codec_pdev = pdev;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > > > +
> > > > +/**
> > > > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > > > + * @dev: DRM device
> > > > + * @hdmi_codec: A pointer to the HDMI codec data
> > > > + *
> > > > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > > > + * While this function should not be necessary for a typical driver, DRM bridge
> > > > + * drivers have to call it from the remove callback if the bridge uses
> > > > + * Connector's HDMI Codec interface.
> > > > + */
> > > > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > > > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > > > +{
> > > > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > > > +                         hdmi_codec->codec_pdev);
> > > > +}
> > >
> > > What would it be useful for?
> >
> > See the last patch,
> > https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/
> >
> > if the bridge driver gets unbound, we should also unregister the codec
> > device. The codec infrastructure uses drmm to allocate data and a drmm
> > action to unregister the codec device. However the bridge drivers are
> > not bound by the drmm lifecycle. So we have to do that manually.
>
> Bridge lifetimes in general are a mess, but why do we need to involve
> drmm if it's manual then?
>
> It's typically something that shouldn't be done by drivers anyway. Most
> of them will get it wrong.
>
> > >
> > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index f750765d8fbc..0eb8d8ed9495 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > > >  struct drm_printer;
> > > >  struct drm_privacy_screen;
> > > >  struct edid;
> > > > +struct hdmi_codec_ops;
> > > >  struct i2c_adapter;
> > > >
> > > >  enum drm_connector_force {
> > > > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> > > >       int (*write_infoframe)(struct drm_connector *connector,
> > > >                              enum hdmi_infoframe_type type,
> > > >                              const u8 *buffer, size_t len);
> > > > +
> > > > +     const struct hdmi_codec_ops *codec_ops;
> > >
> > > I think I'd rather have the HDMI connector framework provide the ASoC
> > > hooks, and make the needed pointer casts / lookups to provide a
> > > consistent API to drivers using it.
> > >
> > > This will probably also solve the issue mentioned above.
> >
> > Ack.
> >
> > >
> > > >  };
> > > >
> > > >  /**
> > > > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> > > >       } infoframes;
> > > >  };
> > > >
> > > > +struct drm_connector_hdmi_codec {
> > > > +     struct device *parent_dev;
> > > > +     struct platform_device *codec_pdev;
> > > > +
> > > > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > > > +
> > > > +     struct mutex lock; /* protects last_state and plugged_cb */
> > > > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > > > +     struct device *plugged_cb_dev;
> > > > +     bool last_state;
> > > > +
> > > > +     int max_i2s_channels;
> > > > +     uint i2s: 1;
> > > > +     uint spdif: 1;
> > > > +};
> > >
> > > It would be great to have some documentation on what those are,
> > > last_state and the mutex especially raise attention :)
> >
> > Yep, as I wrote in the cover letter, underdocumented.
> >
> > >
> > >
> > > >  /**
> > > >   * struct drm_connector - central DRM connector control structure
> > > >   *
> > > > @@ -2119,6 +2138,12 @@ struct drm_connector {
> > > >        * @hdmi: HDMI-related variable and properties.
> > > >        */
> > > >       struct drm_connector_hdmi hdmi;
> > > > +
> > > > +     /**
> > > > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > > > +      * for DisplayPort audio.
> > > > +      */
> > > > +     struct drm_connector_hdmi_codec hdmi_codec;
> > >
> > > I'd rather make this part of drm_connector_hdmi, it cannot work without it.
> >
> > It can. DisplayPort drivers also use hdmi_codec_ops. They should be
> > able to benefit from this implementation.
>
> That's totally doable if we create a structure (and functions) that are
> embedded in both drm_connector_hdmi and the future drm_connector_dp
>
> Maxime

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > Sorry for taking some time to review this series.
> > 
> > No problem, that's not long.
> > 
> > >
> > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > framework to be used by these drivers. This removes a requirement to
> > > > implement get_eld() callback and provides default implementation for
> > > > codec's plug handling.
> > > >
> > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > can be used by DisplayPort drivers.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h                |  33 ++++++
> > > >  4 files changed, 199 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 68cc9258ffc4..e113a6eade23 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -45,6 +45,7 @@ drm-y := \
> > > >       drm_client_modeset.o \
> > > >       drm_color_mgmt.o \
> > > >       drm_connector.o \
> > > > +     drm_connector_hdmi_codec.o \
> > > >       drm_crtc.o \
> > > >       drm_displayid.o \
> > > >       drm_drv.o \
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index 3d73a981004c..66d6e9487339 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > > >       mutex_init(&connector->mutex);
> > > >       mutex_init(&connector->edid_override_mutex);
> > > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > > +     mutex_init(&connector->hdmi_codec.lock);
> > > >       connector->edid_blob_ptr = NULL;
> > > >       connector->epoch_counter = 0;
> > > >       connector->tile_blob_ptr = NULL;
> > > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > > >
> > > >       connector->hdmi.funcs = hdmi_funcs;
> > > >
> > > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > >               connector->funcs->atomic_destroy_state(connector,
> > > >                                                      connector->state);
> > > >
> > > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > > >       mutex_destroy(&connector->mutex);
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > new file mode 100644
> > > > index 000000000000..a3a7ad117f6f
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > @@ -0,0 +1,157 @@
> > > > +/*
> > > > + * Copyright (c) 2024 Linaro Ltd
> > > > + *
> > > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > > + * documentation for any purpose is hereby granted without fee, provided that
> > > > + * the above copyright notice appear in all copies and that both that copyright
> > > > + * notice and this permission notice appear in supporting documentation, and
> > > > + * that the name of the copyright holders not be used in advertising or
> > > > + * publicity pertaining to distribution of the software without specific,
> > > > + * written prior permission.  The copyright holders make no representations
> > > > + * about the suitability of this software for any purpose.  It is provided "as
> > > > + * is" without express or implied warranty.
> > > > + *
> > > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > + * OF THIS SOFTWARE.
> > > > + */
> > > > +
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/drm_managed.h>
> > > > +
> > > > +#include <sound/hdmi-codec.h>
> > > > +
> > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > +                                         uint8_t *buf, size_t len)
> > > > +{
> > > > +     struct drm_connector *connector = data;
> > > > +
> > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > +                                                 void *data,
> > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > +                                                 struct device *codec_dev)
> > > > +{
> > > > +     struct drm_connector *connector = data;
> > > > +
> > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > +
> > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > +
> > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > +                                          bool plugged)
> > > > +{
> > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > +
> > > > +     connector->hdmi_codec.last_state = plugged;
> > > > +
> > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > +                                              connector->hdmi_codec.last_state);
> > > > +
> > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > >
> > > I think we should do this the other way around, or rather, like we do
> > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > function to call from drivers, that will perform whatever is needed
> > > depending on the driver's capabilities.
> > 
> > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > 'plugged' term,
> 
> Is it misnamed?
> 
> It's documented as:
> 
>   Hook callback function to handle connector plug event. Optional.
> 
> > but most of the drivers notify the ASoC / codec during atomic_enable /
> > atomic_disable path, because usually the audio path can not work with
> > the video path being disabled.
> 
> That's not clear to me either:
> 
>   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
>     enable/disable
> 
>   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> 
>   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
>     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> 
> So it doesn't look like there's a majority we can align with, and
> neither should we: we need to figure out what we *need* to do and when,
> and do that.
> 
> From the documentation and quickly through the code though, handling it
> in detect looks like the right call.

It is tempting to have it in the hotplug call. However:

- It is used to send events to the ASoC Jack, marking the output as
  plugged or unplugged. Once the output is plugged, userspace might
  consider using it for the audio output. Please correct me if I'm
  wrong, but I don't think one can output audio to the HDMI plug unless
  there is a video stream.

- Having it in the hotplug notification chain is also troublesome. As
  Dave pointed out in the quoted piece of code, it should come after
  reading the EDID on the connect event. On the disconnect event it
  should probably come before calling the notification chain, to let
  audio code interract correctly with the fully enabled display devices.

Having both points in mind, I think it's better to have those calls in
enable/disable paths. This way the EDID (for ELD) is definitely read
without the need to call drm_get_edid manually. The ASoC can start
playing audio immediately, etc.

> > I'll rename this function to something like ..hdmi_codec_enable. or
> > ... hdmi_codec_set_enabled.
> > 
> > >
> > > So something like drm_connector_hdmi_handle_hotplug, which would then do
> > > the above if there's audio support.
> > >
> > > > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > > > +                                                 void *ptr)
> > > > +{
> > > > +     struct platform_device *pdev = ptr;
> > > > +
> > > > +     platform_device_unregister(pdev);
> > > > +}
> > > > +
> > > > +/**
> > > > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > > > + * @dev: DRM device
> > > > + * @connector: A pointer to the connector to allocate codec for
> > > > + * @ops: callbacks for this connector
> > > > + *
> > > > + * Create a HDMI codec device to be used with the specified connector.
> > > > + *
> > > > + * Cleanup is automatically handled with in a DRM-managed action.
> > > > + *
> > > > + * The connector structure should be allocated with drmm_kzalloc().
> > > > + *
> > > > + * Returns:
> > > > + * Zero on success, error code on failure.
> > > > + */
> > > > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > > > +                                 struct drm_connector *connector,
> > > > +                                 const struct hdmi_codec_ops *base_ops)
> > > > +{
> > > > +     struct hdmi_codec_pdata codec_pdata = {};
> > > > +     struct platform_device *pdev;
> > > > +     struct hdmi_codec_ops *ops;
> > > > +     int ret;
> > > > +
> > > > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > > +     if (!ops)
> > > > +             return -ENOMEM;
> > >
> > > Do we actually need to allocate a new structure here?
> > 
> > I didn't want to change the hdmi-codec's logic too much. But maybe
> > it's really better to have generic ops implementation here that calls
> > into the driver-specific callbacks.
> > 
> > > > +     *ops = *base_ops;
> > > > +
> > > > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > > > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > > > +
> > > > +     codec_pdata.ops = ops;
> > > > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > > > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > > > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > > > +     codec_pdata.data = connector;
> > > > +
> > > > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > > > +                                          HDMI_CODEC_DRV_NAME,
> > > > +                                          PLATFORM_DEVID_AUTO,
> > > > +                                          &codec_pdata, sizeof(codec_pdata));
> > >
> > > I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> > > what I'm trying to say is that the reason HDMI support has been so
> > > heterogenous is precisely because of the proliferation of functions they
> > > needed to call, and so most drivers were doing the bare minimum until it
> > > worked (or they encountered a bug).
> > >
> > > What I was trying to do with the HDMI connector stuff was to make the
> > > easiest approach the one that works according to the spec, for
> > > everything.
> > >
> > > Audio is optional, so it should be a togglable thing (either by an
> > > additional function or parameter), but the drivers shouldn't have to set
> > > everything more than what the function requires.
> > 
> > I'll see what I can do. I had more or less the same goals, being hit
> > by the lack of the plugged_cb and get_eld support in the bridge's
> > implementation.
> > 
> > > Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> > > structure as the device data and overwrite whatever we put there.
> > 
> > It registers driver_data for the created device, it doesn't touch parent_dev.
> > 
> > >
> > > We worked around it in vc4 by making sure that snd_soc_card was right at
> > > the start of the driver structure and thus both pointers would be equal,
> > > but we have to deal with it here too.
> > 
> > Hmm, maybe I'm missing something. The snd_soc_card is a different
> > story. The bridges just provide the hdmi_codec_ops, the card itself is
> > handled by the other driver.
> 
> For bridges, sure. For full blown controllers, it might be handled by
> the driver directly if there's no external controllers involved.

Hmm, I see. Let me check how vc4 handles it. But anyway, snd_soc_card is
out of scope for this patchset. The driver has to manage it anyway. And
for the hdmi_audio_codec there is no conflict.

> > > > +     if (IS_ERR(pdev))
> > > > +             return PTR_ERR(pdev);
> > > > +
> > > > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     connector->hdmi_codec.codec_pdev = pdev;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > > > +
> > > > +/**
> > > > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > > > + * @dev: DRM device
> > > > + * @hdmi_codec: A pointer to the HDMI codec data
> > > > + *
> > > > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > > > + * While this function should not be necessary for a typical driver, DRM bridge
> > > > + * drivers have to call it from the remove callback if the bridge uses
> > > > + * Connector's HDMI Codec interface.
> > > > + */
> > > > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > > > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > > > +{
> > > > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > > > +                         hdmi_codec->codec_pdev);
> > > > +}
> > >
> > > What would it be useful for?
> > 
> > See the last patch,
> > https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/
> > 
> > if the bridge driver gets unbound, we should also unregister the codec
> > device. The codec infrastructure uses drmm to allocate data and a drmm
> > action to unregister the codec device. However the bridge drivers are
> > not bound by the drmm lifecycle. So we have to do that manually.
> 
> Bridge lifetimes in general are a mess, but why do we need to involve
> drmm if it's manual then?
> 
> It's typically something that shouldn't be done by drivers anyway. Most
> of them will get it wrong.

Non-bridge drivers are not such mess and using drmm_ makes it simpler
for them. Also in case of DP MST this will make like slightly easier as
the audio codec will be torn down together with the connector being
gone.

But really, I'm open to any solution that will work. Maybe it's better
to use devm_add_action_or_reset(codec->parent);

> > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index f750765d8fbc..0eb8d8ed9495 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > > >  struct drm_printer;
> > > >  struct drm_privacy_screen;
> > > >  struct edid;
> > > > +struct hdmi_codec_ops;
> > > >  struct i2c_adapter;
> > > >
> > > >  enum drm_connector_force {
> > > > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> > > >       int (*write_infoframe)(struct drm_connector *connector,
> > > >                              enum hdmi_infoframe_type type,
> > > >                              const u8 *buffer, size_t len);
> > > > +
> > > > +     const struct hdmi_codec_ops *codec_ops;
> > >
> > > I think I'd rather have the HDMI connector framework provide the ASoC
> > > hooks, and make the needed pointer casts / lookups to provide a
> > > consistent API to drivers using it.
> > >
> > > This will probably also solve the issue mentioned above.
> > 
> > Ack.
> > 
> > >
> > > >  };
> > > >
> > > >  /**
> > > > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> > > >       } infoframes;
> > > >  };
> > > >
> > > > +struct drm_connector_hdmi_codec {
> > > > +     struct device *parent_dev;
> > > > +     struct platform_device *codec_pdev;
> > > > +
> > > > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > > > +
> > > > +     struct mutex lock; /* protects last_state and plugged_cb */
> > > > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > > > +     struct device *plugged_cb_dev;
> > > > +     bool last_state;
> > > > +
> > > > +     int max_i2s_channels;
> > > > +     uint i2s: 1;
> > > > +     uint spdif: 1;
> > > > +};
> > >
> > > It would be great to have some documentation on what those are,
> > > last_state and the mutex especially raise attention :)
> > 
> > Yep, as I wrote in the cover letter, underdocumented.
> > 
> > >
> > >
> > > >  /**
> > > >   * struct drm_connector - central DRM connector control structure
> > > >   *
> > > > @@ -2119,6 +2138,12 @@ struct drm_connector {
> > > >        * @hdmi: HDMI-related variable and properties.
> > > >        */
> > > >       struct drm_connector_hdmi hdmi;
> > > > +
> > > > +     /**
> > > > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > > > +      * for DisplayPort audio.
> > > > +      */
> > > > +     struct drm_connector_hdmi_codec hdmi_codec;
> > >
> > > I'd rather make this part of drm_connector_hdmi, it cannot work without it.
> > 
> > It can. DisplayPort drivers also use hdmi_codec_ops. They should be
> > able to benefit from this implementation.
> 
> That's totally doable if we create a structure (and functions) that are
> embedded in both drm_connector_hdmi and the future drm_connector_dp

There is no drm_connector_dp (yet), but the drivers can already benefit
from using the generic hdmi_codec. Later on, when drm_connector_dp
appears, we can move the codec into both hdmi and DP structures.

I can probably convert msm/hdmi and msm/dp to use this framework if that
helps to express the idea.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 04:10:05PM GMT, Dave Stevenson wrote:
> Hi Maxime and Dmitry
> 
> On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Sorry for taking some time to review this series.
> > >
> > > No problem, that's not long.
> > >
> > > >
> > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > framework to be used by these drivers. This removes a requirement to
> > > > > implement get_eld() callback and provides default implementation for
> > > > > codec's plug handling.
> > > > >
> > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > can be used by DisplayPort drivers.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > >  4 files changed, 199 insertions(+)
> > > > >

[...]

> > > > > +
> > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > +                                         uint8_t *buf, size_t len)
> > > > > +{
> > > > > +     struct drm_connector *connector = data;
> > > > > +
> > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > +                                                 void *data,
> > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > +                                                 struct device *codec_dev)
> > > > > +{
> > > > > +     struct drm_connector *connector = data;
> > > > > +
> > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > +
> > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > +
> > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > +                                          bool plugged)
> > > > > +{
> > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > +
> > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > +                                              connector->hdmi_codec.last_state);
> > > > > +
> > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > >
> > > > I think we should do this the other way around, or rather, like we do
> > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > function to call from drivers, that will perform whatever is needed
> > > > depending on the driver's capabilities.
> > >
> > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > 'plugged' term,
> >
> > Is it misnamed?
> >
> > It's documented as:
> >
> >   Hook callback function to handle connector plug event. Optional.
> >
> > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > atomic_disable path, because usually the audio path can not work with
> > > the video path being disabled.
> >
> > That's not clear to me either:
> >
> >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> >     enable/disable
> >
> >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> >
> >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> 
> FWIW I have a patch in the next set that adds the call to vc4. The
> downstream version of the patch is at [1].

Nice!

> > So it doesn't look like there's a majority we can align with, and
> > neither should we: we need to figure out what we *need* to do and when,
> > and do that.
> >
> > From the documentation and quickly through the code though, handling it
> > in detect looks like the right call.
> 
> We concluded that hotplug detect appeared to be the right place as well.

Probably you also stumbled upon hotplug vs enable/disable. Could you
please comment, why you made your decision towards hotplug path?

> 
>   Dave
> 
> [1] https://github.com/raspberrypi/linux/commit/051392bfdc6dc54563ed9909cc1164e8d734af43
> 


-- 
With best wishes
Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dave Stevenson]

Hi Dmitry

On Wed, 26 Jun 2024 at 17:11, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Jun 26, 2024 at 04:10:05PM GMT, Dave Stevenson wrote:
> > Hi Maxime and Dmitry
> >
> > On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Sorry for taking some time to review this series.
> > > >
> > > > No problem, that's not long.
> > > >
> > > > >
> > > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > > framework to be used by these drivers. This removes a requirement to
> > > > > > implement get_eld() callback and provides default implementation for
> > > > > > codec's plug handling.
> > > > > >
> > > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > > can be used by DisplayPort drivers.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > > >  4 files changed, 199 insertions(+)
> > > > > >
>
> [...]
>
> > > > > > +
> > > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > > +                                         uint8_t *buf, size_t len)
> > > > > > +{
> > > > > > +     struct drm_connector *connector = data;
> > > > > > +
> > > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > > +                                                 void *data,
> > > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > > +                                                 struct device *codec_dev)
> > > > > > +{
> > > > > > +     struct drm_connector *connector = data;
> > > > > > +
> > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > > +
> > > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > > +
> > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > > +                                          bool plugged)
> > > > > > +{
> > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > > +
> > > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > > +                                              connector->hdmi_codec.last_state);
> > > > > > +
> > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > > >
> > > > > I think we should do this the other way around, or rather, like we do
> > > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > > function to call from drivers, that will perform whatever is needed
> > > > > depending on the driver's capabilities.
> > > >
> > > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > > 'plugged' term,
> > >
> > > Is it misnamed?
> > >
> > > It's documented as:
> > >
> > >   Hook callback function to handle connector plug event. Optional.
> > >
> > > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > > atomic_disable path, because usually the audio path can not work with
> > > > the video path being disabled.
> > >
> > > That's not clear to me either:
> > >
> > >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> > >     enable/disable
> > >
> > >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > >
> > >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> > >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> >
> > FWIW I have a patch in the next set that adds the call to vc4. The
> > downstream version of the patch is at [1].
>
> Nice!
>
> > > So it doesn't look like there's a majority we can align with, and
> > > neither should we: we need to figure out what we *need* to do and when,
> > > and do that.
> > >
> > > From the documentation and quickly through the code though, handling it
> > > in detect looks like the right call.
> >
> > We concluded that hotplug detect appeared to be the right place as well.
>
> Probably you also stumbled upon hotplug vs enable/disable. Could you
> please comment, why you made your decision towards hotplug path?

We hit it in trying to get Pipewire to do the right thing on
hotplugging HDMI cables, and updating the lists of available audio
destinations in desktop plugins.
My memory is a little hazy, but I seem to recall the logic was that
whilst changing audio destination when you unplug the currently
selected HDMI output is reasonable, but doing so because you changed
resolution or the screen saver kicked in was less user friendly.
mtk_hdmi was used as a basis for the patch, although it's all largely
boilerplate anyway.

Yes the audio has to stop on enable/disable as HDMI video dictates all
the timings.
I've just checked with aplay playing audio and kmstest to change video
mode - audio pauses as it is disabled and resumes when the new mode is
selected.
One observation that I can't immediately explain is that if I use
kmstest to disable the HDMI display that is playing the audio, aplay
still completes without any errors logged. Using "time" on aplay is
returning the same duration for the playback whether the HDMI output
is enabled or not. That may be down to the vc4 hardware with the HDMI
FIFO accepting the data at the correct rate whether the video side is
enabled or not, but that is just a guess.

  Dave

> >
> >   Dave
> >
> > [1] https://github.com/raspberrypi/linux/commit/051392bfdc6dc54563ed9909cc1164e8d734af43
> >
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 06:07:50PM GMT, Dave Stevenson wrote:
> Hi Dmitry
> 
> On Wed, 26 Jun 2024 at 17:11, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, Jun 26, 2024 at 04:10:05PM GMT, Dave Stevenson wrote:
> > > Hi Maxime and Dmitry
> > >
> > > On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Sorry for taking some time to review this series.
> > > > >
> > > > > No problem, that's not long.
> > > > >
> > > > > >
> > > > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > > > framework to be used by these drivers. This removes a requirement to
> > > > > > > implement get_eld() callback and provides default implementation for
> > > > > > > codec's plug handling.
> > > > > > >
> > > > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > > > can be used by DisplayPort drivers.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > > > >  4 files changed, 199 insertions(+)
> > > > > > >
> >
> > [...]
> >
> > > > > > > +
> > > > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > > > +                                         uint8_t *buf, size_t len)
> > > > > > > +{
> > > > > > > +     struct drm_connector *connector = data;
> > > > > > > +
> > > > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > > > +                                                 void *data,
> > > > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > > > +                                                 struct device *codec_dev)
> > > > > > > +{
> > > > > > > +     struct drm_connector *connector = data;
> > > > > > > +
> > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > > > +
> > > > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > > > +
> > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > > > +                                          bool plugged)
> > > > > > > +{
> > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > > > +
> > > > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > > > +                                              connector->hdmi_codec.last_state);
> > > > > > > +
> > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > > > >
> > > > > > I think we should do this the other way around, or rather, like we do
> > > > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > > > function to call from drivers, that will perform whatever is needed
> > > > > > depending on the driver's capabilities.
> > > > >
> > > > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > > > 'plugged' term,
> > > >
> > > > Is it misnamed?
> > > >
> > > > It's documented as:
> > > >
> > > >   Hook callback function to handle connector plug event. Optional.
> > > >
> > > > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > > > atomic_disable path, because usually the audio path can not work with
> > > > > the video path being disabled.
> > > >
> > > > That's not clear to me either:
> > > >
> > > >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> > > >     enable/disable
> > > >
> > > >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > > >
> > > >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> > > >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> > >
> > > FWIW I have a patch in the next set that adds the call to vc4. The
> > > downstream version of the patch is at [1].
> >
> > Nice!
> >
> > > > So it doesn't look like there's a majority we can align with, and
> > > > neither should we: we need to figure out what we *need* to do and when,
> > > > and do that.
> > > >
> > > > From the documentation and quickly through the code though, handling it
> > > > in detect looks like the right call.
> > >
> > > We concluded that hotplug detect appeared to be the right place as well.
> >
> > Probably you also stumbled upon hotplug vs enable/disable. Could you
> > please comment, why you made your decision towards hotplug path?
> 
> We hit it in trying to get Pipewire to do the right thing on
> hotplugging HDMI cables, and updating the lists of available audio
> destinations in desktop plugins.
> My memory is a little hazy, but I seem to recall the logic was that
> whilst changing audio destination when you unplug the currently
> selected HDMI output is reasonable, but doing so because you changed
> resolution or the screen saver kicked in was less user friendly.
> mtk_hdmi was used as a basis for the patch, although it's all largely
> boilerplate anyway.

Hmm, I should check how this is handled on the standard desktops. With
the DisplayPort and link training it might take a significant amount of
time to switch the mode.

> Yes the audio has to stop on enable/disable as HDMI video dictates all
> the timings.
> I've just checked with aplay playing audio and kmstest to change video
> mode - audio pauses as it is disabled and resumes when the new mode is
> selected.
> One observation that I can't immediately explain is that if I use
> kmstest to disable the HDMI display that is playing the audio, aplay
> still completes without any errors logged. Using "time" on aplay is
> returning the same duration for the playback whether the HDMI output
> is enabled or not. That may be down to the vc4 hardware with the HDMI
> FIFO accepting the data at the correct rate whether the video side is
> enabled or not, but that is just a guess.

I guess so. With msm/hdmi and with msm/dp we should be getting an error
when the video is turned off. I don't remember if it is an immediate
error or something that happens at the end of the period. Adding Srini,
our audio expert, he should know it better.

For external HDMI bridges I completely have no idea, but I guess we
don't need to worry too much, as they are just taking I2S or SPDIF audio
from the bus.

In the worst case we conclude that the calling point is driver-dependent
and as such it is not suitable to call the plugged callback from the
drm_bridge_connector.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard]

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

On Wed, Jun 26, 2024 at 09:55:21PM GMT, Dmitry Baryshkov wrote:
> On Wed, Jun 26, 2024 at 06:07:50PM GMT, Dave Stevenson wrote:
> > Hi Dmitry
> > 
> > On Wed, 26 Jun 2024 at 17:11, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Wed, Jun 26, 2024 at 04:10:05PM GMT, Dave Stevenson wrote:
> > > > Hi Maxime and Dmitry
> > > >
> > > > On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > > > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > Sorry for taking some time to review this series.
> > > > > >
> > > > > > No problem, that's not long.
> > > > > >
> > > > > > >
> > > > > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > > > > framework to be used by these drivers. This removes a requirement to
> > > > > > > > implement get_eld() callback and provides default implementation for
> > > > > > > > codec's plug handling.
> > > > > > > >
> > > > > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > > > > can be used by DisplayPort drivers.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > > > > >  4 files changed, 199 insertions(+)
> > > > > > > >
> > >
> > > [...]
> > >
> > > > > > > > +
> > > > > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > > > > +                                         uint8_t *buf, size_t len)
> > > > > > > > +{
> > > > > > > > +     struct drm_connector *connector = data;
> > > > > > > > +
> > > > > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > > > > +
> > > > > > > > +     return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > > > > +                                                 void *data,
> > > > > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > > > > +                                                 struct device *codec_dev)
> > > > > > > > +{
> > > > > > > > +     struct drm_connector *connector = data;
> > > > > > > > +
> > > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > > +
> > > > > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > > > > +
> > > > > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > > > > +
> > > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > > +
> > > > > > > > +     return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > > > > +                                          bool plugged)
> > > > > > > > +{
> > > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > > +
> > > > > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > > > > +
> > > > > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > > > > +                                              connector->hdmi_codec.last_state);
> > > > > > > > +
> > > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > > > > >
> > > > > > > I think we should do this the other way around, or rather, like we do
> > > > > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > > > > function to call from drivers, that will perform whatever is needed
> > > > > > > depending on the driver's capabilities.
> > > > > >
> > > > > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > > > > 'plugged' term,
> > > > >
> > > > > Is it misnamed?
> > > > >
> > > > > It's documented as:
> > > > >
> > > > >   Hook callback function to handle connector plug event. Optional.
> > > > >
> > > > > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > > > > atomic_disable path, because usually the audio path can not work with
> > > > > > the video path being disabled.
> > > > >
> > > > > That's not clear to me either:
> > > > >
> > > > >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> > > > >     enable/disable
> > > > >
> > > > >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > > > >
> > > > >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> > > > >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> > > >
> > > > FWIW I have a patch in the next set that adds the call to vc4. The
> > > > downstream version of the patch is at [1].
> > >
> > > Nice!
> > >
> > > > > So it doesn't look like there's a majority we can align with, and
> > > > > neither should we: we need to figure out what we *need* to do and when,
> > > > > and do that.
> > > > >
> > > > > From the documentation and quickly through the code though, handling it
> > > > > in detect looks like the right call.
> > > >
> > > > We concluded that hotplug detect appeared to be the right place as well.
> > >
> > > Probably you also stumbled upon hotplug vs enable/disable. Could you
> > > please comment, why you made your decision towards hotplug path?
> > 
> > We hit it in trying to get Pipewire to do the right thing on
> > hotplugging HDMI cables, and updating the lists of available audio
> > destinations in desktop plugins.
> > My memory is a little hazy, but I seem to recall the logic was that
> > whilst changing audio destination when you unplug the currently
> > selected HDMI output is reasonable, but doing so because you changed
> > resolution or the screen saver kicked in was less user friendly.
> > mtk_hdmi was used as a basis for the patch, although it's all largely
> > boilerplate anyway.
> 
> Hmm, I should check how this is handled on the standard desktops. With
> the DisplayPort and link training it might take a significant amount of
> time to switch the mode.
> 
> > Yes the audio has to stop on enable/disable as HDMI video dictates all
> > the timings.
> > I've just checked with aplay playing audio and kmstest to change video
> > mode - audio pauses as it is disabled and resumes when the new mode is
> > selected.
> > One observation that I can't immediately explain is that if I use
> > kmstest to disable the HDMI display that is playing the audio, aplay
> > still completes without any errors logged. Using "time" on aplay is
> > returning the same duration for the playback whether the HDMI output
> > is enabled or not. That may be down to the vc4 hardware with the HDMI
> > FIFO accepting the data at the correct rate whether the video side is
> > enabled or not, but that is just a guess.
> 
> I guess so. With msm/hdmi and with msm/dp we should be getting an error
> when the video is turned off. I don't remember if it is an immediate
> error or something that happens at the end of the period. Adding Srini,
> our audio expert, he should know it better.
> 
> For external HDMI bridges I completely have no idea, but I guess we
> don't need to worry too much, as they are just taking I2S or SPDIF audio
> from the bus.
> 
> In the worst case we conclude that the calling point is driver-dependent
> and as such it is not suitable to call the plugged callback from the
> drm_bridge_connector.

It would really surprise me here: the spec will require something, ALSA
will require something else, and we'll have to bridge the gap, but
there's nothing really device specific here, it's all software.

Maxime

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

Re: [PATCH RFC 4/5] drm/bridge: connector: add support for HDMI codec framework

[Maxime Ripard]

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

On Fri, Jun 21, 2024 at 02:10:22PM GMT, Dmitry Baryshkov wrote:
> On Fri, 21 Jun 2024 at 12:30, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Sat, Jun 15, 2024 at 08:53:33PM GMT, Dmitry Baryshkov wrote:
> > > Add necessary glue code to be able to use new HDMI codec framework from
> > > the DRM bridge drivers. The drm_bridge implements a limited set of the
> > > hdmi_codec_ops interface, with the functions accepting both
> > > drm_connector and drm_bridge instead of just a generic void pointer.
> > >
> > > This framework is integrated with the DRM HDMI Connector framework, but
> > > can also be used for DisplayPort connectors.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/drm_bridge_connector.c | 130 ++++++++++++++++++++++++++++++++-
> > >  include/drm/drm_bridge.h               |  46 ++++++++++++
> > >  2 files changed, 174 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > index 0869b663f17e..71d6fdc2391f 100644
> > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > @@ -20,6 +20,8 @@
> > >  #include <drm/drm_probe_helper.h>
> > >  #include <drm/display/drm_hdmi_state_helper.h>
> > >
> > > +#include <sound/hdmi-codec.h>
> > > +
> > >  /**
> > >   * DOC: overview
> > >   *
> > > @@ -95,6 +97,14 @@ struct drm_bridge_connector {
> > >        * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
> > >        */
> > >       struct drm_bridge *bridge_hdmi;
> > > +     /**
> > > +      * @bridge_hdmi_codec:
> > > +      *
> > > +      * The bridge in the chain that implements necessary support for the
> > > +      * HDMI Audio Codec infrastructure, if any (see
> > > +      * &DRM_BRIDGE_OP_HDMI_CODEC).
> > > +      */
> > > +     struct drm_bridge *bridge_hdmi_codec;
> >
> > Can we have a setup where one bridge would support the video stream and
> > another one the audio?
> >
> > I think for now I'd rather make them both provided by the same bridge,
> > and we can always change that later on if we need to.
> 
> The same point here (and for your second comment): DisplayPort audio
> support.

Well, yeah, but then we can do the same thing for DisplayPort and share
some code when needed.

And like I said, we can change that later if we need to, but there's no
point in trying to make something super flexible if we're not quite sure
what the requirements are.

Maxime

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

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard]

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

On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
> On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Sorry for taking some time to review this series.
> > > 
> > > No problem, that's not long.
> > > 
> > > >
> > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > framework to be used by these drivers. This removes a requirement to
> > > > > implement get_eld() callback and provides default implementation for
> > > > > codec's plug handling.
> > > > >
> > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > can be used by DisplayPort drivers.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > >  4 files changed, 199 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > > index 68cc9258ffc4..e113a6eade23 100644
> > > > > --- a/drivers/gpu/drm/Makefile
> > > > > +++ b/drivers/gpu/drm/Makefile
> > > > > @@ -45,6 +45,7 @@ drm-y := \
> > > > >       drm_client_modeset.o \
> > > > >       drm_color_mgmt.o \
> > > > >       drm_connector.o \
> > > > > +     drm_connector_hdmi_codec.o \
> > > > >       drm_crtc.o \
> > > > >       drm_displayid.o \
> > > > >       drm_drv.o \
> > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > > index 3d73a981004c..66d6e9487339 100644
> > > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > > > >       mutex_init(&connector->mutex);
> > > > >       mutex_init(&connector->edid_override_mutex);
> > > > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > > > +     mutex_init(&connector->hdmi_codec.lock);
> > > > >       connector->edid_blob_ptr = NULL;
> > > > >       connector->epoch_counter = 0;
> > > > >       connector->tile_blob_ptr = NULL;
> > > > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > > > >
> > > > >       connector->hdmi.funcs = hdmi_funcs;
> > > > >
> > > > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > > > +             if (ret)
> > > > > +                     return ret;
> > > > > +     }
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > > >               connector->funcs->atomic_destroy_state(connector,
> > > > >                                                      connector->state);
> > > > >
> > > > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > > > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > > > >       mutex_destroy(&connector->mutex);
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > new file mode 100644
> > > > > index 000000000000..a3a7ad117f6f
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > @@ -0,0 +1,157 @@
> > > > > +/*
> > > > > + * Copyright (c) 2024 Linaro Ltd
> > > > > + *
> > > > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > > > + * documentation for any purpose is hereby granted without fee, provided that
> > > > > + * the above copyright notice appear in all copies and that both that copyright
> > > > > + * notice and this permission notice appear in supporting documentation, and
> > > > > + * that the name of the copyright holders not be used in advertising or
> > > > > + * publicity pertaining to distribution of the software without specific,
> > > > > + * written prior permission.  The copyright holders make no representations
> > > > > + * about the suitability of this software for any purpose.  It is provided "as
> > > > > + * is" without express or implied warranty.
> > > > > + *
> > > > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > > + * OF THIS SOFTWARE.
> > > > > + */
> > > > > +
> > > > > +#include <linux/mutex.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#include <drm/drm_connector.h>
> > > > > +#include <drm/drm_managed.h>
> > > > > +
> > > > > +#include <sound/hdmi-codec.h>
> > > > > +
> > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > +                                         uint8_t *buf, size_t len)
> > > > > +{
> > > > > +     struct drm_connector *connector = data;
> > > > > +
> > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > +                                                 void *data,
> > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > +                                                 struct device *codec_dev)
> > > > > +{
> > > > > +     struct drm_connector *connector = data;
> > > > > +
> > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > +
> > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > +
> > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > +                                          bool plugged)
> > > > > +{
> > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > +
> > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > +
> > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > +                                              connector->hdmi_codec.last_state);
> > > > > +
> > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > >
> > > > I think we should do this the other way around, or rather, like we do
> > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > function to call from drivers, that will perform whatever is needed
> > > > depending on the driver's capabilities.
> > > 
> > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > 'plugged' term,
> > 
> > Is it misnamed?
> > 
> > It's documented as:
> > 
> >   Hook callback function to handle connector plug event. Optional.
> > 
> > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > atomic_disable path, because usually the audio path can not work with
> > > the video path being disabled.
> > 
> > That's not clear to me either:
> > 
> >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> >     enable/disable
> > 
> >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > 
> >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> > 
> > So it doesn't look like there's a majority we can align with, and
> > neither should we: we need to figure out what we *need* to do and when,
> > and do that.
> > 
> > From the documentation and quickly through the code though, handling it
> > in detect looks like the right call.
> 
> It is tempting to have it in the hotplug call. However:
> 
> - It is used to send events to the ASoC Jack, marking the output as
>   plugged or unplugged. Once the output is plugged, userspace might
>   consider using it for the audio output. Please correct me if I'm
>   wrong, but I don't think one can output audio to the HDMI plug unless
>   there is a video stream.

That's something to check in the HDMI spec and with the ALSA
maintainers.

> - Having it in the hotplug notification chain is also troublesome. As
>   Dave pointed out in the quoted piece of code, it should come after
>   reading the EDID on the connect event. On the disconnect event it
>   should probably come before calling the notification chain, to let
>   audio code interract correctly with the fully enabled display devices.

EDIDs are fetched when hotplug is detected anyway, and we need it for
other things anyway (like CEC).

> Having both points in mind, I think it's better to have those calls in
> enable/disable paths. This way the EDID (for ELD) is definitely read
> without the need to call drm_get_edid manually. The ASoC can start
> playing audio immediately, etc.

Again, it doesn't really matter what is the most convenient. What
matters is what is the correct thing to do, both from the HDMI spec and
ALSA userspace PoV.

And if we can't make that convenient, then maybe we just shouldn't try.

> > > I'll rename this function to something like ..hdmi_codec_enable. or
> > > ... hdmi_codec_set_enabled.
> > > 
> > > >
> > > > So something like drm_connector_hdmi_handle_hotplug, which would then do
> > > > the above if there's audio support.
> > > >
> > > > > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > > > > +                                                 void *ptr)
> > > > > +{
> > > > > +     struct platform_device *pdev = ptr;
> > > > > +
> > > > > +     platform_device_unregister(pdev);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > > > > + * @dev: DRM device
> > > > > + * @connector: A pointer to the connector to allocate codec for
> > > > > + * @ops: callbacks for this connector
> > > > > + *
> > > > > + * Create a HDMI codec device to be used with the specified connector.
> > > > > + *
> > > > > + * Cleanup is automatically handled with in a DRM-managed action.
> > > > > + *
> > > > > + * The connector structure should be allocated with drmm_kzalloc().
> > > > > + *
> > > > > + * Returns:
> > > > > + * Zero on success, error code on failure.
> > > > > + */
> > > > > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > > > > +                                 struct drm_connector *connector,
> > > > > +                                 const struct hdmi_codec_ops *base_ops)
> > > > > +{
> > > > > +     struct hdmi_codec_pdata codec_pdata = {};
> > > > > +     struct platform_device *pdev;
> > > > > +     struct hdmi_codec_ops *ops;
> > > > > +     int ret;
> > > > > +
> > > > > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > > > +     if (!ops)
> > > > > +             return -ENOMEM;
> > > >
> > > > Do we actually need to allocate a new structure here?
> > > 
> > > I didn't want to change the hdmi-codec's logic too much. But maybe
> > > it's really better to have generic ops implementation here that calls
> > > into the driver-specific callbacks.
> > > 
> > > > > +     *ops = *base_ops;
> > > > > +
> > > > > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > > > > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > > > > +
> > > > > +     codec_pdata.ops = ops;
> > > > > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > > > > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > > > > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > > > > +     codec_pdata.data = connector;
> > > > > +
> > > > > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > > > > +                                          HDMI_CODEC_DRV_NAME,
> > > > > +                                          PLATFORM_DEVID_AUTO,
> > > > > +                                          &codec_pdata, sizeof(codec_pdata));
> > > >
> > > > I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> > > > what I'm trying to say is that the reason HDMI support has been so
> > > > heterogenous is precisely because of the proliferation of functions they
> > > > needed to call, and so most drivers were doing the bare minimum until it
> > > > worked (or they encountered a bug).
> > > >
> > > > What I was trying to do with the HDMI connector stuff was to make the
> > > > easiest approach the one that works according to the spec, for
> > > > everything.
> > > >
> > > > Audio is optional, so it should be a togglable thing (either by an
> > > > additional function or parameter), but the drivers shouldn't have to set
> > > > everything more than what the function requires.
> > > 
> > > I'll see what I can do. I had more or less the same goals, being hit
> > > by the lack of the plugged_cb and get_eld support in the bridge's
> > > implementation.
> > > 
> > > > Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> > > > structure as the device data and overwrite whatever we put there.
> > > 
> > > It registers driver_data for the created device, it doesn't touch parent_dev.
> > > 
> > > >
> > > > We worked around it in vc4 by making sure that snd_soc_card was right at
> > > > the start of the driver structure and thus both pointers would be equal,
> > > > but we have to deal with it here too.
> > > 
> > > Hmm, maybe I'm missing something. The snd_soc_card is a different
> > > story. The bridges just provide the hdmi_codec_ops, the card itself is
> > > handled by the other driver.
> > 
> > For bridges, sure. For full blown controllers, it might be handled by
> > the driver directly if there's no external controllers involved.
> 
> Hmm, I see. Let me check how vc4 handles it. But anyway, snd_soc_card is
> out of scope for this patchset. The driver has to manage it anyway. And
> for the hdmi_audio_codec there is no conflict.

Out of scope, sure, but if we need to rework and retest the whole thing
when we get there, it's not great either. So we should take it into
account still. Not care about or work on it, but leave the door open.

> > > > > +     if (IS_ERR(pdev))
> > > > > +             return PTR_ERR(pdev);
> > > > > +
> > > > > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     connector->hdmi_codec.codec_pdev = pdev;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > > > > +
> > > > > +/**
> > > > > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > > > > + * @dev: DRM device
> > > > > + * @hdmi_codec: A pointer to the HDMI codec data
> > > > > + *
> > > > > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > > > > + * While this function should not be necessary for a typical driver, DRM bridge
> > > > > + * drivers have to call it from the remove callback if the bridge uses
> > > > > + * Connector's HDMI Codec interface.
> > > > > + */
> > > > > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > > > > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > > > > +{
> > > > > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > > > > +                         hdmi_codec->codec_pdev);
> > > > > +}
> > > >
> > > > What would it be useful for?
> > > 
> > > See the last patch,
> > > https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/
> > > 
> > > if the bridge driver gets unbound, we should also unregister the codec
> > > device. The codec infrastructure uses drmm to allocate data and a drmm
> > > action to unregister the codec device. However the bridge drivers are
> > > not bound by the drmm lifecycle. So we have to do that manually.
> > 
> > Bridge lifetimes in general are a mess, but why do we need to involve
> > drmm if it's manual then?
> > 
> > It's typically something that shouldn't be done by drivers anyway. Most
> > of them will get it wrong.
> 
> Non-bridge drivers are not such mess and using drmm_ makes it simpler
> for them.

That's arguable, but it's mostly because the framework allows those
drivers to not be messy :)

It doesn't with bridges.

> Also in case of DP MST this will make like slightly easier as the
> audio codec will be torn down together with the connector being gone.
> 
> But really, I'm open to any solution that will work. Maybe it's better
> to use devm_add_action_or_reset(codec->parent);

My point is that there's no point in registering a drmm action if
drivers are supposed to call it anyway. So we can just use neither drmm
or devm, and it'll work just the same.

But I still think we don't need to allocate the structure in the first
place and just put it into drm_connector.

> > > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index f750765d8fbc..0eb8d8ed9495 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > > > >  struct drm_printer;
> > > > >  struct drm_privacy_screen;
> > > > >  struct edid;
> > > > > +struct hdmi_codec_ops;
> > > > >  struct i2c_adapter;
> > > > >
> > > > >  enum drm_connector_force {
> > > > > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> > > > >       int (*write_infoframe)(struct drm_connector *connector,
> > > > >                              enum hdmi_infoframe_type type,
> > > > >                              const u8 *buffer, size_t len);
> > > > > +
> > > > > +     const struct hdmi_codec_ops *codec_ops;
> > > >
> > > > I think I'd rather have the HDMI connector framework provide the ASoC
> > > > hooks, and make the needed pointer casts / lookups to provide a
> > > > consistent API to drivers using it.
> > > >
> > > > This will probably also solve the issue mentioned above.
> > > 
> > > Ack.
> > > 
> > > >
> > > > >  };
> > > > >
> > > > >  /**
> > > > > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> > > > >       } infoframes;
> > > > >  };
> > > > >
> > > > > +struct drm_connector_hdmi_codec {
> > > > > +     struct device *parent_dev;
> > > > > +     struct platform_device *codec_pdev;
> > > > > +
> > > > > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > > > > +
> > > > > +     struct mutex lock; /* protects last_state and plugged_cb */
> > > > > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > > > > +     struct device *plugged_cb_dev;
> > > > > +     bool last_state;
> > > > > +
> > > > > +     int max_i2s_channels;
> > > > > +     uint i2s: 1;
> > > > > +     uint spdif: 1;
> > > > > +};
> > > >
> > > > It would be great to have some documentation on what those are,
> > > > last_state and the mutex especially raise attention :)
> > > 
> > > Yep, as I wrote in the cover letter, underdocumented.
> > > 
> > > >
> > > >
> > > > >  /**
> > > > >   * struct drm_connector - central DRM connector control structure
> > > > >   *
> > > > > @@ -2119,6 +2138,12 @@ struct drm_connector {
> > > > >        * @hdmi: HDMI-related variable and properties.
> > > > >        */
> > > > >       struct drm_connector_hdmi hdmi;
> > > > > +
> > > > > +     /**
> > > > > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > > > > +      * for DisplayPort audio.
> > > > > +      */
> > > > > +     struct drm_connector_hdmi_codec hdmi_codec;
> > > >
> > > > I'd rather make this part of drm_connector_hdmi, it cannot work without it.
> > > 
> > > It can. DisplayPort drivers also use hdmi_codec_ops. They should be
> > > able to benefit from this implementation.
> > 
> > That's totally doable if we create a structure (and functions) that are
> > embedded in both drm_connector_hdmi and the future drm_connector_dp
> 
> There is no drm_connector_dp (yet), but the drivers can already benefit
> from using the generic hdmi_codec. Later on, when drm_connector_dp
> appears, we can move the codec into both hdmi and DP structures.
> 
> I can probably convert msm/hdmi and msm/dp to use this framework if that
> helps to express the idea.

I think there's something I don't get here: why does DP gets in the way,
and why can't we just do the HDMI support now, and then reuse the same
struct and internal functions with DP later on?

I think if we want DP support, we would need to create a DP framework
like we did for HDMI, and that would be a major undertaking just for
audio support.

Maxime

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

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Thu, Jun 27, 2024 at 11:49:37AM GMT, Maxime Ripard wrote:
> On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
> > On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> > > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Sorry for taking some time to review this series.
> > > > 
> > > > No problem, that's not long.
> > > > 
> > > > >
> > > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > > framework to be used by these drivers. This removes a requirement to
> > > > > > implement get_eld() callback and provides default implementation for
> > > > > > codec's plug handling.
> > > > > >
> > > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > > can be used by DisplayPort drivers.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > > >  4 files changed, 199 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > > > index 68cc9258ffc4..e113a6eade23 100644
> > > > > > --- a/drivers/gpu/drm/Makefile
> > > > > > +++ b/drivers/gpu/drm/Makefile
> > > > > > @@ -45,6 +45,7 @@ drm-y := \
> > > > > >       drm_client_modeset.o \
> > > > > >       drm_color_mgmt.o \
> > > > > >       drm_connector.o \
> > > > > > +     drm_connector_hdmi_codec.o \
> > > > > >       drm_crtc.o \
> > > > > >       drm_displayid.o \
> > > > > >       drm_drv.o \
> > > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > > > index 3d73a981004c..66d6e9487339 100644
> > > > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > > > > >       mutex_init(&connector->mutex);
> > > > > >       mutex_init(&connector->edid_override_mutex);
> > > > > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > > > > +     mutex_init(&connector->hdmi_codec.lock);
> > > > > >       connector->edid_blob_ptr = NULL;
> > > > > >       connector->epoch_counter = 0;
> > > > > >       connector->tile_blob_ptr = NULL;
> > > > > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > > > > >
> > > > > >       connector->hdmi.funcs = hdmi_funcs;
> > > > > >
> > > > > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > > > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > > > > +             if (ret)
> > > > > > +                     return ret;
> > > > > > +     }
> > > > > > +
> > > > > >       return 0;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > > > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > > > >               connector->funcs->atomic_destroy_state(connector,
> > > > > >                                                      connector->state);
> > > > > >
> > > > > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > > > > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > > > > >       mutex_destroy(&connector->mutex);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..a3a7ad117f6f
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > > @@ -0,0 +1,157 @@
> > > > > > +/*
> > > > > > + * Copyright (c) 2024 Linaro Ltd
> > > > > > + *
> > > > > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > > > > + * documentation for any purpose is hereby granted without fee, provided that
> > > > > > + * the above copyright notice appear in all copies and that both that copyright
> > > > > > + * notice and this permission notice appear in supporting documentation, and
> > > > > > + * that the name of the copyright holders not be used in advertising or
> > > > > > + * publicity pertaining to distribution of the software without specific,
> > > > > > + * written prior permission.  The copyright holders make no representations
> > > > > > + * about the suitability of this software for any purpose.  It is provided "as
> > > > > > + * is" without express or implied warranty.
> > > > > > + *
> > > > > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > > > + * OF THIS SOFTWARE.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/mutex.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +
> > > > > > +#include <drm/drm_connector.h>
> > > > > > +#include <drm/drm_managed.h>
> > > > > > +
> > > > > > +#include <sound/hdmi-codec.h>
> > > > > > +
> > > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > > +                                         uint8_t *buf, size_t len)
> > > > > > +{
> > > > > > +     struct drm_connector *connector = data;
> > > > > > +
> > > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > > +                                                 void *data,
> > > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > > +                                                 struct device *codec_dev)
> > > > > > +{
> > > > > > +     struct drm_connector *connector = data;
> > > > > > +
> > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > > +
> > > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > > +
> > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > > +                                          bool plugged)
> > > > > > +{
> > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > +
> > > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > > +
> > > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > > +                                              connector->hdmi_codec.last_state);
> > > > > > +
> > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > > >
> > > > > I think we should do this the other way around, or rather, like we do
> > > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > > function to call from drivers, that will perform whatever is needed
> > > > > depending on the driver's capabilities.
> > > > 
> > > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > > 'plugged' term,
> > > 
> > > Is it misnamed?
> > > 
> > > It's documented as:
> > > 
> > >   Hook callback function to handle connector plug event. Optional.
> > > 
> > > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > > atomic_disable path, because usually the audio path can not work with
> > > > the video path being disabled.
> > > 
> > > That's not clear to me either:
> > > 
> > >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> > >     enable/disable
> > > 
> > >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > > 
> > >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> > >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> > > 
> > > So it doesn't look like there's a majority we can align with, and
> > > neither should we: we need to figure out what we *need* to do and when,
> > > and do that.
> > > 
> > > From the documentation and quickly through the code though, handling it
> > > in detect looks like the right call.
> > 
> > It is tempting to have it in the hotplug call. However:
> > 
> > - It is used to send events to the ASoC Jack, marking the output as
> >   plugged or unplugged. Once the output is plugged, userspace might
> >   consider using it for the audio output. Please correct me if I'm
> >   wrong, but I don't think one can output audio to the HDMI plug unless
> >   there is a video stream.
> 
> That's something to check in the HDMI spec and with the ALSA
> maintainers.

Mark and Liam are on CC list. I've also pinged Mark on the IRC (on
#alsa, if the channel logs are preserved somewhere)

<lumag> I'm trying to implement a somewhat generic implementation that the drivers can hook in. The main discussion is at [link to this discussion]
<lumag> So in theory that affects all ASoC platforms having HDMI or DP audio output
<broonie> In that case I'd be conservative and try to follow the state of the physical connection as closely as possible.

So it is really 'plugged'.

> 
> > - Having it in the hotplug notification chain is also troublesome. As
> >   Dave pointed out in the quoted piece of code, it should come after
> >   reading the EDID on the connect event. On the disconnect event it
> >   should probably come before calling the notification chain, to let
> >   audio code interract correctly with the fully enabled display devices.
> 
> EDIDs are fetched when hotplug is detected anyway, and we need it for
> other things anyway (like CEC).

I see that:

- VC4 reads EDID and sets CEC address directly in hotplug notifier and
  then again in get_modes callback. (why is it necessary in the hotplug
  notifier, if it's done anyway in get_modes?)

- sun4i sets CEC address from get_modes

- ADV7511 does a trick and sets CEC address from edid_read() callback
  (with the FIXME from Jani that basically tells us to move this to
  get_modes)

- omapdrm clears CEC address from hpd_notify, but sets it from the
  edid_read() callback with the same FIXME.

- i915 sets CEC address from .detect_ctx callback

So there is no uniformity too. Handling it from drm_bridge_connector() /
get_modes might help, but that requires clearing one of TODO items.

> 
> > Having both points in mind, I think it's better to have those calls in
> > enable/disable paths. This way the EDID (for ELD) is definitely read
> > without the need to call drm_get_edid manually. The ASoC can start
> > playing audio immediately, etc.
> 
> Again, it doesn't really matter what is the most convenient. What
> matters is what is the correct thing to do, both from the HDMI spec and
> ALSA userspace PoV.
> 
> And if we can't make that convenient, then maybe we just shouldn't try.

Ok, judging from the broonie's answer, it should be HPD, indeed (or
maybe get_modes, as not to require an additional EDID read).

> > > > I'll rename this function to something like ..hdmi_codec_enable. or
> > > > ... hdmi_codec_set_enabled.
> > > > 
> > > > >
> > > > > So something like drm_connector_hdmi_handle_hotplug, which would then do
> > > > > the above if there's audio support.
> > > > >
> > > > > > +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> > > > > > +                                                 void *ptr)
> > > > > > +{
> > > > > > +     struct platform_device *pdev = ptr;
> > > > > > +
> > > > > > +     platform_device_unregister(pdev);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> > > > > > + * @dev: DRM device
> > > > > > + * @connector: A pointer to the connector to allocate codec for
> > > > > > + * @ops: callbacks for this connector
> > > > > > + *
> > > > > > + * Create a HDMI codec device to be used with the specified connector.
> > > > > > + *
> > > > > > + * Cleanup is automatically handled with in a DRM-managed action.
> > > > > > + *
> > > > > > + * The connector structure should be allocated with drmm_kzalloc().
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * Zero on success, error code on failure.
> > > > > > + */
> > > > > > +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> > > > > > +                                 struct drm_connector *connector,
> > > > > > +                                 const struct hdmi_codec_ops *base_ops)
> > > > > > +{
> > > > > > +     struct hdmi_codec_pdata codec_pdata = {};
> > > > > > +     struct platform_device *pdev;
> > > > > > +     struct hdmi_codec_ops *ops;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> > > > > > +     if (!ops)
> > > > > > +             return -ENOMEM;
> > > > >
> > > > > Do we actually need to allocate a new structure here?
> > > > 
> > > > I didn't want to change the hdmi-codec's logic too much. But maybe
> > > > it's really better to have generic ops implementation here that calls
> > > > into the driver-specific callbacks.
> > > > 
> > > > > > +     *ops = *base_ops;
> > > > > > +
> > > > > > +     ops->get_eld = drm_connector_hdmi_codec_get_eld;
> > > > > > +     ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> > > > > > +
> > > > > > +     codec_pdata.ops = ops;
> > > > > > +     codec_pdata.i2s = connector->hdmi_codec.i2s,
> > > > > > +     codec_pdata.spdif = connector->hdmi_codec.spdif,
> > > > > > +     codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> > > > > > +     codec_pdata.data = connector;
> > > > > > +
> > > > > > +     pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> > > > > > +                                          HDMI_CODEC_DRV_NAME,
> > > > > > +                                          PLATFORM_DEVID_AUTO,
> > > > > > +                                          &codec_pdata, sizeof(codec_pdata));
> > > > >
> > > > > I think parent_dev should be setup by drm_connector_hdmi_init. I guess
> > > > > what I'm trying to say is that the reason HDMI support has been so
> > > > > heterogenous is precisely because of the proliferation of functions they
> > > > > needed to call, and so most drivers were doing the bare minimum until it
> > > > > worked (or they encountered a bug).
> > > > >
> > > > > What I was trying to do with the HDMI connector stuff was to make the
> > > > > easiest approach the one that works according to the spec, for
> > > > > everything.
> > > > >
> > > > > Audio is optional, so it should be a togglable thing (either by an
> > > > > additional function or parameter), but the drivers shouldn't have to set
> > > > > everything more than what the function requires.
> > > > 
> > > > I'll see what I can do. I had more or less the same goals, being hit
> > > > by the lack of the plugged_cb and get_eld support in the bridge's
> > > > implementation.
> > > > 
> > > > > Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
> > > > > structure as the device data and overwrite whatever we put there.
> > > > 
> > > > It registers driver_data for the created device, it doesn't touch parent_dev.
> > > > 
> > > > >
> > > > > We worked around it in vc4 by making sure that snd_soc_card was right at
> > > > > the start of the driver structure and thus both pointers would be equal,
> > > > > but we have to deal with it here too.
> > > > 
> > > > Hmm, maybe I'm missing something. The snd_soc_card is a different
> > > > story. The bridges just provide the hdmi_codec_ops, the card itself is
> > > > handled by the other driver.
> > > 
> > > For bridges, sure. For full blown controllers, it might be handled by
> > > the driver directly if there's no external controllers involved.
> > 
> > Hmm, I see. Let me check how vc4 handles it. But anyway, snd_soc_card is
> > out of scope for this patchset. The driver has to manage it anyway. And
> > for the hdmi_audio_codec there is no conflict.
> 
> Out of scope, sure, but if we need to rework and retest the whole thing
> when we get there, it's not great either. So we should take it into
> account still. Not care about or work on it, but leave the door open.

Anyway, for VC4:

static struct hdmi_codec_pdata vc4_hdmi_codec_pdata = {
        .ops = &vc4_hdmi_codec_ops,
        .max_i2s_channels = 8,
        .i2s = 1,
};

codec_pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
					   PLATFORM_DEVID_AUTO,
					   &vc4_hdmi_codec_pdata,
					   sizeof(vc4_hdmi_codec_pdata));

So for the codec it also passes a separate data structure, not realted
to the snd_soc_card data or to the VC4's pdata.

> 
> > > > > > +     if (IS_ERR(pdev))
> > > > > > +             return PTR_ERR(pdev);
> > > > > > +
> > > > > > +     ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     connector->hdmi_codec.codec_pdev = pdev;
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> > > > > > +
> > > > > > +/**
> > > > > > + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> > > > > > + * @dev: DRM device
> > > > > > + * @hdmi_codec: A pointer to the HDMI codec data
> > > > > > + *
> > > > > > + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> > > > > > + * While this function should not be necessary for a typical driver, DRM bridge
> > > > > > + * drivers have to call it from the remove callback if the bridge uses
> > > > > > + * Connector's HDMI Codec interface.
> > > > > > + */
> > > > > > +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> > > > > > +                                 struct drm_connector_hdmi_codec *hdmi_codec)
> > > > > > +{
> > > > > > +     drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> > > > > > +                         hdmi_codec->codec_pdev);
> > > > > > +}
> > > > >
> > > > > What would it be useful for?
> > > > 
> > > > See the last patch,
> > > > https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@linaro.org/
> > > > 
> > > > if the bridge driver gets unbound, we should also unregister the codec
> > > > device. The codec infrastructure uses drmm to allocate data and a drmm
> > > > action to unregister the codec device. However the bridge drivers are
> > > > not bound by the drmm lifecycle. So we have to do that manually.
> > > 
> > > Bridge lifetimes in general are a mess, but why do we need to involve
> > > drmm if it's manual then?
> > > 
> > > It's typically something that shouldn't be done by drivers anyway. Most
> > > of them will get it wrong.
> > 
> > Non-bridge drivers are not such mess and using drmm_ makes it simpler
> > for them.
> 
> That's arguable, but it's mostly because the framework allows those
> drivers to not be messy :)
> 
> It doesn't with bridges.
> 
> > Also in case of DP MST this will make like slightly easier as the
> > audio codec will be torn down together with the connector being gone.
> > 
> > But really, I'm open to any solution that will work. Maybe it's better
> > to use devm_add_action_or_reset(codec->parent);
> 
> My point is that there's no point in registering a drmm action if
> drivers are supposed to call it anyway. So we can just use neither drmm
> or devm, and it'll work just the same.
> 
> But I still think we don't need to allocate the structure in the first
> place and just put it into drm_connector.

This might work indeed. I was thinking about it from a different
direction.

> > > > > > +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > > index f750765d8fbc..0eb8d8ed9495 100644
> > > > > > --- a/include/drm/drm_connector.h
> > > > > > +++ b/include/drm/drm_connector.h
> > > > > > @@ -46,6 +46,7 @@ struct drm_property_blob;
> > > > > >  struct drm_printer;
> > > > > >  struct drm_privacy_screen;
> > > > > >  struct edid;
> > > > > > +struct hdmi_codec_ops;
> > > > > >  struct i2c_adapter;
> > > > > >
> > > > > >  enum drm_connector_force {
> > > > > > @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
> > > > > >       int (*write_infoframe)(struct drm_connector *connector,
> > > > > >                              enum hdmi_infoframe_type type,
> > > > > >                              const u8 *buffer, size_t len);
> > > > > > +
> > > > > > +     const struct hdmi_codec_ops *codec_ops;
> > > > >
> > > > > I think I'd rather have the HDMI connector framework provide the ASoC
> > > > > hooks, and make the needed pointer casts / lookups to provide a
> > > > > consistent API to drivers using it.
> > > > >
> > > > > This will probably also solve the issue mentioned above.
> > > > 
> > > > Ack.
> > > > 
> > > > >
> > > > > >  };
> > > > > >
> > > > > >  /**
> > > > > > @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
> > > > > >       } infoframes;
> > > > > >  };
> > > > > >
> > > > > > +struct drm_connector_hdmi_codec {
> > > > > > +     struct device *parent_dev;
> > > > > > +     struct platform_device *codec_pdev;
> > > > > > +
> > > > > > +     const struct drm_connector_hdmi_codec_funcs *funcs;
> > > > > > +
> > > > > > +     struct mutex lock; /* protects last_state and plugged_cb */
> > > > > > +     void (*plugged_cb)(struct device *dev, bool plugged);
> > > > > > +     struct device *plugged_cb_dev;
> > > > > > +     bool last_state;
> > > > > > +
> > > > > > +     int max_i2s_channels;
> > > > > > +     uint i2s: 1;
> > > > > > +     uint spdif: 1;
> > > > > > +};
> > > > >
> > > > > It would be great to have some documentation on what those are,
> > > > > last_state and the mutex especially raise attention :)
> > > > 
> > > > Yep, as I wrote in the cover letter, underdocumented.
> > > > 
> > > > >
> > > > >
> > > > > >  /**
> > > > > >   * struct drm_connector - central DRM connector control structure
> > > > > >   *
> > > > > > @@ -2119,6 +2138,12 @@ struct drm_connector {
> > > > > >        * @hdmi: HDMI-related variable and properties.
> > > > > >        */
> > > > > >       struct drm_connector_hdmi hdmi;
> > > > > > +
> > > > > > +     /**
> > > > > > +      * @hdmi_codec: HDMI codec properties and variables. Also might be used
> > > > > > +      * for DisplayPort audio.
> > > > > > +      */
> > > > > > +     struct drm_connector_hdmi_codec hdmi_codec;
> > > > >
> > > > > I'd rather make this part of drm_connector_hdmi, it cannot work without it.
> > > > 
> > > > It can. DisplayPort drivers also use hdmi_codec_ops. They should be
> > > > able to benefit from this implementation.
> > > 
> > > That's totally doable if we create a structure (and functions) that are
> > > embedded in both drm_connector_hdmi and the future drm_connector_dp
> > 
> > There is no drm_connector_dp (yet), but the drivers can already benefit
> > from using the generic hdmi_codec. Later on, when drm_connector_dp
> > appears, we can move the codec into both hdmi and DP structures.
> > 
> > I can probably convert msm/hdmi and msm/dp to use this framework if that
> > helps to express the idea.
> 
> I think there's something I don't get here: why does DP gets in the way,
> and why can't we just do the HDMI support now, and then reuse the same
> struct and internal functions with DP later on?
> 
> I think if we want DP support, we would need to create a DP framework
> like we did for HDMI, and that would be a major undertaking just for
> audio support.

That's what I wanted to defer for now. But I think I got your point
here. I'll extend drm_connector_hdmi and DRM_OP_HDMI instead.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard]

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

On Thu, Jun 27, 2024 at 04:29:49PM GMT, Dmitry Baryshkov wrote:
> On Thu, Jun 27, 2024 at 11:49:37AM GMT, Maxime Ripard wrote:
> > On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
> > > On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> > > > On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> > > > > On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Sorry for taking some time to review this series.
> > > > > 
> > > > > No problem, that's not long.
> > > > > 
> > > > > >
> > > > > > On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> > > > > > > Several DRM drivers implement HDMI codec support (despite its name it
> > > > > > > applies to both HDMI and DisplayPort drivers). Implement generic
> > > > > > > framework to be used by these drivers. This removes a requirement to
> > > > > > > implement get_eld() callback and provides default implementation for
> > > > > > > codec's plug handling.
> > > > > > >
> > > > > > > The framework is integrated with the DRM HDMI Connector framework, but
> > > > > > > can be used by DisplayPort drivers.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/Makefile                   |   1 +
> > > > > > >  drivers/gpu/drm/drm_connector.c            |   8 ++
> > > > > > >  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> > > > > > >  include/drm/drm_connector.h                |  33 ++++++
> > > > > > >  4 files changed, 199 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > > > > index 68cc9258ffc4..e113a6eade23 100644
> > > > > > > --- a/drivers/gpu/drm/Makefile
> > > > > > > +++ b/drivers/gpu/drm/Makefile
> > > > > > > @@ -45,6 +45,7 @@ drm-y := \
> > > > > > >       drm_client_modeset.o \
> > > > > > >       drm_color_mgmt.o \
> > > > > > >       drm_connector.o \
> > > > > > > +     drm_connector_hdmi_codec.o \
> > > > > > >       drm_crtc.o \
> > > > > > >       drm_displayid.o \
> > > > > > >       drm_drv.o \
> > > > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > > > > index 3d73a981004c..66d6e9487339 100644
> > > > > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > > > > @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> > > > > > >       mutex_init(&connector->mutex);
> > > > > > >       mutex_init(&connector->edid_override_mutex);
> > > > > > >       mutex_init(&connector->hdmi.infoframes.lock);
> > > > > > > +     mutex_init(&connector->hdmi_codec.lock);
> > > > > > >       connector->edid_blob_ptr = NULL;
> > > > > > >       connector->epoch_counter = 0;
> > > > > > >       connector->tile_blob_ptr = NULL;
> > > > > > > @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> > > > > > >
> > > > > > >       connector->hdmi.funcs = hdmi_funcs;
> > > > > > >
> > > > > > > +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> > > > > > > +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> > > > > > > +             if (ret)
> > > > > > > +                     return ret;
> > > > > > > +     }
> > > > > > > +
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> > > > > > > @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> > > > > > >               connector->funcs->atomic_destroy_state(connector,
> > > > > > >                                                      connector->state);
> > > > > > >
> > > > > > > +     mutex_destroy(&connector->hdmi_codec.lock);
> > > > > > >       mutex_destroy(&connector->hdmi.infoframes.lock);
> > > > > > >       mutex_destroy(&connector->mutex);
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..a3a7ad117f6f
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> > > > > > > @@ -0,0 +1,157 @@
> > > > > > > +/*
> > > > > > > + * Copyright (c) 2024 Linaro Ltd
> > > > > > > + *
> > > > > > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > > > > > + * documentation for any purpose is hereby granted without fee, provided that
> > > > > > > + * the above copyright notice appear in all copies and that both that copyright
> > > > > > > + * notice and this permission notice appear in supporting documentation, and
> > > > > > > + * that the name of the copyright holders not be used in advertising or
> > > > > > > + * publicity pertaining to distribution of the software without specific,
> > > > > > > + * written prior permission.  The copyright holders make no representations
> > > > > > > + * about the suitability of this software for any purpose.  It is provided "as
> > > > > > > + * is" without express or implied warranty.
> > > > > > > + *
> > > > > > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > > > > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > > > > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > > > > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > > > > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > > > > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > > > > > + * OF THIS SOFTWARE.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/mutex.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +
> > > > > > > +#include <drm/drm_connector.h>
> > > > > > > +#include <drm/drm_managed.h>
> > > > > > > +
> > > > > > > +#include <sound/hdmi-codec.h>
> > > > > > > +
> > > > > > > +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> > > > > > > +                                         uint8_t *buf, size_t len)
> > > > > > > +{
> > > > > > > +     struct drm_connector *connector = data;
> > > > > > > +
> > > > > > > +     //  FIXME: locking against drm_edid_to_eld ?
> > > > > > > +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> > > > > > > +                                                 void *data,
> > > > > > > +                                                 hdmi_codec_plugged_cb fn,
> > > > > > > +                                                 struct device *codec_dev)
> > > > > > > +{
> > > > > > > +     struct drm_connector *connector = data;
> > > > > > > +
> > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     connector->hdmi_codec.plugged_cb = fn;
> > > > > > > +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> > > > > > > +
> > > > > > > +     fn(codec_dev, connector->hdmi_codec.last_state);
> > > > > > > +
> > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> > > > > > > +                                          bool plugged)
> > > > > > > +{
> > > > > > > +     mutex_lock(&connector->hdmi_codec.lock);
> > > > > > > +
> > > > > > > +     connector->hdmi_codec.last_state = plugged;
> > > > > > > +
> > > > > > > +     if (connector->hdmi_codec.plugged_cb &&
> > > > > > > +         connector->hdmi_codec.plugged_cb_dev)
> > > > > > > +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> > > > > > > +                                              connector->hdmi_codec.last_state);
> > > > > > > +
> > > > > > > +     mutex_unlock(&connector->hdmi_codec.lock);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> > > > > >
> > > > > > I think we should do this the other way around, or rather, like we do
> > > > > > for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> > > > > > things (CEC, HDMI 2.0, audio), so it would be best to have a single
> > > > > > function to call from drivers, that will perform whatever is needed
> > > > > > depending on the driver's capabilities.
> > > > > 
> > > > > I see, this API is probably misnamed. The hdmi_codec_ops use the
> > > > > 'plugged' term,
> > > > 
> > > > Is it misnamed?
> > > > 
> > > > It's documented as:
> > > > 
> > > >   Hook callback function to handle connector plug event. Optional.
> > > > 
> > > > > but most of the drivers notify the ASoC / codec during atomic_enable /
> > > > > atomic_disable path, because usually the audio path can not work with
> > > > > the video path being disabled.
> > > > 
> > > > That's not clear to me either:
> > > > 
> > > >   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> > > >     enable/disable
> > > > 
> > > >   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> > > > 
> > > >   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> > > >     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> > > > 
> > > > So it doesn't look like there's a majority we can align with, and
> > > > neither should we: we need to figure out what we *need* to do and when,
> > > > and do that.
> > > > 
> > > > From the documentation and quickly through the code though, handling it
> > > > in detect looks like the right call.
> > > 
> > > It is tempting to have it in the hotplug call. However:
> > > 
> > > - It is used to send events to the ASoC Jack, marking the output as
> > >   plugged or unplugged. Once the output is plugged, userspace might
> > >   consider using it for the audio output. Please correct me if I'm
> > >   wrong, but I don't think one can output audio to the HDMI plug unless
> > >   there is a video stream.
> > 
> > That's something to check in the HDMI spec and with the ALSA
> > maintainers.
> 
> Mark and Liam are on CC list. I've also pinged Mark on the IRC (on
> #alsa, if the channel logs are preserved somewhere)
> 
> <lumag> I'm trying to implement a somewhat generic implementation that the drivers can hook in. The main discussion is at [link to this discussion]
> <lumag> So in theory that affects all ASoC platforms having HDMI or DP audio output
> <broonie> In that case I'd be conservative and try to follow the state of the physical connection as closely as possible.
> 
> So it is really 'plugged'.

Ack.

> > > - Having it in the hotplug notification chain is also troublesome. As
> > >   Dave pointed out in the quoted piece of code, it should come after
> > >   reading the EDID on the connect event. On the disconnect event it
> > >   should probably come before calling the notification chain, to let
> > >   audio code interract correctly with the fully enabled display devices.
> > 
> > EDIDs are fetched when hotplug is detected anyway, and we need it for
> > other things anyway (like CEC).
> 
> I see that:
> 
> - VC4 reads EDID and sets CEC address directly in hotplug notifier and
>   then again in get_modes callback. (why is it necessary in the hotplug
>   notifier, if it's done anyway in get_modes?)

vc4 is probably somewhat correct, but also a bit redundant here: the CEC
physical address is supposed to be set when we detect a sink.

When that sink is removed, we don't have a physical address anymore and
we thus need to call cec_phys_addr_invalidate.

When a sink is plugged again, we need to call cec_s_phys_addr() with the
sink address.

So what vc4_hdmi_handle_hotplug is doing is fine, but the one in the
get_modes might be redundant.

> - sun4i sets CEC address from get_modes

Yeah, that doesn't work. 

If the display is switched to another one and if the userspace doesn't
react to the hotplug event by calling get_modes, the physical address
will be the old one.

But since it's a polled hotplug, it's a situation that generally sucks.

> - ADV7511 does a trick and sets CEC address from edid_read() callback
>   (with the FIXME from Jani that basically tells us to move this to
>   get_modes)

Same situation than sun4i here, except for the fact that it can handle
hotplug through an interrupt.

> - omapdrm clears CEC address from hpd_notify, but sets it from the
>   edid_read() callback with the same FIXME.

I think it's still broken there. It properly clears the physical address
when the sink is disconnected, but relies on someone calling get_modes
to set it again, which isn't guaranteed.

> - i915 sets CEC address from .detect_ctx callback

That one is vc4 minus the get_modes redundancy.

> So there is no uniformity too. Handling it from drm_bridge_connector() /
> get_modes might help, but that requires clearing one of TODO items.

There's no uniformity, but I guess both vc4 and i915 are much more
battle-tested than sun4i, omapdrm, or adv7511 might be, and they both
behave pretty much the same.

Generally speaking (and it might be sad), but i915 is what userspace
expects, so it's usually what we want to follow.

Maxime

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

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Jonas Karlman]

On 2024-07-25 11:25, Maxime Ripard wrote:
> On Thu, Jun 27, 2024 at 04:29:49PM GMT, Dmitry Baryshkov wrote:
>> On Thu, Jun 27, 2024 at 11:49:37AM GMT, Maxime Ripard wrote:
>>> On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
>>>> On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
>>>>> On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
>>>>>> On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Sorry for taking some time to review this series.
>>>>>>
>>>>>> No problem, that's not long.
>>>>>>
>>>>>>>
>>>>>>> On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
>>>>>>>> Several DRM drivers implement HDMI codec support (despite its name it
>>>>>>>> applies to both HDMI and DisplayPort drivers). Implement generic
>>>>>>>> framework to be used by these drivers. This removes a requirement to
>>>>>>>> implement get_eld() callback and provides default implementation for
>>>>>>>> codec's plug handling.
>>>>>>>>
>>>>>>>> The framework is integrated with the DRM HDMI Connector framework, but
>>>>>>>> can be used by DisplayPort drivers.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/Makefile                   |   1 +
>>>>>>>>  drivers/gpu/drm/drm_connector.c            |   8 ++
>>>>>>>>  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
>>>>>>>>  include/drm/drm_connector.h                |  33 ++++++
>>>>>>>>  4 files changed, 199 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>>>>>> index 68cc9258ffc4..e113a6eade23 100644
>>>>>>>> --- a/drivers/gpu/drm/Makefile
>>>>>>>> +++ b/drivers/gpu/drm/Makefile
>>>>>>>> @@ -45,6 +45,7 @@ drm-y := \
>>>>>>>>       drm_client_modeset.o \
>>>>>>>>       drm_color_mgmt.o \
>>>>>>>>       drm_connector.o \
>>>>>>>> +     drm_connector_hdmi_codec.o \
>>>>>>>>       drm_crtc.o \
>>>>>>>>       drm_displayid.o \
>>>>>>>>       drm_drv.o \
>>>>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>>>>> index 3d73a981004c..66d6e9487339 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>>>>> @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
>>>>>>>>       mutex_init(&connector->mutex);
>>>>>>>>       mutex_init(&connector->edid_override_mutex);
>>>>>>>>       mutex_init(&connector->hdmi.infoframes.lock);
>>>>>>>> +     mutex_init(&connector->hdmi_codec.lock);
>>>>>>>>       connector->edid_blob_ptr = NULL;
>>>>>>>>       connector->epoch_counter = 0;
>>>>>>>>       connector->tile_blob_ptr = NULL;
>>>>>>>> @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>>>>>>>>
>>>>>>>>       connector->hdmi.funcs = hdmi_funcs;
>>>>>>>>
>>>>>>>> +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
>>>>>>>> +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
>>>>>>>> +             if (ret)
>>>>>>>> +                     return ret;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>>       return 0;
>>>>>>>>  }
>>>>>>>>  EXPORT_SYMBOL(drmm_connector_hdmi_init);
>>>>>>>> @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>>>>>>>>               connector->funcs->atomic_destroy_state(connector,
>>>>>>>>                                                      connector->state);
>>>>>>>>
>>>>>>>> +     mutex_destroy(&connector->hdmi_codec.lock);
>>>>>>>>       mutex_destroy(&connector->hdmi.infoframes.lock);
>>>>>>>>       mutex_destroy(&connector->mutex);
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..a3a7ad117f6f
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
>>>>>>>> @@ -0,0 +1,157 @@
>>>>>>>> +/*
>>>>>>>> + * Copyright (c) 2024 Linaro Ltd
>>>>>>>> + *
>>>>>>>> + * Permission to use, copy, modify, distribute, and sell this software and its
>>>>>>>> + * documentation for any purpose is hereby granted without fee, provided that
>>>>>>>> + * the above copyright notice appear in all copies and that both that copyright
>>>>>>>> + * notice and this permission notice appear in supporting documentation, and
>>>>>>>> + * that the name of the copyright holders not be used in advertising or
>>>>>>>> + * publicity pertaining to distribution of the software without specific,
>>>>>>>> + * written prior permission.  The copyright holders make no representations
>>>>>>>> + * about the suitability of this software for any purpose.  It is provided "as
>>>>>>>> + * is" without express or implied warranty.
>>>>>>>> + *
>>>>>>>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
>>>>>>>> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
>>>>>>>> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
>>>>>>>> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
>>>>>>>> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
>>>>>>>> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
>>>>>>>> + * OF THIS SOFTWARE.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <linux/mutex.h>
>>>>>>>> +#include <linux/platform_device.h>
>>>>>>>> +
>>>>>>>> +#include <drm/drm_connector.h>
>>>>>>>> +#include <drm/drm_managed.h>
>>>>>>>> +
>>>>>>>> +#include <sound/hdmi-codec.h>
>>>>>>>> +
>>>>>>>> +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
>>>>>>>> +                                         uint8_t *buf, size_t len)
>>>>>>>> +{
>>>>>>>> +     struct drm_connector *connector = data;
>>>>>>>> +
>>>>>>>> +     //  FIXME: locking against drm_edid_to_eld ?
>>>>>>>> +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
>>>>>>>> +                                                 void *data,
>>>>>>>> +                                                 hdmi_codec_plugged_cb fn,
>>>>>>>> +                                                 struct device *codec_dev)
>>>>>>>> +{
>>>>>>>> +     struct drm_connector *connector = data;
>>>>>>>> +
>>>>>>>> +     mutex_lock(&connector->hdmi_codec.lock);
>>>>>>>> +
>>>>>>>> +     connector->hdmi_codec.plugged_cb = fn;
>>>>>>>> +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
>>>>>>>> +
>>>>>>>> +     fn(codec_dev, connector->hdmi_codec.last_state);
>>>>>>>> +
>>>>>>>> +     mutex_unlock(&connector->hdmi_codec.lock);
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
>>>>>>>> +                                          bool plugged)
>>>>>>>> +{
>>>>>>>> +     mutex_lock(&connector->hdmi_codec.lock);
>>>>>>>> +
>>>>>>>> +     connector->hdmi_codec.last_state = plugged;
>>>>>>>> +
>>>>>>>> +     if (connector->hdmi_codec.plugged_cb &&
>>>>>>>> +         connector->hdmi_codec.plugged_cb_dev)
>>>>>>>> +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
>>>>>>>> +                                              connector->hdmi_codec.last_state);
>>>>>>>> +
>>>>>>>> +     mutex_unlock(&connector->hdmi_codec.lock);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
>>>>>>>
>>>>>>> I think we should do this the other way around, or rather, like we do
>>>>>>> for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
>>>>>>> things (CEC, HDMI 2.0, audio), so it would be best to have a single
>>>>>>> function to call from drivers, that will perform whatever is needed
>>>>>>> depending on the driver's capabilities.
>>>>>>
>>>>>> I see, this API is probably misnamed. The hdmi_codec_ops use the
>>>>>> 'plugged' term,
>>>>>
>>>>> Is it misnamed?
>>>>>
>>>>> It's documented as:
>>>>>
>>>>>   Hook callback function to handle connector plug event. Optional.
>>>>>
>>>>>> but most of the drivers notify the ASoC / codec during atomic_enable /
>>>>>> atomic_disable path, because usually the audio path can not work with
>>>>>> the video path being disabled.
>>>>>
>>>>> That's not clear to me either:
>>>>>
>>>>>   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
>>>>>     enable/disable
>>>>>
>>>>>   - anx7625, mtk_hdmi and mtk_dp calls it in detect
>>>>>
>>>>>   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
>>>>>     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
>>>>>
>>>>> So it doesn't look like there's a majority we can align with, and
>>>>> neither should we: we need to figure out what we *need* to do and when,
>>>>> and do that.
>>>>>
>>>>> From the documentation and quickly through the code though, handling it
>>>>> in detect looks like the right call.
>>>>
>>>> It is tempting to have it in the hotplug call. However:
>>>>
>>>> - It is used to send events to the ASoC Jack, marking the output as
>>>>   plugged or unplugged. Once the output is plugged, userspace might
>>>>   consider using it for the audio output. Please correct me if I'm
>>>>   wrong, but I don't think one can output audio to the HDMI plug unless
>>>>   there is a video stream.
>>>
>>> That's something to check in the HDMI spec and with the ALSA
>>> maintainers.
>>
>> Mark and Liam are on CC list. I've also pinged Mark on the IRC (on
>> #alsa, if the channel logs are preserved somewhere)
>>
>> <lumag> I'm trying to implement a somewhat generic implementation that the drivers can hook in. The main discussion is at [link to this discussion]
>> <lumag> So in theory that affects all ASoC platforms having HDMI or DP audio output
>> <broonie> In that case I'd be conservative and try to follow the state of the physical connection as closely as possible.
>>
>> So it is really 'plugged'.
> 
> Ack.
> 
>>>> - Having it in the hotplug notification chain is also troublesome. As
>>>>   Dave pointed out in the quoted piece of code, it should come after
>>>>   reading the EDID on the connect event. On the disconnect event it
>>>>   should probably come before calling the notification chain, to let
>>>>   audio code interract correctly with the fully enabled display devices.
>>>
>>> EDIDs are fetched when hotplug is detected anyway, and we need it for
>>> other things anyway (like CEC).
>>
>> I see that:
>>
>> - VC4 reads EDID and sets CEC address directly in hotplug notifier and
>>   then again in get_modes callback. (why is it necessary in the hotplug
>>   notifier, if it's done anyway in get_modes?)
> 
> vc4 is probably somewhat correct, but also a bit redundant here: the CEC
> physical address is supposed to be set when we detect a sink.
> 
> When that sink is removed, we don't have a physical address anymore and
> we thus need to call cec_phys_addr_invalidate.
> 
> When a sink is plugged again, we need to call cec_s_phys_addr() with the
> sink address.
> 
> So what vc4_hdmi_handle_hotplug is doing is fine, but the one in the
> get_modes might be redundant.
> 
>> - sun4i sets CEC address from get_modes
> 
> Yeah, that doesn't work. 
> 
> If the display is switched to another one and if the userspace doesn't
> react to the hotplug event by calling get_modes, the physical address
> will be the old one.
> 
> But since it's a polled hotplug, it's a situation that generally sucks.
> 
>> - ADV7511 does a trick and sets CEC address from edid_read() callback
>>   (with the FIXME from Jani that basically tells us to move this to
>>   get_modes)
> 
> Same situation than sun4i here, except for the fact that it can handle
> hotplug through an interrupt.
> 
>> - omapdrm clears CEC address from hpd_notify, but sets it from the
>>   edid_read() callback with the same FIXME.
> 
> I think it's still broken there. It properly clears the physical address
> when the sink is disconnected, but relies on someone calling get_modes
> to set it again, which isn't guaranteed.
> 
>> - i915 sets CEC address from .detect_ctx callback
> 
> That one is vc4 minus the get_modes redundancy.
> 
>> So there is no uniformity too. Handling it from drm_bridge_connector() /
>> get_modes might help, but that requires clearing one of TODO items.
> 
> There's no uniformity, but I guess both vc4 and i915 are much more
> battle-tested than sun4i, omapdrm, or adv7511 might be, and they both
> behave pretty much the same.
> 
> Generally speaking (and it might be sad), but i915 is what userspace
> expects, so it's usually what we want to follow.

For the dw-hdmi connector I also moved the edid read and cec phys addr
handling from hpd irq to the detect callback in [1]. That seem to be the
best option as it gets called by core after/during hpd processing and
also more closely matches i915.

Was not fully sure what to do about the hdmi-codec callback so left it
at enable/disable as that seemed to best match the expected plugged
state. However, dw-hdmi connector will more than likely trigger a
disconnected/connected state change and a disable/enable cycle during
an edid refresh from the sink, i.e. when a connected AVR is powered on
or off, so it does now always match the physical plugged state.

[1] https://lore.kernel.org/all/20240611155108.1436502-1-jonas@kwiboo.se/

Regards,
Jonas

> 
> Maxime

Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Dmitry Baryshkov]

On Thu, Jul 25, 2024 at 01:06:35PM GMT, Jonas Karlman wrote:
> On 2024-07-25 11:25, Maxime Ripard wrote:
> > On Thu, Jun 27, 2024 at 04:29:49PM GMT, Dmitry Baryshkov wrote:
> >> On Thu, Jun 27, 2024 at 11:49:37AM GMT, Maxime Ripard wrote:
> >>> On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
> >>>> On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> >>>>> On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> >>>>>> On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@kernel.org> wrote:
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Sorry for taking some time to review this series.
> >>>>>>
> >>>>>> No problem, that's not long.
> >>>>>>
> >>>>>>>
> >>>>>>> On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> >>>>>>>> Several DRM drivers implement HDMI codec support (despite its name it
> >>>>>>>> applies to both HDMI and DisplayPort drivers). Implement generic
> >>>>>>>> framework to be used by these drivers. This removes a requirement to
> >>>>>>>> implement get_eld() callback and provides default implementation for
> >>>>>>>> codec's plug handling.
> >>>>>>>>
> >>>>>>>> The framework is integrated with the DRM HDMI Connector framework, but
> >>>>>>>> can be used by DisplayPort drivers.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>>> ---
> >>>>>>>>  drivers/gpu/drm/Makefile                   |   1 +
> >>>>>>>>  drivers/gpu/drm/drm_connector.c            |   8 ++
> >>>>>>>>  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
> >>>>>>>>  include/drm/drm_connector.h                |  33 ++++++
> >>>>>>>>  4 files changed, 199 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >>>>>>>> index 68cc9258ffc4..e113a6eade23 100644
> >>>>>>>> --- a/drivers/gpu/drm/Makefile
> >>>>>>>> +++ b/drivers/gpu/drm/Makefile
> >>>>>>>> @@ -45,6 +45,7 @@ drm-y := \
> >>>>>>>>       drm_client_modeset.o \
> >>>>>>>>       drm_color_mgmt.o \
> >>>>>>>>       drm_connector.o \
> >>>>>>>> +     drm_connector_hdmi_codec.o \
> >>>>>>>>       drm_crtc.o \
> >>>>>>>>       drm_displayid.o \
> >>>>>>>>       drm_drv.o \
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>>>>>>> index 3d73a981004c..66d6e9487339 100644
> >>>>>>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>>>>>> @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
> >>>>>>>>       mutex_init(&connector->mutex);
> >>>>>>>>       mutex_init(&connector->edid_override_mutex);
> >>>>>>>>       mutex_init(&connector->hdmi.infoframes.lock);
> >>>>>>>> +     mutex_init(&connector->hdmi_codec.lock);
> >>>>>>>>       connector->edid_blob_ptr = NULL;
> >>>>>>>>       connector->epoch_counter = 0;
> >>>>>>>>       connector->tile_blob_ptr = NULL;
> >>>>>>>> @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >>>>>>>>
> >>>>>>>>       connector->hdmi.funcs = hdmi_funcs;
> >>>>>>>>
> >>>>>>>> +     if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> >>>>>>>> +             ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> >>>>>>>> +             if (ret)
> >>>>>>>> +                     return ret;
> >>>>>>>> +     }
> >>>>>>>> +
> >>>>>>>>       return 0;
> >>>>>>>>  }
> >>>>>>>>  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> >>>>>>>> @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
> >>>>>>>>               connector->funcs->atomic_destroy_state(connector,
> >>>>>>>>                                                      connector->state);
> >>>>>>>>
> >>>>>>>> +     mutex_destroy(&connector->hdmi_codec.lock);
> >>>>>>>>       mutex_destroy(&connector->hdmi.infoframes.lock);
> >>>>>>>>       mutex_destroy(&connector->mutex);
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..a3a7ad117f6f
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> >>>>>>>> @@ -0,0 +1,157 @@
> >>>>>>>> +/*
> >>>>>>>> + * Copyright (c) 2024 Linaro Ltd
> >>>>>>>> + *
> >>>>>>>> + * Permission to use, copy, modify, distribute, and sell this software and its
> >>>>>>>> + * documentation for any purpose is hereby granted without fee, provided that
> >>>>>>>> + * the above copyright notice appear in all copies and that both that copyright
> >>>>>>>> + * notice and this permission notice appear in supporting documentation, and
> >>>>>>>> + * that the name of the copyright holders not be used in advertising or
> >>>>>>>> + * publicity pertaining to distribution of the software without specific,
> >>>>>>>> + * written prior permission.  The copyright holders make no representations
> >>>>>>>> + * about the suitability of this software for any purpose.  It is provided "as
> >>>>>>>> + * is" without express or implied warranty.
> >>>>>>>> + *
> >>>>>>>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> >>>>>>>> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> >>>>>>>> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> >>>>>>>> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> >>>>>>>> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> >>>>>>>> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> >>>>>>>> + * OF THIS SOFTWARE.
> >>>>>>>> + */
> >>>>>>>> +
> >>>>>>>> +#include <linux/mutex.h>
> >>>>>>>> +#include <linux/platform_device.h>
> >>>>>>>> +
> >>>>>>>> +#include <drm/drm_connector.h>
> >>>>>>>> +#include <drm/drm_managed.h>
> >>>>>>>> +
> >>>>>>>> +#include <sound/hdmi-codec.h>
> >>>>>>>> +
> >>>>>>>> +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> >>>>>>>> +                                         uint8_t *buf, size_t len)
> >>>>>>>> +{
> >>>>>>>> +     struct drm_connector *connector = data;
> >>>>>>>> +
> >>>>>>>> +     //  FIXME: locking against drm_edid_to_eld ?
> >>>>>>>> +     memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> >>>>>>>> +
> >>>>>>>> +     return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> >>>>>>>> +                                                 void *data,
> >>>>>>>> +                                                 hdmi_codec_plugged_cb fn,
> >>>>>>>> +                                                 struct device *codec_dev)
> >>>>>>>> +{
> >>>>>>>> +     struct drm_connector *connector = data;
> >>>>>>>> +
> >>>>>>>> +     mutex_lock(&connector->hdmi_codec.lock);
> >>>>>>>> +
> >>>>>>>> +     connector->hdmi_codec.plugged_cb = fn;
> >>>>>>>> +     connector->hdmi_codec.plugged_cb_dev = codec_dev;
> >>>>>>>> +
> >>>>>>>> +     fn(codec_dev, connector->hdmi_codec.last_state);
> >>>>>>>> +
> >>>>>>>> +     mutex_unlock(&connector->hdmi_codec.lock);
> >>>>>>>> +
> >>>>>>>> +     return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> >>>>>>>> +                                          bool plugged)
> >>>>>>>> +{
> >>>>>>>> +     mutex_lock(&connector->hdmi_codec.lock);
> >>>>>>>> +
> >>>>>>>> +     connector->hdmi_codec.last_state = plugged;
> >>>>>>>> +
> >>>>>>>> +     if (connector->hdmi_codec.plugged_cb &&
> >>>>>>>> +         connector->hdmi_codec.plugged_cb_dev)
> >>>>>>>> +             connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> >>>>>>>> +                                              connector->hdmi_codec.last_state);
> >>>>>>>> +
> >>>>>>>> +     mutex_unlock(&connector->hdmi_codec.lock);
> >>>>>>>> +}
> >>>>>>>> +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);
> >>>>>>>
> >>>>>>> I think we should do this the other way around, or rather, like we do
> >>>>>>> for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
> >>>>>>> things (CEC, HDMI 2.0, audio), so it would be best to have a single
> >>>>>>> function to call from drivers, that will perform whatever is needed
> >>>>>>> depending on the driver's capabilities.
> >>>>>>
> >>>>>> I see, this API is probably misnamed. The hdmi_codec_ops use the
> >>>>>> 'plugged' term,
> >>>>>
> >>>>> Is it misnamed?
> >>>>>
> >>>>> It's documented as:
> >>>>>
> >>>>>   Hook callback function to handle connector plug event. Optional.
> >>>>>
> >>>>>> but most of the drivers notify the ASoC / codec during atomic_enable /
> >>>>>> atomic_disable path, because usually the audio path can not work with
> >>>>>> the video path being disabled.
> >>>>>
> >>>>> That's not clear to me either:
> >>>>>
> >>>>>   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> >>>>>     enable/disable
> >>>>>
> >>>>>   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> >>>>>
> >>>>>   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> >>>>>     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> >>>>>
> >>>>> So it doesn't look like there's a majority we can align with, and
> >>>>> neither should we: we need to figure out what we *need* to do and when,
> >>>>> and do that.
> >>>>>
> >>>>> From the documentation and quickly through the code though, handling it
> >>>>> in detect looks like the right call.
> >>>>
> >>>> It is tempting to have it in the hotplug call. However:
> >>>>
> >>>> - It is used to send events to the ASoC Jack, marking the output as
> >>>>   plugged or unplugged. Once the output is plugged, userspace might
> >>>>   consider using it for the audio output. Please correct me if I'm
> >>>>   wrong, but I don't think one can output audio to the HDMI plug unless
> >>>>   there is a video stream.
> >>>
> >>> That's something to check in the HDMI spec and with the ALSA
> >>> maintainers.
> >>
> >> Mark and Liam are on CC list. I've also pinged Mark on the IRC (on
> >> #alsa, if the channel logs are preserved somewhere)
> >>
> >> <lumag> I'm trying to implement a somewhat generic implementation that the drivers can hook in. The main discussion is at [link to this discussion]
> >> <lumag> So in theory that affects all ASoC platforms having HDMI or DP audio output
> >> <broonie> In that case I'd be conservative and try to follow the state of the physical connection as closely as possible.
> >>
> >> So it is really 'plugged'.
> > 
> > Ack.
> > 
> >>>> - Having it in the hotplug notification chain is also troublesome. As
> >>>>   Dave pointed out in the quoted piece of code, it should come after
> >>>>   reading the EDID on the connect event. On the disconnect event it
> >>>>   should probably come before calling the notification chain, to let
> >>>>   audio code interract correctly with the fully enabled display devices.
> >>>
> >>> EDIDs are fetched when hotplug is detected anyway, and we need it for
> >>> other things anyway (like CEC).
> >>
> >> I see that:
> >>
> >> - VC4 reads EDID and sets CEC address directly in hotplug notifier and
> >>   then again in get_modes callback. (why is it necessary in the hotplug
> >>   notifier, if it's done anyway in get_modes?)
> > 
> > vc4 is probably somewhat correct, but also a bit redundant here: the CEC
> > physical address is supposed to be set when we detect a sink.
> > 
> > When that sink is removed, we don't have a physical address anymore and
> > we thus need to call cec_phys_addr_invalidate.
> > 
> > When a sink is plugged again, we need to call cec_s_phys_addr() with the
> > sink address.
> > 
> > So what vc4_hdmi_handle_hotplug is doing is fine, but the one in the
> > get_modes might be redundant.

I see the logic here, but I don't like the duplication between
detect_ctx() and get_modes(). Doesn't it make drm_edid_read_ddc() in
get_modes() redundant?

> >> - sun4i sets CEC address from get_modes
> > 
> > Yeah, that doesn't work. 
> > 
> > If the display is switched to another one and if the userspace doesn't
> > react to the hotplug event by calling get_modes, the physical address
> > will be the old one.

Ack.

> > 
> > But since it's a polled hotplug, it's a situation that generally sucks.
> > 
> >> - ADV7511 does a trick and sets CEC address from edid_read() callback
> >>   (with the FIXME from Jani that basically tells us to move this to
> >>   get_modes)
> > 
> > Same situation than sun4i here, except for the fact that it can handle
> > hotplug through an interrupt.
> > 
> >> - omapdrm clears CEC address from hpd_notify, but sets it from the
> >>   edid_read() callback with the same FIXME.
> > 
> > I think it's still broken there. It properly clears the physical address
> > when the sink is disconnected, but relies on someone calling get_modes
> > to set it again, which isn't guaranteed.
> > 
> >> - i915 sets CEC address from .detect_ctx callback
> > 
> > That one is vc4 minus the get_modes redundancy.
> > 
> >> So there is no uniformity too. Handling it from drm_bridge_connector() /
> >> get_modes might help, but that requires clearing one of TODO items.
> > 
> > There's no uniformity, but I guess both vc4 and i915 are much more
> > battle-tested than sun4i, omapdrm, or adv7511 might be, and they both
> > behave pretty much the same.
> > 
> > Generally speaking (and it might be sad), but i915 is what userspace
> > expects, so it's usually what we want to follow.
> 
> For the dw-hdmi connector I also moved the edid read and cec phys addr
> handling from hpd irq to the detect callback in [1]. That seem to be the
> best option as it gets called by core after/during hpd processing and
> also more closely matches i915.

Yes, it seems that we should settle on this (and maybe document it as
the BCP).

> Was not fully sure what to do about the hdmi-codec callback so left it
> at enable/disable as that seemed to best match the expected plugged
> state. However, dw-hdmi connector will more than likely trigger a
> disconnected/connected state change and a disable/enable cycle during
> an edid refresh from the sink, i.e. when a connected AVR is powered on
> or off, so it does now always match the physical plugged state.

If I understood Mark correctly, the plugged callback should really
reflect the 'plugged' state (well, probably after reading the EDID to
make ELD available to ASoC subsys). Maybe we should have a separate
control for making the sink available for audio output, but nobody has
that yet.

> [1] https://lore.kernel.org/all/20240611155108.1436502-1-jonas@kwiboo.se/

-- 
With best wishes
Dmitry

Re: (subset) [PATCH RFC 0/5] drm: add DRM HDMI Codec framework

[Dmitry Baryshkov]

On Sat, 15 Jun 2024 20:53:29 +0300, Dmitry Baryshkov wrote:
> While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector
> framework, I stumbled upon an issue while handling the Audio InfoFrames.
> The HDMI codec callbacks weren't receiving the drm_atomic_state, so
> there was no simple way to get the drm_connector that stayed at the end
> of the bridge chain. At the same point the drm_hdmi_connector functions
> expected to get drm_connector instance.
> 
> [...]

Applied to drm-misc-next, thanks!

[1/5] drm/bridge: lt9611: use HDMI Connector helper to set InfoFrames
      commit: ac4627c304e7a09068d0e2dba34d21e492649f8d

I'm pushing this patch separately from the rest of the series, it provides an
example of converting DRM bridge driver to the HDMI Connector framework.

Best regards,
-- 
With best wishes
Dmitry