diff mbox

[18/24] drm/i915/icl: implement icl_digital_port_connected()

Message ID 20180522002558.29262-19-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R May 22, 2018, 12:25 a.m. UTC 
Do like the other functions and check for the ISR bits. We have plans
to add a few more checks in this code in the next patches, that's why
it's a little more verbose than it could be.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  5 ++++
 drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

Comments

Lucas De Marchi June 19, 2018, 10:28 p.m. UTC | #1 
On Mon, May 21, 2018 at 05:25:52PM -0700, Paulo Zanoni wrote:
> Do like the other functions and check for the ISR bits. We have plans
> to add a few more checks in this code in the next patches, that's why
> it's a little more verbose than it could be.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  5 ++++
>  drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 49a72320e794..24308d4435f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7055,6 +7055,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 | \
> @@ -7063,6 +7064,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 | \
> @@ -7486,6 +7488,9 @@ enum {
>  #define   ICP_GMBUS			(1 << 23)
>  #define   ICP_DDIB_HOTPLUG		(1 << 17)
>  #define   ICP_DDIA_HOTPLUG		(1 << 16)
> +#define ICP_TC_HOTPLUG(tc_port)		(1 << ((tc_port) + 24))
> +#define ICP_DDI_HOTPLUG(port)		(1 << ((port) + 16))
> +
>  
>  #define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG |	\
>  					 ICP_DDIA_HOTPLUG)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 102070940095..b477124717e7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4722,6 +4722,61 @@ 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(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(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);
> +	u32 legacy_bit = ICP_TC_HOTPLUG(tc_port);
> +	u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
> +	u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
> +	bool is_legacy = false, is_typec = false, is_tbt = false;
> +	u32 cpu_isr;

why *cpu*_isr? hpd_isr, isr or val would be better IMO

> +
> +	if (I915_READ(ICP_SDE_ISR) & legacy_bit)
> +		is_legacy = true;
> +
> +	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
> +	if (cpu_isr & typec_bit)
> +		is_typec = true;
> +	if (cpu_isr & tbt_bit)
> +		is_tbt = true;
> +
> +	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> +	if (!is_legacy && !is_typec && !is_tbt)
> +		return false;
> +
> +	return true;

you know you could "return is_legacy + is_typec + is_tbt;", right? you
are already doing it above, it may make sense to remove the extra
branch. Or not.

Feel free to disagree and push.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

> +}
> +
> +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
> @@ -4749,6 +4804,8 @@ 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 if (IS_ICELAKE(dev_priv))
> +		return icl_digital_port_connected(encoder);
>  	else
>  		return spt_digital_port_connected(encoder);
>  }
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Zanoni, Paulo R June 20, 2018, 9:01 p.m. UTC | #2 
Em Ter, 2018-06-19 às 15:28 -0700, Lucas De Marchi escreveu:
> On Mon, May 21, 2018 at 05:25:52PM -0700, Paulo Zanoni wrote:
> > Do like the other functions and check for the ISR bits. We have
> > plans
> > to add a few more checks in this code in the next patches, that's
> > why
> > it's a little more verbose than it could be.
> > 
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  5 ++++
> >  drivers/gpu/drm/i915/intel_dp.c | 57
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 49a72320e794..24308d4435f5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7055,6 +7055,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
> > | \
> > @@ -7063,6 +7064,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 | \
> > @@ -7486,6 +7488,9 @@ enum {
> >  #define   ICP_GMBUS			(1 << 23)
> >  #define   ICP_DDIB_HOTPLUG		(1 << 17)
> >  #define   ICP_DDIA_HOTPLUG		(1 << 16)
> > +#define ICP_TC_HOTPLUG(tc_port)		(1 << ((tc_port) +
> > 24))
> > +#define ICP_DDI_HOTPLUG(port)		(1 << ((port) + 16))
> > +
> >  
> >  #define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG |	
> > \
> >  					 ICP_DDIA_HOTPLUG)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 102070940095..b477124717e7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4722,6 +4722,61 @@ 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(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(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);
> > +	u32 legacy_bit = ICP_TC_HOTPLUG(tc_port);
> > +	u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
> > +	u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
> > +	bool is_legacy = false, is_typec = false, is_tbt = false;
> > +	u32 cpu_isr;
> 
> why *cpu*_isr? hpd_isr, isr or val would be better IMO

Because we have 2 ISR registers we check, one is on the CPU and the
other is on the PCH. We just don't have a variable for pch_isr because
we don't need. Jusc alling it "isr" or "hpd_isr" would be misleading.

> 
> > +
> > +	if (I915_READ(ICP_SDE_ISR) & legacy_bit)
> > +		is_legacy = true;
> > +
> > +	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
> > +	if (cpu_isr & typec_bit)
> > +		is_typec = true;
> > +	if (cpu_isr & tbt_bit)
> > +		is_tbt = true;
> > +
> > +	WARN_ON(is_legacy + is_typec + is_tbt > 1);
> > +	if (!is_legacy && !is_typec && !is_tbt)
> > +		return false;
> > +
> > +	return true;
> 
> you know you could "return is_legacy + is_typec + is_tbt;", right?
> you
> are already doing it above, it may make sense to remove the extra
> branch. Or not.

Because the next patch adds code between the "return false" and the
last return, so I'd have to change it there.


> 
> Feel free to disagree and push.
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks,
Paulo

> 
> 
> Lucas De Marchi
> 
> > +}
> > +
> > +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
> > @@ -4749,6 +4804,8 @@ 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 if (IS_ICELAKE(dev_priv))
> > +		return icl_digital_port_connected(encoder);
> >  	else
> >  		return spt_digital_port_connected(encoder);
> >  }
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 49a72320e794..24308d4435f5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7055,6 +7055,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 | \
@@ -7063,6 +7064,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 | \
@@ -7486,6 +7488,9 @@  enum {
 #define   ICP_GMBUS			(1 << 23)
 #define   ICP_DDIB_HOTPLUG		(1 << 17)
 #define   ICP_DDIA_HOTPLUG		(1 << 16)
+#define ICP_TC_HOTPLUG(tc_port)		(1 << ((tc_port) + 24))
+#define ICP_DDI_HOTPLUG(port)		(1 << ((port) + 16))
+
 
 #define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG |	\
 					 ICP_DDIA_HOTPLUG)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 102070940095..b477124717e7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4722,6 +4722,61 @@  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(ICP_SDE_ISR) & ICP_DDI_HOTPLUG(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);
+	u32 legacy_bit = ICP_TC_HOTPLUG(tc_port);
+	u32 typec_bit = GEN11_TC_HOTPLUG(tc_port);
+	u32 tbt_bit = GEN11_TBT_HOTPLUG(tc_port);
+	bool is_legacy = false, is_typec = false, is_tbt = false;
+	u32 cpu_isr;
+
+	if (I915_READ(ICP_SDE_ISR) & legacy_bit)
+		is_legacy = true;
+
+	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
+	if (cpu_isr & typec_bit)
+		is_typec = true;
+	if (cpu_isr & tbt_bit)
+		is_tbt = true;
+
+	WARN_ON(is_legacy + is_typec + is_tbt > 1);
+	if (!is_legacy && !is_typec && !is_tbt)
+		return false;
+
+	return true;
+}
+
+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
@@ -4749,6 +4804,8 @@  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 if (IS_ICELAKE(dev_priv))
+		return icl_digital_port_connected(encoder);
 	else
 		return spt_digital_port_connected(encoder);
 }