[Intel-gfx] [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang, Elaine]

> 
> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang at intel.com> wrote:
> > Hi Jani, Ville,
> >
> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
> >
> > Thanks,
> > Elaine
> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> >> From: Elaine Wang <elaine.wang at intel.com>
> >> >>
> >> >> Some platforms don't have display. To avoid accessing the
> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
> >> >> IRQ install or reset function.
> >> >>
> >> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >> >> Signed-off-by: Elaine Wang <elaine.wang at intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
> >> >> drm_device
> >> *dev)
> >> >>
> >> POWER_DOMAIN_PIPE(pipe)))
> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >> >>
> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> +	}
> >> >
> >> > Hmm. These are north side registers so looking at PCH_NOP feels
> >> > questionable.
> >>
> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> >>
> >> Jani.
> >
> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
> saw
> > following code in i915_drv.c. Is there any exception?
> >
> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
> > drm_i915_private *dev_priv) {
> > 	struct pci_dev *pch = NULL;
> >
> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
> setting
> > 	 * (which really amounts to a PCH but no South Display).
> > 	 */
> 
> The key is in this comment; "In all current cases", where "current" is 3½ years
> ago. IIRC this was written for some Xeons which did have a PCH but no
> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
> need the North Display for some outputs. And I guess you might still have a
> PCH but no display at all.
> 
> I'm just saying, we should not overload this hack to, say, cover platforms that
> don't even have a PCH, or platforms that have a PCH but a functioning North
> Display.
> 
> BR,
> Jani.
> 
I understand your point now. Thank you for explaining this. I'll update the patch and
Use num_pipes for checking whether display engine exists.
Thanks,
Elaine
> 
> > 	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> > 		dev_priv->pch_type = PCH_NOP;
> > 		return;
> > 	}
> >
> > Thanks,
> > Elaine
> >>
> >>
> >> >
> >> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
> >> >>
> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct
> >> >> drm_device
> >> *dev)
> >> >>  		ibx_irq_pre_postinstall(dev);
> >> >>
> >> >>  	gen8_gt_irq_postinstall(dev_priv);
> >> >> -	gen8_de_irq_postinstall(dev_priv);
> >> >> +
> >> >> +	if (!HAS_PCH_NOP(dev_priv))
> >> >> +		gen8_de_irq_postinstall(dev_priv);
> >> >>
> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> >>  		ibx_irq_postinstall(dev);
> >> >> --
> >> >> 1.9.1
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Technology Center