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. 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. 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. 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. > /* > * At least 10 times better when using a higher > * frequency than requested, instead of a lower. > -- > 2.20.1 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel