[PATCHv2] drm/sun4i: validate modes for HDMI

Submitted by Hans Verkuil on Dec. 8, 2017, 3:48 p.m.

Details

Message ID 3762ba00-f00e-9709-e57c-0f07128c751c@xs4all.nl
State New
Headers show
Series "drm/sun4i: validate modes for HDMI" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Hans Verkuil Dec. 8, 2017, 3:48 p.m.
When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some
digging found that there is no check against the upper pixelclock limit of the HDMI
output, so X selects a 4kp60 format at 594 MHz, which obviously won't work.

The patch below adds a check for the upper bound of what this hardware can do, and
it checks if the requested tmds clock can be obtained.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index dda904ec0534..c10400a19b33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -208,8 +208,27 @@  static int sun4i_hdmi_get_modes(struct drm_connector *connector)
 	return ret;
 }

+static int sun4i_hdmi_mode_valid(struct drm_connector *connector,
+				 struct drm_display_mode *mode)
+{
+	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
+	unsigned long rate = mode->clock * 1000;
+	long rounded_rate;
+
+	/* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */
+	if (rate > 165000000)
+		return MODE_CLOCK_HIGH;
+	rounded_rate = clk_round_rate(hdmi->tmds_clk, rate);
+	if (rounded_rate < rate)
+		return MODE_CLOCK_LOW;
+	if (rounded_rate > rate)
+		return MODE_CLOCK_HIGH;
+	return MODE_OK;
+}
+
 static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
 	.get_modes	= sun4i_hdmi_get_modes,
+	.mode_valid	= sun4i_hdmi_mode_valid,
 };

 static enum drm_connector_status

Comments

Hi Hans,

On Fri, Dec 08, 2017 at 04:48:47PM +0100, Hans Verkuil wrote:
> When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some
> digging found that there is no check against the upper pixelclock limit of the HDMI
> output, so X selects a 4kp60 format at 594 MHz, which obviously won't work.
> 
> The patch below adds a check for the upper bound of what this hardware can do, and
> it checks if the requested tmds clock can be obtained.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index dda904ec0534..c10400a19b33 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -208,8 +208,27 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
> 
> +static int sun4i_hdmi_mode_valid(struct drm_connector *connector,
> +				 struct drm_display_mode *mode)
> +{
> +	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> +	unsigned long rate = mode->clock * 1000;
> +	long rounded_rate;
> +
> +	/* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */
> +	if (rate > 165000000)
> +		return MODE_CLOCK_HIGH;
> +	rounded_rate = clk_round_rate(hdmi->tmds_clk, rate);
> +	if (rounded_rate < rate)
> +		return MODE_CLOCK_LOW;
> +	if (rounded_rate > rate)
> +		return MODE_CLOCK_HIGH;
> +	return MODE_OK;
> +}

This looks much better, thanks!

One thing that I was mentionning in my other mail is that our rate
rounding might not provide the exact TMDS clock rate advertised by the
EDID, while staying in the tolerancy.

We've raised this issue before, without coming to a conclusion. Would
you happen to know what that tolerancy would be on an HDMI link?

Thanks!
Maxime
On 13/12/17 11:30, Maxime Ripard wrote:
> Hi Hans,
> 
> On Fri, Dec 08, 2017 at 04:48:47PM +0100, Hans Verkuil wrote:
>> When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some
>> digging found that there is no check against the upper pixelclock limit of the HDMI
>> output, so X selects a 4kp60 format at 594 MHz, which obviously won't work.
>>
>> The patch below adds a check for the upper bound of what this hardware can do, and
>> it checks if the requested tmds clock can be obtained.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index dda904ec0534..c10400a19b33 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -208,8 +208,27 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>>  	return ret;
>>  }
>>
>> +static int sun4i_hdmi_mode_valid(struct drm_connector *connector,
>> +				 struct drm_display_mode *mode)
>> +{
>> +	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
>> +	unsigned long rate = mode->clock * 1000;
>> +	long rounded_rate;
>> +
>> +	/* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */
>> +	if (rate > 165000000)
>> +		return MODE_CLOCK_HIGH;
>> +	rounded_rate = clk_round_rate(hdmi->tmds_clk, rate);
>> +	if (rounded_rate < rate)
>> +		return MODE_CLOCK_LOW;
>> +	if (rounded_rate > rate)
>> +		return MODE_CLOCK_HIGH;
>> +	return MODE_OK;
>> +}
> 
> This looks much better, thanks!
> 
> One thing that I was mentionning in my other mail is that our rate
> rounding might not provide the exact TMDS clock rate advertised by the
> EDID, while staying in the tolerancy.
> 
> We've raised this issue before, without coming to a conclusion. Would
> you happen to know what that tolerancy would be on an HDMI link?

I can't actually find anything about that in the HDMI spec. However, the VESA DMT
spec specifies a tolerance of +/- 0.5% for the pixelclock.

The HDMI 2.1 spec also suggests that this is the tolerance (caveat: I have not had
the time to study this in detail, but it does mention it when describing the new
variable refresh rate feature).

That said, the problem with a 0.5% tolerance is that the slight slowdown for 59.94
vs 60 Hz framerate falls within that tolerance (it's a 0.1% slowdown).

Generally clocks will be able to hit the standard frequencies (74.25, 148.5, etc)
exactly, but if you want to slow down for 59.94 framerate they tend to be off by
a bit.

In the end I think keeping a margin of 0.4 or 0.5% is the best approach.

Regards,

	Hans
Hi,

On 13-12-2017 11:05, Hans Verkuil wrote:
> On 13/12/17 11:30, Maxime Ripard wrote:
>> Hi Hans,
>>
>> On Fri, Dec 08, 2017 at 04:48:47PM +0100, Hans Verkuil wrote:
>>> When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some
>>> digging found that there is no check against the upper pixelclock limit of the HDMI
>>> output, so X selects a 4kp60 format at 594 MHz, which obviously won't work.
>>>
>>> The patch below adds a check for the upper bound of what this hardware can do, and
>>> it checks if the requested tmds clock can be obtained.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>> index dda904ec0534..c10400a19b33 100644
>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>> @@ -208,8 +208,27 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>>>  	return ret;
>>>  }
>>>
>>> +static int sun4i_hdmi_mode_valid(struct drm_connector *connector,
>>> +				 struct drm_display_mode *mode)
>>> +{
>>> +	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
>>> +	unsigned long rate = mode->clock * 1000;
>>> +	long rounded_rate;
>>> +
>>> +	/* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */
>>> +	if (rate > 165000000)
>>> +		return MODE_CLOCK_HIGH;
>>> +	rounded_rate = clk_round_rate(hdmi->tmds_clk, rate);
>>> +	if (rounded_rate < rate)
>>> +		return MODE_CLOCK_LOW;
>>> +	if (rounded_rate > rate)
>>> +		return MODE_CLOCK_HIGH;
>>> +	return MODE_OK;
>>> +}
>> This looks much better, thanks!
>>
>> One thing that I was mentionning in my other mail is that our rate
>> rounding might not provide the exact TMDS clock rate advertised by the
>> EDID, while staying in the tolerancy.
>>
>> We've raised this issue before, without coming to a conclusion. Would
>> you happen to know what that tolerancy would be on an HDMI link?
> I can't actually find anything about that in the HDMI spec. However, the VESA DMT
> spec specifies a tolerance of +/- 0.5% for the pixelclock.
>
> The HDMI 2.1 spec also suggests that this is the tolerance (caveat: I have not had
> the time to study this in detail, but it does mention it when describing the new
> variable refresh rate feature).
>
> That said, the problem with a 0.5% tolerance is that the slight slowdown for 59.94
> vs 60 Hz framerate falls within that tolerance (it's a 0.1% slowdown).
>
> Generally clocks will be able to hit the standard frequencies (74.25, 148.5, etc)
> exactly, but if you want to slow down for 59.94 framerate they tend to be off by
> a bit.
>
> In the end I think keeping a margin of 0.4 or 0.5% is the best approach.

We had the same problem in arcpgu, please check if commit [1] can
be used by you.

Best Regards,
Jose Miguel Abreu

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22d0be2a557e53a22feb484e8fce255fe09e6ad5

>
> Regards,
>
> 	Hans
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=HwJwBnLWatgtXZ6tmlyN-ibL-Cj4hheMGPCEW6rh7sY&s=0f011r4qaTVGDQc5-niD-Faj5yc2kl6jq2KtxKdU20o&e=
Hi Jose,

On Wed, Dec 13, 2017 at 12:07:05PM +0000, Jose Abreu wrote:
> On 13-12-2017 11:05, Hans Verkuil wrote:
> > On 13/12/17 11:30, Maxime Ripard wrote:
> >> Hi Hans,
> >>
> >> On Fri, Dec 08, 2017 at 04:48:47PM +0100, Hans Verkuil wrote:
> >>> When I connected my cubieboard running 4.15-rc1 to my 4k display I got no picture. Some
> >>> digging found that there is no check against the upper pixelclock limit of the HDMI
> >>> output, so X selects a 4kp60 format at 594 MHz, which obviously won't work.
> >>>
> >>> The patch below adds a check for the upper bound of what this hardware can do, and
> >>> it checks if the requested tmds clock can be obtained.
> >>>
> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>> ---
> >>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 19 +++++++++++++++++++
> >>>  1 file changed, 19 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >>> index dda904ec0534..c10400a19b33 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >>> @@ -208,8 +208,27 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >>>  	return ret;
> >>>  }
> >>>
> >>> +static int sun4i_hdmi_mode_valid(struct drm_connector *connector,
> >>> +				 struct drm_display_mode *mode)
> >>> +{
> >>> +	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> >>> +	unsigned long rate = mode->clock * 1000;
> >>> +	long rounded_rate;
> >>> +
> >>> +	/* 165 MHz is the typical max pixelclock frequency for HDMI <= 1.2 */
> >>> +	if (rate > 165000000)
> >>> +		return MODE_CLOCK_HIGH;
> >>> +	rounded_rate = clk_round_rate(hdmi->tmds_clk, rate);
> >>> +	if (rounded_rate < rate)
> >>> +		return MODE_CLOCK_LOW;
> >>> +	if (rounded_rate > rate)
> >>> +		return MODE_CLOCK_HIGH;
> >>> +	return MODE_OK;
> >>> +}
> >> This looks much better, thanks!
> >>
> >> One thing that I was mentionning in my other mail is that our rate
> >> rounding might not provide the exact TMDS clock rate advertised by the
> >> EDID, while staying in the tolerancy.
> >>
> >> We've raised this issue before, without coming to a conclusion. Would
> >> you happen to know what that tolerancy would be on an HDMI link?
> > I can't actually find anything about that in the HDMI spec. However, the VESA DMT
> > spec specifies a tolerance of +/- 0.5% for the pixelclock.
> >
> > The HDMI 2.1 spec also suggests that this is the tolerance (caveat: I have not had
> > the time to study this in detail, but it does mention it when describing the new
> > variable refresh rate feature).
> >
> > That said, the problem with a 0.5% tolerance is that the slight slowdown for 59.94
> > vs 60 Hz framerate falls within that tolerance (it's a 0.1% slowdown).
> >
> > Generally clocks will be able to hit the standard frequencies (74.25, 148.5, etc)
> > exactly, but if you want to slow down for 59.94 framerate they tend to be off by
> > a bit.
> >
> > In the end I think keeping a margin of 0.4 or 0.5% is the best approach.
> 
> We had the same problem in arcpgu, please check if commit [1] can
> be used by you.
> 
> Best Regards,
> Jose Miguel Abreu
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22d0be2a557e53a22feb484e8fce255fe09e6ad5

Yeah, something like that would definitely work. I'm not entirely sure
whether we need to report whether the clock was too high or too low,
but I'm totally fine with something like that.

Hans, do you want to resend a new version of this patch?

Thanks!
Maxime