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. > 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.. -- t _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
