Re: [Intel-gfx] [PATCH] drm/atomic-helper: nonblocking commit support

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Stone <daniels@collabora.com>,
	Tomeu Vizoso <tomeu.vizoso@gmail.com>
Subject: Re: [Intel-gfx] [PATCH] drm/atomic-helper: nonblocking commit support
Date: Tue, 31 May 2016 16:22:57 +0200	[thread overview]
Message-ID: <f722514a-d4e9-96d7-ad55-c662ad5a95db@linux.intel.com> (raw)
In-Reply-To: <1464595318-12234-1-git-send-email-daniel.vetter@ffwll.ch>

Op 30-05-16 om 10:01 schreef Daniel Vetter:
> Design ideas:
>
> - split up the actual commit into different phases, and have
>   completions for each of them. This will be useful for the future
>   when we want to interleave phases much more aggressively, for e.g.
>   queue depth > 1. For not it's just a minimal optimization compared
>   to current common nonblocking implementation patterns from drivers,
>   which all stall for the entire commit to complete, including vblank
>   waits and cleanups.
>
> - Extract a separate atomic_commit_hw hook since that's the part most
>   drivers will need to overwrite, hopefully allowing even more shared
>   code.
>
> - Enforce EBUSY seamntics by attaching one of the completions to the
>   flip_done vblank event. Side benefit of forcing atomic drivers using
>   these helpers to implement event handlign at least semi-correct. I'm
>   evil that way ;-)
>
> - Ridiculously modular, as usual.
>
> - The main tracking unit for a commit stays struct drm_atomic_state,
>   and the ownership rules for that are unchanged. Ownership still
>   gets transferred to the driver (and subsequently to the worker) on
>   successful commits. What is added is a small, per-crtc, refcounted
>   structure to track pending commits called struct drm_crtc_commit.
>   No actual state is attached to that though, it's purely for ordering
>   and waiting.
>
> - Dependencies are implicitly handled by assuming that any CRTC part
>   of &drm_atomic_state is a dependency, and that the current commit
>   must wait for any commits to complete on those CRTC. This way
>   drivers can easily add more depencies using
>   drm_atomic_get_crtc_state(), which is very natural since in most
>   case a dependency exists iff there's some bit of state that needs to
>   be cross checked.
>
>   Removing depencies is not possible, drivers simply need to be
>   careful to not include every CRTC in a commit if that's not
>   necessary. Which is a good idea anyway, since that also avoids
>   ww_mutex lock contention.
>
> - Queue depth > 1 sees some prep work in this patch by adding a stall
>   paramater to drm_atomic_helper_swap_states(). To be able to push
>   commits entirely free-standing and in a deeper queue through the
>   back-end the driver must not access any obj->state pointers. This
>   means we need to track the old state in drm_atomic_state (much
>   easier with the consolidated arrays), and pass them all explicitly
>   to driver backends (this will be serious amounts of churn).
>  
>   Once that's done stall can be set to false in swap_states.
>
> Features: Contains bugs because totally untested.
>
> v2: Dont ask for flip_done signalling when the CRTC is off and stays
> off: Drivers don't handle events in that case. Instead complete right
> away. This way future commits don't need to have special-case logic,
> but can keep blocking for the flip_done completion.
>
> v3: Tons of fixes:
> - Stall for preceeding commit for real, not the current one by
>   accident.
> - Add WARN_ON in case drivers don't fire the drm event.
> - Don't double-free drm events.
>
> v4: Make legacy cursor not stall.
>
> v5: Extend the helper hook to cover the entire commit tail. Some
> drivers need special code for cleanup and vblank waiting, this makes
> it a bit more useful. Inspired by the rockchip driver.
>
> v6: Add WARN_ON to catch drivers who forget to send out the
> drm event.
>
> v7: Fixup the stalls in swap_state for real!!
I don't think stalling belongs to swap_state, that should be a separate helper call.

When nonblocking = false the waiting is still performed uninterruptibly. I believe that this is an error,
and all blocking waiting should be completed before calling swap_state to ensure -EINTR can be
propagated correctly as much as possible. Perhaps also return -EBUSY if wait times out.

You specified a timeout of 10 HZ, why is that? If we wait interruptibly then there's no need for a timeout.
I also think the timeout may be too short, if we commit 3 crtc's it leaves 3.3 seconds for each. In case
there's a modeset enable and disable, that leaves 1.6 seconds for each enable/disable. Might be too short..

Patch is also a bit hard to review with so many lines changed, could this be done in pieces instead of all at once?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel