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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Paulo Zanoni <paulo.r.zanoni@intel.com>, g@freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/i915: Enforce max hdisplay/hblank_start limits on HSW/BDW FDI
Date: Mon, 18 Jun 2018 16:13:03 +0300	[thread overview]
Message-ID: <20180618131303.GE20518@intel.com> (raw)
In-Reply-To: <1529096952.2740.37.camel@intel.com>

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. */

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx