[v3,05/43] drm/bridge: analogix_dp: Don't power bridge in analogix_dp_bind
diff mbox

Message ID 20180130202913.28724-6-thierry.escande@collabora.com
State New
Headers show

Commit Message

Thierry Escande Jan. 30, 2018, 8:28 p.m. UTC
From: zain wang <wzz@rock-chips.com>

The bridge does not need to be powered in analogix_dp_bind(), so
remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
as well as their power-off counterparts.

Cc: Stéphane Marchesin <marcheu@chromium.org>
Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
[the patch originally just removed the power_on portion, seanpaul removed
the power off code as well as improved the commit message]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Heiko Stübner Feb. 28, 2018, 2:37 p.m. UTC | #1
Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
> From: zain wang <wzz@rock-chips.com>
> 
> The bridge does not need to be powered in analogix_dp_bind(), so
> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
> as well as their power-off counterparts.
> 
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: zain wang <wzz@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> [the patch originally just removed the power_on portion, seanpaul removed
> the power off code as well as improved the commit message]
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
> cb5e18d6ba04..1477ea9ba85d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
> drm_device *drm_dev,
> 
>  	pm_runtime_enable(dev);
> 
> -	pm_runtime_get_sync(dev);
> -	phy_power_on(dp->phy);
> -
> -	analogix_dp_init_dp(dp);
> -
>  	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>  					analogix_dp_hardirq,
>  					analogix_dp_irq_thread,

Not 100% sure here, as the driver has the request-irq + disable-irq hack
here. So a pending interrupt could possibly fire between request and
disable.

Right now the block should be on, but can it still handle such an irq
when the power is removed?

So before removing the power here, we might want something
similar to what Marc posted for the vop [0] for the analogix-dp?


Heiko

[0] https://patchwork.kernel.org/patch/10210513/
Marc Zyngier Feb. 28, 2018, 2:54 p.m. UTC | #2
On 28/02/18 14:37, Heiko Stübner wrote:
> Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
>> From: zain wang <wzz@rock-chips.com>
>>
>> The bridge does not need to be powered in analogix_dp_bind(), so
>> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
>> as well as their power-off counterparts.
>>
>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>> Signed-off-by: zain wang <wzz@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> [the patch originally just removed the power_on portion, seanpaul removed
>> the power off code as well as improved the commit message]
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
>> ---
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
>>  1 file changed, 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
>> cb5e18d6ba04..1477ea9ba85d 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>>
>>  	pm_runtime_enable(dev);
>>
>> -	pm_runtime_get_sync(dev);
>> -	phy_power_on(dp->phy);
>> -
>> -	analogix_dp_init_dp(dp);
>> -
>>  	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>>  					analogix_dp_hardirq,
>>  					analogix_dp_irq_thread,
> 
> Not 100% sure here, as the driver has the request-irq + disable-irq hack
> here. So a pending interrupt could possibly fire between request and
> disable.
> 
> Right now the block should be on, but can it still handle such an irq
> when the power is removed?

Probably not (see below).

> So before removing the power here, we might want something
> similar to what Marc posted for the vop [0] for the analogix-dp?

You can do that trick only if the interrupt is not shared. In the VOP
case, it is shared with the IOMMU, which makes it more... interesting.

And when it comes to power and the analogix-dp driver, I've been
carrying this[1] for a while. Fully exploitable from userspace. I know
it is about to be replaced by this series, but at least 4.15 and 4.16
are affected.

	M.

[1] https://www.spinics.net/lists/arm-kernel/msg623892.html
Heiko Stübner Feb. 28, 2018, 2:56 p.m. UTC | #3
Am Mittwoch, 28. Februar 2018, 15:54:30 CET schrieb Marc Zyngier:
> On 28/02/18 14:37, Heiko Stübner wrote:
> > Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
> >> From: zain wang <wzz@rock-chips.com>
> >> 
> >> The bridge does not need to be powered in analogix_dp_bind(), so
> >> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
> >> as well as their power-off counterparts.
> >> 
> >> Cc: Stéphane Marchesin <marcheu@chromium.org>
> >> Signed-off-by: zain wang <wzz@rock-chips.com>
> >> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> >> [the patch originally just removed the power_on portion, seanpaul removed
> >> the power off code as well as improved the commit message]
> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
> >>  1 file changed, 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
> >> cb5e18d6ba04..1477ea9ba85d 100644
> >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
> >> drm_device *drm_dev,
> >> 
> >>  	pm_runtime_enable(dev);
> >> 
> >> -	pm_runtime_get_sync(dev);
> >> -	phy_power_on(dp->phy);
> >> -
> >> -	analogix_dp_init_dp(dp);
> >> -
> >> 
> >>  	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
> >>  	
> >>  					analogix_dp_hardirq,
> >>  					analogix_dp_irq_thread,
> > 
> > Not 100% sure here, as the driver has the request-irq + disable-irq hack
> > here. So a pending interrupt could possibly fire between request and
> > disable.
> > 
> > Right now the block should be on, but can it still handle such an irq
> > when the power is removed?
> 
> Probably not (see below).
> 
> > So before removing the power here, we might want something
> > similar to what Marc posted for the vop [0] for the analogix-dp?
> 
> You can do that trick only if the interrupt is not shared. In the VOP
> case, it is shared with the IOMMU, which makes it more... interesting.

Yep, which is why I mentioned it, as the dp-irq should not be shared
I'd think :-)


Heiko
Heiko Stübner Feb. 28, 2018, 3:20 p.m. UTC | #4
Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
> From: zain wang <wzz@rock-chips.com>
> 
> The bridge does not need to be powered in analogix_dp_bind(), so
> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
> as well as their power-off counterparts.
> 
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: zain wang <wzz@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> [the patch originally just removed the power_on portion, seanpaul removed
> the power off code as well as improved the commit message]
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
> cb5e18d6ba04..1477ea9ba85d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
> drm_device *drm_dev,
> 
>  	pm_runtime_enable(dev);
> 
> -	pm_runtime_get_sync(dev);
> -	phy_power_on(dp->phy);
> -
> -	analogix_dp_init_dp(dp);
> -
>  	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>  					analogix_dp_hardirq,
>  					analogix_dp_irq_thread,
> @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct
> drm_device *drm_dev, goto err_disable_pm_runtime;
>  	}
> 
> -	phy_power_off(dp->phy);
> -	pm_runtime_put(dev);
> -
>  	return dp;
> 
>  err_disable_pm_runtime:
> 
> -	phy_power_off(dp->phy);
> -	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
> 
>  	return ERR_PTR(ret);

In general, this patch seems to also create the opposite than
"drm/bridge: analogix_dp: Keep PHY powered between driver bind/unbind" [0]

posted on monday?

[0] https://patchwork.kernel.org/patch/10242493/
Marek Szyprowski March 1, 2018, 8:19 a.m. UTC | #5
Hi Heiko,

Thanks for adding me to this thread.

On 2018-02-28 16:20, Heiko Stübner wrote:
> Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
>> From: zain wang <wzz@rock-chips.com>
>>
>> The bridge does not need to be powered in analogix_dp_bind(), so
>> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
>> as well as their power-off counterparts.
>>
>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>> Signed-off-by: zain wang <wzz@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> [the patch originally just removed the power_on portion, seanpaul removed
>> the power off code as well as improved the commit message]
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
>> ---
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
>>   1 file changed, 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
>> cb5e18d6ba04..1477ea9ba85d 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>>
>>   	pm_runtime_enable(dev);
>>
>> -	pm_runtime_get_sync(dev);
>> -	phy_power_on(dp->phy);
>> -
>> -	analogix_dp_init_dp(dp);
>> -
>>   	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>>   					analogix_dp_hardirq,
>>   					analogix_dp_irq_thread,
>> @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev, goto err_disable_pm_runtime;
>>   	}
>>
>> -	phy_power_off(dp->phy);
>> -	pm_runtime_put(dev);
>> -
>>   	return dp;
>>
>>   err_disable_pm_runtime:
>>
>> -	phy_power_off(dp->phy);
>> -	pm_runtime_put(dev);
>>   	pm_runtime_disable(dev);
>>
>>   	return ERR_PTR(ret);
> In general, this patch seems to also create the opposite than
> "drm/bridge: analogix_dp: Keep PHY powered between driver bind/unbind" [0]
>
> posted on monday?
>
> [0] https://patchwork.kernel.org/patch/10242493/

Well, my patch was a quick workaround to avoid board freeze.

This patch looks like a proper fix. Besides removing runtime pm and phy
power calls from dp_bind, it also removes dp register access done in
analogix_dp_init_dp, as there is really no need to touch registers in bind
operation.

The patchset however suffers from other issues on Exynos hardware. I 
will post
them in that thread.

Best regards
Marek Szyprowski March 1, 2018, 1:37 p.m. UTC | #6
Hi,

On 2018-01-30 21:28, Thierry Escande wrote:
> From: zain wang <wzz@rock-chips.com>
>
> The bridge does not need to be powered in analogix_dp_bind(), so
> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
> as well as their power-off counterparts.
>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: zain wang <wzz@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> [the patch originally just removed the power_on portion, seanpaul removed
> the power off code as well as improved the commit message]
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> ---
>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 ----------
>   1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index cb5e18d6ba04..1477ea9ba85d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>   
>   	pm_runtime_enable(dev);
>   
> -	pm_runtime_get_sync(dev);
> -	phy_power_on(dp->phy);
> -
> -	analogix_dp_init_dp(dp);
> -
>   	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>   					analogix_dp_hardirq,
>   					analogix_dp_irq_thread,
> @@ -1414,15 +1409,10 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>   		goto err_disable_pm_runtime;
>   	}
>   
> -	phy_power_off(dp->phy);
> -	pm_runtime_put(dev);
> -
>   	return dp;
>   
>   err_disable_pm_runtime:
>   
> -	phy_power_off(dp->phy);
> -	pm_runtime_put(dev);
>   	pm_runtime_disable(dev);
>   
>   	return ERR_PTR(ret);

Once this change is applied, there is also no need to keep dp->clock
prepared & enabled between bind/unbind.

analogix_dp_set_bridge() and analogix_dp_bridge_disable() properly manage
dp->clock on their own.

Best regards

Patch
diff mbox

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index cb5e18d6ba04..1477ea9ba85d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1382,11 +1382,6 @@  analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	pm_runtime_enable(dev);
 
-	pm_runtime_get_sync(dev);
-	phy_power_on(dp->phy);
-
-	analogix_dp_init_dp(dp);
-
 	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
 					analogix_dp_hardirq,
 					analogix_dp_irq_thread,
@@ -1414,15 +1409,10 @@  analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 		goto err_disable_pm_runtime;
 	}
 
-	phy_power_off(dp->phy);
-	pm_runtime_put(dev);
-
 	return dp;
 
 err_disable_pm_runtime:
 
-	phy_power_off(dp->phy);
-	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 
 	return ERR_PTR(ret);