atmel-hlcdc select pixel-clock outside of given range

atmel-hlcdc select pixel-clock outside of given range

[Peter Rosin]

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.

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.

Sure, I can work around it by saying
		clock-frequency = <53350000 66000000 80000000>;
but that is ... just an inappropriate workaround.

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?
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?

Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: atmel-hlcdc select pixel-clock outside of given range

[Peter Rosin]

On 2018-08-22 13:50, Peter Rosin 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.
> 
> 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.
> 
> Sure, I can work around it by saying
> 		clock-frequency = <53350000 66000000 80000000>;
> but that is ... just an inappropriate workaround.
> 
> 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?
> 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 Thierry Reding since I'm dragging panel-lvds into this]

I dug a little deeper and this is apparently in no way the fault of the
atmel-hlcdc driver. It has no knowledge of the upper and lower bounds of
the given display-timings, and it simply can't be responsible for digging
them up.

Those constraints are thrown away in the panel-lvds driver just after they
are parsed in panel_lvds_parse_dt() with the call to videomode_from_timing().
Apparently, the panel-lvds driver is only ever going to report one supported
mode, with the typical timings, and has no way of forwarding the given
constraints that I see. So, what should it do?

IMHO, it is a serious design limitation that the constraints on the mode
cannot be propagated to the driver responsible for generating it.

So, given this, maybe it is not so obvious that you should always round
the pixel-clock divider up in the atmel-hlcdc driver? In my case it would
certainly have been better if the pixel-clock was rounded to the closes
possible divider even if that would have generated a pixel-clock frequency
1.5% higher than requested by the drm machinery...

Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: atmel-hlcdc select pixel-clock outside of given range

[Boris Brezillon]

Hi Peter,

On Wed, 22 Aug 2018 13:50:47 +0200
Peter Rosin <peda@axentia.se> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel