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@xxxxxxxxx wrote:
> >>>>From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >>>>
> >>>>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@xxxxxxxx>
> >>>>Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> >>>>---
> >>>> 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@xxxxxxxxx>
Queued for -next.t
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
