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

Chris Wilson
2018-02-13T22:02:33Z

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