Re: [PATCH] drm/i915/icl: implement icl_digital_port_connected()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Em Qua, 2018-07-25 às 10:27 -0700, Paulo Zanoni escreveu:
> Em Ter, 2018-07-24 às 18:19 -0700, Lucas De Marchi escreveu:
> > On Tue, Jul 24, 2018 at 05:28:09PM -0700, Paulo Zanoni wrote:
> > > Do like the other functions and check for the status bits. The
> > > "Hot
> > > Plug Detection" page from our documentation says we can't just
> > > use
> > > the
> > > ISR bits on the CPU side, so use the correct registers.
> > 
> > nit: here you are saying it's for all of them rather than just a
> > subset.
> > My suggestion is:
> > 
> > Do like the other functions and check for the status bits. For
> > TypeC/TBT
> > on North Display Engine the "Hot Plug Detection" page from our
> > documentation says we can't just use the ISR bits to find the live
> > state, so use the correct registers: DFLEXDPSP TC Live State field.
> > 
> > 
> > > 
> > > v2: Rebase.
> > > v3:
> > >   - Simplify true/false assignment (Rodrigo).
> > >   - Reorganize is_gen if ladder (Rodrigo).
> > >   - Don't use the ISR for TC/TBT CPU bits.
> > > 
> > > Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h |  8 +++++
> > >  drivers/gpu/drm/i915/intel_dp.c | 55
> > > ++++++++++++++++++++++++++++++++-
> > >  2 files changed, 62 insertions(+), 1 deletion(-)
> > > 
> > > My understanding is that there's agreement on this patch since it
> > > just
> > > mimicks what the other gens do. Let's have this one agreed on
> > > (merged)
> > > first before we continue the discussions, especially since this
> > > one
> > > significantly affects the CI system.
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 73946055aa15..3eaff32dd74e 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7210,6 +7210,7 @@ enum {
> > >  #define  GEN11_TC3_HOTPLUG			(1 << 18)
> > >  #define  GEN11_TC2_HOTPLUG			(1 << 17)
> > >  #define  GEN11_TC1_HOTPLUG			(1 << 16)
> > > +#define  GEN11_TC_HOTPLUG(tc_port)		(1 <<
> > > ((tc_port)
> > > + 16))
> > >  #define  GEN11_DE_TC_HOTPLUG_MASK		(GEN11_TC4_HOTP
> > > LU
> > > G | \
> > >  						 GEN11_TC3_HOTPL
> > > UG
> > > > \
> > > 
> > >  						 GEN11_TC2_HOTPL
> > > UG
> > > > \
> > > 
> > > @@ -7218,6 +7219,7 @@ enum {
> > >  #define  GEN11_TBT3_HOTPLUG			(1 << 2)
> > >  #define  GEN11_TBT2_HOTPLUG			(1 << 1)
> > >  #define  GEN11_TBT1_HOTPLUG			(1 << 0)
> > > +#define  GEN11_TBT_HOTPLUG(tc_port)		(1 <<
> > > (tc_port))
> > >  #define  GEN11_DE_TBT_HOTPLUG_MASK		(GEN11_TBT4_HO
> > > TP
> > > LUG | \
> > >  						 GEN11_TBT3_HOTP
> > > LU
> > > G | \
> > >  						 GEN11_TBT2_HOTP
> > > LU
> > > G | \
> > > @@ -7590,6 +7592,8 @@ enum {
> > >  #define SDE_GMBUS_ICP			(1 << 23)
> > >  #define SDE_DDIB_HOTPLUG_ICP		(1 << 17)
> > >  #define SDE_DDIA_HOTPLUG_ICP		(1 << 16)
> > > +#define SDE_TC_HOTPLUG_ICP(tc_port)	(1 << ((tc_port) +
> > > 24))
> > > +#define SDE_DDI_HOTPLUG_ICP(port)	(1 << ((port) + 16))
> > >  #define SDE_DDI_MASK_ICP		(SDE_DDIB_HOTPLUG_ICP |	
> > > \
> > >  					 SDE_DDIA_HOTPLUG_ICP)
> > >  #define SDE_TC_MASK_ICP			(SDE_TC4_HOTPLUG_
> > > IC
> > > P |	\
> > > @@ -10654,4 +10658,8 @@ enum skl_power_gate {
> > >  						_ICL_DSC1_RC_BUF
> > > _T
> > > HRESH_1_UDW_PB, \
> > >  						_ICL_DSC1_RC_BUF
> > > _T
> > > HRESH_1_UDW_PC)
> > >  
> > > +#define PORT_TX_DFLEXDPSP			_MMIO(0x1638A0)
> > > +#define   TC_LIVE_STATE_TBT(tc_port)		(1 <<
> > > ((tc_port) * 8 + 6))
> > > +#define   TC_LIVE_STATE_TC(tc_port)		(1 <<
> > > ((tc_port) * 8 + 5))
> > > +
> > >  #endif /* _I915_REG_H_ */
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index cd0f649b57a5..998d698788f9 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4586,6 +4586,57 @@ static bool
> > > bxt_digital_port_connected(struct intel_encoder *encoder)
> > >  	return I915_READ(GEN8_DE_PORT_ISR) & bit;
> > >  }
> > >  
> > > +static bool icl_combo_port_connected(struct drm_i915_private
> > > *dev_priv,
> > > +				     struct intel_digital_port
> > > *intel_dig_port)
> > > +{
> > > +	enum port port = intel_dig_port->base.port;
> > > +
> > > +	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> > > +}
> > > +
> > > +static bool icl_tc_port_connected(struct drm_i915_private
> > > *dev_priv,
> > > +				  struct intel_digital_port
> > > *intel_dig_port)
> > > +{
> > > +	enum port port = intel_dig_port->base.port;
> > > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > > +	bool is_legacy, is_typec, is_tbt;
> > > +	u32 dpsp;
> > > +
> > > +	is_legacy = I915_READ(SDEISR) &
> > > SDE_TC_HOTPLUG_ICP(tc_port);
> > > +
> > > +	/*
> > > +	 * The spec says we shouldn't be using the ISR bits for
> > > detecting
> > > +	 * between TC and TBT. We should use DFLEXDPSP.
> > > +	 */
> > > +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
> > 
> > I only wonder if we should be reading the North DE even if
> > is_legacy
> > is
> > true above rather than using an early return. Anyway:
> 
> (missed this part in the other email)
> 
> That makes sense in the current state of the code. It would remove
> our
> ability to hit the WARN_ON below, but that doesn't seem to be a major
> issue.
> 
> Now when we go back to discussing our implementation for the connect
> flows we may end up having to revert to the current form, but we can
> always do that if we need it.

Forget what I just said: patch 2 will already have to revert the early
return.

> 
> > 
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> 
> Thanks! I'll submit v4.
> 
> > 
> > Lucas De Marchi
> > 
> > > +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> > > +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> > > +
> > > +	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> > > +
> > > +	return is_legacy || is_typec || is_tbt;
> > > +}
> > > +
> > > +static bool icl_digital_port_connected(struct intel_encoder
> > > *encoder)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > base.dev);
> > > 
> > > +	struct intel_digital_port *dig_port =
> > > enc_to_dig_port(&encoder->base);
> > > +
> > > +	switch (encoder->hpd_pin) {
> > > +	case HPD_PORT_A:
> > > +	case HPD_PORT_B:
> > > +		return icl_combo_port_connected(dev_priv,
> > > dig_port);
> > > +	case HPD_PORT_C:
> > > +	case HPD_PORT_D:
> > > +	case HPD_PORT_E:
> > > +	case HPD_PORT_F:
> > > +		return icl_tc_port_connected(dev_priv,
> > > dig_port);
> > > +	default:
> > > +		MISSING_CASE(encoder->hpd_pin);
> > > +		return false;
> > > +	}
> > > +}
> > > +
> > >  /*
> > >   * intel_digital_port_connected - is the specified port
> > > connected?
> > >   * @encoder: intel_encoder
> > > @@ -4613,8 +4664,10 @@ bool intel_digital_port_connected(struct
> > > intel_encoder *encoder)
> > >  		return bdw_digital_port_connected(encoder);
> > >  	else if (IS_GEN9_LP(dev_priv))
> > >  		return bxt_digital_port_connected(encoder);
> > > -	else
> > > +	else if (IS_GEN9_BC(dev_priv) || IS_GEN10(dev_priv))
> > >  		return spt_digital_port_connected(encoder);
> > > +	else
> > > +		return icl_digital_port_connected(encoder);
> > >  }
> > >  
> > >  static struct edid *
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux