Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Stash hpd status bits under dev_priv

2020-05-07 Thread Imre Deak 
On Thu, May 07, 2020 at 09:53:13AM +0300, Ville Syrjälä wrote:
> On Wed, May 06, 2020 at 07:03:41PM +0300, Imre Deak wrote:
> > On Wed, Mar 11, 2020 at 05:54:21PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > Instead of constnantly having to figure out which hpd status bit
> > > array to use let's store them under dev_priv.
> > > 
> > > Should perhaps take this further and stash even more stuff to
> > > make the hpd handling more abstract yet.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |   2 +
> > >  drivers/gpu/drm/i915/i915_irq.c | 198 ++--
> > >  2 files changed, 111 insertions(+), 89 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 19195bde4921..b5afbabf4c3b 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -149,6 +149,8 @@ enum hpd_pin {
> > >  struct i915_hotplug {
> > >   struct delayed_work hotplug_work;
> > >  
> > > + const u32 *hpd, *pch_hpd;
> > > +
> > >   struct {
> > >   unsigned long last_jiffies;
> > >   int count;
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 9f0653cf0510..b95d952bd47d 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -124,7 +124,6 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
> > >   [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS,
> > >  };
> > >  
> > > -/* BXT hpd list */
> > >  static const u32 hpd_bxt[HPD_NUM_PINS] = {
> > >   [HPD_PORT_A] = BXT_DE_PORT_HP_DDIA,
> > >   [HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
> > > @@ -168,6 +167,44 @@ static const u32 hpd_tgp[HPD_NUM_PINS] = {
> > >   [HPD_PORT_I] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
> > >  };
> > >  
> > > +static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
> > > +{
> > > + struct i915_hotplug *hpd = _priv->hotplug;
> > > +
> > > + if (HAS_GMCH(dev_priv)) {
> > > + if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> > > + IS_CHERRYVIEW(dev_priv))
> > > + hpd->hpd = hpd_status_g4x;
> > > + else
> > > + hpd->hpd = hpd_status_i915;
> > > + return;
> > > + }
> > > +
> > > + if (INTEL_GEN(dev_priv) >= 12)
> > > + hpd->hpd = hpd_gen12;
> > > + else if (INTEL_GEN(dev_priv) >= 11)
> > > + hpd->hpd = hpd_gen11;
> > > + else if (IS_GEN9_LP(dev_priv))
> > > + hpd->hpd = hpd_bxt;
> > > + else if (INTEL_GEN(dev_priv) >= 8)
> > > + hpd->hpd = hpd_bdw;
> > > + else if (INTEL_GEN(dev_priv) >= 7)
> > > + hpd->hpd = hpd_ivb;
> > > + else
> > > + hpd->hpd = hpd_ilk;
> > > +
> > > + 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))
> > > + hpd->pch_hpd = hpd_icp;
> > > + else if (HAS_PCH_SPT(dev_priv))
> > 
> > What about CNP?
> 
> Argh. We have too many of these. Do we even need all of them?

Not sure, but atm it's special cased in a few places (ddc/hpd setup, and
rawclk readout). Here it's just the same as SPT.

> > > + hpd->pch_hpd = hpd_spt;
> > > + else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_CPT(dev_priv))
> > > + hpd->pch_hpd = hpd_cpt;
> > > + else if (HAS_PCH_IBX(dev_priv))
> > > + hpd->pch_hpd = hpd_ibx;
> > 
> > Can we add MISSING_CASE for PCH platforms?
> 
> else if (HAS_PCH_SPLIT())
>   MISSING_CASE(INTEL_PCH_TYPE())
> ?

Yes, with && !PCH_NOP.

> 
> -- 
> Ville Syrjälä
> Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Stash hpd status bits under dev_priv

2020-05-07 Thread Ville Syrjälä 
On Wed, May 06, 2020 at 07:03:41PM +0300, Imre Deak wrote:
> On Wed, Mar 11, 2020 at 05:54:21PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Instead of constnantly having to figure out which hpd status bit
> > array to use let's store them under dev_priv.
> > 
> > Should perhaps take this further and stash even more stuff to
> > make the hpd handling more abstract yet.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |   2 +
> >  drivers/gpu/drm/i915/i915_irq.c | 198 ++--
> >  2 files changed, 111 insertions(+), 89 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 19195bde4921..b5afbabf4c3b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -149,6 +149,8 @@ enum hpd_pin {
> >  struct i915_hotplug {
> > struct delayed_work hotplug_work;
> >  
> > +   const u32 *hpd, *pch_hpd;
> > +
> > struct {
> > unsigned long last_jiffies;
> > int count;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 9f0653cf0510..b95d952bd47d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -124,7 +124,6 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
> > [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS,
> >  };
> >  
> > -/* BXT hpd list */
> >  static const u32 hpd_bxt[HPD_NUM_PINS] = {
> > [HPD_PORT_A] = BXT_DE_PORT_HP_DDIA,
> > [HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
> > @@ -168,6 +167,44 @@ static const u32 hpd_tgp[HPD_NUM_PINS] = {
> > [HPD_PORT_I] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
> >  };
> >  
> > +static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
> > +{
> > +   struct i915_hotplug *hpd = _priv->hotplug;
> > +
> > +   if (HAS_GMCH(dev_priv)) {
> > +   if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> > +   IS_CHERRYVIEW(dev_priv))
> > +   hpd->hpd = hpd_status_g4x;
> > +   else
> > +   hpd->hpd = hpd_status_i915;
> > +   return;
> > +   }
> > +
> > +   if (INTEL_GEN(dev_priv) >= 12)
> > +   hpd->hpd = hpd_gen12;
> > +   else if (INTEL_GEN(dev_priv) >= 11)
> > +   hpd->hpd = hpd_gen11;
> > +   else if (IS_GEN9_LP(dev_priv))
> > +   hpd->hpd = hpd_bxt;
> > +   else if (INTEL_GEN(dev_priv) >= 8)
> > +   hpd->hpd = hpd_bdw;
> > +   else if (INTEL_GEN(dev_priv) >= 7)
> > +   hpd->hpd = hpd_ivb;
> > +   else
> > +   hpd->hpd = hpd_ilk;
> > +
> > +   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))
> > +   hpd->pch_hpd = hpd_icp;
> > +   else if (HAS_PCH_SPT(dev_priv))
> 
> What about CNP?

Argh. We have too many of these. Do we even need all of them?

> 
> > +   hpd->pch_hpd = hpd_spt;
> > +   else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_CPT(dev_priv))
> > +   hpd->pch_hpd = hpd_cpt;
> > +   else if (HAS_PCH_IBX(dev_priv))
> > +   hpd->pch_hpd = hpd_ibx;
> 
> Can we add MISSING_CASE for PCH platforms?

else if (HAS_PCH_SPLIT())
MISSING_CASE(INTEL_PCH_TYPE())
?

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Stash hpd status bits under dev_priv

2020-05-06 Thread Imre Deak 
On Wed, Mar 11, 2020 at 05:54:21PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Instead of constnantly having to figure out which hpd status bit
> array to use let's store them under dev_priv.
> 
> Should perhaps take this further and stash even more stuff to
> make the hpd handling more abstract yet.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h |   2 +
>  drivers/gpu/drm/i915/i915_irq.c | 198 ++--
>  2 files changed, 111 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 19195bde4921..b5afbabf4c3b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -149,6 +149,8 @@ enum hpd_pin {
>  struct i915_hotplug {
>   struct delayed_work hotplug_work;
>  
> + const u32 *hpd, *pch_hpd;
> +
>   struct {
>   unsigned long last_jiffies;
>   int count;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9f0653cf0510..b95d952bd47d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -124,7 +124,6 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
>   [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS,
>  };
>  
> -/* BXT hpd list */
>  static const u32 hpd_bxt[HPD_NUM_PINS] = {
>   [HPD_PORT_A] = BXT_DE_PORT_HP_DDIA,
>   [HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
> @@ -168,6 +167,44 @@ static const u32 hpd_tgp[HPD_NUM_PINS] = {
>   [HPD_PORT_I] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
>  };
>  
> +static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
> +{
> + struct i915_hotplug *hpd = _priv->hotplug;
> +
> + if (HAS_GMCH(dev_priv)) {
> + if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> + IS_CHERRYVIEW(dev_priv))
> + hpd->hpd = hpd_status_g4x;
> + else
> + hpd->hpd = hpd_status_i915;
> + return;
> + }
> +
> + if (INTEL_GEN(dev_priv) >= 12)
> + hpd->hpd = hpd_gen12;
> + else if (INTEL_GEN(dev_priv) >= 11)
> + hpd->hpd = hpd_gen11;
> + else if (IS_GEN9_LP(dev_priv))
> + hpd->hpd = hpd_bxt;
> + else if (INTEL_GEN(dev_priv) >= 8)
> + hpd->hpd = hpd_bdw;
> + else if (INTEL_GEN(dev_priv) >= 7)
> + hpd->hpd = hpd_ivb;
> + else
> + hpd->hpd = hpd_ilk;
> +
> + 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))
> + hpd->pch_hpd = hpd_icp;
> + else if (HAS_PCH_SPT(dev_priv))

What about CNP?

> + hpd->pch_hpd = hpd_spt;
> + else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_CPT(dev_priv))
> + hpd->pch_hpd = hpd_cpt;
> + else if (HAS_PCH_IBX(dev_priv))
> + hpd->pch_hpd = hpd_ibx;

Can we add MISSING_CASE for PCH platforms?

> +}
> +
>  static void
>  intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
> @@ -1504,33 +1541,27 @@ static void i9xx_hpd_irq_handler(struct 
> drm_i915_private *dev_priv,
>u32 hotplug_status)
>  {
>   u32 pin_mask = 0, long_mask = 0;
> + u32 hotplug_trigger;
>  
> - if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> - IS_CHERRYVIEW(dev_priv)) {
> - u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
> + if (IS_G4X(dev_priv) ||
> + IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
> + else
> + hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>  
> - if (hotplug_trigger) {
> - intel_get_hpd_pins(dev_priv, _mask, _mask,
> -hotplug_trigger, hotplug_trigger,
> -hpd_status_g4x,
> -i9xx_port_hotplug_long_detect);
> + if (hotplug_trigger) {
> + intel_get_hpd_pins(dev_priv, _mask, _mask,
> +hotplug_trigger, hotplug_trigger,
> +dev_priv->hotplug.hpd,
> +i9xx_port_hotplug_long_detect);
>  
> - intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
> - }
> -
> - if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
> - dp_aux_irq_handler(dev_priv);
> - } else {
> - u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
> -
> - if (hotplug_trigger) {
> - intel_get_hpd_pins(dev_priv, _mask, _mask,
> -hotplug_trigger, hotplug_trigger,
> -hpd_status_i915,
> -

[Intel-gfx] [PATCH v3 2/3] drm/i915: Stash hpd status bits under dev_priv

2020-03-11 Thread Ville Syrjala 
From: Ville Syrjälä 

Instead of constnantly having to figure out which hpd status bit
array to use let's store them under dev_priv.

Should perhaps take this further and stash even more stuff to
make the hpd handling more abstract yet.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h |   2 +
 drivers/gpu/drm/i915/i915_irq.c | 198 ++--
 2 files changed, 111 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 19195bde4921..b5afbabf4c3b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -149,6 +149,8 @@ enum hpd_pin {
 struct i915_hotplug {
struct delayed_work hotplug_work;
 
+   const u32 *hpd, *pch_hpd;
+
struct {
unsigned long last_jiffies;
int count;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9f0653cf0510..b95d952bd47d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -124,7 +124,6 @@ static const u32 hpd_status_i915[HPD_NUM_PINS] = {
[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS,
 };
 
-/* BXT hpd list */
 static const u32 hpd_bxt[HPD_NUM_PINS] = {
[HPD_PORT_A] = BXT_DE_PORT_HP_DDIA,
[HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
@@ -168,6 +167,44 @@ static const u32 hpd_tgp[HPD_NUM_PINS] = {
[HPD_PORT_I] = SDE_TC_HOTPLUG_ICP(PORT_TC6),
 };
 
+static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
+{
+   struct i915_hotplug *hpd = _priv->hotplug;
+
+   if (HAS_GMCH(dev_priv)) {
+   if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
+   IS_CHERRYVIEW(dev_priv))
+   hpd->hpd = hpd_status_g4x;
+   else
+   hpd->hpd = hpd_status_i915;
+   return;
+   }
+
+   if (INTEL_GEN(dev_priv) >= 12)
+   hpd->hpd = hpd_gen12;
+   else if (INTEL_GEN(dev_priv) >= 11)
+   hpd->hpd = hpd_gen11;
+   else if (IS_GEN9_LP(dev_priv))
+   hpd->hpd = hpd_bxt;
+   else if (INTEL_GEN(dev_priv) >= 8)
+   hpd->hpd = hpd_bdw;
+   else if (INTEL_GEN(dev_priv) >= 7)
+   hpd->hpd = hpd_ivb;
+   else
+   hpd->hpd = hpd_ilk;
+
+   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))
+   hpd->pch_hpd = hpd_icp;
+   else if (HAS_PCH_SPT(dev_priv))
+   hpd->pch_hpd = hpd_spt;
+   else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_CPT(dev_priv))
+   hpd->pch_hpd = hpd_cpt;
+   else if (HAS_PCH_IBX(dev_priv))
+   hpd->pch_hpd = hpd_ibx;
+}
+
 static void
 intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
@@ -1504,33 +1541,27 @@ static void i9xx_hpd_irq_handler(struct 
drm_i915_private *dev_priv,
 u32 hotplug_status)
 {
u32 pin_mask = 0, long_mask = 0;
+   u32 hotplug_trigger;
 
-   if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
-   IS_CHERRYVIEW(dev_priv)) {
-   u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
+   if (IS_G4X(dev_priv) ||
+   IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+   hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
+   else
+   hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
-   if (hotplug_trigger) {
-   intel_get_hpd_pins(dev_priv, _mask, _mask,
-  hotplug_trigger, hotplug_trigger,
-  hpd_status_g4x,
-  i9xx_port_hotplug_long_detect);
+   if (hotplug_trigger) {
+   intel_get_hpd_pins(dev_priv, _mask, _mask,
+  hotplug_trigger, hotplug_trigger,
+  dev_priv->hotplug.hpd,
+  i9xx_port_hotplug_long_detect);
 
-   intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
-   }
-
-   if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
-   dp_aux_irq_handler(dev_priv);
-   } else {
-   u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
-
-   if (hotplug_trigger) {
-   intel_get_hpd_pins(dev_priv, _mask, _mask,
-  hotplug_trigger, hotplug_trigger,
-  hpd_status_i915,
-  i9xx_port_hotplug_long_detect);
-   intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
-   }
+   intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
}
+
+   if