Re: [PATCH v5 24/36] drm/bridge: analogix_dp: Reorder plat_data->power_off to happen sooner

[Archit Taneja]

On Saturday 10 March 2018 03:53 AM, Enric Balletbo i Serra wrote:

From: Douglas Anderson 

The current user of the analogix power_off is "analogix_dp-rockchip".
That driver does this:
- deactivate PSR
- turn off a clock

Both of these things (especially deactive PSR) should be done before
we turn the PHY power off and turn off analog power.  Let's move the
callback up.

Note that without this patch (and with
https://patchwork.kernel.org/patch/9553349/ [seanpaul: this patch was
not applied, but it seems like the race can still occur]), I experienced
an error in reboot testing where one thread was at:

   rockchip_drm_psr_deactivate
   rockchip_dp_powerdown
   analogix_dp_bridge_disable
   drm_bridge_disable

...and the other thread was at:

   analogix_dp_send_psr_spd
   analogix_dp_enable_psr
   analogix_dp_psr_set
   psr_flush_handler

The flush handler thread was finding AUX channel errors and eventually
reported "Failed to apply PSR", where I had a kgdb breakpoint. Presumably
the device would have eventually given up and shut down anyway, but it
seems better to fix the order to be more correct.



Reviewed-by: Archit Taneja 

Thanks,
Archit


Cc: Kristian H. Kristensen 
Signed-off-by: Douglas Anderson 
Signed-off-by: Sean Paul 
Signed-off-by: Thierry Escande 
Reviewed-by: Andrzej Hajda 
Signed-off-by: Enric Balletbo i Serra 
Tested-by: Marek Szyprowski 
---

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 37b16643f14c..eaf93cbd47a8 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1337,12 +1337,13 @@ static void analogix_dp_bridge_disable(struct 
drm_bridge *bridge)
}
  
  	disable_irq(dp->irq);

-   analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
-   phy_power_off(dp->phy);
  
  	if (dp->plat_data->power_off)

dp->plat_data->power_off(dp->plat_data);
  
+	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);

+   phy_power_off(dp->phy);
+
clk_disable_unprepare(dp->clock);
  
  	pm_runtime_put_sync(dp->dev);



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

[PATCH v5 24/36] drm/bridge: analogix_dp: Reorder plat_data->power_off to happen sooner

[Enric Balletbo i Serra]

From: Douglas Anderson 

The current user of the analogix power_off is "analogix_dp-rockchip".
That driver does this:
- deactivate PSR
- turn off a clock

Both of these things (especially deactive PSR) should be done before
we turn the PHY power off and turn off analog power.  Let's move the
callback up.

Note that without this patch (and with
https://patchwork.kernel.org/patch/9553349/ [seanpaul: this patch was
not applied, but it seems like the race can still occur]), I experienced
an error in reboot testing where one thread was at:

  rockchip_drm_psr_deactivate
  rockchip_dp_powerdown
  analogix_dp_bridge_disable
  drm_bridge_disable

...and the other thread was at:

  analogix_dp_send_psr_spd
  analogix_dp_enable_psr
  analogix_dp_psr_set
  psr_flush_handler

The flush handler thread was finding AUX channel errors and eventually
reported "Failed to apply PSR", where I had a kgdb breakpoint. Presumably
the device would have eventually given up and shut down anyway, but it
seems better to fix the order to be more correct.

Cc: Kristian H. Kristensen 
Signed-off-by: Douglas Anderson 
Signed-off-by: Sean Paul 
Signed-off-by: Thierry Escande 
Reviewed-by: Andrzej Hajda 
Signed-off-by: Enric Balletbo i Serra 
Tested-by: Marek Szyprowski 
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 37b16643f14c..eaf93cbd47a8 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1337,12 +1337,13 @@ static void analogix_dp_bridge_disable(struct 
drm_bridge *bridge)
}
 
disable_irq(dp->irq);
-   analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
-   phy_power_off(dp->phy);
 
if (dp->plat_data->power_off)
dp->plat_data->power_off(dp->plat_data);
 
+   analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
+   phy_power_off(dp->phy);
+
clk_disable_unprepare(dp->clock);
 
pm_runtime_put_sync(dp->dev);
-- 
2.16.1

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