[PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Chen-Yu Tsai]

The panels shipped with Allwinner devices are very "generic", i.e.
they do not have model numbers or reliable sources of information
for the timings (that we know of) other than the fex files shipped
on them. The dot clock frequency provided in the fex files have all
been rounded to the nearest MHz, as that is the unit used in them.

We were using the simple panel "urt,umsh-8596md-t" as a substitute
for the A13 Q8 tablets in the absence of a specific model for what
may be many different but otherwise timing compatible panels. This
was usable without any visual artifacts or side effects, until the
dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
rgb: Validate the clock rate").

The reason this check fails is because the dotclock frequency for
this model is 33.26 MHz, which is not achievable with our dot clock
hardware, and the rate returned by clk_round_rate deviates slightly,
causing the driver to reject the display mode.

The LCD panels have some tolerance on the dot clock frequency, even
if it's not specified in their datasheets.

This patch adds a 5% tolerence to the dot clock check.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---

The few LCD panel datasheets I found did not list minimums or maximums
for the dot clock rate. The 5% tolerance is just something I made up.
The point is to be able to use our dot clock, which doesn't have the
resolution needed to generate the exact clock rate requested. AFAIK
the sun4i driver is one of the strictest ones with regards to the dot
clock frequency. Some drivers don't even check it.

The clock rate given in vendor fex files are already rounded down to
MHz resolution. I doubt not using the exact rate as specified in simple
panels would cause any issues. But my experience is limited here.
Feedback on this is requested.

---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d198ad7e5323..66ad86afa561 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -93,11 +93,12 @@ static int sun4i_rgb_mode_valid(struct drm_connector *connector,
 
 	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
 
+	/* Check against a 5% tolerance for the dot clock */
 	rounded_rate = clk_round_rate(tcon->dclk, rate);
-	if (rounded_rate < rate)
+	if (rounded_rate < rate * 19 / 20)
 		return MODE_CLOCK_LOW;
 
-	if (rounded_rate > rate)
+	if (rounded_rate > rate * 21 / 20)
 		return MODE_CLOCK_HIGH;
 
 	DRM_DEBUG_DRIVER("Clock rate OK\n");
-- 
2.10.2

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> The panels shipped with Allwinner devices are very "generic", i.e.
> they do not have model numbers or reliable sources of information
> for the timings (that we know of) other than the fex files shipped
> on them. The dot clock frequency provided in the fex files have all
> been rounded to the nearest MHz, as that is the unit used in them.
> 
> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> for the A13 Q8 tablets in the absence of a specific model for what
> may be many different but otherwise timing compatible panels. This
> was usable without any visual artifacts or side effects, until the
> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> rgb: Validate the clock rate").
> 
> The reason this check fails is because the dotclock frequency for
> this model is 33.26 MHz, which is not achievable with our dot clock
> hardware, and the rate returned by clk_round_rate deviates slightly,
> causing the driver to reject the display mode.
> 
> The LCD panels have some tolerance on the dot clock frequency, even
> if it's not specified in their datasheets.
> 
> This patch adds a 5% tolerence to the dot clock check.

As we discussed already, I really believe this is just as arbitrary as
the current behaviour.

Some panels require an exact frequency, some have a minimal frequency
but no maximum, some have a maximum frequency but no minimal, and I
guess most of them deviates by how much exactly they can take (and
possibly can take more easily a higher frequency, but are less
tolerant if you take a frequency lower than the nominal.

And we cannot remove that check entirely, since some bridges will
report out of range frequencies for higher modes that we know we
cannot reach.

We could just try to see if the screen pixel clock frequency is out of
the pixel clock range we can generate, but then we will loop back on
how much out of range is it exactly, and is it within the screen
tolerancy.

We have an API to deal with the panel tolerancies in the DRM panel
framework, we can (and should) use it.

I'm not sure how others usually deal with this though. I think I
remember Eric telling me that for the RPi they just adjusted the
timings a bit, but they only really had a single panel to deal with.

Daniel, Eric, Laurent, Sean? Any ideas?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Chen-Yu Tsai]

On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
>> The panels shipped with Allwinner devices are very "generic", i.e.
>> they do not have model numbers or reliable sources of information
>> for the timings (that we know of) other than the fex files shipped
>> on them. The dot clock frequency provided in the fex files have all
>> been rounded to the nearest MHz, as that is the unit used in them.
>>
>> We were using the simple panel "urt,umsh-8596md-t" as a substitute
>> for the A13 Q8 tablets in the absence of a specific model for what
>> may be many different but otherwise timing compatible panels. This
>> was usable without any visual artifacts or side effects, until the
>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
>> rgb: Validate the clock rate").
>>
>> The reason this check fails is because the dotclock frequency for
>> this model is 33.26 MHz, which is not achievable with our dot clock
>> hardware, and the rate returned by clk_round_rate deviates slightly,
>> causing the driver to reject the display mode.
>>
>> The LCD panels have some tolerance on the dot clock frequency, even
>> if it's not specified in their datasheets.
>>
>> This patch adds a 5% tolerence to the dot clock check.
>
> As we discussed already, I really believe this is just as arbitrary as
> the current behaviour.

Yes. I agree. This patch is mainly to give something that works for
people who don't care about the details, and to get some feedback
from people that do.

>
> Some panels require an exact frequency, some have a minimal frequency
> but no maximum, some have a maximum frequency but no minimal, and I
> guess most of them deviates by how much exactly they can take (and
> possibly can take more easily a higher frequency, but are less
> tolerant if you take a frequency lower than the nominal.
>
> And we cannot remove that check entirely, since some bridges will
> report out of range frequencies for higher modes that we know we
> cannot reach.

I believe this should be handled by the bridge driver in the check
callback? The callback I'm changing is attached to the connector,
which I think doesn't get used if you have a bridge instead.
And this only checks the pre-registered display modes, such as
those specified in simple-panel or EDID.

> We could just try to see if the screen pixel clock frequency is out of
> the pixel clock range we can generate, but then we will loop back on
> how much out of range is it exactly, and is it within the screen
> tolerancy.
>
> We have an API to deal with the panel tolerancies in the DRM panel
> framework, we can (and should) use it.

If you mean the get_timings callback, it's not very useful. Most of
the panels in simple-panel do not use the display_timings structure,
so they don't return anything. And I get that. The few datasheets
I found don't list min/max tolerances for the dotclock.

The ones that do have the min/max the same as the recommended value.
This may or may not be accurate. IIRC the one panel that had this
that I did check didn't list min/max values in its datasheet.

>
> I'm not sure how others usually deal with this though. I think I
> remember Eric telling me that for the RPi they just adjusted the
> timings a bit, but they only really had a single panel to deal with.
>
> Daniel, Eric, Laurent, Sean? Any ideas?

Yes! Feedback please! Between Maxime and me I think we only have a
limited number of panels, with some overlap.

Regards
ChenYu

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Laurent Pinchart]

Hello,

On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> >> The panels shipped with Allwinner devices are very "generic", i.e.
> >> they do not have model numbers or reliable sources of information
> >> for the timings (that we know of) other than the fex files shipped
> >> on them. The dot clock frequency provided in the fex files have all
> >> been rounded to the nearest MHz, as that is the unit used in them.
> >> 
> >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> >> for the A13 Q8 tablets in the absence of a specific model for what
> >> may be many different but otherwise timing compatible panels. This
> >> was usable without any visual artifacts or side effects, until the
> >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> >> rgb: Validate the clock rate").
> >> 
> >> The reason this check fails is because the dotclock frequency for
> >> this model is 33.26 MHz, which is not achievable with our dot clock
> >> hardware, and the rate returned by clk_round_rate deviates slightly,
> >> causing the driver to reject the display mode.
> >> 
> >> The LCD panels have some tolerance on the dot clock frequency, even
> >> if it's not specified in their datasheets.
> >> 
> >> This patch adds a 5% tolerence to the dot clock check.
> > 
> > As we discussed already, I really believe this is just as arbitrary as
> > the current behaviour.
> 
> Yes. I agree. This patch is mainly to give something that works for
> people who don't care about the details, and to get some feedback
> from people that do.
> 
> > Some panels require an exact frequency,

There's no such thing as an exact frequency, there will always be some 
tolerance (and if your display controller can really generate an exact 
frequency I'd be very interested in that hardware :-)).

This is something that has been bugging me for some time now. The problem has 
been mostly ignored, or worked around in different ways by different drivers. 
I'm afraid I have no generic solution available, but I think we should try to 
agree on a common behaviour.

I don't believe it would be reasonable to request each panel to report a 
tolerance, as the value is most of the time not available from the 
documentation (when documentation is available). Worse, I'm pretty sure that 
most panels documented as fixed timing can actually accept a wide range of 
timings. The timings reported in the datasheet are just the nominal values.

Panels that don't support multiple resolutions obviously require fixed active 
h/v values. Even if they can tolerate some departure from the nominal timings 
for the sync and porches lengths, it might not be very useful to support that 
as I don't expect the display controllers and encoders to be a limiting factor 
by not supporting the particular timings that a panel considers as nominal. On 
the other hand, departing from the nominal pixel clock frequency is needed as 
we can't achieve an exact match, and even possibly to have some control over 
the frame rate (although that might also require changing the sync and porches 
timings). Without specific information about panel tolerance, do we have any 
option other than picking an arbitrary value ?

> > some have a minimal frequency
> > but no maximum, some have a maximum frequency but no minimal, and I
> > guess most of them deviates by how much exactly they can take (and
> > possibly can take more easily a higher frequency, but are less
> > tolerant if you take a frequency lower than the nominal.
> > 
> > And we cannot remove that check entirely, since some bridges will
> > report out of range frequencies for higher modes that we know we
> > cannot reach.
> 
> I believe this should be handled by the bridge driver in the check
> callback? The callback I'm changing is attached to the connector,
> which I think doesn't get used if you have a bridge instead.
> And this only checks the pre-registered display modes, such as
> those specified in simple-panel or EDID.
> 
> > We could just try to see if the screen pixel clock frequency is out of
> > the pixel clock range we can generate, but then we will loop back on
> > how much out of range is it exactly, and is it within the screen
> > tolerancy.
> > 
> > We have an API to deal with the panel tolerancies in the DRM panel
> > framework, we can (and should) use it.
> 
> If you mean the get_timings callback, it's not very useful. Most of
> the panels in simple-panel do not use the display_timings structure,
> so they don't return anything. And I get that. The few datasheets
> I found don't list min/max tolerances for the dotclock.
> 
> The ones that do have the min/max the same as the recommended value.
> This may or may not be accurate. IIRC the one panel that had this
> that I did check didn't list min/max values in its datasheet.
> 
> > I'm not sure how others usually deal with this though. I think I
> > remember Eric telling me that for the RPi they just adjusted the
> > timings a bit, but they only really had a single panel to deal with.
> > 
> > Daniel, Eric, Laurent, Sean? Any ideas?
> 
> Yes! Feedback please! Between Maxime and me I think we only have a
> limited number of panels, with some overlap.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4119 bytes --]

Moi,

On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> > >> The panels shipped with Allwinner devices are very "generic", i.e.
> > >> they do not have model numbers or reliable sources of information
> > >> for the timings (that we know of) other than the fex files shipped
> > >> on them. The dot clock frequency provided in the fex files have all
> > >> been rounded to the nearest MHz, as that is the unit used in them.
> > >> 
> > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > >> for the A13 Q8 tablets in the absence of a specific model for what
> > >> may be many different but otherwise timing compatible panels. This
> > >> was usable without any visual artifacts or side effects, until the
> > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > >> rgb: Validate the clock rate").
> > >> 
> > >> The reason this check fails is because the dotclock frequency for
> > >> this model is 33.26 MHz, which is not achievable with our dot clock
> > >> hardware, and the rate returned by clk_round_rate deviates slightly,
> > >> causing the driver to reject the display mode.
> > >> 
> > >> The LCD panels have some tolerance on the dot clock frequency, even
> > >> if it's not specified in their datasheets.
> > >> 
> > >> This patch adds a 5% tolerence to the dot clock check.
> > > 
> > > As we discussed already, I really believe this is just as arbitrary as
> > > the current behaviour.
> > 
> > Yes. I agree. This patch is mainly to give something that works for
> > people who don't care about the details, and to get some feedback
> > from people that do.
> > 
> > > Some panels require an exact frequency,
> 
> There's no such thing as an exact frequency, there will always be some 
> tolerance (and if your display controller can really generate an exact 
> frequency I'd be very interested in that hardware :-)).
> 
> This is something that has been bugging me for some time now. The problem has 
> been mostly ignored, or worked around in different ways by different drivers. 
> I'm afraid I have no generic solution available, but I think we should try to 
> agree on a common behaviour.
> 
> I don't believe it would be reasonable to request each panel to report a 
> tolerance, as the value is most of the time not available from the 
> documentation (when documentation is available). Worse, I'm pretty sure that 
> most panels documented as fixed timing can actually accept a wide range of 
> timings. The timings reported in the datasheet are just the nominal values.
> 
> Panels that don't support multiple resolutions obviously require fixed active 
> h/v values. Even if they can tolerate some departure from the nominal timings 
> for the sync and porches lengths, it might not be very useful to support that 
> as I don't expect the display controllers and encoders to be a limiting factor 
> by not supporting the particular timings that a panel considers as nominal. On 
> the other hand, departing from the nominal pixel clock frequency is needed as 
> we can't achieve an exact match, and even possibly to have some control over 
> the frame rate (although that might also require changing the sync and porches 
> timings). Without specific information about panel tolerance, do we have any 
> option other than picking an arbitrary value ?

If you consider only panels, yes, chances are the EE picked a panel
that has a decent chance to work (especially since most of the boards
we support are consumer electronics products, and people like to have
a panel that works on their tablet).

However, bridges are a different story, and provide on some SoCs modes
that are way out of reach for our pixel clock, which is why we had
that test in the first place.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Sean Paul]

On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> > >> The panels shipped with Allwinner devices are very "generic", i.e.
> > >> they do not have model numbers or reliable sources of information
> > >> for the timings (that we know of) other than the fex files shipped
> > >> on them. The dot clock frequency provided in the fex files have all
> > >> been rounded to the nearest MHz, as that is the unit used in them.
> > >> 
> > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > >> for the A13 Q8 tablets in the absence of a specific model for what
> > >> may be many different but otherwise timing compatible panels. This
> > >> was usable without any visual artifacts or side effects, until the
> > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > >> rgb: Validate the clock rate").
> > >> 
> > >> The reason this check fails is because the dotclock frequency for
> > >> this model is 33.26 MHz, which is not achievable with our dot clock
> > >> hardware, and the rate returned by clk_round_rate deviates slightly,
> > >> causing the driver to reject the display mode.
> > >> 
> > >> The LCD panels have some tolerance on the dot clock frequency, even
> > >> if it's not specified in their datasheets.
> > >> 
> > >> This patch adds a 5% tolerence to the dot clock check.
> > > 
> > > As we discussed already, I really believe this is just as arbitrary as
> > > the current behaviour.
> > 
> > Yes. I agree. This patch is mainly to give something that works for
> > people who don't care about the details, and to get some feedback
> > from people that do.
> > 
> > > Some panels require an exact frequency,
> 
> There's no such thing as an exact frequency, there will always be some 
> tolerance (and if your display controller can really generate an exact 
> frequency I'd be very interested in that hardware :-)).

There is such thing as exact frequency when you have to worry about panel
warranty. We are fortunate, since we can talk to the panel manufacturer and
discuss which frequencies are tolerable inside the warranty. We usually hand
pick a rate/mode within these constraints.

Also, even though things *look* ok on the panel at a certain clock rate, running
outside the specified clock could damage the hardware.

I don't think it's unreasonable to add tolerances to drm_panel, since they will
differ in what is acceptable. The tricky part is teasing out what the tolerances
are.

Sean

> 
> This is something that has been bugging me for some time now. The problem has 
> been mostly ignored, or worked around in different ways by different drivers. 
> I'm afraid I have no generic solution available, but I think we should try to 
> agree on a common behaviour.
> 
> I don't believe it would be reasonable to request each panel to report a 
> tolerance, as the value is most of the time not available from the 
> documentation (when documentation is available). Worse, I'm pretty sure that 
> most panels documented as fixed timing can actually accept a wide range of 
> timings. The timings reported in the datasheet are just the nominal values.
> 
> Panels that don't support multiple resolutions obviously require fixed active 
> h/v values. Even if they can tolerate some departure from the nominal timings 
> for the sync and porches lengths, it might not be very useful to support that 
> as I don't expect the display controllers and encoders to be a limiting factor 
> by not supporting the particular timings that a panel considers as nominal. On 
> the other hand, departing from the nominal pixel clock frequency is needed as 
> we can't achieve an exact match, and even possibly to have some control over 
> the frame rate (although that might also require changing the sync and porches 
> timings). Without specific information about panel tolerance, do we have any 
> option other than picking an arbitrary value ?
> 
> > > some have a minimal frequency
> > > but no maximum, some have a maximum frequency but no minimal, and I
> > > guess most of them deviates by how much exactly they can take (and
> > > possibly can take more easily a higher frequency, but are less
> > > tolerant if you take a frequency lower than the nominal.
> > > 
> > > And we cannot remove that check entirely, since some bridges will
> > > report out of range frequencies for higher modes that we know we
> > > cannot reach.
> > 
> > I believe this should be handled by the bridge driver in the check
> > callback? The callback I'm changing is attached to the connector,
> > which I think doesn't get used if you have a bridge instead.
> > And this only checks the pre-registered display modes, such as
> > those specified in simple-panel or EDID.
> > 
> > > We could just try to see if the screen pixel clock frequency is out of
> > > the pixel clock range we can generate, but then we will loop back on
> > > how much out of range is it exactly, and is it within the screen
> > > tolerancy.
> > > 
> > > We have an API to deal with the panel tolerancies in the DRM panel
> > > framework, we can (and should) use it.
> > 
> > If you mean the get_timings callback, it's not very useful. Most of
> > the panels in simple-panel do not use the display_timings structure,
> > so they don't return anything. And I get that. The few datasheets
> > I found don't list min/max tolerances for the dotclock.
> > 
> > The ones that do have the min/max the same as the recommended value.
> > This may or may not be accurate. IIRC the one panel that had this
> > that I did check didn't list min/max values in its datasheet.
> > 
> > > I'm not sure how others usually deal with this though. I think I
> > > remember Eric telling me that for the RPi they just adjusted the
> > > timings a bit, but they only really had a single panel to deal with.
> > > 
> > > Daniel, Eric, Laurent, Sean? Any ideas?
> > 
> > Yes! Feedback please! Between Maxime and me I think we only have a
> > limited number of panels, with some overlap.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Lucas Stach]

+CC Thierry, as the drm_panel maintainer.

Am Donnerstag, den 23.02.2017, 10:54 -0500 schrieb Sean Paul:
> On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> > > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> > > >> The panels shipped with Allwinner devices are very "generic", i.e.
> > > >> they do not have model numbers or reliable sources of information
> > > >> for the timings (that we know of) other than the fex files shipped
> > > >> on them. The dot clock frequency provided in the fex files have all
> > > >> been rounded to the nearest MHz, as that is the unit used in them.
> > > >> 
> > > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > > >> for the A13 Q8 tablets in the absence of a specific model for what
> > > >> may be many different but otherwise timing compatible panels. This
> > > >> was usable without any visual artifacts or side effects, until the
> > > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > > >> rgb: Validate the clock rate").
> > > >> 
> > > >> The reason this check fails is because the dotclock frequency for
> > > >> this model is 33.26 MHz, which is not achievable with our dot clock
> > > >> hardware, and the rate returned by clk_round_rate deviates slightly,
> > > >> causing the driver to reject the display mode.
> > > >> 
> > > >> The LCD panels have some tolerance on the dot clock frequency, even
> > > >> if it's not specified in their datasheets.
> > > >> 
> > > >> This patch adds a 5% tolerence to the dot clock check.
> > > > 
> > > > As we discussed already, I really believe this is just as arbitrary as
> > > > the current behaviour.
> > > 
> > > Yes. I agree. This patch is mainly to give something that works for
> > > people who don't care about the details, and to get some feedback
> > > from people that do.
> > > 
> > > > Some panels require an exact frequency,
> > 
> > There's no such thing as an exact frequency, there will always be some 
> > tolerance (and if your display controller can really generate an exact 
> > frequency I'd be very interested in that hardware :-)).
> 
> There is such thing as exact frequency when you have to worry about panel
> warranty. We are fortunate, since we can talk to the panel manufacturer and
> discuss which frequencies are tolerable inside the warranty. We usually hand
> pick a rate/mode within these constraints.
> 
> Also, even though things *look* ok on the panel at a certain clock rate, running
> outside the specified clock could damage the hardware.
> 
> I don't think it's unreasonable to add tolerances to drm_panel, since they will
> differ in what is acceptable. The tricky part is teasing out what the tolerances
> are.

The drm_panel already allows to specify all panel timings in pairs of
min/typical/max. Just use this instead of some arbitrary tolerance.

I know most panels currently only expose a fixed mode, but it's not hard
to convert this into a display_timing if you can get hold of the display
datasheet. A lot of the panel datasheets I had to deal with lately
actually specify all the required information. Most of the time you need
to sanity check things, as most vendors seem to produce them by "copy
the last datasheet we made and change only necessary fields", but that
really isn't a hard job.

Regards,
Lucas

Re: [linux-sunxi] Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Chen-Yu Tsai]

On Fri, Feb 24, 2017 at 5:51 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> +CC Thierry, as the drm_panel maintainer.
>
> Am Donnerstag, den 23.02.2017, 10:54 -0500 schrieb Sean Paul:
>> On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
>> > Hello,
>> >
>> > On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
>> > > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
>> > > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
>> > > >> The panels shipped with Allwinner devices are very "generic", i.e.
>> > > >> they do not have model numbers or reliable sources of information
>> > > >> for the timings (that we know of) other than the fex files shipped
>> > > >> on them. The dot clock frequency provided in the fex files have all
>> > > >> been rounded to the nearest MHz, as that is the unit used in them.
>> > > >>
>> > > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
>> > > >> for the A13 Q8 tablets in the absence of a specific model for what
>> > > >> may be many different but otherwise timing compatible panels. This
>> > > >> was usable without any visual artifacts or side effects, until the
>> > > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
>> > > >> rgb: Validate the clock rate").
>> > > >>
>> > > >> The reason this check fails is because the dotclock frequency for
>> > > >> this model is 33.26 MHz, which is not achievable with our dot clock
>> > > >> hardware, and the rate returned by clk_round_rate deviates slightly,
>> > > >> causing the driver to reject the display mode.
>> > > >>
>> > > >> The LCD panels have some tolerance on the dot clock frequency, even
>> > > >> if it's not specified in their datasheets.
>> > > >>
>> > > >> This patch adds a 5% tolerence to the dot clock check.
>> > > >
>> > > > As we discussed already, I really believe this is just as arbitrary as
>> > > > the current behaviour.
>> > >
>> > > Yes. I agree. This patch is mainly to give something that works for
>> > > people who don't care about the details, and to get some feedback
>> > > from people that do.
>> > >
>> > > > Some panels require an exact frequency,
>> >
>> > There's no such thing as an exact frequency, there will always be some
>> > tolerance (and if your display controller can really generate an exact
>> > frequency I'd be very interested in that hardware :-)).
>>
>> There is such thing as exact frequency when you have to worry about panel
>> warranty. We are fortunate, since we can talk to the panel manufacturer and
>> discuss which frequencies are tolerable inside the warranty. We usually hand
>> pick a rate/mode within these constraints.
>>
>> Also, even though things *look* ok on the panel at a certain clock rate, running
>> outside the specified clock could damage the hardware.
>>
>> I don't think it's unreasonable to add tolerances to drm_panel, since they will
>> differ in what is acceptable. The tricky part is teasing out what the tolerances
>> are.
>
> The drm_panel already allows to specify all panel timings in pairs of
> min/typical/max. Just use this instead of some arbitrary tolerance.
>
> I know most panels currently only expose a fixed mode, but it's not hard
> to convert this into a display_timing if you can get hold of the display
> datasheet. A lot of the panel datasheets I had to deal with lately
> actually specify all the required information. Most of the time you need
> to sanity check things, as most vendors seem to produce them by "copy
> the last datasheet we made and change only necessary fields", but that
> really isn't a hard job.

I wish it were that easy to identify panels and find datasheets, especially
when the vendors themselves use whatever cheap panel they can get their hands
on. Think no-brand cheap tablets from Aliexpress. These cheap panels may have
some model-number-like markings on them, but Googling fails to produce anything,
other than more vendors/items on Aliexpress.

Take the Q8 tablets with Allwinner SoCs in them. We have no idea what
panel the vendors use, and panels even vary between batches. We're using
the urt,umsh-8596md series of compatibles, which seems to have compatible
timings. Now the datasheet I found for them [1] just lists a typical
frequency for the pixel clock, and no min/max values. The typical frequency
is unachievable, for reasons already explained in the commit message.

Another example would be the LG LP079X01 or Auo B079XAN01 DSI panels, which
supposedly were also used in the 1st gen. iPad Minis. I found datasheets for
both [2][3], but neither lists tolerances for the dot clock.

So we're back to where we started. What is a reasonable tolerance to set in
the driver if we don't know the tolerances of the panel?


Regards
ChenYu


[1] http://www.udoo.org/download/files/datasheets/datasheet7.pdf
[2] http://recomit.de/service_manuals/LP079X01-SMA1.pdf
[3] http://img.jdzj.com/UserDocument/2012Y/hxzczmf/Document/20121229105325.pdf

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4394 bytes --]

On Fri, Feb 24, 2017 at 10:51:12AM +0100, Lucas Stach wrote:
> +CC Thierry, as the drm_panel maintainer.
> 
> Am Donnerstag, den 23.02.2017, 10:54 -0500 schrieb Sean Paul:
> > On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> > > > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > > > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> > > > >> The panels shipped with Allwinner devices are very "generic", i.e.
> > > > >> they do not have model numbers or reliable sources of information
> > > > >> for the timings (that we know of) other than the fex files shipped
> > > > >> on them. The dot clock frequency provided in the fex files have all
> > > > >> been rounded to the nearest MHz, as that is the unit used in them.
> > > > >> 
> > > > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > > > >> for the A13 Q8 tablets in the absence of a specific model for what
> > > > >> may be many different but otherwise timing compatible panels. This
> > > > >> was usable without any visual artifacts or side effects, until the
> > > > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > > > >> rgb: Validate the clock rate").
> > > > >> 
> > > > >> The reason this check fails is because the dotclock frequency for
> > > > >> this model is 33.26 MHz, which is not achievable with our dot clock
> > > > >> hardware, and the rate returned by clk_round_rate deviates slightly,
> > > > >> causing the driver to reject the display mode.
> > > > >> 
> > > > >> The LCD panels have some tolerance on the dot clock frequency, even
> > > > >> if it's not specified in their datasheets.
> > > > >> 
> > > > >> This patch adds a 5% tolerence to the dot clock check.
> > > > > 
> > > > > As we discussed already, I really believe this is just as arbitrary as
> > > > > the current behaviour.
> > > > 
> > > > Yes. I agree. This patch is mainly to give something that works for
> > > > people who don't care about the details, and to get some feedback
> > > > from people that do.
> > > > 
> > > > > Some panels require an exact frequency,
> > > 
> > > There's no such thing as an exact frequency, there will always be some 
> > > tolerance (and if your display controller can really generate an exact 
> > > frequency I'd be very interested in that hardware :-)).
> > 
> > There is such thing as exact frequency when you have to worry about panel
> > warranty. We are fortunate, since we can talk to the panel manufacturer and
> > discuss which frequencies are tolerable inside the warranty. We usually hand
> > pick a rate/mode within these constraints.
> > 
> > Also, even though things *look* ok on the panel at a certain clock rate, running
> > outside the specified clock could damage the hardware.
> > 
> > I don't think it's unreasonable to add tolerances to drm_panel, since they will
> > differ in what is acceptable. The tricky part is teasing out what the tolerances
> > are.
> 
> The drm_panel already allows to specify all panel timings in pairs of
> min/typical/max. Just use this instead of some arbitrary tolerance.
> 
> I know most panels currently only expose a fixed mode, but it's not hard
> to convert this into a display_timing if you can get hold of the display
> datasheet. A lot of the panel datasheets I had to deal with lately
> actually specify all the required information. Most of the time you need
> to sanity check things, as most vendors seem to produce them by "copy
> the last datasheet we made and change only necessary fields", but that
> really isn't a hard job.

Agreed. Back at the time support for min/typ/max timings was introduced
specifically to address such issues. Conversion to those timings from a
fixed mode is as simple as making min = max = typ, where typ is the DRM
display mode value.

There's a helper that will return a DRM display mode based on the
typical values for drivers that don't care (i.e. most) and drivers that
do care can simply use the timings directly and adjust as necessary. It
would be rather backwards to add tolerances to display drivers, because
that's not where the tolerances are defined. Panels impose the
restrictions, so it should be panel drivers that contain the data.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Laurent Pinchart]

Hi Sean,

On Thursday 23 Feb 2017 10:54:37 Sean Paul wrote:
> On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> > On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> >> On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> >>> On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> >>>> The panels shipped with Allwinner devices are very "generic", i.e.
> >>>> they do not have model numbers or reliable sources of information
> >>>> for the timings (that we know of) other than the fex files shipped
> >>>> on them. The dot clock frequency provided in the fex files have all
> >>>> been rounded to the nearest MHz, as that is the unit used in them.
> >>>> 
> >>>> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> >>>> for the A13 Q8 tablets in the absence of a specific model for what
> >>>> may be many different but otherwise timing compatible panels. This
> >>>> was usable without any visual artifacts or side effects, until the
> >>>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> >>>> rgb: Validate the clock rate").
> >>>> 
> >>>> The reason this check fails is because the dotclock frequency for
> >>>> this model is 33.26 MHz, which is not achievable with our dot clock
> >>>> hardware, and the rate returned by clk_round_rate deviates slightly,
> >>>> causing the driver to reject the display mode.
> >>>> 
> >>>> The LCD panels have some tolerance on the dot clock frequency, even
> >>>> if it's not specified in their datasheets.
> >>>> 
> >>>> This patch adds a 5% tolerence to the dot clock check.
> >>> 
> >>> As we discussed already, I really believe this is just as arbitrary as
> >>> the current behaviour.
> >> 
> >> Yes. I agree. This patch is mainly to give something that works for
> >> people who don't care about the details, and to get some feedback
> >> from people that do.
> >> 
> >>> Some panels require an exact frequency,
> > 
> > There's no such thing as an exact frequency, there will always be some
> > tolerance (and if your display controller can really generate an exact
> > frequency I'd be very interested in that hardware :-)).
> 
> There is such thing as exact frequency when you have to worry about panel
> warranty.

There's no such thing as exact frequency when you live in a world that has to 
comply with the laws of physics :-) This would require an infinitely precise 
clock, and there's no such thing.

> We are fortunate, since we can talk to the panel manufacturer and discuss
> which frequencies are tolerable inside the warranty. We usually hand pick a
> rate/mode within these constraints.
>
> Also, even though things *look* ok on the panel at a certain clock rate,
> running outside the specified clock could damage the hardware.

I'm curious, how so ? What happens to the panel if you feed it a clock outside 
of that range ?

> I don't think it's unreasonable to add tolerances to drm_panel, since they
> will differ in what is acceptable. The tricky part is teasing out what the
> tolerances are.

More than tricky, in the vast majority of cases we don't have that information 
at all :-/

> > This is something that has been bugging me for some time now. The problem
> > has been mostly ignored, or worked around in different ways by different
> > drivers. I'm afraid I have no generic solution available, but I think we
> > should try to agree on a common behaviour.
> > 
> > I don't believe it would be reasonable to request each panel to report a
> > tolerance, as the value is most of the time not available from the
> > documentation (when documentation is available). Worse, I'm pretty sure
> > that most panels documented as fixed timing can actually accept a wide
> > range of timings. The timings reported in the datasheet are just the
> > nominal values.
> > 
> > Panels that don't support multiple resolutions obviously require fixed
> > active h/v values. Even if they can tolerate some departure from the
> > nominal timings for the sync and porches lengths, it might not be very
> > useful to support that as I don't expect the display controllers and
> > encoders to be a limiting factor by not supporting the particular timings
> > that a panel considers as nominal. On the other hand, departing from the
> > nominal pixel clock frequency is needed as we can't achieve an exact
> > match, and even possibly to have some control over the frame rate
> > (although that might also require changing the sync and porches timings).
> > Without specific information about panel tolerance, do we have any option
> > other than picking an arbitrary value ?
> > 
> >>> some have a minimal frequency
> >>> but no maximum, some have a maximum frequency but no minimal, and I
> >>> guess most of them deviates by how much exactly they can take (and
> >>> possibly can take more easily a higher frequency, but are less
> >>> tolerant if you take a frequency lower than the nominal.
> >>> 
> >>> And we cannot remove that check entirely, since some bridges will
> >>> report out of range frequencies for higher modes that we know we
> >>> cannot reach.
> >> 
> >> I believe this should be handled by the bridge driver in the check
> >> callback? The callback I'm changing is attached to the connector,
> >> which I think doesn't get used if you have a bridge instead.
> >> And this only checks the pre-registered display modes, such as
> >> those specified in simple-panel or EDID.
> >> 
> >>> We could just try to see if the screen pixel clock frequency is out of
> >>> the pixel clock range we can generate, but then we will loop back on
> >>> how much out of range is it exactly, and is it within the screen
> >>> tolerancy.
> >>> 
> >>> We have an API to deal with the panel tolerancies in the DRM panel
> >>> framework, we can (and should) use it.
> >> 
> >> If you mean the get_timings callback, it's not very useful. Most of
> >> the panels in simple-panel do not use the display_timings structure,
> >> so they don't return anything. And I get that. The few datasheets
> >> I found don't list min/max tolerances for the dotclock.
> >> 
> >> The ones that do have the min/max the same as the recommended value.
> >> This may or may not be accurate. IIRC the one panel that had this
> >> that I did check didn't list min/max values in its datasheet.
> >> 
> >>> I'm not sure how others usually deal with this though. I think I
> >>> remember Eric telling me that for the RPi they just adjusted the
> >>> timings a bit, but they only really had a single panel to deal with.
> >>> 
> >>> Daniel, Eric, Laurent, Sean? Any ideas?
> >> 
> >> Yes! Feedback please! Between Maxime and me I think we only have a
> >> limited number of panels, with some overlap.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Wed, Dec 07, 2016 at 10:26:25AM +0800, Chen-Yu Tsai wrote:
> > Some panels require an exact frequency, some have a minimal frequency
> > but no maximum, some have a maximum frequency but no minimal, and I
> > guess most of them deviates by how much exactly they can take (and
> > possibly can take more easily a higher frequency, but are less
> > tolerant if you take a frequency lower than the nominal.
> >
> > And we cannot remove that check entirely, since some bridges will
> > report out of range frequencies for higher modes that we know we
> > cannot reach.
> 
> I believe this should be handled by the bridge driver in the check
> callback?

This doesn't really have anything to do with the bridge itself, it's a
limitation on the encoder. For all we know, the bridge might be able
to operate at the higher resolutions without any issues if the encoder
was able to.

> The callback I'm changing is attached to the connector, which I
> think doesn't get used if you have a bridge instead.  And this only
> checks the pre-registered display modes, such as those specified in
> simple-panel or EDID.

Geeee, I forgot to send that one (and thought I did)... I'll send that
patch next week, but basically, I was creating a mode_valid hook at
the encoder level, and moving the RGB mode_valid hook from the
connector to the encoder (since it really is an encoder limitation).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]

Maxime Ripard <maxime.ripard@free-electrons.com> writes:

> [ Unknown signature status ]
> On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
>> The panels shipped with Allwinner devices are very "generic", i.e.
>> they do not have model numbers or reliable sources of information
>> for the timings (that we know of) other than the fex files shipped
>> on them. The dot clock frequency provided in the fex files have all
>> been rounded to the nearest MHz, as that is the unit used in them.
>> 
>> We were using the simple panel "urt,umsh-8596md-t" as a substitute
>> for the A13 Q8 tablets in the absence of a specific model for what
>> may be many different but otherwise timing compatible panels. This
>> was usable without any visual artifacts or side effects, until the
>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
>> rgb: Validate the clock rate").
>> 
>> The reason this check fails is because the dotclock frequency for
>> this model is 33.26 MHz, which is not achievable with our dot clock
>> hardware, and the rate returned by clk_round_rate deviates slightly,
>> causing the driver to reject the display mode.
>> 
>> The LCD panels have some tolerance on the dot clock frequency, even
>> if it's not specified in their datasheets.
>> 
>> This patch adds a 5% tolerence to the dot clock check.
>
> As we discussed already, I really believe this is just as arbitrary as
> the current behaviour.
>
> Some panels require an exact frequency, some have a minimal frequency
> but no maximum, some have a maximum frequency but no minimal, and I
> guess most of them deviates by how much exactly they can take (and
> possibly can take more easily a higher frequency, but are less
> tolerant if you take a frequency lower than the nominal.
>
> And we cannot remove that check entirely, since some bridges will
> report out of range frequencies for higher modes that we know we
> cannot reach.
>
> We could just try to see if the screen pixel clock frequency is out of
> the pixel clock range we can generate, but then we will loop back on
> how much out of range is it exactly, and is it within the screen
> tolerancy.
>
> We have an API to deal with the panel tolerancies in the DRM panel
> framework, we can (and should) use it.
>
> I'm not sure how others usually deal with this though. I think I
> remember Eric telling me that for the RPi they just adjusted the
> timings a bit, but they only really had a single panel to deal with.

For RPi, you just adjust the pixel clock of the panel's mode to be
whatever the platform can support, and expand the blanking intervals to
get the refresh rate back to desired.  This is nothing like what the
datasheet says, but it's not important what the datasheet says, it's
important what makes the product work.

Our clock driver looks for the best matching clock that's not over the
target rate.  This is somewhat unfortunate, as you end up slightly
inflating your requested clocks so that a possible clock lands under
that.  I'd rather we chose the closest matching clock, but then people
get worried about what if selected clock rate is 1% higher than expected
(the answer is "nothing").

I think this patch is a fine solution, and the alternative would be to
just drop the mode high/low check and say that if you're pairing a panel
with some display hardware, it's up to you to make sure that the panel's
mode actually scans out successfully.  Then, since compatible strings
are cheap, you can use a new one if necessary to attach better modes to
the panel for a particular clock driver by adjusting your timings to get
closer to the refresh rates you want.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Laurent Pinchart]

Hi Eric,

On Wednesday 07 Dec 2016 11:16:32 Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> > [ Unknown signature status ]
> > 
> > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> >> The panels shipped with Allwinner devices are very "generic", i.e.
> >> they do not have model numbers or reliable sources of information
> >> for the timings (that we know of) other than the fex files shipped
> >> on them. The dot clock frequency provided in the fex files have all
> >> been rounded to the nearest MHz, as that is the unit used in them.
> >> 
> >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> >> for the A13 Q8 tablets in the absence of a specific model for what
> >> may be many different but otherwise timing compatible panels. This
> >> was usable without any visual artifacts or side effects, until the
> >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> >> rgb: Validate the clock rate").
> >> 
> >> The reason this check fails is because the dotclock frequency for
> >> this model is 33.26 MHz, which is not achievable with our dot clock
> >> hardware, and the rate returned by clk_round_rate deviates slightly,
> >> causing the driver to reject the display mode.
> >> 
> >> The LCD panels have some tolerance on the dot clock frequency, even
> >> if it's not specified in their datasheets.
> >> 
> >> This patch adds a 5% tolerence to the dot clock check.
> > 
> > As we discussed already, I really believe this is just as arbitrary as
> > the current behaviour.
> > 
> > Some panels require an exact frequency, some have a minimal frequency
> > but no maximum, some have a maximum frequency but no minimal, and I
> > guess most of them deviates by how much exactly they can take (and
> > possibly can take more easily a higher frequency, but are less
> > tolerant if you take a frequency lower than the nominal.
> > 
> > And we cannot remove that check entirely, since some bridges will
> > report out of range frequencies for higher modes that we know we
> > cannot reach.
> > 
> > We could just try to see if the screen pixel clock frequency is out of
> > the pixel clock range we can generate, but then we will loop back on
> > how much out of range is it exactly, and is it within the screen
> > tolerancy.
> > 
> > We have an API to deal with the panel tolerancies in the DRM panel
> > framework, we can (and should) use it.
> > 
> > I'm not sure how others usually deal with this though. I think I
> > remember Eric telling me that for the RPi they just adjusted the
> > timings a bit, but they only really had a single panel to deal with.
> 
> For RPi, you just adjust the pixel clock of the panel's mode to be
> whatever the platform can support, and expand the blanking intervals to
> get the refresh rate back to desired.  This is nothing like what the
> datasheet says, but it's not important what the datasheet says, it's
> important what makes the product work.
> 
> Our clock driver looks for the best matching clock that's not over the
> target rate.  This is somewhat unfortunate, as you end up slightly
> inflating your requested clocks so that a possible clock lands under
> that.  I'd rather we chose the closest matching clock, but then people
> get worried about what if selected clock rate is 1% higher than expected
> (the answer is "nothing").

But if the closest match is 10% off and higher results could be different, in 
which case a lower match that is 11% off might be better. The hard part is to 
decide where to set the limit, and I'm afraid the answer is likely system-
dependent.

> I think this patch is a fine solution, and the alternative would be to
> just drop the mode high/low check and say that if you're pairing a panel
> with some display hardware, it's up to you to make sure that the panel's
> mode actually scans out successfully.  Then, since compatible strings
> are cheap, you can use a new one if necessary to attach better modes to
> the panel for a particular clock driver by adjusting your timings to get
> closer to the refresh rates you want.

Given that timings tolerance can be system-dependent and not only panel-
dependent, it would make sense to specify them in DT (possibly an optional 
properties with reasonable default values computed by drivers).

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4880 bytes --]

Hi Eric,

On Wed, Dec 07, 2016 at 11:16:32AM -0800, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
> > [ Unknown signature status ]
> > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> >> The panels shipped with Allwinner devices are very "generic", i.e.
> >> they do not have model numbers or reliable sources of information
> >> for the timings (that we know of) other than the fex files shipped
> >> on them. The dot clock frequency provided in the fex files have all
> >> been rounded to the nearest MHz, as that is the unit used in them.
> >> 
> >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> >> for the A13 Q8 tablets in the absence of a specific model for what
> >> may be many different but otherwise timing compatible panels. This
> >> was usable without any visual artifacts or side effects, until the
> >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> >> rgb: Validate the clock rate").
> >> 
> >> The reason this check fails is because the dotclock frequency for
> >> this model is 33.26 MHz, which is not achievable with our dot clock
> >> hardware, and the rate returned by clk_round_rate deviates slightly,
> >> causing the driver to reject the display mode.
> >> 
> >> The LCD panels have some tolerance on the dot clock frequency, even
> >> if it's not specified in their datasheets.
> >> 
> >> This patch adds a 5% tolerence to the dot clock check.
> >
> > As we discussed already, I really believe this is just as arbitrary as
> > the current behaviour.
> >
> > Some panels require an exact frequency, some have a minimal frequency
> > but no maximum, some have a maximum frequency but no minimal, and I
> > guess most of them deviates by how much exactly they can take (and
> > possibly can take more easily a higher frequency, but are less
> > tolerant if you take a frequency lower than the nominal.
> >
> > And we cannot remove that check entirely, since some bridges will
> > report out of range frequencies for higher modes that we know we
> > cannot reach.
> >
> > We could just try to see if the screen pixel clock frequency is out of
> > the pixel clock range we can generate, but then we will loop back on
> > how much out of range is it exactly, and is it within the screen
> > tolerancy.
> >
> > We have an API to deal with the panel tolerancies in the DRM panel
> > framework, we can (and should) use it.
> >
> > I'm not sure how others usually deal with this though. I think I
> > remember Eric telling me that for the RPi they just adjusted the
> > timings a bit, but they only really had a single panel to deal with.
> 
> For RPi, you just adjust the pixel clock of the panel's mode to be
> whatever the platform can support, and expand the blanking intervals to
> get the refresh rate back to desired.  This is nothing like what the
> datasheet says, but it's not important what the datasheet says, it's
> important what makes the product work.

Ok, that was what I was recalling from our previous discussion on that
topic.

> Our clock driver looks for the best matching clock that's not over the
> target rate.  This is somewhat unfortunate, as you end up slightly
> inflating your requested clocks so that a possible clock lands under
> that.  I'd rather we chose the closest matching clock, but then people
> get worried about what if selected clock rate is 1% higher than expected
> (the answer is "nothing").

Whose feedback was that? Users?

> I think this patch is a fine solution, and the alternative would be to
> just drop the mode high/low check and say that if you're pairing a panel
> with some display hardware, it's up to you to make sure that the panel's
> mode actually scans out successfully.  Then, since compatible strings
> are cheap, you can use a new one if necessary to attach better modes to
> the panel for a particular clock driver by adjusting your timings to get
> closer to the refresh rates you want.

That's one expectation we can have for panels, but we had that test
for bridges. On some SoCs, the pixel clock is pretty limited and can
only reach around 720p60 or 1080p30. If you have a monitor attached
that will return EDIDs, chances are that it will report modes that you
know have no chance to work.

This check was here to rule out those cases and prevent them from
showing up in the list of modes.

So we basically have two different things to care about. We want to be
tolerant so that most panels just work, but not too tolerant to rule
out modes that we know we can't reach. We're only covering the latter,
and we should take into account the former, but we definitely need
some kind of check.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]