Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock

Hi Laurent,

On 3/17/21 4:32 AM, Laurent Pinchart wrote:

Hi Quanyang,

Thank you for the patch.

On Wed, Mar 10, 2021 at 12:59:45PM +0800, quanyang.wang@windriver.com wrote:

quoted

From: Quanyang Wang <redacted>

The Runtime PM subsystem will force the device "fd4a0000.zynqmp-display"
to enter suspend state while booting if the following conditions are met:
- the usage counter is zero (pm_runtime_get_sync hasn't been called yet)
- no 'active' children (no zynqmp-dp-snd-xx node under dpsub node)
- no other device in the same power domain (dpdma node has no
		"power-domains = <&zynqmp_firmware PD_DP>" property)

So there is a scenario as below:
1) DP device enters suspend state   <- call zynqmp_gpd_power_off
2) zynqmp_disp_crtc_setup_clock	    <- configurate register VPLL_FRAC_CFG
3) pm_runtime_get_sync		    <- call zynqmp_gpd_power_on and clear previous
				       VPLL_FRAC_CFG configuration
4) clk_prepare_enable(disp->pclk)   <- enable failed since VPLL_FRAC_CFG
				       configuration is corrupted

 From above, we can see that pm_runtime_get_sync may clear register
VPLL_FRAC_CFG configuration and result the failure of clk enabling.
Putting pm_runtime_get_sync at the very beginning of the function
zynqmp_disp_crtc_atomic_enable can resolve this issue.

Isn't this an issue in the firmware though, which shouldn't clear the
previous VPLLF_FRAC_CFG ?

Thank you for your review.  I used to look into the atf and PWU code and 
it seems (I didn't add debug code

to PMU to make sure if PMU really does this, I only in kernel call 
zynqmp_pm_get_pll_frac_data to make sure that

the value in data field of VPLL_FRAC_CFG register is changed from 0x4000 
to 0x0 after run pm_runtime_get_sync)

that PMU intends to reset VPLL when there is an  Off and On in DP 
Powerdomain.


Linux ATF                                 PWU

zynqmp_gpd_power_on
->zynqmp_pm_set_requirement
-->send PM_SET_REQUIREMENT to ATF  ==>	    ATF send ipi to PWU   ==>   PmSetRequirement
									->PmRequirementUpdate
									-->PmUpdateSlave(masterReq->slave)
									--->PmSlaveChangeState
  									---->PmSlaveChangeState
  									----->PmSlaveClearAfterState
  									------>PmClockRelease
  									------->PmClockReleaseInt(&ch->clock->base)
  									-------->clk->class->release(clk)
  									--------->PmPllBypassAndReset //Here reset the VPLL then VPLL_FRAC_CFG is cleared.
    
   

Yes,  I bumped into the same issue and I used to make a patch (Patch 1) 
as below.

I didn't send it to mainline because it seems not to be a driver issue. 
The mode of VPLL

is not set correctly because:

1) VPLL is enabled before linux

2) linux calling pm_clock_set_pll_mode can't really set register because 
in ATF

it only store the mode value to a structure and wait a clk-enable 
request to do

the register-set operation.

3) linux call clk_enable will not send a clk-enable request since it 
checks that

the VPLL is already hardware-enabled because of 1).

So the firmware should disable VPLL when it exits and also in linux 
zynqmp clk driver

there should be a check list to reset some clks to a predefined state.


By the way, there is a tiny patch (Patch 2) to fix the black screen 
issue in DP. I think you may

be preparing a big patch which add drm properties to this driver and it 
may contain this modification,

so I didn't send it.


Thanks,

Quanyang

Patch 1:

 From 93311de2c61e87f2426b89259d81cded71aee673 Mon Sep 17 00:00:00 2001
From: Quanyang Wang <redacted>
Date: Thu, 3 Dec 2020 19:19:50 +0800
Subject: [PATCH 1/3] drm: xlnx: set PLL_MODE_FRAC mode to VPLL_FRAC_CFG by
  re-enabling disp->pclk

When the function clk_set_rate configures the rate of disp->pclk,
zynqmp_pm_set_pll_frac_mode will be called to set VPLL's mode to
be PLL_MODE_FRAC or PLL_MODE_INT by invoking an SMC call to ATF.
But in ATF, the service pm_clock_set_pll_mode doesn't really set
the VPLL_FRAC_CFG register but only stores the mode value to
struct pm_pll *pll. The operation that sets the register must be
triggered by zynqmp_pm_clock_enable.

Since disp->pclk is enabled in hardware before linux booting,
clk_prepare_enable will skip over zynqmp_pm_clock_enable. So
we have to enable then disable disp->pclk, and re-enable it to
make sure that zynqmp_pm_clock_enable is triggered and the mode is
set to VPLL_FRAC_CFG. Or else VPLL will work in an incorrect mode.

Signed-off-by: Quanyang Wang <redacted>
---
  drivers/gpu/drm/xlnx/zynqmp_disp.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 

b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 98bd48f13fd1..19753ffc424e 100644

--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c

@@ -1668,6 +1668,11 @@ int zynqmp_disp_probe(struct zynqmp_dpsub *dpsub, 

struct drm_device *drm)
             dev_err(disp->dev, "failed to init any video clock\n");
             return PTR_ERR(disp->pclk);
         }
+
+       /* Make sure that disp->pclk is disabled in hardware */
+       ret = clk_prepare_enable(disp->pclk);
+       clk_disable_unprepare(disp->pclk);
+
         disp->pclk_from_ps = true;
     }

Patch 2:

 From 8c6d36bcb4e79e0e5f8e157446cd994b4a2126f0 Mon Sep 17 00:00:00 2001
From: Quanyang Wang <redacted>
Date: Thu, 3 Dec 2020 19:32:56 +0800
Subject: [PATCH 2/3] drm: xlnx: configure alpha value to make graphic layer
  opaque

Since graphics layer is primary, and video layer is overaly,
we need to configure the V_BLEND_SET_GLOBAL_ALPHA_REG register
to make graphic layer opaque by default, or else graphic layer
will be transparent and invisible.

Signed-off-by: Quanyang Wang <redacted>
---
  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 3 ++-
  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 1 +
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 

b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 19753ffc424e..5c84589e1899 100644

--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c

@@ -1468,7 +1468,8 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc *crtc,
     zynqmp_disp_blend_set_output_format(&disp->blend,
                         ZYNQMP_DPSUB_FORMAT_RGB);
     zynqmp_disp_blend_set_bg_color(&disp->blend, 0, 0, 0);
-   zynqmp_disp_blend_set_global_alpha(&disp->blend, false, 0);
+   zynqmp_disp_blend_set_global_alpha(&disp->blend, true,
+                  ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_MAX);

     zynqmp_disp_enable(disp);

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h 

b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
index f92a006d5070..ef409aca11ad 100644

--- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h

@@ -22,6 +22,7 @@
  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA       0xc
  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_VALUE(n)  ((n) << 1)
  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_EN        BIT(0)
+#define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_MAX   0xff
  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT     0x14
  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_RGB     0x0
  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_YCBCR444    0x1

quoted

---
  drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 148add0ca1d6..909e6c265406 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c

@@ -1445,9 +1445,10 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc *crtc,
  	struct drm_display_mode *adjusted_mode = &crtc->state->adjusted_mode;
  	int ret, vrefresh;
  
+	pm_runtime_get_sync(disp->dev);
+
  	zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode);
  
-	pm_runtime_get_sync(disp->dev);
  	ret = clk_prepare_enable(disp->pclk);
  	if (ret) {
  		dev_err(disp->dev, "failed to enable a pixel clock\n");

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