Re: [PATCH 08/27] drm/atomic: Add struct drm_crtc_commit to track async updates

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxx>
> Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> Tested-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux