[PATCH v2 0/2] DSIPHY RPM

[PATCH v2 0/2] DSIPHY RPM

[Konrad Dybcio]

Some recent SoCs use power rails that we model as GENPDs to power the
DSIPHY. This series attempts to make such configurations suspendable.

Tested on SM6375.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v2:
- Improve patch 1 commit message
- Add return value checking in patch 1
- Link to v1: https://lore.kernel.org/r/20230620-topic-dsiphy_rpm-v1-0-446a7fd0ce43@linaro.org

---
Konrad Dybcio (2):
      drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
      drm/msm/dsi: Enable runtime PM

 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
---
base-commit: 9dbf40840551df336c95ce2a3adbdd25ed53c0ef
change-id: 20230620-topic-dsiphy_rpm-ea4817badaab

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH v2 1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks

[Konrad Dybcio]

This helper has been introduced to avoid programmer errors (missing
_put calls leading to dangling refcnt) when using pm_runtime_get, use it.

While at it, start checking the return value.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 9d5795c58a98..2f319e0eb74f 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -516,7 +516,9 @@ static int dsi_phy_enable_resource(struct msm_dsi_phy *phy)
 	struct device *dev = &phy->pdev->dev;
 	int ret;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
 
 	ret = clk_prepare_enable(phy->ahb_clk);
 	if (ret) {

-- 
2.41.0

Re: [PATCH v2 1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks

[Dmitry Baryshkov]

On 20/06/2023 14:43, Konrad Dybcio wrote:
> This helper has been introduced to avoid programmer errors (missing
> _put calls leading to dangling refcnt) when using pm_runtime_get, use it.
> 
> While at it, start checking the return value.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

What about also adding the following tag?

Fixes: 5c8290284402 ("drm/msm/dsi: Split PHY drivers to separate files")


-- 
With best wishes
Dmitry

[PATCH v2 2/2] drm/msm/dsi: Enable runtime PM

[Konrad Dybcio]

Some devices power the DSI PHY/PLL through a power rail that we model
as a GENPD. Enable runtime PM to make it suspendable.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 2f319e0eb74f..22431e106529 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -689,6 +689,10 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
 				     "Unable to get ahb clk\n");
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
 	/* PLL init will call into clk_register which requires
 	 * register access, so we need to enable power and ahb clock.
 	 */

-- 
2.41.0

Re: [PATCH v2 2/2] drm/msm/dsi: Enable runtime PM

[Dmitry Baryshkov]

On 20/06/2023 14:43, Konrad Dybcio wrote:
> Some devices power the DSI PHY/PLL through a power rail that we model
> as a GENPD. Enable runtime PM to make it suspendable.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

So, we were calling pm_runtime_get/put, but we didn't have runtime PM 
enabled for this device? It might be a nice place for dev_warn() in the 
driver core.

Nevertheless:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 2f319e0eb74f..22431e106529 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -689,6 +689,10 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, PTR_ERR(phy->ahb_clk),
>   				     "Unable to get ahb clk\n");
>   
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>   	/* PLL init will call into clk_register which requires
>   	 * register access, so we need to enable power and ahb clock.
>   	 */
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] DSIPHY RPM

[Dmitry Baryshkov]

On Tue, 20 Jun 2023 13:43:19 +0200, Konrad Dybcio wrote:
> Some recent SoCs use power rails that we model as GENPDs to power the
> DSIPHY. This series attempts to make such configurations suspendable.
> 
> Tested on SM6375.
> 
> 

Applied, thanks!

[1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
      https://gitlab.freedesktop.org/lumag/msm/-/commit/a402e0e61b75
[2/2] drm/msm/dsi: Enable runtime PM
      https://gitlab.freedesktop.org/lumag/msm/-/commit/4e905c2acc9d

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH v2 0/2] DSIPHY RPM

[Dmitry Baryshkov]

On 11/07/2023 17:21, Dmitry Baryshkov wrote:
> 
> On Tue, 20 Jun 2023 13:43:19 +0200, Konrad Dybcio wrote:
>> Some recent SoCs use power rails that we model as GENPDs to power the
>> DSIPHY. This series attempts to make such configurations suspendable.
>>
>> Tested on SM6375.
>>
>>
> 
> Applied, thanks!
> 
> [1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
>        https://gitlab.freedesktop.org/lumag/msm/-/commit/a402e0e61b75
> [2/2] drm/msm/dsi: Enable runtime PM
>        https://gitlab.freedesktop.org/lumag/msm/-/commit/4e905c2acc9d

Unfortunately this series breaks our CI, see [1], [2]. I had to remove 
these patches for now.

I suppose this is either because of a probe deferral or because of 
having subset of drivers built as module. Konrad, could you please take 
a look?

[1] https://gitlab.freedesktop.org/drm/msm/-/jobs/45271774
[2] https://gitlab.freedesktop.org/drm/msm/-/jobs/45271775

-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] DSIPHY RPM

[Konrad Dybcio]

On 11.07.2023 18:31, Dmitry Baryshkov wrote:
> On 11/07/2023 17:21, Dmitry Baryshkov wrote:
>>
>> On Tue, 20 Jun 2023 13:43:19 +0200, Konrad Dybcio wrote:
>>> Some recent SoCs use power rails that we model as GENPDs to power the
>>> DSIPHY. This series attempts to make such configurations suspendable.
>>>
>>> Tested on SM6375.
>>>
>>>
>>
>> Applied, thanks!
>>
>> [1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
>>        https://gitlab.freedesktop.org/lumag/msm/-/commit/a402e0e61b75
>> [2/2] drm/msm/dsi: Enable runtime PM
>>        https://gitlab.freedesktop.org/lumag/msm/-/commit/4e905c2acc9d
> 
> Unfortunately this series breaks our CI, see [1], [2]. I had to remove these patches for now.
> 
> I suppose this is either because of a probe deferral or because of having subset of drivers built as module. Konrad, could you please take a look?
I see no reason why it would break things :/

You can see that rpmhpd sync_state has not completed, which means all
PDs should still be pinned at max vote..

Can we somehow retest it?

If it still fails, can you try enabling runtime pm on dispcc and hooking
up vddcx?

Konrad
> 
> [1] https://gitlab.freedesktop.org/drm/msm/-/jobs/45271774
> [2] https://gitlab.freedesktop.org/drm/msm/-/jobs/45271775
> 

Re: [PATCH v2 0/2] DSIPHY RPM

[Konrad Dybcio]

On 15.07.2023 17:25, Konrad Dybcio wrote:
> On 11.07.2023 18:31, Dmitry Baryshkov wrote:
>> On 11/07/2023 17:21, Dmitry Baryshkov wrote:
>>>
>>> On Tue, 20 Jun 2023 13:43:19 +0200, Konrad Dybcio wrote:
>>>> Some recent SoCs use power rails that we model as GENPDs to power the
>>>> DSIPHY. This series attempts to make such configurations suspendable.
>>>>
>>>> Tested on SM6375.
>>>>
>>>>
>>>
>>> Applied, thanks!
>>>
>>> [1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
>>>        https://gitlab.freedesktop.org/lumag/msm/-/commit/a402e0e61b75
>>> [2/2] drm/msm/dsi: Enable runtime PM
>>>        https://gitlab.freedesktop.org/lumag/msm/-/commit/4e905c2acc9d
>>
>> Unfortunately this series breaks our CI, see [1], [2]. I had to remove these patches for now.
>>
>> I suppose this is either because of a probe deferral or because of having subset of drivers built as module. Konrad, could you please take a look?
> I see no reason why it would break things :/
> 
> You can see that rpmhpd sync_state has not completed, which means all
> PDs should still be pinned at max vote..
> 
> Can we somehow retest it?
> 
> If it still fails, can you try enabling runtime pm on dispcc and hooking
> up vddcx?
IIRC this was a fluke with the CI, can we retry?

Konrad

Re: [PATCH v2 0/2] DSIPHY RPM

[Dmitry Baryshkov]

On Tue, 20 Jun 2023 13:43:19 +0200, Konrad Dybcio wrote:
> Some recent SoCs use power rails that we model as GENPDs to power the
> DSIPHY. This series attempts to make such configurations suspendable.
> 
> Tested on SM6375.
> 
> 

Applied, thanks!

[1/2] drm/msm/dsi: Use pm_runtime_resume_and_get to prevent refcnt leaks
      https://gitlab.freedesktop.org/lumag/msm/-/commit/3d07a411b4fa
[2/2] drm/msm/dsi: Enable runtime PM
      https://gitlab.freedesktop.org/lumag/msm/-/commit/6ab502bc1cf3

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>