Re: [PATCH 05/20] drm/i915/icl: Interrupt handling

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Oscar Mateo <oscar.mateo@intel.com>,
	Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
	Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 05/20] drm/i915/icl: Interrupt handling
Date: Tue, 13 Feb 2018 22:02:26 +0000	[thread overview]
Message-ID: <151855934632.31524.10852028459060113134@mail.alporthouse.com> (raw)
In-Reply-To: <fb64c23f-2760-f90f-03a9-7ffe7d2167a3@intel.com>

Quoting Oscar Mateo (2018-02-13 21:56:32)
> 
> 
> On 02/13/2018 11:18 AM, Daniele Ceraolo Spurio wrote:
> >
> >
> > On 13/02/18 08:37, Mika Kuoppala wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> +static irqreturn_t
> >> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> >> +{
> >> +    irqreturn_t ret = IRQ_NONE;
> >> +    u16 irq[2][32];
> >> +    u32 dw, ident;
> >> +    unsigned long tmp;
> >> +    unsigned int bank, bit, engine;
> >> +    unsigned long wait_start, wait_end;
> >> +
> >> +    memset(irq, 0, sizeof(irq));
> >> +
> >> +    for (bank = 0; bank < 2; bank++) {
> >> +        if (master_ctl & GEN11_GT_DW_IRQ(bank)) {
> >> +            dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
> >> +            if (!dw)
> >> +                DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> >> +            tmp = dw;
> >> +            for_each_set_bit(bit, &tmp, 32) {
> >> +                I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit);
> >> +                wait_start = local_clock() >> 10;
> >> +                /* NB: Specs do not specify how long to spin wait.
> >> +                 * Taking 100us as an educated guess */
> >> +                wait_end = wait_start + 100;
> >> +                do {
> >> +                    ident = 
> >> I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
> >> +                } while (!(ident & GEN11_INTR_DATA_VALID) &&
> >> +                     !time_after((unsigned long)local_clock() >> 10, 
> >> wait_end));
> >
> > Patch 18/20 changes this to time_after64 and wait_end to u64, which 
> > looks like the correct way to handle this. Since splitting part of 
> > this loop out also helps making things cleaner, would it make sense to 
> > squash patch 18/20 into this one?
> >
> 
> Makes sense to me. Tvrtko, what do you think?

The squash should be made, but time_after64 is no more correct since the
native 32b/64b wrapped arithmetic is accurate. So what can be done here
is remove the casts and use time_after32() if we truly cared.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx