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(-)
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 net147@gmail.com --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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;
Hi,
On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
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 net147@gmail.com
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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...
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 ...
best_parent = rounded;
... that best_parent is going to be set there.
Maxime
Hi Maxime,
On 5 January 2018 at 06:56, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
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 net147@gmail.com
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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...
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 ...
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
On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote:
Hi Maxime,
On 5 January 2018 at 06:56, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
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 net147@gmail.com
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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...
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 ...
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.
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
Hi Maxime,
On 5 January 2018 at 21:03, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote:
On 5 January 2018 at 06:56, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
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 net147@gmail.com
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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...
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 ...
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.
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
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 net147@gmail.com --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 4d235e5ea31c..88eeeaf34638 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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; } } }
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 net147@gmail.com --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 88eeeaf34638..3ecffa52c814 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -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; } }
dri-devel@lists.freedesktop.org
-
Jonathan Liu -
Maxime Ripard