[PATCH v3 1/2] drm: bridge: dw-hdmi: Add hook for resume

[PATCH v3 1/2] drm: bridge: dw-hdmi: Add hook for resume

[Douglas Anderson]

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.

As part of this change we'll refactor the hardware init bits of
dw-hdmi to happen all in one function and all at the same time.  Since
we need to init the interrupt mutes before we request the IRQ, this
means moving the hardware init earlier in the function, but there
should be no problems with that.  Also as part of this we now
unconditionally init the "i2c" parts of dw-hdmi, but again that ought
to be fine.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Changes in v3:
- Change resume to void function (Laurent)

Changes in v2:
- No empty stub for suspend (Laurent)
- Refactor to use the same code in probe and resume (Laurent)
- Unconditionally init i2c (seems OK + needed before hdmi->i2c init)
- Combine "init" of i2c and "setup" of i2c (no reason to split)

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 48 ++++++++++++++---------
 include/drm/bridge/dw_hdmi.h              |  2 +
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 66bd66bad44c..a00ccf123877 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -228,6 +228,13 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
 
 static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
 {
+	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
+		    HDMI_PHY_I2CM_INT_ADDR);
+
+	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
+		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
+		    HDMI_PHY_I2CM_CTLINT_ADDR);
+
 	/* Software reset */
 	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
 
@@ -1926,16 +1933,6 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	return 0;
 }
 
-static void dw_hdmi_setup_i2c(struct dw_hdmi *hdmi)
-{
-	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
-		    HDMI_PHY_I2CM_INT_ADDR);
-
-	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
-		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
-		    HDMI_PHY_I2CM_CTLINT_ADDR);
-}
-
 static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
 {
 	u8 ih_mute;
@@ -2436,6 +2433,21 @@ static const struct regmap_config hdmi_regmap_32bit_config = {
 	.max_register	= HDMI_I2CM_FS_SCL_LCNT_0_ADDR << 2,
 };
 
+static void dw_hdmi_init_hw(struct dw_hdmi *hdmi)
+{
+	initialize_hdmi_ih_mutes(hdmi);
+
+	/*
+	 * Reset HDMI DDC I2C master controller and mute I2CM interrupts.
+	 * Even if we are using a separate i2c adapter doing this doesn't
+	 * hurt.
+	 */
+	dw_hdmi_i2c_init(hdmi);
+
+	if (hdmi->phy.ops->setup_hpd)
+		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
+}
+
 static struct dw_hdmi *
 __dw_hdmi_probe(struct platform_device *pdev,
 		const struct dw_hdmi_plat_data *plat_data)
@@ -2587,7 +2599,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		 prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
 		 hdmi->phy.name);
 
-	initialize_hdmi_ih_mutes(hdmi);
+	dw_hdmi_init_hw(hdmi);
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -2626,10 +2638,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 	hdmi->bridge.of_node = pdev->dev.of_node;
 #endif
 
-	dw_hdmi_setup_i2c(hdmi);
-	if (hdmi->phy.ops->setup_hpd)
-		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
-
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;
 	pdevinfo.id = PLATFORM_DEVID_AUTO;
@@ -2682,10 +2690,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
 		hdmi->cec = platform_device_register_full(&pdevinfo);
 	}
 
-	/* Reset HDMI DDC I2C master controller and mute I2CM interrupts */
-	if (hdmi->i2c)
-		dw_hdmi_i2c_init(hdmi);
-
 	return hdmi;
 
 err_iahb:
@@ -2789,6 +2793,12 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
+void dw_hdmi_resume(struct dw_hdmi *hdmi)
+{
+	dw_hdmi_init_hw(hdmi);
+}
+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..601243b56b69 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -154,6 +154,8 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
 			     struct drm_encoder *encoder,
 			     const struct dw_hdmi_plat_data *plat_data);
 
+void 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.22.0.rc1.311.g5d7573a151-goog

[PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Douglas Anderson]

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 call the core dw-hdmi's suspend/resume functions to restore
things.

NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
"late/early" versions of suspend/resume because we found that the VOP
was sometimes resuming before dw_hdmi and then calling into us before
we were fully resumed.  For now I have gone back to the normal
suspend/resume because I can't reproduce the problems.

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

Changes in v3:
- dw_hdmi_resume() is now a void function (Laurent)

Changes in v2:
- Add forgotten static (Laurent)
- No empty stub for suspend (Laurent)

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 4cdc9f86c2e5..7bb0f922b303 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
+{
+	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_resume(hdmi->hdmi);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_hdmi_rockchip_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume)
+};
+
 struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
 	.probe  = dw_hdmi_rockchip_probe,
 	.remove = dw_hdmi_rockchip_remove,
 	.driver = {
 		.name = "dwhdmi-rockchip",
+		.pm = &dw_hdmi_rockchip_pm,
 		.of_match_table = dw_hdmi_rockchip_dt_ids,
 	},
 };
-- 
2.22.0.rc1.311.g5d7573a151-goog

[PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Douglas Anderson]

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 call the core dw-hdmi's suspend/resume functions to restore
things.

NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
"late/early" versions of suspend/resume because we found that the VOP
was sometimes resuming before dw_hdmi and then calling into us before
we were fully resumed.  For now I have gone back to the normal
suspend/resume because I can't reproduce the problems.

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

Changes in v3:
- dw_hdmi_resume() is now a void function (Laurent)

Changes in v2:
- Add forgotten static (Laurent)
- No empty stub for suspend (Laurent)

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 4cdc9f86c2e5..7bb0f922b303 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
+{
+	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_resume(hdmi->hdmi);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_hdmi_rockchip_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume)
+};
+
 struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
 	.probe  = dw_hdmi_rockchip_probe,
 	.remove = dw_hdmi_rockchip_remove,
 	.driver = {
 		.name = "dwhdmi-rockchip",
+		.pm = &dw_hdmi_rockchip_pm,
 		.of_match_table = dw_hdmi_rockchip_dt_ids,
 	},
 };
-- 
2.22.0.rc1.311.g5d7573a151-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] drm: bridge: dw-hdmi: Add hook for resume

[Sean Paul]

On Tue, Jun 04, 2019 at 01:42:06PM -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.
> 
> As part of this change we'll refactor the hardware init bits of
> dw-hdmi to happen all in one function and all at the same time.  Since
> we need to init the interrupt mutes before we request the IRQ, this
> means moving the hardware init earlier in the function, but there
> should be no problems with that.  Also as part of this we now
> unconditionally init the "i2c" parts of dw-hdmi, but again that ought
> to be fine.
> 

Reviewed-by: Sean Paul <sean@poorly.run>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> Changes in v3:
> - Change resume to void function (Laurent)
> 
> Changes in v2:
> - No empty stub for suspend (Laurent)
> - Refactor to use the same code in probe and resume (Laurent)
> - Unconditionally init i2c (seems OK + needed before hdmi->i2c init)
> - Combine "init" of i2c and "setup" of i2c (no reason to split)
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 48 ++++++++++++++---------
>  include/drm/bridge/dw_hdmi.h              |  2 +
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 66bd66bad44c..a00ccf123877 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -228,6 +228,13 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>  
>  static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
>  {
> +	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
> +		    HDMI_PHY_I2CM_INT_ADDR);
> +
> +	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
> +		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
> +		    HDMI_PHY_I2CM_CTLINT_ADDR);
> +
>  	/* Software reset */
>  	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
>  
> @@ -1926,16 +1933,6 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	return 0;
>  }
>  
> -static void dw_hdmi_setup_i2c(struct dw_hdmi *hdmi)
> -{
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
> -		    HDMI_PHY_I2CM_INT_ADDR);
> -
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
> -		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
> -		    HDMI_PHY_I2CM_CTLINT_ADDR);
> -}
> -
>  static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
>  {
>  	u8 ih_mute;
> @@ -2436,6 +2433,21 @@ static const struct regmap_config hdmi_regmap_32bit_config = {
>  	.max_register	= HDMI_I2CM_FS_SCL_LCNT_0_ADDR << 2,
>  };
>  
> +static void dw_hdmi_init_hw(struct dw_hdmi *hdmi)
> +{
> +	initialize_hdmi_ih_mutes(hdmi);
> +
> +	/*
> +	 * Reset HDMI DDC I2C master controller and mute I2CM interrupts.
> +	 * Even if we are using a separate i2c adapter doing this doesn't
> +	 * hurt.
> +	 */
> +	dw_hdmi_i2c_init(hdmi);
> +
> +	if (hdmi->phy.ops->setup_hpd)
> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> +}
> +
>  static struct dw_hdmi *
>  __dw_hdmi_probe(struct platform_device *pdev,
>  		const struct dw_hdmi_plat_data *plat_data)
> @@ -2587,7 +2599,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  		 prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
>  		 hdmi->phy.name);
>  
> -	initialize_hdmi_ih_mutes(hdmi);
> +	dw_hdmi_init_hw(hdmi);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> @@ -2626,10 +2638,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  	hdmi->bridge.of_node = pdev->dev.of_node;
>  #endif
>  
> -	dw_hdmi_setup_i2c(hdmi);
> -	if (hdmi->phy.ops->setup_hpd)
> -		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> -
>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>  	pdevinfo.parent = dev;
>  	pdevinfo.id = PLATFORM_DEVID_AUTO;
> @@ -2682,10 +2690,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  		hdmi->cec = platform_device_register_full(&pdevinfo);
>  	}
>  
> -	/* Reset HDMI DDC I2C master controller and mute I2CM interrupts */
> -	if (hdmi->i2c)
> -		dw_hdmi_i2c_init(hdmi);
> -
>  	return hdmi;
>  
>  err_iahb:
> @@ -2789,6 +2793,12 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>  
> +void dw_hdmi_resume(struct dw_hdmi *hdmi)
> +{
> +	dw_hdmi_init_hw(hdmi);
> +}
> +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..601243b56b69 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -154,6 +154,8 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
>  			     struct drm_encoder *encoder,
>  			     const struct dw_hdmi_plat_data *plat_data);
>  
> +void 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.22.0.rc1.311.g5d7573a151-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 1/2] drm: bridge: dw-hdmi: Add hook for resume

[Sean Paul]

On Tue, Jun 04, 2019 at 01:42:06PM -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.
> 
> As part of this change we'll refactor the hardware init bits of
> dw-hdmi to happen all in one function and all at the same time.  Since
> we need to init the interrupt mutes before we request the IRQ, this
> means moving the hardware init earlier in the function, but there
> should be no problems with that.  Also as part of this we now
> unconditionally init the "i2c" parts of dw-hdmi, but again that ought
> to be fine.
> 

Reviewed-by: Sean Paul <sean@poorly.run>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> Changes in v3:
> - Change resume to void function (Laurent)
> 
> Changes in v2:
> - No empty stub for suspend (Laurent)
> - Refactor to use the same code in probe and resume (Laurent)
> - Unconditionally init i2c (seems OK + needed before hdmi->i2c init)
> - Combine "init" of i2c and "setup" of i2c (no reason to split)
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 48 ++++++++++++++---------
>  include/drm/bridge/dw_hdmi.h              |  2 +
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 66bd66bad44c..a00ccf123877 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -228,6 +228,13 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>  
>  static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
>  {
> +	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
> +		    HDMI_PHY_I2CM_INT_ADDR);
> +
> +	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
> +		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
> +		    HDMI_PHY_I2CM_CTLINT_ADDR);
> +
>  	/* Software reset */
>  	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
>  
> @@ -1926,16 +1933,6 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	return 0;
>  }
>  
> -static void dw_hdmi_setup_i2c(struct dw_hdmi *hdmi)
> -{
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
> -		    HDMI_PHY_I2CM_INT_ADDR);
> -
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL |
> -		    HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL,
> -		    HDMI_PHY_I2CM_CTLINT_ADDR);
> -}
> -
>  static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
>  {
>  	u8 ih_mute;
> @@ -2436,6 +2433,21 @@ static const struct regmap_config hdmi_regmap_32bit_config = {
>  	.max_register	= HDMI_I2CM_FS_SCL_LCNT_0_ADDR << 2,
>  };
>  
> +static void dw_hdmi_init_hw(struct dw_hdmi *hdmi)
> +{
> +	initialize_hdmi_ih_mutes(hdmi);
> +
> +	/*
> +	 * Reset HDMI DDC I2C master controller and mute I2CM interrupts.
> +	 * Even if we are using a separate i2c adapter doing this doesn't
> +	 * hurt.
> +	 */
> +	dw_hdmi_i2c_init(hdmi);
> +
> +	if (hdmi->phy.ops->setup_hpd)
> +		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> +}
> +
>  static struct dw_hdmi *
>  __dw_hdmi_probe(struct platform_device *pdev,
>  		const struct dw_hdmi_plat_data *plat_data)
> @@ -2587,7 +2599,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  		 prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
>  		 hdmi->phy.name);
>  
> -	initialize_hdmi_ih_mutes(hdmi);
> +	dw_hdmi_init_hw(hdmi);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> @@ -2626,10 +2638,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  	hdmi->bridge.of_node = pdev->dev.of_node;
>  #endif
>  
> -	dw_hdmi_setup_i2c(hdmi);
> -	if (hdmi->phy.ops->setup_hpd)
> -		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
> -
>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>  	pdevinfo.parent = dev;
>  	pdevinfo.id = PLATFORM_DEVID_AUTO;
> @@ -2682,10 +2690,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  		hdmi->cec = platform_device_register_full(&pdevinfo);
>  	}
>  
> -	/* Reset HDMI DDC I2C master controller and mute I2CM interrupts */
> -	if (hdmi->i2c)
> -		dw_hdmi_i2c_init(hdmi);
> -
>  	return hdmi;
>  
>  err_iahb:
> @@ -2789,6 +2793,12 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi)
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>  
> +void dw_hdmi_resume(struct dw_hdmi *hdmi)
> +{
> +	dw_hdmi_init_hw(hdmi);
> +}
> +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..601243b56b69 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -154,6 +154,8 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
>  			     struct drm_encoder *encoder,
>  			     const struct dw_hdmi_plat_data *plat_data);
>  
> +void 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.22.0.rc1.311.g5d7573a151-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> things.
> 
> NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> "late/early" versions of suspend/resume because we found that the VOP
> was sometimes resuming before dw_hdmi and then calling into us before
> we were fully resumed.  For now I have gone back to the normal
> suspend/resume because I can't reproduce the problems.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - dw_hdmi_resume() is now a void function (Laurent)
> 
> Changes in v2:
> - Add forgotten static (Laurent)
> - No empty stub for suspend (Laurent)
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 4cdc9f86c2e5..7bb0f922b303 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> +{
> +	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +	dw_hdmi_resume(hdmi->hdmi);

The rockchip driver is already using the atomic suspend/resume helpers (via the
modeset helpers). Would you be able to accomplish the same thing by just moving
this call into the encoder enable callback? 

.enable is called on resume via the atomic commit framework, so everything is
ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
but I don't think that would be a problem?

Sean

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dw_hdmi_rockchip_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume)
> +};
> +
>  struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
>  	.probe  = dw_hdmi_rockchip_probe,
>  	.remove = dw_hdmi_rockchip_remove,
>  	.driver = {
>  		.name = "dwhdmi-rockchip",
> +		.pm = &dw_hdmi_rockchip_pm,
>  		.of_match_table = dw_hdmi_rockchip_dt_ids,
>  	},
>  };
> -- 
> 2.22.0.rc1.311.g5d7573a151-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> things.
> 
> NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> "late/early" versions of suspend/resume because we found that the VOP
> was sometimes resuming before dw_hdmi and then calling into us before
> we were fully resumed.  For now I have gone back to the normal
> suspend/resume because I can't reproduce the problems.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - dw_hdmi_resume() is now a void function (Laurent)
> 
> Changes in v2:
> - Add forgotten static (Laurent)
> - No empty stub for suspend (Laurent)
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 4cdc9f86c2e5..7bb0f922b303 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> +{
> +	struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +	dw_hdmi_resume(hdmi->hdmi);

The rockchip driver is already using the atomic suspend/resume helpers (via the
modeset helpers). Would you be able to accomplish the same thing by just moving
this call into the encoder enable callback? 

.enable is called on resume via the atomic commit framework, so everything is
ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
but I don't think that would be a problem?

Sean

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dw_hdmi_rockchip_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, dw_hdmi_rockchip_resume)
> +};
> +
>  struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
>  	.probe  = dw_hdmi_rockchip_probe,
>  	.remove = dw_hdmi_rockchip_remove,
>  	.driver = {
>  		.name = "dwhdmi-rockchip",
> +		.pm = &dw_hdmi_rockchip_pm,
>  		.of_match_table = dw_hdmi_rockchip_dt_ids,
>  	},
>  };
> -- 
> 2.22.0.rc1.311.g5d7573a151-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Doug Anderson]

Hi,

On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
>
> On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > things.
> >
> > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > "late/early" versions of suspend/resume because we found that the VOP
> > was sometimes resuming before dw_hdmi and then calling into us before
> > we were fully resumed.  For now I have gone back to the normal
> > suspend/resume because I can't reproduce the problems.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v3:
> > - dw_hdmi_resume() is now a void function (Laurent)
> >
> > Changes in v2:
> > - Add forgotten static (Laurent)
> > - No empty stub for suspend (Laurent)
> >
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > index 4cdc9f86c2e5..7bb0f922b303 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > +{
> > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > +
> > +     dw_hdmi_resume(hdmi->hdmi);
>
> The rockchip driver is already using the atomic suspend/resume helpers (via the
> modeset helpers). Would you be able to accomplish the same thing by just moving
> this call into the encoder enable callback?
>
> .enable is called on resume via the atomic commit framework, so everything is
> ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> but I don't think that would be a problem?

I tried and it sorta kinda half worked, but...

1. One of the problems solved by this patch is making "hot plug
detect" work after suspend / resume.  AKA: if you have nothing plugged
in to the HDMI port and then suspend/resume you need to be able to
detect when something is plugged in.  When nothing is plugged in then
the ".enable" isn't called at resume time.

2. I'm not so convinced about the whole ordering being correct.
Unfortunately on my system (Chrome OS running the chromeos-4.19
kernel) we end up getting an i2c transfer before the ".enable" is
called.  I put a dump_stack() in the i2c transfer:

[   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
     4.19.47 #60
[   42.221182] Hardware name: Rockchip (Device Tree)
[   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
(show_stack+0x20/0x24)
[   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
(dump_stack+0x84/0xa4)
[   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
(dw_hdmi_i2c_wait+0x6c/0xa8)
[   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
(dw_hdmi_i2c_xfer+0x1a8/0x30c)
[   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
(__i2c_transfer+0x3a8/0x5d8)
[   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
(i2c_transfer+0x94/0xc4)
[   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
(drm_do_probe_ddc_edid+0xbc/0x11c)
[   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
(drm_probe_ddc+0x34/0x5c)
[   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
(drm_get_edid+0x60/0x2e0)
[   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
(dw_hdmi_connector_get_modes+0x30/0x78)
[   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
[<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
[   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
[   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
(drm_ioctl_kernel+0xa0/0xf0)
[   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
(drm_ioctl+0x32c/0x3c0)
[   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
[   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
(do_vfs_ioctl+0x718/0x8b0)
[   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
(ksys_ioctl+0x5c/0x84)
[   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
[   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
(__sys_trace_return+0x0/0x10)

...I see several transfers fail and then finally a few seconds later
finally see the .enable call:

[   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
[   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end

I can gather more info if it's useful.

===

...any chance we can keep the patch as-is, or do you have ideas of how
to solve the above problems?


Thanks!

-Doug

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Doug Anderson]

Hi,

On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
>
> On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > things.
> >
> > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > "late/early" versions of suspend/resume because we found that the VOP
> > was sometimes resuming before dw_hdmi and then calling into us before
> > we were fully resumed.  For now I have gone back to the normal
> > suspend/resume because I can't reproduce the problems.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v3:
> > - dw_hdmi_resume() is now a void function (Laurent)
> >
> > Changes in v2:
> > - Add forgotten static (Laurent)
> > - No empty stub for suspend (Laurent)
> >
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > index 4cdc9f86c2e5..7bb0f922b303 100644
> > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > +{
> > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > +
> > +     dw_hdmi_resume(hdmi->hdmi);
>
> The rockchip driver is already using the atomic suspend/resume helpers (via the
> modeset helpers). Would you be able to accomplish the same thing by just moving
> this call into the encoder enable callback?
>
> .enable is called on resume via the atomic commit framework, so everything is
> ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> but I don't think that would be a problem?

I tried and it sorta kinda half worked, but...

1. One of the problems solved by this patch is making "hot plug
detect" work after suspend / resume.  AKA: if you have nothing plugged
in to the HDMI port and then suspend/resume you need to be able to
detect when something is plugged in.  When nothing is plugged in then
the ".enable" isn't called at resume time.

2. I'm not so convinced about the whole ordering being correct.
Unfortunately on my system (Chrome OS running the chromeos-4.19
kernel) we end up getting an i2c transfer before the ".enable" is
called.  I put a dump_stack() in the i2c transfer:

[   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
     4.19.47 #60
[   42.221182] Hardware name: Rockchip (Device Tree)
[   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
(show_stack+0x20/0x24)
[   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
(dump_stack+0x84/0xa4)
[   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
(dw_hdmi_i2c_wait+0x6c/0xa8)
[   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
(dw_hdmi_i2c_xfer+0x1a8/0x30c)
[   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
(__i2c_transfer+0x3a8/0x5d8)
[   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
(i2c_transfer+0x94/0xc4)
[   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
(drm_do_probe_ddc_edid+0xbc/0x11c)
[   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
(drm_probe_ddc+0x34/0x5c)
[   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
(drm_get_edid+0x60/0x2e0)
[   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
(dw_hdmi_connector_get_modes+0x30/0x78)
[   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
[<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
[   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
[   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
(drm_ioctl_kernel+0xa0/0xf0)
[   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
(drm_ioctl+0x32c/0x3c0)
[   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
[   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
(do_vfs_ioctl+0x718/0x8b0)
[   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
(ksys_ioctl+0x5c/0x84)
[   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
[   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
(__sys_trace_return+0x0/0x10)

...I see several transfers fail and then finally a few seconds later
finally see the .enable call:

[   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
[   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end

I can gather more info if it's useful.

===

...any chance we can keep the patch as-is, or do you have ideas of how
to solve the above problems?


Thanks!

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Thu, Jun 06, 2019 at 03:58:21PM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
> >
> > On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > > things.
> > >
> > > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > > "late/early" versions of suspend/resume because we found that the VOP
> > > was sometimes resuming before dw_hdmi and then calling into us before
> > > we were fully resumed.  For now I have gone back to the normal
> > > suspend/resume because I can't reproduce the problems.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > > Changes in v3:
> > > - dw_hdmi_resume() is now a void function (Laurent)
> > >
> > > Changes in v2:
> > > - Add forgotten static (Laurent)
> > > - No empty stub for suspend (Laurent)
> > >
> > >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > index 4cdc9f86c2e5..7bb0f922b303 100644
> > > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> > >       return 0;
> > >  }
> > >
> > > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > > +{
> > > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > > +
> > > +     dw_hdmi_resume(hdmi->hdmi);
> >
> > The rockchip driver is already using the atomic suspend/resume helpers (via the
> > modeset helpers). Would you be able to accomplish the same thing by just moving
> > this call into the encoder enable callback?
> >
> > .enable is called on resume via the atomic commit framework, so everything is
> > ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> > but I don't think that would be a problem?
>
> I tried and it sorta kinda half worked, but...
>
> 1. One of the problems solved by this patch is making "hot plug
> detect" work after suspend / resume.  AKA: if you have nothing plugged
> in to the HDMI port and then suspend/resume you need to be able to
> detect when something is plugged in.  When nothing is plugged in then
> the ".enable" isn't called at resume time.
>

Ahh, ok. So we've hit this with other bridges/dongles as well, and yeah the
solution is to keep the bridge powered up enough to detect hotplug, so you would
need to do some work in .resume

Usually there's a second stage of enable where you power things on more fully
and that is done in .enable

> 2. I'm not so convinced about the whole ordering being correct.
> Unfortunately on my system (Chrome OS running the chromeos-4.19
> kernel) we end up getting an i2c transfer before the ".enable" is
> called.  I put a dump_stack() in the i2c transfer:
>
> [   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
>      4.19.47 #60
> [   42.221182] Hardware name: Rockchip (Device Tree)
> [   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
> (show_stack+0x20/0x24)
> [   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
> (dump_stack+0x84/0xa4)
> [   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
> (dw_hdmi_i2c_wait+0x6c/0xa8)
> [   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
> (dw_hdmi_i2c_xfer+0x1a8/0x30c)
> [   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
> (__i2c_transfer+0x3a8/0x5d8)
> [   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
> (i2c_transfer+0x94/0xc4)
> [   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
> (drm_do_probe_ddc_edid+0xbc/0x11c)
> [   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
> (drm_probe_ddc+0x34/0x5c)
> [   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
> (drm_get_edid+0x60/0x2e0)
> [   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
> (dw_hdmi_connector_get_modes+0x30/0x78)
> [   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
> [<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
> [   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
> from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
> [   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
> (drm_ioctl_kernel+0xa0/0xf0)
> [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> (drm_ioctl+0x32c/0x3c0)
> [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> (do_vfs_ioctl+0x718/0x8b0)
> [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> (ksys_ioctl+0x5c/0x84)
> [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> (__sys_trace_return+0x0/0x10)
>
> ...I see several transfers fail and then finally a few seconds later
> finally see the .enable call:

This is usually solved by wrapping the code in detect() with an enable/disable
pair to turn on enough circuitry to do the i2c writes for edid read.

>
> [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
>
> I can gather more info if it's useful.
>
> ===
>
> ...any chance we can keep the patch as-is, or do you have ideas of how
> to solve the above problems?

Yeah, given the context I think this is Ok to apply as-is. Maybe we could
shave out some of the work done in resume and move it to a helper called in
enable/detect, but I don't think it's necessary to hold up getting things
working.

Thanks for the detailed explanation (as always :)

Reviewed-by: Sean Paul <sean@poorly.run>


>
>
> Thanks!
>
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS
> [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> (drm_ioctl+0x32c/0x3c0)
> [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> (do_vfs_ioctl+0x718/0x8b0)
> [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> (ksys_ioctl+0x5c/0x84)
> [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> (__sys_trace_return+0x0/0x10)
>
> ...I see several transfers fail and then finally a few seconds later
> finally see the .enable call:
>
> [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
>
> I can gather more info if it's useful.
>
> ===
>
> ...any chance we can keep the patch as-is, or do you have ideas of how
> to solve the above problems?
>
>
> Thanks!
>
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Thu, Jun 06, 2019 at 03:58:21PM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
> >
> > On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > > things.
> > >
> > > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > > "late/early" versions of suspend/resume because we found that the VOP
> > > was sometimes resuming before dw_hdmi and then calling into us before
> > > we were fully resumed.  For now I have gone back to the normal
> > > suspend/resume because I can't reproduce the problems.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > > Changes in v3:
> > > - dw_hdmi_resume() is now a void function (Laurent)
> > >
> > > Changes in v2:
> > > - Add forgotten static (Laurent)
> > > - No empty stub for suspend (Laurent)
> > >
> > >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > index 4cdc9f86c2e5..7bb0f922b303 100644
> > > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> > >       return 0;
> > >  }
> > >
> > > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > > +{
> > > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > > +
> > > +     dw_hdmi_resume(hdmi->hdmi);
> >
> > The rockchip driver is already using the atomic suspend/resume helpers (via the
> > modeset helpers). Would you be able to accomplish the same thing by just moving
> > this call into the encoder enable callback?
> >
> > .enable is called on resume via the atomic commit framework, so everything is
> > ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> > but I don't think that would be a problem?
>
> I tried and it sorta kinda half worked, but...
>
> 1. One of the problems solved by this patch is making "hot plug
> detect" work after suspend / resume.  AKA: if you have nothing plugged
> in to the HDMI port and then suspend/resume you need to be able to
> detect when something is plugged in.  When nothing is plugged in then
> the ".enable" isn't called at resume time.
>

Ahh, ok. So we've hit this with other bridges/dongles as well, and yeah the
solution is to keep the bridge powered up enough to detect hotplug, so you would
need to do some work in .resume

Usually there's a second stage of enable where you power things on more fully
and that is done in .enable

> 2. I'm not so convinced about the whole ordering being correct.
> Unfortunately on my system (Chrome OS running the chromeos-4.19
> kernel) we end up getting an i2c transfer before the ".enable" is
> called.  I put a dump_stack() in the i2c transfer:
>
> [   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
>      4.19.47 #60
> [   42.221182] Hardware name: Rockchip (Device Tree)
> [   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
> (show_stack+0x20/0x24)
> [   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
> (dump_stack+0x84/0xa4)
> [   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
> (dw_hdmi_i2c_wait+0x6c/0xa8)
> [   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
> (dw_hdmi_i2c_xfer+0x1a8/0x30c)
> [   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
> (__i2c_transfer+0x3a8/0x5d8)
> [   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
> (i2c_transfer+0x94/0xc4)
> [   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
> (drm_do_probe_ddc_edid+0xbc/0x11c)
> [   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
> (drm_probe_ddc+0x34/0x5c)
> [   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
> (drm_get_edid+0x60/0x2e0)
> [   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
> (dw_hdmi_connector_get_modes+0x30/0x78)
> [   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
> [<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
> [   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
> from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
> [   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
> (drm_ioctl_kernel+0xa0/0xf0)
> [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> (drm_ioctl+0x32c/0x3c0)
> [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> (do_vfs_ioctl+0x718/0x8b0)
> [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> (ksys_ioctl+0x5c/0x84)
> [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> (__sys_trace_return+0x0/0x10)
>
> ...I see several transfers fail and then finally a few seconds later
> finally see the .enable call:

This is usually solved by wrapping the code in detect() with an enable/disable
pair to turn on enough circuitry to do the i2c writes for edid read.

>
> [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
>
> I can gather more info if it's useful.
>
> ===
>
> ...any chance we can keep the patch as-is, or do you have ideas of how
> to solve the above problems?

Yeah, given the context I think this is Ok to apply as-is. Maybe we could
shave out some of the work done in resume and move it to a helper called in
enable/detect, but I don't think it's necessary to hold up getting things
working.

Thanks for the detailed explanation (as always :)

Reviewed-by: Sean Paul <sean@poorly.run>


>
>
> Thanks!
>
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS
> [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> (drm_ioctl+0x32c/0x3c0)
> [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> (do_vfs_ioctl+0x718/0x8b0)
> [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> (ksys_ioctl+0x5c/0x84)
> [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> (__sys_trace_return+0x0/0x10)
>
> ...I see several transfers fail and then finally a few seconds later
> finally see the .enable call:
>
> [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
>
> I can gather more info if it's useful.
>
> ===
>
> ...any chance we can keep the patch as-is, or do you have ideas of how
> to solve the above problems?
>
>
> Thanks!
>
> -Doug

-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Fri, Jun 07, 2019 at 02:06:03PM -0400, Sean Paul wrote:
> On Thu, Jun 06, 2019 at 03:58:21PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
> > >
> > > On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > > > things.
> > > >
> > > > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > > > "late/early" versions of suspend/resume because we found that the VOP
> > > > was sometimes resuming before dw_hdmi and then calling into us before
> > > > we were fully resumed.  For now I have gone back to the normal
> > > > suspend/resume because I can't reproduce the problems.
> > > >
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - dw_hdmi_resume() is now a void function (Laurent)
> > > >
> > > > Changes in v2:
> > > > - Add forgotten static (Laurent)
> > > > - No empty stub for suspend (Laurent)
> > > >
> > > >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > index 4cdc9f86c2e5..7bb0f922b303 100644
> > > > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > > > +{
> > > > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > > > +
> > > > +     dw_hdmi_resume(hdmi->hdmi);
> > >
> > > The rockchip driver is already using the atomic suspend/resume helpers (via the
> > > modeset helpers). Would you be able to accomplish the same thing by just moving
> > > this call into the encoder enable callback?
> > >
> > > .enable is called on resume via the atomic commit framework, so everything is
> > > ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> > > but I don't think that would be a problem?
> >
> > I tried and it sorta kinda half worked, but...
> >
> > 1. One of the problems solved by this patch is making "hot plug
> > detect" work after suspend / resume.  AKA: if you have nothing plugged
> > in to the HDMI port and then suspend/resume you need to be able to
> > detect when something is plugged in.  When nothing is plugged in then
> > the ".enable" isn't called at resume time.
> >
> 
> Ahh, ok. So we've hit this with other bridges/dongles as well, and yeah the
> solution is to keep the bridge powered up enough to detect hotplug, so you would
> need to do some work in .resume
> 
> Usually there's a second stage of enable where you power things on more fully
> and that is done in .enable
> 
> > 2. I'm not so convinced about the whole ordering being correct.
> > Unfortunately on my system (Chrome OS running the chromeos-4.19
> > kernel) we end up getting an i2c transfer before the ".enable" is
> > called.  I put a dump_stack() in the i2c transfer:
> >
> > [   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
> >      4.19.47 #60
> > [   42.221182] Hardware name: Rockchip (Device Tree)
> > [   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
> > (show_stack+0x20/0x24)
> > [   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
> > (dump_stack+0x84/0xa4)
> > [   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
> > (dw_hdmi_i2c_wait+0x6c/0xa8)
> > [   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
> > (dw_hdmi_i2c_xfer+0x1a8/0x30c)
> > [   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
> > (__i2c_transfer+0x3a8/0x5d8)
> > [   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
> > (i2c_transfer+0x94/0xc4)
> > [   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
> > (drm_do_probe_ddc_edid+0xbc/0x11c)
> > [   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
> > (drm_probe_ddc+0x34/0x5c)
> > [   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
> > (drm_get_edid+0x60/0x2e0)
> > [   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
> > (dw_hdmi_connector_get_modes+0x30/0x78)
> > [   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
> > [<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
> > [   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
> > from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
> > [   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
> > (drm_ioctl_kernel+0xa0/0xf0)
> > [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> > (drm_ioctl+0x32c/0x3c0)
> > [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> > [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> > (do_vfs_ioctl+0x718/0x8b0)
> > [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> > (ksys_ioctl+0x5c/0x84)
> > [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> > [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> > (__sys_trace_return+0x0/0x10)
> >
> > ...I see several transfers fail and then finally a few seconds later
> > finally see the .enable call:
> 
> This is usually solved by wrapping the code in detect() with an enable/disable
> pair to turn on enough circuitry to do the i2c writes for edid read.
> 
> >
> > [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> > [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
> >
> > I can gather more info if it's useful.
> >
> > ===
> >
> > ...any chance we can keep the patch as-is, or do you have ideas of how
> > to solve the above problems?
> 
> Yeah, given the context I think this is Ok to apply as-is. Maybe we could
> shave out some of the work done in resume and move it to a helper called in
> enable/detect, but I don't think it's necessary to hold up getting things
> working.
> 
> Thanks for the detailed explanation (as always :)
> 
> Reviewed-by: Sean Paul <sean@poorly.run>

Applied both patches to drm-misc-next, thank you!

Sean

> 
> 
> >
> >
> > Thanks!
> >
> > -Doug
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> > [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> > (drm_ioctl+0x32c/0x3c0)
> > [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> > [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> > (do_vfs_ioctl+0x718/0x8b0)
> > [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> > (ksys_ioctl+0x5c/0x84)
> > [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> > [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> > (__sys_trace_return+0x0/0x10)
> >
> > ...I see several transfers fail and then finally a few seconds later
> > finally see the .enable call:
> >
> > [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> > [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
> >
> > I can gather more info if it's useful.
> >
> > ===
> >
> > ...any chance we can keep the patch as-is, or do you have ideas of how
> > to solve the above problems?
> >
> >
> > Thanks!
> >
> > -Doug
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume

[Sean Paul]

On Fri, Jun 07, 2019 at 02:06:03PM -0400, Sean Paul wrote:
> On Thu, Jun 06, 2019 at 03:58:21PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
> > >
> > > On Tue, Jun 04, 2019 at 01:42: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 call the core dw-hdmi's suspend/resume functions to restore
> > > > things.
> > > >
> > > > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
> > > > "late/early" versions of suspend/resume because we found that the VOP
> > > > was sometimes resuming before dw_hdmi and then calling into us before
> > > > we were fully resumed.  For now I have gone back to the normal
> > > > suspend/resume because I can't reproduce the problems.
> > > >
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - dw_hdmi_resume() is now a void function (Laurent)
> > > >
> > > > Changes in v2:
> > > > - Add forgotten static (Laurent)
> > > > - No empty stub for suspend (Laurent)
> > > >
> > > >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > index 4cdc9f86c2e5..7bb0f922b303 100644
> > > > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
> > > > +{
> > > > +     struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
> > > > +
> > > > +     dw_hdmi_resume(hdmi->hdmi);
> > >
> > > The rockchip driver is already using the atomic suspend/resume helpers (via the
> > > modeset helpers). Would you be able to accomplish the same thing by just moving
> > > this call into the encoder enable callback?
> > >
> > > .enable is called on resume via the atomic commit framework, so everything is
> > > ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
> > > but I don't think that would be a problem?
> >
> > I tried and it sorta kinda half worked, but...
> >
> > 1. One of the problems solved by this patch is making "hot plug
> > detect" work after suspend / resume.  AKA: if you have nothing plugged
> > in to the HDMI port and then suspend/resume you need to be able to
> > detect when something is plugged in.  When nothing is plugged in then
> > the ".enable" isn't called at resume time.
> >
> 
> Ahh, ok. So we've hit this with other bridges/dongles as well, and yeah the
> solution is to keep the bridge powered up enough to detect hotplug, so you would
> need to do some work in .resume
> 
> Usually there's a second stage of enable where you power things on more fully
> and that is done in .enable
> 
> > 2. I'm not so convinced about the whole ordering being correct.
> > Unfortunately on my system (Chrome OS running the chromeos-4.19
> > kernel) we end up getting an i2c transfer before the ".enable" is
> > called.  I put a dump_stack() in the i2c transfer:
> >
> > [   42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G         C
> >      4.19.47 #60
> > [   42.221182] Hardware name: Rockchip (Device Tree)
> > [   42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
> > (show_stack+0x20/0x24)
> > [   42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
> > (dump_stack+0x84/0xa4)
> > [   42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
> > (dw_hdmi_i2c_wait+0x6c/0xa8)
> > [   42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
> > (dw_hdmi_i2c_xfer+0x1a8/0x30c)
> > [   42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
> > (__i2c_transfer+0x3a8/0x5d8)
> > [   42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
> > (i2c_transfer+0x94/0xc4)
> > [   42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
> > (drm_do_probe_ddc_edid+0xbc/0x11c)
> > [   42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
> > (drm_probe_ddc+0x34/0x5c)
> > [   42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
> > (drm_get_edid+0x60/0x2e0)
> > [   42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
> > (dw_hdmi_connector_get_modes+0x30/0x78)
> > [   42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
> > [<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
> > [   42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
> > from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
> > [   42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
> > (drm_ioctl_kernel+0xa0/0xf0)
> > [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> > (drm_ioctl+0x32c/0x3c0)
> > [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> > [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> > (do_vfs_ioctl+0x718/0x8b0)
> > [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> > (ksys_ioctl+0x5c/0x84)
> > [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> > [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> > (__sys_trace_return+0x0/0x10)
> >
> > ...I see several transfers fail and then finally a few seconds later
> > finally see the .enable call:
> 
> This is usually solved by wrapping the code in detect() with an enable/disable
> pair to turn on enough circuitry to do the i2c writes for edid read.
> 
> >
> > [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> > [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
> >
> > I can gather more info if it's useful.
> >
> > ===
> >
> > ...any chance we can keep the patch as-is, or do you have ideas of how
> > to solve the above problems?
> 
> Yeah, given the context I think this is Ok to apply as-is. Maybe we could
> shave out some of the work done in resume and move it to a helper called in
> enable/detect, but I don't think it's necessary to hold up getting things
> working.
> 
> Thanks for the detailed explanation (as always :)
> 
> Reviewed-by: Sean Paul <sean@poorly.run>

Applied both patches to drm-misc-next, thank you!

Sean

> 
> 
> >
> >
> > Thanks!
> >
> > -Doug
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> > [   42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
> > (drm_ioctl+0x32c/0x3c0)
> > [   42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
> > [   42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
> > (do_vfs_ioctl+0x718/0x8b0)
> > [   42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
> > (ksys_ioctl+0x5c/0x84)
> > [   42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
> > [   42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
> > (__sys_trace_return+0x0/0x10)
> >
> > ...I see several transfers fail and then finally a few seconds later
> > finally see the .enable call:
> >
> > [   44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
> > [   44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
> >
> > I can gather more info if it's useful.
> >
> > ===
> >
> > ...any chance we can keep the patch as-is, or do you have ideas of how
> > to solve the above problems?
> >
> >
> > Thanks!
> >
> > -Doug
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel