* [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex @ 2017-01-30 20:03 Thierry Reding 2017-02-23 16:20 ` Thierry Reding 0 siblings, 1 reply; 4+ messages in thread From: Thierry Reding @ 2017-01-30 20:03 UTC (permalink / raw) To: Ben Skeggs; +Cc: nouveau, dri-devel From: Thierry Reding <treding@nvidia.com> The gk20a implementation of instance memory uses vmap()/vunmap() to map memory regions into the kernel's virtual address space. These functions may sleep, so protecting them by a spin lock is not safe. This triggers a warning if the DEBUG_ATOMIC_SLEEP Kconfig option is enabled. Fix this by using a mutex instead. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index a6a7fa0d7679..7f5244d57d2f 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -94,7 +94,7 @@ struct gk20a_instmem { struct nvkm_instmem base; /* protects vaddr_* and gk20a_instobj::vaddr* */ - spinlock_t lock; + struct mutex lock; /* CPU mappings LRU */ unsigned int vaddr_use; @@ -184,11 +184,10 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) struct gk20a_instmem *imem = node->base.imem; struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; const u64 size = nvkm_memory_size(memory); - unsigned long flags; nvkm_ltc_flush(ltc); - spin_lock_irqsave(&imem->lock, flags); + mutex_lock(&imem->lock); if (node->base.vaddr) { if (!node->use_cpt) { @@ -216,7 +215,7 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) out: node->use_cpt++; - spin_unlock_irqrestore(&imem->lock, flags); + mutex_unlock(&imem->lock); return node->base.vaddr; } @@ -239,9 +238,8 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) struct gk20a_instobj_iommu *node = gk20a_instobj_iommu(memory); struct gk20a_instmem *imem = node->base.imem; struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; - unsigned long flags; - spin_lock_irqsave(&imem->lock, flags); + mutex_lock(&imem->lock); /* we should at least have one user to release... */ if (WARN_ON(node->use_cpt == 0)) @@ -252,7 +250,7 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) list_add_tail(&node->vaddr_node, &imem->vaddr_lru); out: - spin_unlock_irqrestore(&imem->lock, flags); + mutex_unlock(&imem->lock); wmb(); nvkm_ltc_invalidate(ltc); @@ -306,19 +304,18 @@ gk20a_instobj_dtor_iommu(struct nvkm_memory *memory) struct gk20a_instmem *imem = node->base.imem; struct device *dev = imem->base.subdev.device->dev; struct nvkm_mm_node *r; - unsigned long flags; int i; if (unlikely(list_empty(&node->base.mem.regions))) goto out; - spin_lock_irqsave(&imem->lock, flags); + mutex_lock(&imem->lock); /* vaddr has already been recycled */ if (node->base.vaddr) gk20a_instobj_iommu_recycle_vaddr(node); - spin_unlock_irqrestore(&imem->lock, flags); + mutex_unlock(&imem->lock); r = list_first_entry(&node->base.mem.regions, struct nvkm_mm_node, rl_entry); @@ -580,7 +577,7 @@ gk20a_instmem_new(struct nvkm_device *device, int index, if (!(imem = kzalloc(sizeof(*imem), GFP_KERNEL))) return -ENOMEM; nvkm_instmem_ctor(&gk20a_instmem, device, index, &imem->base); - spin_lock_init(&imem->lock); + mutex_init(&imem->lock); *pimem = &imem->base; /* do not allow more than 1MB of CPU-mapped instmem */ -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex 2017-01-30 20:03 [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex Thierry Reding @ 2017-02-23 16:20 ` Thierry Reding 2017-02-24 7:25 ` Alexandre Courbot 0 siblings, 1 reply; 4+ messages in thread From: Thierry Reding @ 2017-02-23 16:20 UTC (permalink / raw) To: Ben Skeggs; +Cc: nouveau, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 3770 bytes --] On Mon, Jan 30, 2017 at 09:03:07PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The gk20a implementation of instance memory uses vmap()/vunmap() to map > memory regions into the kernel's virtual address space. These functions > may sleep, so protecting them by a spin lock is not safe. This triggers > a warning if the DEBUG_ATOMIC_SLEEP Kconfig option is enabled. Fix this > by using a mutex instead. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) Alex, could you take a look at this? Thanks, Thierry > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c > index a6a7fa0d7679..7f5244d57d2f 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c > @@ -94,7 +94,7 @@ struct gk20a_instmem { > struct nvkm_instmem base; > > /* protects vaddr_* and gk20a_instobj::vaddr* */ > - spinlock_t lock; > + struct mutex lock; > > /* CPU mappings LRU */ > unsigned int vaddr_use; > @@ -184,11 +184,10 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) > struct gk20a_instmem *imem = node->base.imem; > struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; > const u64 size = nvkm_memory_size(memory); > - unsigned long flags; > > nvkm_ltc_flush(ltc); > > - spin_lock_irqsave(&imem->lock, flags); > + mutex_lock(&imem->lock); > > if (node->base.vaddr) { > if (!node->use_cpt) { > @@ -216,7 +215,7 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) > > out: > node->use_cpt++; > - spin_unlock_irqrestore(&imem->lock, flags); > + mutex_unlock(&imem->lock); > > return node->base.vaddr; > } > @@ -239,9 +238,8 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) > struct gk20a_instobj_iommu *node = gk20a_instobj_iommu(memory); > struct gk20a_instmem *imem = node->base.imem; > struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; > - unsigned long flags; > > - spin_lock_irqsave(&imem->lock, flags); > + mutex_lock(&imem->lock); > > /* we should at least have one user to release... */ > if (WARN_ON(node->use_cpt == 0)) > @@ -252,7 +250,7 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) > list_add_tail(&node->vaddr_node, &imem->vaddr_lru); > > out: > - spin_unlock_irqrestore(&imem->lock, flags); > + mutex_unlock(&imem->lock); > > wmb(); > nvkm_ltc_invalidate(ltc); > @@ -306,19 +304,18 @@ gk20a_instobj_dtor_iommu(struct nvkm_memory *memory) > struct gk20a_instmem *imem = node->base.imem; > struct device *dev = imem->base.subdev.device->dev; > struct nvkm_mm_node *r; > - unsigned long flags; > int i; > > if (unlikely(list_empty(&node->base.mem.regions))) > goto out; > > - spin_lock_irqsave(&imem->lock, flags); > + mutex_lock(&imem->lock); > > /* vaddr has already been recycled */ > if (node->base.vaddr) > gk20a_instobj_iommu_recycle_vaddr(node); > > - spin_unlock_irqrestore(&imem->lock, flags); > + mutex_unlock(&imem->lock); > > r = list_first_entry(&node->base.mem.regions, struct nvkm_mm_node, > rl_entry); > @@ -580,7 +577,7 @@ gk20a_instmem_new(struct nvkm_device *device, int index, > if (!(imem = kzalloc(sizeof(*imem), GFP_KERNEL))) > return -ENOMEM; > nvkm_instmem_ctor(&gk20a_instmem, device, index, &imem->base); > - spin_lock_init(&imem->lock); > + mutex_init(&imem->lock); > *pimem = &imem->base; > > /* do not allow more than 1MB of CPU-mapped instmem */ > -- > 2.11.0 > [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex 2017-02-23 16:20 ` Thierry Reding @ 2017-02-24 7:25 ` Alexandre Courbot [not found] ` <6febf110-a04a-da21-20b4-638bde1eaf36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Courbot @ 2017-02-24 7:25 UTC (permalink / raw) To: Thierry Reding, Ben Skeggs; +Cc: nouveau, dri-devel On 02/24/2017 01:20 AM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jan 30, 2017 at 09:03:07PM +0100, Thierry Reding wrote: >> From: Thierry Reding <treding@nvidia.com> >> >> The gk20a implementation of instance memory uses vmap()/vunmap() to map >> memory regions into the kernel's virtual address space. These functions >> may sleep, so protecting them by a spin lock is not safe. This triggers >> a warning if the DEBUG_ATOMIC_SLEEP Kconfig option is enabled. Fix this >> by using a mutex instead. >> >> Signed-off-by: Thierry Reding <treding@nvidia.com> >> --- >> drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) > > Alex, could you take a look at this? Sorry! Yes, using a mutex here should be safe since vmap() can sleep anyway. And I don't think this code can ever be reached in atomic context (Ben can confirm that last point). Tested this patch and it seems to work like a charm. Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> Tested-by: Alexandre Courbot <acourbot@nvidia.com> > > Thanks, > Thierry > >> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> index a6a7fa0d7679..7f5244d57d2f 100644 >> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> @@ -94,7 +94,7 @@ struct gk20a_instmem { >> struct nvkm_instmem base; >> >> /* protects vaddr_* and gk20a_instobj::vaddr* */ >> - spinlock_t lock; >> + struct mutex lock; >> >> /* CPU mappings LRU */ >> unsigned int vaddr_use; >> @@ -184,11 +184,10 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) >> struct gk20a_instmem *imem = node->base.imem; >> struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; >> const u64 size = nvkm_memory_size(memory); >> - unsigned long flags; >> >> nvkm_ltc_flush(ltc); >> >> - spin_lock_irqsave(&imem->lock, flags); >> + mutex_lock(&imem->lock); >> >> if (node->base.vaddr) { >> if (!node->use_cpt) { >> @@ -216,7 +215,7 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory *memory) >> >> out: >> node->use_cpt++; >> - spin_unlock_irqrestore(&imem->lock, flags); >> + mutex_unlock(&imem->lock); >> >> return node->base.vaddr; >> } >> @@ -239,9 +238,8 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) >> struct gk20a_instobj_iommu *node = gk20a_instobj_iommu(memory); >> struct gk20a_instmem *imem = node->base.imem; >> struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; >> - unsigned long flags; >> >> - spin_lock_irqsave(&imem->lock, flags); >> + mutex_lock(&imem->lock); >> >> /* we should at least have one user to release... */ >> if (WARN_ON(node->use_cpt == 0)) >> @@ -252,7 +250,7 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) >> list_add_tail(&node->vaddr_node, &imem->vaddr_lru); >> >> out: >> - spin_unlock_irqrestore(&imem->lock, flags); >> + mutex_unlock(&imem->lock); >> >> wmb(); >> nvkm_ltc_invalidate(ltc); >> @@ -306,19 +304,18 @@ gk20a_instobj_dtor_iommu(struct nvkm_memory *memory) >> struct gk20a_instmem *imem = node->base.imem; >> struct device *dev = imem->base.subdev.device->dev; >> struct nvkm_mm_node *r; >> - unsigned long flags; >> int i; >> >> if (unlikely(list_empty(&node->base.mem.regions))) >> goto out; >> >> - spin_lock_irqsave(&imem->lock, flags); >> + mutex_lock(&imem->lock); >> >> /* vaddr has already been recycled */ >> if (node->base.vaddr) >> gk20a_instobj_iommu_recycle_vaddr(node); >> >> - spin_unlock_irqrestore(&imem->lock, flags); >> + mutex_unlock(&imem->lock); >> >> r = list_first_entry(&node->base.mem.regions, struct nvkm_mm_node, >> rl_entry); >> @@ -580,7 +577,7 @@ gk20a_instmem_new(struct nvkm_device *device, int index, >> if (!(imem = kzalloc(sizeof(*imem), GFP_KERNEL))) >> return -ENOMEM; >> nvkm_instmem_ctor(&gk20a_instmem, device, index, &imem->base); >> - spin_lock_init(&imem->lock); >> + mutex_init(&imem->lock); >> *pimem = &imem->base; >> >> /* do not allow more than 1MB of CPU-mapped instmem */ >> -- >> 2.11.0 >> > > * Unknown Key > * 0x7F3EB3A1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <6febf110-a04a-da21-20b4-638bde1eaf36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex [not found] ` <6febf110-a04a-da21-20b4-638bde1eaf36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> @ 2017-02-24 7:59 ` Ben Skeggs 0 siblings, 0 replies; 4+ messages in thread From: Ben Skeggs @ 2017-02-24 7:59 UTC (permalink / raw) To: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1.1: Type: text/plain, Size: 5355 bytes --] On 02/24/2017 05:25 PM, Alexandre Courbot wrote: > On 02/24/2017 01:20 AM, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Mon, Jan 30, 2017 at 09:03:07PM +0100, Thierry Reding wrote: >>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>> >>> The gk20a implementation of instance memory uses vmap()/vunmap() to map >>> memory regions into the kernel's virtual address space. These functions >>> may sleep, so protecting them by a spin lock is not safe. This triggers >>> a warning if the DEBUG_ATOMIC_SLEEP Kconfig option is enabled. Fix this >>> by using a mutex instead. >>> >>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> >>> --- >>> drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c | 19 >>> ++++++++----------- >>> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> Alex, could you take a look at this? > > Sorry! Yes, using a mutex here should be safe since vmap() can sleep > anyway. And I don't think this code can ever be reached in atomic > context (Ben can confirm that last point). Tested this patch and it > seems to work like a charm. That should be true on this chipset. Though, if we ever need to touch the grctx or something in response to an interrupt (iirc nvgpu does this, maybe?), this could change. Granted, we probably should switch to threaded interrupts at some point anyway. As it currently stands, it should be fine. Ben. > > Reviewed-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > Tested-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> > >> >> Thanks, >> Thierry >> >>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> index a6a7fa0d7679..7f5244d57d2f 100644 >>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> @@ -94,7 +94,7 @@ struct gk20a_instmem { >>> struct nvkm_instmem base; >>> >>> /* protects vaddr_* and gk20a_instobj::vaddr* */ >>> - spinlock_t lock; >>> + struct mutex lock; >>> >>> /* CPU mappings LRU */ >>> unsigned int vaddr_use; >>> @@ -184,11 +184,10 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory >>> *memory) >>> struct gk20a_instmem *imem = node->base.imem; >>> struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; >>> const u64 size = nvkm_memory_size(memory); >>> - unsigned long flags; >>> >>> nvkm_ltc_flush(ltc); >>> >>> - spin_lock_irqsave(&imem->lock, flags); >>> + mutex_lock(&imem->lock); >>> >>> if (node->base.vaddr) { >>> if (!node->use_cpt) { >>> @@ -216,7 +215,7 @@ gk20a_instobj_acquire_iommu(struct nvkm_memory >>> *memory) >>> >>> out: >>> node->use_cpt++; >>> - spin_unlock_irqrestore(&imem->lock, flags); >>> + mutex_unlock(&imem->lock); >>> >>> return node->base.vaddr; >>> } >>> @@ -239,9 +238,8 @@ gk20a_instobj_release_iommu(struct nvkm_memory >>> *memory) >>> struct gk20a_instobj_iommu *node = gk20a_instobj_iommu(memory); >>> struct gk20a_instmem *imem = node->base.imem; >>> struct nvkm_ltc *ltc = imem->base.subdev.device->ltc; >>> - unsigned long flags; >>> >>> - spin_lock_irqsave(&imem->lock, flags); >>> + mutex_lock(&imem->lock); >>> >>> /* we should at least have one user to release... */ >>> if (WARN_ON(node->use_cpt == 0)) >>> @@ -252,7 +250,7 @@ gk20a_instobj_release_iommu(struct nvkm_memory >>> *memory) >>> list_add_tail(&node->vaddr_node, &imem->vaddr_lru); >>> >>> out: >>> - spin_unlock_irqrestore(&imem->lock, flags); >>> + mutex_unlock(&imem->lock); >>> >>> wmb(); >>> nvkm_ltc_invalidate(ltc); >>> @@ -306,19 +304,18 @@ gk20a_instobj_dtor_iommu(struct nvkm_memory >>> *memory) >>> struct gk20a_instmem *imem = node->base.imem; >>> struct device *dev = imem->base.subdev.device->dev; >>> struct nvkm_mm_node *r; >>> - unsigned long flags; >>> int i; >>> >>> if (unlikely(list_empty(&node->base.mem.regions))) >>> goto out; >>> >>> - spin_lock_irqsave(&imem->lock, flags); >>> + mutex_lock(&imem->lock); >>> >>> /* vaddr has already been recycled */ >>> if (node->base.vaddr) >>> gk20a_instobj_iommu_recycle_vaddr(node); >>> >>> - spin_unlock_irqrestore(&imem->lock, flags); >>> + mutex_unlock(&imem->lock); >>> >>> r = list_first_entry(&node->base.mem.regions, struct nvkm_mm_node, >>> rl_entry); >>> @@ -580,7 +577,7 @@ gk20a_instmem_new(struct nvkm_device *device, int >>> index, >>> if (!(imem = kzalloc(sizeof(*imem), GFP_KERNEL))) >>> return -ENOMEM; >>> nvkm_instmem_ctor(&gk20a_instmem, device, index, &imem->base); >>> - spin_lock_init(&imem->lock); >>> + mutex_init(&imem->lock); >>> *pimem = &imem->base; >>> >>> /* do not allow more than 1MB of CPU-mapped instmem */ >>> -- >>> 2.11.0 >>> >> >> * Unknown Key >> * 0x7F3EB3A1 >> > > _______________________________________________ > Nouveau mailing list > Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/nouveau [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-24 7:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-30 20:03 [PATCH] drm/nouveau: gk20a: Turn instmem lock into mutex Thierry Reding
2017-02-23 16:20 ` Thierry Reding
2017-02-24 7:25 ` Alexandre Courbot
[not found] ` <6febf110-a04a-da21-20b4-638bde1eaf36-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2017-02-24 7:59 ` Ben Skeggs
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.