[PATCH 2/2] drm/mediatek: Don't support hdmi connector creation

[PATCH 2/2] drm/mediatek: Don't support hdmi connector creation

[Dafna Hirschfeld]

commit f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
broke the display support for elm device since mtk_dpi calls
drm_bridge_attach with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
while mtk_hdmi does not yet support this flag.

These two patches fix that by adding support for DRM_BRIDGE_ATTACH_NO_CONNECTOR
in mtk_hdmi bridge attachment. The first patch moves the mkt-hdmi bridge
to the atomic API in order to be able to access the connector through the
global state.

Dafna Hirschfeld (2):
  drm/mediatek: Switch the hdmi bridge ops to the atomic versions
  drm/mediatek: Don't support hdmi connector creation

 drivers/gpu/drm/mediatek/mtk_hdmi.c | 171 ++++++++++++----------------
 1 file changed, 73 insertions(+), 98 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/mediatek: Switch the hdmi bridge ops to the atomic versions

[Dafna Hirschfeld]

The bridge operation '.enable' and the audio cb '.get_eld'
access hdmi->conn. In the future we will want to support
the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
not have direct access to the connector.
The atomic version '.atomic_enable' allows accessing the
current connector from the state.
This patch switches the bridge to the atomic version and
saves the current connector in a new field 'curr_conn'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 41 ++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 8ee55f9e2954..9f415c508b33 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -154,6 +154,7 @@ struct mtk_hdmi {
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
 	struct drm_connector conn;
+	struct drm_connector *curr_conn;
 	struct device *dev;
 	const struct mtk_hdmi_conf *conf;
 	struct phy *phy;
@@ -974,7 +975,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
 	ssize_t err;
 
 	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
-						       &hdmi->conn, mode);
+						       hdmi->curr_conn, mode);
 	if (err < 0) {
 		dev_err(hdmi->dev,
 			"Failed to get AVI infoframe from mode: %zd\n", err);
@@ -1054,7 +1055,7 @@ static int mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
 	ssize_t err;
 
 	err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
-							  &hdmi->conn, mode);
+							  hdmi->curr_conn, mode);
 	if (err) {
 		dev_err(hdmi->dev,
 			"Failed to get vendor infoframe from mode: %zd\n", err);
@@ -1357,7 +1358,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
-static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 
@@ -1368,10 +1370,13 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
 	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
 	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
 
+	hdmi->curr_conn = NULL;
+
 	hdmi->enabled = false;
 }
 
-static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_state)
 {
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 
@@ -1406,7 +1411,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge *bridge,
 	drm_mode_copy(&hdmi->mode, adjusted_mode);
 }
 
-static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_state)
 {
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 
@@ -1426,10 +1432,16 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi *hdmi,
 		mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
 }
 
-static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *old_state)
 {
+	struct drm_atomic_state *state = old_state->base.state;
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 
+	/* Retrieve the connector through the atomic state. */
+	hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
+								   bridge->encoder);
+
 	mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode);
 	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
 	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
@@ -1440,13 +1452,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
 }
 
 static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.attach = mtk_hdmi_bridge_attach,
 	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
-	.disable = mtk_hdmi_bridge_disable,
-	.post_disable = mtk_hdmi_bridge_post_disable,
+	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
+	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
 	.mode_set = mtk_hdmi_bridge_mode_set,
-	.pre_enable = mtk_hdmi_bridge_pre_enable,
-	.enable = mtk_hdmi_bridge_enable,
+	.atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
+	.atomic_enable = mtk_hdmi_bridge_atomic_enable,
 };
 
 static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
@@ -1662,8 +1677,10 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf,
 {
 	struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
 
-	memcpy(buf, hdmi->conn.eld, min(sizeof(hdmi->conn.eld), len));
-
+	if (hdmi->curr_conn)
+		memcpy(buf, hdmi->curr_conn->eld, min(sizeof(hdmi->curr_conn->eld), len));
+	else
+		memset(buf, 0, len);
 	return 0;
 }
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/mediatek: Don't support hdmi connector creation

[Dafna Hirschfeld]

commit f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
broke the display support for elm device since mtk_dpi calls
drm_bridge_attach with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
while mtk_hdmi does not yet support this flag.

Fix this by accepting DRM_BRIDGE_ATTACH_NO_CONNECTOR in bridge attachment.
Implement the drm_bridge_funcs .detect() and .get_edid() operations, and
call drm_bridge_hpd_notify() to report HPD. This provides the
necessary API to support disabling connector creation.

This patch is inspired by a similar patch for bridge/synopsys/dw-hdmi.c:
commit ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional")
But with the difference that in mtk-hdmi only the option of not creating
a connector is supported.

Fixes: f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 130 ++++++++++------------------
 1 file changed, 44 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 9f415c508b33..9da5dfb7c7fb 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -153,7 +153,6 @@ struct mtk_hdmi_conf {
 struct mtk_hdmi {
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
-	struct drm_connector conn;
 	struct drm_connector *curr_conn;
 	struct device *dev;
 	const struct mtk_hdmi_conf *conf;
@@ -187,11 +186,6 @@ static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b)
 	return container_of(b, struct mtk_hdmi, bridge);
 }
 
-static inline struct mtk_hdmi *hdmi_ctx_from_conn(struct drm_connector *c)
-{
-	return container_of(c, struct mtk_hdmi, conn);
-}
-
 static u32 mtk_hdmi_read(struct mtk_hdmi *hdmi, u32 offset)
 {
 	return readl(hdmi->regs + offset);
@@ -1202,48 +1196,30 @@ mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi)
 	       connector_status_connected : connector_status_disconnected;
 }
 
-static enum drm_connector_status hdmi_conn_detect(struct drm_connector *conn,
-						  bool force)
-{
-	struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
-	return mtk_hdmi_update_plugged_status(hdmi);
-}
-
-static void hdmi_conn_destroy(struct drm_connector *conn)
+static struct edid *mtk_hdmi_get_edid(struct mtk_hdmi *hdmi,
+				      struct drm_connector *connector)
 {
-	struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
-
-	mtk_cec_set_hpd_event(hdmi->cec_dev, NULL, NULL);
-
-	drm_connector_cleanup(conn);
-}
-
-static int mtk_hdmi_conn_get_modes(struct drm_connector *conn)
-{
-	struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
 	struct edid *edid;
-	int ret;
 
 	if (!hdmi->ddc_adpt)
-		return -ENODEV;
-
-	edid = drm_get_edid(conn, hdmi->ddc_adpt);
+		return NULL;
+	edid = drm_get_edid(connector, hdmi->ddc_adpt);
 	if (!edid)
-		return -ENODEV;
-
+		return NULL;
 	hdmi->dvi_mode = !drm_detect_monitor_audio(edid);
+	return edid;
+}
 
-	drm_connector_update_edid_property(conn, edid);
-
-	ret = drm_add_edid_modes(conn, edid);
-	kfree(edid);
-	return ret;
+static enum drm_connector_status mtk_hdmi_detect(struct mtk_hdmi *hdmi)
+{
+	return mtk_hdmi_update_plugged_status(hdmi);
 }
 
-static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn,
-				    struct drm_display_mode *mode)
+static int mtk_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+				      const struct drm_display_info *info,
+				      const struct drm_display_mode *mode)
 {
-	struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
+	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 	struct drm_bridge *next_bridge;
 
 	dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n",
@@ -1268,74 +1244,50 @@ static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn,
 	return drm_mode_validate_size(mode, 0x1fff, 0x1fff);
 }
 
-static struct drm_encoder *mtk_hdmi_conn_best_enc(struct drm_connector *conn)
-{
-	struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
-
-	return hdmi->bridge.encoder;
-}
-
-static const struct drm_connector_funcs mtk_hdmi_connector_funcs = {
-	.detect = hdmi_conn_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = hdmi_conn_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static const struct drm_connector_helper_funcs
-		mtk_hdmi_connector_helper_funcs = {
-	.get_modes = mtk_hdmi_conn_get_modes,
-	.mode_valid = mtk_hdmi_conn_mode_valid,
-	.best_encoder = mtk_hdmi_conn_best_enc,
-};
-
 static void mtk_hdmi_hpd_event(bool hpd, struct device *dev)
 {
 	struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
 
-	if (hdmi && hdmi->bridge.encoder && hdmi->bridge.encoder->dev)
+	if (hdmi && hdmi->bridge.encoder && hdmi->bridge.encoder->dev) {
+		static enum drm_connector_status status;
+
+		status = mtk_hdmi_detect(hdmi);
 		drm_helper_hpd_irq_event(hdmi->bridge.encoder->dev);
+		drm_bridge_hpd_notify(&hdmi->bridge, status);
+	}
 }
 
 /*
  * Bridge callbacks
  */
 
+static enum drm_connector_status mtk_hdmi_bridge_detect(struct drm_bridge *bridge)
+{
+	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
+
+	return mtk_hdmi_detect(hdmi);
+}
+
+static struct edid *mtk_hdmi_bridge_get_edid(struct drm_bridge *bridge,
+					     struct drm_connector *connector)
+{
+	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
+
+	return mtk_hdmi_get_edid(hdmi, connector);
+}
+
 static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
 				  enum drm_bridge_attach_flags flags)
 {
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		DRM_ERROR("%s: The flag DRM_BRIDGE_ATTACH_NO_CONNECTOR must be supplied\n",
+			  __func__);
 		return -EINVAL;
 	}
 
-	ret = drm_connector_init_with_ddc(bridge->encoder->dev, &hdmi->conn,
-					  &mtk_hdmi_connector_funcs,
-					  DRM_MODE_CONNECTOR_HDMIA,
-					  hdmi->ddc_adpt);
-	if (ret) {
-		dev_err(hdmi->dev, "Failed to initialize connector: %d\n", ret);
-		return ret;
-	}
-	drm_connector_helper_add(&hdmi->conn, &mtk_hdmi_connector_helper_funcs);
-
-	hdmi->conn.polled = DRM_CONNECTOR_POLL_HPD;
-	hdmi->conn.interlace_allowed = true;
-	hdmi->conn.doublescan_allowed = false;
-
-	ret = drm_connector_attach_encoder(&hdmi->conn,
-						bridge->encoder);
-	if (ret) {
-		dev_err(hdmi->dev,
-			"Failed to attach connector to encoder: %d\n", ret);
-		return ret;
-	}
-
 	if (hdmi->next_bridge) {
 		ret = drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
 					bridge, flags);
@@ -1452,6 +1404,7 @@ static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
+	.mode_valid = mtk_hdmi_bridge_mode_valid,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
@@ -1462,6 +1415,8 @@ static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
 	.mode_set = mtk_hdmi_bridge_mode_set,
 	.atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
 	.atomic_enable = mtk_hdmi_bridge_atomic_enable,
+	.detect = mtk_hdmi_bridge_detect,
+	.get_edid = mtk_hdmi_bridge_get_edid,
 };
 
 static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
@@ -1772,6 +1727,9 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
 
 	hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs;
 	hdmi->bridge.of_node = pdev->dev.of_node;
+	hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
+			   | DRM_BRIDGE_OP_HPD;
+	hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
 	drm_bridge_add(&hdmi->bridge);
 
 	ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/mediatek: Switch the hdmi bridge ops to the atomic versions

[Enric Balletbo i Serra]

Hi Dafna,

Thank you for your patch. It'd be nice if you can cc the linux-mediatek ML for
next version, so Mediatek people is more aware of this change. IMHO cc'ing the
lkml is also a good practice.

On 24/3/21 20:12, Dafna Hirschfeld wrote:
> The bridge operation '.enable' and the audio cb '.get_eld'
> access hdmi->conn. In the future we will want to support
> the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
> not have direct access to the connector.
> The atomic version '.atomic_enable' allows accessing the
> current connector from the state.
> This patch switches the bridge to the atomic version and
> saves the current connector in a new field 'curr_conn'.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c | 41 ++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 8ee55f9e2954..9f415c508b33 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -154,6 +154,7 @@ struct mtk_hdmi {
>  	struct drm_bridge bridge;
>  	struct drm_bridge *next_bridge;
>  	struct drm_connector conn;
> +	struct drm_connector *curr_conn;

Adding a new drm_connector (curr_conn) is something that surprised me at the
beginning, then I saw that in the second patch you remove the first
drm_connector (conn). I didn't test it yet, but did you test this patch alone?
without the second one, maybe I'm missing something but looks to me that this
patch alone breaks more functionalities of the driver? (and I know that the
driver is broken right now)

Is really needed to create a new drm_connector to switch to the atomic versions?


>  	struct device *dev;
>  	const struct mtk_hdmi_conf *conf;
>  	struct phy *phy;
> @@ -974,7 +975,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
>  	ssize_t err;
>  
>  	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> -						       &hdmi->conn, mode);
> +						       hdmi->curr_conn, mode);
>  	if (err < 0) {
>  		dev_err(hdmi->dev,
>  			"Failed to get AVI infoframe from mode: %zd\n", err);
> @@ -1054,7 +1055,7 @@ static int mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
>  	ssize_t err;
>  
>  	err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
> -							  &hdmi->conn, mode);
> +							  hdmi->curr_conn, mode);
>  	if (err) {
>  		dev_err(hdmi->dev,
>  			"Failed to get vendor infoframe from mode: %zd\n", err);
> @@ -1357,7 +1358,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
>  	return true;
>  }
>  
> -static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *old_bridge_state)
>  {
>  	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>  
> @@ -1368,10 +1370,13 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
>  	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
>  	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
>  
> +	hdmi->curr_conn = NULL;
> +
>  	hdmi->enabled = false;
>  }
>  
> -static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *old_state)
>  {
>  	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>  
> @@ -1406,7 +1411,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>  	drm_mode_copy(&hdmi->mode, adjusted_mode);
>  }
>  
> -static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *old_state)
>  {
>  	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>  
> @@ -1426,10 +1432,16 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi *hdmi,
>  		mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
>  }
>  
> -static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *old_state)
>  {
> +	struct drm_atomic_state *state = old_state->base.state;
>  	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>  
> +	/* Retrieve the connector through the atomic state. */
> +	hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
> +								   bridge->encoder);
> +
>  	mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode);
>  	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
>  	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
> @@ -1440,13 +1452,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
>  }
>  
>  static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.attach = mtk_hdmi_bridge_attach,
>  	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
> -	.disable = mtk_hdmi_bridge_disable,
> -	.post_disable = mtk_hdmi_bridge_post_disable,
> +	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
> +	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
>  	.mode_set = mtk_hdmi_bridge_mode_set,
> -	.pre_enable = mtk_hdmi_bridge_pre_enable,
> -	.enable = mtk_hdmi_bridge_enable,
> +	.atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
> +	.atomic_enable = mtk_hdmi_bridge_atomic_enable,
>  };
>  
>  static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
> @@ -1662,8 +1677,10 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf,
>  {
>  	struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
>  
> -	memcpy(buf, hdmi->conn.eld, min(sizeof(hdmi->conn.eld), len));
> -
> +	if (hdmi->curr_conn)
> +		memcpy(buf, hdmi->curr_conn->eld, min(sizeof(hdmi->curr_conn->eld), len));
> +	else
> +		memset(buf, 0, len);
>  	return 0;
>  }
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/mediatek: Switch the hdmi bridge ops to the atomic versions

[Dafna Hirschfeld]

Hi,

On 25.03.21 11:43, Enric Balletbo i Serra wrote:
> Hi Dafna,
> 
> Thank you for your patch. It'd be nice if you can cc the linux-mediatek ML for
> next version, so Mediatek people is more aware of this change. IMHO cc'ing the
> lkml is also a good practice.

ok, I added all the mails from get_maintainer.pl. linux-mediatek was not there,
I can add it.

> 
> On 24/3/21 20:12, Dafna Hirschfeld wrote:
>> The bridge operation '.enable' and the audio cb '.get_eld'
>> access hdmi->conn. In the future we will want to support
>> the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
>> not have direct access to the connector.
>> The atomic version '.atomic_enable' allows accessing the
>> current connector from the state.
>> This patch switches the bridge to the atomic version and
>> saves the current connector in a new field 'curr_conn'.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_hdmi.c | 41 ++++++++++++++++++++---------
>>   1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
>> index 8ee55f9e2954..9f415c508b33 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
>> @@ -154,6 +154,7 @@ struct mtk_hdmi {
>>   	struct drm_bridge bridge;
>>   	struct drm_bridge *next_bridge;
>>   	struct drm_connector conn;
>> +	struct drm_connector *curr_conn;
> 
> Adding a new drm_connector (curr_conn) is something that surprised me at the
> beginning, then I saw that in the second patch you remove the first
> drm_connector (conn). I didn't test it yet, but did you test this patch alone?
> without the second one, maybe I'm missing something but looks to me that this
> patch alone breaks more functionalities of the driver? (and I know that the
> driver is broken right now)

Hi, I didn't test it alone, but indeed this patch was wirrten with the intention
that 'conn' will be removed next patch. But I don't think that patch should break
functionality.

> 
> Is really needed to create a new drm_connector to switch to the atomic versions?

No, indeed this is the wrong patch to add curr_conn, I should add curr_conn to the
second patch or maybe to a seperate last patch so not to overload the patch.

Thanks,
Dafna

> 
> 
>>   	struct device *dev;
>>   	const struct mtk_hdmi_conf *conf;
>>   	struct phy *phy;
>> @@ -974,7 +975,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
>>   	ssize_t err;
>>   
>>   	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
>> -						       &hdmi->conn, mode);
>> +						       hdmi->curr_conn, mode);
>>   	if (err < 0) {
>>   		dev_err(hdmi->dev,
>>   			"Failed to get AVI infoframe from mode: %zd\n", err);
>> @@ -1054,7 +1055,7 @@ static int mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
>>   	ssize_t err;
>>   
>>   	err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
>> -							  &hdmi->conn, mode);
>> +							  hdmi->curr_conn, mode);
>>   	if (err) {
>>   		dev_err(hdmi->dev,
>>   			"Failed to get vendor infoframe from mode: %zd\n", err);
>> @@ -1357,7 +1358,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
>>   	return true;
>>   }
>>   
>> -static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
>> +static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
>> +					   struct drm_bridge_state *old_bridge_state)
>>   {
>>   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>>   
>> @@ -1368,10 +1370,13 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
>>   	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
>>   	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
>>   
>> +	hdmi->curr_conn = NULL;
>> +
>>   	hdmi->enabled = false;
>>   }
>>   
>> -static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
>> +static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>> +						struct drm_bridge_state *old_state)
>>   {
>>   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>>   
>> @@ -1406,7 +1411,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>>   	drm_mode_copy(&hdmi->mode, adjusted_mode);
>>   }
>>   
>> -static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
>> +static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>> +					      struct drm_bridge_state *old_state)
>>   {
>>   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>>   
>> @@ -1426,10 +1432,16 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi *hdmi,
>>   		mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
>>   }
>>   
>> -static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
>> +static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
>> +					  struct drm_bridge_state *old_state)
>>   {
>> +	struct drm_atomic_state *state = old_state->base.state;
>>   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>>   
>> +	/* Retrieve the connector through the atomic state. */
>> +	hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
>> +								   bridge->encoder);
>> +
>>   	mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode);
>>   	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
>>   	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
>> @@ -1440,13 +1452,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
>>   }
>>   
>>   static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
>> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>>   	.attach = mtk_hdmi_bridge_attach,
>>   	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
>> -	.disable = mtk_hdmi_bridge_disable,
>> -	.post_disable = mtk_hdmi_bridge_post_disable,
>> +	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
>> +	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
>>   	.mode_set = mtk_hdmi_bridge_mode_set,
>> -	.pre_enable = mtk_hdmi_bridge_pre_enable,
>> -	.enable = mtk_hdmi_bridge_enable,
>> +	.atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
>> +	.atomic_enable = mtk_hdmi_bridge_atomic_enable,
>>   };
>>   
>>   static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
>> @@ -1662,8 +1677,10 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf,
>>   {
>>   	struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
>>   
>> -	memcpy(buf, hdmi->conn.eld, min(sizeof(hdmi->conn.eld), len));
>> -
>> +	if (hdmi->curr_conn)
>> +		memcpy(buf, hdmi->curr_conn->eld, min(sizeof(hdmi->curr_conn->eld), len));
>> +	else
>> +		memset(buf, 0, len);
>>   	return 0;
>>   }
>>   
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel