[PATCHv2] drm/sun4i: validate modes for HDMI

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20171215/70568ba1/attachment.sig>