Re: [Intel-gfx] [10/12] drm/i915: Introduce HPD_PORT_TC<n>

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2020-07-01 at 00:55 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <
> ville.syrjala@linux.intel.com
> >
> 
> Make a clean split between hpd pins for DDI vs. TC. This matches
> how the actual hardware is split.
> 
> And with this we move the DDI/PHY->HPD pin mapping into the encoder
> init instead of having to remap yet again in the interrupt code.
> 
> Signed-off-by: Ville Syrjälä <
> ville.syrjala@linux.intel.com
> >
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c     |  65 +++++++++-
>  drivers/gpu/drm/i915/display/intel_hotplug.c |  25 +---
>  drivers/gpu/drm/i915/i915_drv.h              |  17 +--
>  drivers/gpu/drm/i915/i915_irq.c              | 121 +++++--------------
>  4 files changed, 102 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index d024491738b3..a2c9815c5abc 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4847,6 +4847,57 @@ intel_ddi_max_lanes(struct intel_digital_port *intel_dport)
>  	return max_lanes;
>  }
>  
> +static enum hpd_pin tgl_hpd_pin(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	if (port >= PORT_D)
> +		return HPD_PORT_TC1 + port - PORT_D;
> +	else
> +		return HPD_PORT_A + port - PORT_A;
> +}
> +
> +static enum hpd_pin rkl_hpd_pin(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	if (HAS_PCH_TGP(dev_priv))
> +		return tgl_hpd_pin(dev_priv, port);
> +
> +	if (port >= PORT_D)
> +		return HPD_PORT_C + port - PORT_D;

The above looks wrong, for it would match with only the return bellow.

> +	else
> +		return HPD_PORT_A + port - PORT_A;
> +}
> +
> +static enum hpd_pin icl_hpd_pin(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	if (port >= PORT_C)
> +		return HPD_PORT_TC1 + port - PORT_C;
> +	else
> +		return HPD_PORT_A + port - PORT_A;
> +}
> +
> +static enum hpd_pin ehl_hpd_pin(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	if (port == PORT_D)
> +		return HPD_PORT_A;
> +
> +	if (HAS_PCH_MCC(dev_priv))
> +		return icl_hpd_pin(dev_priv, port);

Maybe call tgl_hpd_pin() for HAS_PCH_MCC()? The code bellow will match but just for consistency.

Other than the RKL comment it looks good to me.

> +
> +	return HPD_PORT_A + port - PORT_A;
> +}
> +
> +static enum hpd_pin cnl_hpd_pin(struct drm_i915_private *dev_priv,
> +				enum port port)
> +{
> +	if (port == PORT_F)
> +		return HPD_PORT_E;
> +
> +	return HPD_PORT_A + port - PORT_A;
> +}
> +
>  void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  {
>  	struct intel_digital_port *intel_dig_port;
> @@ -4907,7 +4958,19 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	encoder->port = port;
>  	encoder->cloneable = 0;
>  	encoder->pipe_mask = ~0;
> -	encoder->hpd_pin = intel_hpd_pin_default(dev_priv, port);
> +
> +	if (IS_ROCKETLAKE(dev_priv))
> +		encoder->hpd_pin = rkl_hpd_pin(dev_priv, port);
> +	else if (INTEL_GEN(dev_priv) >= 12)
> +		encoder->hpd_pin = tgl_hpd_pin(dev_priv, port);
> +	else if (IS_ELKHARTLAKE(dev_priv))
> +		encoder->hpd_pin = ehl_hpd_pin(dev_priv, port);
> +	else if (IS_GEN(dev_priv, 11))
> +		encoder->hpd_pin = icl_hpd_pin(dev_priv, port);
> +	else if (IS_GEN(dev_priv, 10))
> +		encoder->hpd_pin = cnl_hpd_pin(dev_priv, port);
> +	else
> +		encoder->hpd_pin = intel_hpd_pin_default(dev_priv, port);
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		intel_dig_port->saved_port_bits = intel_de_read(dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 80bcfff032e9..8a8e77314a4e 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -81,33 +81,12 @@
>   *
>   * It is only valid and used by digital port encoder.
>   *
> - * Return pin that is associatade with @port and HDP_NONE if no pin is
> - * hard associated with that @port.
> + * Return pin that is associatade with @port.
>   */
>  enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  				   enum port port)
>  {
> -	enum phy phy = intel_port_to_phy(dev_priv, port);
> -
> -	/*
> -	 * RKL + TGP PCH is a special case; we effectively choose the hpd_pin
> -	 * based on the DDI rather than the PHY (i.e., the last two outputs
> -	 * shold be HPD_PORT_{D,E} rather than {C,D}.  Note that this differs
> -	 * from the behavior of both TGL+TGP and RKL+CMP.
> -	 */
> -	if (IS_ROCKETLAKE(dev_priv) && HAS_PCH_TGP(dev_priv))
> -		return HPD_PORT_A + port - PORT_A;
> -
> -	switch (phy) {
> -	case PHY_F:
> -		return IS_CNL_WITH_PORT_F(dev_priv) ? HPD_PORT_E : HPD_PORT_F;
> -	case PHY_A ... PHY_E:
> -	case PHY_G ... PHY_I:
> -		return HPD_PORT_A + phy - PHY_A;
> -	default:
> -		MISSING_CASE(phy);
> -		return HPD_NONE;
> -	}
> +	return HPD_PORT_A + port - PORT_A;
>  }
>  
>  #define HPD_STORM_DETECT_PERIOD		1000
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6e9072ab30a1..dcd35cd97f01 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -113,13 +113,6 @@
>  
>  struct drm_i915_gem_object;
>  
> -/*
> - * The code assumes that the hpd_pins below have consecutive values and
> - * starting with HPD_PORT_A, the HPD pin associated with any port can be
> - * retrieved by adding the corresponding port (or phy) enum value to
> - * HPD_PORT_A in most cases. For example:
> - * HPD_PORT_C = HPD_PORT_A + PHY_C - PHY_A
> - */
>  enum hpd_pin {
>  	HPD_NONE = 0,
>  	HPD_TV = HPD_NONE,     /* TV is known to be unreliable */
> @@ -131,10 +124,12 @@ enum hpd_pin {
>  	HPD_PORT_C,
>  	HPD_PORT_D,
>  	HPD_PORT_E,
> -	HPD_PORT_F,
> -	HPD_PORT_G,
> -	HPD_PORT_H,
> -	HPD_PORT_I,
> +	HPD_PORT_TC1,
> +	HPD_PORT_TC2,
> +	HPD_PORT_TC3,
> +	HPD_PORT_TC4,
> +	HPD_PORT_TC5,
> +	HPD_PORT_TC6,
>  
>  	HPD_NUM_PINS
>  };
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 92d74448ee03..95ab4432a87d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -131,40 +131,24 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
>  };
>  
>  static const u32 hpd_gen11[HPD_NUM_PINS] = {
> -	[HPD_PORT_C] = GEN11_TC_HOTPLUG(PORT_TC1) | GEN11_TBT_HOTPLUG(PORT_TC1),
> -	[HPD_PORT_D] = GEN11_TC_HOTPLUG(PORT_TC2) | GEN11_TBT_HOTPLUG(PORT_TC2),
> -	[HPD_PORT_E] = GEN11_TC_HOTPLUG(PORT_TC3) | GEN11_TBT_HOTPLUG(PORT_TC3),
> -	[HPD_PORT_F] = GEN11_TC_HOTPLUG(PORT_TC4) | GEN11_TBT_HOTPLUG(PORT_TC4),
> -};
> -
> -static const u32 hpd_gen12[HPD_NUM_PINS] = {
> -	[HPD_PORT_D] = GEN11_TC_HOTPLUG(PORT_TC1) | GEN11_TBT_HOTPLUG(PORT_TC1),
> -	[HPD_PORT_E] = GEN11_TC_HOTPLUG(PORT_TC2) | GEN11_TBT_HOTPLUG(PORT_TC2),
> -	[HPD_PORT_F] = GEN11_TC_HOTPLUG(PORT_TC3) | GEN11_TBT_HOTPLUG(PORT_TC3),
> -	[HPD_PORT_G] = GEN11_TC_HOTPLUG(PORT_TC4) | GEN11_TBT_HOTPLUG(PORT_TC4),
> -	[HPD_PORT_H] = GEN11_TC_HOTPLUG(PORT_TC5) | GEN11_TBT_HOTPLUG(PORT_TC5),
> -	[HPD_PORT_I] = GEN11_TC_HOTPLUG(PORT_TC6) | GEN11_TBT_HOTPLUG(PORT_TC6),
> +	[HPD_PORT_TC1] = GEN11_TC_HOTPLUG(PORT_TC1) | GEN11_TBT_HOTPLUG(PORT_TC1),
> +	[HPD_PORT_TC2] = GEN11_TC_HOTPLUG(PORT_TC2) | GEN11_TBT_HOTPLUG(PORT_TC2),
> +	[HPD_PORT_TC3] = GEN11_TC_HOTPLUG(PORT_TC3) | GEN11_TBT_HOTPLUG(PORT_TC3),
> +	[HPD_PORT_TC4] = GEN11_TC_HOTPLUG(PORT_TC4) | GEN11_TBT_HOTPLUG(PORT_TC4),
> +	[HPD_PORT_TC5] = GEN11_TC_HOTPLUG(PORT_TC5) | GEN11_TBT_HOTPLUG(PORT_TC5),
> +	[HPD_PORT_TC6] = GEN11_TC_HOTPLUG(PORT_TC6) | GEN11_TBT_HOTPLUG(PORT_TC6),
>  };
>  
>  static const u32 hpd_icp[HPD_NUM_PINS] = {
> -	[HPD_PORT_A] = SDE_DDI_HOTPLUG_ICP(PORT_A),
> -	[HPD_PORT_B] = SDE_DDI_HOTPLUG_ICP(PORT_B),
> -	[HPD_PORT_C] = SDE_TC_HOTPLUG_ICP(PORT_TC1),
> -	[HPD_PORT_D] = SDE_TC_HOTPLUG_ICP(PORT_TC2),
> -	[HPD_PORT_E] = SDE_TC_HOTPLUG_ICP(PORT_TC3),
> -	[HPD_PORT_F] = SDE_TC_HOTPLUG_ICP(PORT_TC4),
> -};
> -
> -static const u32 hpd_tgp[HPD_NUM_PINS] = {
>  	[HPD_PORT_A] = SDE_DDI_HOTPLUG_ICP(PORT_A),
>  	[HPD_PORT_B] = SDE_DDI_HOTPLUG_ICP(PORT_B),
>  	[HPD_PORT_C] = SDE_DDI_HOTPLUG_ICP(PORT_C),
> -	[HPD_PORT_D] = SDE_TC_HOTPLUG_ICP(PORT_TC1),
> -	[HPD_PORT_E] = SDE_TC_HOTPLUG_ICP(PORT_TC2),
> -	[HPD_PORT_F] = SDE_TC_HOTPLUG_ICP(PORT_TC3),
> -	[HPD_PORT_G] = SDE_TC_HOTPLUG_ICP(PORT_TC4),
> -	[HPD_PORT_H] = SDE_TC_HOTPLUG_ICP(PORT_TC5),
> -	[HPD_PORT_I] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
> +	[HPD_PORT_TC1] = SDE_TC_HOTPLUG_ICP(PORT_TC1),
> +	[HPD_PORT_TC2] = SDE_TC_HOTPLUG_ICP(PORT_TC2),
> +	[HPD_PORT_TC3] = SDE_TC_HOTPLUG_ICP(PORT_TC3),
> +	[HPD_PORT_TC4] = SDE_TC_HOTPLUG_ICP(PORT_TC4),
> +	[HPD_PORT_TC5] = SDE_TC_HOTPLUG_ICP(PORT_TC5),
> +	[HPD_PORT_TC6] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
>  };
>  
>  static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
> @@ -180,9 +164,7 @@ static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
>  		return;
>  	}
>  
> -	if (INTEL_GEN(dev_priv) >= 12)
> -		hpd->hpd = hpd_gen12;
> -	else if (INTEL_GEN(dev_priv) >= 11)
> +	if (INTEL_GEN(dev_priv) >= 11)
>  		hpd->hpd = hpd_gen11;
>  	else if (IS_GEN9_LP(dev_priv))
>  		hpd->hpd = hpd_bxt;
> @@ -196,9 +178,8 @@ static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
>  	if (!HAS_PCH_SPLIT(dev_priv) || HAS_PCH_NOP(dev_priv))
>  		return;
>  
> -	if (HAS_PCH_TGP(dev_priv) || HAS_PCH_JSP(dev_priv))
> -		hpd->pch_hpd = hpd_tgp;
> -	else if (HAS_PCH_ICP(dev_priv) || HAS_PCH_MCC(dev_priv))
> +	if (HAS_PCH_TGP(dev_priv) || HAS_PCH_JSP(dev_priv) ||
> +	    HAS_PCH_ICP(dev_priv) || HAS_PCH_MCC(dev_priv))
>  		hpd->pch_hpd = hpd_icp;
>  	else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_SPT(dev_priv))
>  		hpd->pch_hpd = hpd_spt;
> @@ -1048,33 +1029,17 @@ static void ivb_parity_work(struct work_struct *work)
>  static bool gen11_port_hotplug_long_detect(enum hpd_pin pin, u32 val)
>  {
>  	switch (pin) {
> -	case HPD_PORT_C:
> +	case HPD_PORT_TC1:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC1);
> -	case HPD_PORT_D:
> +	case HPD_PORT_TC2:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC2);
> -	case HPD_PORT_E:
> +	case HPD_PORT_TC3:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC3);
> -	case HPD_PORT_F:
> +	case HPD_PORT_TC4:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC4);
> -	default:
> -		return false;
> -	}
> -}
> -
> -static bool gen12_port_hotplug_long_detect(enum hpd_pin pin, u32 val)
> -{
> -	switch (pin) {
> -	case HPD_PORT_D:
> -		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC1);
> -	case HPD_PORT_E:
> -		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC2);
> -	case HPD_PORT_F:
> -		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC3);
> -	case HPD_PORT_G:
> -		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC4);
> -	case HPD_PORT_H:
> +	case HPD_PORT_TC5:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC5);
> -	case HPD_PORT_I:
> +	case HPD_PORT_TC6:
>  		return val & GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC6);
>  	default:
>  		return false;
> @@ -1112,33 +1077,17 @@ static bool icp_ddi_port_hotplug_long_detect(enum hpd_pin pin, u32 val)
>  static bool icp_tc_port_hotplug_long_detect(enum hpd_pin pin, u32 val)
>  {
>  	switch (pin) {
> -	case HPD_PORT_C:
> +	case HPD_PORT_TC1:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1);
> -	case HPD_PORT_D:
> +	case HPD_PORT_TC2:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC2);
> -	case HPD_PORT_E:
> +	case HPD_PORT_TC3:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC3);
> -	case HPD_PORT_F:
> +	case HPD_PORT_TC4:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC4);
> -	default:
> -		return false;
> -	}
> -}
> -
> -static bool tgp_tc_port_hotplug_long_detect(enum hpd_pin pin, u32 val)
> -{
> -	switch (pin) {
> -	case HPD_PORT_D:
> -		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1);
> -	case HPD_PORT_E:
> -		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC2);
> -	case HPD_PORT_F:
> -		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC3);
> -	case HPD_PORT_G:
> -		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC4);
> -	case HPD_PORT_H:
> +	case HPD_PORT_TC5:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC5);
> -	case HPD_PORT_I:
> +	case HPD_PORT_TC6:
>  		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC6);
>  	default:
>  		return false;
> @@ -1892,19 +1841,16 @@ static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  {
>  	u32 ddi_hotplug_trigger, tc_hotplug_trigger;
>  	u32 pin_mask = 0, long_mask = 0;
> -	bool (*tc_port_hotplug_long_detect)(enum hpd_pin pin, u32 val);
>  
>  	if (HAS_PCH_TGP(dev_priv)) {
>  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
>  		tc_hotplug_trigger = pch_iir & SDE_TC_MASK_TGP;
> -		tc_port_hotplug_long_detect = tgp_tc_port_hotplug_long_detect;
>  	} else if (HAS_PCH_JSP(dev_priv)) {
>  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
>  		tc_hotplug_trigger = 0;
>  	} else if (HAS_PCH_MCC(dev_priv)) {
>  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_ICP;
>  		tc_hotplug_trigger = pch_iir & SDE_TC_HOTPLUG_ICP(PORT_TC1);
> -		tc_port_hotplug_long_detect = icp_tc_port_hotplug_long_detect;
>  	} else {
>  		drm_WARN(&dev_priv->drm, !HAS_PCH_ICP(dev_priv),
>  			 "Unrecognized PCH type 0x%x\n",
> @@ -1912,7 +1858,6 @@ static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  
>  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_ICP;
>  		tc_hotplug_trigger = pch_iir & SDE_TC_MASK_ICP;
> -		tc_port_hotplug_long_detect = icp_tc_port_hotplug_long_detect;
>  	}
>  
>  	if (ddi_hotplug_trigger) {
> @@ -1936,7 +1881,7 @@ static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
>  				   tc_hotplug_trigger, dig_hotplug_reg,
>  				   dev_priv->hotplug.pch_hpd,
> -				   tc_port_hotplug_long_detect);
> +				   icp_tc_port_hotplug_long_detect);
>  	}
>  
>  	if (pin_mask)
> @@ -2184,12 +2129,6 @@ static void gen11_hpd_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
>  	u32 pin_mask = 0, long_mask = 0;
>  	u32 trigger_tc = iir & GEN11_DE_TC_HOTPLUG_MASK;
>  	u32 trigger_tbt = iir & GEN11_DE_TBT_HOTPLUG_MASK;
> -	long_pulse_detect_func long_pulse_detect;
> -
> -	if (INTEL_GEN(dev_priv) >= 12)
> -		long_pulse_detect = gen12_port_hotplug_long_detect;
> -	else
> -		long_pulse_detect = gen11_port_hotplug_long_detect;
>  
>  	if (trigger_tc) {
>  		u32 dig_hotplug_reg;
> @@ -2200,7 +2139,7 @@ static void gen11_hpd_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
>  		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
>  				   trigger_tc, dig_hotplug_reg,
>  				   dev_priv->hotplug.hpd,
> -				   long_pulse_detect);
> +				   gen11_port_hotplug_long_detect);
>  	}
>  
>  	if (trigger_tbt) {
> @@ -2212,7 +2151,7 @@ static void gen11_hpd_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
>  		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
>  				   trigger_tbt, dig_hotplug_reg,
>  				   dev_priv->hotplug.hpd,
> -				   long_pulse_detect);
> +				   gen11_port_hotplug_long_detect);
>  	}
>  
>  	if (pin_mask)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx