Re: [PATCH] drm/i915: Reinstate order of operations in {intel, logical}_ring_begin()

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Reinstate order of operations in {intel, logical}_ring_begin()
Date: Tue, 16 Jun 2015 14:18:55 +0200	[thread overview]
Message-ID: <20150616121855.GT23637@phenom.ffwll.local> (raw)
In-Reply-To: <55800291.7060201@intel.com>

On Tue, Jun 16, 2015 at 12:03:45PM +0100, Dave Gordon wrote:
> On 15/06/15 21:41, Chris Wilson wrote:
> > On Mon, Jun 15, 2015 at 07:11:37PM +0100, Dave Gordon wrote:
> >>> It still applies. If you submit say 1024 interrupted execbuffers they
> >>
> >> What is an interrupted execbuffer? AFAICT we hold the struct_mutex while
> >> stuffing the ringbuffer so we can only ever be in the process of adding
> >> instructions to one ringbuffer at a time, and we don't (now) interleave
> >> any flip commands (execlists mode requires mmio flip). Is there still
> >> something that just adds random stuff to someone else's OLR?
> > 
> > Write commands to stream, fail to add the request because the wait for
> > ring space fails due to a pending signals. Repeat until the entire ring
> > is filled by a single pending request.
> > -Chris
> 
> Uuuurgh ... I always said there was something wrong with leaving partial
> command streams in the ringbuffer :(
> 
> In the Android version we make sure that can't happen by checking that
> there's going to be enough space for all the commands generated by the
> request before we start writing into the ringbuffer, so no ring_begin()
> call thereafter can ever have to wait for more space.

Yeah that's the guarantee that the olr removal series should finally put
into place by pre-reserving sufficient amounts of ringspace to guarantee
we can put the execbuf in fully or not at all.

> Of course, having a global lock doesn't help. With multiple ringbuffers
> per engine, one should really only need a per-ringbuffer lock for
> writing into the ringbuffer ... and we should reset the tail pointer if
> writing the request is interrupted or aborted.

Maybe someone should look at per-buffer locks before trying to split up
the low-level hw locks ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx