From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH v2] drm/i915: use semaphores for the display plane Date: Wed, 11 Apr 2012 08:46:40 -0700 Message-ID: <20120411084640.71337575@bwidawsk.net> References: <1333662456-10976-1-git-send-email-ben@bwidawsk.net> <1334145197_364741@CP5-2952> <20120411120642.GJ4296@phenom.ffwll.local> 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 3F963A08FA for ; Wed, 11 Apr 2012 08:46:48 -0700 (PDT) In-Reply-To: <20120411120642.GJ4296@phenom.ffwll.local> 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: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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? This sounds like a pretty bad hack when it at some point in modesetting code... who would have thought? > - we update from->sync_seqno before to->sync_to successfully emits the > sync. That should happen after sync_to (and obviously only if that > succeeds). Probably a remnant of when ring_begin couldn't fail. This deserves to be fixed, I will do this today if y'all haven't done it already. > - the seqno - 1 semantics of sync_to is annoying me. Imo that kind of > low-level stuff should be handled by the sync_to implementation. If it ain't broke, don't fix it? I do agree with you, but I'll venture to guess that I am less annoyed by it. > > Unfortunately neither the bug noticed by Chris nor the sync_seqno thing > can easily be exercised with i-g-t :( > > Cheers, Daniel