| Message ID | 20210930182506.1236811-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/a6xx: Track current ctx by seqno | expand |
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. -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 >
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
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 {