Re: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4 lanes.

From: "Zhang, Xiong Y" <xiong.y.zhang@intel.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 7/6] drm/i915/skl: DDI-E and DDI-A shares 4	lanes.
Date: Thu, 27 Aug 2015 02:52:03 +0000	[thread overview]
Message-ID: <8082FF9BCB2B054996454E47167FF4EC029CC11B@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1440607188.3879.268.camel@intel.com>

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