[v2] drm/msm/hdmi: rework and fix the HPD even generation

[PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Dmitry Baryshkov 1 year, 8 months ago

The HDMI block needs to be enabled to properly generate HPD events. Make
sure it is not turned off in the disable paths if HPD delivery is enabled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi.h        | 2 ++
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 8 +++++++-
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 9 ++++++++-
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index a9437054c015..2890196857f8 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -409,6 +409,7 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	hdmi->pdev = pdev;
 	hdmi->config = config;
 	spin_lock_init(&hdmi->reg_lock);
+	mutex_init(&hdmi->state_mutex);
 
 	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
 	if (ret && ret != -ENODEV)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 268ff8604423..7f0ca5252018 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -42,6 +42,8 @@ struct hdmi {
 
 	/* video state: */
 	bool power_on;
+	bool hpd_enabled;
+	struct mutex state_mutex; /* protects two booleans */
 	unsigned long int pixclock;
 
 	void __iomem *mmio;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index cddba640d292..104107ed47d0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 
 	DBG("power up");
 
+	mutex_lock(&hdmi->state_mutex);
 	if (!hdmi->power_on) {
 		msm_hdmi_phy_resource_enable(phy);
 		msm_hdmi_power_on(bridge);
 		hdmi->power_on = true;
 	}
+	mutex_unlock(&hdmi->state_mutex);
 
 	if (hdmi->hdmi_mode) {
 		msm_hdmi_config_avi_infoframe(hdmi);
@@ -147,7 +149,10 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 		msm_hdmi_hdcp_off(hdmi->hdcp_ctrl);
 
 	DBG("power down");
-	msm_hdmi_set_mode(hdmi, false);
+
+	/* Keep the HDMI enabled if the HPD is enabled */
+	mutex_lock(&hdmi->state_mutex);
+	msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
 
 	msm_hdmi_phy_powerdown(phy);
 
@@ -158,6 +163,7 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 			msm_hdmi_audio_update(hdmi);
 		msm_hdmi_phy_resource_disable(phy);
 	}
+	mutex_unlock(&hdmi->state_mutex);
 }
 
 static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index d3353c6148ed..cb89e9e2c6ea 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -73,10 +73,14 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	mutex_lock(&hdmi->state_mutex);
 	msm_hdmi_set_mode(hdmi, false);
 	msm_hdmi_phy_reset(hdmi);
 	msm_hdmi_set_mode(hdmi, true);
 
+	hdmi->hpd_enabled = true;
+	mutex_unlock(&hdmi->state_mutex);
+
 	hdmi_write(hdmi, REG_HDMI_USEC_REFTIMER, 0x0001001b);
 
 	/* enable HPD events: */
@@ -106,7 +110,10 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
 	/* Disable HPD interrupt */
 	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
 
-	msm_hdmi_set_mode(hdmi, false);
+	mutex_lock(&hdmi->state_mutex);
+	hdmi->hpd_enabled = false;
+	msm_hdmi_set_mode(hdmi, hdmi->power_on);
+	mutex_unlock(&hdmi->state_mutex);
 
 	pm_runtime_put(dev);
 }

-- 
2.39.2

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Jessica Zhang 1 year, 7 months ago


On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> The HDMI block needs to be enabled to properly generate HPD events. Make
> sure it is not turned off in the disable paths if HPD delivery is enabled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 1 +
>   drivers/gpu/drm/msm/hdmi/hdmi.h        | 2 ++
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 8 +++++++-
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 9 ++++++++-
>   4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index a9437054c015..2890196857f8 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -409,6 +409,7 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	hdmi->pdev = pdev;
>   	hdmi->config = config;
>   	spin_lock_init(&hdmi->reg_lock);
> +	mutex_init(&hdmi->state_mutex);
>   
>   	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
>   	if (ret && ret != -ENODEV)
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 268ff8604423..7f0ca5252018 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -42,6 +42,8 @@ struct hdmi {
>   
>   	/* video state: */
>   	bool power_on;
> +	bool hpd_enabled;
> +	struct mutex state_mutex; /* protects two booleans */
>   	unsigned long int pixclock;
>   
>   	void __iomem *mmio;
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index cddba640d292..104107ed47d0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>   
>   	DBG("power up");
>   
> +	mutex_lock(&hdmi->state_mutex);
>   	if (!hdmi->power_on) {
>   		msm_hdmi_phy_resource_enable(phy);
>   		msm_hdmi_power_on(bridge);
>   		hdmi->power_on = true;
>   	}
> +	mutex_unlock(&hdmi->state_mutex);
>   
>   	if (hdmi->hdmi_mode) {
>   		msm_hdmi_config_avi_infoframe(hdmi);
> @@ -147,7 +149,10 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>   		msm_hdmi_hdcp_off(hdmi->hdcp_ctrl);
>   
>   	DBG("power down");
> -	msm_hdmi_set_mode(hdmi, false);
> +
> +	/* Keep the HDMI enabled if the HPD is enabled */
> +	mutex_lock(&hdmi->state_mutex);
> +	msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
>   
>   	msm_hdmi_phy_powerdown(phy);
>   
> @@ -158,6 +163,7 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>   			msm_hdmi_audio_update(hdmi);
>   		msm_hdmi_phy_resource_disable(phy);
>   	}
> +	mutex_unlock(&hdmi->state_mutex);
>   }
>   
>   static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index d3353c6148ed..cb89e9e2c6ea 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -73,10 +73,14 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	if (ret)
>   		return ret;
>   
> +	mutex_lock(&hdmi->state_mutex);
>   	msm_hdmi_set_mode(hdmi, false);
>   	msm_hdmi_phy_reset(hdmi);
>   	msm_hdmi_set_mode(hdmi, true);
>   
> +	hdmi->hpd_enabled = true;
> +	mutex_unlock(&hdmi->state_mutex);
> +
>   	hdmi_write(hdmi, REG_HDMI_USEC_REFTIMER, 0x0001001b);
>   
>   	/* enable HPD events: */
> @@ -106,7 +110,10 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>   	/* Disable HPD interrupt */
>   	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
>   
> -	msm_hdmi_set_mode(hdmi, false);
> +	mutex_lock(&hdmi->state_mutex);
> +	hdmi->hpd_enabled = false;
> +	msm_hdmi_set_mode(hdmi, hdmi->power_on);
> +	mutex_unlock(&hdmi->state_mutex);
>   
>   	pm_runtime_put(dev);
>   }
> 
> -- 
> 2.39.2
>

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Markus Elfring 1 year, 8 months ago

…
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>
>  	DBG("power up");
>
> +	mutex_lock(&hdmi->state_mutex);
>  	if (!hdmi->power_on) {
…
>  	}
> +	mutex_unlock(&hdmi->state_mutex);
>
>  	if (hdmi->hdmi_mode) {
>  		msm_hdmi_config_avi_infoframe(hdmi);
…

Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196

Regards,
Markus

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Dmitry Baryshkov 1 year, 8 months ago

On Wed, 12 Jun 2024 at 16:01, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> >
> >       DBG("power up");
> >
> > +     mutex_lock(&hdmi->state_mutex);
> >       if (!hdmi->power_on) {
> …
> >       }
> > +     mutex_unlock(&hdmi->state_mutex);
> >
> >       if (hdmi->hdmi_mode) {
> >               msm_hdmi_config_avi_infoframe(hdmi);
> …
>
> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196

I am not.


-- 
With best wishes
Dmitry

Re: [v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Markus Elfring 1 year, 8 months ago

>> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
>> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
>
> I am not.

Under which circumstances will development interests grow for scope-based resource management?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#L124

Regards,
Markus

Re: [v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

Posted by Dmitry Baryshkov 1 year, 8 months ago

On Wed, 12 Jun 2024 at 17:32, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
> >> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
> >
> > I am not.
>
> Under which circumstances will development interests grow for scope-based resource management?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#L124

I consider guard() and free() to be counterintuitive, harder to follow
and semantically troublesome.

-- 
With best wishes
Dmitry