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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> 
>> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@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@linux.intel.com> wrote:
>> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> >> >> From: Elaine Wang <elaine.wang@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@chris-wilson.co.uk>
>> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> >> Signed-off-by: Elaine Wang <elaine.wang@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.

Ville, how about adding something like:

#define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)

and possibly

#define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)

So we could clarify the code, and abstract the rules, so it'll perhaps
be easier to change them later?

Though I fear we may end up needing to add a finer granularity depending
on which parts we do need to and must not touch, and that might not
always map to low granularity north/south display. *shrug*

BR,
Jani.


> 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@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx