[PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

[PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

[Jonathan Liu]

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

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

[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 <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

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

[Maxime Ripard]

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

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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

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

[Jonathan Liu]

Hi Maxime,

On 5 January 2018 at 06:56, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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

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

[Maxime Ripard]

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

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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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

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

[Jonathan Liu]

Hi Maxime,

On 5 January 2018 at 21:03, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  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

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

[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 <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

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

[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 <net147-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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