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

[Imre Deak <imre.deak@intel.com>]

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ä <ville.syrjala@linux.intel.com>
> > > 
> > > 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ä <ville.syrjala@linux.intel.com>
> > > ---
> > >  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 = &dev_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