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(-)
--
2.15.0
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(-)
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;
}
}
}
--
2.15.0
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(-)
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;
}
}
--
2.15.0
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(-)
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;
--
2.15.0
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 <[email protected]>
> ---
> 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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hi Maxime,
On 5 January 2018 at 06:56, Maxime Ripard
<[email protected]> 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 <[email protected]>
>> ---
>> 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
> <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hi Maxime,
On 5 January 2018 at 21:03, Maxime Ripard
<[email protected]> wrote:
> On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote:
>> On 5 January 2018 at 06:56, Maxime Ripard
>> <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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