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@xxxxxxxxx>
>
Imagine with the i915_irq.c change?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
