[4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
diff mbox series

Message ID 1575984287-26787-5-git-send-email-claudiu.beznea@microchip.com
State New
Headers show
Series
  • fixes for atmel-hlcdc
Related show

Commit Message

Claudiu Beznea Dec. 10, 2019, 1:24 p.m. UTC
This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
because allowing selecting a higher pixel clock may overclock
LCD devices, not all of them being capable of this.

Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------
 1 file changed, 12 deletions(-)

Comments

Peter Rosin Dec. 10, 2019, 2:11 p.m. UTC | #1
On 2019-12-10 14:24, Claudiu Beznea wrote:
> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
> because allowing selecting a higher pixel clock may overclock
> LCD devices, not all of them being capable of this.

Without this patch, there are panels that are *severly* underclocked (on the
magnitude of 40MHz instead of 65MHz or something like that, I don't remember
the exact figures). And they are of course not capable of that. All panels
have *some* slack as to what frequencies are supported, and the patch was
written under the assumption that the preferred frequency of the panel was
requested, which should leave at least a *little* headroom.

So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
and what it gets with/without the patch?

Or is the revert based on some theory of a perceived risk of toasting a panel?

In short, this revert regresses my use case and I would like at least a hook to
re-enable the removed logic.

Cheers,
Peter

> Cc: Peter Rosin <peda@axentia.se>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 721fa88bf71d..1a70dff1a417 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  		div = DIV_ROUND_UP(prate, mode_rate);
>  		if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
>  			div = ATMEL_HLCDC_CLKDIV_MASK;
> -	} else {
> -		int div_low = prate / mode_rate;
> -
> -		if (div_low >= 2 &&
> -		    ((prate / div_low - mode_rate) <
> -		     10 * (mode_rate - prate / div)))
> -			/*
> -			 * At least 10 times better when using a higher
> -			 * frequency than requested, instead of a lower.
> -			 * So, go with that.
> -			 */
> -			div = div_low;
>  	}
>  
>  	cfg |= ATMEL_HLCDC_CLKDIV(div);
>
Claudiu Beznea Dec. 10, 2019, 2:59 p.m. UTC | #2
On 10.12.2019 16:11, Peter Rosin wrote:
> On 2019-12-10 14:24, Claudiu Beznea wrote:
>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>> because allowing selecting a higher pixel clock may overclock
>> LCD devices, not all of them being capable of this.
> 
> Without this patch, there are panels that are *severly* underclocked (on the
> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
> the exact figures). 

With patch that switches by default to 2xsystem clock for pixel clock, if
using 133MHz system clock (as you specified in the patch I proposed for
revert here) that would go, without this patch at 53MHz if 65MHz is
requested. Correct me if I'm wrong.

> And they are of course not capable of that. All panels
> have *some* slack as to what frequencies are supported, and the patch was
> written under the assumption that the preferred frequency of the panel was
> requested, which should leave at least a *little* headroom.

I see, but from my point of view, the upper layers should decide what
frequency settings should be done on the LCD controller and not let this at
 the driver's latitude.

> 
> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
> and what it gets with/without the patch?

I have 2 use cases:
1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
the reverted patch the resulted computed pixel clock would be 80MHz.
Previously it was at 66MHz
2/ system clock = 133MHz and the requested pixel clock (mode_rate) 60MHz.
With the reverted patch the computed pixel clock would be 66MHz.

I took into account the patch that uses by default 2xsystem clock as pixel
clock (and this was on a system that supported it).

> 
> Or is the revert based on some theory of a perceived risk of toasting a panel?

It's based on the use cases I mentioned above.

> 
> In short, this revert regresses my use case and I would like at least a hook to
> re-enable the removed logic.

I see, but, FMPOV, you have to take into account that some of the devices
don't support it.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Peter
> 
>> Cc: Peter Rosin <peda@axentia.se>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 721fa88bf71d..1a70dff1a417 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>               div = DIV_ROUND_UP(prate, mode_rate);
>>               if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
>>                       div = ATMEL_HLCDC_CLKDIV_MASK;
>> -     } else {
>> -             int div_low = prate / mode_rate;
>> -
>> -             if (div_low >= 2 &&
>> -                 ((prate / div_low - mode_rate) <
>> -                  10 * (mode_rate - prate / div)))
>> -                     /*
>> -                      * At least 10 times better when using a higher
>> -                      * frequency than requested, instead of a lower.
>> -                      * So, go with that.
>> -                      */
>> -                     div = div_low;
>>       }
>>
>>       cfg |= ATMEL_HLCDC_CLKDIV(div);
>>
>
Peter Rosin Dec. 10, 2019, 5:22 p.m. UTC | #3
On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 10.12.2019 16:11, Peter Rosin wrote:
>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>> because allowing selecting a higher pixel clock may overclock
>>> LCD devices, not all of them being capable of this.
>>
>> Without this patch, there are panels that are *severly* underclocked (on the
>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>> the exact figures). 
> 
> With patch that switches by default to 2xsystem clock for pixel clock, if
> using 133MHz system clock (as you specified in the patch I proposed for
> revert here) that would go, without this patch at 53MHz if 65MHz is
> requested. Correct me if I'm wrong.

It might have been 53MHz, whatever it was it was too low for things to work.

>> And they are of course not capable of that. All panels
>> have *some* slack as to what frequencies are supported, and the patch was
>> written under the assumption that the preferred frequency of the panel was
>> requested, which should leave at least a *little* headroom.
> 
> I see, but from my point of view, the upper layers should decide what
> frequency settings should be done on the LCD controller and not let this at
>  the driver's latitude.

Right, but the upper layers do not support negotiating a frequency from
ranges. At least the didn't when the patch was written, and implementing
*that* seemed like a huge undertaking.

>>
>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
>> and what it gets with/without the patch?
> 
> I have 2 use cases:
> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
> the reverted patch the resulted computed pixel clock would be 80MHz.
> Previously it was at 66MHz

I don't see how that's possible.

[doing some calculation by hand]

Arrgh. *blush*

The code does not do what I intended for it to do.
Can you please try this instead of reverting?

Cheers,
Peter

From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>
Date: Tue, 10 Dec 2019 18:11:28 +0100
Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested

The intention was to only select a higher pixel-clock rate than the
requested, if a slight overclocking would result in a rate significantly
closer to the requested rate than if the conservative lower pixel-clock
rate is selected. The fixed patch has the logic the other way around and
actually prefers the higher frequency. Fix that.

Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 9e34bce089d0..03691845d37a 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 		int div_low = prate / mode_rate;
 
 		if (div_low >= 2 &&
-		    ((prate / div_low - mode_rate) <
-		     10 * (mode_rate - prate / div)))
+		    (10 * (prate / div_low - mode_rate) <
+		     (mode_rate - prate / div)))
 			/*
 			 * At least 10 times better when using a higher
 			 * frequency than requested, instead of a lower.
Claudiu Beznea Dec. 11, 2019, 11:45 a.m. UTC | #4
On 10.12.2019 19:22, Peter Rosin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote:
>>
>>
>> On 10.12.2019 16:11, Peter Rosin wrote:
>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>> because allowing selecting a higher pixel clock may overclock
>>>> LCD devices, not all of them being capable of this.
>>>
>>> Without this patch, there are panels that are *severly* underclocked (on the
>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>>> the exact figures).
>>
>> With patch that switches by default to 2xsystem clock for pixel clock, if
>> using 133MHz system clock (as you specified in the patch I proposed for
>> revert here) that would go, without this patch at 53MHz if 65MHz is
>> requested. Correct me if I'm wrong.
> 
> It might have been 53MHz, whatever it was it was too low for things to work.
> 
>>> And they are of course not capable of that. All panels
>>> have *some* slack as to what frequencies are supported, and the patch was
>>> written under the assumption that the preferred frequency of the panel was
>>> requested, which should leave at least a *little* headroom.
>>
>> I see, but from my point of view, the upper layers should decide what
>> frequency settings should be done on the LCD controller and not let this at
>>  the driver's latitude.
> 
> Right, but the upper layers do not support negotiating a frequency from
> ranges. At least the didn't when the patch was written, and implementing
> *that* seemed like a huge undertaking.
> 
>>>
>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
>>> and what it gets with/without the patch?
>>
>> I have 2 use cases:
>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
>> the reverted patch the resulted computed pixel clock would be 80MHz.
>> Previously it was at 66MHz
> 
> I don't see how that's possible.
> 
> [doing some calculation by hand]
> 
> Arrgh. *blush*
> 
> The code does not do what I intended for it to do.
> Can you please try this instead of reverting?
> 
> Cheers,
> Peter
> 
> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> Date: Tue, 10 Dec 2019 18:11:28 +0100
> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
> 
> The intention was to only select a higher pixel-clock rate than the
> requested, if a slight overclocking would result in a rate significantly
> closer to the requested rate than if the conservative lower pixel-clock
> rate is selected. The fixed patch has the logic the other way around and
> actually prefers the higher frequency. Fix that.
> 
> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 9e34bce089d0..03691845d37a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>                 int div_low = prate / mode_rate;
> 
>                 if (div_low >= 2 &&
> -                   ((prate / div_low - mode_rate) <
> -                    10 * (mode_rate - prate / div)))
> +                   (10 * (prate / div_low - mode_rate) <
> +                    (mode_rate - prate / div)))

I tested it on my setup (I have only one of those specified above) and it
is OK. Doing some math for the other setup it should also be OK.

As a whole, I'm OK with this at the moment (let's hope it will work for all
use-cases) but still I am not OK with selecting here, in the driver,
something that might work. Although I am not familiar with how other DRM
drivers are handling this kind of scenarios. Maybe you and/or other DRM
guys knows more about it.

Just as a notice, it may worth adding a print message saying what was
frequency was requested and what frequency has been setup by driver.

>                         /*
>                          * At least 10 times better when using a higher
>                          * frequency than requested, instead of a lower.
> --
> 2.20.1
>
Peter Rosin Dec. 11, 2019, 1:28 p.m. UTC | #5
On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 10.12.2019 19:22, Peter Rosin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote:
>>>
>>>
>>> On 10.12.2019 16:11, Peter Rosin wrote:
>>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>>> because allowing selecting a higher pixel clock may overclock
>>>>> LCD devices, not all of them being capable of this.
>>>>
>>>> Without this patch, there are panels that are *severly* underclocked (on the
>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>>>> the exact figures).
>>>
>>> With patch that switches by default to 2xsystem clock for pixel clock, if
>>> using 133MHz system clock (as you specified in the patch I proposed for
>>> revert here) that would go, without this patch at 53MHz if 65MHz is
>>> requested. Correct me if I'm wrong.
>>
>> It might have been 53MHz, whatever it was it was too low for things to work.
>>
>>>> And they are of course not capable of that. All panels
>>>> have *some* slack as to what frequencies are supported, and the patch was
>>>> written under the assumption that the preferred frequency of the panel was
>>>> requested, which should leave at least a *little* headroom.
>>>
>>> I see, but from my point of view, the upper layers should decide what
>>> frequency settings should be done on the LCD controller and not let this at
>>>  the driver's latitude.
>>
>> Right, but the upper layers do not support negotiating a frequency from
>> ranges. At least the didn't when the patch was written, and implementing
>> *that* seemed like a huge undertaking.
>>
>>>>
>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
>>>> and what it gets with/without the patch?
>>>
>>> I have 2 use cases:
>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
>>> the reverted patch the resulted computed pixel clock would be 80MHz.
>>> Previously it was at 66MHz
>>
>> I don't see how that's possible.
>>
>> [doing some calculation by hand]
>>
>> Arrgh. *blush*
>>
>> The code does not do what I intended for it to do.
>> Can you please try this instead of reverting?
>>
>> Cheers,
>> Peter
>>
>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
>> From: Peter Rosin <peda@axentia.se>
>> Date: Tue, 10 Dec 2019 18:11:28 +0100
>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
>>
>> The intention was to only select a higher pixel-clock rate than the
>> requested, if a slight overclocking would result in a rate significantly
>> closer to the requested rate than if the conservative lower pixel-clock
>> rate is selected. The fixed patch has the logic the other way around and
>> actually prefers the higher frequency. Fix that.
>>
>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 9e34bce089d0..03691845d37a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>                 int div_low = prate / mode_rate;
>>
>>                 if (div_low >= 2 &&
>> -                   ((prate / div_low - mode_rate) <
>> -                    10 * (mode_rate - prate / div)))
>> +                   (10 * (prate / div_low - mode_rate) <
>> +                    (mode_rate - prate / div)))
> 
> I tested it on my setup (I have only one of those specified above) and it
> is OK. Doing some math for the other setup it should also be OK.

Glad to hear it, and thanks for testing/verifying!

> As a whole, I'm OK with this at the moment (let's hope it will work for all
> use-cases) but still I am not OK with selecting here, in the driver,
> something that might work.

The driver has to select *something*. If it can deliver the exact requested
frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better
and more likely to produce a picture than 66MHz, when 65MHz is requested?
That's of course an impossible question for the driver to answer.

So, if you are not ok with that, you need to implement something that uses
the min/max fields from the various fields inside struct display_timing
instead of only looking at the typ field. E.g. the panel_lvds driver calls
videomode_from_timings() and the result is a single possible mode with only
the typical timings, with no negotiation of the best option within the
given ranges with the other drivers involved with the pipe. I think the
panel-simple driver also makes this one-sided decision of only making use
of the typ field for each given timing range. Having dabbled a bit in what
the sound stack does to negotiate the sample rate, sample format and
channel count etc, I can only predict that retrofitting something like that
for video modes will be ... interesting. Which is probably why it's not
done at all, at least not in the general case.

And yes, I agree, the current mechanics are less than ideal. But I have no
time to do anything about it.

>                            Although I am not familiar with how other DRM
> drivers are handling this kind of scenarios. Maybe you and/or other DRM
> guys knows more about it.

I don't know (and I mean it literally), but maybe these chips are special
as they typically end up with very small dividers and thus large frequency
steps? BTW, I do not consider myself a DRM guy, I have only tried to
fix that which did not work out for our needs...

> Just as a notice, it may worth adding a print message saying what was
> frequency was requested and what frequency has been setup by driver.

I have no problem with that.

Cheers,
Peter

> 
>>                         /*
>>                          * At least 10 times better when using a higher
>>                          * frequency than requested, instead of a lower.
>> --
>> 2.20.1
>>
Claudiu Beznea Dec. 13, 2019, 9:28 a.m. UTC | #6
On 11.12.2019 15:28, Peter Rosin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote:
>>
>>
>> On 10.12.2019 19:22, Peter Rosin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote:
>>>>
>>>>
>>>> On 10.12.2019 16:11, Peter Rosin wrote:
>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>>>> because allowing selecting a higher pixel clock may overclock
>>>>>> LCD devices, not all of them being capable of this.
>>>>>
>>>>> Without this patch, there are panels that are *severly* underclocked (on the
>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>>>>> the exact figures).
>>>>
>>>> With patch that switches by default to 2xsystem clock for pixel clock, if
>>>> using 133MHz system clock (as you specified in the patch I proposed for
>>>> revert here) that would go, without this patch at 53MHz if 65MHz is
>>>> requested. Correct me if I'm wrong.
>>>
>>> It might have been 53MHz, whatever it was it was too low for things to work.
>>>
>>>>> And they are of course not capable of that. All panels
>>>>> have *some* slack as to what frequencies are supported, and the patch was
>>>>> written under the assumption that the preferred frequency of the panel was
>>>>> requested, which should leave at least a *little* headroom.
>>>>
>>>> I see, but from my point of view, the upper layers should decide what
>>>> frequency settings should be done on the LCD controller and not let this at
>>>>  the driver's latitude.
>>>
>>> Right, but the upper layers do not support negotiating a frequency from
>>> ranges. At least the didn't when the patch was written, and implementing
>>> *that* seemed like a huge undertaking.
>>>
>>>>>
>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
>>>>> and what it gets with/without the patch?
>>>>
>>>> I have 2 use cases:
>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
>>>> the reverted patch the resulted computed pixel clock would be 80MHz.
>>>> Previously it was at 66MHz
>>>
>>> I don't see how that's possible.
>>>
>>> [doing some calculation by hand]
>>>
>>> Arrgh. *blush*
>>>
>>> The code does not do what I intended for it to do.
>>> Can you please try this instead of reverting?
>>>
>>> Cheers,
>>> Peter
>>>
>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
>>> From: Peter Rosin <peda@axentia.se>
>>> Date: Tue, 10 Dec 2019 18:11:28 +0100
>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
>>>
>>> The intention was to only select a higher pixel-clock rate than the
>>> requested, if a slight overclocking would result in a rate significantly
>>> closer to the requested rate than if the conservative lower pixel-clock
>>> rate is selected. The fixed patch has the logic the other way around and
>>> actually prefers the higher frequency. Fix that.
>>>
>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>> index 9e34bce089d0..03691845d37a 100644
>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>>                 int div_low = prate / mode_rate;
>>>
>>>                 if (div_low >= 2 &&
>>> -                   ((prate / div_low - mode_rate) <
>>> -                    10 * (mode_rate - prate / div)))
>>> +                   (10 * (prate / div_low - mode_rate) <
>>> +                    (mode_rate - prate / div)))
>>
>> I tested it on my setup (I have only one of those specified above) and it
>> is OK. Doing some math for the other setup it should also be OK.
> 
> Glad to hear it, and thanks for testing/verifying!
> 
>> As a whole, I'm OK with this at the moment (let's hope it will work for all
>> use-cases) but still I am not OK with selecting here, in the driver,
>> something that might work.
> 
> The driver has to select *something*. If it can deliver the exact requested
> frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better
> and more likely to produce a picture than 66MHz, when 65MHz is requested?
> That's of course an impossible question for the driver to answer.
> 
> So, if you are not ok with that, you need to implement something that uses
> the min/max fields from the various fields inside struct display_timing
> instead of only looking at the typ field. E.g. the panel_lvds driver calls
> videomode_from_timings() and the result is a single possible mode with only
> the typical timings, with no negotiation of the best option within the
> given ranges with the other drivers involved with the pipe. I think the
> panel-simple driver also makes this one-sided decision of only making use
> of the typ field for each given timing range. Having dabbled a bit in what
> the sound stack does to negotiate the sample rate, sample format and
> channel count etc, I can only predict that retrofitting something like that
> for video modes will be ... interesting. Which is probably why it's not
> done at all, at least not in the general case.
> 
> And yes, I agree, the current mechanics are less than ideal. But I have no
> time to do anything about it.
> 
>>                            Although I am not familiar with how other DRM
>> drivers are handling this kind of scenarios. Maybe you and/or other DRM
>> guys knows more about it.
> 
> I don't know (and I mean it literally), but maybe these chips are special
> as they typically end up with very small dividers and thus large frequency
> steps? BTW, I do not consider myself a DRM guy, I have only tried to
> fix that which did not work out for our needs...
> 
>> Just as a notice, it may worth adding a print message saying what was
>> frequency was requested and what frequency has been setup by driver.
> 
> I have no problem with that.

Hi Peter,

I intend to prepare my v2 of this series. How would you like to proceed
with the patch you provided? Are you OK if I add it to my v2 of this series
or would you prefer to send it on your own?

Thank you,
Claudiu Beznea

> 
> Cheers,
> Peter
> 
>>
>>>                         /*
>>>                          * At least 10 times better when using a higher
>>>                          * frequency than requested, instead of a lower.
>>> --
>>> 2.20.1
>>>
>
Peter Rosin Dec. 13, 2019, 9:30 a.m. UTC | #7
On 2019-12-13 10:28, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 11.12.2019 15:28, Peter Rosin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote:
>>>
>>>
>>> On 10.12.2019 19:22, Peter Rosin wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote:
>>>>>
>>>>>
>>>>> On 10.12.2019 16:11, Peter Rosin wrote:
>>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote:
>>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0.
>>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>>>>> because allowing selecting a higher pixel clock may overclock
>>>>>>> LCD devices, not all of them being capable of this.
>>>>>>
>>>>>> Without this patch, there are panels that are *severly* underclocked (on the
>>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember
>>>>>> the exact figures).
>>>>>
>>>>> With patch that switches by default to 2xsystem clock for pixel clock, if
>>>>> using 133MHz system clock (as you specified in the patch I proposed for
>>>>> revert here) that would go, without this patch at 53MHz if 65MHz is
>>>>> requested. Correct me if I'm wrong.
>>>>
>>>> It might have been 53MHz, whatever it was it was too low for things to work.
>>>>
>>>>>> And they are of course not capable of that. All panels
>>>>>> have *some* slack as to what frequencies are supported, and the patch was
>>>>>> written under the assumption that the preferred frequency of the panel was
>>>>>> requested, which should leave at least a *little* headroom.
>>>>>
>>>>> I see, but from my point of view, the upper layers should decide what
>>>>> frequency settings should be done on the LCD controller and not let this at
>>>>>  the driver's latitude.
>>>>
>>>> Right, but the upper layers do not support negotiating a frequency from
>>>> ranges. At least the didn't when the patch was written, and implementing
>>>> *that* seemed like a huge undertaking.
>>>>
>>>>>>
>>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs
>>>>>> and what it gets with/without the patch?
>>>>>
>>>>> I have 2 use cases:
>>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With
>>>>> the reverted patch the resulted computed pixel clock would be 80MHz.
>>>>> Previously it was at 66MHz
>>>>
>>>> I don't see how that's possible.
>>>>
>>>> [doing some calculation by hand]
>>>>
>>>> Arrgh. *blush*
>>>>
>>>> The code does not do what I intended for it to do.
>>>> Can you please try this instead of reverting?
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001
>>>> From: Peter Rosin <peda@axentia.se>
>>>> Date: Tue, 10 Dec 2019 18:11:28 +0100
>>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested
>>>>
>>>> The intention was to only select a higher pixel-clock rate than the
>>>> requested, if a slight overclocking would result in a rate significantly
>>>> closer to the requested rate than if the conservative lower pixel-clock
>>>> rate is selected. The fixed patch has the logic the other way around and
>>>> actually prefers the higher frequency. Fix that.
>>>>
>>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested")
>>>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> index 9e34bce089d0..03691845d37a 100644
>>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>>>                 int div_low = prate / mode_rate;
>>>>
>>>>                 if (div_low >= 2 &&
>>>> -                   ((prate / div_low - mode_rate) <
>>>> -                    10 * (mode_rate - prate / div)))
>>>> +                   (10 * (prate / div_low - mode_rate) <
>>>> +                    (mode_rate - prate / div)))
>>>
>>> I tested it on my setup (I have only one of those specified above) and it
>>> is OK. Doing some math for the other setup it should also be OK.
>>
>> Glad to hear it, and thanks for testing/verifying!
>>
>>> As a whole, I'm OK with this at the moment (let's hope it will work for all
>>> use-cases) but still I am not OK with selecting here, in the driver,
>>> something that might work.
>>
>> The driver has to select *something*. If it can deliver the exact requested
>> frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better
>> and more likely to produce a picture than 66MHz, when 65MHz is requested?
>> That's of course an impossible question for the driver to answer.
>>
>> So, if you are not ok with that, you need to implement something that uses
>> the min/max fields from the various fields inside struct display_timing
>> instead of only looking at the typ field. E.g. the panel_lvds driver calls
>> videomode_from_timings() and the result is a single possible mode with only
>> the typical timings, with no negotiation of the best option within the
>> given ranges with the other drivers involved with the pipe. I think the
>> panel-simple driver also makes this one-sided decision of only making use
>> of the typ field for each given timing range. Having dabbled a bit in what
>> the sound stack does to negotiate the sample rate, sample format and
>> channel count etc, I can only predict that retrofitting something like that
>> for video modes will be ... interesting. Which is probably why it's not
>> done at all, at least not in the general case.
>>
>> And yes, I agree, the current mechanics are less than ideal. But I have no
>> time to do anything about it.
>>
>>>                            Although I am not familiar with how other DRM
>>> drivers are handling this kind of scenarios. Maybe you and/or other DRM
>>> guys knows more about it.
>>
>> I don't know (and I mean it literally), but maybe these chips are special
>> as they typically end up with very small dividers and thus large frequency
>> steps? BTW, I do not consider myself a DRM guy, I have only tried to
>> fix that which did not work out for our needs...
>>
>>> Just as a notice, it may worth adding a print message saying what was
>>> frequency was requested and what frequency has been setup by driver.
>>
>> I have no problem with that.
> 
> Hi Peter,
> 
> I intend to prepare my v2 of this series. How would you like to proceed
> with the patch you provided? Are you OK if I add it to my v2 of this series
> or would you prefer to send it on your own?

It would be awesome if you shepherd it for me, thanks!

Cheers,
Peter

Patch
diff mbox series

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 721fa88bf71d..1a70dff1a417 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -117,18 +117,6 @@  static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 		div = DIV_ROUND_UP(prate, mode_rate);
 		if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
 			div = ATMEL_HLCDC_CLKDIV_MASK;
-	} else {
-		int div_low = prate / mode_rate;
-
-		if (div_low >= 2 &&
-		    ((prate / div_low - mode_rate) <
-		     10 * (mode_rate - prate / div)))
-			/*
-			 * At least 10 times better when using a higher
-			 * frequency than requested, instead of a lower.
-			 * So, go with that.
-			 */
-			div = div_low;
 	}
 
 	cfg |= ATMEL_HLCDC_CLKDIV(div);