From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 2/2] drm/i915: use semaphores for the display plane Date: Thu, 22 Mar 2012 09:07:09 -0700 Message-ID: <20120322090709.59f3f5f2@bwidawsk.net> References: <1332375553-7174-1-git-send-email-ben@bwidawsk.net> <1332375553-7174-3-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 8CD869EB24 for ; Thu, 22 Mar 2012 09:07:20 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 22 Mar 2012 09:56:07 +0000 Chris Wilson wrote: > On Wed, 21 Mar 2012 17:19:13 -0700, Ben Widawsky > 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 > > --- > > 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 >