From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH v2] drm/i915: use semaphores for the display plane Date: Wed, 11 Apr 2012 16:52:33 +0100 Message-ID: <1334159558_368607@CP5-2952> References: <1333662456-10976-1-git-send-email-ben@bwidawsk.net> <1334145197_364741@CP5-2952> <20120411120642.GJ4296@phenom.ffwll.local> <20120411084640.71337575@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (smtp.fireflyinternet.com [109.228.6.236]) by gabe.freedesktop.org (Postfix) with ESMTP id 52F989E7B1 for ; Wed, 11 Apr 2012 08:52:43 -0700 (PDT) In-Reply-To: <20120411084640.71337575@bwidawsk.net> 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: Ben Widawsky , Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, 11 Apr 2012 08:46:40 -0700, Ben Widawsky wrote: > On Wed, 11 Apr 2012 14:06:42 +0200 > Daniel Vetter wrote: > > > On Wed, Apr 11, 2012 at 12:53:15PM +0100, Chris Wilson wrote: > > > On Thu, 5 Apr 2012 14:47:36 -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 (and the ring can > > > > even go to sleep if the HW supports it). > > > > > > > > v2: > > > > squash 2 patches into 1 (me) > > > > un-inline the enable_semaphores function (Daniel) > > > > remove comment about SNB hangs from i915_gem_object_sync (Chris) > > > > rename intel_enable_semaphores to i915_semaphore_is_enabled (me) > > > > removed page flip comment; "no why" (Chris) > > > > > > > > To address other comments from Daniel (irc): > > > > update the comment to say 'vt-d is crap, don't enable semaphores' > > > > - I think you misinterpreted Chris' comment, it already exists. > > > > checking out whether we can pageflip on the render ring on ivb (didn't > > > > work on early silicon) > > > > - We don't want to enable workarounds for early silicon unless we have > > > > to. > > > > - I can't find any references in the docs about this. > > > > optionally use it if the fb is already busy on the render ring > > > > - This should be how the code already worked, unless I am > > > > misunderstanding your meaning. > > > > > > > > CC: Chris Wilson > > > > Signed-off-by: Ben Widawsky > > > > > > > +int > > > > +i915_gem_object_sync(struct drm_i915_gem_object *obj, > > > > + struct intel_ring_buffer *to) > > > > +{ > > > > + struct intel_ring_buffer *from = obj->ring; > > > > + u32 seqno; > > > > + int ret, idx; > > > > + > > > > + if (from == NULL || to == from) > > > > + return 0; > > > > + > > > > + if (!i915_semaphore_is_enabled(obj->base.dev)) > > > Bug^ :( > > > > To elaborate, for to == NULL we need to do a synchronous wait_rendering, > > too. This happens for set_base and modeset. Furthermore I've noticed two > > other things while reading this function that imo deserve each another > > patch: > > if (from == NULL && !obj->active) should suffice? if (to == NULL || !i915_sempahores) return i915_gem_object_wait_rendering(obj); -Chris -- Chris Wilson, Intel Open Source Technology Centre