On Wed, Jun 08, 2016 at 02:19:00PM +0200, Daniel Vetter wrote:
> Split out from my big nonblocking atomic commit helper code as prep
> work. While add it, also add some neat asciiart to document how it's
> supposed to be used.
>
> v2: Resurrect misplaced hunk in the kerneldoc.
>
> Tested-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Tomeu Vizoso <tomeu.vizoso at gmail.com>
> Cc: Daniel Stone <daniels at collabora.com>
> Tested-by: Liviu Dudau <Liviu.Dudau at arm.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 22 +++++++
> drivers/gpu/drm/drm_crtc.c | 3 +
> drivers/gpu/drm/drm_fops.c | 6 ++
> include/drm/drmP.h | 1 +
> include/drm/drm_atomic.h | 6 ++
> include/drm/drm_crtc.h | 149
> +++++++++++++++++++++++++++++++++++++++++--
> 6 files changed, 181 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5e4b820a977c..d99ab2f6663f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -33,6 +33,20 @@
>
> #include "drm_crtc_internal.h"
>
> +static void crtc_commit_free(struct kref *kref)
> +{
> + struct drm_crtc_commit *commit =
> + container_of(kref, struct drm_crtc_commit, ref);
> +
> + kfree(commit);
> +}
> +
> +void drm_crtc_commit_put(struct drm_crtc_commit *commit)
> +{
> + kref_put(&commit->ref, crtc_commit_free);
> +}
> +EXPORT_SYMBOL(drm_crtc_commit_put);
> +
> /**
> * drm_atomic_state_default_release -
> * release memory initialized by drm_atomic_state_init
> @@ -148,6 +162,14 @@ void drm_atomic_state_default_clear(struct
> drm_atomic_state *state)
>
> crtc->funcs->atomic_destroy_state(crtc,
> state->crtcs[i].state);
> +
> + if (state->crtcs[i].commit) {
> + kfree(state->crtcs[i].commit->event);
> + state->crtcs[i].commit->event = NULL;
> + drm_crtc_commit_put(state->crtcs[i].commit);
> + }
> +
> + state->crtcs[i].commit = NULL;
> state->crtcs[i].ptr = NULL;
> state->crtcs[i].state = NULL;
> }
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d3d0b4162e76..ce0569c3f942 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -669,6 +669,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
> struct drm_crtc *crtc,
> crtc->dev = dev;
> crtc->funcs = funcs;
>
> + INIT_LIST_HEAD(&crtc->commit_list);
> + spin_lock_init(&crtc->commit_lock);
> +
> drm_modeset_lock_init(&crtc->mutex);
> ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
> if (ret)
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 44b3ecdeca63..323c238fcac7 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -686,6 +686,12 @@ void drm_send_event_locked(struct drm_device *dev,
> struct drm_pending_event *e)
> {
> assert_spin_locked(&dev->event_lock);
>
> + if (e->completion) {
> + /* ->completion might disappear as soon as it signalled. */
> + complete_all(e->completion);
> + e->completion = NULL;
> + }
> +
> if (e->fence) {
> fence_signal(e->fence);
> fence_put(e->fence);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 15fe4a21a9da..dce655abd23d 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -283,6 +283,7 @@ struct drm_ioctl_desc {
>
> /* Event queued up for userspace to read */
> struct drm_pending_event {
> + struct completion *completion;
> struct drm_event *event;
> struct fence *fence;
> struct list_head link;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index a16861c882aa..856a9c85a838 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -30,6 +30,12 @@
>
> #include <drm/drm_crtc.h>
>
> +void drm_crtc_commit_put(struct drm_crtc_commit *commit);
> +static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
> +{
> + kref_get(&commit->ref);
> +}
> +
> struct drm_atomic_state * __must_check
> drm_atomic_state_alloc(struct drm_device *dev);
> void drm_atomic_state_clear(struct drm_atomic_state *state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 520abafc9d00..14aa8212e30f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -727,9 +727,6 @@ struct drm_crtc_funcs {
> * @gamma_store: gamma ramp values
> * @helper_private: mid-layer private data
> * @properties: property tracking for this CRTC
> - * @state: current atomic state for this CRTC
> - * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
> - * legacy IOCTLs
> *
> * Each CRTC may have one or more connectors associated with it. This
> structure
> * allows the CRTC to be controlled.
> @@ -786,11 +783,37 @@ struct drm_crtc {
>
> struct drm_object_properties properties;
>
> + /**
> + * @state:
> + *
> + * Current atomic state for this CRTC.
> + */
> struct drm_crtc_state *state;
>
> - /*
> - * For legacy crtc IOCTLs so that atomic drivers can get at the locking
> - * acquire context.
> + /**
> + * @commit_list:
> + *
> + * List of &drm_crtc_commit structures tracking pending commits.
> + * Protected by @commit_lock. This list doesn't hold its own full
> + * reference, but burrows it from the ongoing commit. Commit entries
> + * must be removed from this list once the commit is fully completed,
> + * but before it's correspoding &drm_atomic_state gets destroyed.
> + */
> + struct list_head commit_list;
> +
> + /**
> + * @commit_lock:
> + *
> + * Spinlock to protect @commit_list.
> + */
> + spinlock_t commit_lock;
> +
> + /**
> + * @acquire_ctx:
> + *
> + * Per-CRTC implicit acquire context used by atomic drivers for legacy
> + * IOCTLs, so that atomic drivers can get at the locking acquire
> + * context.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
> };
> @@ -1732,6 +1755,111 @@ struct drm_bridge {
> void *driver_private;
> };
>
> +/**
> + * struct drm_crtc_commit - track modeset commits on a CRTC
> + *
> + * This structure is used to track pending modeset changes and atomic commit
> on
> + * a per-CRTC basis. Since updating the list should never block this
> structure
> + * is reference counted to allow waiters to safely wait on an event to
> complete,
> + * without holding any locks.
> + *
> + * It has 3 different events in total to allow a fine-grained
> synchronoization
spelling police: synchronization
> + * between outstanding updates::
> + *
> + * atomic commit thread hardware
> + *
> + * write new state into hardware ----> ...
> + * signal hw_done
> + * switch to new state on next
> + * ... v/hblank
> + *
> + * wait for buffers to show up ...
> + *
> + * ... send completion irq
> + * irq handler signals flip_done
> + * cleanup old buffers
> + *
> + * signal cleanup_done
> + *
> + * wait for flip_done <----
> + * clean up atomic state
> + *
> + * The important bit to know is that cleanup_done is the terminal event, but
> the
> + * ordering between flip_done and hw_done is entirely up to the specific
> driver
> + * and modeset state change.
> + *
> + * For an implementation of how to use this look at
> + * drm_atomic_helper_setup_commit() from the atomic helper library.
> + */
> +struct drm_crtc_commit {
> + /**
> + * @crtc:
> + *
> + * DRM CRTC for this commit.
> + */
> + struct drm_crtc *crtc;
> +
> + /**
> + * @ref:
> + *
> + * Reference count for this structure. Needed to allow blocking on
> + * completions without the risk of the completion disappearing
> + * meanwhile.
> + */
> + struct kref ref;
> +
> + /**
> + * @flip_done:
> + *
> + * Will be signaled when the hardware has flipped to the new set of
> + * buffers. Signals at the same time as when the drm event for this
> + * commit is sent to userspace, or when an out-fence is singalled. Note
> + * that for most hardware, in most cases this happens after @hw_done is
> + * signalled.
> + */
> + struct completion flip_done;
> +
> + /**
> + * @hw_done:
> + *
> + * Will be signalled when all hw register changes for this commit have
> + * been written out. Especially when disabling a pipe this can be much
> + * later than than @flip_done, since that can signal already when the
> + * screen goes black, whereas to fully shut down a pipe more register
> + * I/O is required.
> + *
> + * Note that this does not need to include separately reference-counted
> + * resources like backing storage buffer pinning, or runtime pm
> + * management.
> + */
> + struct completion hw_done;
> +
> + /**
> + * @cleanup_done:
> + *
> + * Will be signalled after old buffers have been cleaned up again by
what do you mean by "cleaned up *again*" ? If you are trying to emphasize the
following sentence, I think you can drop "again" from it, but again, I'm only
a software developer :)
Best regards,
Liviu
> + * calling drm_atomic_helper_cleanup_planes(). Since this can only
> + * happen after a vblank wait completed it might be a bit later. This
> + * completion is useful to throttle updates and avoid hardware updates
> + * getting ahead of the buffer cleanup too much.
> + */
> + struct completion cleanup_done;
> +
> + /**
> + * @commit_entry:
> + *
> + * Entry on the per-CRTC commit_list. Protected by crtc->commit_lock.
> + */
> + struct list_head commit_entry;
> +
> + /**
> + * @event:
> + *
> + * &drm_pending_vblank_event pointer to clean up private events.
> + */
> + struct drm_pending_vblank_event *event;
> +};
> +
> struct __drm_planes_state {
> struct drm_plane *ptr;
> struct drm_plane_state *state;
> @@ -1740,6 +1868,7 @@ struct __drm_planes_state {
> struct __drm_crtcs_state {
> struct drm_crtc *ptr;
> struct drm_crtc_state *state;
> + struct drm_crtc_commit *commit;
> };
>
> struct __drm_connnectors_state {
> @@ -1770,6 +1899,14 @@ struct drm_atomic_state {
> struct __drm_connnectors_state *connectors;
>
> struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> + /**
> + * @commit_work:
> + *
> + * Work item which can be used by the driver or helpers to execute the
> + * commit without blocking.
> + */
> + struct work_struct commit_work;
> };
>
>
> --
> 2.8.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ã)_/¯
