[PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In commit
 commit 6375b768a9850b6154478993e5fb566fa4614a9c
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Mar 3 11:33:36 2014 +0200

    drm/i915: Reject >165MHz modes w/ DVI monitors

the driver started to filter out display modes which exceed the
single-link DVI 165Mz dotclock limits when the monitor doesn't report
itself as being HDMI compliant. The intent was to filter out all
EDID derived modes that require dual-link DVI to operate since we
don't support dual-link.

However the patch went a bit too far and also causes the driver to reject
such modes even when specified by the user. Normally we don't check the
sink limitations when setting a mode from the user. This allows the user
to specify any mode whether the sink reports to support it or not. This
can be useful since often the sinks support more modes than they report
in the EDID.

So relax the checks a bit, and apply the single-link DVI dotclock limit
only when filtering the mode list, and ignore the limit when setting
a user specified mode.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=72961
Tested-by: Nicholas Vinson <nvinson@comcast.net>
Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee3181e..ca5d23d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -841,11 +841,11 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	}
 }
 
-static int hdmi_portclock_limit(struct intel_hdmi *hdmi)
+static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
 {
 	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
 
-	if (!hdmi->has_hdmi_sink || IS_G4X(dev))
+	if ((respect_dvi_limit && !hdmi->has_hdmi_sink) || IS_G4X(dev))
 		return 165000;
 	else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
 		return 300000;
@@ -857,7 +857,8 @@ static enum drm_mode_status
 intel_hdmi_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
 {
-	if (mode->clock > hdmi_portclock_limit(intel_attached_hdmi(connector)))
+	if (mode->clock > hdmi_portclock_limit(intel_attached_hdmi(connector),
+					       true))
 		return MODE_CLOCK_HIGH;
 	if (mode->clock < 20000)
 		return MODE_CLOCK_LOW;
@@ -875,7 +876,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	int clock_12bpc = pipe_config->adjusted_mode.crtc_clock * 3 / 2;
-	int portclock_limit = hdmi_portclock_limit(intel_hdmi);
+	int portclock_limit = hdmi_portclock_limit(intel_hdmi, false);
 	int desired_bpp;
 
 	if (intel_hdmi->color_range_auto) {
-- 
1.8.3.2

Re: [Intel-gfx] [PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[Chris Wilson]

On Thu, Mar 27, 2014 at 11:08:45AM +0200, ville.syrjala@linux.intel.com wrote:
> So relax the checks a bit, and apply the single-link DVI dotclock limit
> only when filtering the mode list, and ignore the limit when setting
> a user specified mode.

Mind enlightening me as to how this actually works? I thought all
display modes were validated before we used them, so how come this
sneaks through?

So it goes like this:

userspace calls GETCONNECTOR
kernel: fill_modes -> drm_helper_probe_single_connector_modes -> mode_valid?

but

userspace calls SETCRTC with a random mode
kernel: applies random mode without validation

Seriously we don't do any checking that the mode given to SETCRTC is
applicable and not in any way harmful before setting registers?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[Ville Syrjälä]

On Thu, Mar 27, 2014 at 09:37:36AM +0000, Chris Wilson wrote:
> On Thu, Mar 27, 2014 at 11:08:45AM +0200, ville.syrjala@linux.intel.com wrote:
> > So relax the checks a bit, and apply the single-link DVI dotclock limit
> > only when filtering the mode list, and ignore the limit when setting
> > a user specified mode.
> 
> Mind enlightening me as to how this actually works? I thought all
> display modes were validated before we used them, so how come this
> sneaks through?
> 
> So it goes like this:
> 
> userspace calls GETCONNECTOR
> kernel: fill_modes -> drm_helper_probe_single_connector_modes -> mode_valid?
> 
> but
> 
> userspace calls SETCRTC with a random mode
> kernel: applies random mode without validation
> 
> Seriously we don't do any checking that the mode given to SETCRTC is
> applicable and not in any way harmful before setting registers?

Pretty much. Calling our user mode validation even "minimal" is a bit
of a stretch. And the checks we have in the .mode_valid() hooks are
lacking as well.

As far as this particular patch goes, we do check the port clock in the
hdmi compute_config hook, so I had to relax it a bit to ignore the single
link DVI limit there, and instead just check it against the hardware
specific limit from BSpec. I think that's a reasonable thing to do. If
we wanted to protect the user (or the equipment in case of CRTs) more,
we should probably check things against the EDID min/max scan rates and
max clock.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[Daniel Vetter]

On Thu, Mar 27, 2014 at 01:03:31PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 27, 2014 at 09:37:36AM +0000, Chris Wilson wrote:
> > On Thu, Mar 27, 2014 at 11:08:45AM +0200, ville.syrjala@linux.intel.com wrote:
> > > So relax the checks a bit, and apply the single-link DVI dotclock limit
> > > only when filtering the mode list, and ignore the limit when setting
> > > a user specified mode.
> > 
> > Mind enlightening me as to how this actually works? I thought all
> > display modes were validated before we used them, so how come this
> > sneaks through?
> > 
> > So it goes like this:
> > 
> > userspace calls GETCONNECTOR
> > kernel: fill_modes -> drm_helper_probe_single_connector_modes -> mode_valid?
> > 
> > but
> > 
> > userspace calls SETCRTC with a random mode
> > kernel: applies random mode without validation
> > 
> > Seriously we don't do any checking that the mode given to SETCRTC is
> > applicable and not in any way harmful before setting registers?
> 
> Pretty much. Calling our user mode validation even "minimal" is a bit
> of a stretch. And the checks we have in the .mode_valid() hooks are
> lacking as well.

I've had patches floating around which implemented mode_valid in terms of
compute_config (or well, mode_adjust as it was called back then). But
there are slight differences in semantics so that didn't pan out too well.

But yeah, encoders need to share the back-end mode checking functions
between mode_valid and compute_config otherwise we'll just let random gunk
get through.

Checking at the crtc level is better since modes without a valid pll
config won't get through. Well, if we'd compute the pll settings a bit
earlier ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit

[Jani Nikula]

Pushed to -fixes, thanks for the patch and Daniel's r-b on IRC.

BR,
Jani.

On Thu, 27 Mar 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In commit
>  commit 6375b768a9850b6154478993e5fb566fa4614a9c
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Mon Mar 3 11:33:36 2014 +0200
>
>     drm/i915: Reject >165MHz modes w/ DVI monitors
>
> the driver started to filter out display modes which exceed the
> single-link DVI 165Mz dotclock limits when the monitor doesn't report
> itself as being HDMI compliant. The intent was to filter out all
> EDID derived modes that require dual-link DVI to operate since we
> don't support dual-link.
>
> However the patch went a bit too far and also causes the driver to reject
> such modes even when specified by the user. Normally we don't check the
> sink limitations when setting a mode from the user. This allows the user
> to specify any mode whether the sink reports to support it or not. This
> can be useful since often the sinks support more modes than they report
> in the EDID.
>
> So relax the checks a bit, and apply the single-link DVI dotclock limit
> only when filtering the mode list, and ignore the limit when setting
> a user specified mode.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=72961
> Tested-by: Nicholas Vinson <nvinson@comcast.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ee3181e..ca5d23d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -841,11 +841,11 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  	}
>  }
>  
> -static int hdmi_portclock_limit(struct intel_hdmi *hdmi)
> +static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>  {
>  	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
>  
> -	if (!hdmi->has_hdmi_sink || IS_G4X(dev))
> +	if ((respect_dvi_limit && !hdmi->has_hdmi_sink) || IS_G4X(dev))
>  		return 165000;
>  	else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
>  		return 300000;
> @@ -857,7 +857,8 @@ static enum drm_mode_status
>  intel_hdmi_mode_valid(struct drm_connector *connector,
>  		      struct drm_display_mode *mode)
>  {
> -	if (mode->clock > hdmi_portclock_limit(intel_attached_hdmi(connector)))
> +	if (mode->clock > hdmi_portclock_limit(intel_attached_hdmi(connector),
> +					       true))
>  		return MODE_CLOCK_HIGH;
>  	if (mode->clock < 20000)
>  		return MODE_CLOCK_LOW;
> @@ -875,7 +876,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>  	int clock_12bpc = pipe_config->adjusted_mode.crtc_clock * 3 / 2;
> -	int portclock_limit = hdmi_portclock_limit(intel_hdmi);
> +	int portclock_limit = hdmi_portclock_limit(intel_hdmi, false);
>  	int desired_bpp;
>  
>  	if (intel_hdmi->color_range_auto) {
> -- 
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center