Re: [PATCH 02/13] drm/msm: Change msm_gpu_submit() API

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

On Mon, Oct 01, 2018 at 06:01:34PM +0530, Sharat Masetty wrote:
> When the scheduler comes into picture, the msm_gpu_submit() will be
> called from the scheduler worker thread. So save the necessary information
> into msm job structure for use at the actual GPU submission time. This
> also simplifies the msm_gpu_submit() API.

When I read this, I immediately thought you were changing the format of the
submit ioctl struct, but really you are just changing the parameters of the
functions - this isn't an API by definition because it isn't an interface
to anything else.  Words like API get people all worked up - I would recommend
rewording this to be less scary / more accurate.

> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/msm_gem.h        | 1 +
>  drivers/gpu/drm/msm/msm_gem_submit.c | 8 +++++---
>  drivers/gpu/drm/msm/msm_gpu.c        | 8 ++++----
>  drivers/gpu/drm/msm/msm_gpu.h        | 3 +--
>  4 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 287f974..289abe5 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -137,6 +137,7 @@ enum msm_gem_lock {
>   */
>  struct msm_gem_submit {
>  	struct drm_device *dev;
> +	struct msm_file_private *ctx;
>  	struct msm_gpu *gpu;
>  	struct list_head node;   /* node in ring submit list */
>  	struct list_head bo_list;
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 00e6347..14906b9 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -32,7 +32,7 @@
>  
>  static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  		struct msm_gpu *gpu, struct msm_gpu_submitqueue *queue,
> -		uint32_t nr_bos, uint32_t nr_cmds)
> +		uint32_t nr_bos, uint32_t nr_cmds, struct msm_file_private *ctx)

submit_create() is a catch-22.  If I saw all that code inline I would probably
want it to be a sub function too, but on the other hand we're steadily adding
new parameters to it and adding new lines of code that the calling function
doesn't need to translate.

>  {
>  	struct msm_gem_submit *submit;
>  	uint64_t sz = sizeof(*submit) + ((u64)nr_bos * sizeof(submit->bos[0])) +
> @@ -47,6 +47,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
>  
>  	submit->dev = dev;
>  	submit->gpu = gpu;
> +	submit->ctx = ctx;
>  	submit->fence = NULL;
>  	submit->out_fence_id = -1;
>  	submit->pid = get_pid(task_pid(current));
> @@ -474,7 +475,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> -	submit = submit_create(dev, gpu, queue, args->nr_bos, args->nr_cmds);
> +	submit = submit_create(dev, gpu, queue, args->nr_bos,
> +			args->nr_cmds, ctx);

Like maybe we can compromise and pass in args directly to submit_create to help
offset the new parameters.

>  	if (!submit) {
>  		ret = -ENOMEM;
>  		goto out_unlock;
> @@ -560,7 +562,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  
>  	submit->nr_cmds = i;
>  
> -	msm_gpu_submit(gpu, submit, ctx);
> +	msm_gpu_submit(submit);
>  	if (IS_ERR(submit->fence)) {
>  		ret = PTR_ERR(submit->fence);
>  		submit->fence = NULL;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index eb67172..5f9cd85 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -602,9 +602,9 @@ void msm_gpu_retire(struct msm_gpu *gpu)
>  }
>  
>  /* add bo's to gpu's ring, and kick gpu: */
> -struct dma_fence *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_gem_submit *submit)
>  {
> +	struct msm_gpu *gpu = submit->gpu;
>  	struct drm_device *dev = gpu->dev;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_ringbuffer *ring = submit->ring;
> @@ -648,8 +648,8 @@ struct dma_fence *msm_gpu_submit(struct msm_gpu *gpu,
>  			msm_gem_move_to_active(&msm_obj->base, gpu, false, submit->fence);
>  	}
>  
> -	gpu->funcs->submit(gpu, submit, ctx);
> -	priv->lastctx = ctx;
> +	gpu->funcs->submit(gpu, submit, submit->ctx);
> +	priv->lastctx = submit->ctx;
>  
>  	hangcheck_timer_reset(gpu);
>  
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index b562b25..dd55979 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -235,8 +235,7 @@ 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);
> -struct dma_fence *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_gem_submit *submit);
>  
>  int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct msm_gpu *gpu, const struct msm_gpu_funcs *funcs,
> -- 
> 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