From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Try to initialize eDP for both ports B and C on VLV Date: Thu, 31 Oct 2013 21:05:05 +0200 Message-ID: <20131031190504.GE13047@intel.com> References: <1383237982-18995-1-git-send-email-ville.syrjala@linux.intel.com> <20131031101800.184f2179@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E45BEEEB9E for ; Thu, 31 Oct 2013 12:05:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20131031101800.184f2179@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Oct 31, 2013 at 10:18:00AM -0700, Jesse Barnes wrote: > On Thu, 31 Oct 2013 18:46:22 +0200 > ville.syrjala@linux.intel.com wrote: > = > > From: Ville Syrj=E4l=E4 > > = > > On VLV both ports B and C can support either DP or eDP. Try to > > initialize eDP first on each port, and if that fails fall back to > > regular DP connector. > > = > > intel_dp_init_typed() is now like the old intel_dp_init, except you pass > > in the connector type. If you pass DRM_MODE_CONNECTOR_Unknown, the code > > falls back to the old port/VBT based auto detection to determine the > > actual type. > > = > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D71051 > > Tested-by: Robert Hooker > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 3 +- > > drivers/gpu/drm/i915/intel_display.c | 11 ++++-- > > drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++------= -------- > > drivers/gpu/drm/i915/intel_drv.h | 5 ++- > > 4 files changed, 54 insertions(+), 31 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/in= tel_ddi.c > > index 31f4fe2..dd2660c 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1356,7 +1356,8 @@ intel_ddi_init_dp_connector(struct intel_digital_= port *intel_dig_port) > > return NULL; > > = > > intel_dig_port->dp.output_reg =3D DDI_BUF_CTL(port); > > - if (!intel_dp_init_connector(intel_dig_port, connector)) { > > + if (!intel_dp_init_connector(intel_dig_port, connector, > > + DRM_MODE_CONNECTOR_Unknown)) { > > kfree(connector); > > return NULL; > > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 8f40ae3..16225eb 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9984,15 +9984,20 @@ static void intel_setup_outputs(struct drm_devi= ce *dev) > > intel_hdmi_init(dev, VLV_DISPLAY_BASE + GEN4_HDMIB, > > PORT_B); > > if (I915_READ(VLV_DISPLAY_BASE + DP_B) & DP_DETECTED) > > - intel_dp_init(dev, VLV_DISPLAY_BASE + DP_B, PORT_B); > > + if (!intel_dp_init_typed(dev, VLV_DISPLAY_BASE + DP_B, > > + PORT_B, DRM_MODE_CONNECTOR_eDP)) > > + intel_dp_init_typed(dev, VLV_DISPLAY_BASE + DP_B, > > + PORT_B, DRM_MODE_CONNECTOR_DisplayPort); > > } > > = > > if (I915_READ(VLV_DISPLAY_BASE + GEN4_HDMIC) & SDVO_DETECTED) { > > intel_hdmi_init(dev, VLV_DISPLAY_BASE + GEN4_HDMIC, > > PORT_C); > > if (I915_READ(VLV_DISPLAY_BASE + DP_C) & DP_DETECTED) > > - intel_dp_init(dev, VLV_DISPLAY_BASE + DP_C, > > - PORT_C); > > + if (!intel_dp_init_typed(dev, VLV_DISPLAY_BASE + DP_C, > > + PORT_C, DRM_MODE_CONNECTOR_eDP)) > > + intel_dp_init_typed(dev, VLV_DISPLAY_BASE + DP_C, > > + PORT_C, DRM_MODE_CONNECTOR_DisplayPort); > > } > > = > > intel_dsi_init(dev); > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/int= el_dp.c > > index b3cc333..a97f186 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3540,7 +3540,8 @@ static bool intel_edp_init_connector(struct intel= _dp *intel_dp, > > = > > bool > > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > - struct intel_connector *intel_connector) > > + struct intel_connector *intel_connector, > > + int type) > > { > > struct drm_connector *connector =3D &intel_connector->base; > > struct intel_dp *intel_dp =3D &intel_dig_port->dp; > > @@ -3549,33 +3550,32 @@ intel_dp_init_connector(struct intel_digital_po= rt *intel_dig_port, > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > enum port port =3D intel_dig_port->port; > > const char *name =3D NULL; > > - int type, error; > > + int error; > > = > > /* Preserve the current hw state. */ > > intel_dp->DP =3D I915_READ(intel_dp->output_reg); > > intel_dp->attached_connector =3D intel_connector; > > = > > - type =3D DRM_MODE_CONNECTOR_DisplayPort; > > - /* > > - * FIXME : We need to initialize built-in panels before external pane= ls. > > - * For X0, DP_C is fixed as eDP. Revisit this as part of VLV eDP clea= nup > > - */ > > - switch (port) { > > - case PORT_A: > > - type =3D DRM_MODE_CONNECTOR_eDP; > > - break; > > - case PORT_C: > > - if (IS_VALLEYVIEW(dev)) > > - type =3D DRM_MODE_CONNECTOR_eDP; > > - break; > > - case PORT_D: > > - if (HAS_PCH_SPLIT(dev) && intel_dpd_is_edp(dev)) > > + if (type =3D=3D DRM_MODE_CONNECTOR_Unknown) { > > + type =3D DRM_MODE_CONNECTOR_DisplayPort; > > + > > + switch (port) { > > + case PORT_A: > > type =3D DRM_MODE_CONNECTOR_eDP; > > - break; > > - default: /* silence GCC warning */ > > - break; > > + break; > > + case PORT_D: > > + if (HAS_PCH_SPLIT(dev) && intel_dpd_is_edp(dev)) > > + type =3D DRM_MODE_CONNECTOR_eDP; > > + break; > > + default: /* silence GCC warning */ > > + break; > > + } > > } > > = > > + if (WARN_ON(type !=3D DRM_MODE_CONNECTOR_DisplayPort && > > + type !=3D DRM_MODE_CONNECTOR_eDP)) > > + return false; > > + > > /* > > * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but > > * for DP the encoder type can be set by the caller to > > @@ -3598,7 +3598,11 @@ intel_dp_init_connector(struct intel_digital_por= t *intel_dig_port, > > ironlake_panel_vdd_work); > > = > > intel_connector_attach_encoder(intel_connector, intel_encoder); > > - drm_sysfs_connector_add(connector); > > + error =3D drm_sysfs_connector_add(connector); > > + if (error) { > > + drm_connector_cleanup(connector); > > + return false; > > + } > > = > > if (HAS_DDI(dev)) > > intel_connector->get_hw_state =3D intel_ddi_connector_get_hw_state; > > @@ -3680,8 +3684,9 @@ intel_dp_init_connector(struct intel_digital_port= *intel_dig_port, > > return true; > > } > > = > > -void > > -intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > +bool > > +intel_dp_init_typed(struct drm_device *dev, int output_reg, > > + enum port port, int type) > > { > > struct intel_digital_port *intel_dig_port; > > struct intel_encoder *intel_encoder; > > @@ -3690,12 +3695,12 @@ intel_dp_init(struct drm_device *dev, int outpu= t_reg, enum port port) > > = > > intel_dig_port =3D kzalloc(sizeof(*intel_dig_port), GFP_KERNEL); > > if (!intel_dig_port) > > - return; > > + return false; > > = > > intel_connector =3D kzalloc(sizeof(*intel_connector), GFP_KERNEL); > > if (!intel_connector) { > > kfree(intel_dig_port); > > - return; > > + return false; > > } > > = > > intel_encoder =3D &intel_dig_port->base; > > @@ -3727,9 +3732,18 @@ intel_dp_init(struct drm_device *dev, int output= _reg, enum port port) > > intel_encoder->cloneable =3D false; > > intel_encoder->hot_plug =3D intel_dp_hot_plug; > > = > > - if (!intel_dp_init_connector(intel_dig_port, intel_connector)) { > > + if (!intel_dp_init_connector(intel_dig_port, intel_connector, type)) { > > drm_encoder_cleanup(encoder); > > kfree(intel_dig_port); > > kfree(intel_connector); > > + return false; > > } > > + > > + return true; > > +} > > + > > +void > > +intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > +{ > > + intel_dp_init_typed(dev, output_reg, port, DRM_MODE_CONNECTOR_Unknown= ); > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 9d2624f..ff982d9 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -697,8 +697,11 @@ void intel_display_set_init_power(struct drm_devic= e *dev, bool enable); > > = > > /* intel_dp.c */ > > void intel_dp_init(struct drm_device *dev, int output_reg, enum port p= ort); > > +bool intel_dp_init_typed(struct drm_device *dev, int output_reg, > > + enum port port, int type); > > bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > - struct intel_connector *intel_connector); > > + struct intel_connector *intel_connector, > > + int type); > > void intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_complete_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > = > We're supposed to get this info from the VBT, but we don't have the new > spec yet. If we get that, we can parse it out in intel_bios.c and use > it in intel_dp.c. > = > In cases where that may be unreliable, your approach might be fine, > though I suppose the typed init addition should be separate from the > "try both kinds" addition on VLV. Yeah this patch is just wrong. I guess I had imagined a better world where the specs would actually be sane and the panel would always tell us what type it is. We could try to tell eDP and DP apart first via the optional eDP DPCD registers, but if that doesn't confirm that it's eDP, we'd need to do something silly like trying to talk to the panel w/ vdd off. If we get an answer only w/ vdd on, then it would eDP. But I'm going to try the VBT approach now, based on the spec Paulo had. -- = Ville Syrj=E4l=E4 Intel OTC