From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP Date: Fri, 14 Sep 2012 19:07:15 +0300 Message-ID: <20120914160715.GI19732@intel.com> References: <1347464827-14187-1-git-send-email-ville.syrjala@linux.intel.com> <1347464827-14187-5-git-send-email-ville.syrjala@linux.intel.com> <84c8a8$5o3vcu@orsmga001.jf.intel.com> <20120914142130.GE19732@intel.com> <84c8a8$5o4ac7@orsmga001.jf.intel.com> <20120914153021.GF19732@intel.com> <84c8a8$5o5d5l@orsmga001.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id B39709F5E8 for ; Fri, 14 Sep 2012 09:07:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <84c8a8$5o5d5l@orsmga001.jf.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Sep 14, 2012 at 04:56:00PM +0100, Chris Wilson wrote: > On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrj=E4l=E4 wrote: > > On Fri, Sep 14, 2012 at 03:27:05PM +0100, Chris Wilson wrote: > > > On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrj=E4l=E4 wrote: > > > > On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote: > > > > > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com= wrote: > > > > > > +static void intel_flip_finish(struct drm_flip *flip) > > > > > > +{ > > > > > > + struct intel_flip *intel_flip =3D > > > > > > + container_of(flip, struct intel_flip, base); > > > > > > + struct drm_device *dev =3D intel_flip->crtc->dev; > > > > > > + > > > > > > + if (intel_flip->old_bo) { > > > > > > + mutex_lock(&dev->struct_mutex); > > > > > > + > > > > > > + intel_finish_fb(intel_flip->old_bo); > > > > > = > > > > > So if I understand correctly, this code is called after the flip = is > > > > > already complete? > > > > = > > > > Yes. > > > > = > > > > > The intel_finish_fb() exists to flush pending batches and flips o= n the > > > > > current fb, prior to changing the scanout registers. (There is a > > > > > hardware dependency such that the GPU may be executing a command = that > > > > > required the current modesetting.) In the case of flip completion= , all > > > > > of those dependencies have already been retired and so the finish= should > > > > > be a no-op. And so it should no be required, nor the changes to > > > > > intel_finish_fb (which should have included a change in the name = to > > > > > indicate that is now taking the fb_obj). > > > > = > > > > Actually I'm not quite sure where this intel_finish_fb() call origi= nated. > > > > Based on the name it didn't make sense to me, but I left it there f= or > > > > now. Hmm. OK it came from one patch from Imre while I was on vacati= on. > > > > I suppose he got it from intel_pipe_set_base() which does call > > > > intel_finish_fb() on the old fb. Why does it do that? > > > = > > > It all boils down to the modeset being asynchronous to the GPU > > > processing the command stream. So we may be currently processing a ba= tch > > > that is waiting on the pipe to go past a particular scanline, and if = the > > > modesetting were to disable that pipe, or to change its size, then we > > > risk the WAIT_FOR_EVENT never completing - leading to hangcheck > > > detecting the frozen display and an angry user. > > = > > intel_pipe_set_base() won't disable the pipe or change the size, > > it'll just flip the primary plane. So that doesn't quite explain > > why the call is there, as opposed to being called just from the > > full modeset path. > = > Hmm, at the time it was a convenient point. Now, it is clearly called too > late in the modeset sequence. Daniel, fix please. :) > = > > Also wouldn't any batch buffer with WAIT_FOR_EVENT be in risk of > > stalling, not just ones related to the old fb? > > = > > > The other aspect is to synchronize the modeset with any outstanding > > > pageflips. > > = > > Right, that does make sense. But doing it from a function called > > intel_finish_fb() is a bit confusing, as the condition really > > shouldn't depend on any specific fb object. But I suppose this is > > just a result of the "only one outstanding flip" policy. > = > Again, a nice convenient point, calling it an intel_crtc_wait_*() would > probably help (after fixing the ordering). > = > > BTW regarding this WAIT_FOR_EVENT thing. I got the impression that > > the scanline window wait doesn't work on recent hardware generations > > any more. Is that true? I was thinking that perhaps I could use it > > along with the load register command to perform the flips through > > the command queue. > = > That impression is pretty accurate. There is a suggestion that some form > of scanline wait was restored for IVB, but driving it seems pretty hit > and miss. Atomic flipping should all be possible with MI_DISPLAY_FLIP, > so presumably you are mostly thinking about atomic modeset? Is the > presumption that it will be an infrequent request and so better to keep > as simple as possible? MI_DISPLAY_FLIP is _very_ limited. You can't really do anything interesting with. Sure it's enough to flip the buffers of some game or whatever, but it's practically useless for hardware composition type stuff. -- = Ville Syrj=E4l=E4 Intel OTC