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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 11, 2018 at 02:59:03PM -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.
> 
> v2: Rebase.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> (v1)
> 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 |  4 +++
>  drivers/gpu/drm/i915/intel_dp.c | 57 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b95bab7a3d24..c1f350469ff6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7198,6 +7198,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 | \
> @@ -7206,6 +7207,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 | \
> @@ -7578,6 +7580,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 |	\
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5be07e1d816d..f2197dea02d0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4744,6 +4744,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(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);
> +	u32 legacy_bit = SDE_TC_HOTPLUG_ICP(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(SDEISR) & 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;

what about something like below?

+{
+	enum port port = intel_dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	bool is_legacy = false, is_typec = false, is_tbt = false;
+	u32 cpu_isr;
+
+	is_legacy = I915_READ(SDEISR) & legacy_bit;
+
+	cpu_isr = I915_READ(GEN11_DE_HPD_ISR);
+	is_typec = cpu_isr & typec_bit;
+	is_tbt = cpu_isr & tbt_bit;
+
+	WARN_ON(is_legacy && (is_typec || is_tbt));
+	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
> @@ -4771,6 +4826,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);

this is messing the order and getting hard to read.
probably better to invert the order in a separated patch and then
put ICL on top.

>  	else
>  		return spt_digital_port_connected(encoder);
>  }
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx