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

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

[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

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

[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


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

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

[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

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

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

[quanyang.wang]

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");

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

[quanyang.wang]

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");

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

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

[quanyang.wang]

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");
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

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

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

[Laurent Pinchart]

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

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

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

[Laurent Pinchart]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[quanyang.wang]

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");

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

[quanyang.wang]

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");

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

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

[quanyang.wang]

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");
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michal Simek]

+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");

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

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

[Michal Simek]

+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");
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rohit Visavalia]

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.

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");
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rohit Visavalia]

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.

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");
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[quanyang.wang]

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");

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

[quanyang.wang]

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");

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

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

[quanyang.wang]

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");
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel