[Intel-gfx] [PATCH 05/31] drm/i915/execlists: Process one CSB update at a time

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-06-27 10:46:37)
> 
> On 25/06/2018 10:48, Chris Wilson wrote:
> > In the next patch, we will process the CSB events directly from the
> > submission path, rather than only after a CS interrupt. Hence, we will
> > no longer have the need for a loop until the has-interrupt bit is clear,
> > and in the meantime can remove that small optimisation.
> 
> So strictly speaking this patch would need to go after the one which 
> removes the need to loop.

Hmm. We can just remove the test_and_set_bit here so the tasklet is
unconditionally called, that should resolve the doubt.

> > +             /*
> > +              * We are flying near dragons again.
> > +              *
> > +              * We hold a reference to the request in execlist_port[]
> > +              * but no more than that. We are operating in softirq
> > +              * context and so cannot hold any mutex or sleep. That
> > +              * prevents us stopping the requests we are processing
> > +              * in port[] from being retired simultaneously (the
> > +              * breadcrumb will be complete before we see the
> > +              * context-switch). As we only hold the reference to the
> > +              * request, any pointer chasing underneath the request
> > +              * is subject to a potential use-after-free. Thus we
> > +              * store all of the bookkeeping within port[] as
> > +              * required, and avoid using unguarded pointers beneath
> > +              * request itself. The same applies to the atomic
> > +              * status notifier.
> > +              */
> 
> I need a reformat-comment-to-wrap plugin for my editor. Just noticing 
> that some width has been freed up so number of lines could be reduced. 
> But don't waste time on it.
> 
> >   
> > -                     /* We should never get a COMPLETED | IDLE_ACTIVE! */
> > -                     GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
> > +             status = READ_ONCE(buf[2 * head]); /* maybe mmio! */
> > +             GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
> > +                       engine->name, head,
> > +                       status, buf[2*head + 1],
> > +                       execlists->active);
> > +
> > +             if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
> > +                           GEN8_CTX_STATUS_PREEMPTED))
> > +                     execlists_set_active(execlists,
> > +                                          EXECLISTS_ACTIVE_HWACK);
> > +             if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
> > +                     execlists_clear_active(execlists,
> > +                                            EXECLISTS_ACTIVE_HWACK);
> > +
> > +             if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> > +                     continue;
> >   
> > -                     if (status & GEN8_CTX_STATUS_COMPLETE &&
> > -                         buf[2*head + 1] == execlists->preempt_complete_status) {
> > -                             GEM_TRACE("%s preempt-idle\n", engine->name);
> > -                             complete_preempt_context(execlists);
> > -                             continue;
> > -                     }
> > +             /* We should never get a COMPLETED | IDLE_ACTIVE! */
> > +             GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
> >   
> > -                     if (status & GEN8_CTX_STATUS_PREEMPTED &&
> > -                         execlists_is_active(execlists,
> > -                                             EXECLISTS_ACTIVE_PREEMPT))
> > -                             continue;
> > +             if (status & GEN8_CTX_STATUS_COMPLETE &&
> > +                 buf[2*head + 1] == execlists->preempt_complete_status) {
> > +                     GEM_TRACE("%s preempt-idle\n", engine->name);
> > +                     complete_preempt_context(execlists);
> > +                     continue;
> > +             }
> >   
> > -                     GEM_BUG_ON(!execlists_is_active(execlists,
> > -                                                     EXECLISTS_ACTIVE_USER));
> > +             if (status & GEN8_CTX_STATUS_PREEMPTED &&
> > +                 execlists_is_active(execlists,
> > +                                     EXECLISTS_ACTIVE_PREEMPT))
> 
> But reformatting actual code would have probably been a good idea. Don't 
> do it now, or I'll have to re-read it all!

! I was trying to be good and just have the re-indent as separate patch
:)

> Nothing seems lost or added, so:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
Imagine with the i915_irq.c change?
-Chris