| Message ID | 1438994022-12437-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Vivi, Rodrigo > Sent: Saturday, August 8, 2015 8:34 AM > To: intel-gfx@lists.freedesktop.org > Cc: Vivi, Rodrigo; Zhang, Xiong Y > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes. > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we need to > configure lane count propperly for both. > > This was based on Sonika's > [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 110d546..557cecf 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device *dev, enum > port port) > struct intel_digital_port *intel_dig_port; > struct intel_encoder *intel_encoder; > struct drm_encoder *encoder; > - bool init_hdmi, init_dp; > + bool init_hdmi, init_dp, ddi_e_present; > + > + /* > + * On SKL we don't have a way to detect DDI-E so we rely on VBT. > + */ > + ddie_present = IS_SKYLAKE(dev) && > + (dev_priv->vbt.ddi_port_info[PORT_E].supports_dp || > + dev_priv->vbt.ddi_port_info[PORT_E].supports_dvi || > + dev_priv->vbt.ddi_port_info[PORT_E].supports_hdmi); [Zhang, Xiong Y] ddie_present should be ddi_e_present > > init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || > dev_priv->vbt.ddi_port_info[port].supports_hdmi); > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device *dev, enum > port port) > intel_dig_port->port = port; > intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & > (DDI_BUF_PORT_REVERSAL | > - DDI_A_4_LANES); > + ddi_e_present ? 0 : DDI_A_4_LANES); [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when DDI-E doesn't exist, I think your patch will do nothing. > > intel_encoder->type = INTEL_OUTPUT_UNKNOWN; > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); diff --git > a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index > 3ff2080..7ada79e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -159,9 +159,11 @@ static u8 intel_dp_max_lane_count(struct intel_dp > *intel_dp) > u8 source_max, sink_max; > > source_max = 4; > - if (HAS_DDI(dev) && intel_dig_port->port == PORT_A && > - (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == 0) > - source_max = 2; > + if (HAS_DDI(dev) && (intel_dig_port->port == PORT_E || > + (intel_dig_port->port == PORT_A && > + (intel_dig_port->saved_port_bits & > + DDI_A_4_LANES) == 0)) > + source_max = 2; [Zhang, Xiong Y] it miss ')' at the end line. > > sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > -- > 2.4.3
On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > -----Original Message----- > > From: Vivi, Rodrigo > > Sent: Saturday, August 8, 2015 8:34 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes. > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we > > need to > > configure lane count propperly for both. > > > > This was based on Sonika's > > [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 110d546..557cecf 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device *dev, > > enum > > port port) > > struct intel_digital_port *intel_dig_port; > > struct intel_encoder *intel_encoder; > > struct drm_encoder *encoder; > > - bool init_hdmi, init_dp; > > + bool init_hdmi, init_dp, ddi_e_present; > > + > > + /* > > + * On SKL we don't have a way to detect DDI-E so we rely > > on VBT. > > + */ > > + ddie_present = IS_SKYLAKE(dev) && > > + (dev_priv->vbt.ddi_port_info[PORT_E].supports_dp > > || > > + dev_priv->vbt.ddi_port_info[PORT_E].supports_dvi > > || > > + dev_priv > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > init_hdmi = (dev_priv > > ->vbt.ddi_port_info[port].supports_dvi || > > dev_priv > > ->vbt.ddi_port_info[port].supports_hdmi); > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device *dev, > > enum > > port port) > > intel_dig_port->port = port; > > intel_dig_port->saved_port_bits = > > I915_READ(DDI_BUF_CTL(port)) & > > (DDI_BUF_PORT_REVERSAL | > > - DDI_A_4_LANES); > > + ddi_e_present ? 0 : > > DDI_A_4_LANES); > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when DDI-E > doesn't exist, > I think your patch will do nothing. Actually DDI_A_4_LANES is being already set unconditionally, so Sonika's patch has no effect. saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and also it is used to calculate the number of lanes. With this patch we stop setting DDI_A_4_LANES when ddi_e is present so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E > > > > intel_encoder->type = INTEL_OUTPUT_UNKNOWN; > > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > > diff --git > > a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index > > 3ff2080..7ada79e 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -159,9 +159,11 @@ static u8 intel_dp_max_lane_count(struct > > intel_dp > > *intel_dp) > > u8 source_max, sink_max; > > > > source_max = 4; > > - if (HAS_DDI(dev) && intel_dig_port->port == PORT_A && > > - (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == > > 0) > > - source_max = 2; > > + if (HAS_DDI(dev) && (intel_dig_port->port == PORT_E || > > + (intel_dig_port->port == PORT_A && > > + (intel_dig_port->saved_port_bits & > > + DDI_A_4_LANES) == 0)) > > + source_max = 2; > [Zhang, Xiong Y] it miss ')' at the end line. > > > > sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > > > -- > > 2.4.3 >
> On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > > -----Original Message----- > > > From: Vivi, Rodrigo > > > Sent: Saturday, August 8, 2015 8:34 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes. > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we need > > > to configure lane count propperly for both. > > > > > > This was based on Sonika's > > > [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt > > > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > index 110d546..557cecf 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device *dev, > > > enum port port) > > > struct intel_digital_port *intel_dig_port; > > > struct intel_encoder *intel_encoder; > > > struct drm_encoder *encoder; > > > - bool init_hdmi, init_dp; > > > + bool init_hdmi, init_dp, ddi_e_present; > > > + > > > + /* > > > + * On SKL we don't have a way to detect DDI-E so we rely > > > on VBT. > > > + */ > > > + ddie_present = IS_SKYLAKE(dev) && > > > + (dev_priv->vbt.ddi_port_info[PORT_E].supports_dp > > > || > > > + dev_priv->vbt.ddi_port_info[PORT_E].supports_dvi > > > || > > > + dev_priv > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > > > init_hdmi = (dev_priv > > > ->vbt.ddi_port_info[port].supports_dvi || > > > dev_priv > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device *dev, > > > enum port port) > > > intel_dig_port->port = port; > > > intel_dig_port->saved_port_bits = > > > I915_READ(DDI_BUF_CTL(port)) & > > > (DDI_BUF_PORT_REVERSAL | > > > - DDI_A_4_LANES); > > > + ddi_e_present ? 0 : > > > DDI_A_4_LANES); > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when DDI-E > > doesn't exist, I think your patch will do nothing. > > Actually DDI_A_4_LANES is being already set unconditionally, so Sonika's > patch has no effect. [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under many conditions. + if (IS_SKYLAKE(dev) && port == PORT_A + && !(val & DDI_BUF_CTL_ENABLE) + && !dev_priv->vbt.ddi_e_used) + I915_WRITE(DDI_BUF_CTL(port), val | DDI_A_4_LANES) > > saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and also it is > used to calculate the number of lanes. > > With this patch we stop setting DDI_A_4_LANES when ddi_e is present so > DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES when ddi_e is present. But this patch won't set DDI_A_4_LANES under following conditions which is purpose for Sonika patch 1. Bios fail to driver eDP and doesn't enable DDI_A buffer 2. Bios clear DDI_A_4_LANES 3. DDI_E isn't present thanks >
On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: > > On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > > > -----Original Message----- > > > > From: Vivi, Rodrigo > > > > Sent: Saturday, August 8, 2015 8:34 AM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 > > > > lanes. > > > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we > > > > need > > > > to configure lane count propperly for both. > > > > > > > > This was based on Sonika's > > > > [PATCH] drm/i915/skl: Select DDIA lane capability based upon > > > > vbt > > > > > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > index 110d546..557cecf 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device > > > > *dev, > > > > enum port port) > > > > struct intel_digital_port *intel_dig_port; > > > > struct intel_encoder *intel_encoder; > > > > struct drm_encoder *encoder; > > > > - bool init_hdmi, init_dp; > > > > + bool init_hdmi, init_dp, ddi_e_present; > > > > + > > > > + /* > > > > + * On SKL we don't have a way to detect DDI-E so we > > > > rely > > > > on VBT. > > > > + */ > > > > + ddie_present = IS_SKYLAKE(dev) && > > > > + (dev_priv > > > > ->vbt.ddi_port_info[PORT_E].supports_dp > > > > > > > > > > + dev_priv > > > > ->vbt.ddi_port_info[PORT_E].supports_dvi > > > > > > > > > > + dev_priv > > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > > > > > init_hdmi = (dev_priv > > > > ->vbt.ddi_port_info[port].supports_dvi || > > > > dev_priv > > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device > > > > *dev, > > > > enum port port) > > > > intel_dig_port->port = port; > > > > intel_dig_port->saved_port_bits = > > > > I915_READ(DDI_BUF_CTL(port)) & > > > > > > > > (DDI_BUF_PORT_REVERSAL | > > > > - DDI_A_4_LANES); > > > > + ddi_e_present ? 0 : > > > > DDI_A_4_LANES); > > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when > > > DDI-E > > > doesn't exist, I think your patch will do nothing. > > > > Actually DDI_A_4_LANES is being already set unconditionally, so > > Sonika's > > patch has no effect. > [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under many > conditions. > + if (IS_SKYLAKE(dev) && port == PORT_A > + && !(val & DDI_BUF_CTL_ENABLE) > + && !dev_priv->vbt.ddi_e_used) > + I915_WRITE(DDI_BUF_CTL(port), val | DDI_A_4_LANES) > > > > saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and > > also it is > > used to calculate the number of lanes. > > > > With this patch we stop setting DDI_A_4_LANES when ddi_e is present > > so > > DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E > [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES when ddi_e > is present. > But this patch won't set DDI_A_4_LANES under following conditions > which is purpose for Sonika patch > 1. Bios fail to driver eDP and doesn't enable DDI_A buffer If DDI_A isn't enabled we don't need to set DDI_A_4_LANES > 2. Bios clear DDI_A_4_LANES > 3. DDI_E isn't present I don't agree... This is already covered on current code. DDI_A_4_LANES is already being set when enabling DDI_A. > > thanks > >
On 11.08.2015 10:05, Zhang, Xiong Y wrote: >> -----Original Message----- >> From: Vivi, Rodrigo >> Sent: Saturday, August 8, 2015 8:34 AM >> To: intel-gfx@lists.freedesktop.org >> Cc: Vivi, Rodrigo; Zhang, Xiong Y >> Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes. >> >> DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we need to >> configure lane count propperly for both. >> >> This was based on Sonika's >> [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt >> >> Credits-to: Sonika Jindal <sonika.jindal@intel.com> >> Cc: Xiong Zhang <xiong.y.zhang@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- >> drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- >> 2 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 110d546..557cecf 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device *dev, enum >> port port) >> struct intel_digital_port *intel_dig_port; >> struct intel_encoder *intel_encoder; >> struct drm_encoder *encoder; >> - bool init_hdmi, init_dp; >> + bool init_hdmi, init_dp, ddi_e_present; >> + >> + /* >> + * On SKL we don't have a way to detect DDI-E so we rely on VBT. >> + */ >> + ddie_present = IS_SKYLAKE(dev) && >> + (dev_priv->vbt.ddi_port_info[PORT_E].supports_dp || >> + dev_priv->vbt.ddi_port_info[PORT_E].supports_dvi || >> + dev_priv->vbt.ddi_port_info[PORT_E].supports_hdmi); > [Zhang, Xiong Y] ddie_present should be ddi_e_present >> >> init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || >> dev_priv->vbt.ddi_port_info[port].supports_hdmi); >> @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device *dev, enum >> port port) >> intel_dig_port->port = port; >> intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & >> (DDI_BUF_PORT_REVERSAL | >> - DDI_A_4_LANES); >> + ddi_e_present ? 0 : DDI_A_4_LANES); > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when DDI-E doesn't exist, > I think your patch will do nothing. >> >> intel_encoder->type = INTEL_OUTPUT_UNKNOWN; >> intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); diff --git >> a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index >> 3ff2080..7ada79e 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -159,9 +159,11 @@ static u8 intel_dp_max_lane_count(struct intel_dp >> *intel_dp) >> u8 source_max, sink_max; >> >> source_max = 4; >> - if (HAS_DDI(dev) && intel_dig_port->port == PORT_A && >> - (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == 0) >> - source_max = 2; >> + if (HAS_DDI(dev) && (intel_dig_port->port == PORT_E || >> + (intel_dig_port->port == PORT_A && >> + (intel_dig_port->saved_port_bits & >> + DDI_A_4_LANES) == 0)) >> + source_max = 2; > [Zhang, Xiong Y] it miss ')' at the end line. >> >> sink_max = drm_dp_max_lane_count(intel_dp->dpcd); with these build fixes the series is Tested-by: Timo Aaltonen <timo.aaltonen@canonical.com> the VGA port on SKL-H SDP was functional after all.. (DP-to-VGA)
> On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: > > > On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > > > > -----Original Message----- > > > > > From: Vivi, Rodrigo > > > > > Sent: Saturday, August 8, 2015 8:34 AM > > > > > To: intel-gfx@lists.freedesktop.org > > > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 > > > > > lanes. > > > > > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we > > > > > need to configure lane count propperly for both. > > > > > > > > > > This was based on Sonika's > > > > > [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt > > > > > > > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > > index 110d546..557cecf 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device > > > > > *dev, enum port port) > > > > > struct intel_digital_port *intel_dig_port; > > > > > struct intel_encoder *intel_encoder; > > > > > struct drm_encoder *encoder; > > > > > - bool init_hdmi, init_dp; > > > > > + bool init_hdmi, init_dp, ddi_e_present; > > > > > + > > > > > + /* > > > > > + * On SKL we don't have a way to detect DDI-E so we > > > > > rely > > > > > on VBT. > > > > > + */ > > > > > + ddie_present = IS_SKYLAKE(dev) && > > > > > + (dev_priv > > > > > ->vbt.ddi_port_info[PORT_E].supports_dp > > > > > > > > > > > > + dev_priv > > > > > ->vbt.ddi_port_info[PORT_E].supports_dvi > > > > > > > > > > > > + dev_priv > > > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > > > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > > > > > > > init_hdmi = (dev_priv > > > > > ->vbt.ddi_port_info[port].supports_dvi || > > > > > dev_priv > > > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device > > > > > *dev, enum port port) > > > > > intel_dig_port->port = port; > > > > > intel_dig_port->saved_port_bits = > > > > > I915_READ(DDI_BUF_CTL(port)) & > > > > > > > > > > (DDI_BUF_PORT_REVERSAL | > > > > > - DDI_A_4_LANES); > > > > > + ddi_e_present ? 0 : > > > > > DDI_A_4_LANES); > > > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when > > > > DDI-E doesn't exist, I think your patch will do nothing. > > > > > > Actually DDI_A_4_LANES is being already set unconditionally, so > > > Sonika's patch has no effect. > > [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under many > > conditions. > > + if (IS_SKYLAKE(dev) && port == PORT_A > > + && !(val & DDI_BUF_CTL_ENABLE) > > + && !dev_priv->vbt.ddi_e_used) > > + I915_WRITE(DDI_BUF_CTL(port), val | DDI_A_4_LANES) > > > > > > saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and > > > also it is used to calculate the number of lanes. > > > > > > With this patch we stop setting DDI_A_4_LANES when ddi_e is present > > > so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E > > [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES when ddi_e > > is present. > > But this patch won't set DDI_A_4_LANES under following conditions > > which is purpose for Sonika patch 1. Bios fail to driver eDP and > > doesn't enable DDI_A buffer > > If DDI_A isn't enabled we don't need to set DDI_A_4_LANES [Zhang, Xiong Y] From commit message on Sonika patch, she want to set DDI_A_4_LANES on such case. Maybe she met such fail case on one high resolution eDP screen. Let's Sonikia explain it. > > > 2. Bios clear DDI_A_4_LANES > > 3. DDI_E isn't present > > I don't agree... This is already covered on current code. DDI_A_4_LANES is > already being set when enabling DDI_A. > > > > > > thanks > > >
On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: >> On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: >>>> On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: >>>>>> -----Original Message----- >>>>>> From: Vivi, Rodrigo >>>>>> Sent: Saturday, August 8, 2015 8:34 AM >>>>>> To: intel-gfx@lists.freedesktop.org >>>>>> Cc: Vivi, Rodrigo; Zhang, Xiong Y >>>>>> Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 >>>>>> lanes. >>>>>> >>>>>> DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we >>>>>> need to configure lane count propperly for both. >>>>>> >>>>>> This was based on Sonika's >>>>>> [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt >>>>>> >>>>>> Credits-to: Sonika Jindal <sonika.jindal@intel.com> >>>>>> Cc: Xiong Zhang <xiong.y.zhang@intel.com> >>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- >>>>>> drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- >>>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>>>>> b/drivers/gpu/drm/i915/intel_ddi.c >>>>>> index 110d546..557cecf 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>>> @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device >>>>>> *dev, enum port port) >>>>>> struct intel_digital_port *intel_dig_port; >>>>>> struct intel_encoder *intel_encoder; >>>>>> struct drm_encoder *encoder; >>>>>> - bool init_hdmi, init_dp; >>>>>> + bool init_hdmi, init_dp, ddi_e_present; >>>>>> + >>>>>> + /* >>>>>> + * On SKL we don't have a way to detect DDI-E so we >>>>>> rely >>>>>> on VBT. >>>>>> + */ >>>>>> + ddie_present = IS_SKYLAKE(dev) && >>>>>> + (dev_priv >>>>>> ->vbt.ddi_port_info[PORT_E].supports_dp >>>>>>>> >>>>>> + dev_priv >>>>>> ->vbt.ddi_port_info[PORT_E].supports_dvi >>>>>>>> >>>>>> + dev_priv >>>>>> ->vbt.ddi_port_info[PORT_E].supports_hdmi); >>>>> [Zhang, Xiong Y] ddie_present should be ddi_e_present >>>>>> >>>>>> init_hdmi = (dev_priv >>>>>> ->vbt.ddi_port_info[port].supports_dvi || >>>>>> dev_priv >>>>>> ->vbt.ddi_port_info[port].supports_hdmi); >>>>>> @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device >>>>>> *dev, enum port port) >>>>>> intel_dig_port->port = port; >>>>>> intel_dig_port->saved_port_bits = >>>>>> I915_READ(DDI_BUF_CTL(port)) & >>>>>> >>>>>> (DDI_BUF_PORT_REVERSAL | >>>>>> - DDI_A_4_LANES); >>>>>> + ddi_e_present ? 0 : >>>>>> DDI_A_4_LANES); >>>>> [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when >>>>> DDI-E doesn't exist, I think your patch will do nothing. >>>> >>>> Actually DDI_A_4_LANES is being already set unconditionally, so >>>> Sonika's patch has no effect. >>> [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under many >>> conditions. >>> + if (IS_SKYLAKE(dev) && port == PORT_A >>> + && !(val & DDI_BUF_CTL_ENABLE) >>> + && !dev_priv->vbt.ddi_e_used) >>> + I915_WRITE(DDI_BUF_CTL(port), val | DDI_A_4_LANES) >>>> >>>> saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and >>>> also it is used to calculate the number of lanes. >>>> >>>> With this patch we stop setting DDI_A_4_LANES when ddi_e is present >>>> so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E >>> [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES when ddi_e >>> is present. >>> But this patch won't set DDI_A_4_LANES under following conditions >>> which is purpose for Sonika patch 1. Bios fail to driver eDP and >>> doesn't enable DDI_A buffer >> >> If DDI_A isn't enabled we don't need to set DDI_A_4_LANES > [Zhang, Xiong Y] From commit message on Sonika patch, she want to > set DDI_A_4_LANES on such case. Maybe she met such fail case on one high > resolution eDP screen. Let's Sonikia explain it. >> >>> 2. Bios clear DDI_A_4_LANES >>> 3. DDI_E isn't present >> >> I don't agree... This is already covered on current code. DDI_A_4_LANES is >> already being set when enabling DDI_A. >> As Zhang mentioned and as my commit message explains, my patch is needed when bios failed to drive edp (In my case it was an intermediate frequency supported panel which was set to 3.24 GHz and bios didn't have support for intermediate frequencies), it will not enable DDIA in which case, it will not set DDI_BUF_CTL and DDI Lane capability will remain 0 (which is DDIA with 2 lanes and DDIE with 2 lanes). So, since the native resolution of that panel was high and couldn't work with 2 lanes. So, ideally we should not rely on bios to set the initial value and set it based upon whether DDI_E is used or not. So, my patch has some effect :) >> >>> >>> thanks >
On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com> wrote: > On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: >>> On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: >>>>> On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: >>>>>>> -----Original Message----- >>>>>>> From: Vivi, Rodrigo >>>>>>> Sent: Saturday, August 8, 2015 8:34 AM >>>>>>> To: intel-gfx@lists.freedesktop.org >>>>>>> Cc: Vivi, Rodrigo; Zhang, Xiong Y >>>>>>> Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 >>>>>>> lanes. >>>>>>> >>>>>>> DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we >>>>>>> need to configure lane count propperly for both. >>>>>>> >>>>>>> This was based on Sonika's >>>>>>> [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt >>>>>>> >>>>>>> Credits-to: Sonika Jindal <sonika.jindal@intel.com> >>>>>>> Cc: Xiong Zhang <xiong.y.zhang@intel.com> >>>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- >>>>>>> drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- >>>>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> index 110d546..557cecf 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device >>>>>>> *dev, enum port port) >>>>>>> struct intel_digital_port *intel_dig_port; >>>>>>> struct intel_encoder *intel_encoder; >>>>>>> struct drm_encoder *encoder; >>>>>>> - bool init_hdmi, init_dp; >>>>>>> + bool init_hdmi, init_dp, ddi_e_present; >>>>>>> + >>>>>>> + /* >>>>>>> + * On SKL we don't have a way to detect DDI-E so we >>>>>>> rely >>>>>>> on VBT. >>>>>>> + */ >>>>>>> + ddie_present = IS_SKYLAKE(dev) && >>>>>>> + (dev_priv >>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dp >>>>>>>>> >>>>>>> + dev_priv >>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dvi >>>>>>>>> >>>>>>> + dev_priv >>>>>>> ->vbt.ddi_port_info[PORT_E].supports_hdmi); >>>>>> [Zhang, Xiong Y] ddie_present should be ddi_e_present >>>>>>> >>>>>>> init_hdmi = (dev_priv >>>>>>> ->vbt.ddi_port_info[port].supports_dvi || >>>>>>> dev_priv >>>>>>> ->vbt.ddi_port_info[port].supports_hdmi); >>>>>>> @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device >>>>>>> *dev, enum port port) >>>>>>> intel_dig_port->port = port; >>>>>>> intel_dig_port->saved_port_bits = >>>>>>> I915_READ(DDI_BUF_CTL(port)) & >>>>>>> >>>>>>> (DDI_BUF_PORT_REVERSAL | >>>>>>> - DDI_A_4_LANES); >>>>>>> + ddi_e_present ? 0 : >>>>>>> DDI_A_4_LANES); >>>>>> [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes when >>>>>> DDI-E doesn't exist, I think your patch will do nothing. >>>>> >>>>> Actually DDI_A_4_LANES is being already set unconditionally, so >>>>> Sonika's patch has no effect. >>>> [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under many >>>> conditions. >>>> + if (IS_SKYLAKE(dev) && port == PORT_A >>>> + && !(val & DDI_BUF_CTL_ENABLE) >>>> + && !dev_priv->vbt.ddi_e_used) >>>> + I915_WRITE(DDI_BUF_CTL(port), val | DDI_A_4_LANES) >>>>> >>>>> saved_port_bits goes to intel_dp->DP that goes to DDI_BUF_CTL and >>>>> also it is used to calculate the number of lanes. >>>>> >>>>> With this patch we stop setting DDI_A_4_LANES when ddi_e is present >>>>> so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E >>>> [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES when ddi_e >>>> is present. >>>> But this patch won't set DDI_A_4_LANES under following conditions >>>> which is purpose for Sonika patch 1. Bios fail to driver eDP and >>>> doesn't enable DDI_A buffer >>> >>> If DDI_A isn't enabled we don't need to set DDI_A_4_LANES >> [Zhang, Xiong Y] From commit message on Sonika patch, she want to >> set DDI_A_4_LANES on such case. Maybe she met such fail case on one high >> resolution eDP screen. Let's Sonikia explain it. >>> >>>> 2. Bios clear DDI_A_4_LANES >>>> 3. DDI_E isn't present >>> >>> I don't agree... This is already covered on current code. DDI_A_4_LANES is >>> already being set when enabling DDI_A. >>> > As Zhang mentioned and as my commit message explains, my patch is needed > when bios failed to drive edp (In my case it was an intermediate > frequency supported panel which was set to 3.24 GHz and bios didn't have > support for intermediate frequencies), it will not enable DDIA in which > case, it will not set DDI_BUF_CTL and DDI Lane capability will remain 0 > (which is DDIA with 2 lanes and DDIE with 2 lanes). > So, since the native resolution of that panel was high and couldn't work > with 2 lanes. > So, ideally we should not rely on bios to set the initial value and set > it based upon whether DDI_E is used or not. > So, my patch has some effect :) Rodrigo? Please figure out what the needed patch is, and send it. BR, Jani. >>> >>>> >>>> thanks >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2015-08-26 at 11:15 +0300, Jani Nikula wrote: > On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com> > wrote: > > On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: > > > > On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: > > > > > > On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > > > > > > > -----Original Message----- > > > > > > > > From: Vivi, Rodrigo > > > > > > > > Sent: Saturday, August 8, 2015 8:34 AM > > > > > > > > To: intel-gfx@lists.freedesktop.org > > > > > > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > > > > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A > > > > > > > > shares 4 > > > > > > > > lanes. > > > > > > > > > > > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is > > > > > > > > present we > > > > > > > > need to configure lane count propperly for both. > > > > > > > > > > > > > > > > This was based on Sonika's > > > > > > > > [PATCH] drm/i915/skl: Select DDIA lane capability based > > > > > > > > upon vbt > > > > > > > > > > > > > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > > > > > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > > > > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > index 110d546..557cecf 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct > > > > > > > > drm_device > > > > > > > > *dev, enum port port) > > > > > > > > struct intel_digital_port *intel_dig_port; > > > > > > > > struct intel_encoder *intel_encoder; > > > > > > > > struct drm_encoder *encoder; > > > > > > > > - bool init_hdmi, init_dp; > > > > > > > > + bool init_hdmi, init_dp, ddi_e_present; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * On SKL we don't have a way to detect DDI-E > > > > > > > > so we > > > > > > > > rely > > > > > > > > on VBT. > > > > > > > > + */ > > > > > > > > + ddie_present = IS_SKYLAKE(dev) && > > > > > > > > + (dev_priv > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dp > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dvi > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > > > > > > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > > > > > > > > > > > > > init_hdmi = (dev_priv > > > > > > > > ->vbt.ddi_port_info[port].supports_dvi || > > > > > > > > dev_priv > > > > > > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > > > > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct > > > > > > > > drm_device > > > > > > > > *dev, enum port port) > > > > > > > > intel_dig_port->port = port; > > > > > > > > intel_dig_port->saved_port_bits = > > > > > > > > I915_READ(DDI_BUF_CTL(port)) & > > > > > > > > > > > > > > > > (DDI_BUF_PORT_REVERSAL | > > > > > > > > - > > > > > > > > DDI_A_4_LANES); > > > > > > > > + > > > > > > > > ddi_e_present ? 0 : > > > > > > > > DDI_A_4_LANES); > > > > > > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes > > > > > > > when > > > > > > > DDI-E doesn't exist, I think your patch will do nothing. > > > > > > > > > > > > Actually DDI_A_4_LANES is being already set > > > > > > unconditionally, so > > > > > > Sonika's patch has no effect. > > > > > [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under > > > > > many > > > > > conditions. > > > > > + if (IS_SKYLAKE(dev) && port == PORT_A > > > > > + && !(val & DDI_BUF_CTL_ENABLE) > > > > > + && !dev_priv->vbt.ddi_e_used) > > > > > + I915_WRITE(DDI_BUF_CTL(port), val | > > > > > DDI_A_4_LANES) > > > > > > > > > > > > saved_port_bits goes to intel_dp->DP that goes to > > > > > > DDI_BUF_CTL and > > > > > > also it is used to calculate the number of lanes. > > > > > > > > > > > > With this patch we stop setting DDI_A_4_LANES when ddi_e is > > > > > > present > > > > > > so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E > > > > > [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES > > > > > when ddi_e > > > > > is present. > > > > > But this patch won't set DDI_A_4_LANES under following > > > > > conditions > > > > > which is purpose for Sonika patch 1. Bios fail to driver eDP > > > > > and > > > > > doesn't enable DDI_A buffer > > > > > > > > If DDI_A isn't enabled we don't need to set DDI_A_4_LANES > > > [Zhang, Xiong Y] From commit message on Sonika patch, she want to > > > set DDI_A_4_LANES on such case. Maybe she met such fail case on > > > one high > > > resolution eDP screen. Let's Sonikia explain it. > > > > > > > > > 2. Bios clear DDI_A_4_LANES > > > > > 3. DDI_E isn't present > > > > > > > > I don't agree... This is already covered on current code. > > > > DDI_A_4_LANES is > > > > already being set when enabling DDI_A. > > > > > > As Zhang mentioned and as my commit message explains, my patch is > > needed > > when bios failed to drive edp (In my case it was an intermediate > > frequency supported panel which was set to 3.24 GHz and bios didn't > > have > > support for intermediate frequencies), it will not enable DDIA in > > which > > case, it will not set DDI_BUF_CTL and DDI Lane capability will > > remain 0 > > (which is DDIA with 2 lanes and DDIE with 2 lanes). > > So, since the native resolution of that panel was high and couldn't > > work > > with 2 lanes. > > So, ideally we should not rely on bios to set the initial value and > > set > > it based upon whether DDI_E is used or not. > > So, my patch has some effect :) > > Rodrigo? Please figure out what the needed patch is, and send it. I've just read Sonika's patch again to see if I could convince myself to stop being stubborn... I realized that our patches are independent. I still believe we need this one here... We just need a reviewer. But I'm really a stubborn and I'm not convinced we need the other patch. I still can't see how we would end up enabling DDI-A without setting the lanes. For me if we don't call intel_ddi_init(port_A) we don't need to set lanes or there is something else really wrong, and if we call it this bit will be *always* set already. > > BR, > Jani. > > > > > > > > > > > > > > > > > thanks > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
> On Wed, 2015-08-26 at 11:15 +0300, Jani Nikula wrote: > > On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com> > > wrote: > > > On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: > > > > > On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: > > > > > > > On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: > > > > > > > > > -----Original Message----- > > > > > > > > > From: Vivi, Rodrigo > > > > > > > > > Sent: Saturday, August 8, 2015 8:34 AM > > > > > > > > > To: intel-gfx@lists.freedesktop.org > > > > > > > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > > > > > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A > > > > > > > > > shares 4 > > > > > > > > > lanes. > > > > > > > > > > > > > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E is > > > > > > > > > present we > > > > > > > > > need to configure lane count propperly for both. > > > > > > > > > > > > > > > > > > This was based on Sonika's > > > > > > > > > [PATCH] drm/i915/skl: Select DDIA lane capability based > > > > > > > > > upon vbt > > > > > > > > > > > > > > > > > > Credits-to: Sonika Jindal <sonika.jindal@intel.com> > > > > > > > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- > > > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > > > > > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > index 110d546..557cecf 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct > > > > > > > > > drm_device > > > > > > > > > *dev, enum port port) > > > > > > > > > struct intel_digital_port *intel_dig_port; > > > > > > > > > struct intel_encoder *intel_encoder; > > > > > > > > > struct drm_encoder *encoder; > > > > > > > > > - bool init_hdmi, init_dp; > > > > > > > > > + bool init_hdmi, init_dp, ddi_e_present; > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * On SKL we don't have a way to detect DDI-E > > > > > > > > > so we > > > > > > > > > rely > > > > > > > > > on VBT. > > > > > > > > > + */ > > > > > > > > > + ddie_present = IS_SKYLAKE(dev) && > > > > > > > > > + (dev_priv > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dp > > > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dvi > > > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > > > > > > > [Zhang, Xiong Y] ddie_present should be ddi_e_present > > > > > > > > > > > > > > > > > > init_hdmi = (dev_priv > > > > > > > > > ->vbt.ddi_port_info[port].supports_dvi || > > > > > > > > > dev_priv > > > > > > > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > > > > > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct > > > > > > > > > drm_device > > > > > > > > > *dev, enum port port) > > > > > > > > > intel_dig_port->port = port; > > > > > > > > > intel_dig_port->saved_port_bits = > > > > > > > > > I915_READ(DDI_BUF_CTL(port)) & > > > > > > > > > > > > > > > > > > (DDI_BUF_PORT_REVERSAL | > > > > > > > > > - > > > > > > > > > DDI_A_4_LANES); > > > > > > > > > + > > > > > > > > > ddi_e_present ? 0 : > > > > > > > > > DDI_A_4_LANES); > > > > > > > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes > > > > > > > > when > > > > > > > > DDI-E doesn't exist, I think your patch will do nothing. > > > > > > > > > > > > > > Actually DDI_A_4_LANES is being already set > > > > > > > unconditionally, so > > > > > > > Sonika's patch has no effect. > > > > > > [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under > > > > > > many > > > > > > conditions. > > > > > > + if (IS_SKYLAKE(dev) && port == PORT_A > > > > > > + && !(val & DDI_BUF_CTL_ENABLE) > > > > > > + && !dev_priv->vbt.ddi_e_used) > > > > > > + I915_WRITE(DDI_BUF_CTL(port), val | > > > > > > DDI_A_4_LANES) > > > > > > > > > > > > > > saved_port_bits goes to intel_dp->DP that goes to > > > > > > > DDI_BUF_CTL and > > > > > > > also it is used to calculate the number of lanes. > > > > > > > > > > > > > > With this patch we stop setting DDI_A_4_LANES when ddi_e is > > > > > > > present > > > > > > > so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E > > > > > > [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES > > > > > > when ddi_e > > > > > > is present. > > > > > > But this patch won't set DDI_A_4_LANES under following > > > > > > conditions > > > > > > which is purpose for Sonika patch 1. Bios fail to driver eDP > > > > > > and > > > > > > doesn't enable DDI_A buffer > > > > > > > > > > If DDI_A isn't enabled we don't need to set DDI_A_4_LANES > > > > [Zhang, Xiong Y] From commit message on Sonika patch, she want to > > > > set DDI_A_4_LANES on such case. Maybe she met such fail case on > > > > one high > > > > resolution eDP screen. Let's Sonikia explain it. > > > > > > > > > > > 2. Bios clear DDI_A_4_LANES > > > > > > 3. DDI_E isn't present > > > > > > > > > > I don't agree... This is already covered on current code. > > > > > DDI_A_4_LANES is > > > > > already being set when enabling DDI_A. > > > > > > > > As Zhang mentioned and as my commit message explains, my patch is > > > needed > > > when bios failed to drive edp (In my case it was an intermediate > > > frequency supported panel which was set to 3.24 GHz and bios didn't > > > have > > > support for intermediate frequencies), it will not enable DDIA in > > > which > > > case, it will not set DDI_BUF_CTL and DDI Lane capability will > > > remain 0 > > > (which is DDIA with 2 lanes and DDIE with 2 lanes). > > > So, since the native resolution of that panel was high and couldn't > > > work > > > with 2 lanes. > > > So, ideally we should not rely on bios to set the initial value and > > > set > > > it based upon whether DDI_E is used or not. > > > So, my patch has some effect :) > > > > Rodrigo? Please figure out what the needed patch is, and send it. > > I've just read Sonika's patch again to see if I could convince myself > to stop being stubborn... > > I realized that our patches are independent. I still believe we need > this one here... We just need a reviewer. > > But I'm really a stubborn and I'm not convinced we need the other > patch. I still can't see how we would end up enabling DDI-A without > setting the lanes. For me if we don't call intel_ddi_init(port_A) we > don't need to set lanes or there is something else really wrong, and if > we call it this bit will be *always* set already. [Zhang, Xiong Y] From Sonika experience, "always" isn't true because bios fail in initialize eDP. In a short, if DDI-E is present, we should clear DDI_A_4_LANES, your patch will do this. If DDI-E isn't present, we should set DDI_A_4_LANES, Sonika's patch will do it, but your patch miss it. If you think your and Sonika's patch are independent, you could resend your patch with modified commit message. thanks > > > > > BR, > > Jani. > > > > > > > > > > > > > > > > > > > > > > > thanks > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On 27.08.2015 05:52, Zhang, Xiong Y wrote: >> On Wed, 2015-08-26 at 11:15 +0300, Jani Nikula wrote: >>> On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com> >>> wrote: >>>> On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: >>>>>> On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: >>>>>>>> On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y wrote: >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Vivi, Rodrigo >>>>>>>>>> Sent: Saturday, August 8, 2015 8:34 AM >>>>>>>>>> To: intel-gfx@lists.freedesktop.org >>>>>>>>>> Cc: Vivi, Rodrigo; Zhang, Xiong Y >>>>>>>>>> Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A >>>>>>>>>> shares 4 >>>>>>>>>> lanes. >>>>>>>>>> >>>>>>>>>> DDI-A and DDI-E shares the 4 lanes. So when DDI-E is >>>>>>>>>> present we >>>>>>>>>> need to configure lane count propperly for both. >>>>>>>>>> >>>>>>>>>> This was based on Sonika's >>>>>>>>>> [PATCH] drm/i915/skl: Select DDIA lane capability based >>>>>>>>>> upon vbt >>>>>>>>>> >>>>>>>>>> Credits-to: Sonika Jindal <sonika.jindal@intel.com> >>>>>>>>>> Cc: Xiong Zhang <xiong.y.zhang@intel.com> >>>>>>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- >>>>>>>>>> drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- >>>>>>>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>>>>>>>>> b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>>>>> index 110d546..557cecf 100644 >>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>>>>> @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct >>>>>>>>>> drm_device >>>>>>>>>> *dev, enum port port) >>>>>>>>>> struct intel_digital_port *intel_dig_port; >>>>>>>>>> struct intel_encoder *intel_encoder; >>>>>>>>>> struct drm_encoder *encoder; >>>>>>>>>> - bool init_hdmi, init_dp; >>>>>>>>>> + bool init_hdmi, init_dp, ddi_e_present; >>>>>>>>>> + >>>>>>>>>> + /* >>>>>>>>>> + * On SKL we don't have a way to detect DDI-E >>>>>>>>>> so we >>>>>>>>>> rely >>>>>>>>>> on VBT. >>>>>>>>>> + */ >>>>>>>>>> + ddie_present = IS_SKYLAKE(dev) && >>>>>>>>>> + (dev_priv >>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dp >>>>>>>>>>>> >>>>>>>>>> + dev_priv >>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_dvi >>>>>>>>>>>> >>>>>>>>>> + dev_priv >>>>>>>>>> ->vbt.ddi_port_info[PORT_E].supports_hdmi); >>>>>>>>> [Zhang, Xiong Y] ddie_present should be ddi_e_present >>>>>>>>>> >>>>>>>>>> init_hdmi = (dev_priv >>>>>>>>>> ->vbt.ddi_port_info[port].supports_dvi || >>>>>>>>>> dev_priv >>>>>>>>>> ->vbt.ddi_port_info[port].supports_hdmi); >>>>>>>>>> @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct >>>>>>>>>> drm_device >>>>>>>>>> *dev, enum port port) >>>>>>>>>> intel_dig_port->port = port; >>>>>>>>>> intel_dig_port->saved_port_bits = >>>>>>>>>> I915_READ(DDI_BUF_CTL(port)) & >>>>>>>>>> >>>>>>>>>> (DDI_BUF_PORT_REVERSAL | >>>>>>>>>> - >>>>>>>>>> DDI_A_4_LANES); >>>>>>>>>> + >>>>>>>>>> ddi_e_present ? 0 : >>>>>>>>>> DDI_A_4_LANES); >>>>>>>>> [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 lanes >>>>>>>>> when >>>>>>>>> DDI-E doesn't exist, I think your patch will do nothing. >>>>>>>> >>>>>>>> Actually DDI_A_4_LANES is being already set >>>>>>>> unconditionally, so >>>>>>>> Sonika's patch has no effect. >>>>>>> [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES under >>>>>>> many >>>>>>> conditions. >>>>>>> + if (IS_SKYLAKE(dev) && port == PORT_A >>>>>>> + && !(val & DDI_BUF_CTL_ENABLE) >>>>>>> + && !dev_priv->vbt.ddi_e_used) >>>>>>> + I915_WRITE(DDI_BUF_CTL(port), val | >>>>>>> DDI_A_4_LANES) >>>>>>>> >>>>>>>> saved_port_bits goes to intel_dp->DP that goes to >>>>>>>> DDI_BUF_CTL and >>>>>>>> also it is used to calculate the number of lanes. >>>>>>>> >>>>>>>> With this patch we stop setting DDI_A_4_LANES when ddi_e is >>>>>>>> present >>>>>>>> so DDI-A keeps with 2 lanes and let other 2 lanes for DDI-E >>>>>>> [Zhang, Xiong Y] Yes, this patch will clear DDI_A_4_LANES >>>>>>> when ddi_e >>>>>>> is present. >>>>>>> But this patch won't set DDI_A_4_LANES under following >>>>>>> conditions >>>>>>> which is purpose for Sonika patch 1. Bios fail to driver eDP >>>>>>> and >>>>>>> doesn't enable DDI_A buffer >>>>>> >>>>>> If DDI_A isn't enabled we don't need to set DDI_A_4_LANES >>>>> [Zhang, Xiong Y] From commit message on Sonika patch, she want to >>>>> set DDI_A_4_LANES on such case. Maybe she met such fail case on >>>>> one high >>>>> resolution eDP screen. Let's Sonikia explain it. >>>>>> >>>>>>> 2. Bios clear DDI_A_4_LANES >>>>>>> 3. DDI_E isn't present >>>>>> >>>>>> I don't agree... This is already covered on current code. >>>>>> DDI_A_4_LANES is >>>>>> already being set when enabling DDI_A. >>>>>> >>>> As Zhang mentioned and as my commit message explains, my patch is >>>> needed >>>> when bios failed to drive edp (In my case it was an intermediate >>>> frequency supported panel which was set to 3.24 GHz and bios didn't >>>> have >>>> support for intermediate frequencies), it will not enable DDIA in >>>> which >>>> case, it will not set DDI_BUF_CTL and DDI Lane capability will >>>> remain 0 >>>> (which is DDIA with 2 lanes and DDIE with 2 lanes). >>>> So, since the native resolution of that panel was high and couldn't >>>> work >>>> with 2 lanes. >>>> So, ideally we should not rely on bios to set the initial value and >>>> set >>>> it based upon whether DDI_E is used or not. >>>> So, my patch has some effect :) >>> >>> Rodrigo? Please figure out what the needed patch is, and send it. >> >> I've just read Sonika's patch again to see if I could convince myself >> to stop being stubborn... >> >> I realized that our patches are independent. I still believe we need >> this one here... We just need a reviewer. >> >> But I'm really a stubborn and I'm not convinced we need the other >> patch. I still can't see how we would end up enabling DDI-A without >> setting the lanes. For me if we don't call intel_ddi_init(port_A) we >> don't need to set lanes or there is something else really wrong, and if >> we call it this bit will be *always* set already. > [Zhang, Xiong Y] From Sonika experience, "always" isn't true because bios fail in initialize eDP. > In a short, if DDI-E is present, we should clear DDI_A_4_LANES, your patch will do this. > If DDI-E isn't present, we should set DDI_A_4_LANES, Sonika's patch will do it, but your patch miss it. > If you think your and Sonika's patch are independent, you could resend your patch with modified commit message. If it helps with the debate, this patch caused a regression on an I+N hybrid machine where the display remained black after booting up..
On Thu, 2015-08-27 at 17:31 +0300, Timo Aaltonen wrote: > On 27.08.2015 05:52, Zhang, Xiong Y wrote: > > > On Wed, 2015-08-26 at 11:15 +0300, Jani Nikula wrote: > > > > On Thu, 13 Aug 2015, "Jindal, Sonika" <sonika.jindal@intel.com> > > > > wrote: > > > > > On 8/13/2015 8:57 AM, Zhang, Xiong Y wrote: > > > > > > > On Wed, 2015-08-12 at 02:20 +0000, Zhang, Xiong Y wrote: > > > > > > > > > On Tue, 2015-08-11 at 07:05 +0000, Zhang, Xiong Y > > > > > > > > > wrote: > > > > > > > > > > > -----Original Message----- > > > > > > > > > > > From: Vivi, Rodrigo > > > > > > > > > > > Sent: Saturday, August 8, 2015 8:34 AM > > > > > > > > > > > To: intel-gfx@lists.freedesktop.org > > > > > > > > > > > Cc: Vivi, Rodrigo; Zhang, Xiong Y > > > > > > > > > > > Subject: [PATCH 7/6] drm/i915/skl: DDI-E and DDI > > > > > > > > > > > -A > > > > > > > > > > > shares 4 > > > > > > > > > > > lanes. > > > > > > > > > > > > > > > > > > > > > > DDI-A and DDI-E shares the 4 lanes. So when DDI-E > > > > > > > > > > > is > > > > > > > > > > > present we > > > > > > > > > > > need to configure lane count propperly for both. > > > > > > > > > > > > > > > > > > > > > > This was based on Sonika's > > > > > > > > > > > [PATCH] drm/i915/skl: Select DDIA lane capability > > > > > > > > > > > based > > > > > > > > > > > upon vbt > > > > > > > > > > > > > > > > > > > > > > Credits-to: Sonika Jindal < > > > > > > > > > > > sonika.jindal@intel.com> > > > > > > > > > > > Cc: Xiong Zhang <xiong.y.zhang@intel.com> > > > > > > > > > > > Signed-off-by: Rodrigo Vivi < > > > > > > > > > > > rodrigo.vivi@intel.com> > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 12 > > > > > > > > > > > ++++++++++-- > > > > > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- > > > > > > > > > > > 2 files changed, 15 insertions(+), 5 deletions( > > > > > > > > > > > -) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > > > index 110d546..557cecf 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > > > > > > > @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct > > > > > > > > > > > drm_device > > > > > > > > > > > *dev, enum port port) > > > > > > > > > > > struct intel_digital_port > > > > > > > > > > > *intel_dig_port; > > > > > > > > > > > struct intel_encoder *intel_encoder; > > > > > > > > > > > struct drm_encoder *encoder; > > > > > > > > > > > - bool init_hdmi, init_dp; > > > > > > > > > > > + bool init_hdmi, init_dp, ddi_e_present; > > > > > > > > > > > + > > > > > > > > > > > + /* > > > > > > > > > > > + * On SKL we don't have a way to detect > > > > > > > > > > > DDI-E > > > > > > > > > > > so we > > > > > > > > > > > rely > > > > > > > > > > > on VBT. > > > > > > > > > > > + */ > > > > > > > > > > > + ddie_present = IS_SKYLAKE(dev) && > > > > > > > > > > > + (dev_priv > > > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dp > > > > > > > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_dvi > > > > > > > > > > > > > > > > > > > > > > > > + dev_priv > > > > > > > > > > > ->vbt.ddi_port_info[PORT_E].supports_hdmi); > > > > > > > > > > [Zhang, Xiong Y] ddie_present should be > > > > > > > > > > ddi_e_present > > > > > > > > > > > > > > > > > > > > > > init_hdmi = (dev_priv > > > > > > > > > > > ->vbt.ddi_port_info[port].supports_dvi || > > > > > > > > > > > dev_priv > > > > > > > > > > > ->vbt.ddi_port_info[port].supports_hdmi); > > > > > > > > > > > @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct > > > > > > > > > > > drm_device > > > > > > > > > > > *dev, enum port port) > > > > > > > > > > > intel_dig_port->port = port; > > > > > > > > > > > intel_dig_port->saved_port_bits = > > > > > > > > > > > I915_READ(DDI_BUF_CTL(port)) & > > > > > > > > > > > > > > > > > > > > > > (DDI_BUF_PORT_REVERSAL | > > > > > > > > > > > - > > > > > > > > > > > DDI_A_4_LANES); > > > > > > > > > > > + > > > > > > > > > > > ddi_e_present ? 0 : > > > > > > > > > > > DDI_A_4_LANES); > > > > > > > > > > [Zhang, Xiong Y] Sonika's patch will set DDI-A to 4 > > > > > > > > > > lanes > > > > > > > > > > when > > > > > > > > > > DDI-E doesn't exist, I think your patch will do > > > > > > > > > > nothing. > > > > > > > > > > > > > > > > > > Actually DDI_A_4_LANES is being already set > > > > > > > > > unconditionally, so > > > > > > > > > Sonika's patch has no effect. > > > > > > > > [Zhang, Xiong Y] No. Sonika's patch set DDI_A_4_LANES > > > > > > > > under > > > > > > > > many > > > > > > > > conditions. > > > > > > > > + if (IS_SKYLAKE(dev) && port == PORT_A > > > > > > > > + && !(val & DDI_BUF_CTL_ENABLE) > > > > > > > > + && !dev_priv->vbt.ddi_e_used) > > > > > > > > + I915_WRITE(DDI_BUF_CTL(port), val | > > > > > > > > DDI_A_4_LANES) > > > > > > > > > > > > > > > > > > saved_port_bits goes to intel_dp->DP that goes to > > > > > > > > > DDI_BUF_CTL and > > > > > > > > > also it is used to calculate the number of lanes. > > > > > > > > > > > > > > > > > > With this patch we stop setting DDI_A_4_LANES when > > > > > > > > > ddi_e is > > > > > > > > > present > > > > > > > > > so DDI-A keeps with 2 lanes and let other 2 lanes for > > > > > > > > > DDI-E > > > > > > > > [Zhang, Xiong Y] Yes, this patch will clear > > > > > > > > DDI_A_4_LANES > > > > > > > > when ddi_e > > > > > > > > is present. > > > > > > > > But this patch won't set DDI_A_4_LANES under following > > > > > > > > conditions > > > > > > > > which is purpose for Sonika patch 1. Bios fail to > > > > > > > > driver eDP > > > > > > > > and > > > > > > > > doesn't enable DDI_A buffer > > > > > > > > > > > > > > If DDI_A isn't enabled we don't need to set DDI_A_4_LANES > > > > > > [Zhang, Xiong Y] From commit message on Sonika patch, she > > > > > > want to > > > > > > set DDI_A_4_LANES on such case. Maybe she met such fail > > > > > > case on > > > > > > one high > > > > > > resolution eDP screen. Let's Sonikia explain it. > > > > > > > > > > > > > > > 2. Bios clear DDI_A_4_LANES > > > > > > > > 3. DDI_E isn't present > > > > > > > > > > > > > > I don't agree... This is already covered on current code. > > > > > > > DDI_A_4_LANES is > > > > > > > already being set when enabling DDI_A. > > > > > > > > > > > > As Zhang mentioned and as my commit message explains, my > > > > > patch is > > > > > needed > > > > > when bios failed to drive edp (In my case it was an > > > > > intermediate > > > > > frequency supported panel which was set to 3.24 GHz and bios > > > > > didn't > > > > > have > > > > > support for intermediate frequencies), it will not enable > > > > > DDIA in > > > > > which > > > > > case, it will not set DDI_BUF_CTL and DDI Lane capability > > > > > will > > > > > remain 0 > > > > > (which is DDIA with 2 lanes and DDIE with 2 lanes). > > > > > So, since the native resolution of that panel was high and > > > > > couldn't > > > > > work > > > > > with 2 lanes. > > > > > So, ideally we should not rely on bios to set the initial > > > > > value and > > > > > set > > > > > it based upon whether DDI_E is used or not. > > > > > So, my patch has some effect :) > > > > > > > > Rodrigo? Please figure out what the needed patch is, and send > > > > it. > > > > > > I've just read Sonika's patch again to see if I could convince > > > myself > > > to stop being stubborn... > > > > > > I realized that our patches are independent. I still believe we > > > need > > > this one here... We just need a reviewer. > > > > > > But I'm really a stubborn and I'm not convinced we need the other > > > patch. I still can't see how we would end up enabling DDI-A > > > without > > > setting the lanes. For me if we don't call intel_ddi_init(port_A) > > > we > > > don't need to set lanes or there is something else really wrong, > > > and if > > > we call it this bit will be *always* set already. > > [Zhang, Xiong Y] From Sonika experience, "always" isn't true > > because bios fail in initialize eDP. > > In a short, if DDI-E is present, we should clear DDI_A_4_LANES, > > your patch will do this. > > If DDI-E isn't present, we should set DDI_A_4_LANES, Sonika's patch > > will do it, but your patch miss it. so I believe what I still can't understand is why we need to set DDI_A_4_LANES when not using DDI_A... > > If you think your and Sonika's patch are independent, you could > > resend your patch with modified commit message. > > If it helps with the debate, this patch caused a regression on an I+N > hybrid machine where the display remained black after booting up.. ... but nevermind... Apparently my patch is wrong and causing trouble more than helping... Thanks Timo. >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 110d546..557cecf 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3178,7 +3178,15 @@ void intel_ddi_init(struct drm_device *dev, enum port port) struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct drm_encoder *encoder; - bool init_hdmi, init_dp; + bool init_hdmi, init_dp, ddi_e_present; + + /* + * On SKL we don't have a way to detect DDI-E so we rely on VBT. + */ + ddie_present = IS_SKYLAKE(dev) && + (dev_priv->vbt.ddi_port_info[PORT_E].supports_dp || + dev_priv->vbt.ddi_port_info[PORT_E].supports_dvi || + dev_priv->vbt.ddi_port_info[PORT_E].supports_hdmi); init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || dev_priv->vbt.ddi_port_info[port].supports_hdmi); @@ -3210,7 +3218,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port) intel_dig_port->port = port; intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & (DDI_BUF_PORT_REVERSAL | - DDI_A_4_LANES); + ddi_e_present ? 0 : DDI_A_4_LANES); intel_encoder->type = INTEL_OUTPUT_UNKNOWN; intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3ff2080..7ada79e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -159,9 +159,11 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) u8 source_max, sink_max; source_max = 4; - if (HAS_DDI(dev) && intel_dig_port->port == PORT_A && - (intel_dig_port->saved_port_bits & DDI_A_4_LANES) == 0) - source_max = 2; + if (HAS_DDI(dev) && (intel_dig_port->port == PORT_E || + (intel_dig_port->port == PORT_A && + (intel_dig_port->saved_port_bits & + DDI_A_4_LANES) == 0)) + source_max = 2; sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
DDI-A and DDI-E shares the 4 lanes. So when DDI-E is present we need to configure lane count propperly for both. This was based on Sonika's [PATCH] drm/i915/skl: Select DDIA lane capability based upon vbt Credits-to: Sonika Jindal <sonika.jindal@intel.com> Cc: Xiong Zhang <xiong.y.zhang@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++-- drivers/gpu/drm/i915/intel_dp.c | 8 +++++--- 2 files changed, 15 insertions(+), 5 deletions(-)