* [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock @ 2021-03-10 4:59 quanyang.wang 2021-03-16 11:46 ` quanyang.wang 2021-03-16 20:32 ` Laurent Pinchart 0 siblings, 2 replies; 5+ messages in thread From: quanyang.wang @ 2021-03-10 4:59 UTC (permalink / raw) To: Hyun Kwon, Laurent Pinchart, David Airlie, Daniel Vetter, Michal Simek Cc: dri-devel, linux-arm-kernel, linux-kernel, quanyang.wang From: Quanyang Wang <quanyang.wang@windriver.com> 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. Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> --- 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"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock 2021-03-10 4:59 [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock quanyang.wang @ 2021-03-16 11:46 ` quanyang.wang 2021-03-16 20:32 ` Laurent Pinchart 1 sibling, 0 replies; 5+ messages in thread From: quanyang.wang @ 2021-03-16 11:46 UTC (permalink / raw) To: Hyun Kwon, Laurent Pinchart, David Airlie, Daniel Vetter, Michal Simek Cc: dri-devel, linux-arm-kernel, linux-kernel Ping. Any comment on this patch? Thanks, Quanyang On 3/10/21 12:59 PM, quanyang.wang@windriver.com wrote: > From: Quanyang Wang <quanyang.wang@windriver.com> > > 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. > > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> > --- > 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"); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock 2021-03-10 4:59 [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock quanyang.wang 2021-03-16 11:46 ` quanyang.wang @ 2021-03-16 20:32 ` Laurent Pinchart 2021-03-17 3:00 ` quanyang.wang 1 sibling, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2021-03-16 20:32 UTC (permalink / raw) To: quanyang.wang Cc: Hyun Kwon, David Airlie, Daniel Vetter, Michal Simek, dri-devel, linux-arm-kernel, linux-kernel Hi Quanyang, Thank you for the patch. On Wed, Mar 10, 2021 at 12:59:45PM +0800, quanyang.wang@windriver.com wrote: > From: Quanyang Wang <quanyang.wang@windriver.com> > > 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 ? > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> Nonetheless, this change looks good to me, I actually had the same patch in my tree while investigation issues related to the clock rate, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I was hoping it would solve the issue I'm experiencing with the DP clock, but that's not the case :-( In a nutshell, when the DP is first started, the clock frequency is incorrect. The following quick & dirty patch fixes the problem: diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index 74ac0a064eb5..fdbe1b0640aa 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -1439,6 +1439,10 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc *crtc, pm_runtime_get_sync(disp->dev); + ret = clk_prepare_enable(disp->pclk); + if (!ret) + clk_disable_unprepare(disp->pclk); + zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode); ret = clk_prepare_enable(disp->pclk); The problem doesn't seem to be in the kernel, but on the TF-A or PMU firmware side. Have you experienced this by any chance ? > --- > 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"); -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock 2021-03-16 20:32 ` Laurent Pinchart @ 2021-03-17 3:00 ` quanyang.wang [not found] ` <72a24e1f-8708-7f87-b1b5-a2b171bf4576@xilinx.com> 0 siblings, 1 reply; 5+ messages in thread From: quanyang.wang @ 2021-03-17 3:00 UTC (permalink / raw) To: Laurent Pinchart Cc: Hyun Kwon, David Airlie, Daniel Vetter, Michal Simek, dri-devel, linux-arm-kernel, linux-kernel 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: >> From: Quanyang Wang <quanyang.wang@windriver.com> >> >> 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. > >> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> > Nonetheless, this change looks good to me, I actually had the same patch > in my tree while investigation issues related to the clock rate, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I was hoping it would solve the issue I'm experiencing with the DP > clock, but that's not the case :-( In a nutshell, when the DP is first > started, the clock frequency is incorrect. The following quick & dirty > patch fixes the problem: > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index 74ac0a064eb5..fdbe1b0640aa 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -1439,6 +1439,10 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc *crtc, > > pm_runtime_get_sync(disp->dev); > > + ret = clk_prepare_enable(disp->pclk); > + if (!ret) > + clk_disable_unprepare(disp->pclk); > + > zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode); > > ret = clk_prepare_enable(disp->pclk); > > The problem doesn't seem to be in the kernel, but on the TF-A or PMU > firmware side. Have you experienced this by any chance ? 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 <quanyang.wang@windriver.com> 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 <quanyang.wang@windriver.com> --- 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 <quanyang.wang@windriver.com> 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 <quanyang.wang@windriver.com> --- 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 >> --- >> 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"); ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <72a24e1f-8708-7f87-b1b5-a2b171bf4576@xilinx.com>]
[parent not found: <DM6PR02MB666647B8BF6F1AABFA744383B36A9@DM6PR02MB6666.namprd02.prod.outlook.com>]
* Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock [not found] ` <DM6PR02MB666647B8BF6F1AABFA744383B36A9@DM6PR02MB6666.namprd02.prod.outlook.com> @ 2021-03-17 11:50 ` quanyang.wang 0 siblings, 0 replies; 5+ messages in thread From: quanyang.wang @ 2021-03-17 11:50 UTC (permalink / raw) To: Rohit Visavalia, Michal Simek, Laurent Pinchart Cc: Hyun Kwon, David Airlie, Daniel Vetter, dri-devel, linux-arm-kernel, linux-kernel Hi Rohit, On 3/17/21 7:17 PM, Rohit Visavalia wrote: > Hi Quanyang & Laurent, > > I tested this patch(which moves pm_runtime_get_sync at the very beginning of the function zynqmp_disp_crtc_atomic_enable), i don't see any behavior change with patch, means with patch also DP display is not getting up and it is required to resolution change to make it up. > Also valid bit is not getting set for VPLL_FRAC_CFG on bootup, same behavior as without patch. Thank you for your test. This patch works under the condition of applying the below patch (drm: xlnx: set PLL_MODE_FRAC mode to VPLL_FRAC_CFG by re-enabling disp->pclk) (Patch 1) first. Without Patch 1, the VPLL mode isn't set correctly and clk_prepare_enable don't take effect. DP monitor can't receive any signal since the clk is wrong. After applying Patch 1, there will be error log in the boot message: [ 1.325602] zynqmp_pll_enable() clock enable failed for vpll_int, ret = -22 [ 1.325614] zynqmp-dpsub fd4a0000.display: failed to enable a pixel clock [ 1.456106] ------------[ cut here ]------------ [ 1.456109] [CRTC:33:crtc-0] vblank wait timed out [ 1.456152] WARNING: CPU: 2 PID: 75 at drivers/gpu/drm/drm_atomic_helper.c:1512 drm_atomic_helper_wait_for_vblanks.part.0+0x27c/0x298 [ 1.456172] Modules linked in: [ 1.456181] CPU: 2 PID: 75 Comm: kworker/2:1 Not tainted 5.12.0-rc2-00338-gf78d76e72a46-dirty #4 [ 1.456188] Hardware name: ZynqMP ZCU102 Rev1.0 (DT) [ 1.456194] Workqueue: events deferred_probe_work_func Then applying this patch ( drm: xlnx: call pm_runtime_get_sync before setting pixel clock), this issue will be fixed, DP monitor will receive signal, but screen is still black. Then applying the below patch (drm: xlnx: configure alpha value to make graphic layer opaque) (Patch 2), the DP will work well. Thanks, Quanyang > > Thanks, > Rohit > >> -----Original Message----- >> From: Michal Simek <michal.simek@xilinx.com> >> Sent: Wednesday, March 17, 2021 1:56 PM >> To: quanyang.wang <quanyang.wang@windriver.com>; Laurent Pinchart >> <laurent.pinchart@ideasonboard.com>; Rohit Visavalia >> <RVISAVAL@xilinx.com> >> Cc: Hyun Kwon <hyunk@xlnx.xilinx.com>; David Airlie <airlied@linux.ie>; >> Daniel Vetter <daniel@ffwll.ch>; Michal Simek <michals@xilinx.com>; dri- >> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel >> clock >> >> +Rohit >> >> On 3/17/21 4:00 AM, quanyang.wang wrote: >>> 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: >>>>> From: Quanyang Wang <quanyang.wang@windriver.com> >>>>> >>>>> 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. >>> >>>>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> >>>> Nonetheless, this change looks good to me, I actually had the same >>>> patch in my tree while investigation issues related to the clock >>>> rate, so >>>> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> I was hoping it would solve the issue I'm experiencing with the DP >>>> clock, but that's not the case :-( In a nutshell, when the DP is >>>> first started, the clock frequency is incorrect. The following quick >>>> & dirty patch fixes the problem: >>>> >>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c >>>> b/drivers/gpu/drm/xlnx/zynqmp_disp.c >>>> index 74ac0a064eb5..fdbe1b0640aa 100644 >>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c >>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c >>>> @@ -1439,6 +1439,10 @@ zynqmp_disp_crtc_atomic_enable(struct >> drm_crtc >>>> *crtc, >>>> >>>> pm_runtime_get_sync(disp->dev); >>>> >>>> + ret = clk_prepare_enable(disp->pclk); >>>> + if (!ret) >>>> + clk_disable_unprepare(disp->pclk); >>>> + >>>> zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode); >>>> >>>> ret = clk_prepare_enable(disp->pclk); >>>> >>>> The problem doesn't seem to be in the kernel, but on the TF-A or PMU >>>> firmware side. Have you experienced this by any chance ? >>> 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 <quanyang.wang@windriver.com> >>> 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 <quanyang.wang@windriver.com> >>> --- >>> 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 <quanyang.wang@windriver.com> >>> 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 <quanyang.wang@windriver.com> >>> --- >>> 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 >>> >>>>> --- >>>>> 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"); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-17 11:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 4:59 [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock quanyang.wang 2021-03-16 11:46 ` quanyang.wang 2021-03-16 20:32 ` Laurent Pinchart 2021-03-17 3:00 ` quanyang.wang [not found] ` <72a24e1f-8708-7f87-b1b5-a2b171bf4576@xilinx.com> [not found] ` <DM6PR02MB666647B8BF6F1AABFA744383B36A9@DM6PR02MB6666.namprd02.prod.outlook.com> 2021-03-17 11:50 ` quanyang.wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).