On 2019-12-13 10:28, Claudiu.Beznea@xxxxxxxxxxxxx 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@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? It would be awesome if you shepherd it for me, thanks! Cheers, Peter _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel