[PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz

[PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz

[Dmitry Osipenko]

RGB output doesn't allow to change parent clock rate of the display and
PCLK rate is set to 0Hz in this case. The tegra_dc_commit_state() shall
not set the display clock to 0Hz since this change propagates to the
parent clock. The DISP clock is defined as a NODIV clock by the tegra-clk
driver and all NODIV clocks use the CLK_SET_RATE_PARENT flag.

This bug stayed unnoticed because by default PLLP is used as the parent
clock for the display controller and PLLP silently skips the erroneous 0Hz
rate changes because it always has active child clocks that don't permit
rate changes. The PLLP isn't acceptable for some devices that we want to
upstream (like Samsung Galaxy Tab and ASUS TF700T) due to a display panel
clock rate requirements that can't be fulfilled by using PLLP and then the
bug pops up in this case since parent clock is set to 0Hz, killing the
display output.

Don't touch DC clock if pclk=0 in order to fix the problem.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 1399e4032701..4ecda4cdf345 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1723,6 +1723,11 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
 			dev_err(dc->dev,
 				"failed to set clock rate to %lu Hz\n",
 				state->pclk);
+
+		err = clk_set_rate(dc->clk, state->pclk);
+		if (err < 0)
+			dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
+				dc->clk, state->pclk, err);
 	}
 
 	DRM_DEBUG_KMS("rate: %lu, div: %u\n", clk_get_rate(dc->clk),
@@ -1733,11 +1738,6 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
 		value = SHIFT_CLK_DIVIDER(state->div) | PIXEL_CLK_DIVIDER_PCD1;
 		tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
 	}
-
-	err = clk_set_rate(dc->clk, state->pclk);
-	if (err < 0)
-		dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
-			dc->clk, state->pclk, err);
 }
 
 static void tegra_dc_stop(struct tegra_dc *dc)
-- 
2.29.2

[PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz

[Dmitry Osipenko]

RGB output doesn't allow to change parent clock rate of the display and
PCLK rate is set to 0Hz in this case. The tegra_dc_commit_state() shall
not set the display clock to 0Hz since this change propagates to the
parent clock. The DISP clock is defined as a NODIV clock by the tegra-clk
driver and all NODIV clocks use the CLK_SET_RATE_PARENT flag.

This bug stayed unnoticed because by default PLLP is used as the parent
clock for the display controller and PLLP silently skips the erroneous 0Hz
rate changes because it always has active child clocks that don't permit
rate changes. The PLLP isn't acceptable for some devices that we want to
upstream (like Samsung Galaxy Tab and ASUS TF700T) due to a display panel
clock rate requirements that can't be fulfilled by using PLLP and then the
bug pops up in this case since parent clock is set to 0Hz, killing the
display output.

Don't touch DC clock if pclk=0 in order to fix the problem.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 1399e4032701..4ecda4cdf345 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1723,6 +1723,11 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
 			dev_err(dc->dev,
 				"failed to set clock rate to %lu Hz\n",
 				state->pclk);
+
+		err = clk_set_rate(dc->clk, state->pclk);
+		if (err < 0)
+			dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
+				dc->clk, state->pclk, err);
 	}
 
 	DRM_DEBUG_KMS("rate: %lu, div: %u\n", clk_get_rate(dc->clk),
@@ -1733,11 +1738,6 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
 		value = SHIFT_CLK_DIVIDER(state->div) | PIXEL_CLK_DIVIDER_PCD1;
 		tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
 	}
-
-	err = clk_set_rate(dc->clk, state->pclk);
-	if (err < 0)
-		dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
-			dc->clk, state->pclk, err);
 }
 
 static void tegra_dc_stop(struct tegra_dc *dc)
-- 
2.29.2

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

Re: [PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Tue, Mar 02, 2021 at 04:15:06PM +0300, Dmitry Osipenko wrote:
> RGB output doesn't allow to change parent clock rate of the display and
> PCLK rate is set to 0Hz in this case. The tegra_dc_commit_state() shall
> not set the display clock to 0Hz since this change propagates to the
> parent clock. The DISP clock is defined as a NODIV clock by the tegra-clk
> driver and all NODIV clocks use the CLK_SET_RATE_PARENT flag.
> 
> This bug stayed unnoticed because by default PLLP is used as the parent
> clock for the display controller and PLLP silently skips the erroneous 0Hz
> rate changes because it always has active child clocks that don't permit
> rate changes. The PLLP isn't acceptable for some devices that we want to
> upstream (like Samsung Galaxy Tab and ASUS TF700T) due to a display panel
> clock rate requirements that can't be fulfilled by using PLLP and then the
> bug pops up in this case since parent clock is set to 0Hz, killing the
> display output.
> 
> Don't touch DC clock if pclk=0 in order to fix the problem.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] drm/tegra: dc: Don't set PLL clock to 0Hz

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1248 bytes --]

On Tue, Mar 02, 2021 at 04:15:06PM +0300, Dmitry Osipenko wrote:
> RGB output doesn't allow to change parent clock rate of the display and
> PCLK rate is set to 0Hz in this case. The tegra_dc_commit_state() shall
> not set the display clock to 0Hz since this change propagates to the
> parent clock. The DISP clock is defined as a NODIV clock by the tegra-clk
> driver and all NODIV clocks use the CLK_SET_RATE_PARENT flag.
> 
> This bug stayed unnoticed because by default PLLP is used as the parent
> clock for the display controller and PLLP silently skips the erroneous 0Hz
> rate changes because it always has active child clocks that don't permit
> rate changes. The PLLP isn't acceptable for some devices that we want to
> upstream (like Samsung Galaxy Tab and ASUS TF700T) due to a display panel
> clock rate requirements that can't be fulfilled by using PLLP and then the
> bug pops up in this case since parent clock is set to 0Hz, killing the
> display output.
> 
> Don't touch DC clock if pclk=0 in order to fix the problem.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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