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@xxxxxxxxxx>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>> 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);
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
