Re: [PATCH 2/2] drm/i915: use semaphores for the display plane

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, 22 Mar 2012 09:56:07 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, 21 Mar 2012 17:19:13 -0700, Ben Widawsky <ben@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@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
>