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@xxxxxxxxxxxxxx>
> ---
> 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
