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:
> 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.


> 	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx