[Intel-gfx] [PATCH V2] drm/i915/glk: Add Quirk for GLK NUC HDMI port issues.

[Imre Deak]

On Wed, Jun 27, 2018 at 03:26:54PM -0700, Clint Taylor wrote:
> 
> 
> On 06/25/2018 03:33 AM, Imre Deak wrote:
> > On Wed, Jun 13, 2018 at 02:48:49PM -0700, clinton.a.taylor at intel.com wrote:
> > > From: Clint Taylor <clinton.a.taylor at intel.com>
> > > 
> > > On GLK NUC platforms the HDMI retiming buffer needs additional disabled
> > > time to correctly sync to a faster incoming signal.
> > > 
> > > When measured on a scope the highspeed lines of the HDMI clock turn off
> > >   for ~400uS during a normal resolution change. The HDMI retimer on the
> > >   GLK NUC appears to require at least a full frame of quiet time before a
> > > new faster clock can be correctly sync'd. The worst case scenario appears
> > > to be 23.98Hz modes which requires a wait of 41.25ms. Add a quirk to the
> > > driver for GLK NUC that waits 42ms.
> > > 
> > > V2: Add more devices to the quirk list
> > > 
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105887
> > > Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >   drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
> > >   drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
> > >   3 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index be8c2f0..da196b4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -656,6 +656,7 @@ enum intel_sbi_destination {
> > >   #define QUIRK_BACKLIGHT_PRESENT (1<<3)
> > >   #define QUIRK_PIN_SWIZZLED_PAGES (1<<5)
> > >   #define QUIRK_INCREASE_T12_DELAY (1<<6)
> > > +#define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7)
> > >   struct intel_fbdev;
> > >   struct intel_fbc_work;
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index ca73387..bc3d012 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1791,6 +1791,9 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
> > >   	I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), temp);
> > >   }
> > > +/* Quirk time computed based on 24fps frame time of 41.25ms */
> > > +#define DDI_DISABLED_QUIRK_TIME 42
> > > +
> > >   void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> > >   				       enum transcoder cpu_transcoder)
> > >   {
> > > @@ -1800,6 +1803,11 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> > >   	val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK | TRANS_DDI_DP_VC_PAYLOAD_ALLOC);
> > >   	val |= TRANS_DDI_PORT_NONE;
> > >   	I915_WRITE(reg, val);
> > > +
> > > +	if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME) {
> > > +		msleep(DDI_DISABLED_QUIRK_TIME);
> > > +		DRM_DEBUG_KMS("Quirk Increase DDI disabled time\n");
> > No need for the define or the debug message here imo. msleep() can be
> > inaccurate (even sleeping less than expected), so I'd use a bigger
> > margin, sleeping say 100ms.
> 
> Easy to do and I can submit v3.
> 
> > 
> > If the problem is with the HDMI retimer chip, we should limit the quirk
> > to HDMI outputs.
> Output type is not available in intel_disable_ddi() and I would prefer not
> to change the interface only for the quirk. I could move the quirk execution
> up a level to haswell_crtc_disable() and detect the old_crtc_state->type
> before executing the quirk. However, the quirk is only being detected for
> certain boards that only have HDMI outputs. Unless more boards are added to
> the quirk_list[] we really don't need logic to detect HDMI outputs.

Ok, but I'm sure we'll forget about this detail once we have to update
the list with a GLK having a DP output too. OTOH, changing
intel_ddi_disable_transcoder_func() to accept
struct intel_crtc_state *crtc_state would make it symmetric with
intel_ddi_enable_transcoder_func(), so imo a good change on its own.

--Imre

> 
> -Clint
> 
> > > +	}
> > >   }
> > >   int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 8251e18..40e0306 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14749,6 +14749,17 @@ static void quirk_increase_t12_delay(struct drm_device *dev)
> > >   	DRM_INFO("Applying T12 delay quirk\n");
> > >   }
> > > +/* GeminiLake NUC HDMI outputs require additional off time
> > > + * this allows the onboard retimer to correctly sync to signal
> > > + */
> > > +static void quirk_increase_ddi_disabled_time(struct drm_device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +	dev_priv->quirks |= QUIRK_INCREASE_DDI_DISABLED_TIME;
> > > +	DRM_INFO("Applying Increase DDI Disabled quirk\n");
> > > +}
> > > +
> > >   struct intel_quirk {
> > >   	int device;
> > >   	int subsystem_vendor;
> > > @@ -14835,6 +14846,14 @@ static int intel_dmi_reverse_brightness(const struct dmi_system_id *id)
> > >   	/* Toshiba Satellite P50-C-18C */
> > >   	{ 0x191B, 0x1179, 0xF840, quirk_increase_t12_delay },
> > > +
> > > +	/* GeminiLake NUC */
> > > +	{ 0x3185, 0x8086, 0x2072, quirk_increase_ddi_disabled_time },
> > > +	{ 0x3184, 0x8086, 0x2072, quirk_increase_ddi_disabled_time },
> > > +	/* ASRock ITX*/
> > > +	{ 0x3185, 0x1849, 0x2212, quirk_increase_ddi_disabled_time },
> > > +	{ 0x3184, 0x1849, 0x2212, quirk_increase_ddi_disabled_time },
> > > +
> > Extra w/s. A quirk list looks ok for now since we can't identify the
> > retimer chip in any other way.
> > 
> > >   };
> > >   static void intel_init_quirks(struct drm_device *dev)
> > > -- 
> > > 1.9.1
> > > 
>