Re: [Intel-gfx] [PATCH 2/4] drm/i915: Respect alternate_ddc_pin for all DDI ports

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 13, 2016 at 09:29:30PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 11:06:55AM -0700, Jim Bride wrote:
> > On Tue, Oct 11, 2016 at 08:52:46PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > 
> > > The VBT provides the platform a way to mix and match the DDI ports vs.
> > > GMBUS pins. Currently we only trust the VBT for DDI E, which I suppose
> > > has no standard GMBUS pin assignment. However, there are machines out
> > > there that use a non-standard mapping for the other ports as well.
> > > Let's start trusting the VBT on this one for all ports on DDI platforms.
> > > 
> > > I've structured the code such that other platforms could easily start
> > > using this as well, by simply filling in the ddi_port_info. IIRC there
> > > may be CHV system that might actually need this.
> > > 
> > > v2: Include a commit message, include a debug message during init
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Cc: Maarten Maathuis <madman2003@xxxxxxxxx>
> > > Tested-by: Maarten Maatt show huis <madman2003@xxxxxxxxx>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97877
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 84 ++++++++++++++++++++++-----------------
> > >  1 file changed, 48 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 8d46f5836746..9ca86e901fc8 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1799,6 +1799,50 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
> > >  	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > >  }
> > >  
> > > +static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> > > +			     enum port port)
> > > +{
> > > +	const struct ddi_vbt_port_info *info =
> > > +		&dev_priv->vbt.ddi_port_info[port];
> > > +	u8 ddc_pin;
> > > +
> > > +	if (info->alternate_ddc_pin) {
> > > +		DRM_DEBUG_KMS("Using DDC pin 0x%x for port %c (VBT)\n",
> > > +			      info->alternate_ddc_pin, port_name(port));
> > > +		return info->alternate_ddc_pin;
> > > +	}
> > > +
> > > +	switch (port) {
> > > +	case PORT_B:
> > > +		if (IS_BROXTON(dev_priv))
> > > +			ddc_pin = GMBUS_PIN_1_BXT;
> > > +		else
> > > +			ddc_pin = GMBUS_PIN_DPB;
> > > +		break;
> > > +	case PORT_C:
> > > +		if (IS_BROXTON(dev_priv))
> > > +			ddc_pin = GMBUS_PIN_2_BXT;
> > > +		else
> > > +			ddc_pin = GMBUS_PIN_DPC;
> > > +		break;
> > > +	case PORT_D:
> > > +		if (IS_CHERRYVIEW(dev_priv))
> > > +			ddc_pin = GMBUS_PIN_DPD_CHV;
> > > +		else
> > > +			ddc_pin = GMBUS_PIN_DPD;
> > 
> > In the code removed below there's a specific case covering Broxton that
> > isn't accounted for here.  Are we sure that we don't need that logic here?
> 
> Yeah, BXT doesn't have port D.
> 
> I guess we could split this up into a few platform specific functions that
> could then have the MISSING_CASE() for the impossible ports. A bit like
> the AUX ctl/data register setup functions we have. Do pople want that
> level of paranoia here?

tbh seems overkill, we should go boom in the platform gmbus code if you
end up with an impossible port.
-Daniel

> 
> > 
> > Jim
> > 
> > > +		break;
> > > +	default:
> > > +		MISSING_CASE(port);
> > > +		ddc_pin = GMBUS_PIN_DPB;
> > > +		break;
> > > +	}
> > > +
> > > +	DRM_DEBUG_KMS("Using DDC pin 0x%x for port %c (platform default)\n",
> > > +		      ddc_pin, port_name(port));
> > > +
> > > +	return ddc_pin;
> > > +}
> > > +
> > >  void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >  			       struct intel_connector *intel_connector)
> > >  {
> > > @@ -1808,7 +1852,6 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >  	struct drm_device *dev = intel_encoder->base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum port port = intel_dig_port->port;
> > > -	uint8_t alternate_ddc_pin;
> > >  
> > >  	DRM_DEBUG_KMS("Adding HDMI connector on port %c\n",
> > >  		      port_name(port));
> > > @@ -1826,12 +1869,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >  	connector->doublescan_allowed = 0;
> > >  	connector->stereo_allowed = 1;
> > >  
> > > +	intel_hdmi->ddc_bus = intel_hdmi_ddc_pin(dev_priv, port);
> > > +
> > >  	switch (port) {
> > >  	case PORT_B:
> > > -		if (IS_BROXTON(dev_priv))
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_1_BXT;
> > > -		else
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPB;
> > >  		/*
> > >  		 * On BXT A0/A1, sw needs to activate DDIA HPD logic and
> > >  		 * interrupts to check the external panel connection.
> > > @@ -1842,46 +1883,17 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >  			intel_encoder->hpd_pin = HPD_PORT_B;
> > >  		break;
> > >  	case PORT_C:
> > > -		if (IS_BROXTON(dev_priv))
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_2_BXT;
> > > -		else
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPC;
> > >  		intel_encoder->hpd_pin = HPD_PORT_C;
> > >  		break;
> > >  	case PORT_D:
> > > -		if (WARN_ON(IS_BROXTON(dev_priv)))
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DISABLED;
> > > -		else if (IS_CHERRYVIEW(dev_priv))
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPD_CHV;
> > > -		else
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPD;
> > >  		intel_encoder->hpd_pin = HPD_PORT_D;
> > >  		break;
> > >  	case PORT_E:
> > > -		/* On SKL PORT E doesn't have seperate GMBUS pin
> > > -		 *  We rely on VBT to set a proper alternate GMBUS pin. */
> > > -		alternate_ddc_pin =
> > > -			dev_priv->vbt.ddi_port_info[PORT_E].alternate_ddc_pin;
> > > -		switch (alternate_ddc_pin) {
> > > -		case DDC_PIN_B:
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPB;
> > > -			break;
> > > -		case DDC_PIN_C:
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPC;
> > > -			break;
> > > -		case DDC_PIN_D:
> > > -			intel_hdmi->ddc_bus = GMBUS_PIN_DPD;
> > > -			break;
> > > -		default:
> > > -			MISSING_CASE(alternate_ddc_pin);
> > > -		}
> > >  		intel_encoder->hpd_pin = HPD_PORT_E;
> > >  		break;
> > > -	case PORT_A:
> > > -		intel_encoder->hpd_pin = HPD_PORT_A;
> > > -		/* Internal port only for eDP. */
> > >  	default:
> > > -		BUG();
> > > +		MISSING_CASE(port);
> > > +		return;
> > >  	}
> > >  
> > >  	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]