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

From: <Claudiu.Beznea@microchip.com>
To: <peda@axentia.se>, <sam@ravnborg.org>, <bbrezillon@kernel.org>,
	<airlied@linux.ie>, <daniel@ffwll.ch>,
	<Nicolas.Ferre@microchip.com>, <alexandre.belloni@bootlin.com>,
	<Ludovic.Desroches@microchip.com>, <lee.jones@linaro.org>
Cc: <dri-devel@lists.freedesktop.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested"
Date: Tue, 10 Dec 2019 14:59:27 +0000	[thread overview]
Message-ID: <5fbad2cd-0dbe-0be5-833a-f7a612d48012@microchip.com> (raw)
In-Reply-To: <4c3ffc48-7aa5-1e48-b0e9-a50c4eea7c38@axentia.se>

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