drm/i915/icl: implement icl_digital_port_connected()

Commit Message

Zanoni, Paulo R July 25, 2018, 12:28 a.m. UTC

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.

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 <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Signed-off-by: Rodrigo Vivi <[e-mail]>
Signed-off-by: Paulo Zanoni <[e-mail]>
---
 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.

Comments

Lucas De Marchi July 25, 2018, 1:19 a.m. UTC | #1

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 <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Signed-off-by: Rodrigo Vivi <[e-mail]>
> Signed-off-by: Paulo Zanoni <[e-mail]>
> ---
>  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_HOTPLUG | \
>  						 GEN11_TC3_HOTPLUG | \
>  						 GEN11_TC2_HOTPLUG | \
> @@ -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_HOTPLUG | \
>  						 GEN11_TBT3_HOTPLUG | \
>  						 GEN11_TBT2_HOTPLUG | \
> @@ -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_ICP |	\
> @@ -10654,4 +10658,8 @@ enum skl_power_gate {
>  						_ICL_DSC1_RC_BUF_THRESH_1_UDW_PB, \
>  						_ICL_DSC1_RC_BUF_THRESH_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:

Reviewed-by: Lucas De Marchi <[e-mail]>

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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R July 25, 2018, 4:59 p.m. UTC | #2

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.

No, I'm saying it's for the CPU side, the CPU side is a subset which
excludes the PCH. There are 3 types: the legacy ones (on the PCH, not
CPU), the TC ones (on the CPU) and the TBT ones (on the CPU too). On
our driver, "CPU" in these contexts has been used as a synonym for
North Display since the PCH was introduced.

But I can be more explicit if you want, no problem.

> 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 <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Paulo Zanoni <[e-mail]>
> > ---
> >  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_HOTPLU
> > G | \
> >  						 GEN11_TC3_HOTPLUG
> > | \
> >  						 GEN11_TC2_HOTPLUG
> > | \
> > @@ -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_HOTP
> > LUG | \
> >  						 GEN11_TBT3_HOTPLU
> > G | \
> >  						 GEN11_TBT2_HOTPLU
> > 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:
> 
> Reviewed-by: Lucas De Marchi <[e-mail]>
> 
> 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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R July 25, 2018, 5:27 p.m. UTC | #3

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 <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Paulo Zanoni <[e-mail]>
> > ---
> >  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_HOTPLU
> > G | \
> >  						 GEN11_TC3_HOTPLUG
> > | \
> >  						 GEN11_TC2_HOTPLUG
> > | \
> > @@ -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_HOTP
> > LUG | \
> >  						 GEN11_TBT3_HOTPLU
> > G | \
> >  						 GEN11_TBT2_HOTPLU
> > 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.

> 
> Reviewed-by: Lucas De Marchi <[e-mail]>

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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R July 25, 2018, 5:59 p.m. UTC | #4

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 <[e-mail]>
> > > Cc: Rodrigo Vivi <[e-mail]>
> > > Signed-off-by: Rodrigo Vivi <[e-mail]>
> > > Signed-off-by: Paulo Zanoni <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> 
> 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
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lucas De Marchi July 25, 2018, 8:04 p.m. UTC | #5

On Wed, Jul 25, 2018 at 10:59:32AM -0700, Paulo Zanoni wrote:
> 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 <[e-mail]>
> > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > Signed-off-by: Rodrigo Vivi <[e-mail]>
> > > > Signed-off-by: Paulo Zanoni <[e-mail]>
> > > > ---
> > > >  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.

yeah, I should have read patch 2 before commenting here. I thought we
were moving away from updating the type while we check if it's
connected.

LGTM

Lucas De Marchi

> 
> > 
> > > 
> > > Reviewed-by: Lucas De Marchi <[e-mail]>
> > 
> > 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
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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_HOTPLUG | \
 						 GEN11_TC3_HOTPLUG | \
 						 GEN11_TC2_HOTPLUG | \
@@ -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_HOTPLUG | \
 						 GEN11_TBT3_HOTPLUG | \
 						 GEN11_TBT2_HOTPLUG | \
@@ -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_ICP |	\
@@ -10654,4 +10658,8 @@  enum skl_power_gate {
 						_ICL_DSC1_RC_BUF_THRESH_1_UDW_PB, \
 						_ICL_DSC1_RC_BUF_THRESH_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);
+	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 *