On Thu, Sep 30, 2021 at 11:34 AM Daniel Vetter daniel@ffwll.ch wrote:

On Thu, Sep 30, 2021 at 8:20 PM Rob Clark robdclark@gmail.com wrote:

...

From: Rob Clark robdclark@chromium.org

In theory a context can be destroyed and a new one allocated at the same address, making the pointer comparision to detect when we don't need to update the current pagetables invalid. Instead assign a sequence number to each context on creation, and use this for the check.

Fixes: 84c31ee16f90 ("drm/msm/a6xx: Add support for per-instance pagetables") Signed-off-by: Rob Clark robdclark@chromium.org

Usually the weak reference pattern is to wrap a spinlock or something around it and clear it out on destruction. Or hold a full reference (e.g. on intel hw the hw goes unhappy if there's no context/pagetables at all, so we always need some). But I guess this works too.

yeah, the seqno approach was so that gen agnostic code didn't have to go fishing around in a6xx_gpu..

I guess the downside is userspace could open/close the dev file 2^32 times without submitting any rendering to trigger the same sort of issue. But there are plenty of easier ways for userspace to trigger faults if it wanted.

BR, -R

-Daniel

...

---

drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++--- drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 11 ++++++++++- drivers/gpu/drm/msm/msm_drv.c | 3 +++ drivers/gpu/drm/msm/msm_drv.h | 1 + 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 5e1ae3df42ba..e0a8b2fd1ff0 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -106,7 +106,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu, u32 asid; u64 memptr = rbmemptr(ring, ttbr0);

•   if (ctx == a6xx_gpu->cur_ctx)
  

•   if (ctx->seqno == a6xx_gpu->cur_ctx_seqno)
            return;
  
    if (msm_iommu_pagetable_params(ctx->aspace->mmu, &ttbr, &asid))
  

@@ -139,7 +139,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu, OUT_PKT7(ring, CP_EVENT_WRITE, 1); OUT_RING(ring, 0x31);

•   a6xx_gpu->cur_ctx = ctx;
  

•   a6xx_gpu->cur_ctx_seqno = ctx->seqno;
  

}

static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit) @@ -1081,7 +1081,7 @@ static int hw_init(struct msm_gpu *gpu) /* Always come up on rb 0 */ a6xx_gpu->cur_ring = gpu->rb[0];

•   a6xx_gpu->cur_ctx = NULL;
  

•   a6xx_gpu->cur_ctx_seqno = 0;
  
    /* Enable the SQE_to start the CP engine */
    gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 1);
  

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h index 0bc2d062f54a..8e5527c881b1 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h @@ -19,7 +19,16 @@ struct a6xx_gpu { uint64_t sqe_iova;

    struct msm_ringbuffer *cur_ring;

•   struct msm_file_private *cur_ctx;
  

•   /**
  

•    * cur_ctx_seqno:
  

•    *
  

•    * The ctx->seqno value of the context with current pgtables
  

•    * installed.  Tracked by seqno rather than pointer value to
  

•    * avoid dangling pointers, and cases where a ctx can be freed
  

•    * and a new one created with the same address.
  

•    */
  

•   int cur_ctx_seqno;
  
    struct a6xx_gmu gmu;
  
  

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 624078b3adf2..30c1efc3d8a0 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -711,6 +711,7 @@ static void load_gpu(struct drm_device *dev)

static int context_init(struct drm_device *dev, struct drm_file *file) {

•   static atomic_t ident = ATOMIC_INIT(0);
    struct msm_drm_private *priv = dev->dev_private;
    struct msm_file_private *ctx;
  
  

@@ -727,6 +728,8 @@ static int context_init(struct drm_device *dev, struct drm_file *file) ctx->aspace = msm_gpu_create_private_address_space(priv->gpu, current); file->driver_priv = ctx;

•   ctx->seqno = atomic_inc_return(&ident);
  

•   return 0;
  

}

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index de062450add4..8633d0059a3e 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -59,6 +59,7 @@ struct msm_file_private { int queueid; struct msm_gem_address_space *aspace; struct kref ref;

•   int seqno;
  

};

enum msm_mdp_plane_property {

2.31.1

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch