diff mbox series

[1/2] drm: bridge: dw-hdmi: Add hooks for suspend/resume

Message ID 20190502223808.185180-1-dianders@chromium.org (mailing list archive)
State New, archived
Headers show
Series [1/2] drm: bridge: dw-hdmi: Add hooks for suspend/resume | expand

Commit Message

Doug Anderson May 2, 2019, 10:38 p.m. UTC
On Rockchip rk3288-based Chromebooks when you do a suspend/resume
cycle:

1. You lose the ability to detect an HDMI device being plugged in.

2. If you're using the i2c bus built in to dw_hdmi then it stops
working.

Let's add a hook to the core dw-hdmi driver so that we can call it in
dw_hdmi-rockchip in the next commit.

NOTE: the exact set of steps I've done here in resume come from
looking at the normal dw_hdmi init sequence in upstream Linux plus the
sequence that we did in downstream Chrome OS 3.14.  Testing show that
it seems to work, but if an extra step is needed or something here is
not needed we could improve it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h              |  3 +++
 2 files changed, 24 insertions(+)

Comments

Doug Anderson May 15, 2019, 4:22 p.m. UTC | #1
Hi

From: Douglas Anderson <dianders@chromium.org>
Date: Thu, May 2, 2019 at 3:38 PM
To: Heiko Stuebner, Sandy Huang, Andrzej Hajda, Laurent Pinchart
Cc: <linux-rockchip@lists.infradead.org>, Neil Armstrong,
<mka@chromium.org>, Sean Paul, Douglas Anderson, Zheng Yang, Sam
Ravnborg, <dri-devel@lists.freedesktop.org>,
<linux-kernel@vger.kernel.org>, Ville Syrjälä, David Airlie, Jernej
Skrabec, Daniel Vetter

> On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> cycle:
>
> 1. You lose the ability to detect an HDMI device being plugged in.
>
> 2. If you're using the i2c bus built in to dw_hdmi then it stops
> working.
>
> Let's add a hook to the core dw-hdmi driver so that we can call it in
> dw_hdmi-rockchip in the next commit.
>
> NOTE: the exact set of steps I've done here in resume come from
> looking at the normal dw_hdmi init sequence in upstream Linux plus the
> sequence that we did in downstream Chrome OS 3.14.  Testing show that
> it seems to work, but if an extra step is needed or something here is
> not needed we could improve it.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
>  include/drm/bridge/dw_hdmi.h              |  3 +++
>  2 files changed, 24 insertions(+)

Did anyone have any thoughts on this patch series?  Thanks!  :-)

-Doug
Sean Paul May 15, 2019, 5:58 p.m. UTC | #2
On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote:
> On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> cycle:
> 
> 1. You lose the ability to detect an HDMI device being plugged in.
> 
> 2. If you're using the i2c bus built in to dw_hdmi then it stops
> working.
> 
> Let's add a hook to the core dw-hdmi driver so that we can call it in
> dw_hdmi-rockchip in the next commit.
> 
> NOTE: the exact set of steps I've done here in resume come from
> looking at the normal dw_hdmi init sequence in upstream Linux plus the
> sequence that we did in downstream Chrome OS 3.14.  Testing show that
> it seems to work, but if an extra step is needed or something here is
> not needed we could improve it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
>  include/drm/bridge/dw_hdmi.h              |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index db761329a1e3..4b38bfd43e59 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>  
> +int dw_hdmi_suspend(struct dw_hdmi *hdmi)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
> +
> +int dw_hdmi_resume(struct dw_hdmi *hdmi)
> +{
> +	initialize_hdmi_ih_mutes(hdmi);
> +
> +	dw_hdmi_setup_i2c(hdmi);
> +	if (hdmi->i2c)
> +		dw_hdmi_i2c_init(hdmi);
> +
> +	if (hdmi->phy.ops->setup_hpd)
> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_resume);

Both patches look good to me, I'd probably prefer to just smash them together,
but meh.

If no one more authoritative chimes in, I'll apply them to -misc in a few days.

Sean

> +
>  MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
>  MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
>  MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 66e70770cce5..c4132e9a5ae3 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -154,6 +154,9 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
>  			     struct drm_encoder *encoder,
>  			     const struct dw_hdmi_plat_data *plat_data);
>  
> +int dw_hdmi_suspend(struct dw_hdmi *hdmi);
> +int dw_hdmi_resume(struct dw_hdmi *hdmi);
> +
>  void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
>  
>  void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
> -- 
> 2.21.0.1020.gf2820cf01a-goog
>
Doug Anderson May 15, 2019, 6:01 p.m. UTC | #3
Hi,

On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote:

> On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote:
> > On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> > cycle:
> >
> > 1. You lose the ability to detect an HDMI device being plugged in.
> >
> > 2. If you're using the i2c bus built in to dw_hdmi then it stops
> > working.
> >
> > Let's add a hook to the core dw-hdmi driver so that we can call it in
> > dw_hdmi-rockchip in the next commit.
> >
> > NOTE: the exact set of steps I've done here in resume come from
> > looking at the normal dw_hdmi init sequence in upstream Linux plus the
> > sequence that we did in downstream Chrome OS 3.14.  Testing show that
> > it seems to work, but if an extra step is needed or something here is
> > not needed we could improve it.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
> >  include/drm/bridge/dw_hdmi.h              |  3 +++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index db761329a1e3..4b38bfd43e59 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
> >  }
> >  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
> >
> > +int dw_hdmi_suspend(struct dw_hdmi *hdmi)
> > +{
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
> > +
> > +int dw_hdmi_resume(struct dw_hdmi *hdmi)
> > +{
> > +     initialize_hdmi_ih_mutes(hdmi);
> > +
> > +     dw_hdmi_setup_i2c(hdmi);
> > +     if (hdmi->i2c)
> > +             dw_hdmi_i2c_init(hdmi);
> > +
> > +     if (hdmi->phy.ops->setup_hpd)
> > +             hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dw_hdmi_resume);
>
> Both patches look good to me, I'd probably prefer to just smash them together,
> but meh.
>
> If no one more authoritative chimes in, I'll apply them to -misc in a few days.

Sure.  I can smash them and re-post or you can smash them for me or we
can keep them as-is.  I originally separated because I wasn't sure if
they'd eventually go through different trees.  Just let me know!  :-)

-Doug
Sean Paul May 15, 2019, 6:05 p.m. UTC | #4
On Wed, May 15, 2019 at 11:01:26AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote:
> 
> > On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote:
> > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> > > cycle:
> > >
> > > 1. You lose the ability to detect an HDMI device being plugged in.
> > >
> > > 2. If you're using the i2c bus built in to dw_hdmi then it stops
> > > working.
> > >
> > > Let's add a hook to the core dw-hdmi driver so that we can call it in
> > > dw_hdmi-rockchip in the next commit.
> > >
> > > NOTE: the exact set of steps I've done here in resume come from
> > > looking at the normal dw_hdmi init sequence in upstream Linux plus the
> > > sequence that we did in downstream Chrome OS 3.14.  Testing show that
> > > it seems to work, but if an extra step is needed or something here is
> > > not needed we could improve it.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
> > >  include/drm/bridge/dw_hdmi.h              |  3 +++
> > >  2 files changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > index db761329a1e3..4b38bfd43e59 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
> > >
> > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi)
> > > +{
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
> > > +
> > > +int dw_hdmi_resume(struct dw_hdmi *hdmi)
> > > +{
> > > +     initialize_hdmi_ih_mutes(hdmi);
> > > +
> > > +     dw_hdmi_setup_i2c(hdmi);
> > > +     if (hdmi->i2c)
> > > +             dw_hdmi_i2c_init(hdmi);
> > > +
> > > +     if (hdmi->phy.ops->setup_hpd)
> > > +             hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dw_hdmi_resume);
> >
> > Both patches look good to me, I'd probably prefer to just smash them together,
> > but meh.
> >
> > If no one more authoritative chimes in, I'll apply them to -misc in a few days.
> 
> Sure.  I can smash them and re-post or you can smash them for me or we
> can keep them as-is.  I originally separated because I wasn't sure if
> they'd eventually go through different trees.  Just let me know!  :-)

Definitely no need to repost. It's entirely possible Andrzej or Heiko prefer to
have the dw-hdmi stuff broken out anyways. My opinion is of little value here :)

Sean

> 
> -Doug
Heiko Stuebner May 15, 2019, 8:03 p.m. UTC | #5
Am Mittwoch, 15. Mai 2019, 20:05:03 CEST schrieb Sean Paul:
> On Wed, May 15, 2019 at 11:01:26AM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote:
> > 
> > > On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote:
> > > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> > > > cycle:
> > > >
> > > > 1. You lose the ability to detect an HDMI device being plugged in.
> > > >
> > > > 2. If you're using the i2c bus built in to dw_hdmi then it stops
> > > > working.
> > > >
> > > > Let's add a hook to the core dw-hdmi driver so that we can call it in
> > > > dw_hdmi-rockchip in the next commit.
> > > >
> > > > NOTE: the exact set of steps I've done here in resume come from
> > > > looking at the normal dw_hdmi init sequence in upstream Linux plus the
> > > > sequence that we did in downstream Chrome OS 3.14.  Testing show that
> > > > it seems to work, but if an extra step is needed or something here is
> > > > not needed we could improve it.
> > > >
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
> > > >  include/drm/bridge/dw_hdmi.h              |  3 +++
> > > >  2 files changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > > index db761329a1e3..4b38bfd43e59 100644
> > > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
> > > >
> > > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi)
> > > > +{
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
> > > > +
> > > > +int dw_hdmi_resume(struct dw_hdmi *hdmi)
> > > > +{
> > > > +     initialize_hdmi_ih_mutes(hdmi);
> > > > +
> > > > +     dw_hdmi_setup_i2c(hdmi);
> > > > +     if (hdmi->i2c)
> > > > +             dw_hdmi_i2c_init(hdmi);
> > > > +
> > > > +     if (hdmi->phy.ops->setup_hpd)
> > > > +             hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dw_hdmi_resume);
> > >
> > > Both patches look good to me, I'd probably prefer to just smash them together,
> > > but meh.
> > >
> > > If no one more authoritative chimes in, I'll apply them to -misc in a few days.
> > 
> > Sure.  I can smash them and re-post or you can smash them for me or we
> > can keep them as-is.  I originally separated because I wasn't sure if
> > they'd eventually go through different trees.  Just let me know!  :-)
> 
> Definitely no need to repost. It's entirely possible Andrzej or Heiko prefer to
> have the dw-hdmi stuff broken out anyways. My opinion is of little value here :)

I guess my own preference is to keep them as they are now - so separate.
It makes it easier to see what gets done and also keeps the boundary on
where to split pretty clear.


Heiko
Laurent Pinchart May 16, 2019, 10:18 a.m. UTC | #6
Hi Douglas,

Thank you for the patch.

On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote:
> On Rockchip rk3288-based Chromebooks when you do a suspend/resume
> cycle:
> 
> 1. You lose the ability to detect an HDMI device being plugged in.
> 
> 2. If you're using the i2c bus built in to dw_hdmi then it stops
> working.
> 
> Let's add a hook to the core dw-hdmi driver so that we can call it in
> dw_hdmi-rockchip in the next commit.
> 
> NOTE: the exact set of steps I've done here in resume come from
> looking at the normal dw_hdmi init sequence in upstream Linux plus the
> sequence that we did in downstream Chrome OS 3.14.  Testing show that
> it seems to work, but if an extra step is needed or something here is
> not needed we could improve it.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++
>  include/drm/bridge/dw_hdmi.h              |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index db761329a1e3..4b38bfd43e59 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>  
> +int dw_hdmi_suspend(struct dw_hdmi *hdmi)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
> +

As this is empty, should we leave it out ? It adds a bit of bloat to the
kernel for no real reason, and we can add it later if required.

> +int dw_hdmi_resume(struct dw_hdmi *hdmi)
> +{
> +	initialize_hdmi_ih_mutes(hdmi);
> +
> +	dw_hdmi_setup_i2c(hdmi);
> +	if (hdmi->i2c)
> +		dw_hdmi_i2c_init(hdmi);
> +
> +	if (hdmi->phy.ops->setup_hpd)
> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> +
> +	return 0;

How about refactoring the probe function to extract hardware
initialisation to a separate function, and calling it from here ?

> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_resume);
> +
>  MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
>  MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
>  MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 66e70770cce5..c4132e9a5ae3 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -154,6 +154,9 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
>  			     struct drm_encoder *encoder,
>  			     const struct dw_hdmi_plat_data *plat_data);
>  
> +int dw_hdmi_suspend(struct dw_hdmi *hdmi);
> +int dw_hdmi_resume(struct dw_hdmi *hdmi);
> +
>  void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
>  
>  void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index db761329a1e3..4b38bfd43e59 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2780,6 +2780,27 @@  void dw_hdmi_unbind(struct dw_hdmi *hdmi)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
+int dw_hdmi_suspend(struct dw_hdmi *hdmi)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
+
+int dw_hdmi_resume(struct dw_hdmi *hdmi)
+{
+	initialize_hdmi_ih_mutes(hdmi);
+
+	dw_hdmi_setup_i2c(hdmi);
+	if (hdmi->i2c)
+		dw_hdmi_i2c_init(hdmi);
+
+	if (hdmi->phy.ops->setup_hpd)
+		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_resume);
+
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
 MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
 MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 66e70770cce5..c4132e9a5ae3 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -154,6 +154,9 @@  struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
 			     struct drm_encoder *encoder,
 			     const struct dw_hdmi_plat_data *plat_data);
 
+int dw_hdmi_suspend(struct dw_hdmi *hdmi);
+int dw_hdmi_resume(struct dw_hdmi *hdmi);
+
 void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);