[Intel-gfx] [PATCH] drm/i915: Poison rings after use

[Chris Wilson]

Quoting Mika Kuoppala (2020-02-11 13:53:56)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On retiring the request, we should not re-use these elements in the ring
> > (at least not until we fill the ringbuffer and knowingly reuse the space).
> > Leave behind some poison to (hopefully) trap ourselves if we make a
> > mistake.
> >
> > Suggested-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 0ecc2cf64216..9ee7bf0200b0 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -203,6 +203,19 @@ static void free_capture_list(struct i915_request *request)
> >       }
> >  }
> >  
> > +static void __i915_request_fill(struct i915_request *rq, u8 val)
> > +{
> > +     void *vaddr = rq->ring->vaddr;
> > +     u32 head;
> > +
> > +     head = rq->infix;
> > +     if (rq->postfix < head) {
> > +             memset(vaddr + head, val, rq->ring->size - head);
> > +             head = 0;
> > +     }
> > +     memset(vaddr + head, val, rq->postfix - head);
> > +}
> > +
> >  static void remove_from_engine(struct i915_request *rq)
> >  {
> >       struct intel_engine_cs *engine, *locked;
> > @@ -247,6 +260,9 @@ bool i915_request_retire(struct i915_request *rq)
> >        */
> >       GEM_BUG_ON(!list_is_first(&rq->link,
> >                                 &i915_request_timeline(rq)->requests));
> > +     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> > +             /* Poison before we release our space in the ring */
> > +             __i915_request_fill(rq, POISON_FREE);
> 
> Would it be too detrimental for perf to check for POISON_FREE when
> we emit the requests?

intel_ring_begin() does memset32(cs, POISON_INUSE, bytes / sizeof(*cs))
already, with the expectation that the HW dies if we miss a dword.

We could check in intel_ring_begin() that it is previously POISON_FREE
(we'd need to poison at allocation as well).

For it to be practical, we would also need to dump more information at
the point of detection. Which feels like it wants to reuse the debug
info we dump for errors. A bit more work, and should look at what we can
reuse from slab-debug and friends.
-Chris