Re: [PATCH 01/13] drm/msm: Track GPU fences with idr

[Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On Mon, Oct 01, 2018 at 06:01:33PM +0530, Sharat Masetty wrote:
> Track the GPU fences created at submit time with idr instead of the ring
> the sequence number. This helps with easily changing the underlying
> fence to something we don't truly own, like the scheduler fence.
> 
> Also move the GPU fence allocation to msm_gpu_submit() and have
> the function return the fence. This suits well when integrating with the
> GPU scheduler.
> 
> Additionally remove the non-interruptible wait variant from msm_wait_fence()
> as it is not used.

So basically this is just propping up the msm_wait_fence ioctl a bit more?
At what point should we just deprecate that bad boy and move on with our lives?

> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/msm_drv.c        |  3 +--
>  drivers/gpu/drm/msm/msm_fence.c      | 30 ++++++++++++++----------------
>  drivers/gpu/drm/msm/msm_fence.h      |  5 +++--
>  drivers/gpu/drm/msm/msm_gem.h        |  1 +
>  drivers/gpu/drm/msm/msm_gem_submit.c | 26 ++++++++++++++++++--------
>  drivers/gpu/drm/msm/msm_gpu.c        | 10 ++++++++--
>  drivers/gpu/drm/msm/msm_gpu.h        |  4 ++--
>  drivers/gpu/drm/msm/msm_ringbuffer.c |  5 +++++
>  drivers/gpu/drm/msm/msm_ringbuffer.h |  1 +
>  9 files changed, 53 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 021a0b6..8eaa1bd 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -746,8 +746,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
>  	if (!queue)
>  		return -ENOENT;
>  
> -	ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
> -		true);
> +	ret = msm_wait_fence(gpu->rb[queue->prio], args->fence, &timeout);
>  
>  	msm_submitqueue_put(queue);
>  	return ret;
> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> index 349c12f..0e7912b 100644
> --- a/drivers/gpu/drm/msm/msm_fence.c
> +++ b/drivers/gpu/drm/msm/msm_fence.c
> @@ -50,41 +50,39 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
>  }
>  
>  /* legacy path for WAIT_FENCE ioctl: */
> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> -		ktime_t *timeout, bool interruptible)
> +int msm_wait_fence(struct msm_ringbuffer *ring, uint32_t fence_id,
> +		ktime_t *timeout)
>  {
> +	struct dma_fence *fence;
>  	int ret;
>  
> -	if (fence > fctx->last_fence) {
> -		DRM_ERROR("%s: waiting on invalid fence: %u (of %u)\n",
> -				fctx->name, fence, fctx->last_fence);
> -		return -EINVAL;
> +	rcu_read_lock();
> +	fence = idr_find(&ring->fence_idr, fence_id);
> +
> +	if (!fence || !dma_fence_get_rcu(fence)) {
> +		rcu_read_unlock();
> +		return 0;
>  	}
> +	rcu_read_unlock();
>  
>  	if (!timeout) {
>  		/* no-wait: */
> -		ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
> +		ret = dma_fence_is_signaled(fence) ? 0 : -EBUSY;
>  	} else {
>  		unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
>  
> -		if (interruptible)
> -			ret = wait_event_interruptible_timeout(fctx->event,
> -				fence_completed(fctx, fence),
> -				remaining_jiffies);
> -		else
> -			ret = wait_event_timeout(fctx->event,
> -				fence_completed(fctx, fence),
> -				remaining_jiffies);
> +		ret = dma_fence_wait_timeout(fence, true, remaining_jiffies);
>  
>  		if (ret == 0) {
>  			DBG("timeout waiting for fence: %u (completed: %u)",
> -					fence, fctx->completed_fence);
> +					fence_id, ring->memptrs->fence);
>  			ret = -ETIMEDOUT;
>  		} else if (ret != -ERESTARTSYS) {
>  			ret = 0;
>  		}
>  	}
>  
> +	dma_fence_put(fence);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> index b9fe059..a8133f7 100644
> --- a/drivers/gpu/drm/msm/msm_fence.h
> +++ b/drivers/gpu/drm/msm/msm_fence.h
> @@ -19,6 +19,7 @@
>  #define __MSM_FENCE_H__
>  
>  #include "msm_drv.h"
> +#include "msm_ringbuffer.h"
>  
>  struct msm_fence_context {
>  	struct drm_device *dev;
> @@ -35,8 +36,8 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,
>  		const char *name);
>  void msm_fence_context_free(struct msm_fence_context *fctx);
>  
> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> -		ktime_t *timeout, bool interruptible);
> +int msm_wait_fence(struct msm_ringbuffer *ring, uint32_t fence_id,
> +		ktime_t *timeout);
>  void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence);
>  
>  struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx);
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index c5d9bd3..287f974 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -143,6 +143,7 @@ struct msm_gem_submit {
>  	struct ww_acquire_ctx ticket;
>  	uint32_t seqno;		/* Sequence number of the submit on the ring */
>  	struct dma_fence *fence;
> +	int out_fence_id;
>  	struct msm_gpu_submitqueue *queue;
>  	struct pid *pid;    /* submitting process */
>  	bool valid;         /* true if no cmdstream patching needed */
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 7bd83e0..00e6347 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -48,6 +48,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  	submit->dev = dev;
>  	submit->gpu = gpu;
>  	submit->fence = NULL;
> +	submit->out_fence_id = -1;
>  	submit->pid = get_pid(task_pid(current));
>  	submit->cmd = (void *)&submit->bos[nr_bos];
>  	submit->queue = queue;
> @@ -66,6 +67,8 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  
>  void msm_gem_submit_free(struct msm_gem_submit *submit)
>  {
> +	idr_remove(&submit->ring->fence_idr, submit->out_fence_id);
> +
>  	dma_fence_put(submit->fence);
>  	list_del(&submit->node);
>  	put_pid(submit->pid);
> @@ -557,26 +560,33 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  
>  	submit->nr_cmds = i;
>  
> -	submit->fence = msm_fence_alloc(ring->fctx);
> +	msm_gpu_submit(gpu, submit, ctx);
>  	if (IS_ERR(submit->fence)) {
>  		ret = PTR_ERR(submit->fence);
>  		submit->fence = NULL;
>  		goto out;
>  	}
>  
> +	/*
> +	 * No protection should be okay here since this is protected by the big
> +	 * GPU lock.
> +	 */
> +	submit->out_fence_id = idr_alloc_cyclic(&ring->fence_idr, submit->fence,
> +			0, INT_MAX, GFP_KERNEL);
> +
> +	if (submit->out_fence_id < 0) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	args->fence = submit->out_fence_id;
> +
>  	if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
>  		sync_file = sync_file_create(submit->fence);
>  		if (!sync_file) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	}
> -
> -	msm_gpu_submit(gpu, submit, ctx);
> -
> -	args->fence = submit->fence->seqno;
> -
> -	if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
>  		fd_install(out_fence_fd, sync_file->file);
>  		args->fence_fd = out_fence_fd;
>  	}
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 1c09acf..eb67172 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -602,8 +602,8 @@ void msm_gpu_retire(struct msm_gpu *gpu)
>  }
>  
>  /* add bo's to gpu's ring, and kick gpu: */
> -void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
> -		struct msm_file_private *ctx)
> +struct dma_fence *msm_gpu_submit(struct msm_gpu *gpu,
> +		struct msm_gem_submit *submit, struct msm_file_private *ctx)
>  {
>  	struct drm_device *dev = gpu->dev;
>  	struct msm_drm_private *priv = dev->dev_private;
> @@ -612,6 +612,10 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
> +	submit->fence = msm_fence_alloc(ring->fctx);
> +	if (IS_ERR(submit->fence))
> +		return submit->fence;
> +
>  	pm_runtime_get_sync(&gpu->pdev->dev);
>  
>  	msm_gpu_hw_init(gpu);
> @@ -648,6 +652,8 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>  	priv->lastctx = ctx;
>  
>  	hangcheck_timer_reset(gpu);
> +
> +	return submit->fence;
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index b824117..b562b25 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -235,8 +235,8 @@ int msm_gpu_perfcntr_sample(struct msm_gpu *gpu, uint32_t *activetime,
>  		uint32_t *totaltime, uint32_t ncntrs, uint32_t *cntrs);
>  
>  void msm_gpu_retire(struct msm_gpu *gpu);
> -void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
> -		struct msm_file_private *ctx);
> +struct dma_fence *msm_gpu_submit(struct msm_gpu *gpu,
> +		struct msm_gem_submit *submit, struct msm_file_private *ctx);
>  
>  int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct msm_gpu *gpu, const struct msm_gpu_funcs *funcs,
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
> index 6f5295b..734f2b8 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> @@ -59,6 +59,8 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,
>  
>  	ring->fctx = msm_fence_context_alloc(gpu->dev, name);
>  
> +	idr_init(&ring->fence_idr);
> +
>  	return ring;
>  
>  fail:
> @@ -78,5 +80,8 @@ void msm_ringbuffer_destroy(struct msm_ringbuffer *ring)
>  		msm_gem_put_vaddr(ring->bo);
>  		drm_gem_object_put_unlocked(ring->bo);
>  	}
> +
> +	idr_destroy(&ring->fence_idr);
> +
>  	kfree(ring);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
> index cffce09..b74a0a9 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.h
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
> @@ -40,6 +40,7 @@ struct msm_ringbuffer {
>  	struct msm_rbmemptrs *memptrs;
>  	uint64_t memptrs_iova;
>  	struct msm_fence_context *fctx;
> +	struct idr fence_idr;
>  	spinlock_t lock;
>  };
>  
> -- 
> 1.9.1
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno