Re: [PATCH] drm/i915: Check VBT for port presence in addition to the strap on VLV/CHV

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jun 03, 2016 at 10:56:24AM +0100, Chris Wilson wrote:
> On Fri, Jun 03, 2016 at 12:17:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently some CHV boards failed to hook up the port presence straps
> > for HDMI ports as well (earlier we assumed this problem only affected
> > eDP ports). So let's check the VBT in addition to the strap, and if
> > either one claims that the port is present go ahead and register the
> > relevant connector.
> > 
> > While at it, change port D to register DP before HDMI as we do for ports
> > B and C since
> > commit 457c52d87e5d ("drm/i915: Only ignore eDP ports that are connected")
> > 
> > Also print a debug message when we register a HDMI connector to aid
> > in diagnosing missing/incorrect ports. We already had such a print for
> > DP/eDP.
> > 
> > v2: Improve the comment in the code a bit, note the port D change in
> >     the commit message
> > 
> > Cc: Radoslav Duda <radosd@radosd.com>
> > Tested-by: Radoslav Duda <radosd@radosd.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96321
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_bios.c    | 39 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_hdmi.c    |  3 +++
> >  4 files changed, 64 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 96d5034830f0..be9a6390b435 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3619,6 +3619,7 @@ int intel_bios_init(struct drm_i915_private *dev_priv);
> >  bool intel_bios_is_valid_vbt(const void *buf, size_t size);
> >  bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv);
> >  bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin);
> > +bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port);
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 713a02db378a..da5ed4a850b9 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1570,6 +1570,45 @@ bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin)
> >  }
> >  
> >  /**
> > + * intel_bios_is_port_present - is the specified digital port present
> > + * @dev_priv:	i915 device instance
> > + * @port:	port to check
> > + *
> > + * Return true if the device in %port is present.
> > + */
> > +bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port port)
> > +{
> > +	static const struct {
> > +		u16 dp, hdmi;
> > +	} port_mapping[] = {
> 
> 		/* FIXME maybe deal with port A as well? */
> 
> > +		[PORT_B] = { DVO_PORT_DPB, DVO_PORT_HDMIB, },
> > +		[PORT_C] = { DVO_PORT_DPC, DVO_PORT_HDMIC, },
> > +		[PORT_D] = { DVO_PORT_DPD, DVO_PORT_HDMID, },
> > +		[PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
> > +	};
> > +	int i;
> > +
> > +	/* FIXME maybe deal with port A as well? */
> > +	if (WARN_ON(port == PORT_A) || port >= ARRAY_SIZE(port_mapping))
> 
> if (WARN_ON(port >= ARRAY_SIZE() || !port_mapping[port]))
> 
> looks a bit more generic?

Hmm. DVO_PORT_HDMIA==0, so if we'd start to handle port A here, we'd
have to make sure to use the dp port type for the null check. But yeah,
your idea here seem fairly sane to me. Though I'm thinking someone
should perhaps try to refactor all of these port type check functions
a bit. Would be nice if every one of them didn't have copy paste the
child dev walk. I think I'll leave these plans to simmer a bit, and
jump on them later if/when I'm bored.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks.

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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