[Intel-gfx] [PATCH] drm/i915: Reserve space improvements

[Daniel Vetter]

On Wed, Jul 01, 2015 at 11:44:07AM +0100, Tomas Elf wrote:
> On 30/06/2015 17:15, Tomas Elf wrote:
> >On 30/06/2015 16:53, John Harrison wrote:
> >>On 30/06/2015 15:43, Tomas Elf wrote:
> >>>On 30/06/2015 12:40, John.C.Harrison at Intel.com wrote:
> >>>>From: John Harrison <John.C.Harrison at Intel.com>
> >>>>
> >>>>An earlier patch was added to reserve space in the ring buffer for the
> >>>>commands issued during 'add_request()'. The initial version was
> >>>>pessimistic in the way it handled buffer wrapping and would cause
> >>>>premature wraps and thus waste ring space.
> >>>>
> >>>>This patch updates the code to better handle the wrap case. It no
> >>>>longer enforces that the space being asked for and the reserved space
> >>>>are a single contiguous block. Instead, it allows the reserve to be on
> >>>>the far end of a wrap operation. It still guarantees that the space is
> >>>>available so when the wrap occurs, no wait will happen. Thus the wrap
> >>>>cannot fail which is the whole point of the exercise.
> >>>>
> >>>>Also fixed a merge failure with some comments from the original patch.
> >>>>
> >>>>v2: Incorporated suggestion by David Gordon to move the wrap code
> >>>>inside the prepare function and thus allow a single combined
> >>>>wait_for_space() call rather than doing one before the wrap and
> >>>>another after. This also makes the prepare code much simpler and
> >>>>easier to follow.
> >>>>
> >>>>v3: Fix for 'effective_size' vs 'size' during ring buffer remainder
> >>>>calculations (spotted by Tomas Elf).
> >>>>
> >>>>For: VIZ-5115
> >>>>CC: Daniel Vetter <daniel at ffwll.ch>
> >>>>Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/i915/intel_lrc.c        | 73
> >>>>+++++++++++++-------------
> >>>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 90
> >>>>+++++++++++++++++++--------------
> >>>>  drivers/gpu/drm/i915/intel_ringbuffer.h |  4 +-
> >>>>  3 files changed, 90 insertions(+), 77 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>index b36e064..7d9515d 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>@@ -663,12 +663,12 @@ static int logical_ring_wait_for_space(struct
> >>>>drm_i915_gem_request *req,
> >>>>      unsigned space;
> >>>>      int ret;
> >>>>
> >>>>-    /* The whole point of reserving space is to not wait! */
> >>>>-    WARN_ON(ringbuf->reserved_in_use);
> >>>>-
> >>>>      if (intel_ring_space(ringbuf) >= bytes)
> >>>>          return 0;
> >>>>
> >>>>+    /* The whole point of reserving space is to not wait! */
> >>>>+    WARN_ON(ringbuf->reserved_in_use);
> >>>>+
> >>>>      list_for_each_entry(target, &ring->request_list, list) {
> >>>>          /*
> >>>>           * The request queue is per-engine, so can contain requests
> >>>>@@ -718,22 +718,11 @@ intel_logical_ring_advance_and_submit(struct
> >>>>drm_i915_gem_request *request)
> >>>>      execlists_context_queue(request);
> >>>>  }
> >>>>
> >>>>-static int logical_ring_wrap_buffer(struct drm_i915_gem_request *req)
> >>>>+static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
> >>>>  {
> >>>>-    struct intel_ringbuffer *ringbuf = req->ringbuf;
> >>>>      uint32_t __iomem *virt;
> >>>>      int rem = ringbuf->size - ringbuf->tail;
> >>>>
> >>>>-    /* Can't wrap if space has already been reserved! */
> >>>>-    WARN_ON(ringbuf->reserved_in_use);
> >>>>-
> >>>>-    if (ringbuf->space < rem) {
> >>>>-        int ret = logical_ring_wait_for_space(req, rem);
> >>>>-
> >>>>-        if (ret)
> >>>>-            return ret;
> >>>>-    }
> >>>>-
> >>>>      virt = ringbuf->virtual_start + ringbuf->tail;
> >>>>      rem /= 4;
> >>>>      while (rem--)
> >>>>@@ -741,40 +730,50 @@ static int logical_ring_wrap_buffer(struct
> >>>>drm_i915_gem_request *req)
> >>>>
> >>>>      ringbuf->tail = 0;
> >>>>      intel_ring_update_space(ringbuf);
> >>>>-
> >>>>-    return 0;
> >>>>  }
> >>>>
> >>>>  static int logical_ring_prepare(struct drm_i915_gem_request *req,
> >>>>int bytes)
> >>>>  {
> >>>>      struct intel_ringbuffer *ringbuf = req->ringbuf;
> >>>>-    int ret;
> >>>>-
> >>>>-    /*
> >>>>-     * Add on the reserved size to the request to make sure that after
> >>>>-     * the intended commands have been emitted, there is guaranteed to
> >>>>-     * still be enough free space to send them to the hardware.
> >>>>-     */
> >>>>-    if (!ringbuf->reserved_in_use)
> >>>>-        bytes += ringbuf->reserved_size;
> >>>>-
> >>>>-    if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> >>>>-        ret = logical_ring_wrap_buffer(req);
> >>>>-        if (unlikely(ret))
> >>>>-            return ret;
> >>>>+    int remain_usable = ringbuf->effective_size - ringbuf->tail;
> >>>>+    int remain_actual = ringbuf->size - ringbuf->tail;
> >>>
> >>>You could add a comment here (and in the legacy implementation)
> >>>explaining why we make the distinction between remain_usable and
> >>>remain_actual. Or you could add the comment when we actually use
> >>>remain_actual further down in the function. It's up to you.
> >>>
> >>>We first need to be pessimistic about how much space is left in the
> >>>ring buffer when determining the need for wrapping - therefore we use
> >>>remain_usable, which disregards the end-of-buffer padding - and then
> >>>we need to be pessimistic about how much ring space we need to wait
> >>>for - therefore we use remain_actual, which takes the end-of-buffer
> >>>padding into consideration to make sure that we're actually not
> >>>waiting for too little space.
> >>
> >>It's not about being pessimistic or optimistic. That implies there is
> >>some choice, that one version is a little bit better in one situation
> >>but either would do. Whereas this is about functional correctness. The
> >>difference between the actual ring size and the usable ring size is not
> >>some reserved space thing to make something go faster. This is about the
> >>hardware locking up if the entire buffer is used. I think 'usable' and
> >>'actual' are fairly obvious names. If you want to know the details about
> >>why there is an 'effective_size' in the first place then look up
> >>'effective_size' in the code and read the comment about i830 hangs.
> >
> >You're probably right. Let's just forget about it.
> >
> >>
> >>>If you add those comments maybe you could also rename the variables to
> >>>something like "remaining_space_usable" etc. since "remain_usable" is
> >>>more of a verb than a noun. But that's seriously nitpicking.
> >>Or maybe
> >>'remaining_space_that_is_usable_without_causing_an_i830_to_hang_because_it_skips_the_last_two_cachelines'?
> >>
> >>There is such a thing as being too verbose and making the code hard to
> >>read.
> >
> >That is indeed a very long variable name. It's 104 characters long so
> >that wouldn't clear checkpatch.pl :). My nitpicky suggestion was 9
> >characters longer. And, yeah, it's possible to get too verbose. This
> >driver is not even remotely close to that point as it stands today :).
> >
> >Thanks,
> >Tomas
> 
> Anyway, I think we're done here.
> 
> Reviewed-by: Tomas Elf <tomas.elf at intel.com>

Queued for -next.t
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch