Re: [Intel-gfx] [PATCH 12/26] drm/atomic-helper: Massage swap_state signature somewhat

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 12/26] drm/atomic-helper: Massage swap_state signature somewhat
Date: Tue, 31 May 2016 12:46:37 +0200	[thread overview]
Message-ID: <20160531104637.GB30560@phenom.ffwll.local> (raw)
In-Reply-To: <a81bf0cc-9140-5fdd-5382-c40949222d7a@linux.intel.com>

On Tue, May 31, 2016 at 10:43:56AM +0200, Maarten Lankhorst wrote:
> Op 30-05-16 om 17:09 schreef Daniel Vetter:
> > On Mon, May 30, 2016 at 03:08:39PM +0200, Maarten Lankhorst wrote:
> >> Op 29-05-16 om 20:35 schreef Daniel Vetter:
> >>> - dev is redundant, we have state->atomic
> >>> - add stall parameter, which must be set when swapping needs to stall
> >>>   for preceeding commits to stop looking at ->state pointers. Currently
> >>>   all drivers need this to be, just prep work for a glorious future.
> >> I have to disagree, if you want to stall it should be done in a separate helper before swapping state.
> >> That function should also have the ability to fail, since we haven't called swap_state yet. :)
> >>
> >> Maybe with nonblock as parameter, so it could fail with -EBUSY if it would stall, and -EINTR if interrupted?
> > This is not the stalling you're thinking of. The EBUSY check is in
> > drm_atomic_helper_setup_commit (and as such entirely optional).
> >
> > And if you don't ever use the nonblocking helpers, no struct
> > drm_crtc_commit will ever show up in crtc->commit_list, which means this
> > here also doesn't do anything.
> >
> > This stall here is just to make sure that the previous commit doesn't get
> > confused because we've ripped out crtc/plane/connector->state from
> > underneath it. And as soon as we add previous_state pointers to
> > drm_atomic_state and wire it up through all the hooks we can set
> > stall=false here, since then pipelining with a queue depth > 1 is
> > possible.
> If a driver supports depth > 1 it could skip the extra help waiter and just call swap_state.
> The helper should return -EINTR when needed.
> 
> if (nonblock) {
> ret = drm_atomic_helper_wait_for_completion(state);
> if (ret)
> return ret;
> }
> 
> drm_atomic_helper_swap_state..

That's why there's this stall parameter ... Also,
wait_for_completion(state) is not enough, there's 3 different completions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel