diff mbox

[3/3] drm/i915: Enforce max hdisplay/hblank_start limits on HSW/BDW FDI

Message ID 20180615174406.12258-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä June 15, 2018, 5:44 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The PCH transcoder registers are only 12 bits wide for the hdisplay
and hblank_start values. On HSW/BDW the CPU side registers are 13
bits wide. intel_mode_valid() only checks against the higher limit
(since we don't know where the mode is to be used), so an extra
check is required against the FDI limits.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Zanoni, Paulo R June 15, 2018, 9:09 p.m. UTC | #1 
Em Sex, 2018-06-15 às 20:44 +0300, Ville Syrjala escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The PCH transcoder registers are only 12 bits wide for the hdisplay
> and hblank_start values. On HSW/BDW the CPU side registers are 13
> bits wide. intel_mode_valid() only checks against the higher limit
> (since we don't know where the mode is to be used), so an extra
> check is required against the FDI limits.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c
> b/drivers/gpu/drm/i915/intel_crt.c
> index 95aa29cf2d9c..457b1a2d05b8 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -333,6 +333,10 @@ intel_crt_mode_valid(struct drm_connector
> *connector,
>  	    (ironlake_get_lanes_required(mode->clock, 270000, 24) >
> 2))
>  		return MODE_CLOCK_HIGH;
>  
> +	/* HSW/BDW FDI limited to 4k */
> +	if (mode->hdisplay > 4096)
> +		return MODE_H_ILLEGAL;
> +
>  	return MODE_OK;
>  }
>  
> @@ -375,6 +379,11 @@ static bool hsw_crt_compute_config(struct
> intel_encoder *encoder,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
>  		return false;
>  
> +	/* HSW/BDW FDI limited to 4k */
> +	if (adjusted_mode->crtc_hdisplay > 4096 ||
> +	    adjusted_mode->crtc_hblank_start > 4096)
> +		return false;

Meh, doubling checks is not cool.

By the way, doesn't this chunk make more sense inside
ironlake_fdi_compute_config()?

Just to make sure: the only cases that could escape mode_valid() and
make it into compute_config() are for panel fitting, right?

With or without changes:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks,
Paulo 

> +
>  	pipe_config->has_pch_encoder = true;
>  
>  	/* LPT FDI RX only supports 8bpc. */
Ville Syrjälä June 18, 2018, 1:13 p.m. UTC | #2 
On Fri, Jun 15, 2018 at 02:09:12PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-06-15 às 20:44 +0300, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The PCH transcoder registers are only 12 bits wide for the hdisplay
> > and hblank_start values. On HSW/BDW the CPU side registers are 13
> > bits wide. intel_mode_valid() only checks against the higher limit
> > (since we don't know where the mode is to be used), so an extra
> > check is required against the FDI limits.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c
> > b/drivers/gpu/drm/i915/intel_crt.c
> > index 95aa29cf2d9c..457b1a2d05b8 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -333,6 +333,10 @@ intel_crt_mode_valid(struct drm_connector
> > *connector,
> >  	    (ironlake_get_lanes_required(mode->clock, 270000, 24) >
> > 2))
> >  		return MODE_CLOCK_HIGH;
> >  
> > +	/* HSW/BDW FDI limited to 4k */
> > +	if (mode->hdisplay > 4096)
> > +		return MODE_H_ILLEGAL;
> > +
> >  	return MODE_OK;
> >  }
> >  
> > @@ -375,6 +379,11 @@ static bool hsw_crt_compute_config(struct
> > intel_encoder *encoder,
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
> >  		return false;
> >  
> > +	/* HSW/BDW FDI limited to 4k */
> > +	if (adjusted_mode->crtc_hdisplay > 4096 ||
> > +	    adjusted_mode->crtc_hblank_start > 4096)
> > +		return false;
> 
> Meh, doubling checks is not cool.
> 
> By the way, doesn't this chunk make more sense inside
> ironlake_fdi_compute_config()?

Hmm. I don't see a particularly good reason for putting it there. All
that guy is doing is computing the number of lanes and the M/N values.
Nothing to do with transcoder timings. It's also called fairly late in
the process, so we'd end up doing quite a bit of pointless work that
way.

> 
> Just to make sure: the only cases that could escape mode_valid() and
> make it into compute_config() are for panel fitting, right?

The connector .mode_valid() won't be called for setcrtc/atomic
ioctls at all.

> 
> With or without changes:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Thanks,
> Paulo 
> 
> > +
> >  	pipe_config->has_pch_encoder = true;
> >  
> >  	/* LPT FDI RX only supports 8bpc. */
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 95aa29cf2d9c..457b1a2d05b8 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -333,6 +333,10 @@  intel_crt_mode_valid(struct drm_connector *connector,
 	    (ironlake_get_lanes_required(mode->clock, 270000, 24) > 2))
 		return MODE_CLOCK_HIGH;
 
+	/* HSW/BDW FDI limited to 4k */
+	if (mode->hdisplay > 4096)
+		return MODE_H_ILLEGAL;
+
 	return MODE_OK;
 }
 
@@ -375,6 +379,11 @@  static bool hsw_crt_compute_config(struct intel_encoder *encoder,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
 		return false;
 
+	/* HSW/BDW FDI limited to 4k */
+	if (adjusted_mode->crtc_hdisplay > 4096 ||
+	    adjusted_mode->crtc_hblank_start > 4096)
+		return false;
+
 	pipe_config->has_pch_encoder = true;
 
 	/* LPT FDI RX only supports 8bpc. */