* [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
@ 2017-06-13 22:52 Sushmita Susheelendra
[not found] ` <1497394374-19982-1-git-send-email-ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Sushmita Susheelendra @ 2017-06-13 22:52 UTC (permalink / raw)
To: freedreno; +Cc: linux-arm-msm, dri-devel, Sushmita Susheelendra
Buffer object specific resources like pages, domains, sg list
need not be protected with struct_mutex. They can be protected
with a buffer object level lock. This simplifies locking and
makes it easier to avoid potential recursive locking scenarios
for SVM involving mmap_sem and struct_mutex. This also removes
unnecessary serialization when creating buffer objects, and also
between buffer object creation and GPU command submission.
Change-Id: I4fba9f8c38a6cd13f80f660639d1e74d4336e3fb
Signed-off-by: Sushmita Susheelendra <ssusheel@codeaurora.org>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 -
drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 -
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 -
drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +-
drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 2 +-
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 2 -
drivers/gpu/drm/msm/msm_drv.c | 1 +
drivers/gpu/drm/msm/msm_drv.h | 7 +-
drivers/gpu/drm/msm/msm_fbdev.c | 6 +-
drivers/gpu/drm/msm/msm_gem.c | 190 +++++++++++++++----------------
drivers/gpu/drm/msm/msm_gem.h | 2 +
drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-
drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
drivers/gpu/drm/msm/msm_gpu.c | 4 +-
drivers/gpu/drm/msm/msm_rd.c | 4 +-
drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
16 files changed, 120 insertions(+), 127 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 31a9bce..7893de1 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -235,9 +235,7 @@ static struct drm_gem_object *a5xx_ucode_load_bo(struct msm_gpu *gpu,
struct drm_gem_object *bo;
void *ptr;
- mutex_lock(&drm->struct_mutex);
bo = msm_gem_new(drm, fw->size - 4, MSM_BO_UNCACHED);
- mutex_unlock(&drm->struct_mutex);
if (IS_ERR(bo))
return bo;
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
index 72d52c7..eb88f44 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
@@ -294,9 +294,7 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
*/
bosize = (cmds_size + (cmds_size / TYPE4_MAX_PAYLOAD) + 1) << 2;
- mutex_lock(&drm->struct_mutex);
a5xx_gpu->gpmu_bo = msm_gem_new(drm, bosize, MSM_BO_UNCACHED);
- mutex_unlock(&drm->struct_mutex);
if (IS_ERR(a5xx_gpu->gpmu_bo))
goto err;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 5b63fc6..1162c15 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -392,10 +392,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
return ret;
}
- mutex_lock(&drm->struct_mutex);
adreno_gpu->memptrs_bo = msm_gem_new(drm, sizeof(*adreno_gpu->memptrs),
MSM_BO_UNCACHED);
- mutex_unlock(&drm->struct_mutex);
if (IS_ERR(adreno_gpu->memptrs_bo)) {
ret = PTR_ERR(adreno_gpu->memptrs_bo);
adreno_gpu->memptrs_bo = NULL;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 4f79b10..6a1b0da 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -980,19 +980,16 @@ static int dsi_tx_buf_alloc(struct msm_dsi_host *msm_host, int size)
uint64_t iova;
if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
- mutex_lock(&dev->struct_mutex);
msm_host->tx_gem_obj = msm_gem_new(dev, size, MSM_BO_UNCACHED);
if (IS_ERR(msm_host->tx_gem_obj)) {
ret = PTR_ERR(msm_host->tx_gem_obj);
pr_err("%s: failed to allocate gem, %d\n",
__func__, ret);
msm_host->tx_gem_obj = NULL;
- mutex_unlock(&dev->struct_mutex);
return ret;
}
- ret = msm_gem_get_iova_locked(msm_host->tx_gem_obj, 0, &iova);
- mutex_unlock(&dev->struct_mutex);
+ ret = msm_gem_get_iova(msm_host->tx_gem_obj, 0, &iova);
if (ret) {
pr_err("%s: failed to get iova, %d\n", __func__, ret);
return ret;
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
index f29194a..15478f8 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
@@ -372,7 +372,7 @@ static void update_cursor(struct drm_crtc *crtc)
if (next_bo) {
/* take a obj ref + iova ref when we start scanning out: */
drm_gem_object_reference(next_bo);
- msm_gem_get_iova_locked(next_bo, mdp4_kms->id, &iova);
+ msm_gem_get_iova(next_bo, mdp4_kms->id, &iova);
/* enable cursor: */
mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 6295204..3a464b3 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -561,9 +561,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
goto fail;
}
- mutex_lock(&dev->struct_mutex);
mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC);
- mutex_unlock(&dev->struct_mutex);
if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
dev_err(dev->dev, "could not allocate blank-cursor bo: %d\n", ret);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 87b5695..bb1f3ee 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -349,6 +349,7 @@ static int msm_init_vram(struct drm_device *dev)
priv->vram.size = size;
drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
+ spin_lock_init(&priv->vram.lock);
attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
attrs |= DMA_ATTR_WRITE_COMBINE;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 28b6f9b..567737d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -157,6 +157,7 @@ struct msm_drm_private {
* and position mm_node->start is in # of pages:
*/
struct drm_mm mm;
+ spinlock_t lock; /* Protects drm_mm node allocation/removal */
} vram;
struct notifier_block vmap_notifier;
@@ -209,8 +210,6 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
int msm_gem_fault(struct vm_fault *vmf);
uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
-int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
- uint64_t *iova);
int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova);
uint64_t msm_gem_iova(struct drm_gem_object *obj, int id);
struct page **msm_gem_get_pages(struct drm_gem_object *obj);
@@ -228,9 +227,7 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
struct dma_buf_attachment *attach, struct sg_table *sg);
int msm_gem_prime_pin(struct drm_gem_object *obj);
void msm_gem_prime_unpin(struct drm_gem_object *obj);
-void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
void *msm_gem_get_vaddr(struct drm_gem_object *obj);
-void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
void msm_gem_put_vaddr(struct drm_gem_object *obj);
int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
void msm_gem_purge(struct drm_gem_object *obj);
@@ -247,6 +244,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
uint32_t size, uint32_t flags, uint32_t *handle);
struct drm_gem_object *msm_gem_new(struct drm_device *dev,
uint32_t size, uint32_t flags);
+struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
+ uint32_t size, uint32_t flags);
struct drm_gem_object *msm_gem_import(struct drm_device *dev,
struct dma_buf *dmabuf, struct sg_table *sgt);
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 951e40f..785925e9 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -95,10 +95,8 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
/* allocate backing bo */
size = mode_cmd.pitches[0] * mode_cmd.height;
DBG("allocating %d bytes for fb %d", size, dev->primary->index);
- mutex_lock(&dev->struct_mutex);
fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT |
MSM_BO_WC | MSM_BO_STOLEN);
- mutex_unlock(&dev->struct_mutex);
if (IS_ERR(fbdev->bo)) {
ret = PTR_ERR(fbdev->bo);
fbdev->bo = NULL;
@@ -124,7 +122,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
* in panic (ie. lock-safe, etc) we could avoid pinning the
* buffer now:
*/
- ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
+ ret = msm_gem_get_iova(fbdev->bo, 0, &paddr);
if (ret) {
dev_err(dev->dev, "failed to get buffer obj iova: %d\n", ret);
goto fail_unlock;
@@ -153,7 +151,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
dev->mode_config.fb_base = paddr;
- fbi->screen_base = msm_gem_get_vaddr_locked(fbdev->bo);
+ fbi->screen_base = msm_gem_get_vaddr(fbdev->bo);
if (IS_ERR(fbi->screen_base)) {
ret = PTR_ERR(fbi->screen_base);
goto fail_unlock;
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 68e509b..1e803fb 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -41,8 +41,7 @@ static bool use_pages(struct drm_gem_object *obj)
}
/* allocate pages from VRAM carveout, used when no IOMMU: */
-static struct page **get_pages_vram(struct drm_gem_object *obj,
- int npages)
+static struct page **get_pages_vram(struct drm_gem_object *obj, int npages)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_drm_private *priv = obj->dev->dev_private;
@@ -54,7 +53,9 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
if (!p)
return ERR_PTR(-ENOMEM);
+ spin_lock(&priv->vram.lock);
ret = drm_mm_insert_node(&priv->vram.mm, msm_obj->vram_node, npages);
+ spin_unlock(&priv->vram.lock);
if (ret) {
drm_free_large(p);
return ERR_PTR(ret);
@@ -69,7 +70,6 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
return p;
}
-/* called with dev->struct_mutex held */
static struct page **get_pages(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -109,6 +109,18 @@ static struct page **get_pages(struct drm_gem_object *obj)
return msm_obj->pages;
}
+static void put_pages_vram(struct drm_gem_object *obj)
+{
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ struct msm_drm_private *priv = obj->dev->dev_private;
+
+ spin_lock(&priv->vram.lock);
+ drm_mm_remove_node(msm_obj->vram_node);
+ spin_unlock(&priv->vram.lock);
+
+ drm_free_large(msm_obj->pages);
+}
+
static void put_pages(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -125,10 +137,8 @@ static void put_pages(struct drm_gem_object *obj)
if (use_pages(obj))
drm_gem_put_pages(obj, msm_obj->pages, true, false);
- else {
- drm_mm_remove_node(msm_obj->vram_node);
- drm_free_large(msm_obj->pages);
- }
+ else
+ put_pages_vram(obj);
msm_obj->pages = NULL;
}
@@ -136,11 +146,12 @@ static void put_pages(struct drm_gem_object *obj)
struct page **msm_gem_get_pages(struct drm_gem_object *obj)
{
- struct drm_device *dev = obj->dev;
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct page **p;
- mutex_lock(&dev->struct_mutex);
+
+ mutex_lock(&msm_obj->lock);
p = get_pages(obj);
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&msm_obj->lock);
return p;
}
@@ -195,25 +206,17 @@ int msm_gem_fault(struct vm_fault *vmf)
{
struct vm_area_struct *vma = vmf->vma;
struct drm_gem_object *obj = vma->vm_private_data;
- struct drm_device *dev = obj->dev;
- struct msm_drm_private *priv = dev->dev_private;
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct page **pages;
unsigned long pfn;
pgoff_t pgoff;
int ret;
- /* This should only happen if userspace tries to pass a mmap'd
- * but unfaulted gem bo vaddr into submit ioctl, triggering
- * a page fault while struct_mutex is already held. This is
- * not a valid use-case so just bail.
+ /*
+ * vm_ops.open/drm_gem_mmap_obj and close get and put
+ * a reference on obj. So, we dont need to hold one here.
*/
- if (priv->struct_mutex_task == current)
- return VM_FAULT_SIGBUS;
-
- /* Make sure we don't parallel update on a fault, nor move or remove
- * something from beneath our feet
- */
- ret = mutex_lock_interruptible(&dev->struct_mutex);
+ ret = mutex_lock_interruptible(&msm_obj->lock);
if (ret)
goto out;
@@ -235,7 +238,7 @@ int msm_gem_fault(struct vm_fault *vmf)
ret = vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV));
out_unlock:
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&msm_obj->lock);
out:
switch (ret) {
case -EAGAIN:
@@ -259,9 +262,10 @@ int msm_gem_fault(struct vm_fault *vmf)
static uint64_t mmap_offset(struct drm_gem_object *obj)
{
struct drm_device *dev = obj->dev;
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
int ret;
- WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+ WARN_ON(!mutex_is_locked(&msm_obj->lock));
/* Make it mmapable */
ret = drm_gem_create_mmap_offset(obj);
@@ -277,21 +281,23 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
{
uint64_t offset;
- mutex_lock(&obj->dev->struct_mutex);
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
+
+ mutex_lock(&msm_obj->lock);
offset = mmap_offset(obj);
- mutex_unlock(&obj->dev->struct_mutex);
+ mutex_unlock(&msm_obj->lock);
return offset;
}
+/* Called with msm_obj->lock locked */
static void
put_iova(struct drm_gem_object *obj)
{
- struct drm_device *dev = obj->dev;
struct msm_drm_private *priv = obj->dev->dev_private;
struct msm_gem_object *msm_obj = to_msm_bo(obj);
int id;
- WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+ WARN_ON(!mutex_is_locked(&msm_obj->lock));
for (id = 0; id < ARRAY_SIZE(msm_obj->domain); id++) {
if (!priv->aspace[id])
@@ -301,25 +307,23 @@ uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
}
}
-/* should be called under struct_mutex.. although it can be called
- * from atomic context without struct_mutex to acquire an extra
- * iova ref if you know one is already held.
- *
- * That means when I do eventually need to add support for unpinning
- * the refcnt counter needs to be atomic_t.
- */
-int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
+/* A reference to obj must be held before calling this function. */
+int msm_gem_get_iova(struct drm_gem_object *obj, int id,
uint64_t *iova)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
int ret = 0;
+ mutex_lock(&msm_obj->lock);
+
if (!msm_obj->domain[id].iova) {
struct msm_drm_private *priv = obj->dev->dev_private;
struct page **pages = get_pages(obj);
- if (IS_ERR(pages))
+ if (IS_ERR(pages)) {
+ mutex_unlock(&msm_obj->lock);
return PTR_ERR(pages);
+ }
if (iommu_present(&platform_bus_type)) {
ret = msm_gem_map_vma(priv->aspace[id], &msm_obj->domain[id],
@@ -332,26 +336,7 @@ int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
if (!ret)
*iova = msm_obj->domain[id].iova;
- return ret;
-}
-
-/* get iova, taking a reference. Should have a matching put */
-int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova)
-{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- int ret;
-
- /* this is safe right now because we don't unmap until the
- * bo is deleted:
- */
- if (msm_obj->domain[id].iova) {
- *iova = msm_obj->domain[id].iova;
- return 0;
- }
-
- mutex_lock(&obj->dev->struct_mutex);
- ret = msm_gem_get_iova_locked(obj, id, iova);
- mutex_unlock(&obj->dev->struct_mutex);
+ mutex_unlock(&msm_obj->lock);
return ret;
}
@@ -405,45 +390,37 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
return ret;
}
-void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj)
+void *msm_gem_get_vaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
+
+ mutex_lock(&msm_obj->lock);
if (!msm_obj->vaddr) {
struct page **pages = get_pages(obj);
- if (IS_ERR(pages))
+ if (IS_ERR(pages)) {
+ mutex_unlock(&msm_obj->lock);
return ERR_CAST(pages);
+ }
msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
VM_MAP, pgprot_writecombine(PAGE_KERNEL));
- if (msm_obj->vaddr == NULL)
+ if (msm_obj->vaddr == NULL) {
+ mutex_unlock(&msm_obj->lock);
return ERR_PTR(-ENOMEM);
+ }
}
msm_obj->vmap_count++;
+ mutex_unlock(&msm_obj->lock);
return msm_obj->vaddr;
}
-void *msm_gem_get_vaddr(struct drm_gem_object *obj)
-{
- void *ret;
- mutex_lock(&obj->dev->struct_mutex);
- ret = msm_gem_get_vaddr_locked(obj);
- mutex_unlock(&obj->dev->struct_mutex);
- return ret;
-}
-
-void msm_gem_put_vaddr_locked(struct drm_gem_object *obj)
+void msm_gem_put_vaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
+
+ mutex_lock(&msm_obj->lock);
WARN_ON(msm_obj->vmap_count < 1);
msm_obj->vmap_count--;
-}
-
-void msm_gem_put_vaddr(struct drm_gem_object *obj)
-{
- mutex_lock(&obj->dev->struct_mutex);
- msm_gem_put_vaddr_locked(obj);
- mutex_unlock(&obj->dev->struct_mutex);
+ mutex_unlock(&msm_obj->lock);
}
/* Update madvise status, returns true if not purged, else
@@ -697,6 +674,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
list_del(&msm_obj->mm_list);
+ mutex_lock(&msm_obj->lock);
+
put_iova(obj);
if (obj->import_attach) {
@@ -720,6 +699,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
drm_gem_object_release(obj);
+ mutex_unlock(&msm_obj->lock);
kfree(msm_obj);
}
@@ -730,14 +710,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
struct drm_gem_object *obj;
int ret;
- ret = mutex_lock_interruptible(&dev->struct_mutex);
- if (ret)
- return ret;
-
obj = msm_gem_new(dev, size, flags);
- mutex_unlock(&dev->struct_mutex);
-
if (IS_ERR(obj))
return PTR_ERR(obj);
@@ -752,7 +726,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
static int msm_gem_new_impl(struct drm_device *dev,
uint32_t size, uint32_t flags,
struct reservation_object *resv,
- struct drm_gem_object **obj)
+ struct drm_gem_object **obj,
+ bool struct_mutex_locked)
{
struct msm_drm_private *priv = dev->dev_private;
struct msm_gem_object *msm_obj;
@@ -781,6 +756,8 @@ static int msm_gem_new_impl(struct drm_device *dev,
if (!msm_obj)
return -ENOMEM;
+ mutex_init(&msm_obj->lock);
+
if (use_vram)
msm_obj->vram_node = &msm_obj->domain[0].node;
@@ -795,21 +772,25 @@ static int msm_gem_new_impl(struct drm_device *dev,
}
INIT_LIST_HEAD(&msm_obj->submit_entry);
- list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
+ if (struct_mutex_locked) {
+ list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
+ } else {
+ mutex_lock(&dev->struct_mutex);
+ list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
+ mutex_unlock(&dev->struct_mutex);
+ }
*obj = &msm_obj->base;
return 0;
}
-struct drm_gem_object *msm_gem_new(struct drm_device *dev,
- uint32_t size, uint32_t flags)
+static struct drm_gem_object *_msm_gem_new(struct drm_device *dev,
+ uint32_t size, uint32_t flags, bool struct_mutex_locked)
{
struct drm_gem_object *obj = NULL;
int ret;
- WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
size = PAGE_ALIGN(size);
/* Disallow zero sized objects as they make the underlying
@@ -818,7 +799,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
if (size == 0)
return ERR_PTR(-EINVAL);
- ret = msm_gem_new_impl(dev, size, flags, NULL, &obj);
+ ret = msm_gem_new_impl(dev, size, flags, NULL, &obj, struct_mutex_locked);
if (ret)
goto fail;
@@ -833,10 +814,22 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
return obj;
fail:
- drm_gem_object_unreference(obj);
+ drm_gem_object_unreference_unlocked(obj);
return ERR_PTR(ret);
}
+struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
+ uint32_t size, uint32_t flags)
+{
+ return _msm_gem_new(dev, size, flags, true);
+}
+
+struct drm_gem_object *msm_gem_new(struct drm_device *dev,
+ uint32_t size, uint32_t flags)
+{
+ return _msm_gem_new(dev, size, flags, false);
+}
+
struct drm_gem_object *msm_gem_import(struct drm_device *dev,
struct dma_buf *dmabuf, struct sg_table *sgt)
{
@@ -853,7 +846,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
size = PAGE_ALIGN(dmabuf->size);
- ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj);
+ ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj, false);
if (ret)
goto fail;
@@ -862,17 +855,22 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
npages = size / PAGE_SIZE;
msm_obj = to_msm_bo(obj);
+ mutex_lock(&msm_obj->lock);
msm_obj->sgt = sgt;
msm_obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
if (!msm_obj->pages) {
+ mutex_unlock(&msm_obj->lock);
ret = -ENOMEM;
goto fail;
}
ret = drm_prime_sg_to_page_addr_arrays(sgt, msm_obj->pages, NULL, npages);
- if (ret)
+ if (ret) {
+ mutex_unlock(&msm_obj->lock);
goto fail;
+ }
+ mutex_unlock(&msm_obj->lock);
return obj;
fail:
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 1b4cf20..b1bfc10 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -31,6 +31,7 @@ struct msm_gem_address_space {
* and position mm_node->start is in # of pages:
*/
struct drm_mm mm;
+ spinlock_t lock; /* Protects drm_mm node allocation/removal */
struct msm_mmu *mmu;
struct kref kref;
};
@@ -87,6 +88,7 @@ struct msm_gem_object {
* an IOMMU. Also used for stolen/splashscreen buffer.
*/
struct drm_mm_node *vram_node;
+ struct mutex lock; /* Protects resources associated with bo */
};
#define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 1c545eb..8420551 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -245,7 +245,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
uint64_t iova;
/* if locking succeeded, pin bo: */
- ret = msm_gem_get_iova_locked(&msm_obj->base,
+ ret = msm_gem_get_iova(&msm_obj->base,
submit->gpu->id, &iova);
if (ret)
@@ -301,7 +301,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
/* For now, just map the entire thing. Eventually we probably
* to do it page-by-page, w/ kmap() if not vmap()d..
*/
- ptr = msm_gem_get_vaddr_locked(&obj->base);
+ ptr = msm_gem_get_vaddr(&obj->base);
if (IS_ERR(ptr)) {
ret = PTR_ERR(ptr);
@@ -359,7 +359,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
}
out:
- msm_gem_put_vaddr_locked(&obj->base);
+ msm_gem_put_vaddr(&obj->base);
return ret;
}
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index f285d7e..c36321bc 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -50,7 +50,9 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
aspace->mmu->funcs->unmap(aspace->mmu, vma->iova, sgt, size);
}
+ spin_lock(&aspace->lock);
drm_mm_remove_node(&vma->node);
+ spin_unlock(&aspace->lock);
vma->iova = 0;
@@ -63,10 +65,15 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
{
int ret;
- if (WARN_ON(drm_mm_node_allocated(&vma->node)))
+ spin_lock(&aspace->lock);
+ if (WARN_ON(drm_mm_node_allocated(&vma->node))) {
+ spin_unlock(&aspace->lock);
return 0;
+ }
ret = drm_mm_insert_node(&aspace->mm, &vma->node, npages);
+ spin_unlock(&aspace->lock);
+
if (ret)
return ret;
@@ -94,6 +101,7 @@ struct msm_gem_address_space *
if (!aspace)
return ERR_PTR(-ENOMEM);
+ spin_lock_init(&aspace->lock);
aspace->name = name;
aspace->mmu = msm_iommu_new(dev, domain);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 97b9c38..d6e5cb2 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -495,7 +495,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
/* submit takes a reference to the bo and iova until retired: */
drm_gem_object_reference(&msm_obj->base);
- msm_gem_get_iova_locked(&msm_obj->base,
+ msm_gem_get_iova(&msm_obj->base,
submit->gpu->id, &iova);
if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
@@ -662,9 +662,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
/* Create ringbuffer: */
- mutex_lock(&drm->struct_mutex);
gpu->rb = msm_ringbuffer_new(gpu, ringsz);
- mutex_unlock(&drm->struct_mutex);
if (IS_ERR(gpu->rb)) {
ret = PTR_ERR(gpu->rb);
gpu->rb = NULL;
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
index 0e81faa..0366b80 100644
--- a/drivers/gpu/drm/msm/msm_rd.c
+++ b/drivers/gpu/drm/msm/msm_rd.c
@@ -268,7 +268,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
struct msm_gem_object *obj = submit->bos[idx].obj;
const char *buf;
- buf = msm_gem_get_vaddr_locked(&obj->base);
+ buf = msm_gem_get_vaddr(&obj->base);
if (IS_ERR(buf))
return;
@@ -283,7 +283,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
(uint32_t[3]){ iova, size, iova >> 32 }, 12);
rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
- msm_gem_put_vaddr_locked(&obj->base);
+ msm_gem_put_vaddr(&obj->base);
}
/* called under struct_mutex */
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 67b34e0..791bca3 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -40,7 +40,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int size)
goto fail;
}
- ring->start = msm_gem_get_vaddr_locked(ring->bo);
+ ring->start = msm_gem_get_vaddr(ring->bo);
if (IS_ERR(ring->start)) {
ret = PTR_ERR(ring->start);
goto fail;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <1497394374-19982-1-git-send-email-ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2017-06-13 23:10 ` Rob Clark
0 siblings, 0 replies; 13+ messages in thread
From: Rob Clark @ 2017-06-13 23:10 UTC (permalink / raw)
To: Sushmita Susheelendra
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
On Tue, Jun 13, 2017 at 6:52 PM, Sushmita Susheelendra
<ssusheel@codeaurora.org> wrote:
> Buffer object specific resources like pages, domains, sg list
> need not be protected with struct_mutex. They can be protected
> with a buffer object level lock. This simplifies locking and
> makes it easier to avoid potential recursive locking scenarios
> for SVM involving mmap_sem and struct_mutex. This also removes
> unnecessary serialization when creating buffer objects, and also
> between buffer object creation and GPU command submission.
oh, cool.. this might be a bit conflicty w/ "drm/msm: fix locking
inconsistency for gpu->hw_init()" and possibly the patchset to support
arbitrary # of vma's per gem bo. But it is definitely the direction
we want to go. It should also help us move away from the
mutex_trylock_recursive() hack in the shrinker code.
I'll have a go at rebasing it, and in the process taking a closer
look, tomorrow since this is something that has been on the TODO list
for quite a while now and is pretty badly needed.
BR,
-R
> Change-Id: I4fba9f8c38a6cd13f80f660639d1e74d4336e3fb
> Signed-off-by: Sushmita Susheelendra <ssusheel@codeaurora.org>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 -
> drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 -
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 -
> drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 2 -
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> drivers/gpu/drm/msm/msm_drv.h | 7 +-
> drivers/gpu/drm/msm/msm_fbdev.c | 6 +-
> drivers/gpu/drm/msm/msm_gem.c | 190 +++++++++++++++----------------
> drivers/gpu/drm/msm/msm_gem.h | 2 +
> drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-
> drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
> drivers/gpu/drm/msm/msm_rd.c | 4 +-
> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
> 16 files changed, 120 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 31a9bce..7893de1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -235,9 +235,7 @@ static struct drm_gem_object *a5xx_ucode_load_bo(struct msm_gpu *gpu,
> struct drm_gem_object *bo;
> void *ptr;
>
> - mutex_lock(&drm->struct_mutex);
> bo = msm_gem_new(drm, fw->size - 4, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(bo))
> return bo;
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> index 72d52c7..eb88f44 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> @@ -294,9 +294,7 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
> */
> bosize = (cmds_size + (cmds_size / TYPE4_MAX_PAYLOAD) + 1) << 2;
>
> - mutex_lock(&drm->struct_mutex);
> a5xx_gpu->gpmu_bo = msm_gem_new(drm, bosize, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(a5xx_gpu->gpmu_bo))
> goto err;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 5b63fc6..1162c15 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -392,10 +392,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> return ret;
> }
>
> - mutex_lock(&drm->struct_mutex);
> adreno_gpu->memptrs_bo = msm_gem_new(drm, sizeof(*adreno_gpu->memptrs),
> MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(adreno_gpu->memptrs_bo)) {
> ret = PTR_ERR(adreno_gpu->memptrs_bo);
> adreno_gpu->memptrs_bo = NULL;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 4f79b10..6a1b0da 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -980,19 +980,16 @@ static int dsi_tx_buf_alloc(struct msm_dsi_host *msm_host, int size)
> uint64_t iova;
>
> if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
> - mutex_lock(&dev->struct_mutex);
> msm_host->tx_gem_obj = msm_gem_new(dev, size, MSM_BO_UNCACHED);
> if (IS_ERR(msm_host->tx_gem_obj)) {
> ret = PTR_ERR(msm_host->tx_gem_obj);
> pr_err("%s: failed to allocate gem, %d\n",
> __func__, ret);
> msm_host->tx_gem_obj = NULL;
> - mutex_unlock(&dev->struct_mutex);
> return ret;
> }
>
> - ret = msm_gem_get_iova_locked(msm_host->tx_gem_obj, 0, &iova);
> - mutex_unlock(&dev->struct_mutex);
> + ret = msm_gem_get_iova(msm_host->tx_gem_obj, 0, &iova);
> if (ret) {
> pr_err("%s: failed to get iova, %d\n", __func__, ret);
> return ret;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> index f29194a..15478f8 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> @@ -372,7 +372,7 @@ static void update_cursor(struct drm_crtc *crtc)
> if (next_bo) {
> /* take a obj ref + iova ref when we start scanning out: */
> drm_gem_object_reference(next_bo);
> - msm_gem_get_iova_locked(next_bo, mdp4_kms->id, &iova);
> + msm_gem_get_iova(next_bo, mdp4_kms->id, &iova);
>
> /* enable cursor: */
> mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> index 6295204..3a464b3 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> @@ -561,9 +561,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
> goto fail;
> }
>
> - mutex_lock(&dev->struct_mutex);
> mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
> ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
> dev_err(dev->dev, "could not allocate blank-cursor bo: %d\n", ret);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 87b5695..bb1f3ee 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -349,6 +349,7 @@ static int msm_init_vram(struct drm_device *dev)
> priv->vram.size = size;
>
> drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
> + spin_lock_init(&priv->vram.lock);
>
> attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> attrs |= DMA_ATTR_WRITE_COMBINE;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 28b6f9b..567737d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -157,6 +157,7 @@ struct msm_drm_private {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
> } vram;
>
> struct notifier_block vmap_notifier;
> @@ -209,8 +210,6 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
> int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> int msm_gem_fault(struct vm_fault *vmf);
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> - uint64_t *iova);
> int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova);
> uint64_t msm_gem_iova(struct drm_gem_object *obj, int id);
> struct page **msm_gem_get_pages(struct drm_gem_object *obj);
> @@ -228,9 +227,7 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
> struct dma_buf_attachment *attach, struct sg_table *sg);
> int msm_gem_prime_pin(struct drm_gem_object *obj);
> void msm_gem_prime_unpin(struct drm_gem_object *obj);
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
> void *msm_gem_get_vaddr(struct drm_gem_object *obj);
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
> void msm_gem_put_vaddr(struct drm_gem_object *obj);
> int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
> void msm_gem_purge(struct drm_gem_object *obj);
> @@ -247,6 +244,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> uint32_t size, uint32_t flags, uint32_t *handle);
> struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> uint32_t size, uint32_t flags);
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags);
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt);
>
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index 951e40f..785925e9 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -95,10 +95,8 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
> /* allocate backing bo */
> size = mode_cmd.pitches[0] * mode_cmd.height;
> DBG("allocating %d bytes for fb %d", size, dev->primary->index);
> - mutex_lock(&dev->struct_mutex);
> fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT |
> MSM_BO_WC | MSM_BO_STOLEN);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(fbdev->bo)) {
> ret = PTR_ERR(fbdev->bo);
> fbdev->bo = NULL;
> @@ -124,7 +122,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
> * in panic (ie. lock-safe, etc) we could avoid pinning the
> * buffer now:
> */
> - ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
> + ret = msm_gem_get_iova(fbdev->bo, 0, &paddr);
> if (ret) {
> dev_err(dev->dev, "failed to get buffer obj iova: %d\n", ret);
> goto fail_unlock;
> @@ -153,7 +151,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>
> dev->mode_config.fb_base = paddr;
>
> - fbi->screen_base = msm_gem_get_vaddr_locked(fbdev->bo);
> + fbi->screen_base = msm_gem_get_vaddr(fbdev->bo);
> if (IS_ERR(fbi->screen_base)) {
> ret = PTR_ERR(fbi->screen_base);
> goto fail_unlock;
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 68e509b..1e803fb 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -41,8 +41,7 @@ static bool use_pages(struct drm_gem_object *obj)
> }
>
> /* allocate pages from VRAM carveout, used when no IOMMU: */
> -static struct page **get_pages_vram(struct drm_gem_object *obj,
> - int npages)
> +static struct page **get_pages_vram(struct drm_gem_object *obj, int npages)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct msm_drm_private *priv = obj->dev->dev_private;
> @@ -54,7 +53,9 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
> if (!p)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock(&priv->vram.lock);
> ret = drm_mm_insert_node(&priv->vram.mm, msm_obj->vram_node, npages);
> + spin_unlock(&priv->vram.lock);
> if (ret) {
> drm_free_large(p);
> return ERR_PTR(ret);
> @@ -69,7 +70,6 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
> return p;
> }
>
> -/* called with dev->struct_mutex held */
> static struct page **get_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -109,6 +109,18 @@ static struct page **get_pages(struct drm_gem_object *obj)
> return msm_obj->pages;
> }
>
> +static void put_pages_vram(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + struct msm_drm_private *priv = obj->dev->dev_private;
> +
> + spin_lock(&priv->vram.lock);
> + drm_mm_remove_node(msm_obj->vram_node);
> + spin_unlock(&priv->vram.lock);
> +
> + drm_free_large(msm_obj->pages);
> +}
> +
> static void put_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -125,10 +137,8 @@ static void put_pages(struct drm_gem_object *obj)
>
> if (use_pages(obj))
> drm_gem_put_pages(obj, msm_obj->pages, true, false);
> - else {
> - drm_mm_remove_node(msm_obj->vram_node);
> - drm_free_large(msm_obj->pages);
> - }
> + else
> + put_pages_vram(obj);
>
> msm_obj->pages = NULL;
> }
> @@ -136,11 +146,12 @@ static void put_pages(struct drm_gem_object *obj)
>
> struct page **msm_gem_get_pages(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **p;
> - mutex_lock(&dev->struct_mutex);
> +
> + mutex_lock(&msm_obj->lock);
> p = get_pages(obj);
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return p;
> }
>
> @@ -195,25 +206,17 @@ int msm_gem_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct drm_gem_object *obj = vma->vm_private_data;
> - struct drm_device *dev = obj->dev;
> - struct msm_drm_private *priv = dev->dev_private;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **pages;
> unsigned long pfn;
> pgoff_t pgoff;
> int ret;
>
> - /* This should only happen if userspace tries to pass a mmap'd
> - * but unfaulted gem bo vaddr into submit ioctl, triggering
> - * a page fault while struct_mutex is already held. This is
> - * not a valid use-case so just bail.
> + /*
> + * vm_ops.open/drm_gem_mmap_obj and close get and put
> + * a reference on obj. So, we dont need to hold one here.
> */
> - if (priv->struct_mutex_task == current)
> - return VM_FAULT_SIGBUS;
> -
> - /* Make sure we don't parallel update on a fault, nor move or remove
> - * something from beneath our feet
> - */
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> + ret = mutex_lock_interruptible(&msm_obj->lock);
> if (ret)
> goto out;
>
> @@ -235,7 +238,7 @@ int msm_gem_fault(struct vm_fault *vmf)
> ret = vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV));
>
> out_unlock:
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> out:
> switch (ret) {
> case -EAGAIN:
> @@ -259,9 +262,10 @@ int msm_gem_fault(struct vm_fault *vmf)
> static uint64_t mmap_offset(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> /* Make it mmapable */
> ret = drm_gem_create_mmap_offset(obj);
> @@ -277,21 +281,23 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
> {
> uint64_t offset;
> - mutex_lock(&obj->dev->struct_mutex);
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +
> + mutex_lock(&msm_obj->lock);
> offset = mmap_offset(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return offset;
> }
>
> +/* Called with msm_obj->lock locked */
> static void
> put_iova(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int id;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> for (id = 0; id < ARRAY_SIZE(msm_obj->domain); id++) {
> if (!priv->aspace[id])
> @@ -301,25 +307,23 @@ uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
> }
> }
>
> -/* should be called under struct_mutex.. although it can be called
> - * from atomic context without struct_mutex to acquire an extra
> - * iova ref if you know one is already held.
> - *
> - * That means when I do eventually need to add support for unpinning
> - * the refcnt counter needs to be atomic_t.
> - */
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> +/* A reference to obj must be held before calling this function. */
> +int msm_gem_get_iova(struct drm_gem_object *obj, int id,
> uint64_t *iova)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret = 0;
>
> + mutex_lock(&msm_obj->lock);
> +
> if (!msm_obj->domain[id].iova) {
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct page **pages = get_pages(obj);
>
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return PTR_ERR(pages);
> + }
>
> if (iommu_present(&platform_bus_type)) {
> ret = msm_gem_map_vma(priv->aspace[id], &msm_obj->domain[id],
> @@ -332,26 +336,7 @@ int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> if (!ret)
> *iova = msm_obj->domain[id].iova;
>
> - return ret;
> -}
> -
> -/* get iova, taking a reference. Should have a matching put */
> -int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova)
> -{
> - struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - int ret;
> -
> - /* this is safe right now because we don't unmap until the
> - * bo is deleted:
> - */
> - if (msm_obj->domain[id].iova) {
> - *iova = msm_obj->domain[id].iova;
> - return 0;
> - }
> -
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_iova_locked(obj, id, iova);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return ret;
> }
>
> @@ -405,45 +390,37 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> return ret;
> }
>
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj)
> +void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_CAST(pages);
> + }
> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> - if (msm_obj->vaddr == NULL)
> + if (msm_obj->vaddr == NULL) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_PTR(-ENOMEM);
> + }
> }
> msm_obj->vmap_count++;
> + mutex_unlock(&msm_obj->lock);
> return msm_obj->vaddr;
> }
>
> -void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> -{
> - void *ret;
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> - return ret;
> -}
> -
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj)
> +void msm_gem_put_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> WARN_ON(msm_obj->vmap_count < 1);
> msm_obj->vmap_count--;
> -}
> -
> -void msm_gem_put_vaddr(struct drm_gem_object *obj)
> -{
> - mutex_lock(&obj->dev->struct_mutex);
> - msm_gem_put_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> }
>
> /* Update madvise status, returns true if not purged, else
> @@ -697,6 +674,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> list_del(&msm_obj->mm_list);
>
> + mutex_lock(&msm_obj->lock);
> +
> put_iova(obj);
>
> if (obj->import_attach) {
> @@ -720,6 +699,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> drm_gem_object_release(obj);
>
> + mutex_unlock(&msm_obj->lock);
> kfree(msm_obj);
> }
>
> @@ -730,14 +710,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> struct drm_gem_object *obj;
> int ret;
>
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> - if (ret)
> - return ret;
> -
> obj = msm_gem_new(dev, size, flags);
>
> - mutex_unlock(&dev->struct_mutex);
> -
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> @@ -752,7 +726,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> static int msm_gem_new_impl(struct drm_device *dev,
> uint32_t size, uint32_t flags,
> struct reservation_object *resv,
> - struct drm_gem_object **obj)
> + struct drm_gem_object **obj,
> + bool struct_mutex_locked)
> {
> struct msm_drm_private *priv = dev->dev_private;
> struct msm_gem_object *msm_obj;
> @@ -781,6 +756,8 @@ static int msm_gem_new_impl(struct drm_device *dev,
> if (!msm_obj)
> return -ENOMEM;
>
> + mutex_init(&msm_obj->lock);
> +
> if (use_vram)
> msm_obj->vram_node = &msm_obj->domain[0].node;
>
> @@ -795,21 +772,25 @@ static int msm_gem_new_impl(struct drm_device *dev,
> }
>
> INIT_LIST_HEAD(&msm_obj->submit_entry);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + if (struct_mutex_locked) {
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + } else {
> + mutex_lock(&dev->struct_mutex);
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + mutex_unlock(&dev->struct_mutex);
> + }
>
> *obj = &msm_obj->base;
>
> return 0;
> }
>
> -struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> - uint32_t size, uint32_t flags)
> +static struct drm_gem_object *_msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags, bool struct_mutex_locked)
> {
> struct drm_gem_object *obj = NULL;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> size = PAGE_ALIGN(size);
>
> /* Disallow zero sized objects as they make the underlying
> @@ -818,7 +799,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> if (size == 0)
> return ERR_PTR(-EINVAL);
>
> - ret = msm_gem_new_impl(dev, size, flags, NULL, &obj);
> + ret = msm_gem_new_impl(dev, size, flags, NULL, &obj, struct_mutex_locked);
> if (ret)
> goto fail;
>
> @@ -833,10 +814,22 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> return obj;
>
> fail:
> - drm_gem_object_unreference(obj);
> + drm_gem_object_unreference_unlocked(obj);
> return ERR_PTR(ret);
> }
>
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, true);
> +}
> +
> +struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, false);
> +}
> +
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt)
> {
> @@ -853,7 +846,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>
> size = PAGE_ALIGN(dmabuf->size);
>
> - ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj);
> + ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj, false);
> if (ret)
> goto fail;
>
> @@ -862,17 +855,22 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> npages = size / PAGE_SIZE;
>
> msm_obj = to_msm_bo(obj);
> + mutex_lock(&msm_obj->lock);
> msm_obj->sgt = sgt;
> msm_obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
> if (!msm_obj->pages) {
> + mutex_unlock(&msm_obj->lock);
> ret = -ENOMEM;
> goto fail;
> }
>
> ret = drm_prime_sg_to_page_addr_arrays(sgt, msm_obj->pages, NULL, npages);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&msm_obj->lock);
> goto fail;
> + }
>
> + mutex_unlock(&msm_obj->lock);
> return obj;
>
> fail:
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 1b4cf20..b1bfc10 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -31,6 +31,7 @@ struct msm_gem_address_space {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
> struct msm_mmu *mmu;
> struct kref kref;
> };
> @@ -87,6 +88,7 @@ struct msm_gem_object {
> * an IOMMU. Also used for stolen/splashscreen buffer.
> */
> struct drm_mm_node *vram_node;
> + struct mutex lock; /* Protects resources associated with bo */
> };
> #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 1c545eb..8420551 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -245,7 +245,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> uint64_t iova;
>
> /* if locking succeeded, pin bo: */
> - ret = msm_gem_get_iova_locked(&msm_obj->base,
> + ret = msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (ret)
> @@ -301,7 +301,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
> /* For now, just map the entire thing. Eventually we probably
> * to do it page-by-page, w/ kmap() if not vmap()d..
> */
> - ptr = msm_gem_get_vaddr_locked(&obj->base);
> + ptr = msm_gem_get_vaddr(&obj->base);
>
> if (IS_ERR(ptr)) {
> ret = PTR_ERR(ptr);
> @@ -359,7 +359,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
> }
>
> out:
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index f285d7e..c36321bc 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -50,7 +50,9 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
> aspace->mmu->funcs->unmap(aspace->mmu, vma->iova, sgt, size);
> }
>
> + spin_lock(&aspace->lock);
> drm_mm_remove_node(&vma->node);
> + spin_unlock(&aspace->lock);
>
> vma->iova = 0;
>
> @@ -63,10 +65,15 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
> {
> int ret;
>
> - if (WARN_ON(drm_mm_node_allocated(&vma->node)))
> + spin_lock(&aspace->lock);
> + if (WARN_ON(drm_mm_node_allocated(&vma->node))) {
> + spin_unlock(&aspace->lock);
> return 0;
> + }
>
> ret = drm_mm_insert_node(&aspace->mm, &vma->node, npages);
> + spin_unlock(&aspace->lock);
> +
> if (ret)
> return ret;
>
> @@ -94,6 +101,7 @@ struct msm_gem_address_space *
> if (!aspace)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock_init(&aspace->lock);
> aspace->name = name;
> aspace->mmu = msm_iommu_new(dev, domain);
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 97b9c38..d6e5cb2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -495,7 +495,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>
> /* submit takes a reference to the bo and iova until retired: */
> drm_gem_object_reference(&msm_obj->base);
> - msm_gem_get_iova_locked(&msm_obj->base,
> + msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
> @@ -662,9 +662,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>
>
> /* Create ringbuffer: */
> - mutex_lock(&drm->struct_mutex);
> gpu->rb = msm_ringbuffer_new(gpu, ringsz);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(gpu->rb)) {
> ret = PTR_ERR(gpu->rb);
> gpu->rb = NULL;
> diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
> index 0e81faa..0366b80 100644
> --- a/drivers/gpu/drm/msm/msm_rd.c
> +++ b/drivers/gpu/drm/msm/msm_rd.c
> @@ -268,7 +268,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> struct msm_gem_object *obj = submit->bos[idx].obj;
> const char *buf;
>
> - buf = msm_gem_get_vaddr_locked(&obj->base);
> + buf = msm_gem_get_vaddr(&obj->base);
> if (IS_ERR(buf))
> return;
>
> @@ -283,7 +283,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> (uint32_t[3]){ iova, size, iova >> 32 }, 12);
> rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
>
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
> }
>
> /* called under struct_mutex */
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
> index 67b34e0..791bca3 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> @@ -40,7 +40,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int size)
> goto fail;
> }
>
> - ring->start = msm_gem_get_vaddr_locked(ring->bo);
> + ring->start = msm_gem_get_vaddr(ring->bo);
> if (IS_ERR(ring->start)) {
> ret = PTR_ERR(ring->start);
> goto fail;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
2017-06-13 22:52 [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex Sushmita Susheelendra
[not found] ` <1497394374-19982-1-git-send-email-ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2017-06-14 16:49 ` Rob Clark
[not found] ` <CAF6AEGuUNmpbssk0nsf6Nrb3Z-H5ug-gthEkZTqg_9GQAA3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-15 13:20 ` [PATCH] fixup! " Rob Clark
2 siblings, 1 reply; 13+ messages in thread
From: Rob Clark @ 2017-06-14 16:49 UTC (permalink / raw)
To: Sushmita Susheelendra
Cc: freedreno, linux-arm-msm, dri-devel@lists.freedesktop.org
On Tue, Jun 13, 2017 at 6:52 PM, Sushmita Susheelendra
<ssusheel@codeaurora.org> wrote:
> Buffer object specific resources like pages, domains, sg list
> need not be protected with struct_mutex. They can be protected
> with a buffer object level lock. This simplifies locking and
> makes it easier to avoid potential recursive locking scenarios
> for SVM involving mmap_sem and struct_mutex. This also removes
> unnecessary serialization when creating buffer objects, and also
> between buffer object creation and GPU command submission.
I've rebased this on top of the other changes that are in the queue for 4.13:
https://github.com/freedreno/kernel-msm/commit/4e0c4e139a647914cfcf7c413da5c19f9f124885
I've done some light testing on a5xx.. although between this and
moving gpu->hw_init() under struct_mutex, I need to double check on
a3xx/a4xx.
I do think we probably need a bit more work for shrinker. In
particular msm_gem_purge() still assumes everything is protected by a
single struct_mutex, which is no longer true. The tricky thing here
is that shrinker can be triggered by code-paths where we already hold
msm_obj->lock.
I think we probably want anything that ends up in get_pages() or
vmap() to have something along the lines of
if (WARN_ON(msm_obj->madv == DONTNEED)) {
mutex_unlock(&msm_obj->lock);
return -EINVAL; // or -EBUSY, or ??
}
it's really only something that a badly behaved userspace could
triger.. but otoh we probably don't want to let a badly behaved
userspace deadlock things in the kernel. I guess I probably should
have written some test cases for this by now.
> Change-Id: I4fba9f8c38a6cd13f80f660639d1e74d4336e3fb
jfyi, no need for the Change-Id.. I usually strip them out when I
apply patches, but I appreciate if you can strip them out before
sending (because sometimes I forget)
BR,
-R
> Signed-off-by: Sushmita Susheelendra <ssusheel@codeaurora.org>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 -
> drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 -
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 -
> drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 2 -
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> drivers/gpu/drm/msm/msm_drv.h | 7 +-
> drivers/gpu/drm/msm/msm_fbdev.c | 6 +-
> drivers/gpu/drm/msm/msm_gem.c | 190 +++++++++++++++----------------
> drivers/gpu/drm/msm/msm_gem.h | 2 +
> drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-
> drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
> drivers/gpu/drm/msm/msm_rd.c | 4 +-
> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
> 16 files changed, 120 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 31a9bce..7893de1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -235,9 +235,7 @@ static struct drm_gem_object *a5xx_ucode_load_bo(struct msm_gpu *gpu,
> struct drm_gem_object *bo;
> void *ptr;
>
> - mutex_lock(&drm->struct_mutex);
> bo = msm_gem_new(drm, fw->size - 4, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(bo))
> return bo;
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> index 72d52c7..eb88f44 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> @@ -294,9 +294,7 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
> */
> bosize = (cmds_size + (cmds_size / TYPE4_MAX_PAYLOAD) + 1) << 2;
>
> - mutex_lock(&drm->struct_mutex);
> a5xx_gpu->gpmu_bo = msm_gem_new(drm, bosize, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(a5xx_gpu->gpmu_bo))
> goto err;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 5b63fc6..1162c15 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -392,10 +392,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> return ret;
> }
>
> - mutex_lock(&drm->struct_mutex);
> adreno_gpu->memptrs_bo = msm_gem_new(drm, sizeof(*adreno_gpu->memptrs),
> MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(adreno_gpu->memptrs_bo)) {
> ret = PTR_ERR(adreno_gpu->memptrs_bo);
> adreno_gpu->memptrs_bo = NULL;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 4f79b10..6a1b0da 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -980,19 +980,16 @@ static int dsi_tx_buf_alloc(struct msm_dsi_host *msm_host, int size)
> uint64_t iova;
>
> if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
> - mutex_lock(&dev->struct_mutex);
> msm_host->tx_gem_obj = msm_gem_new(dev, size, MSM_BO_UNCACHED);
> if (IS_ERR(msm_host->tx_gem_obj)) {
> ret = PTR_ERR(msm_host->tx_gem_obj);
> pr_err("%s: failed to allocate gem, %d\n",
> __func__, ret);
> msm_host->tx_gem_obj = NULL;
> - mutex_unlock(&dev->struct_mutex);
> return ret;
> }
>
> - ret = msm_gem_get_iova_locked(msm_host->tx_gem_obj, 0, &iova);
> - mutex_unlock(&dev->struct_mutex);
> + ret = msm_gem_get_iova(msm_host->tx_gem_obj, 0, &iova);
> if (ret) {
> pr_err("%s: failed to get iova, %d\n", __func__, ret);
> return ret;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> index f29194a..15478f8 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> @@ -372,7 +372,7 @@ static void update_cursor(struct drm_crtc *crtc)
> if (next_bo) {
> /* take a obj ref + iova ref when we start scanning out: */
> drm_gem_object_reference(next_bo);
> - msm_gem_get_iova_locked(next_bo, mdp4_kms->id, &iova);
> + msm_gem_get_iova(next_bo, mdp4_kms->id, &iova);
>
> /* enable cursor: */
> mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> index 6295204..3a464b3 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> @@ -561,9 +561,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
> goto fail;
> }
>
> - mutex_lock(&dev->struct_mutex);
> mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
> ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
> dev_err(dev->dev, "could not allocate blank-cursor bo: %d\n", ret);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 87b5695..bb1f3ee 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -349,6 +349,7 @@ static int msm_init_vram(struct drm_device *dev)
> priv->vram.size = size;
>
> drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
> + spin_lock_init(&priv->vram.lock);
>
> attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> attrs |= DMA_ATTR_WRITE_COMBINE;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 28b6f9b..567737d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -157,6 +157,7 @@ struct msm_drm_private {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
> } vram;
>
> struct notifier_block vmap_notifier;
> @@ -209,8 +210,6 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
> int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> int msm_gem_fault(struct vm_fault *vmf);
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> - uint64_t *iova);
> int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova);
> uint64_t msm_gem_iova(struct drm_gem_object *obj, int id);
> struct page **msm_gem_get_pages(struct drm_gem_object *obj);
> @@ -228,9 +227,7 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
> struct dma_buf_attachment *attach, struct sg_table *sg);
> int msm_gem_prime_pin(struct drm_gem_object *obj);
> void msm_gem_prime_unpin(struct drm_gem_object *obj);
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
> void *msm_gem_get_vaddr(struct drm_gem_object *obj);
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
> void msm_gem_put_vaddr(struct drm_gem_object *obj);
> int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
> void msm_gem_purge(struct drm_gem_object *obj);
> @@ -247,6 +244,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> uint32_t size, uint32_t flags, uint32_t *handle);
> struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> uint32_t size, uint32_t flags);
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags);
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt);
>
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index 951e40f..785925e9 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -95,10 +95,8 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
> /* allocate backing bo */
> size = mode_cmd.pitches[0] * mode_cmd.height;
> DBG("allocating %d bytes for fb %d", size, dev->primary->index);
> - mutex_lock(&dev->struct_mutex);
> fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT |
> MSM_BO_WC | MSM_BO_STOLEN);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(fbdev->bo)) {
> ret = PTR_ERR(fbdev->bo);
> fbdev->bo = NULL;
> @@ -124,7 +122,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
> * in panic (ie. lock-safe, etc) we could avoid pinning the
> * buffer now:
> */
> - ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
> + ret = msm_gem_get_iova(fbdev->bo, 0, &paddr);
> if (ret) {
> dev_err(dev->dev, "failed to get buffer obj iova: %d\n", ret);
> goto fail_unlock;
> @@ -153,7 +151,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>
> dev->mode_config.fb_base = paddr;
>
> - fbi->screen_base = msm_gem_get_vaddr_locked(fbdev->bo);
> + fbi->screen_base = msm_gem_get_vaddr(fbdev->bo);
> if (IS_ERR(fbi->screen_base)) {
> ret = PTR_ERR(fbi->screen_base);
> goto fail_unlock;
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 68e509b..1e803fb 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -41,8 +41,7 @@ static bool use_pages(struct drm_gem_object *obj)
> }
>
> /* allocate pages from VRAM carveout, used when no IOMMU: */
> -static struct page **get_pages_vram(struct drm_gem_object *obj,
> - int npages)
> +static struct page **get_pages_vram(struct drm_gem_object *obj, int npages)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct msm_drm_private *priv = obj->dev->dev_private;
> @@ -54,7 +53,9 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
> if (!p)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock(&priv->vram.lock);
> ret = drm_mm_insert_node(&priv->vram.mm, msm_obj->vram_node, npages);
> + spin_unlock(&priv->vram.lock);
> if (ret) {
> drm_free_large(p);
> return ERR_PTR(ret);
> @@ -69,7 +70,6 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
> return p;
> }
>
> -/* called with dev->struct_mutex held */
> static struct page **get_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -109,6 +109,18 @@ static struct page **get_pages(struct drm_gem_object *obj)
> return msm_obj->pages;
> }
>
> +static void put_pages_vram(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + struct msm_drm_private *priv = obj->dev->dev_private;
> +
> + spin_lock(&priv->vram.lock);
> + drm_mm_remove_node(msm_obj->vram_node);
> + spin_unlock(&priv->vram.lock);
> +
> + drm_free_large(msm_obj->pages);
> +}
> +
> static void put_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -125,10 +137,8 @@ static void put_pages(struct drm_gem_object *obj)
>
> if (use_pages(obj))
> drm_gem_put_pages(obj, msm_obj->pages, true, false);
> - else {
> - drm_mm_remove_node(msm_obj->vram_node);
> - drm_free_large(msm_obj->pages);
> - }
> + else
> + put_pages_vram(obj);
>
> msm_obj->pages = NULL;
> }
> @@ -136,11 +146,12 @@ static void put_pages(struct drm_gem_object *obj)
>
> struct page **msm_gem_get_pages(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **p;
> - mutex_lock(&dev->struct_mutex);
> +
> + mutex_lock(&msm_obj->lock);
> p = get_pages(obj);
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return p;
> }
>
> @@ -195,25 +206,17 @@ int msm_gem_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct drm_gem_object *obj = vma->vm_private_data;
> - struct drm_device *dev = obj->dev;
> - struct msm_drm_private *priv = dev->dev_private;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **pages;
> unsigned long pfn;
> pgoff_t pgoff;
> int ret;
>
> - /* This should only happen if userspace tries to pass a mmap'd
> - * but unfaulted gem bo vaddr into submit ioctl, triggering
> - * a page fault while struct_mutex is already held. This is
> - * not a valid use-case so just bail.
> + /*
> + * vm_ops.open/drm_gem_mmap_obj and close get and put
> + * a reference on obj. So, we dont need to hold one here.
> */
> - if (priv->struct_mutex_task == current)
> - return VM_FAULT_SIGBUS;
> -
> - /* Make sure we don't parallel update on a fault, nor move or remove
> - * something from beneath our feet
> - */
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> + ret = mutex_lock_interruptible(&msm_obj->lock);
> if (ret)
> goto out;
>
> @@ -235,7 +238,7 @@ int msm_gem_fault(struct vm_fault *vmf)
> ret = vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV));
>
> out_unlock:
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> out:
> switch (ret) {
> case -EAGAIN:
> @@ -259,9 +262,10 @@ int msm_gem_fault(struct vm_fault *vmf)
> static uint64_t mmap_offset(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> /* Make it mmapable */
> ret = drm_gem_create_mmap_offset(obj);
> @@ -277,21 +281,23 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
> {
> uint64_t offset;
> - mutex_lock(&obj->dev->struct_mutex);
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +
> + mutex_lock(&msm_obj->lock);
> offset = mmap_offset(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return offset;
> }
>
> +/* Called with msm_obj->lock locked */
> static void
> put_iova(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int id;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> for (id = 0; id < ARRAY_SIZE(msm_obj->domain); id++) {
> if (!priv->aspace[id])
> @@ -301,25 +307,23 @@ uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
> }
> }
>
> -/* should be called under struct_mutex.. although it can be called
> - * from atomic context without struct_mutex to acquire an extra
> - * iova ref if you know one is already held.
> - *
> - * That means when I do eventually need to add support for unpinning
> - * the refcnt counter needs to be atomic_t.
> - */
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> +/* A reference to obj must be held before calling this function. */
> +int msm_gem_get_iova(struct drm_gem_object *obj, int id,
> uint64_t *iova)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret = 0;
>
> + mutex_lock(&msm_obj->lock);
> +
> if (!msm_obj->domain[id].iova) {
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct page **pages = get_pages(obj);
>
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return PTR_ERR(pages);
> + }
>
> if (iommu_present(&platform_bus_type)) {
> ret = msm_gem_map_vma(priv->aspace[id], &msm_obj->domain[id],
> @@ -332,26 +336,7 @@ int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> if (!ret)
> *iova = msm_obj->domain[id].iova;
>
> - return ret;
> -}
> -
> -/* get iova, taking a reference. Should have a matching put */
> -int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova)
> -{
> - struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - int ret;
> -
> - /* this is safe right now because we don't unmap until the
> - * bo is deleted:
> - */
> - if (msm_obj->domain[id].iova) {
> - *iova = msm_obj->domain[id].iova;
> - return 0;
> - }
> -
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_iova_locked(obj, id, iova);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return ret;
> }
>
> @@ -405,45 +390,37 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> return ret;
> }
>
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj)
> +void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_CAST(pages);
> + }
> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> - if (msm_obj->vaddr == NULL)
> + if (msm_obj->vaddr == NULL) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_PTR(-ENOMEM);
> + }
> }
> msm_obj->vmap_count++;
> + mutex_unlock(&msm_obj->lock);
> return msm_obj->vaddr;
> }
>
> -void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> -{
> - void *ret;
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> - return ret;
> -}
> -
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj)
> +void msm_gem_put_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> WARN_ON(msm_obj->vmap_count < 1);
> msm_obj->vmap_count--;
> -}
> -
> -void msm_gem_put_vaddr(struct drm_gem_object *obj)
> -{
> - mutex_lock(&obj->dev->struct_mutex);
> - msm_gem_put_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> }
>
> /* Update madvise status, returns true if not purged, else
> @@ -697,6 +674,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> list_del(&msm_obj->mm_list);
>
> + mutex_lock(&msm_obj->lock);
> +
> put_iova(obj);
>
> if (obj->import_attach) {
> @@ -720,6 +699,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> drm_gem_object_release(obj);
>
> + mutex_unlock(&msm_obj->lock);
> kfree(msm_obj);
> }
>
> @@ -730,14 +710,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> struct drm_gem_object *obj;
> int ret;
>
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> - if (ret)
> - return ret;
> -
> obj = msm_gem_new(dev, size, flags);
>
> - mutex_unlock(&dev->struct_mutex);
> -
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> @@ -752,7 +726,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> static int msm_gem_new_impl(struct drm_device *dev,
> uint32_t size, uint32_t flags,
> struct reservation_object *resv,
> - struct drm_gem_object **obj)
> + struct drm_gem_object **obj,
> + bool struct_mutex_locked)
> {
> struct msm_drm_private *priv = dev->dev_private;
> struct msm_gem_object *msm_obj;
> @@ -781,6 +756,8 @@ static int msm_gem_new_impl(struct drm_device *dev,
> if (!msm_obj)
> return -ENOMEM;
>
> + mutex_init(&msm_obj->lock);
> +
> if (use_vram)
> msm_obj->vram_node = &msm_obj->domain[0].node;
>
> @@ -795,21 +772,25 @@ static int msm_gem_new_impl(struct drm_device *dev,
> }
>
> INIT_LIST_HEAD(&msm_obj->submit_entry);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + if (struct_mutex_locked) {
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + } else {
> + mutex_lock(&dev->struct_mutex);
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + mutex_unlock(&dev->struct_mutex);
> + }
>
> *obj = &msm_obj->base;
>
> return 0;
> }
>
> -struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> - uint32_t size, uint32_t flags)
> +static struct drm_gem_object *_msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags, bool struct_mutex_locked)
> {
> struct drm_gem_object *obj = NULL;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> size = PAGE_ALIGN(size);
>
> /* Disallow zero sized objects as they make the underlying
> @@ -818,7 +799,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> if (size == 0)
> return ERR_PTR(-EINVAL);
>
> - ret = msm_gem_new_impl(dev, size, flags, NULL, &obj);
> + ret = msm_gem_new_impl(dev, size, flags, NULL, &obj, struct_mutex_locked);
> if (ret)
> goto fail;
>
> @@ -833,10 +814,22 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> return obj;
>
> fail:
> - drm_gem_object_unreference(obj);
> + drm_gem_object_unreference_unlocked(obj);
> return ERR_PTR(ret);
> }
>
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, true);
> +}
> +
> +struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, false);
> +}
> +
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt)
> {
> @@ -853,7 +846,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>
> size = PAGE_ALIGN(dmabuf->size);
>
> - ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj);
> + ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj, false);
> if (ret)
> goto fail;
>
> @@ -862,17 +855,22 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> npages = size / PAGE_SIZE;
>
> msm_obj = to_msm_bo(obj);
> + mutex_lock(&msm_obj->lock);
> msm_obj->sgt = sgt;
> msm_obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
> if (!msm_obj->pages) {
> + mutex_unlock(&msm_obj->lock);
> ret = -ENOMEM;
> goto fail;
> }
>
> ret = drm_prime_sg_to_page_addr_arrays(sgt, msm_obj->pages, NULL, npages);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&msm_obj->lock);
> goto fail;
> + }
>
> + mutex_unlock(&msm_obj->lock);
> return obj;
>
> fail:
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 1b4cf20..b1bfc10 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -31,6 +31,7 @@ struct msm_gem_address_space {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
> struct msm_mmu *mmu;
> struct kref kref;
> };
> @@ -87,6 +88,7 @@ struct msm_gem_object {
> * an IOMMU. Also used for stolen/splashscreen buffer.
> */
> struct drm_mm_node *vram_node;
> + struct mutex lock; /* Protects resources associated with bo */
> };
> #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 1c545eb..8420551 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -245,7 +245,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> uint64_t iova;
>
> /* if locking succeeded, pin bo: */
> - ret = msm_gem_get_iova_locked(&msm_obj->base,
> + ret = msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (ret)
> @@ -301,7 +301,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
> /* For now, just map the entire thing. Eventually we probably
> * to do it page-by-page, w/ kmap() if not vmap()d..
> */
> - ptr = msm_gem_get_vaddr_locked(&obj->base);
> + ptr = msm_gem_get_vaddr(&obj->base);
>
> if (IS_ERR(ptr)) {
> ret = PTR_ERR(ptr);
> @@ -359,7 +359,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
> }
>
> out:
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index f285d7e..c36321bc 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -50,7 +50,9 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
> aspace->mmu->funcs->unmap(aspace->mmu, vma->iova, sgt, size);
> }
>
> + spin_lock(&aspace->lock);
> drm_mm_remove_node(&vma->node);
> + spin_unlock(&aspace->lock);
>
> vma->iova = 0;
>
> @@ -63,10 +65,15 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
> {
> int ret;
>
> - if (WARN_ON(drm_mm_node_allocated(&vma->node)))
> + spin_lock(&aspace->lock);
> + if (WARN_ON(drm_mm_node_allocated(&vma->node))) {
> + spin_unlock(&aspace->lock);
> return 0;
> + }
>
> ret = drm_mm_insert_node(&aspace->mm, &vma->node, npages);
> + spin_unlock(&aspace->lock);
> +
> if (ret)
> return ret;
>
> @@ -94,6 +101,7 @@ struct msm_gem_address_space *
> if (!aspace)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock_init(&aspace->lock);
> aspace->name = name;
> aspace->mmu = msm_iommu_new(dev, domain);
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 97b9c38..d6e5cb2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -495,7 +495,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>
> /* submit takes a reference to the bo and iova until retired: */
> drm_gem_object_reference(&msm_obj->base);
> - msm_gem_get_iova_locked(&msm_obj->base,
> + msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
> @@ -662,9 +662,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>
>
> /* Create ringbuffer: */
> - mutex_lock(&drm->struct_mutex);
> gpu->rb = msm_ringbuffer_new(gpu, ringsz);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(gpu->rb)) {
> ret = PTR_ERR(gpu->rb);
> gpu->rb = NULL;
> diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
> index 0e81faa..0366b80 100644
> --- a/drivers/gpu/drm/msm/msm_rd.c
> +++ b/drivers/gpu/drm/msm/msm_rd.c
> @@ -268,7 +268,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> struct msm_gem_object *obj = submit->bos[idx].obj;
> const char *buf;
>
> - buf = msm_gem_get_vaddr_locked(&obj->base);
> + buf = msm_gem_get_vaddr(&obj->base);
> if (IS_ERR(buf))
> return;
>
> @@ -283,7 +283,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> (uint32_t[3]){ iova, size, iova >> 32 }, 12);
> rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
>
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
> }
>
> /* called under struct_mutex */
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
> index 67b34e0..791bca3 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> @@ -40,7 +40,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int size)
> goto fail;
> }
>
> - ring->start = msm_gem_get_vaddr_locked(ring->bo);
> + ring->start = msm_gem_get_vaddr(ring->bo);
> if (IS_ERR(ring->start)) {
> ret = PTR_ERR(ring->start);
> goto fail;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <CAF6AEGuUNmpbssk0nsf6Nrb3Z-H5ug-gthEkZTqg_9GQAA3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-14 23:08 ` Susheelendra, Sushmita
2017-06-14 23:31 ` Rob Clark
2017-06-15 10:19 ` Chris Wilson
1 sibling, 1 reply; 13+ messages in thread
From: Susheelendra, Sushmita @ 2017-06-14 23:08 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
[-- Attachment #1.1: Type: text/plain, Size: 35279 bytes --]
Hi Rob,
Yes, I’d completely missed the shrinker path in this cleanup. But, yeah, I see how get_pages (which is called with msm_obj->lock held) -> drm_gem_get_pages could trigger shrinker_scan which calls msm_gem_purge.
It makes sense to prevent any get_pages/vmap on objects that’ve been marked as DONTNEED. I’ll send you a patch soon for that.
Thanks,
Sushmita
> On Jun 14, 2017, at 10:49 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Tue, Jun 13, 2017 at 6:52 PM, Sushmita Susheelendra
> <ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org <mailto:ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>> wrote:
>> Buffer object specific resources like pages, domains, sg list
>> need not be protected with struct_mutex. They can be protected
>> with a buffer object level lock. This simplifies locking and
>> makes it easier to avoid potential recursive locking scenarios
>> for SVM involving mmap_sem and struct_mutex. This also removes
>> unnecessary serialization when creating buffer objects, and also
>> between buffer object creation and GPU command submission.
>
> I've rebased this on top of the other changes that are in the queue for 4.13:
>
> https://github.com/freedreno/kernel-msm/commit/4e0c4e139a647914cfcf7c413da5c19f9f124885 <https://github.com/freedreno/kernel-msm/commit/4e0c4e139a647914cfcf7c413da5c19f9f124885>
>
> I've done some light testing on a5xx.. although between this and
> moving gpu->hw_init() under struct_mutex, I need to double check on
> a3xx/a4xx.
>
> I do think we probably need a bit more work for shrinker. In
> particular msm_gem_purge() still assumes everything is protected by a
> single struct_mutex, which is no longer true. The tricky thing here
> is that shrinker can be triggered by code-paths where we already hold
> msm_obj->lock.
>
> I think we probably want anything that ends up in get_pages() or
> vmap() to have something along the lines of
>
> if (WARN_ON(msm_obj->madv == DONTNEED)) {
> mutex_unlock(&msm_obj->lock);
> return -EINVAL; // or -EBUSY, or ??
> }
>
> it's really only something that a badly behaved userspace could
> triger.. but otoh we probably don't want to let a badly behaved
> userspace deadlock things in the kernel. I guess I probably should
> have written some test cases for this by now.
>
>> Change-Id: I4fba9f8c38a6cd13f80f660639d1e74d4336e3fb
>
> jfyi, no need for the Change-Id.. I usually strip them out when I
> apply patches, but I appreciate if you can strip them out before
> sending (because sometimes I forget)
>
> BR,
> -R
>
>> Signed-off-by: Sushmita Susheelendra <ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 -
>> drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 -
>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 -
>> drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +-
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 2 +-
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 2 -
>> drivers/gpu/drm/msm/msm_drv.c | 1 +
>> drivers/gpu/drm/msm/msm_drv.h | 7 +-
>> drivers/gpu/drm/msm/msm_fbdev.c | 6 +-
>> drivers/gpu/drm/msm/msm_gem.c | 190 +++++++++++++++----------------
>> drivers/gpu/drm/msm/msm_gem.h | 2 +
>> drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-
>> drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
>> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
>> drivers/gpu/drm/msm/msm_rd.c | 4 +-
>> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
>> 16 files changed, 120 insertions(+), 127 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> index 31a9bce..7893de1 100644
>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> @@ -235,9 +235,7 @@ static struct drm_gem_object *a5xx_ucode_load_bo(struct msm_gpu *gpu,
>> struct drm_gem_object *bo;
>> void *ptr;
>>
>> - mutex_lock(&drm->struct_mutex);
>> bo = msm_gem_new(drm, fw->size - 4, MSM_BO_UNCACHED);
>> - mutex_unlock(&drm->struct_mutex);
>>
>> if (IS_ERR(bo))
>> return bo;
>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c b/drivers/gpu/drm/msm/adreno/a5xx_power.c
>> index 72d52c7..eb88f44 100644
>> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
>> @@ -294,9 +294,7 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
>> */
>> bosize = (cmds_size + (cmds_size / TYPE4_MAX_PAYLOAD) + 1) << 2;
>>
>> - mutex_lock(&drm->struct_mutex);
>> a5xx_gpu->gpmu_bo = msm_gem_new(drm, bosize, MSM_BO_UNCACHED);
>> - mutex_unlock(&drm->struct_mutex);
>>
>> if (IS_ERR(a5xx_gpu->gpmu_bo))
>> goto err;
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index 5b63fc6..1162c15 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -392,10 +392,8 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>> return ret;
>> }
>>
>> - mutex_lock(&drm->struct_mutex);
>> adreno_gpu->memptrs_bo = msm_gem_new(drm, sizeof(*adreno_gpu->memptrs),
>> MSM_BO_UNCACHED);
>> - mutex_unlock(&drm->struct_mutex);
>> if (IS_ERR(adreno_gpu->memptrs_bo)) {
>> ret = PTR_ERR(adreno_gpu->memptrs_bo);
>> adreno_gpu->memptrs_bo = NULL;
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 4f79b10..6a1b0da 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -980,19 +980,16 @@ static int dsi_tx_buf_alloc(struct msm_dsi_host *msm_host, int size)
>> uint64_t iova;
>>
>> if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
>> - mutex_lock(&dev->struct_mutex);
>> msm_host->tx_gem_obj = msm_gem_new(dev, size, MSM_BO_UNCACHED);
>> if (IS_ERR(msm_host->tx_gem_obj)) {
>> ret = PTR_ERR(msm_host->tx_gem_obj);
>> pr_err("%s: failed to allocate gem, %d\n",
>> __func__, ret);
>> msm_host->tx_gem_obj = NULL;
>> - mutex_unlock(&dev->struct_mutex);
>> return ret;
>> }
>>
>> - ret = msm_gem_get_iova_locked(msm_host->tx_gem_obj, 0, &iova);
>> - mutex_unlock(&dev->struct_mutex);
>> + ret = msm_gem_get_iova(msm_host->tx_gem_obj, 0, &iova);
>> if (ret) {
>> pr_err("%s: failed to get iova, %d\n", __func__, ret);
>> return ret;
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
>> index f29194a..15478f8 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
>> @@ -372,7 +372,7 @@ static void update_cursor(struct drm_crtc *crtc)
>> if (next_bo) {
>> /* take a obj ref + iova ref when we start scanning out: */
>> drm_gem_object_reference(next_bo);
>> - msm_gem_get_iova_locked(next_bo, mdp4_kms->id, &iova);
>> + msm_gem_get_iova(next_bo, mdp4_kms->id, &iova);
>>
>> /* enable cursor: */
>> mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>> index 6295204..3a464b3 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>> @@ -561,9 +561,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
>> goto fail;
>> }
>>
>> - mutex_lock(&dev->struct_mutex);
>> mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC);
>> - mutex_unlock(&dev->struct_mutex);
>> if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
>> ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
>> dev_err(dev->dev, "could not allocate blank-cursor bo: %d\n", ret);
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index 87b5695..bb1f3ee 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -349,6 +349,7 @@ static int msm_init_vram(struct drm_device *dev)
>> priv->vram.size = size;
>>
>> drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
>> + spin_lock_init(&priv->vram.lock);
>>
>> attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
>> attrs |= DMA_ATTR_WRITE_COMBINE;
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 28b6f9b..567737d 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -157,6 +157,7 @@ struct msm_drm_private {
>> * and position mm_node->start is in # of pages:
>> */
>> struct drm_mm mm;
>> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
>> } vram;
>>
>> struct notifier_block vmap_notifier;
>> @@ -209,8 +210,6 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
>> int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>> int msm_gem_fault(struct vm_fault *vmf);
>> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
>> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
>> - uint64_t *iova);
>> int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova);
>> uint64_t msm_gem_iova(struct drm_gem_object *obj, int id);
>> struct page **msm_gem_get_pages(struct drm_gem_object *obj);
>> @@ -228,9 +227,7 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
>> struct dma_buf_attachment *attach, struct sg_table *sg);
>> int msm_gem_prime_pin(struct drm_gem_object *obj);
>> void msm_gem_prime_unpin(struct drm_gem_object *obj);
>> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
>> void *msm_gem_get_vaddr(struct drm_gem_object *obj);
>> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
>> void msm_gem_put_vaddr(struct drm_gem_object *obj);
>> int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
>> void msm_gem_purge(struct drm_gem_object *obj);
>> @@ -247,6 +244,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>> uint32_t size, uint32_t flags, uint32_t *handle);
>> struct drm_gem_object *msm_gem_new(struct drm_device *dev,
>> uint32_t size, uint32_t flags);
>> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
>> + uint32_t size, uint32_t flags);
>> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>> struct dma_buf *dmabuf, struct sg_table *sgt);
>>
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
>> index 951e40f..785925e9 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -95,10 +95,8 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>> /* allocate backing bo */
>> size = mode_cmd.pitches[0] * mode_cmd.height;
>> DBG("allocating %d bytes for fb %d", size, dev->primary->index);
>> - mutex_lock(&dev->struct_mutex);
>> fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT |
>> MSM_BO_WC | MSM_BO_STOLEN);
>> - mutex_unlock(&dev->struct_mutex);
>> if (IS_ERR(fbdev->bo)) {
>> ret = PTR_ERR(fbdev->bo);
>> fbdev->bo = NULL;
>> @@ -124,7 +122,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>> * in panic (ie. lock-safe, etc) we could avoid pinning the
>> * buffer now:
>> */
>> - ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
>> + ret = msm_gem_get_iova(fbdev->bo, 0, &paddr);
>> if (ret) {
>> dev_err(dev->dev, "failed to get buffer obj iova: %d\n", ret);
>> goto fail_unlock;
>> @@ -153,7 +151,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>>
>> dev->mode_config.fb_base = paddr;
>>
>> - fbi->screen_base = msm_gem_get_vaddr_locked(fbdev->bo);
>> + fbi->screen_base = msm_gem_get_vaddr(fbdev->bo);
>> if (IS_ERR(fbi->screen_base)) {
>> ret = PTR_ERR(fbi->screen_base);
>> goto fail_unlock;
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index 68e509b..1e803fb 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -41,8 +41,7 @@ static bool use_pages(struct drm_gem_object *obj)
>> }
>>
>> /* allocate pages from VRAM carveout, used when no IOMMU: */
>> -static struct page **get_pages_vram(struct drm_gem_object *obj,
>> - int npages)
>> +static struct page **get_pages_vram(struct drm_gem_object *obj, int npages)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> struct msm_drm_private *priv = obj->dev->dev_private;
>> @@ -54,7 +53,9 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
>> if (!p)
>> return ERR_PTR(-ENOMEM);
>>
>> + spin_lock(&priv->vram.lock);
>> ret = drm_mm_insert_node(&priv->vram.mm, msm_obj->vram_node, npages);
>> + spin_unlock(&priv->vram.lock);
>> if (ret) {
>> drm_free_large(p);
>> return ERR_PTR(ret);
>> @@ -69,7 +70,6 @@ static struct page **get_pages_vram(struct drm_gem_object *obj,
>> return p;
>> }
>>
>> -/* called with dev->struct_mutex held */
>> static struct page **get_pages(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> @@ -109,6 +109,18 @@ static struct page **get_pages(struct drm_gem_object *obj)
>> return msm_obj->pages;
>> }
>>
>> +static void put_pages_vram(struct drm_gem_object *obj)
>> +{
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> + struct msm_drm_private *priv = obj->dev->dev_private;
>> +
>> + spin_lock(&priv->vram.lock);
>> + drm_mm_remove_node(msm_obj->vram_node);
>> + spin_unlock(&priv->vram.lock);
>> +
>> + drm_free_large(msm_obj->pages);
>> +}
>> +
>> static void put_pages(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> @@ -125,10 +137,8 @@ static void put_pages(struct drm_gem_object *obj)
>>
>> if (use_pages(obj))
>> drm_gem_put_pages(obj, msm_obj->pages, true, false);
>> - else {
>> - drm_mm_remove_node(msm_obj->vram_node);
>> - drm_free_large(msm_obj->pages);
>> - }
>> + else
>> + put_pages_vram(obj);
>>
>> msm_obj->pages = NULL;
>> }
>> @@ -136,11 +146,12 @@ static void put_pages(struct drm_gem_object *obj)
>>
>> struct page **msm_gem_get_pages(struct drm_gem_object *obj)
>> {
>> - struct drm_device *dev = obj->dev;
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> struct page **p;
>> - mutex_lock(&dev->struct_mutex);
>> +
>> + mutex_lock(&msm_obj->lock);
>> p = get_pages(obj);
>> - mutex_unlock(&dev->struct_mutex);
>> + mutex_unlock(&msm_obj->lock);
>> return p;
>> }
>>
>> @@ -195,25 +206,17 @@ int msm_gem_fault(struct vm_fault *vmf)
>> {
>> struct vm_area_struct *vma = vmf->vma;
>> struct drm_gem_object *obj = vma->vm_private_data;
>> - struct drm_device *dev = obj->dev;
>> - struct msm_drm_private *priv = dev->dev_private;
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> struct page **pages;
>> unsigned long pfn;
>> pgoff_t pgoff;
>> int ret;
>>
>> - /* This should only happen if userspace tries to pass a mmap'd
>> - * but unfaulted gem bo vaddr into submit ioctl, triggering
>> - * a page fault while struct_mutex is already held. This is
>> - * not a valid use-case so just bail.
>> + /*
>> + * vm_ops.open/drm_gem_mmap_obj and close get and put
>> + * a reference on obj. So, we dont need to hold one here.
>> */
>> - if (priv->struct_mutex_task == current)
>> - return VM_FAULT_SIGBUS;
>> -
>> - /* Make sure we don't parallel update on a fault, nor move or remove
>> - * something from beneath our feet
>> - */
>> - ret = mutex_lock_interruptible(&dev->struct_mutex);
>> + ret = mutex_lock_interruptible(&msm_obj->lock);
>> if (ret)
>> goto out;
>>
>> @@ -235,7 +238,7 @@ int msm_gem_fault(struct vm_fault *vmf)
>> ret = vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV));
>>
>> out_unlock:
>> - mutex_unlock(&dev->struct_mutex);
>> + mutex_unlock(&msm_obj->lock);
>> out:
>> switch (ret) {
>> case -EAGAIN:
>> @@ -259,9 +262,10 @@ int msm_gem_fault(struct vm_fault *vmf)
>> static uint64_t mmap_offset(struct drm_gem_object *obj)
>> {
>> struct drm_device *dev = obj->dev;
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> int ret;
>>
>> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>>
>> /* Make it mmapable */
>> ret = drm_gem_create_mmap_offset(obj);
>> @@ -277,21 +281,23 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
>> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
>> {
>> uint64_t offset;
>> - mutex_lock(&obj->dev->struct_mutex);
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +
>> + mutex_lock(&msm_obj->lock);
>> offset = mmap_offset(obj);
>> - mutex_unlock(&obj->dev->struct_mutex);
>> + mutex_unlock(&msm_obj->lock);
>> return offset;
>> }
>>
>> +/* Called with msm_obj->lock locked */
>> static void
>> put_iova(struct drm_gem_object *obj)
>> {
>> - struct drm_device *dev = obj->dev;
>> struct msm_drm_private *priv = obj->dev->dev_private;
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> int id;
>>
>> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>>
>> for (id = 0; id < ARRAY_SIZE(msm_obj->domain); id++) {
>> if (!priv->aspace[id])
>> @@ -301,25 +307,23 @@ uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
>> }
>> }
>>
>> -/* should be called under struct_mutex.. although it can be called
>> - * from atomic context without struct_mutex to acquire an extra
>> - * iova ref if you know one is already held.
>> - *
>> - * That means when I do eventually need to add support for unpinning
>> - * the refcnt counter needs to be atomic_t.
>> - */
>> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
>> +/* A reference to obj must be held before calling this function. */
>> +int msm_gem_get_iova(struct drm_gem_object *obj, int id,
>> uint64_t *iova)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> int ret = 0;
>>
>> + mutex_lock(&msm_obj->lock);
>> +
>> if (!msm_obj->domain[id].iova) {
>> struct msm_drm_private *priv = obj->dev->dev_private;
>> struct page **pages = get_pages(obj);
>>
>> - if (IS_ERR(pages))
>> + if (IS_ERR(pages)) {
>> + mutex_unlock(&msm_obj->lock);
>> return PTR_ERR(pages);
>> + }
>>
>> if (iommu_present(&platform_bus_type)) {
>> ret = msm_gem_map_vma(priv->aspace[id], &msm_obj->domain[id],
>> @@ -332,26 +336,7 @@ int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
>> if (!ret)
>> *iova = msm_obj->domain[id].iova;
>>
>> - return ret;
>> -}
>> -
>> -/* get iova, taking a reference. Should have a matching put */
>> -int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova)
>> -{
>> - struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> - int ret;
>> -
>> - /* this is safe right now because we don't unmap until the
>> - * bo is deleted:
>> - */
>> - if (msm_obj->domain[id].iova) {
>> - *iova = msm_obj->domain[id].iova;
>> - return 0;
>> - }
>> -
>> - mutex_lock(&obj->dev->struct_mutex);
>> - ret = msm_gem_get_iova_locked(obj, id, iova);
>> - mutex_unlock(&obj->dev->struct_mutex);
>> + mutex_unlock(&msm_obj->lock);
>> return ret;
>> }
>>
>> @@ -405,45 +390,37 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>> return ret;
>> }
>>
>> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj)
>> +void *msm_gem_get_vaddr(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>> +
>> + mutex_lock(&msm_obj->lock);
>> if (!msm_obj->vaddr) {
>> struct page **pages = get_pages(obj);
>> - if (IS_ERR(pages))
>> + if (IS_ERR(pages)) {
>> + mutex_unlock(&msm_obj->lock);
>> return ERR_CAST(pages);
>> + }
>> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
>> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>> - if (msm_obj->vaddr == NULL)
>> + if (msm_obj->vaddr == NULL) {
>> + mutex_unlock(&msm_obj->lock);
>> return ERR_PTR(-ENOMEM);
>> + }
>> }
>> msm_obj->vmap_count++;
>> + mutex_unlock(&msm_obj->lock);
>> return msm_obj->vaddr;
>> }
>>
>> -void *msm_gem_get_vaddr(struct drm_gem_object *obj)
>> -{
>> - void *ret;
>> - mutex_lock(&obj->dev->struct_mutex);
>> - ret = msm_gem_get_vaddr_locked(obj);
>> - mutex_unlock(&obj->dev->struct_mutex);
>> - return ret;
>> -}
>> -
>> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj)
>> +void msm_gem_put_vaddr(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>> +
>> + mutex_lock(&msm_obj->lock);
>> WARN_ON(msm_obj->vmap_count < 1);
>> msm_obj->vmap_count--;
>> -}
>> -
>> -void msm_gem_put_vaddr(struct drm_gem_object *obj)
>> -{
>> - mutex_lock(&obj->dev->struct_mutex);
>> - msm_gem_put_vaddr_locked(obj);
>> - mutex_unlock(&obj->dev->struct_mutex);
>> + mutex_unlock(&msm_obj->lock);
>> }
>>
>> /* Update madvise status, returns true if not purged, else
>> @@ -697,6 +674,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>>
>> list_del(&msm_obj->mm_list);
>>
>> + mutex_lock(&msm_obj->lock);
>> +
>> put_iova(obj);
>>
>> if (obj->import_attach) {
>> @@ -720,6 +699,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>>
>> drm_gem_object_release(obj);
>>
>> + mutex_unlock(&msm_obj->lock);
>> kfree(msm_obj);
>> }
>>
>> @@ -730,14 +710,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>> struct drm_gem_object *obj;
>> int ret;
>>
>> - ret = mutex_lock_interruptible(&dev->struct_mutex);
>> - if (ret)
>> - return ret;
>> -
>> obj = msm_gem_new(dev, size, flags);
>>
>> - mutex_unlock(&dev->struct_mutex);
>> -
>> if (IS_ERR(obj))
>> return PTR_ERR(obj);
>>
>> @@ -752,7 +726,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>> static int msm_gem_new_impl(struct drm_device *dev,
>> uint32_t size, uint32_t flags,
>> struct reservation_object *resv,
>> - struct drm_gem_object **obj)
>> + struct drm_gem_object **obj,
>> + bool struct_mutex_locked)
>> {
>> struct msm_drm_private *priv = dev->dev_private;
>> struct msm_gem_object *msm_obj;
>> @@ -781,6 +756,8 @@ static int msm_gem_new_impl(struct drm_device *dev,
>> if (!msm_obj)
>> return -ENOMEM;
>>
>> + mutex_init(&msm_obj->lock);
>> +
>> if (use_vram)
>> msm_obj->vram_node = &msm_obj->domain[0].node;
>>
>> @@ -795,21 +772,25 @@ static int msm_gem_new_impl(struct drm_device *dev,
>> }
>>
>> INIT_LIST_HEAD(&msm_obj->submit_entry);
>> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
>> + if (struct_mutex_locked) {
>> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
>> + } else {
>> + mutex_lock(&dev->struct_mutex);
>> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
>> + mutex_unlock(&dev->struct_mutex);
>> + }
>>
>> *obj = &msm_obj->base;
>>
>> return 0;
>> }
>>
>> -struct drm_gem_object *msm_gem_new(struct drm_device *dev,
>> - uint32_t size, uint32_t flags)
>> +static struct drm_gem_object *_msm_gem_new(struct drm_device *dev,
>> + uint32_t size, uint32_t flags, bool struct_mutex_locked)
>> {
>> struct drm_gem_object *obj = NULL;
>> int ret;
>>
>> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> -
>> size = PAGE_ALIGN(size);
>>
>> /* Disallow zero sized objects as they make the underlying
>> @@ -818,7 +799,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
>> if (size == 0)
>> return ERR_PTR(-EINVAL);
>>
>> - ret = msm_gem_new_impl(dev, size, flags, NULL, &obj);
>> + ret = msm_gem_new_impl(dev, size, flags, NULL, &obj, struct_mutex_locked);
>> if (ret)
>> goto fail;
>>
>> @@ -833,10 +814,22 @@ struct drm_gem_object *msm_gem_new(struct drm_device *dev,
>> return obj;
>>
>> fail:
>> - drm_gem_object_unreference(obj);
>> + drm_gem_object_unreference_unlocked(obj);
>> return ERR_PTR(ret);
>> }
>>
>> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
>> + uint32_t size, uint32_t flags)
>> +{
>> + return _msm_gem_new(dev, size, flags, true);
>> +}
>> +
>> +struct drm_gem_object *msm_gem_new(struct drm_device *dev,
>> + uint32_t size, uint32_t flags)
>> +{
>> + return _msm_gem_new(dev, size, flags, false);
>> +}
>> +
>> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>> struct dma_buf *dmabuf, struct sg_table *sgt)
>> {
>> @@ -853,7 +846,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>>
>> size = PAGE_ALIGN(dmabuf->size);
>>
>> - ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj);
>> + ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj, false);
>> if (ret)
>> goto fail;
>>
>> @@ -862,17 +855,22 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>> npages = size / PAGE_SIZE;
>>
>> msm_obj = to_msm_bo(obj);
>> + mutex_lock(&msm_obj->lock);
>> msm_obj->sgt = sgt;
>> msm_obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
>> if (!msm_obj->pages) {
>> + mutex_unlock(&msm_obj->lock);
>> ret = -ENOMEM;
>> goto fail;
>> }
>>
>> ret = drm_prime_sg_to_page_addr_arrays(sgt, msm_obj->pages, NULL, npages);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&msm_obj->lock);
>> goto fail;
>> + }
>>
>> + mutex_unlock(&msm_obj->lock);
>> return obj;
>>
>> fail:
>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
>> index 1b4cf20..b1bfc10 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.h
>> +++ b/drivers/gpu/drm/msm/msm_gem.h
>> @@ -31,6 +31,7 @@ struct msm_gem_address_space {
>> * and position mm_node->start is in # of pages:
>> */
>> struct drm_mm mm;
>> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
>> struct msm_mmu *mmu;
>> struct kref kref;
>> };
>> @@ -87,6 +88,7 @@ struct msm_gem_object {
>> * an IOMMU. Also used for stolen/splashscreen buffer.
>> */
>> struct drm_mm_node *vram_node;
>> + struct mutex lock; /* Protects resources associated with bo */
>> };
>> #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
>> index 1c545eb..8420551 100644
>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
>> @@ -245,7 +245,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
>> uint64_t iova;
>>
>> /* if locking succeeded, pin bo: */
>> - ret = msm_gem_get_iova_locked(&msm_obj->base,
>> + ret = msm_gem_get_iova(&msm_obj->base,
>> submit->gpu->id, &iova);
>>
>> if (ret)
>> @@ -301,7 +301,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
>> /* For now, just map the entire thing. Eventually we probably
>> * to do it page-by-page, w/ kmap() if not vmap()d..
>> */
>> - ptr = msm_gem_get_vaddr_locked(&obj->base);
>> + ptr = msm_gem_get_vaddr(&obj->base);
>>
>> if (IS_ERR(ptr)) {
>> ret = PTR_ERR(ptr);
>> @@ -359,7 +359,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
>> }
>>
>> out:
>> - msm_gem_put_vaddr_locked(&obj->base);
>> + msm_gem_put_vaddr(&obj->base);
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
>> index f285d7e..c36321bc 100644
>> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
>> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
>> @@ -50,7 +50,9 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
>> aspace->mmu->funcs->unmap(aspace->mmu, vma->iova, sgt, size);
>> }
>>
>> + spin_lock(&aspace->lock);
>> drm_mm_remove_node(&vma->node);
>> + spin_unlock(&aspace->lock);
>>
>> vma->iova = 0;
>>
>> @@ -63,10 +65,15 @@ void msm_gem_address_space_put(struct msm_gem_address_space *aspace)
>> {
>> int ret;
>>
>> - if (WARN_ON(drm_mm_node_allocated(&vma->node)))
>> + spin_lock(&aspace->lock);
>> + if (WARN_ON(drm_mm_node_allocated(&vma->node))) {
>> + spin_unlock(&aspace->lock);
>> return 0;
>> + }
>>
>> ret = drm_mm_insert_node(&aspace->mm, &vma->node, npages);
>> + spin_unlock(&aspace->lock);
>> +
>> if (ret)
>> return ret;
>>
>> @@ -94,6 +101,7 @@ struct msm_gem_address_space *
>> if (!aspace)
>> return ERR_PTR(-ENOMEM);
>>
>> + spin_lock_init(&aspace->lock);
>> aspace->name = name;
>> aspace->mmu = msm_iommu_new(dev, domain);
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>> index 97b9c38..d6e5cb2 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>> @@ -495,7 +495,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>>
>> /* submit takes a reference to the bo and iova until retired: */
>> drm_gem_object_reference(&msm_obj->base);
>> - msm_gem_get_iova_locked(&msm_obj->base,
>> + msm_gem_get_iova(&msm_obj->base,
>> submit->gpu->id, &iova);
>>
>> if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
>> @@ -662,9 +662,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>
>>
>> /* Create ringbuffer: */
>> - mutex_lock(&drm->struct_mutex);
>> gpu->rb = msm_ringbuffer_new(gpu, ringsz);
>> - mutex_unlock(&drm->struct_mutex);
>> if (IS_ERR(gpu->rb)) {
>> ret = PTR_ERR(gpu->rb);
>> gpu->rb = NULL;
>> diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
>> index 0e81faa..0366b80 100644
>> --- a/drivers/gpu/drm/msm/msm_rd.c
>> +++ b/drivers/gpu/drm/msm/msm_rd.c
>> @@ -268,7 +268,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
>> struct msm_gem_object *obj = submit->bos[idx].obj;
>> const char *buf;
>>
>> - buf = msm_gem_get_vaddr_locked(&obj->base);
>> + buf = msm_gem_get_vaddr(&obj->base);
>> if (IS_ERR(buf))
>> return;
>>
>> @@ -283,7 +283,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
>> (uint32_t[3]){ iova, size, iova >> 32 }, 12);
>> rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
>>
>> - msm_gem_put_vaddr_locked(&obj->base);
>> + msm_gem_put_vaddr(&obj->base);
>> }
>>
>> /* called under struct_mutex */
>> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
>> index 67b34e0..791bca3 100644
>> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
>> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
>> @@ -40,7 +40,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int size)
>> goto fail;
>> }
>>
>> - ring->start = msm_gem_get_vaddr_locked(ring->bo);
>> + ring->start = msm_gem_get_vaddr(ring->bo);
>> if (IS_ERR(ring->start)) {
>> ret = PTR_ERR(ring->start);
>> goto fail;
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <mailto:majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> More majordomo info at http://vger.kernel.org/majordomo-info.html <http://vger.kernel.org/majordomo-info.html>
[-- Attachment #1.2: Type: text/html, Size: 87259 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
2017-06-14 23:08 ` Susheelendra, Sushmita
@ 2017-06-14 23:31 ` Rob Clark
0 siblings, 0 replies; 13+ messages in thread
From: Rob Clark @ 2017-06-14 23:31 UTC (permalink / raw)
To: Susheelendra, Sushmita
Cc: linux-arm-msm, freedreno, dri-devel@lists.freedesktop.org
I think the trick is what is the best way to synchronize access to
msm_obj->madv in the shrinker path, since we can't reliably just take
msm_obj->lock in shrinker path since we might already hold it:
https://hastebin.com/ubafepahov.xml
fwiw, that was with my work-in-progress attempt to deal with shrinker:
https://hastebin.com/hucijugufo.cs
Anyways, got distracted on other things and didn't get much further,
but hopefully should have some time for another look tomorrow.
Suggestions welcome ;-)
BR,
-R
On Wed, Jun 14, 2017 at 7:08 PM, Susheelendra, Sushmita
<ssusheel@codeaurora.org> wrote:
> Hi Rob,
>
> Yes, I’d completely missed the shrinker path in this cleanup. But, yeah, I
> see how get_pages (which is called with msm_obj->lock held) ->
> drm_gem_get_pages could trigger shrinker_scan which calls msm_gem_purge.
> It makes sense to prevent any get_pages/vmap on objects that’ve been marked
> as DONTNEED. I’ll send you a patch soon for that.
>
> Thanks,
> Sushmita
>
>
> On Jun 14, 2017, at 10:49 AM, Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Jun 13, 2017 at 6:52 PM, Sushmita Susheelendra
> <ssusheel@codeaurora.org> wrote:
>
> Buffer object specific resources like pages, domains, sg list
> need not be protected with struct_mutex. They can be protected
> with a buffer object level lock. This simplifies locking and
> makes it easier to avoid potential recursive locking scenarios
> for SVM involving mmap_sem and struct_mutex. This also removes
> unnecessary serialization when creating buffer objects, and also
> between buffer object creation and GPU command submission.
>
>
> I've rebased this on top of the other changes that are in the queue for
> 4.13:
>
> https://github.com/freedreno/kernel-msm/commit/4e0c4e139a647914cfcf7c413da5c19f9f124885
>
> I've done some light testing on a5xx.. although between this and
> moving gpu->hw_init() under struct_mutex, I need to double check on
> a3xx/a4xx.
>
> I do think we probably need a bit more work for shrinker. In
> particular msm_gem_purge() still assumes everything is protected by a
> single struct_mutex, which is no longer true. The tricky thing here
> is that shrinker can be triggered by code-paths where we already hold
> msm_obj->lock.
>
> I think we probably want anything that ends up in get_pages() or
> vmap() to have something along the lines of
>
> if (WARN_ON(msm_obj->madv == DONTNEED)) {
> mutex_unlock(&msm_obj->lock);
> return -EINVAL; // or -EBUSY, or ??
> }
>
> it's really only something that a badly behaved userspace could
> triger.. but otoh we probably don't want to let a badly behaved
> userspace deadlock things in the kernel. I guess I probably should
> have written some test cases for this by now.
>
> Change-Id: I4fba9f8c38a6cd13f80f660639d1e74d4336e3fb
>
>
> jfyi, no need for the Change-Id.. I usually strip them out when I
> apply patches, but I appreciate if you can strip them out before
> sending (because sometimes I forget)
>
> BR,
> -R
>
> Signed-off-by: Sushmita Susheelendra <ssusheel@codeaurora.org>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 -
> drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 -
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 -
> drivers/gpu/drm/msm/dsi/dsi_host.c | 5 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 2 -
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> drivers/gpu/drm/msm/msm_drv.h | 7 +-
> drivers/gpu/drm/msm/msm_fbdev.c | 6 +-
> drivers/gpu/drm/msm/msm_gem.c | 190
> +++++++++++++++----------------
> drivers/gpu/drm/msm/msm_gem.h | 2 +
> drivers/gpu/drm/msm/msm_gem_submit.c | 6 +-
> drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
> drivers/gpu/drm/msm/msm_rd.c | 4 +-
> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
> 16 files changed, 120 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 31a9bce..7893de1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -235,9 +235,7 @@ static struct drm_gem_object *a5xx_ucode_load_bo(struct
> msm_gpu *gpu,
> struct drm_gem_object *bo;
> void *ptr;
>
> - mutex_lock(&drm->struct_mutex);
> bo = msm_gem_new(drm, fw->size - 4, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(bo))
> return bo;
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> index 72d52c7..eb88f44 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> @@ -294,9 +294,7 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
> */
> bosize = (cmds_size + (cmds_size / TYPE4_MAX_PAYLOAD) + 1) << 2;
>
> - mutex_lock(&drm->struct_mutex);
> a5xx_gpu->gpmu_bo = msm_gem_new(drm, bosize, MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
>
> if (IS_ERR(a5xx_gpu->gpmu_bo))
> goto err;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 5b63fc6..1162c15 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -392,10 +392,8 @@ int adreno_gpu_init(struct drm_device *drm, struct
> platform_device *pdev,
> return ret;
> }
>
> - mutex_lock(&drm->struct_mutex);
> adreno_gpu->memptrs_bo = msm_gem_new(drm,
> sizeof(*adreno_gpu->memptrs),
> MSM_BO_UNCACHED);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(adreno_gpu->memptrs_bo)) {
> ret = PTR_ERR(adreno_gpu->memptrs_bo);
> adreno_gpu->memptrs_bo = NULL;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 4f79b10..6a1b0da 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -980,19 +980,16 @@ static int dsi_tx_buf_alloc(struct msm_dsi_host
> *msm_host, int size)
> uint64_t iova;
>
> if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
> - mutex_lock(&dev->struct_mutex);
> msm_host->tx_gem_obj = msm_gem_new(dev, size,
> MSM_BO_UNCACHED);
> if (IS_ERR(msm_host->tx_gem_obj)) {
> ret = PTR_ERR(msm_host->tx_gem_obj);
> pr_err("%s: failed to allocate gem, %d\n",
> __func__, ret);
> msm_host->tx_gem_obj = NULL;
> - mutex_unlock(&dev->struct_mutex);
> return ret;
> }
>
> - ret = msm_gem_get_iova_locked(msm_host->tx_gem_obj, 0,
> &iova);
> - mutex_unlock(&dev->struct_mutex);
> + ret = msm_gem_get_iova(msm_host->tx_gem_obj, 0, &iova);
> if (ret) {
> pr_err("%s: failed to get iova, %d\n", __func__,
> ret);
> return ret;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> index f29194a..15478f8 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
> @@ -372,7 +372,7 @@ static void update_cursor(struct drm_crtc *crtc)
> if (next_bo) {
> /* take a obj ref + iova ref when we start scanning
> out: */
> drm_gem_object_reference(next_bo);
> - msm_gem_get_iova_locked(next_bo, mdp4_kms->id,
> &iova);
> + msm_gem_get_iova(next_bo, mdp4_kms->id, &iova);
>
> /* enable cursor: */
> mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> index 6295204..3a464b3 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> @@ -561,9 +561,7 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
> goto fail;
> }
>
> - mutex_lock(&dev->struct_mutex);
> mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(mdp4_kms->blank_cursor_bo)) {
> ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
> dev_err(dev->dev, "could not allocate blank-cursor bo: %d\n",
> ret);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 87b5695..bb1f3ee 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -349,6 +349,7 @@ static int msm_init_vram(struct drm_device *dev)
> priv->vram.size = size;
>
> drm_mm_init(&priv->vram.mm, 0, (size >> PAGE_SHIFT) - 1);
> + spin_lock_init(&priv->vram.lock);
>
> attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> attrs |= DMA_ATTR_WRITE_COMBINE;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 28b6f9b..567737d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -157,6 +157,7 @@ struct msm_drm_private {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal
> */
> } vram;
>
> struct notifier_block vmap_notifier;
> @@ -209,8 +210,6 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
> int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> int msm_gem_fault(struct vm_fault *vmf);
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> - uint64_t *iova);
> int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova);
> uint64_t msm_gem_iova(struct drm_gem_object *obj, int id);
> struct page **msm_gem_get_pages(struct drm_gem_object *obj);
> @@ -228,9 +227,7 @@ struct drm_gem_object
> *msm_gem_prime_import_sg_table(struct drm_device *dev,
> struct dma_buf_attachment *attach, struct sg_table *sg);
> int msm_gem_prime_pin(struct drm_gem_object *obj);
> void msm_gem_prime_unpin(struct drm_gem_object *obj);
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
> void *msm_gem_get_vaddr(struct drm_gem_object *obj);
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
> void msm_gem_put_vaddr(struct drm_gem_object *obj);
> int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
> void msm_gem_purge(struct drm_gem_object *obj);
> @@ -247,6 +244,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct
> drm_file *file,
> uint32_t size, uint32_t flags, uint32_t *handle);
> struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> uint32_t size, uint32_t flags);
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags);
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt);
>
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c
> b/drivers/gpu/drm/msm/msm_fbdev.c
> index 951e40f..785925e9 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -95,10 +95,8 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
> /* allocate backing bo */
> size = mode_cmd.pitches[0] * mode_cmd.height;
> DBG("allocating %d bytes for fb %d", size, dev->primary->index);
> - mutex_lock(&dev->struct_mutex);
> fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT |
> MSM_BO_WC | MSM_BO_STOLEN);
> - mutex_unlock(&dev->struct_mutex);
> if (IS_ERR(fbdev->bo)) {
> ret = PTR_ERR(fbdev->bo);
> fbdev->bo = NULL;
> @@ -124,7 +122,7 @@ static int msm_fbdev_create(struct drm_fb_helper
> *helper,
> * in panic (ie. lock-safe, etc) we could avoid pinning the
> * buffer now:
> */
> - ret = msm_gem_get_iova_locked(fbdev->bo, 0, &paddr);
> + ret = msm_gem_get_iova(fbdev->bo, 0, &paddr);
> if (ret) {
> dev_err(dev->dev, "failed to get buffer obj iova: %d\n",
> ret);
> goto fail_unlock;
> @@ -153,7 +151,7 @@ static int msm_fbdev_create(struct drm_fb_helper
> *helper,
>
> dev->mode_config.fb_base = paddr;
>
> - fbi->screen_base = msm_gem_get_vaddr_locked(fbdev->bo);
> + fbi->screen_base = msm_gem_get_vaddr(fbdev->bo);
> if (IS_ERR(fbi->screen_base)) {
> ret = PTR_ERR(fbi->screen_base);
> goto fail_unlock;
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 68e509b..1e803fb 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -41,8 +41,7 @@ static bool use_pages(struct drm_gem_object *obj)
> }
>
> /* allocate pages from VRAM carveout, used when no IOMMU: */
> -static struct page **get_pages_vram(struct drm_gem_object *obj,
> - int npages)
> +static struct page **get_pages_vram(struct drm_gem_object *obj, int npages)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct msm_drm_private *priv = obj->dev->dev_private;
> @@ -54,7 +53,9 @@ static struct page **get_pages_vram(struct drm_gem_object
> *obj,
> if (!p)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock(&priv->vram.lock);
> ret = drm_mm_insert_node(&priv->vram.mm, msm_obj->vram_node, npages);
> + spin_unlock(&priv->vram.lock);
> if (ret) {
> drm_free_large(p);
> return ERR_PTR(ret);
> @@ -69,7 +70,6 @@ static struct page **get_pages_vram(struct drm_gem_object
> *obj,
> return p;
> }
>
> -/* called with dev->struct_mutex held */
> static struct page **get_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -109,6 +109,18 @@ static struct page **get_pages(struct drm_gem_object
> *obj)
> return msm_obj->pages;
> }
>
> +static void put_pages_vram(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + struct msm_drm_private *priv = obj->dev->dev_private;
> +
> + spin_lock(&priv->vram.lock);
> + drm_mm_remove_node(msm_obj->vram_node);
> + spin_unlock(&priv->vram.lock);
> +
> + drm_free_large(msm_obj->pages);
> +}
> +
> static void put_pages(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -125,10 +137,8 @@ static void put_pages(struct drm_gem_object *obj)
>
> if (use_pages(obj))
> drm_gem_put_pages(obj, msm_obj->pages, true, false);
> - else {
> - drm_mm_remove_node(msm_obj->vram_node);
> - drm_free_large(msm_obj->pages);
> - }
> + else
> + put_pages_vram(obj);
>
> msm_obj->pages = NULL;
> }
> @@ -136,11 +146,12 @@ static void put_pages(struct drm_gem_object *obj)
>
> struct page **msm_gem_get_pages(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **p;
> - mutex_lock(&dev->struct_mutex);
> +
> + mutex_lock(&msm_obj->lock);
> p = get_pages(obj);
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return p;
> }
>
> @@ -195,25 +206,17 @@ int msm_gem_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct drm_gem_object *obj = vma->vm_private_data;
> - struct drm_device *dev = obj->dev;
> - struct msm_drm_private *priv = dev->dev_private;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> struct page **pages;
> unsigned long pfn;
> pgoff_t pgoff;
> int ret;
>
> - /* This should only happen if userspace tries to pass a mmap'd
> - * but unfaulted gem bo vaddr into submit ioctl, triggering
> - * a page fault while struct_mutex is already held. This is
> - * not a valid use-case so just bail.
> + /*
> + * vm_ops.open/drm_gem_mmap_obj and close get and put
> + * a reference on obj. So, we dont need to hold one here.
> */
> - if (priv->struct_mutex_task == current)
> - return VM_FAULT_SIGBUS;
> -
> - /* Make sure we don't parallel update on a fault, nor move or remove
> - * something from beneath our feet
> - */
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> + ret = mutex_lock_interruptible(&msm_obj->lock);
> if (ret)
> goto out;
>
> @@ -235,7 +238,7 @@ int msm_gem_fault(struct vm_fault *vmf)
> ret = vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn,
> PFN_DEV));
>
> out_unlock:
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> out:
> switch (ret) {
> case -EAGAIN:
> @@ -259,9 +262,10 @@ int msm_gem_fault(struct vm_fault *vmf)
> static uint64_t mmap_offset(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> /* Make it mmapable */
> ret = drm_gem_create_mmap_offset(obj);
> @@ -277,21 +281,23 @@ static uint64_t mmap_offset(struct drm_gem_object
> *obj)
> uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
> {
> uint64_t offset;
> - mutex_lock(&obj->dev->struct_mutex);
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +
> + mutex_lock(&msm_obj->lock);
> offset = mmap_offset(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return offset;
> }
>
> +/* Called with msm_obj->lock locked */
> static void
> put_iova(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int id;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>
> for (id = 0; id < ARRAY_SIZE(msm_obj->domain); id++) {
> if (!priv->aspace[id])
> @@ -301,25 +307,23 @@ uint64_t msm_gem_mmap_offset(struct drm_gem_object
> *obj)
> }
> }
>
> -/* should be called under struct_mutex.. although it can be called
> - * from atomic context without struct_mutex to acquire an extra
> - * iova ref if you know one is already held.
> - *
> - * That means when I do eventually need to add support for unpinning
> - * the refcnt counter needs to be atomic_t.
> - */
> -int msm_gem_get_iova_locked(struct drm_gem_object *obj, int id,
> +/* A reference to obj must be held before calling this function. */
> +int msm_gem_get_iova(struct drm_gem_object *obj, int id,
> uint64_t *iova)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> int ret = 0;
>
> + mutex_lock(&msm_obj->lock);
> +
> if (!msm_obj->domain[id].iova) {
> struct msm_drm_private *priv = obj->dev->dev_private;
> struct page **pages = get_pages(obj);
>
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return PTR_ERR(pages);
> + }
>
> if (iommu_present(&platform_bus_type)) {
> ret = msm_gem_map_vma(priv->aspace[id],
> &msm_obj->domain[id],
> @@ -332,26 +336,7 @@ int msm_gem_get_iova_locked(struct drm_gem_object *obj,
> int id,
> if (!ret)
> *iova = msm_obj->domain[id].iova;
>
> - return ret;
> -}
> -
> -/* get iova, taking a reference. Should have a matching put */
> -int msm_gem_get_iova(struct drm_gem_object *obj, int id, uint64_t *iova)
> -{
> - struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - int ret;
> -
> - /* this is safe right now because we don't unmap until the
> - * bo is deleted:
> - */
> - if (msm_obj->domain[id].iova) {
> - *iova = msm_obj->domain[id].iova;
> - return 0;
> - }
> -
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_iova_locked(obj, id, iova);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> return ret;
> }
>
> @@ -405,45 +390,37 @@ int msm_gem_dumb_map_offset(struct drm_file *file,
> struct drm_device *dev,
> return ret;
> }
>
> -void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj)
> +void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> - if (IS_ERR(pages))
> + if (IS_ERR(pages)) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_CAST(pages);
> + }
> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> - if (msm_obj->vaddr == NULL)
> + if (msm_obj->vaddr == NULL) {
> + mutex_unlock(&msm_obj->lock);
> return ERR_PTR(-ENOMEM);
> + }
> }
> msm_obj->vmap_count++;
> + mutex_unlock(&msm_obj->lock);
> return msm_obj->vaddr;
> }
>
> -void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> -{
> - void *ret;
> - mutex_lock(&obj->dev->struct_mutex);
> - ret = msm_gem_get_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> - return ret;
> -}
> -
> -void msm_gem_put_vaddr_locked(struct drm_gem_object *obj)
> +void msm_gem_put_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +
> + mutex_lock(&msm_obj->lock);
> WARN_ON(msm_obj->vmap_count < 1);
> msm_obj->vmap_count--;
> -}
> -
> -void msm_gem_put_vaddr(struct drm_gem_object *obj)
> -{
> - mutex_lock(&obj->dev->struct_mutex);
> - msm_gem_put_vaddr_locked(obj);
> - mutex_unlock(&obj->dev->struct_mutex);
> + mutex_unlock(&msm_obj->lock);
> }
>
> /* Update madvise status, returns true if not purged, else
> @@ -697,6 +674,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> list_del(&msm_obj->mm_list);
>
> + mutex_lock(&msm_obj->lock);
> +
> put_iova(obj);
>
> if (obj->import_attach) {
> @@ -720,6 +699,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>
> drm_gem_object_release(obj);
>
> + mutex_unlock(&msm_obj->lock);
> kfree(msm_obj);
> }
>
> @@ -730,14 +710,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct
> drm_file *file,
> struct drm_gem_object *obj;
> int ret;
>
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> - if (ret)
> - return ret;
> -
> obj = msm_gem_new(dev, size, flags);
>
> - mutex_unlock(&dev->struct_mutex);
> -
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> @@ -752,7 +726,8 @@ int msm_gem_new_handle(struct drm_device *dev, struct
> drm_file *file,
> static int msm_gem_new_impl(struct drm_device *dev,
> uint32_t size, uint32_t flags,
> struct reservation_object *resv,
> - struct drm_gem_object **obj)
> + struct drm_gem_object **obj,
> + bool struct_mutex_locked)
> {
> struct msm_drm_private *priv = dev->dev_private;
> struct msm_gem_object *msm_obj;
> @@ -781,6 +756,8 @@ static int msm_gem_new_impl(struct drm_device *dev,
> if (!msm_obj)
> return -ENOMEM;
>
> + mutex_init(&msm_obj->lock);
> +
> if (use_vram)
> msm_obj->vram_node = &msm_obj->domain[0].node;
>
> @@ -795,21 +772,25 @@ static int msm_gem_new_impl(struct drm_device *dev,
> }
>
> INIT_LIST_HEAD(&msm_obj->submit_entry);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + if (struct_mutex_locked) {
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + } else {
> + mutex_lock(&dev->struct_mutex);
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + mutex_unlock(&dev->struct_mutex);
> + }
>
> *obj = &msm_obj->base;
>
> return 0;
> }
>
> -struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> - uint32_t size, uint32_t flags)
> +static struct drm_gem_object *_msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags, bool struct_mutex_locked)
> {
> struct drm_gem_object *obj = NULL;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> size = PAGE_ALIGN(size);
>
> /* Disallow zero sized objects as they make the underlying
> @@ -818,7 +799,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device
> *dev,
> if (size == 0)
> return ERR_PTR(-EINVAL);
>
> - ret = msm_gem_new_impl(dev, size, flags, NULL, &obj);
> + ret = msm_gem_new_impl(dev, size, flags, NULL, &obj,
> struct_mutex_locked);
> if (ret)
> goto fail;
>
> @@ -833,10 +814,22 @@ struct drm_gem_object *msm_gem_new(struct drm_device
> *dev,
> return obj;
>
> fail:
> - drm_gem_object_unreference(obj);
> + drm_gem_object_unreference_unlocked(obj);
> return ERR_PTR(ret);
> }
>
> +struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, true);
> +}
> +
> +struct drm_gem_object *msm_gem_new(struct drm_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + return _msm_gem_new(dev, size, flags, false);
> +}
> +
> struct drm_gem_object *msm_gem_import(struct drm_device *dev,
> struct dma_buf *dmabuf, struct sg_table *sgt)
> {
> @@ -853,7 +846,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device
> *dev,
>
> size = PAGE_ALIGN(dmabuf->size);
>
> - ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj);
> + ret = msm_gem_new_impl(dev, size, MSM_BO_WC, dmabuf->resv, &obj,
> false);
> if (ret)
> goto fail;
>
> @@ -862,17 +855,22 @@ struct drm_gem_object *msm_gem_import(struct
> drm_device *dev,
> npages = size / PAGE_SIZE;
>
> msm_obj = to_msm_bo(obj);
> + mutex_lock(&msm_obj->lock);
> msm_obj->sgt = sgt;
> msm_obj->pages = drm_malloc_ab(npages, sizeof(struct page *));
> if (!msm_obj->pages) {
> + mutex_unlock(&msm_obj->lock);
> ret = -ENOMEM;
> goto fail;
> }
>
> ret = drm_prime_sg_to_page_addr_arrays(sgt, msm_obj->pages, NULL,
> npages);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&msm_obj->lock);
> goto fail;
> + }
>
> + mutex_unlock(&msm_obj->lock);
> return obj;
>
> fail:
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 1b4cf20..b1bfc10 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -31,6 +31,7 @@ struct msm_gem_address_space {
> * and position mm_node->start is in # of pages:
> */
> struct drm_mm mm;
> + spinlock_t lock; /* Protects drm_mm node allocation/removal */
> struct msm_mmu *mmu;
> struct kref kref;
> };
> @@ -87,6 +88,7 @@ struct msm_gem_object {
> * an IOMMU. Also used for stolen/splashscreen buffer.
> */
> struct drm_mm_node *vram_node;
> + struct mutex lock; /* Protects resources associated with bo */
> };
> #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 1c545eb..8420551 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -245,7 +245,7 @@ static int submit_pin_objects(struct msm_gem_submit
> *submit)
> uint64_t iova;
>
> /* if locking succeeded, pin bo: */
> - ret = msm_gem_get_iova_locked(&msm_obj->base,
> + ret = msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (ret)
> @@ -301,7 +301,7 @@ static int submit_reloc(struct msm_gem_submit *submit,
> struct msm_gem_object *ob
> /* For now, just map the entire thing. Eventually we probably
> * to do it page-by-page, w/ kmap() if not vmap()d..
> */
> - ptr = msm_gem_get_vaddr_locked(&obj->base);
> + ptr = msm_gem_get_vaddr(&obj->base);
>
> if (IS_ERR(ptr)) {
> ret = PTR_ERR(ptr);
> @@ -359,7 +359,7 @@ static int submit_reloc(struct msm_gem_submit *submit,
> struct msm_gem_object *ob
> }
>
> out:
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c
> b/drivers/gpu/drm/msm/msm_gem_vma.c
> index f285d7e..c36321bc 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -50,7 +50,9 @@ void msm_gem_address_space_put(struct
> msm_gem_address_space *aspace)
> aspace->mmu->funcs->unmap(aspace->mmu, vma->iova, sgt, size);
> }
>
> + spin_lock(&aspace->lock);
> drm_mm_remove_node(&vma->node);
> + spin_unlock(&aspace->lock);
>
> vma->iova = 0;
>
> @@ -63,10 +65,15 @@ void msm_gem_address_space_put(struct
> msm_gem_address_space *aspace)
> {
> int ret;
>
> - if (WARN_ON(drm_mm_node_allocated(&vma->node)))
> + spin_lock(&aspace->lock);
> + if (WARN_ON(drm_mm_node_allocated(&vma->node))) {
> + spin_unlock(&aspace->lock);
> return 0;
> + }
>
> ret = drm_mm_insert_node(&aspace->mm, &vma->node, npages);
> + spin_unlock(&aspace->lock);
> +
> if (ret)
> return ret;
>
> @@ -94,6 +101,7 @@ struct msm_gem_address_space *
> if (!aspace)
> return ERR_PTR(-ENOMEM);
>
> + spin_lock_init(&aspace->lock);
> aspace->name = name;
> aspace->mmu = msm_iommu_new(dev, domain);
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 97b9c38..d6e5cb2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -495,7 +495,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
> msm_gem_submit *submit,
>
> /* submit takes a reference to the bo and iova until retired:
> */
> drm_gem_object_reference(&msm_obj->base);
> - msm_gem_get_iova_locked(&msm_obj->base,
> + msm_gem_get_iova(&msm_obj->base,
> submit->gpu->id, &iova);
>
> if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
> @@ -662,9 +662,7 @@ int msm_gpu_init(struct drm_device *drm, struct
> platform_device *pdev,
>
>
> /* Create ringbuffer: */
> - mutex_lock(&drm->struct_mutex);
> gpu->rb = msm_ringbuffer_new(gpu, ringsz);
> - mutex_unlock(&drm->struct_mutex);
> if (IS_ERR(gpu->rb)) {
> ret = PTR_ERR(gpu->rb);
> gpu->rb = NULL;
> diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
> index 0e81faa..0366b80 100644
> --- a/drivers/gpu/drm/msm/msm_rd.c
> +++ b/drivers/gpu/drm/msm/msm_rd.c
> @@ -268,7 +268,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> struct msm_gem_object *obj = submit->bos[idx].obj;
> const char *buf;
>
> - buf = msm_gem_get_vaddr_locked(&obj->base);
> + buf = msm_gem_get_vaddr(&obj->base);
> if (IS_ERR(buf))
> return;
>
> @@ -283,7 +283,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
> (uint32_t[3]){ iova, size, iova >> 32 }, 12);
> rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
>
> - msm_gem_put_vaddr_locked(&obj->base);
> + msm_gem_put_vaddr(&obj->base);
> }
>
> /* called under struct_mutex */
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c
> b/drivers/gpu/drm/msm/msm_ringbuffer.c
> index 67b34e0..791bca3 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> @@ -40,7 +40,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu
> *gpu, int size)
> goto fail;
> }
>
> - ring->start = msm_gem_get_vaddr_locked(ring->bo);
> + ring->start = msm_gem_get_vaddr(ring->bo);
> if (IS_ERR(ring->start)) {
> ret = PTR_ERR(ring->start);
> goto fail;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <CAF6AEGuUNmpbssk0nsf6Nrb3Z-H5ug-gthEkZTqg_9GQAA3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-14 23:08 ` Susheelendra, Sushmita
@ 2017-06-15 10:19 ` Chris Wilson
1 sibling, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2017-06-15 10:19 UTC (permalink / raw)
To: Rob Clark, Sushmita Susheelendra
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Quoting Rob Clark (2017-06-14 17:49:02)
> On Tue, Jun 13, 2017 at 6:52 PM, Sushmita Susheelendra
> <ssusheel@codeaurora.org> wrote:
> > Buffer object specific resources like pages, domains, sg list
> > need not be protected with struct_mutex. They can be protected
> > with a buffer object level lock. This simplifies locking and
> > makes it easier to avoid potential recursive locking scenarios
> > for SVM involving mmap_sem and struct_mutex. This also removes
> > unnecessary serialization when creating buffer objects, and also
> > between buffer object creation and GPU command submission.
>
> I've rebased this on top of the other changes that are in the queue for 4.13:
>
> https://github.com/freedreno/kernel-msm/commit/4e0c4e139a647914cfcf7c413da5c19f9f124885
>
> I've done some light testing on a5xx.. although between this and
> moving gpu->hw_init() under struct_mutex, I need to double check on
> a3xx/a4xx.
>
> I do think we probably need a bit more work for shrinker. In
> particular msm_gem_purge() still assumes everything is protected by a
> single struct_mutex, which is no longer true. The tricky thing here
> is that shrinker can be triggered by code-paths where we already hold
> msm_obj->lock.
Make sure that msm_obj->lock *only* covers the backing store (and its
operations). (e.g. I called it obj->mm.lock and moved everything that it
covers into obj->mm). Then you can ensure that you don't attempt to
shrink msm_obj itself whilst holding msm_obj->mm.lock (e.g. by tracking
a obj->mm.pages_pin_count and not attempting to shrink any object that
has its pages still pinned, and you then only allocate underneath the
msm_obj->mm.lock whilst holding a page reference). After all that you
can use mutex_lock_nested(.subclass = MSM_SHRINKER) for the special case
where the shrinker may be acquiring the lock on other objects whilst
holding msm_obj->mm.lock.
-Chris
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
2017-06-13 22:52 [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex Sushmita Susheelendra
[not found] ` <1497394374-19982-1-git-send-email-ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-06-14 16:49 ` Rob Clark
@ 2017-06-15 13:20 ` Rob Clark
[not found] ` <20170615132050.1196-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2 siblings, 1 reply; 13+ messages in thread
From: Rob Clark @ 2017-06-15 13:20 UTC (permalink / raw)
To: dri-devel
Cc: linux-arm-msm, freedreno, Chris Wilson, Sushmita Susheelendra,
Rob Clark
---
This is roughly based on Chris's suggestion, in particular the part
about using mutex_lock_nested(). It's not *exactly* the same, in
particular msm_obj->lock protects a bit more than just backing store
and we don't currently track a pin_count. (Instead we currently
keep pages pinned until the object is purged or freed.)
Instead of making msm_obj->lock only cover backing store, it is
easier to split out madv, which is still protected by struct_mutex,
which is still held by the shrinker, so the shrinker does not need
to grab msm_obj->lock until it purges an object. We avoid going
down any path that could trigger shrinker by ensuring that
msm_obj->madv == WILLNEED. To synchronize access to msm_obj->madv
it is protected by msm_obj->lock inside struct_mutex.
This seems to keep lockdep happy in my testing so far.
drivers/gpu/drm/msm/msm_gem.c | 54 ++++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/msm_gem.h | 1 +
drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 ++++++++
3 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index e132548..f5d1f84 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -26,6 +26,22 @@
#include "msm_gpu.h"
#include "msm_mmu.h"
+/* The shrinker can be triggered while we hold objA->lock, and need
+ * to grab objB->lock to purge it. Lockdep just sees these as a single
+ * class of lock, so we use subclasses to teach it the difference.
+ *
+ * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
+ * OBJ_LOCK_SHRINKER is used in msm_gem_purge().
+ *
+ * It is *essential* that we never go down paths that could trigger the
+ * shrinker for a purgable object. This is ensured by checking that
+ * msm_obj->madv == MSM_MADV_WILLNEED.
+ */
+enum {
+ OBJ_LOCK_NORMAL,
+ OBJ_LOCK_SHRINKER,
+};
+
static dma_addr_t physaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -150,6 +166,12 @@ struct page **msm_gem_get_pages(struct drm_gem_object *obj)
struct page **p;
mutex_lock(&msm_obj->lock);
+
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return ERR_PTR(-EBUSY);
+ }
+
p = get_pages(obj);
mutex_unlock(&msm_obj->lock);
return p;
@@ -220,6 +242,11 @@ int msm_gem_fault(struct vm_fault *vmf)
if (ret)
goto out;
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return VM_FAULT_SIGBUS;
+ }
+
/* make sure we have pages attached now */
pages = get_pages(obj);
if (IS_ERR(pages)) {
@@ -358,6 +385,11 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
mutex_lock(&msm_obj->lock);
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return -EBUSY;
+ }
+
vma = lookup_vma(obj, aspace);
if (!vma) {
@@ -454,6 +486,12 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
mutex_lock(&msm_obj->lock);
+
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return ERR_PTR(-EBUSY);
+ }
+
if (!msm_obj->vaddr) {
struct page **pages = get_pages(obj);
if (IS_ERR(pages)) {
@@ -489,12 +527,18 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ mutex_lock(&msm_obj->lock);
+
WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
if (msm_obj->madv != __MSM_MADV_PURGED)
msm_obj->madv = madv;
- return (msm_obj->madv != __MSM_MADV_PURGED);
+ madv = msm_obj->madv;
+
+ mutex_unlock(&msm_obj->lock);
+
+ return (madv != __MSM_MADV_PURGED);
}
void msm_gem_purge(struct drm_gem_object *obj)
@@ -506,6 +550,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
WARN_ON(!is_purgeable(msm_obj));
WARN_ON(obj->import_attach);
+ mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
+
put_iova(obj);
msm_gem_vunmap(obj);
@@ -526,6 +572,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
0, (loff_t)-1);
+
+ mutex_unlock(&msm_obj->lock);
}
void msm_gem_vunmap(struct drm_gem_object *obj)
@@ -660,7 +708,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
uint64_t off = drm_vma_node_start(&obj->vma_node);
const char *madv;
- WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
+ mutex_lock(&msm_obj->lock);
switch (msm_obj->madv) {
case __MSM_MADV_PURGED:
@@ -701,6 +749,8 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
if (fence)
describe_fence(fence, "Exclusive", m);
rcu_read_unlock();
+
+ mutex_unlock(&msm_obj->lock);
}
void msm_gem_describe_objects(struct list_head *list, struct seq_file *m)
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 9ad5ba4c..2b9b8e9 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -101,6 +101,7 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
static inline bool is_purgeable(struct msm_gem_object *msm_obj)
{
+ WARN_ON(!mutex_is_locked(&msm_obj->base.dev->struct_mutex));
return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
!msm_obj->base.dma_buf && !msm_obj->base.import_attach;
}
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index ab1dd02..e1db4ad 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -20,6 +20,18 @@
static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
{
+ /* NOTE: we are *closer* to being able to get rid of
+ * mutex_trylock_recursive().. the msm_gem code itself does
+ * not need struct_mutex, although codepaths that can trigger
+ * shrinker are still called in code-paths that hold the
+ * struct_mutex.
+ *
+ * Also, msm_obj->madv is protected by struct_mutex.
+ *
+ * The next step is probably split out a seperate lock for
+ * protecting inactive_list, so that shrinker does not need
+ * struct_mutex.
+ */
switch (mutex_trylock_recursive(&dev->struct_mutex)) {
case MUTEX_TRYLOCK_FAILED:
return false;
--
2.9.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <20170615132050.1196-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-06-15 21:59 ` Susheelendra, Sushmita
[not found] ` <D38DFC6B-2CE4-459F-9D35-C21CEE5B362D-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Susheelendra, Sushmita @ 2017-06-15 21:59 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Chris Wilson
[-- Attachment #1.1: Type: text/plain, Size: 7752 bytes --]
Hi Rob,
I can see how we can trigger the shrinker on objB while holding objA->lock. So, the nested lock with class SHRINKER makes sense.
However, I’m trying to figure how the get_pages/vmap/fault path on an objA can end up triggering the shrinker on objA itself. As objA itself would not be purgeable (msm_obj->sgt would not be set)/vunmappable (msm_obj->vaddr would not be set) yet at that point, we would never end up calling msm_gem_purge or msm_gem_vunmap on objA itself right? If that is the case, we may not need the (msm_obj->madv == MSM_MADV_WILLNEED) check? Or am I missing something here?
I think shrinker_vmap would also need the nested SHRINKER lock before it calls msm_gem_vunmap because a vmap on objA could trigger msm_gem_vunmap on objB.
I really like the idea of protecting priv->inactive_list with a separate lock as that is pretty much why the shrinker needs to hold struct_mutex.
Thanks,
Sushmita
> On Jun 15, 2017, at 7:20 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> ---
> This is roughly based on Chris's suggestion, in particular the part
> about using mutex_lock_nested(). It's not *exactly* the same, in
> particular msm_obj->lock protects a bit more than just backing store
> and we don't currently track a pin_count. (Instead we currently
> keep pages pinned until the object is purged or freed.)
>
> Instead of making msm_obj->lock only cover backing store, it is
> easier to split out madv, which is still protected by struct_mutex,
> which is still held by the shrinker, so the shrinker does not need
> to grab msm_obj->lock until it purges an object. We avoid going
> down any path that could trigger shrinker by ensuring that
> msm_obj->madv == WILLNEED. To synchronize access to msm_obj->madv
> it is protected by msm_obj->lock inside struct_mutex.
>
> This seems to keep lockdep happy in my testing so far.
>
> drivers/gpu/drm/msm/msm_gem.c | 54 ++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/msm_gem.h | 1 +
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 ++++++++
> 3 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index e132548..f5d1f84 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -26,6 +26,22 @@
> #include "msm_gpu.h"
> #include "msm_mmu.h"
>
> +/* The shrinker can be triggered while we hold objA->lock, and need
> + * to grab objB->lock to purge it. Lockdep just sees these as a single
> + * class of lock, so we use subclasses to teach it the difference.
> + *
> + * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
> + * OBJ_LOCK_SHRINKER is used in msm_gem_purge().
> + *
> + * It is *essential* that we never go down paths that could trigger the
> + * shrinker for a purgable object. This is ensured by checking that
> + * msm_obj->madv == MSM_MADV_WILLNEED.
> + */
> +enum {
> + OBJ_LOCK_NORMAL,
> + OBJ_LOCK_SHRINKER,
> +};
> +
> static dma_addr_t physaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -150,6 +166,12 @@ struct page **msm_gem_get_pages(struct drm_gem_object *obj)
> struct page **p;
>
> mutex_lock(&msm_obj->lock);
> +
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return ERR_PTR(-EBUSY);
> + }
> +
> p = get_pages(obj);
> mutex_unlock(&msm_obj->lock);
> return p;
> @@ -220,6 +242,11 @@ int msm_gem_fault(struct vm_fault *vmf)
> if (ret)
> goto out;
>
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return VM_FAULT_SIGBUS;
> + }
> +
> /* make sure we have pages attached now */
> pages = get_pages(obj);
> if (IS_ERR(pages)) {
> @@ -358,6 +385,11 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
>
> mutex_lock(&msm_obj->lock);
>
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return -EBUSY;
> + }
> +
> vma = lookup_vma(obj, aspace);
>
> if (!vma) {
> @@ -454,6 +486,12 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> mutex_lock(&msm_obj->lock);
> +
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return ERR_PTR(-EBUSY);
> + }
> +
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> if (IS_ERR(pages)) {
> @@ -489,12 +527,18 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> + mutex_lock(&msm_obj->lock);
> +
> WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>
> if (msm_obj->madv != __MSM_MADV_PURGED)
> msm_obj->madv = madv;
>
> - return (msm_obj->madv != __MSM_MADV_PURGED);
> + madv = msm_obj->madv;
> +
> + mutex_unlock(&msm_obj->lock);
> +
> + return (madv != __MSM_MADV_PURGED);
> }
>
> void msm_gem_purge(struct drm_gem_object *obj)
> @@ -506,6 +550,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
> WARN_ON(!is_purgeable(msm_obj));
> WARN_ON(obj->import_attach);
>
> + mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
> +
> put_iova(obj);
>
> msm_gem_vunmap(obj);
> @@ -526,6 +572,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
>
> invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
> 0, (loff_t)-1);
> +
> + mutex_unlock(&msm_obj->lock);
> }
>
> void msm_gem_vunmap(struct drm_gem_object *obj)
> @@ -660,7 +708,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
> uint64_t off = drm_vma_node_start(&obj->vma_node);
> const char *madv;
>
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> + mutex_lock(&msm_obj->lock);
>
> switch (msm_obj->madv) {
> case __MSM_MADV_PURGED:
> @@ -701,6 +749,8 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
> if (fence)
> describe_fence(fence, "Exclusive", m);
> rcu_read_unlock();
> +
> + mutex_unlock(&msm_obj->lock);
> }
>
> void msm_gem_describe_objects(struct list_head *list, struct seq_file *m)
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 9ad5ba4c..2b9b8e9 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -101,6 +101,7 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
>
> static inline bool is_purgeable(struct msm_gem_object *msm_obj)
> {
> + WARN_ON(!mutex_is_locked(&msm_obj->base.dev->struct_mutex));
> return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
> !msm_obj->base.dma_buf && !msm_obj->base.import_attach;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index ab1dd02..e1db4ad 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -20,6 +20,18 @@
>
> static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> {
> + /* NOTE: we are *closer* to being able to get rid of
> + * mutex_trylock_recursive().. the msm_gem code itself does
> + * not need struct_mutex, although codepaths that can trigger
> + * shrinker are still called in code-paths that hold the
> + * struct_mutex.
> + *
> + * Also, msm_obj->madv is protected by struct_mutex.
> + *
> + * The next step is probably split out a seperate lock for
> + * protecting inactive_list, so that shrinker does not need
> + * struct_mutex.
> + */
> switch (mutex_trylock_recursive(&dev->struct_mutex)) {
> case MUTEX_TRYLOCK_FAILED:
> return false;
> --
> 2.9.4
>
[-- Attachment #1.2: Type: text/html, Size: 17917 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <D38DFC6B-2CE4-459F-9D35-C21CEE5B362D-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
@ 2017-06-15 22:57 ` Rob Clark
[not found] ` <CAF6AEGst=tPn30N-0u5J8-FkBb=ZKKNkjEBGE-M84TxnC054Vw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Rob Clark @ 2017-06-15 22:57 UTC (permalink / raw)
To: Susheelendra, Sushmita
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Chris Wilson
On Thu, Jun 15, 2017 at 5:59 PM, Susheelendra, Sushmita
<ssusheel@codeaurora.org> wrote:
> Hi Rob,
>
> I can see how we can trigger the shrinker on objB while holding objA->lock.
> So, the nested lock with class SHRINKER makes sense.
> However, I’m trying to figure how the get_pages/vmap/fault path on an objA
> can end up triggering the shrinker on objA itself. As objA itself would not
> be purgeable (msm_obj->sgt would not be set)/vunmappable (msm_obj->vaddr
> would not be set) yet at that point, we would never end up calling
> msm_gem_purge or msm_gem_vunmap on objA itself right? If that is the case,
> we may not need the (msm_obj->madv == MSM_MADV_WILLNEED) check? Or am I
> missing something here?
get_pages() would set msm_obj->sgt.. I guess that is protected by
msm_obj->lock, so maybe it would be safe to drop the WILLNEED check.
Otoh, I think that does make things more clear to include the check
and a bit more future-proof.
We do check is_purgable() outside of msm_obj->lock.. I'd have to think
about it for more than a few seconds, but it seems like keeping the
WILLNEED check is a good idea.
> I think shrinker_vmap would also need the nested SHRINKER lock before it
> calls msm_gem_vunmap because a vmap on objA could trigger msm_gem_vunmap on
> objB.
hmm, right.. I guess I still need to test this w/ 32b build (where I
guess vmap shrinker is more likely), but I think you are right about
needing the nested lock in _vunmap() as well.
> I really like the idea of protecting priv->inactive_list with a separate
> lock as that is pretty much why the shrinker needs to hold struct_mutex.
yeah, rough idea is split out (probably?) spin-locks to protect the
list and madv. Then I think we could drop struct_mutex lock for
shrinker.
I think this first patch is pretty close to being ready in time to
queue up for 4.13 (which I probably need to do this weekend). We
should try and tackle the list+madv locks for 4.14, I think, since
this is already a pretty big change.
BR,
-R
> Thanks,
> Sushmita
>
> On Jun 15, 2017, at 7:20 AM, Rob Clark <robdclark@gmail.com> wrote:
>
> ---
> This is roughly based on Chris's suggestion, in particular the part
> about using mutex_lock_nested(). It's not *exactly* the same, in
> particular msm_obj->lock protects a bit more than just backing store
> and we don't currently track a pin_count. (Instead we currently
> keep pages pinned until the object is purged or freed.)
>
> Instead of making msm_obj->lock only cover backing store, it is
> easier to split out madv, which is still protected by struct_mutex,
> which is still held by the shrinker, so the shrinker does not need
> to grab msm_obj->lock until it purges an object. We avoid going
> down any path that could trigger shrinker by ensuring that
> msm_obj->madv == WILLNEED. To synchronize access to msm_obj->madv
> it is protected by msm_obj->lock inside struct_mutex.
>
> This seems to keep lockdep happy in my testing so far.
>
> drivers/gpu/drm/msm/msm_gem.c | 54
> ++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/msm_gem.h | 1 +
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 ++++++++
> 3 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index e132548..f5d1f84 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -26,6 +26,22 @@
> #include "msm_gpu.h"
> #include "msm_mmu.h"
>
> +/* The shrinker can be triggered while we hold objA->lock, and need
> + * to grab objB->lock to purge it. Lockdep just sees these as a single
> + * class of lock, so we use subclasses to teach it the difference.
> + *
> + * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
> + * OBJ_LOCK_SHRINKER is used in msm_gem_purge().
> + *
> + * It is *essential* that we never go down paths that could trigger the
> + * shrinker for a purgable object. This is ensured by checking that
> + * msm_obj->madv == MSM_MADV_WILLNEED.
> + */
> +enum {
> + OBJ_LOCK_NORMAL,
> + OBJ_LOCK_SHRINKER,
> +};
> +
> static dma_addr_t physaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -150,6 +166,12 @@ struct page **msm_gem_get_pages(struct drm_gem_object
> *obj)
> struct page **p;
>
> mutex_lock(&msm_obj->lock);
> +
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return ERR_PTR(-EBUSY);
> + }
> +
> p = get_pages(obj);
> mutex_unlock(&msm_obj->lock);
> return p;
> @@ -220,6 +242,11 @@ int msm_gem_fault(struct vm_fault *vmf)
> if (ret)
> goto out;
>
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return VM_FAULT_SIGBUS;
> + }
> +
> /* make sure we have pages attached now */
> pages = get_pages(obj);
> if (IS_ERR(pages)) {
> @@ -358,6 +385,11 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
>
> mutex_lock(&msm_obj->lock);
>
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return -EBUSY;
> + }
> +
> vma = lookup_vma(obj, aspace);
>
> if (!vma) {
> @@ -454,6 +486,12 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> mutex_lock(&msm_obj->lock);
> +
> + if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
> + mutex_unlock(&msm_obj->lock);
> + return ERR_PTR(-EBUSY);
> + }
> +
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> if (IS_ERR(pages)) {
> @@ -489,12 +527,18 @@ int msm_gem_madvise(struct drm_gem_object *obj,
> unsigned madv)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> + mutex_lock(&msm_obj->lock);
> +
> WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>
> if (msm_obj->madv != __MSM_MADV_PURGED)
> msm_obj->madv = madv;
>
> - return (msm_obj->madv != __MSM_MADV_PURGED);
> + madv = msm_obj->madv;
> +
> + mutex_unlock(&msm_obj->lock);
> +
> + return (madv != __MSM_MADV_PURGED);
> }
>
> void msm_gem_purge(struct drm_gem_object *obj)
> @@ -506,6 +550,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
> WARN_ON(!is_purgeable(msm_obj));
> WARN_ON(obj->import_attach);
>
> + mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
> +
> put_iova(obj);
>
> msm_gem_vunmap(obj);
> @@ -526,6 +572,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
>
> invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
> 0, (loff_t)-1);
> +
> + mutex_unlock(&msm_obj->lock);
> }
>
> void msm_gem_vunmap(struct drm_gem_object *obj)
> @@ -660,7 +708,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct
> seq_file *m)
> uint64_t off = drm_vma_node_start(&obj->vma_node);
> const char *madv;
>
> - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> + mutex_lock(&msm_obj->lock);
>
> switch (msm_obj->madv) {
> case __MSM_MADV_PURGED:
> @@ -701,6 +749,8 @@ void msm_gem_describe(struct drm_gem_object *obj, struct
> seq_file *m)
> if (fence)
> describe_fence(fence, "Exclusive", m);
> rcu_read_unlock();
> +
> + mutex_unlock(&msm_obj->lock);
> }
>
> void msm_gem_describe_objects(struct list_head *list, struct seq_file *m)
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 9ad5ba4c..2b9b8e9 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -101,6 +101,7 @@ static inline bool is_active(struct msm_gem_object
> *msm_obj)
>
> static inline bool is_purgeable(struct msm_gem_object *msm_obj)
> {
> + WARN_ON(!mutex_is_locked(&msm_obj->base.dev->struct_mutex));
> return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
> !msm_obj->base.dma_buf && !msm_obj->base.import_attach;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index ab1dd02..e1db4ad 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -20,6 +20,18 @@
>
> static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> {
> + /* NOTE: we are *closer* to being able to get rid of
> + * mutex_trylock_recursive().. the msm_gem code itself does
> + * not need struct_mutex, although codepaths that can trigger
> + * shrinker are still called in code-paths that hold the
> + * struct_mutex.
> + *
> + * Also, msm_obj->madv is protected by struct_mutex.
> + *
> + * The next step is probably split out a seperate lock for
> + * protecting inactive_list, so that shrinker does not need
> + * struct_mutex.
> + */
> switch (mutex_trylock_recursive(&dev->struct_mutex)) {
> case MUTEX_TRYLOCK_FAILED:
> return false;
> --
> 2.9.4
>
>
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <CAF6AEGst=tPn30N-0u5J8-FkBb=ZKKNkjEBGE-M84TxnC054Vw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-16 14:22 ` Rob Clark
[not found] ` <20170616142207.20821-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Rob Clark @ 2017-06-16 14:22 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Susheelendra, Sushmita, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
Rob Clark, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Chris Wilson
---
Ok, 2nd fixup to handle vmap shrinking.
drivers/gpu/drm/msm/msm_gem.c | 44 +++++++++++++++++++++++++++++++++++--------
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index f5d1f84..3190e05 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -42,6 +42,9 @@ enum {
OBJ_LOCK_SHRINKER,
};
+static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
+
+
static dma_addr_t physaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
void *msm_gem_get_vaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ int ret = 0;
mutex_lock(&msm_obj->lock);
@@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
return ERR_PTR(-EBUSY);
}
+ /* increment vmap_count *before* vmap() call, so shrinker can
+ * check vmap_count (is_vunmapable()) outside of msm_obj->lock.
+ * This guarantees that we won't try to msm_gem_vunmap() this
+ * same object from within the vmap() call (while we already
+ * hold msm_obj->lock)
+ */
+ msm_obj->vmap_count++;
+
if (!msm_obj->vaddr) {
struct page **pages = get_pages(obj);
if (IS_ERR(pages)) {
- mutex_unlock(&msm_obj->lock);
- return ERR_CAST(pages);
+ ret = PTR_ERR(pages);
+ goto fail;
}
msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
VM_MAP, pgprot_writecombine(PAGE_KERNEL));
if (msm_obj->vaddr == NULL) {
- mutex_unlock(&msm_obj->lock);
- return ERR_PTR(-ENOMEM);
+ ret = -ENOMEM;
+ goto fail;
}
}
- msm_obj->vmap_count++;
+
mutex_unlock(&msm_obj->lock);
return msm_obj->vaddr;
+
+fail:
+ msm_obj->vmap_count--;
+ mutex_unlock(&msm_obj->lock);
+ return ERR_PTR(ret);
}
void msm_gem_put_vaddr(struct drm_gem_object *obj)
@@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
put_iova(obj);
- msm_gem_vunmap(obj);
+ msm_gem_vunmap_locked(obj);
put_pages(obj);
@@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
mutex_unlock(&msm_obj->lock);
}
-void msm_gem_vunmap(struct drm_gem_object *obj)
+static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ WARN_ON(!mutex_is_locked(&msm_obj->lock));
+
if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
return;
@@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
msm_obj->vaddr = NULL;
}
+void msm_gem_vunmap(struct drm_gem_object *obj)
+{
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
+
+ mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
+ msm_gem_vunmap_locked(obj);
+ mutex_unlock(&msm_obj->lock);
+}
+
/* must be called before _move_to_active().. */
int msm_gem_sync_object(struct drm_gem_object *obj,
struct msm_fence_context *fctx, bool exclusive)
@@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
drm_prime_gem_destroy(obj, msm_obj->sgt);
} else {
- msm_gem_vunmap(obj);
+ msm_gem_vunmap_locked(obj);
put_pages(obj);
}
--
2.9.4
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <20170616142207.20821-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-06-16 21:32 ` ssusheel-sgV2jX0FEOL9JmXXK+q4OQ
2017-06-16 21:44 ` [Freedreno] " Rob Clark
0 siblings, 1 reply; 13+ messages in thread
From: ssusheel-sgV2jX0FEOL9JmXXK+q4OQ @ 2017-06-16 21:32 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Chris Wilson
Hi Rob,
This looks good to me!
Just one nit: msm_gem_vunmap becomes very shrinker specific as it holds
the msm_obj->lock with the shrinker class. Should we have the caller
i.e. msm_gem_shrinker_vmap hold it instead? Or change it's name to
msm_gem_vunmap_shrinker or something like that...?
It does make sense to make the madv/priv->inactive_list locking cleanup
separately.
Thanks,
Sushmita
On 2017-06-16 08:22, Rob Clark wrote:
> ---
> Ok, 2nd fixup to handle vmap shrinking.
>
> drivers/gpu/drm/msm/msm_gem.c | 44
> +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c
> b/drivers/gpu/drm/msm/msm_gem.c
> index f5d1f84..3190e05 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -42,6 +42,9 @@ enum {
> OBJ_LOCK_SHRINKER,
> };
>
> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
> +
> +
> static dma_addr_t physaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> @@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file,
> struct drm_device *dev,
> void *msm_gem_get_vaddr(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + int ret = 0;
>
> mutex_lock(&msm_obj->lock);
>
> @@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object
> *obj)
> return ERR_PTR(-EBUSY);
> }
>
> + /* increment vmap_count *before* vmap() call, so shrinker can
> + * check vmap_count (is_vunmapable()) outside of msm_obj->lock.
> + * This guarantees that we won't try to msm_gem_vunmap() this
> + * same object from within the vmap() call (while we already
> + * hold msm_obj->lock)
> + */
> + msm_obj->vmap_count++;
> +
> if (!msm_obj->vaddr) {
> struct page **pages = get_pages(obj);
> if (IS_ERR(pages)) {
> - mutex_unlock(&msm_obj->lock);
> - return ERR_CAST(pages);
> + ret = PTR_ERR(pages);
> + goto fail;
> }
> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> if (msm_obj->vaddr == NULL) {
> - mutex_unlock(&msm_obj->lock);
> - return ERR_PTR(-ENOMEM);
> + ret = -ENOMEM;
> + goto fail;
> }
> }
> - msm_obj->vmap_count++;
> +
> mutex_unlock(&msm_obj->lock);
> return msm_obj->vaddr;
> +
> +fail:
> + msm_obj->vmap_count--;
> + mutex_unlock(&msm_obj->lock);
> + return ERR_PTR(ret);
> }
>
> void msm_gem_put_vaddr(struct drm_gem_object *obj)
> @@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
>
> put_iova(obj);
>
> - msm_gem_vunmap(obj);
> + msm_gem_vunmap_locked(obj);
>
> put_pages(obj);
>
> @@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
> mutex_unlock(&msm_obj->lock);
> }
>
> -void msm_gem_vunmap(struct drm_gem_object *obj)
> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
> +
> if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
> return;
>
> @@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
> msm_obj->vaddr = NULL;
> }
>
> +void msm_gem_vunmap(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +
> + mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
> + msm_gem_vunmap_locked(obj);
> + mutex_unlock(&msm_obj->lock);
> +}
> +
> /* must be called before _move_to_active().. */
> int msm_gem_sync_object(struct drm_gem_object *obj,
> struct msm_fence_context *fctx, bool exclusive)
> @@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object
> *obj)
>
> drm_prime_gem_destroy(obj, msm_obj->sgt);
> } else {
> - msm_gem_vunmap(obj);
> + msm_gem_vunmap_locked(obj);
> put_pages(obj);
> }
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Freedreno] [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
2017-06-16 21:32 ` ssusheel-sgV2jX0FEOL9JmXXK+q4OQ
@ 2017-06-16 21:44 ` Rob Clark
[not found] ` <CAF6AEGvbf_bwDz9CHPd-BhNHFaBngspfyV1OE=m8YAtm+W1T2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Rob Clark @ 2017-06-16 21:44 UTC (permalink / raw)
To: Sushmita Susheelendra
Cc: dri-devel@lists.freedesktop.org, linux-arm-msm, freedreno,
Chris Wilson
On Fri, Jun 16, 2017 at 5:32 PM, <ssusheel@codeaurora.org> wrote:
> Hi Rob,
>
> This looks good to me!
>
> Just one nit: msm_gem_vunmap becomes very shrinker specific as it holds the
> msm_obj->lock with the shrinker class. Should we have the caller i.e.
> msm_gem_shrinker_vmap hold it instead? Or change it's name to
> msm_gem_vunmap_shrinker or something like that...?
Hmm, I suppose I could pass the lock class in to msm_gem_vunmap() in
case we ever want to call it elsewhere (iirc, i915 did something like
that)
BR,
-R
> It does make sense to make the madv/priv->inactive_list locking cleanup
> separately.
>
> Thanks,
> Sushmita
>
>
>
> On 2017-06-16 08:22, Rob Clark wrote:
>>
>> ---
>> Ok, 2nd fixup to handle vmap shrinking.
>>
>> drivers/gpu/drm/msm/msm_gem.c | 44
>> +++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index f5d1f84..3190e05 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -42,6 +42,9 @@ enum {
>> OBJ_LOCK_SHRINKER,
>> };
>>
>> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
>> +
>> +
>> static dma_addr_t physaddr(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> @@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file,
>> struct drm_device *dev,
>> void *msm_gem_get_vaddr(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> + int ret = 0;
>>
>> mutex_lock(&msm_obj->lock);
>>
>> @@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
>> return ERR_PTR(-EBUSY);
>> }
>>
>> + /* increment vmap_count *before* vmap() call, so shrinker can
>> + * check vmap_count (is_vunmapable()) outside of msm_obj->lock.
>> + * This guarantees that we won't try to msm_gem_vunmap() this
>> + * same object from within the vmap() call (while we already
>> + * hold msm_obj->lock)
>> + */
>> + msm_obj->vmap_count++;
>> +
>> if (!msm_obj->vaddr) {
>> struct page **pages = get_pages(obj);
>> if (IS_ERR(pages)) {
>> - mutex_unlock(&msm_obj->lock);
>> - return ERR_CAST(pages);
>> + ret = PTR_ERR(pages);
>> + goto fail;
>> }
>> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
>> VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>> if (msm_obj->vaddr == NULL) {
>> - mutex_unlock(&msm_obj->lock);
>> - return ERR_PTR(-ENOMEM);
>> + ret = -ENOMEM;
>> + goto fail;
>> }
>> }
>> - msm_obj->vmap_count++;
>> +
>> mutex_unlock(&msm_obj->lock);
>> return msm_obj->vaddr;
>> +
>> +fail:
>> + msm_obj->vmap_count--;
>> + mutex_unlock(&msm_obj->lock);
>> + return ERR_PTR(ret);
>> }
>>
>> void msm_gem_put_vaddr(struct drm_gem_object *obj)
>> @@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
>>
>> put_iova(obj);
>>
>> - msm_gem_vunmap(obj);
>> + msm_gem_vunmap_locked(obj);
>>
>> put_pages(obj);
>>
>> @@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
>> mutex_unlock(&msm_obj->lock);
>> }
>>
>> -void msm_gem_vunmap(struct drm_gem_object *obj)
>> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
>> {
>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>
>> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>> +
>> if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
>> return;
>>
>> @@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
>> msm_obj->vaddr = NULL;
>> }
>>
>> +void msm_gem_vunmap(struct drm_gem_object *obj)
>> +{
>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +
>> + mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
>> + msm_gem_vunmap_locked(obj);
>> + mutex_unlock(&msm_obj->lock);
>> +}
>> +
>> /* must be called before _move_to_active().. */
>> int msm_gem_sync_object(struct drm_gem_object *obj,
>> struct msm_fence_context *fctx, bool exclusive)
>> @@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>>
>> drm_prime_gem_destroy(obj, msm_obj->sgt);
>> } else {
>> - msm_gem_vunmap(obj);
>> + msm_gem_vunmap_locked(obj);
>> put_pages(obj);
>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex
[not found] ` <CAF6AEGvbf_bwDz9CHPd-BhNHFaBngspfyV1OE=m8YAtm+W1T2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-16 23:04 ` Sushmita Susheelendra
0 siblings, 0 replies; 13+ messages in thread
From: Sushmita Susheelendra @ 2017-06-16 23:04 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, freedreno,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Chris Wilson
Sounds good.
On 2017-06-16 15:44, Rob Clark wrote:
> On Fri, Jun 16, 2017 at 5:32 PM, <ssusheel@codeaurora.org> wrote:
>> Hi Rob,
>>
>> This looks good to me!
>>
>> Just one nit: msm_gem_vunmap becomes very shrinker specific as it
>> holds the
>> msm_obj->lock with the shrinker class. Should we have the caller i.e.
>> msm_gem_shrinker_vmap hold it instead? Or change it's name to
>> msm_gem_vunmap_shrinker or something like that...?
>
> Hmm, I suppose I could pass the lock class in to msm_gem_vunmap() in
> case we ever want to call it elsewhere (iirc, i915 did something like
> that)
>
> BR,
> -R
>
>> It does make sense to make the madv/priv->inactive_list locking
>> cleanup
>> separately.
>>
>> Thanks,
>> Sushmita
>>
>>
>>
>> On 2017-06-16 08:22, Rob Clark wrote:
>>>
>>> ---
>>> Ok, 2nd fixup to handle vmap shrinking.
>>>
>>> drivers/gpu/drm/msm/msm_gem.c | 44
>>> +++++++++++++++++++++++++++++++++++--------
>>> 1 file changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gem.c
>>> b/drivers/gpu/drm/msm/msm_gem.c
>>> index f5d1f84..3190e05 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>>> @@ -42,6 +42,9 @@ enum {
>>> OBJ_LOCK_SHRINKER,
>>> };
>>>
>>> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
>>> +
>>> +
>>> static dma_addr_t physaddr(struct drm_gem_object *obj)
>>> {
>>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>> @@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file
>>> *file,
>>> struct drm_device *dev,
>>> void *msm_gem_get_vaddr(struct drm_gem_object *obj)
>>> {
>>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>> + int ret = 0;
>>>
>>> mutex_lock(&msm_obj->lock);
>>>
>>> @@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object
>>> *obj)
>>> return ERR_PTR(-EBUSY);
>>> }
>>>
>>> + /* increment vmap_count *before* vmap() call, so shrinker can
>>> + * check vmap_count (is_vunmapable()) outside of
>>> msm_obj->lock.
>>> + * This guarantees that we won't try to msm_gem_vunmap() this
>>> + * same object from within the vmap() call (while we already
>>> + * hold msm_obj->lock)
>>> + */
>>> + msm_obj->vmap_count++;
>>> +
>>> if (!msm_obj->vaddr) {
>>> struct page **pages = get_pages(obj);
>>> if (IS_ERR(pages)) {
>>> - mutex_unlock(&msm_obj->lock);
>>> - return ERR_CAST(pages);
>>> + ret = PTR_ERR(pages);
>>> + goto fail;
>>> }
>>> msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
>>> VM_MAP,
>>> pgprot_writecombine(PAGE_KERNEL));
>>> if (msm_obj->vaddr == NULL) {
>>> - mutex_unlock(&msm_obj->lock);
>>> - return ERR_PTR(-ENOMEM);
>>> + ret = -ENOMEM;
>>> + goto fail;
>>> }
>>> }
>>> - msm_obj->vmap_count++;
>>> +
>>> mutex_unlock(&msm_obj->lock);
>>> return msm_obj->vaddr;
>>> +
>>> +fail:
>>> + msm_obj->vmap_count--;
>>> + mutex_unlock(&msm_obj->lock);
>>> + return ERR_PTR(ret);
>>> }
>>>
>>> void msm_gem_put_vaddr(struct drm_gem_object *obj)
>>> @@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
>>>
>>> put_iova(obj);
>>>
>>> - msm_gem_vunmap(obj);
>>> + msm_gem_vunmap_locked(obj);
>>>
>>> put_pages(obj);
>>>
>>> @@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
>>> mutex_unlock(&msm_obj->lock);
>>> }
>>>
>>> -void msm_gem_vunmap(struct drm_gem_object *obj)
>>> +static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
>>> {
>>> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>>
>>> + WARN_ON(!mutex_is_locked(&msm_obj->lock));
>>> +
>>> if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
>>> return;
>>>
>>> @@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
>>> msm_obj->vaddr = NULL;
>>> }
>>>
>>> +void msm_gem_vunmap(struct drm_gem_object *obj)
>>> +{
>>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>> +
>>> + mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
>>> + msm_gem_vunmap_locked(obj);
>>> + mutex_unlock(&msm_obj->lock);
>>> +}
>>> +
>>> /* must be called before _move_to_active().. */
>>> int msm_gem_sync_object(struct drm_gem_object *obj,
>>> struct msm_fence_context *fctx, bool exclusive)
>>> @@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object
>>> *obj)
>>>
>>> drm_prime_gem_destroy(obj, msm_obj->sgt);
>>> } else {
>>> - msm_gem_vunmap(obj);
>>> + msm_gem_vunmap_locked(obj);
>>> put_pages(obj);
>>> }
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-06-16 23:04 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-13 22:52 [PATCH] drm/msm: Separate locking of buffer resources from struct_mutex Sushmita Susheelendra
[not found] ` <1497394374-19982-1-git-send-email-ssusheel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-06-13 23:10 ` Rob Clark
2017-06-14 16:49 ` Rob Clark
[not found] ` <CAF6AEGuUNmpbssk0nsf6Nrb3Z-H5ug-gthEkZTqg_9GQAA3iUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-14 23:08 ` Susheelendra, Sushmita
2017-06-14 23:31 ` Rob Clark
2017-06-15 10:19 ` Chris Wilson
2017-06-15 13:20 ` [PATCH] fixup! " Rob Clark
[not found] ` <20170615132050.1196-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-06-15 21:59 ` Susheelendra, Sushmita
[not found] ` <D38DFC6B-2CE4-459F-9D35-C21CEE5B362D-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-06-15 22:57 ` Rob Clark
[not found] ` <CAF6AEGst=tPn30N-0u5J8-FkBb=ZKKNkjEBGE-M84TxnC054Vw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-16 14:22 ` Rob Clark
[not found] ` <20170616142207.20821-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-06-16 21:32 ` ssusheel-sgV2jX0FEOL9JmXXK+q4OQ
2017-06-16 21:44 ` [Freedreno] " Rob Clark
[not found] ` <CAF6AEGvbf_bwDz9CHPd-BhNHFaBngspfyV1OE=m8YAtm+W1T2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-16 23:04 ` Sushmita Susheelendra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).