Re: [PATCH 04/10] drm/syncobj: remove drm_syncobj_cb and cleanup

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Hi Daniel,

can I get a review for this one? It is essentially just a follow up 
cleanup on one of your patches and shouldn't have any functional effect.

Thanks,
Christian.

Am 04.12.18 um 12:59 schrieb Christian König:
> This completes "drm/syncobj: Drop add/remove_callback from driver
> interface" and cleans up the implementation a bit.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 91 ++++++++++++++-----------------------------
>   include/drm/drm_syncobj.h     | 21 ----------
>   2 files changed, 30 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index db30a0e89db8..e19525af0cce 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -56,6 +56,16 @@
>   #include "drm_internal.h"
>   #include <drm/drm_syncobj.h>
>   
> +struct syncobj_wait_entry {
> +	struct list_head node;
> +	struct task_struct *task;
> +	struct dma_fence *fence;
> +	struct dma_fence_cb fence_cb;
> +};
> +
> +static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
> +				      struct syncobj_wait_entry *wait);
> +
>   /**
>    * drm_syncobj_find - lookup and reference a sync object.
>    * @file_private: drm file private pointer
> @@ -82,58 +92,33 @@ struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>   }
>   EXPORT_SYMBOL(drm_syncobj_find);
>   
> -static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
> -					    struct drm_syncobj_cb *cb,
> -					    drm_syncobj_func_t func)
> +static void drm_syncobj_fence_add_wait(struct drm_syncobj *syncobj,
> +				       struct syncobj_wait_entry *wait)
>   {
> -	cb->func = func;
> -	list_add_tail(&cb->node, &syncobj->cb_list);
> -}
> -
> -static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> -						 struct dma_fence **fence,
> -						 struct drm_syncobj_cb *cb,
> -						 drm_syncobj_func_t func)
> -{
> -	int ret;
> -
> -	*fence = drm_syncobj_fence_get(syncobj);
> -	if (*fence)
> -		return 1;
> +	if (wait->fence)
> +		return;
>   
>   	spin_lock(&syncobj->lock);
>   	/* We've already tried once to get a fence and failed.  Now that we
>   	 * have the lock, try one more time just to be sure we don't add a
>   	 * callback when a fence has already been set.
>   	 */
> -	if (syncobj->fence) {
> -		*fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> -								 lockdep_is_held(&syncobj->lock)));
> -		ret = 1;
> -	} else {
> -		*fence = NULL;
> -		drm_syncobj_add_callback_locked(syncobj, cb, func);
> -		ret = 0;
> -	}
> +	if (syncobj->fence)
> +		wait->fence = dma_fence_get(
> +			rcu_dereference_protected(syncobj->fence, 1));
> +	else
> +		list_add_tail(&wait->node, &syncobj->cb_list);
>   	spin_unlock(&syncobj->lock);
> -
> -	return ret;
>   }
>   
> -void drm_syncobj_add_callback(struct drm_syncobj *syncobj,
> -			      struct drm_syncobj_cb *cb,
> -			      drm_syncobj_func_t func)
> +static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj,
> +				    struct syncobj_wait_entry *wait)
>   {
> -	spin_lock(&syncobj->lock);
> -	drm_syncobj_add_callback_locked(syncobj, cb, func);
> -	spin_unlock(&syncobj->lock);
> -}
> +	if (!wait->node.next)
> +		return;
>   
> -void drm_syncobj_remove_callback(struct drm_syncobj *syncobj,
> -				 struct drm_syncobj_cb *cb)
> -{
>   	spin_lock(&syncobj->lock);
> -	list_del_init(&cb->node);
> +	list_del_init(&wait->node);
>   	spin_unlock(&syncobj->lock);
>   }
>   
> @@ -148,7 +133,7 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   			       struct dma_fence *fence)
>   {
>   	struct dma_fence *old_fence;
> -	struct drm_syncobj_cb *cur, *tmp;
> +	struct syncobj_wait_entry *cur, *tmp;
>   
>   	if (fence)
>   		dma_fence_get(fence);
> @@ -162,7 +147,7 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   	if (fence != old_fence) {
>   		list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>   			list_del_init(&cur->node);
> -			cur->func(syncobj, cur);
> +			syncobj_wait_syncobj_func(syncobj, cur);
>   		}
>   	}
>   
> @@ -608,13 +593,6 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>   					&args->handle);
>   }
>   
> -struct syncobj_wait_entry {
> -	struct task_struct *task;
> -	struct dma_fence *fence;
> -	struct dma_fence_cb fence_cb;
> -	struct drm_syncobj_cb syncobj_cb;
> -};
> -
>   static void syncobj_wait_fence_func(struct dma_fence *fence,
>   				    struct dma_fence_cb *cb)
>   {
> @@ -625,11 +603,8 @@ static void syncobj_wait_fence_func(struct dma_fence *fence,
>   }
>   
>   static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
> -				      struct drm_syncobj_cb *cb)
> +				      struct syncobj_wait_entry *wait)
>   {
> -	struct syncobj_wait_entry *wait =
> -		container_of(cb, struct syncobj_wait_entry, syncobj_cb);
> -
>   	/* This happens inside the syncobj lock */
>   	wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
>   							      lockdep_is_held(&syncobj->lock)));
> @@ -688,12 +663,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>   	 */
>   
>   	if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> -		for (i = 0; i < count; ++i) {
> -			drm_syncobj_fence_get_or_add_callback(syncobjs[i],
> -							      &entries[i].fence,
> -							      &entries[i].syncobj_cb,
> -							      syncobj_wait_syncobj_func);
> -		}
> +		for (i = 0; i < count; ++i)
> +			drm_syncobj_fence_add_wait(syncobjs[i], &entries[i]);
>   	}
>   
>   	do {
> @@ -742,9 +713,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>   
>   cleanup_entries:
>   	for (i = 0; i < count; ++i) {
> -		if (entries[i].syncobj_cb.func)
> -			drm_syncobj_remove_callback(syncobjs[i],
> -						    &entries[i].syncobj_cb);
> +		drm_syncobj_remove_wait(syncobjs[i], &entries[i]);
>   		if (entries[i].fence_cb.func)
>   			dma_fence_remove_callback(entries[i].fence,
>   						  &entries[i].fence_cb);
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index b1fe921f8e8f..7c6ed845c70d 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -28,8 +28,6 @@
>   
>   #include "linux/dma-fence.h"
>   
> -struct drm_syncobj_cb;
> -
>   /**
>    * struct drm_syncobj - sync object.
>    *
> @@ -62,25 +60,6 @@ struct drm_syncobj {
>   	struct file *file;
>   };
>   
> -typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
> -				   struct drm_syncobj_cb *cb);
> -
> -/**
> - * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
> - * @node: used by drm_syncob_add_callback to append this struct to
> - *	  &drm_syncobj.cb_list
> - * @func: drm_syncobj_func_t to call
> - *
> - * This struct will be initialized by drm_syncobj_add_callback, additional
> - * data can be passed along by embedding drm_syncobj_cb in another struct.
> - * The callback will get called the next time drm_syncobj_replace_fence is
> - * called.
> - */
> -struct drm_syncobj_cb {
> -	struct list_head node;
> -	drm_syncobj_func_t func;
> -};
> -
>   void drm_syncobj_free(struct kref *kref);
>   
>   /**

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