On Thu, 22 Mar 2012 09:56:07 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, 21 Mar 2012 17:19:13 -0700, Ben Widawsky <ben at bwidawsk.net>
> wrote:
> > In theory this will have performance and power improvements.
> > Performance because we don't need to stall when the scanout BO is
> > busy, and power because we don't have to stall when the BO is busy
> > ie. we can get the work done sooner and put the CPU to sleep (and
> > one less interrupt required).
> >
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index ce2fee5..96ad162 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3041,9 +3041,7 @@ int i915_gem_object_set_cache_level(struct
> > drm_i915_gem_object *obj,
> > * any flushes to be pipelined (for pageflips).
> > *
> > * For the display plane, we want to be in the GTT but out of any
> > write
> > - * domains. So in many ways this looks like set_to_gtt_domain()
> > apart from the
> > - * ability to pipeline the waits, pinning and any additional
> > subtleties
> > - * that may differentiate the display plane from ordinary buffers.
> > + * domains. So in many ways this looks like set_to_gtt_domain().
> ...apart from the whole pinning and pipelining. It looks less like
> set-to-gtt-domain over time.
>
> Not an improvement. I'll be the first to admit that it is not a great
> comment, but at least it does try to capture why we don't just treat
> the display plane as GTT. A better comment would explain our concept
> of display plane and pipelining.
Separate patch though, don't you think? And it is now pipe-lining the
waits, so I'm confused why you don't think it's an improvement.
>
> > */
> > int
> > i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object
> > *obj, @@ -3058,8 +3056,8 @@
> > i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object
> > *obj, return ret;
> > if (pipelined != obj->ring) {
> > - ret = i915_gem_object_wait_rendering(obj);
> > - if (ret == -ERESTARTSYS)
> > + ret = i915_gem_object_sync(obj, pipelined);
> > + if (ret)
> > return ret;
> > }
>
> This looks eerily familiar.
Yes. Danvet enlightened me that you'd done these patches before, but he
said they no longer applied and I couldn't find them in my mail
anywhere so I wasn't sure how similar it was or wasn't. Assuming I redo
this one, I'll be sure to add a note in the commit.
> -Chris
>
