Re: [RFC] drm/i915: Emit to ringbuffer directly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 09/09/16 14:20, Dave Gordon wrote:
On 09/09/16 09:32, Tvrtko Ursulin wrote:

On 08/09/16 17:40, Chris Wilson wrote:
On Thu, Sep 08, 2016 at 04:12:55PM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

This removes the usage of intel_ring_emit in favour of
directly writing to the ring buffer.

I have the same patch! But I called it out, for historical reasons.

Yes I know we talked about it in the past but I did not think you will
find time to actually write it amongst all the other things.

Oh, except mine uses out[0]...out[N] because gcc prefers that over
*out++ = ...

It copes just fine with the latter here, for example:

    *rbuf++ = cmd;
    *rbuf++ = I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
    *rbuf++ = 0; /* upper addr */
    *rbuf++ = 0; /* value */

Is:

     3e9:       89 10                   mov    %edx,(%rax)
     3eb:       c7 40 04 04 01 00 00    movl   $0x104,0x4(%rax)
     3f2:       c7 40 08 00 00 00 00    movl   $0x0,0x8(%rax)
     3f9:       c7 40 0c 00 00 00 00    movl   $0x0,0xc(%rax)

And for the record, before this patch, with intel_ring_emit:

     53a:       8b 53 3c                mov    0x3c(%rbx),%edx
     53d:       48 8b 4b 08             mov    0x8(%rbx),%rcx
     541:       89 04 11                mov    %eax,(%rcx,%rdx,1)

     544:       8b 43 3c                mov    0x3c(%rbx),%eax
     547:       48 8b 53 08             mov    0x8(%rbx),%rdx
     54b:       83 c0 04                add    $0x4,%eax
     54e:       89 43 3c                mov    %eax,0x3c(%rbx)
     551:       c7 04 02 04 01 00 00    movl   $0x104,(%rdx,%rax,1)

     558:       8b 43 3c                mov    0x3c(%rbx),%eax
     55b:       48 8b 53 08             mov    0x8(%rbx),%rdx
     55f:       83 c0 04                add    $0x4,%eax
     562:       89 43 3c                mov    %eax,0x3c(%rbx)
     565:       c7 04 02 00 00 00 00    movl   $0x0,(%rdx,%rax,1)

     56c:       8b 43 3c                mov    0x3c(%rbx),%eax
     56f:       48 8b 53 08             mov    0x8(%rbx),%rdx
     573:       83 c0 04                add    $0x4,%eax
     576:       89 43 3c                mov    %eax,0x3c(%rbx)
     579:       c7 04 02 00 00 00 00    movl   $0x0,(%rdx,%rax,1)

Yuck :) At least they are not function calls to iowrite any more. :)

Curious that the inlining wasn't doing a better job, though. For
example, it's not preserving %eax as a local cache of 0x3c(%rbx).

Yeah I don't know. Even by employing the restrict keyword in various ways I couldn't make it do a better job.

intel_ring_emit was preventing the compiler for optimising
fetch and increment of the current ring buffer pointer and
therefore generating very verbose code for every write.

It had no useful purpose since all ringbuffer operations
are started and ended with intel_ring_begin and
intel_ring_advance respectively, with no bail out in the
middle possible, so it is fine to increment the tail in
intel_ring_begin and let the code manage the pointer
itself.

Or you could have intel_ring_advance() take the updated local and use
that to update the ring->tail. It could then check that you hadn't
exceeded your allocation, OR that you had used exactly as much as you'd
allocated. I'm sure I had a version that did that, long ago.

Sounds good to me.

Useless instruction removal amounts to approximately
2384 bytes of saved text on my build.

Not sure if this has any measurable performance
implications but executing a ton of useless instructions
on fast paths cannot be good.

It does show up in perf.

Cool.

Patch is not fully polished, but it compiles and runs
on Gen9 at least.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_gem_context.c    |  62 ++--
  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  27 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c        |  57 ++--
  drivers/gpu/drm/i915/intel_display.c       | 113 ++++---
  drivers/gpu/drm/i915/intel_lrc.c           | 223 +++++++-------
  drivers/gpu/drm/i915/intel_mocs.c          |  43 +--
  drivers/gpu/drm/i915/intel_overlay.c       |  69 ++---
  drivers/gpu/drm/i915/intel_ringbuffer.c    | 480
+++++++++++++++--------------
  drivers/gpu/drm/i915/intel_ringbuffer.h    |  19 +-
  9 files changed, 555 insertions(+), 538 deletions(-)

Hmm, mine is bigger.

  drivers/gpu/drm/i915/i915_gem_context.c    |  85 ++--
  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  37 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c        |  62 +--
  drivers/gpu/drm/i915/i915_gem_request.c    | 135 ++++-
  drivers/gpu/drm/i915/i915_gem_request.h    |   2 +
  drivers/gpu/drm/i915/intel_display.c       | 133 +++--
  drivers/gpu/drm/i915/intel_lrc.c           | 188 ++++---
  drivers/gpu/drm/i915/intel_lrc.h           |   2 -
  drivers/gpu/drm/i915/intel_mocs.c          |  50 +-
  drivers/gpu/drm/i915/intel_overlay.c       |  77 ++-
  drivers/gpu/drm/i915/intel_ringbuffer.c    | 762
++++++++++++-----------------
  drivers/gpu/drm/i915/intel_ringbuffer.h    |  36 +-
  12 files changed, 721 insertions(+), 848 deletions(-)

(this includes moving the intel_ring_begin to i915_gem_request)

plus an ealier

  drivers/gpu/drm/i915/i915_gem_request.c |  26 ++---
  drivers/gpu/drm/i915/intel_lrc.c        | 121 ++++++++---------------
  drivers/gpu/drm/i915/intel_ringbuffer.c | 168
+++++++++++---------------------
  drivers/gpu/drm/i915/intel_ringbuffer.h |  10 +-
  4 files changed, 112 insertions(+), 213 deletions(-)

since I wanted parts of it for emitting timelines.

Ok what do you want to do?

Regards,
Tvrtko

You are giving up the possibility of wrapping over the end of the
buffer, but we avoid doing that at present and it doesn't work on some
hardware, so no great loss.

What about

     rptr = ring_begin(req, len);
     if (unlikely(IS_ERR(rptr))
         return PTR_ERR(rptr);

rather than

     ret = intel_ring_begin(req, len, &rbuf);
      if (ret)
          return ret;

(because returning values through ref parameters is ugly).

I tried that first but then just got reminded how error pointers make gcc generate more verbose code. Especially in this case where it would do ERR_PTR followed by PTR_ERR for nothing. So I thought since I am shrinking it, lets go full hog.

Do you have/do you want a Coccinelle script to automate this?

I don't and why not - more Coccinelle examples in the public domain the better. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux