From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset Date: Thu, 27 Sep 2012 15:37:46 +0300 Message-ID: <20120927123746.GL19732@intel.com> References: <1348131363-24529-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 1DCE09EB02 for ; Thu, 27 Sep 2012 05:37:59 -0700 (PDT) Content-Disposition: inline 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: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 20, 2012 at 11:17:51AM +0200, Daniel Vetter wrote: > On Thu, Sep 20, 2012 at 10:56 AM, Chris Wilson = wrote: > > We need to wait for pending operations on the CRTC to retire before we > > can modify the CRTC. For example, if userspace has queued a batch that > > uses a WAIT_FOR_EVENT associated with the current FB, we can not modify > > the pipe with that outstanding, as we may then prevent that > > WAIT_FOR_EVENT from ever completing and so hanging the GPU. (Imagine a > > scanline wait waiting for line 1024 and the pipe being adjusted to > > 600-line mode.) There is also the sequencing issue of the immediate > > update versus a pending pageflip. > > > > In both cases the function to serialise the modeset with the pending > > operations existed but was simply not being called, or called after the > > damage was already done. > > > > Signed-off-by: Chris Wilson > = > I've looked at this situation again and we do have a > wait_for_pending_flips already in per-platform crtc_disable functions, > which are called for for switching off crtcs and also only just > disabling them for a modeset. > = > So I think this finish_fb call in set_base is totally unnecessary and > can be just removed. Moving it to the crtc_set_config function doesn't > help (and this patch misses the case where we disable other crtcs than > set->crtc). This whole wait for pending flips mechanism looks iffy to me. First, when we schedule a flip, we record the fact that there is a pending flip into the old bo's pending_flip atomic mask thingy. When we later want to wait for the CRTC operations to finish, we do 'intel_finish_fb(crtc->fb)'. But notice that 'crtc->fb' is now the fb we flipped _to_, so it can't actually tell us whether there's still a flip pending. -- = Ville Syrj=E4l=E4 Intel OTC