Skip to contentSkip to conversation

drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

PATCH Superseded Jonathan Liu 2 participants 8 messages Dec 26, 2017 – Jan 5, 2018

Subsystems (4)
DRM drivers DRM misc Arm/Allwinner sunXi SoC support DRM drivers for Allwinner A10

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jonathan Liu

author
#0 lore raw

v2 cover drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

This patchset fixes several issues in sun4i_tmds_determine_rate that I discovered while trying to get a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz to display at its native resolution.

Changes for v2:
- Split into separate patches for each issue
- Add details to commit message for reproducing issue

Jonathan Liu (3):
drm/sun4i: hdmi: Check for unset best_parent in
sun4i_tmds_determine_rate
drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate drm/sun4i: hdmi: Add missing rate halving check in
sun4i_tmds_determine_rate

 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Signature · 1 line
2.15.0
author
#1 lore raw

PATCH v2 1/3 drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

We should check if the best match has been set before comparing it.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
index dc332ea56f6c..4d235e5ea31c 100644
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
 					goto out;
 				}
 
-				if (abs(rate - rounded / i) <
+				if (!best_parent || abs(rate - rounded / i) <
 				    abs(rate - best_parent / best_div)) {
 					best_parent = rounded;
 					best_div = i;
Signature · 1 line
2.15.0
#2 lore raw

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

Hi,

Quoted 13 lines · Jonathan Liu

We should check if the best match has been set before comparing it.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
index dc332ea56f6c..4d235e5ea31c 100644

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 105

Quoted 5 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
 					goto out;
 				}
 
-				if (abs(rate - rounded / i) <
+				if (!best_parent || abs(rate - rounded / i) <

Why is that causing any issue?

If best_parent is set to 0...

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 106

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
 				    abs(rate - best_parent / best_div)) {

... the value returned here is going to be rate, which is going to be higher than the first part of the comparison meaning ...

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 107

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
 					best_parent = rounded;

... that best_parent is going to be set there.

Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#3 lore raw

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

Hi Maxime,

Quoted 24 lines · Maxime Ripard
Quoted 13 lines · Jonathan Liu

We should check if the best match has been set before comparing it.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
index dc332ea56f6c..4d235e5ea31c 100644

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 105

Quoted 5 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
                                      goto out;
                              }
 
-                             if (abs(rate - rounded / i) <
+                             if (!best_parent || abs(rate - rounded / i) <

Why is that causing any issue?

If best_parent is set to 0...

Quoted 8 lines

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 106

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                  abs(rate - best_parent / best_div)) {

... the value returned here is going to be rate, which is going to be higher than the first part of the comparison meaning ...

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 107

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                      best_parent = rounded;

... that best_parent is going to be set there.

Consider the following:
rate = 83500000
rounded = ideal * 2

It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met.

Then you can end up with:
req->rate = 0
req->best_parent_rate = 0
req->best_parent_hw = ...

Also, the sun4i_tmds_calc_divider function has a similar check.

Regards,
Jonathan

#4 lore raw

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

Quoted 45 lines · Jonathan Liu

Hi Maxime,

Quoted 24 lines · Maxime Ripard
Quoted 13 lines · Jonathan Liu

We should check if the best match has been set before comparing it.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
index dc332ea56f6c..4d235e5ea31c 100644

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 105

Quoted 5 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
                                      goto out;
                              }
 
-                             if (abs(rate - rounded / i) <
+                             if (!best_parent || abs(rate - rounded / i) <

Why is that causing any issue?

If best_parent is set to 0...

Quoted 8 lines

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 106

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                  abs(rate - best_parent / best_div)) {

... the value returned here is going to be rate, which is going to be higher than the first part of the comparison meaning ...

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 107

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                      best_parent = rounded;

... that best_parent is going to be set there.

Consider the following:
rate = 83500000
rounded = ideal * 2

It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met.

Quoted 6 lines

Then you can end up with:
req->rate = 0
req->best_parent_rate = 0
req->best_parent_hw = ...

Also, the sun4i_tmds_calc_divider function has a similar check.

Ok. That explanation must be part of your commit log, or at least which problem you're trying to address and in which situation it will trigger.

Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#5 lore raw

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

Hi Maxime,

Quoted 49 lines · Maxime Ripard
Quoted 48 lines · Jonathan Liu
Quoted 24 lines · Maxime Ripard
Quoted 13 lines · Jonathan Liu

We should check if the best match has been set before comparing it.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
index dc332ea56f6c..4d235e5ea31c 100644

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 105

Quoted 5 lines
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
                                      goto out;
                              }
 
-                             if (abs(rate - rounded / i) <
+                             if (!best_parent || abs(rate - rounded / i) <

Why is that causing any issue?

If best_parent is set to 0...

Quoted 8 lines

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 106

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                  abs(rate - best_parent / best_div)) {

... the value returned here is going to be rate, which is going to be higher than the first part of the comparison meaning ...

drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c static int sun4i_tmds_determine_rate(struct clk_hw *hw, · line 107

Quoted 1 line
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +0 -0
                                      best_parent = rounded;

... that best_parent is going to be set there.

Consider the following:
rate = 83500000
rounded = ideal * 2

It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met.

Then you can end up with:
req->rate = 0
req->best_parent_rate = 0
req->best_parent_hw = ...

Quoted 5 lines
Quoted 1 line

Also, the sun4i_tmds_calc_divider function has a similar check.

Ok. That explanation must be part of your commit log, or at least which problem you're trying to address and in which situation it will trigger.

Ok. I have added amended the commit message with explanation for v3.

Regards,
Jonathan

author
#6 lore raw

PATCH v2 2/3 drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate

In reply to Jonathan Liu

best_div is set to i which corresponds to rate halving when it should be set to j which corresponds to the divider.

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +1 -1
index 4d235e5ea31c..88eeeaf34638 100644
@@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
 				if (!best_parent || abs(rate - rounded / i) <
 				    abs(rate - best_parent / best_div)) {
 					best_parent = rounded;
-					best_div = i;
+					best_div = j;
 				}
 			}
 		}
Signature · 1 line
2.15.0
author
#7 lore raw

PATCH v2 3/3 drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate

In reply to Jonathan Liu

It was only checking the divider when determing the closest match if it could not match the requested rate exactly.

For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI
with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this
resulted in 1280x800 mode not being available and the following in dmesg
when the kernel is booted with drm.debug=0x3e:
[drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 1480 1680 800 810 816 831 0x48 0x5 [drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK

Fixes 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by Jonathan Liu <[email protected]>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +5 -2
index 88eeeaf34638..3ecffa52c814 100644
@@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
 					goto out;
 				}
 
-				if (!best_parent || abs(rate - rounded / i) <
-				    abs(rate - best_parent / best_div)) {
+				if (!best_parent ||
+				    abs(rate - rounded / i / j) <
+				    abs(rate - best_parent / best_half /
+					best_div)) {
 					best_parent = rounded;
+					best_half = i;
 					best_div = j;
 				}
 			}
Signature · 1 line
2.15.0