Hi Peter,
On Wed, 22 Aug 2018 13:50:47 +0200
Peter Rosin <peda@xxxxxxxxxx> wrote:
> Hi!
>
> I just discovered that the atmel-hlcdc driver picks a pixel-clock
> way outside the given range when used with a panel with these
> timings from the device tree.
>
> panel-timing {
> // 1024x768 @ 60Hz (typical)
> clock-frequency = <53350000 65000000 80000000>;
> hactive = <1024>;
> vactive = <768>;
> hfront-porch = <48 88 88>;
> hback-porch = <96 168 168>;
> hsync-len = <32 64 64>;
> vfront-porch = <8 13 14>;
> vback-porch = <8 13 14>;
> vsync-len = <8 12 14>;
> };
>
> IIUC, the atmel-hlcdc driver uses this code to set the pixel-clock
>
> cfg = 0;
>
> prate = clk_get_rate(crtc->dc->hlcdc->sys_clk);
> mode_rate = adj->crtc_clock * 1000;
> if ((prate / 2) < mode_rate) {
> prate *= 2;
> cfg |= ATMEL_HLCDC_CLKSEL;
> }
>
> div = DIV_ROUND_UP(prate, mode_rate);
> if (div < 2)
> div = 2;
>
> cfg |= ATMEL_HLCDC_CLKDIV(div);
>
> regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0),
> ATMEL_HLCDC_CLKSEL | ATMEL_HLCDC_CLKDIV_MASK |
> ATMEL_HLCDC_CLKPOL, cfg);
>
> I.e., the driver sets the divider so that the output frequency is as high
> as possible, but not above the target frequency, which seems sane enough.
> Until you realize that the target frequency is the typical frequency
> from the above device tree (i.e. 65MHz) without regard to the range of
> allowed frequencies.
Yes. I don't know what's the status now, but when I developed this
driver, the accepted range was not exposed. Now there's a way to define
that in the panel desc, but I'm not sure this information is propagated
to the drm_display_mode that is passed to the CRTC.
>
> In my case, which happens to be really unfortunate, sys_clk is running
> at 132MHz so the above results in div set to 3 which gives an output
> frequency of 44MHz. This is way outside the given 53.35-80Mhz range.
> Why was the mode allowed at all when the constraint was not met?
> Further, selecting a div of 2 gets 66MHz, which is really close to the
> target frequency and well inside the given range.
>
> Given the overall picture, selecting div = 3 seems totally buggy.
Absolutely.
>
> Sure, I can work around it by saying
> clock-frequency = <53350000 66000000 80000000>;
> but that is ... just an inappropriate workaround.
I agree. The proper solution would probably involve exposing
accepted timing ranges in drm_display_mode.
>
> Side note, while looking at the above code, I wonder why ATMEL_HLCDC_CLKSEL
> is not set most of the time, instead of only when it absolutely has to?
That sounds like a good idea. Feel free to propose a patch changing
that.
> Dividing the doubled frequency (which seems to be what the flag is doing)
> increases the accuracy of the output frequency. E.g. I would have gotten
> 52.8Mhz instead of 44MHz. So, still not inside the range, but much closer...
>
> Side note, there is no sanity check in the code for too large div. That
> will cause bits to be written outside the divider field in the register
> and an output frequency that is not what the driver intended. But it is
> probably not that important since div will probably be small most of the
> time?
Adding a check on div makes sense too.
Thanks for reporting the bug and proposing options to fix it.
Boris
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
