diff mbox series

[2/4] drm/i915: unify icp, tgp and mcc irq handling

Message ID 20190829211526.30525-2-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/i915: parameterize south hpd macros | expand

Commit Message

Souza, Jose Aug. 29, 2019, 9:15 p.m. UTC 
From: Lucas De Marchi <lucas.demarchi@intel.com>

The differences are only on the pins, trigger and long_detect function.
The MCC handling is already partially merged, so merge TGP as well.
Remove the pins argument from icp_irq_handler() so we have all the
differences between the 3 set in a common if ladder.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 65 ++++++++-------------------------
 1 file changed, 16 insertions(+), 49 deletions(-)

Comments

Matt Roper Aug. 29, 2019, 11:05 p.m. UTC | #1 
On Thu, Aug 29, 2019 at 02:15:24PM -0700, José Roberto de Souza wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> The differences are only on the pins, trigger and long_detect function.
> The MCC handling is already partially merged, so merge TGP as well.
> Remove the pins argument from icp_irq_handler() so we have all the
> differences between the 3 set in a common if ladder.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Now that everything is parameterized would it be worth unifying the tc
long detect functions too?  E.g., something like

    if (HAS_PCH_TGP(dev_priv))
        return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1 + pin - HPD_PORT_D);
    else if (HAS_PCH_ICP(dev_priv))
        return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1 + pin - HPD_PORT_C);
    else
        MISSING_CASE(INTEL_PCH_TYPE(dev_priv));

Even if you decide to keep it as is, this patch is

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 65 ++++++++-------------------------
>  1 file changed, 16 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 084e322ec15b..5f590987dcd5 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2243,19 +2243,27 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  		cpt_serr_int_handler(dev_priv);
>  }
>  
> -static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir,
> -			    const u32 *pins)
> +static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  {
> -	u32 ddi_hotplug_trigger;
> -	u32 tc_hotplug_trigger;
> +	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);
> +	const u32 *pins;
>  
> -	if (HAS_PCH_MCC(dev_priv)) {
> +	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;
> +		pins = hpd_tgp;
> +	} else if (HAS_PCH_MCC(dev_priv)) {
>  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
>  		tc_hotplug_trigger = 0;
> +		pins = hpd_mcc;
>  	} else {
>  		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;
> +		pins = hpd_icp;
>  	}
>  
>  	if (ddi_hotplug_trigger) {
> @@ -2279,44 +2287,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, pins,
> -				   icp_tc_port_hotplug_long_detect);
> -	}
> -
> -	if (pin_mask)
> -		intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
> -
> -	if (pch_iir & SDE_GMBUS_ICP)
> -		gmbus_irq_handler(dev_priv);
> -}
> -
> -static void tgp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
> -{
> -	u32 ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
> -	u32 tc_hotplug_trigger = pch_iir & SDE_TC_MASK_TGP;
> -	u32 pin_mask = 0, long_mask = 0;
> -
> -	if (ddi_hotplug_trigger) {
> -		u32 dig_hotplug_reg;
> -
> -		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
> -		I915_WRITE(SHOTPLUG_CTL_DDI, dig_hotplug_reg);
> -
> -		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
> -				   ddi_hotplug_trigger,
> -				   dig_hotplug_reg, hpd_tgp,
> -				   icp_ddi_port_hotplug_long_detect);
> -	}
> -
> -	if (tc_hotplug_trigger) {
> -		u32 dig_hotplug_reg;
> -
> -		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
> -		I915_WRITE(SHOTPLUG_CTL_TC, dig_hotplug_reg);
> -
> -		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
> -				   tc_hotplug_trigger,
> -				   dig_hotplug_reg, hpd_tgp,
> -				   tgp_tc_port_hotplug_long_detect);
> +				   tc_port_hotplug_long_detect);
>  	}
>  
>  	if (pin_mask)
> @@ -2767,12 +2738,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  			I915_WRITE(SDEIIR, iir);
>  			ret = IRQ_HANDLED;
>  
> -			if (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP)
> -				tgp_irq_handler(dev_priv, iir);
> -			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MCC)
> -				icp_irq_handler(dev_priv, iir, hpd_mcc);
> -			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> -				icp_irq_handler(dev_priv, iir, hpd_icp);
> +			if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> +				icp_irq_handler(dev_priv, iir);
>  			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
>  				spt_irq_handler(dev_priv, iir);
>  			else
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 30, 2019, 6:06 p.m. UTC | #2 
On Thu, 2019-08-29 at 16:05 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 02:15:24PM -0700, José Roberto de Souza
> wrote:
> > From: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> > The differences are only on the pins, trigger and long_detect
> > function.
> > The MCC handling is already partially merged, so merge TGP as well.
> > Remove the pins argument from icp_irq_handler() so we have all the
> > differences between the 3 set in a common if ladder.
> > 
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Now that everything is parameterized would it be worth unifying the
> tc
> long detect functions too?  E.g., something like
> 
>     if (HAS_PCH_TGP(dev_priv))
>         return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1 + pin -
> HPD_PORT_D);
>     else if (HAS_PCH_ICP(dev_priv))
>         return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1 + pin -
> HPD_PORT_C);
>     else
>         MISSING_CASE(INTEL_PCH_TYPE(dev_priv));
> 
> Even if you decide to keep it as is, this patch is
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


We can do it on top but I personally don't like keep this much detail
of register in .c files.

Thanks for the reviews.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 65 ++++++++---------------------
> > ----
> >  1 file changed, 16 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 084e322ec15b..5f590987dcd5 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2243,19 +2243,27 @@ static void cpt_irq_handler(struct
> > drm_i915_private *dev_priv, u32 pch_iir)
> >  		cpt_serr_int_handler(dev_priv);
> >  }
> >  
> > -static void icp_irq_handler(struct drm_i915_private *dev_priv, u32
> > pch_iir,
> > -			    const u32 *pins)
> > +static void icp_irq_handler(struct drm_i915_private *dev_priv, u32
> > pch_iir)
> >  {
> > -	u32 ddi_hotplug_trigger;
> > -	u32 tc_hotplug_trigger;
> > +	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);
> > +	const u32 *pins;
> >  
> > -	if (HAS_PCH_MCC(dev_priv)) {
> > +	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;
> > +		pins = hpd_tgp;
> > +	} else if (HAS_PCH_MCC(dev_priv)) {
> >  		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
> >  		tc_hotplug_trigger = 0;
> > +		pins = hpd_mcc;
> >  	} else {
> >  		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;
> > +		pins = hpd_icp;
> >  	}
> >  
> >  	if (ddi_hotplug_trigger) {
> > @@ -2279,44 +2287,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, pins,
> > -				   icp_tc_port_hotplug_long_detect);
> > -	}
> > -
> > -	if (pin_mask)
> > -		intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
> > -
> > -	if (pch_iir & SDE_GMBUS_ICP)
> > -		gmbus_irq_handler(dev_priv);
> > -}
> > -
> > -static void tgp_irq_handler(struct drm_i915_private *dev_priv, u32
> > pch_iir)
> > -{
> > -	u32 ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
> > -	u32 tc_hotplug_trigger = pch_iir & SDE_TC_MASK_TGP;
> > -	u32 pin_mask = 0, long_mask = 0;
> > -
> > -	if (ddi_hotplug_trigger) {
> > -		u32 dig_hotplug_reg;
> > -
> > -		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
> > -		I915_WRITE(SHOTPLUG_CTL_DDI, dig_hotplug_reg);
> > -
> > -		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
> > -				   ddi_hotplug_trigger,
> > -				   dig_hotplug_reg, hpd_tgp,
> > -				   icp_ddi_port_hotplug_long_detect);
> > -	}
> > -
> > -	if (tc_hotplug_trigger) {
> > -		u32 dig_hotplug_reg;
> > -
> > -		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
> > -		I915_WRITE(SHOTPLUG_CTL_TC, dig_hotplug_reg);
> > -
> > -		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
> > -				   tc_hotplug_trigger,
> > -				   dig_hotplug_reg, hpd_tgp,
> > -				   tgp_tc_port_hotplug_long_detect);
> > +				   tc_port_hotplug_long_detect);
> >  	}
> >  
> >  	if (pin_mask)
> > @@ -2767,12 +2738,8 @@ gen8_de_irq_handler(struct drm_i915_private
> > *dev_priv, u32 master_ctl)
> >  			I915_WRITE(SDEIIR, iir);
> >  			ret = IRQ_HANDLED;
> >  
> > -			if (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP)
> > -				tgp_irq_handler(dev_priv, iir);
> > -			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MCC)
> > -				icp_irq_handler(dev_priv, iir,
> > hpd_mcc);
> > -			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > -				icp_irq_handler(dev_priv, iir,
> > hpd_icp);
> > +			if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > +				icp_irq_handler(dev_priv, iir);
> >  			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> >  				spt_irq_handler(dev_priv, iir);
> >  			else
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 084e322ec15b..5f590987dcd5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2243,19 +2243,27 @@  static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 		cpt_serr_int_handler(dev_priv);
 }
 
-static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir,
-			    const u32 *pins)
+static void icp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 {
-	u32 ddi_hotplug_trigger;
-	u32 tc_hotplug_trigger;
+	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);
+	const u32 *pins;
 
-	if (HAS_PCH_MCC(dev_priv)) {
+	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;
+		pins = hpd_tgp;
+	} else if (HAS_PCH_MCC(dev_priv)) {
 		ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
 		tc_hotplug_trigger = 0;
+		pins = hpd_mcc;
 	} else {
 		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;
+		pins = hpd_icp;
 	}
 
 	if (ddi_hotplug_trigger) {
@@ -2279,44 +2287,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, pins,
-				   icp_tc_port_hotplug_long_detect);
-	}
-
-	if (pin_mask)
-		intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
-
-	if (pch_iir & SDE_GMBUS_ICP)
-		gmbus_irq_handler(dev_priv);
-}
-
-static void tgp_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
-{
-	u32 ddi_hotplug_trigger = pch_iir & SDE_DDI_MASK_TGP;
-	u32 tc_hotplug_trigger = pch_iir & SDE_TC_MASK_TGP;
-	u32 pin_mask = 0, long_mask = 0;
-
-	if (ddi_hotplug_trigger) {
-		u32 dig_hotplug_reg;
-
-		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
-		I915_WRITE(SHOTPLUG_CTL_DDI, dig_hotplug_reg);
-
-		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   ddi_hotplug_trigger,
-				   dig_hotplug_reg, hpd_tgp,
-				   icp_ddi_port_hotplug_long_detect);
-	}
-
-	if (tc_hotplug_trigger) {
-		u32 dig_hotplug_reg;
-
-		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
-		I915_WRITE(SHOTPLUG_CTL_TC, dig_hotplug_reg);
-
-		intel_get_hpd_pins(dev_priv, &pin_mask, &long_mask,
-				   tc_hotplug_trigger,
-				   dig_hotplug_reg, hpd_tgp,
-				   tgp_tc_port_hotplug_long_detect);
+				   tc_port_hotplug_long_detect);
 	}
 
 	if (pin_mask)
@@ -2767,12 +2738,8 @@  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 			I915_WRITE(SDEIIR, iir);
 			ret = IRQ_HANDLED;
 
-			if (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP)
-				tgp_irq_handler(dev_priv, iir);
-			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MCC)
-				icp_irq_handler(dev_priv, iir, hpd_mcc);
-			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
-				icp_irq_handler(dev_priv, iir, hpd_icp);
+			if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
+				icp_irq_handler(dev_priv, iir);
 			else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
 				spt_irq_handler(dev_priv, iir);
 			else