On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@xxxxxxxxx> wrote:
>>
>> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
>> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> >> >> From: Elaine Wang <elaine.wang@xxxxxxxxx>
>> >> >>
>> >> >> 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@xxxxxxxxxxxxxxxxxx>
>> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
>> >> >> Signed-off-by: Elaine Wang <elaine.wang@xxxxxxxxx>
>> >> >> ---
>> >> >> 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@xxxxxxxxxxxxxxxxxxxxx
>> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
