[PATCH v1 0/2] NVIDIA Tegra display driver improvements

[PATCH v1 0/2] NVIDIA Tegra display driver improvements

[Dmitry Osipenko]

Asus Transformer TF700T tablet requires couple minor changes to the
display driver in order to light up display panel, here they are.

Dmitry Osipenko (2):
  drm/tegra: dc: rgb: Move PCLK shifter programming to CRTC
  drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

 drivers/gpu/drm/tegra/dc.c  | 33 ++++++++++++++++-------
 drivers/gpu/drm/tegra/dc.h  |  1 +
 drivers/gpu/drm/tegra/rgb.c | 53 ++++++++++++++++++++++++++++++++-----
 3 files changed, 71 insertions(+), 16 deletions(-)

-- 
2.32.0

[PATCH v1 1/2] drm/tegra: dc: rgb: Move PCLK shifter programming to CRTC

[Dmitry Osipenko]

Asus TF700T tablet uses TC358768 DPI->DSI bridge that sits between Tegra's
DPI output and display panel input. Bridge requires to have stable PCLK
output before RGB encoder is enabled because it uses PCLK by itself to
clock internal logic and bridge is programmed before Tegra's encoder is
enabled. Hence the PCLK clock shifter must be programmed when CRTC is
enabled, otherwise clock is unstable and bridge hangs because of it.
Move the shifter programming from RGB encoder into CRTC.

Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c  | 6 ++++++
 drivers/gpu/drm/tegra/rgb.c | 4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a29d64f87563..a582ce28b632 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -2107,6 +2107,12 @@ static void tegra_crtc_atomic_enable(struct drm_crtc *crtc,
 		tegra_dc_writel(dc, value, DC_COM_RG_UNDERFLOW);
 	}
 
+	if (dc->rgb) {
+		/* XXX: parameterize? */
+		value = SC0_H_QUALIFIER_NONE | SC1_H_QUALIFIER_NONE;
+		tegra_dc_writel(dc, value, DC_DISP_SHIFT_CLOCK_OPTIONS);
+	}
+
 	tegra_dc_commit(dc);
 
 	drm_crtc_vblank_on(crtc);
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 606c78a2b988..933e14e4609f 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -116,10 +116,6 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
 		DISP_ORDER_RED_BLUE;
 	tegra_dc_writel(rgb->dc, value, DC_DISP_DISP_INTERFACE_CONTROL);
 
-	/* XXX: parameterize? */
-	value = SC0_H_QUALIFIER_NONE | SC1_H_QUALIFIER_NONE;
-	tegra_dc_writel(rgb->dc, value, DC_DISP_SHIFT_CLOCK_OPTIONS);
-
 	tegra_dc_commit(rgb->dc);
 }
 
-- 
2.32.0

[PATCH v1 2/2] drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

[Dmitry Osipenko]

Asus Transformer TF700T is a Tegra30 tablet device which uses RGB->DSI
bridge that requires a precise clock rate in order to operate properly.
Tegra30 has a dedicated PLL for each display controller, hence the PLL
rate can be changed freely. Allow PLL rate changes on Tegra30+ for RGB
output. Configure the clock rate before display controller is enabled
since DC itself may be running off this PLL and it's not okay to change
the rate of the active PLL that doesn't support dynamic frequency
switching since hardware will hang.

Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c  | 27 ++++++++++++--------
 drivers/gpu/drm/tegra/dc.h  |  1 +
 drivers/gpu/drm/tegra/rgb.c | 49 +++++++++++++++++++++++++++++++++++--
 3 files changed, 65 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a582ce28b632..de4a7cb271ce 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1762,10 +1762,9 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
 	return 0;
 }
 
-static void tegra_dc_commit_state(struct tegra_dc *dc,
-				  struct tegra_dc_state *state)
+static void tegra_dc_set_clock_rate(struct tegra_dc *dc,
+				    struct tegra_dc_state *state)
 {
-	u32 value;
 	int err;
 
 	err = clk_set_parent(dc->clk, state->clk);
@@ -1796,11 +1795,6 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
 	DRM_DEBUG_KMS("rate: %lu, div: %u\n", clk_get_rate(dc->clk),
 		      state->div);
 	DRM_DEBUG_KMS("pclk: %lu\n", state->pclk);
-
-	if (!dc->soc->has_nvdisplay) {
-		value = SHIFT_CLK_DIVIDER(state->div) | PIXEL_CLK_DIVIDER_PCD1;
-		tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
-	}
 }
 
 static void tegra_dc_stop(struct tegra_dc *dc)
@@ -2002,6 +1996,9 @@ static void tegra_crtc_atomic_enable(struct drm_crtc *crtc,
 	u32 value;
 	int err;
 
+	/* apply PLL changes */
+	tegra_dc_set_clock_rate(dc, crtc_state);
+
 	err = host1x_client_resume(&dc->client);
 	if (err < 0) {
 		dev_err(dc->dev, "failed to resume: %d\n", err);
@@ -2076,8 +2073,11 @@ static void tegra_crtc_atomic_enable(struct drm_crtc *crtc,
 	else
 		tegra_dc_writel(dc, 0, DC_DISP_BORDER_COLOR);
 
-	/* apply PLL and pixel clock changes */
-	tegra_dc_commit_state(dc, crtc_state);
+	/* apply pixel clock changes */
+	if (!dc->soc->has_nvdisplay) {
+		value = SHIFT_CLK_DIVIDER(crtc_state->div) | PIXEL_CLK_DIVIDER_PCD1;
+		tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
+	}
 
 	/* program display mode */
 	tegra_dc_set_timings(dc, mode);
@@ -2691,6 +2691,7 @@ static const struct tegra_dc_soc_info tegra20_dc_soc_info = {
 	.has_win_b_vfilter_mem_client = true,
 	.has_win_c_without_vert_filter = true,
 	.plane_tiled_memory_bandwidth_x2 = false,
+	.has_pll_d2_out0 = false,
 };
 
 static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
@@ -2713,6 +2714,7 @@ static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
 	.has_win_b_vfilter_mem_client = true,
 	.has_win_c_without_vert_filter = false,
 	.plane_tiled_memory_bandwidth_x2 = true,
+	.has_pll_d2_out0 = true,
 };
 
 static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
@@ -2735,6 +2737,7 @@ static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
 	.has_win_b_vfilter_mem_client = false,
 	.has_win_c_without_vert_filter = false,
 	.plane_tiled_memory_bandwidth_x2 = true,
+	.has_pll_d2_out0 = true,
 };
 
 static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
@@ -2757,6 +2760,7 @@ static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
 	.has_win_b_vfilter_mem_client = false,
 	.has_win_c_without_vert_filter = false,
 	.plane_tiled_memory_bandwidth_x2 = false,
+	.has_pll_d2_out0 = true,
 };
 
 static const struct tegra_dc_soc_info tegra210_dc_soc_info = {
@@ -2779,6 +2783,7 @@ static const struct tegra_dc_soc_info tegra210_dc_soc_info = {
 	.has_win_b_vfilter_mem_client = false,
 	.has_win_c_without_vert_filter = false,
 	.plane_tiled_memory_bandwidth_x2 = false,
+	.has_pll_d2_out0 = true,
 };
 
 static const struct tegra_windowgroup_soc tegra186_dc_wgrps[] = {
@@ -2829,6 +2834,7 @@ static const struct tegra_dc_soc_info tegra186_dc_soc_info = {
 	.wgrps = tegra186_dc_wgrps,
 	.num_wgrps = ARRAY_SIZE(tegra186_dc_wgrps),
 	.plane_tiled_memory_bandwidth_x2 = false,
+	.has_pll_d2_out0 = false,
 };
 
 static const struct tegra_windowgroup_soc tegra194_dc_wgrps[] = {
@@ -2879,6 +2885,7 @@ static const struct tegra_dc_soc_info tegra194_dc_soc_info = {
 	.wgrps = tegra194_dc_wgrps,
 	.num_wgrps = ARRAY_SIZE(tegra194_dc_wgrps),
 	.plane_tiled_memory_bandwidth_x2 = false,
+	.has_pll_d2_out0 = false,
 };
 
 static const struct of_device_id tegra_dc_of_match[] = {
diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index 40378308d527..c9c4c45c0518 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -76,6 +76,7 @@ struct tegra_dc_soc_info {
 	bool has_win_b_vfilter_mem_client;
 	bool has_win_c_without_vert_filter;
 	bool plane_tiled_memory_bandwidth_x2;
+	bool has_pll_d2_out0;
 };
 
 struct tegra_dc {
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 933e14e4609f..ff8fce36d2aa 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -17,6 +17,8 @@ struct tegra_rgb {
 	struct tegra_output output;
 	struct tegra_dc *dc;
 
+	struct clk *pll_d_out0;
+	struct clk *pll_d2_out0;
 	struct clk *clk_parent;
 	struct clk *clk;
 };
@@ -119,6 +121,18 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
 	tegra_dc_commit(rgb->dc);
 }
 
+static bool tegra_rgb_pll_rate_change_allowed(struct tegra_rgb *rgb)
+{
+	if (!rgb->pll_d2_out0)
+		return false;
+
+	if (!clk_is_match(rgb->clk_parent, rgb->pll_d_out0) &&
+	    !clk_is_match(rgb->clk_parent, rgb->pll_d2_out0))
+		return false;
+
+	return true;
+}
+
 static int
 tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
 			       struct drm_crtc_state *crtc_state,
@@ -147,8 +161,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
 	 * and hope that the desired frequency can be matched (or at least
 	 * matched sufficiently close that the panel will still work).
 	 */
-	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
-	pclk = 0;
+	if (tegra_rgb_pll_rate_change_allowed(rgb)) {
+		/*
+		 * Set display controller clock to x2 of PCLK in order to
+		 * produce higher resolution pulse positions.
+		 */
+		div = 2;
+		pclk *= 2;
+	} else {
+		div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
+		pclk = 0;
+	}
 
 	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
 					 pclk, div);
@@ -206,6 +229,22 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
 		return err;
 	}
 
+	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
+	if (IS_ERR(rgb->pll_d_out0)) {
+		err = PTR_ERR(rgb->pll_d_out0);
+		dev_err(dc->dev, "failed to get pll_d_out0: %d\n", err);
+		return err;
+	}
+
+	if (dc->soc->has_pll_d2_out0) {
+		rgb->pll_d2_out0 = clk_get_sys(NULL, "pll_d2_out0");
+		if (IS_ERR(rgb->pll_d2_out0)) {
+			err = PTR_ERR(rgb->pll_d2_out0);
+			dev_err(dc->dev, "failed to get pll_d2_out0: %d\n", err);
+			return err;
+		}
+	}
+
 	dc->rgb = &rgb->output;
 
 	return 0;
@@ -213,9 +252,15 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
 
 int tegra_dc_rgb_remove(struct tegra_dc *dc)
 {
+	struct tegra_rgb *rgb;
+
 	if (!dc->rgb)
 		return 0;
 
+	rgb = to_rgb(dc->rgb);
+	clk_put(rgb->pll_d2_out0);
+	clk_put(rgb->pll_d_out0);
+
 	tegra_output_remove(dc->rgb);
 	dc->rgb = NULL;
 
-- 
2.32.0

Re: [PATCH v1 1/2] drm/tegra: dc: rgb: Move PCLK shifter programming to CRTC

[Thierry Reding]

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

On Thu, Sep 30, 2021 at 01:28:04AM +0300, Dmitry Osipenko wrote:
> Asus TF700T tablet uses TC358768 DPI->DSI bridge that sits between Tegra's
> DPI output and display panel input. Bridge requires to have stable PCLK
> output before RGB encoder is enabled because it uses PCLK by itself to
> clock internal logic and bridge is programmed before Tegra's encoder is
> enabled. Hence the PCLK clock shifter must be programmed when CRTC is
> enabled, otherwise clock is unstable and bridge hangs because of it.
> Move the shifter programming from RGB encoder into CRTC.
> 
> Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c  | 6 ++++++
>  drivers/gpu/drm/tegra/rgb.c | 4 ----
>  2 files changed, 6 insertions(+), 4 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH v1 2/2] drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

[Thierry Reding]

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

On Thu, Sep 30, 2021 at 01:28:05AM +0300, Dmitry Osipenko wrote:
> Asus Transformer TF700T is a Tegra30 tablet device which uses RGB->DSI
> bridge that requires a precise clock rate in order to operate properly.
> Tegra30 has a dedicated PLL for each display controller, hence the PLL
> rate can be changed freely. Allow PLL rate changes on Tegra30+ for RGB
> output. Configure the clock rate before display controller is enabled
> since DC itself may be running off this PLL and it's not okay to change
> the rate of the active PLL that doesn't support dynamic frequency
> switching since hardware will hang.
> 
> Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c  | 27 ++++++++++++--------
>  drivers/gpu/drm/tegra/dc.h  |  1 +
>  drivers/gpu/drm/tegra/rgb.c | 49 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 65 insertions(+), 12 deletions(-)

This seems overly complicated. I especially don't like the way how
clocks are looked up with clk_get_sys() and then used in the comparison.
Could this not be achieved by using assigned-clocks and friends
properties in DT?

Thierry

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

Re: [PATCH v1 2/2] drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

[Dmitry Osipenko]

06.10.2021 21:13, Thierry Reding пишет:
> On Thu, Sep 30, 2021 at 01:28:05AM +0300, Dmitry Osipenko wrote:
>> Asus Transformer TF700T is a Tegra30 tablet device which uses RGB->DSI
>> bridge that requires a precise clock rate in order to operate properly.
>> Tegra30 has a dedicated PLL for each display controller, hence the PLL
>> rate can be changed freely. Allow PLL rate changes on Tegra30+ for RGB
>> output. Configure the clock rate before display controller is enabled
>> since DC itself may be running off this PLL and it's not okay to change
>> the rate of the active PLL that doesn't support dynamic frequency
>> switching since hardware will hang.
>>
>> Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/tegra/dc.c  | 27 ++++++++++++--------
>>  drivers/gpu/drm/tegra/dc.h  |  1 +
>>  drivers/gpu/drm/tegra/rgb.c | 49 +++++++++++++++++++++++++++++++++++--
>>  3 files changed, 65 insertions(+), 12 deletions(-)
> 
> This seems overly complicated. I especially don't like the way how
> clocks are looked up with clk_get_sys() and then used in the comparison.
> Could this not be achieved by using assigned-clocks and friends
> properties in DT?

Assigned-clocks have nothing to do with this patch. We need to check
whether PLLD *is* already pre-assigned as the parent.

Adding properties for describing the clk parents is overly complicated,
clk_get_sys() is a much simpler solution that doesn't involve extra DT
changes.

BTW, assigned-clocks can't be used for display controller  because
controller is usually turned on during boot and reparenting of active DC
will hang machine.

Re: [PATCH v1 2/2] drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

[Dmitry Osipenko]

06.10.2021 21:27, Dmitry Osipenko пишет:
> 06.10.2021 21:13, Thierry Reding пишет:
>> On Thu, Sep 30, 2021 at 01:28:05AM +0300, Dmitry Osipenko wrote:
>>> Asus Transformer TF700T is a Tegra30 tablet device which uses RGB->DSI
>>> bridge that requires a precise clock rate in order to operate properly.
>>> Tegra30 has a dedicated PLL for each display controller, hence the PLL
>>> rate can be changed freely. Allow PLL rate changes on Tegra30+ for RGB
>>> output. Configure the clock rate before display controller is enabled
>>> since DC itself may be running off this PLL and it's not okay to change
>>> the rate of the active PLL that doesn't support dynamic frequency
>>> switching since hardware will hang.
>>>
>>> Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/gpu/drm/tegra/dc.c  | 27 ++++++++++++--------
>>>  drivers/gpu/drm/tegra/dc.h  |  1 +
>>>  drivers/gpu/drm/tegra/rgb.c | 49 +++++++++++++++++++++++++++++++++++--
>>>  3 files changed, 65 insertions(+), 12 deletions(-)
>>
>> This seems overly complicated. I especially don't like the way how
>> clocks are looked up with clk_get_sys() and then used in the comparison.
>> Could this not be achieved by using assigned-clocks and friends
>> properties in DT?
> 
> Assigned-clocks have nothing to do with this patch. We need to check
> whether PLLD *is* already pre-assigned as the parent.
> 
> Adding properties for describing the clk parents is overly complicated,
> clk_get_sys() is a much simpler solution that doesn't involve extra DT
> changes.
> 
> BTW, assigned-clocks can't be used for display controller  because
> controller is usually turned on during boot and reparenting of active DC
> will hang machine.
> 

To make it more clear, we change the DC's "parent" in TF700T device-tree
to PLLD [1] to match the bootloader's configuration, otherwise it will
hang on boot since default "parent" in tegra30.dtsi is PLLP, and thus,
tegra_dc_rgb_probe() will try to reparent it to PLLP. Display of TF700T
won't work with PLLP anyways since it needs a specific clock rate.

[1]
https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra30-asus-tf700t.dts#L13

Re: [PATCH v1 2/2] drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+

[Thierry Reding]

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

On Thu, Sep 30, 2021 at 01:28:05AM +0300, Dmitry Osipenko wrote:
> Asus Transformer TF700T is a Tegra30 tablet device which uses RGB->DSI
> bridge that requires a precise clock rate in order to operate properly.
> Tegra30 has a dedicated PLL for each display controller, hence the PLL
> rate can be changed freely. Allow PLL rate changes on Tegra30+ for RGB
> output. Configure the clock rate before display controller is enabled
> since DC itself may be running off this PLL and it's not okay to change
> the rate of the active PLL that doesn't support dynamic frequency
> switching since hardware will hang.
> 
> Tested-by: Maxim Schwalm <maxim.schwalm@gmail.com> #TF700T
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c  | 27 ++++++++++++--------
>  drivers/gpu/drm/tegra/dc.h  |  1 +
>  drivers/gpu/drm/tegra/rgb.c | 49 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 65 insertions(+), 12 deletions(-)

Applied, thanks.

Thierry

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