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

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

On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 27, 2016 at 04:46:40PM +0200, Jani Nikula wrote:
>> On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Thu, Dec 22, 2016 at 01:18:16PM +0200, David Weinehall wrote:
>> >> On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
>> >> > 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*
>> >> 
>> >> Even if we might end up making things more granular later on,
>> >> I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
>> >> explains what we're testing for, as does HAS_SOUTH_DISPLAY().
>> >
>> > +1 on HAS_DISPLAY.
>> >
>> > Re PCH_NOP vs. PCH_NONE:
>> > - PCH_NONE is for platforms where there's really no PCH anywhere.
>> > - PCH_NOP is for platforms that in general have it, but don't touch it
>> >   it's kinda disabled. This is somewhat relevant to make sure all the
>> >   HAS_PCH_SPLIT checks (of which not all are exclusively in display-only
>> >   code) still work correctly for those platforms.
>> >
>> > Given that I'm not entirely sure what you're aiming for with
>> > HAS_SOUTH_DISPLAY ...
>> 
>> Potentially a more sensible sounding check than HAS_PCH_NOP.
>
> Hm, isn't that covered with HAS_PCH_SPLIT already?

It is, but these are not the same thing, obviously:

#define HAS_PCH_NOP(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_NOP)
#define HAS_PCH_SPLIT(dev_priv) (INTEL_PCH_TYPE(dev_priv) != PCH_NONE)

It's just that checking for HAS_PCH_NOP does not adequately describe
what is being checked, which is clear from the discussion around
PCH_NOP.

BR,
Jani.



> -Daniel

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