| Message ID | 20240522-fd-hdmi-hpd-v2-13-c30bdb7c5c7e@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/hdmi: rework and fix the HPD even generation | expand |
… > +++ 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
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.
>> 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
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.
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 >
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); }
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(-)