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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > > > > > > > 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@xxxxxxxxx> > > > > > > > > > > > Cc: Xiong Zhang <xiong.y.zhang@xxxxxxxxx> > > > > > > > > > > > Signed-off-by: Rodrigo Vivi < > > > > > > > > > > > rodrigo.vivi@xxxxxxxxx> > > > > > > > > > > > --- > > > > > > > > > > > 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. > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
