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

[<Claudiu.Beznea@microchip.com>]

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);
>>
>