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

[Claudiu.Beznea]

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@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx 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@xxxxxxxxxx>
>>> 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@xxxxxxxxxxxxx>
>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>>> ---
>>>  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
>>>
>