[PATCH 00/27] Etnaviv job lifetime issue fixes

[PATCH 00/27] Etnaviv job lifetime issue fixes

[Lucas Stach]

Hi all,

this series fixes the job (submit) lifetime issues exposed by the addition
of the performance counter sampling. After this series the submits are
properly reference counted and cleanup is moved to one central location,
which makes reasoning about the GPU submit path much easier. Lifetime of
the submit and cmdbuf are now the same, which allows to remove some
duplication that was necessary before due to different lifetimes of those
objects.

I confess that this series does more than strictly necessary to fix the
issue exposed by perfmon, but it also includes a lot of prep work for
other big changes to the submit path, which are still in the works.

Also I've cleaned up little things I mentioned while working my way through
those code paths. One more nice effect of this series is that it unlocks
some more concurrency between submits to different GPUs, so provides some
small performance improvements when running on X.Org, where both the 3D and
2D GPU are used. Benefits might be larger for upcoming SoCs with multiple
GPUs.

Please review. This is the base set of patches I would like to land in 4.16.

Regards,
Lucas

Lucas Stach (27):
  drm/etnaviv: fix GPU vs sync point race
  drm/etnaviv: split obj locks in different classes depending on the obj
    type
  drm/etnaviv: add lockdep annotation for userptr object population
  drm/etnaviv: fold __etnaviv_gem_new into caller
  drm/etnaviv: change return type of etnaviv_gem_obj_add to void
  drm/etnaviv: get rid of userptr worker
  drm/etnaviv: remove -EAGAIN handling from submit path
  drm/etnaviv: remove stale TODO in etnaviv_gpu_submit
  drm/etnaviv: don't flush workqueue in etnaviv_gpu_wait_obj_inactive
  drm/etnaviv: remove switch_context member from etnaviv_gpu
  drm/etnaviv: move workqueue to be per GPU
  drm/etnaviv: hold GPU lock while inserting END command
  drm/etnaviv: add lockdep annotations to buffer manipulation functions
  drm/etnaviv: simplify submit_create
  drm/etnaviv: move object fence attachment to gem_submit path
  drm/etnaviv: rename submit fence to out_fence
  drm/etnaviv: attach in fence to submit and move fence wait to
    fence_sync
  drm/etnaviv: move object unpinning to submit cleanup
  drm/etnaviv: move ww_acquire_ctx out of submit object
  drm/etnaviv: refcount the submit object
  drm/etnaviv: move PMRs to submit object
  drm/etnaviv: move exec_state to submit object
  drm/etnaviv: use submit exec_state for perfmon sampling
  drm/etnaviv: move cmdbuf into submit object
  drm/etnaviv: move GPU active handling to bo pin/unpin
  drm/etnaviv: couple runtime PM management to submit object lifetime
  drm/etnaviv: re-enable perfmon support

 drivers/gpu/drm/etnaviv/etnaviv_buffer.c     |  40 ++++--
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c     |  29 +---
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h     |  18 +--
 drivers/gpu/drm/etnaviv/etnaviv_drv.c        |  22 +--
 drivers/gpu/drm/etnaviv/etnaviv_drv.h        |  14 +-
 drivers/gpu/drm/etnaviv/etnaviv_dump.c       |  23 +--
 drivers/gpu/drm/etnaviv/etnaviv_gem.c        | 197 ++++++--------------------
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  22 +--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c  |   7 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 199 ++++++++++++++------------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 204 +++++++++------------------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  11 +-
 drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c   |   2 +-
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c    |   4 +-
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.h    |   2 +-
 15 files changed, 297 insertions(+), 497 deletions(-)

-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/27] drm/etnaviv: fix GPU vs sync point race

[Lucas Stach]

If the FE is restarted before the sync point event is cleared, the GPU
might trigger a completion IRQ for the next sync point before corrupting
the state of the currently running worker.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index e19cbe05da2a..f0fae218e4aa 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1484,22 +1484,18 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	return ret;
 }
 
-static void etnaviv_process_sync_point(struct etnaviv_gpu *gpu,
-	struct etnaviv_event *event)
-{
-	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
-
-	event->sync_point(gpu, event);
-	etnaviv_gpu_start_fe(gpu, addr + 2, 2);
-}
-
 static void sync_point_worker(struct work_struct *work)
 {
 	struct etnaviv_gpu *gpu = container_of(work, struct etnaviv_gpu,
 					       sync_point_work);
+	struct etnaviv_event *event = &gpu->event[gpu->sync_point_event];
+	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
 
-	etnaviv_process_sync_point(gpu, &gpu->event[gpu->sync_point_event]);
+	event->sync_point(gpu, event);
 	event_free(gpu, gpu->sync_point_event);
+
+	/* restart FE last to avoid GPU and IRQ racing against this worker */
+	etnaviv_gpu_start_fe(gpu, addr + 2, 2);
 }
 
 /*
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/27] drm/etnaviv: split obj locks in different classes depending on the obj type

[Lucas Stach]

Userptr, prime and shmem buffer objects have different lock ordering
requirements. This is mostly due to the fact that we don't allow to mmap
userptr buffers, so we won't ever end up in our fault handler for those,
so some of the code pathes are never called with the mmap_sem held.

To avoid lockdep false positives, split them up into different lock classes.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 7 +++++++
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index daee3f1196df..e3582507963d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -24,6 +24,9 @@
 #include "etnaviv_gpu.h"
 #include "etnaviv_mmu.h"
 
+static struct lock_class_key etnaviv_shm_lock_class;
+static struct lock_class_key etnaviv_userptr_lock_class;
+
 static void etnaviv_gem_scatter_map(struct etnaviv_gem_object *etnaviv_obj)
 {
 	struct drm_device *dev = etnaviv_obj->base.dev;
@@ -653,6 +656,8 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	lockdep_set_class(&to_etnaviv_bo(obj)->lock, &etnaviv_shm_lock_class);
+
 	ret = drm_gem_object_init(dev, obj, size);
 	if (ret == 0) {
 		struct address_space *mapping;
@@ -897,6 +902,8 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		return ret;
 
+	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_userptr_lock_class);
+
 	etnaviv_obj->userptr.ptr = ptr;
 	etnaviv_obj->userptr.task = current;
 	etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index ae884723e9b1..ea87bf87b187 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -19,6 +19,7 @@
 #include "etnaviv_drv.h"
 #include "etnaviv_gem.h"
 
+static struct lock_class_key etnaviv_prime_lock_class;
 
 struct sg_table *etnaviv_gem_prime_get_sg_table(struct drm_gem_object *obj)
 {
@@ -125,6 +126,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 	if (ret < 0)
 		return ERR_PTR(ret);
 
+	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
+
 	npages = size / PAGE_SIZE;
 
 	etnaviv_obj->sgt = sgt;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/27] drm/etnaviv: add lockdep annotation for userptr object population

[Lucas Stach]

The current userptr page population will defer work to a work item if
needed to avoid ever taking the mmap_sem in the direct call path. With
the more fine-grained locking in etnaviv this isn't needed anymore, so
a future commit will simplify this code.

Add a lockdep annotation to validate the assumption that the mmap_sem
can be taken in the direct call path.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index e3582507963d..555a331c51a9 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -804,6 +804,8 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
 	struct mm_struct *mm;
 	int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
 
+	might_lock_read(&current->mm->mmap_sem);
+
 	if (etnaviv_obj->userptr.work) {
 		if (IS_ERR(etnaviv_obj->userptr.work)) {
 			ret = PTR_ERR(etnaviv_obj->userptr.work);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/27] drm/etnaviv: fold __etnaviv_gem_new into caller

[Lucas Stach]

This function has only one caller and it isn't expected that there will
be any more in the future. Folding this function into the caller is
helping the readability.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 555a331c51a9..f75105b7e1a4 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -643,8 +643,9 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	return 0;
 }
 
-static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
-		u32 size, u32 flags)
+/* convenience method to construct a GEM buffer object, and userspace handle */
+int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
+	u32 size, u32 flags, u32 *handle)
 {
 	struct drm_gem_object *obj = NULL;
 	int ret;
@@ -665,7 +666,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 		/*
 		 * Our buffers are kept pinned, so allocating them
 		 * from the MOVABLE zone is a really bad idea, and
-		 * conflicts with CMA.  See coments above new_inode()
+		 * conflicts with CMA. See comments above new_inode()
 		 * why this is required _and_ expected if you're
 		 * going to pin these pages.
 		 */
@@ -677,24 +678,6 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
-	return obj;
-
-fail:
-	drm_gem_object_put_unlocked(obj);
-	return ERR_PTR(ret);
-}
-
-/* convenience method to construct a GEM buffer object, and userspace handle */
-int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
-		u32 size, u32 flags, u32 *handle)
-{
-	struct drm_gem_object *obj;
-	int ret;
-
-	obj = __etnaviv_gem_new(dev, size, flags);
-	if (IS_ERR(obj))
-		return PTR_ERR(obj);
-
 	ret = etnaviv_gem_obj_add(dev, obj);
 	if (ret < 0) {
 		drm_gem_object_put_unlocked(obj);
@@ -704,6 +687,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	ret = drm_gem_handle_create(file, obj, handle);
 
 	/* drop reference from allocate - handle holds it now */
+fail:
 	drm_gem_object_put_unlocked(obj);
 
 	return ret;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/27] drm/etnaviv: change return type of etnaviv_gem_obj_add to void

[Lucas Stach]

This function never fails, as it does nothing more than adding the GEM
object to the global device list. Making this explicit through the void
return type allows to drop some unnecessary error handling.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 16 ++++------------
 drivers/gpu/drm/etnaviv/etnaviv_gem.h       |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  4 +---
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index f75105b7e1a4..a52220eeee45 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -586,7 +586,7 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj)
 	kfree(etnaviv_obj);
 }
 
-int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
+void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
 {
 	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
@@ -594,8 +594,6 @@ int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
 	mutex_lock(&priv->gem_lock);
 	list_add_tail(&etnaviv_obj->gem_node, &priv->gem_list);
 	mutex_unlock(&priv->gem_lock);
-
-	return 0;
 }
 
 static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
@@ -678,11 +676,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		goto fail;
 
-	ret = etnaviv_gem_obj_add(dev, obj);
-	if (ret < 0) {
-		drm_gem_object_put_unlocked(obj);
-		return ret;
-	}
+	etnaviv_gem_obj_add(dev, obj);
 
 	ret = drm_gem_handle_create(file, obj, handle);
 
@@ -895,12 +889,10 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
 	etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
 	get_task_struct(current);
 
-	ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
-	if (ret)
-		goto unreference;
+	etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
 
 	ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
-unreference:
+
 	/* drop reference from allocate - handle holds it now */
 	drm_gem_object_put_unlocked(&etnaviv_obj->base);
 	return ret;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index e437fba1209d..00bd9c851a02 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -117,7 +117,7 @@ int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
 int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
 	struct reservation_object *robj, const struct etnaviv_gem_ops *ops,
 	struct etnaviv_gem_object **res);
-int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
+void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
 struct page **etnaviv_gem_get_pages(struct etnaviv_gem_object *obj);
 void etnaviv_gem_put_pages(struct etnaviv_gem_object *obj);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index ea87bf87b187..5704305d41e6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -142,9 +142,7 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
-	ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
-	if (ret)
-		goto fail;
+	etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
 
 	return &etnaviv_obj->base;
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Lucas Stach]

All code paths which populate userptr BOs are fine with the get_pages
function taking the mmap_sem lock. This allows to get rid of the pretty
involved architecture with a worker being scheduled if the mmap_sem
needs to be taken, but instead call GUP directly and allow it to take
the lock if necessary.

This simplifies the code a lot and removes the possibility of this
function returning -EAGAIN, which complicates object population
handling at the callers.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++-----------------------------
 drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
 2 files changed, 23 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index a52220eeee45..fcc969fa0e69 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
 	return 0;
 }
 
-struct get_pages_work {
-	struct work_struct work;
-	struct mm_struct *mm;
-	struct task_struct *task;
-	struct etnaviv_gem_object *etnaviv_obj;
-};
-
-static struct page **etnaviv_gem_userptr_do_get_pages(
-	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct *mm, struct task_struct *task)
-{
-	int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
-	struct page **pvec;
-	uintptr_t ptr;
-	unsigned int flags = 0;
-
-	pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
-	if (!pvec)
-		return ERR_PTR(-ENOMEM);
-
-	if (!etnaviv_obj->userptr.ro)
-		flags |= FOLL_WRITE;
-
-	pinned = 0;
-	ptr = etnaviv_obj->userptr.ptr;
-
-	down_read(&mm->mmap_sem);
-	while (pinned < npages) {
-		ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
-					    flags, pvec + pinned, NULL, NULL);
-		if (ret < 0)
-			break;
-
-		ptr += ret * PAGE_SIZE;
-		pinned += ret;
-	}
-	up_read(&mm->mmap_sem);
-
-	if (ret < 0) {
-		release_pages(pvec, pinned);
-		kvfree(pvec);
-		return ERR_PTR(ret);
-	}
-
-	return pvec;
-}
-
-static void __etnaviv_gem_userptr_get_pages(struct work_struct *_work)
-{
-	struct get_pages_work *work = container_of(_work, typeof(*work), work);
-	struct etnaviv_gem_object *etnaviv_obj = work->etnaviv_obj;
-	struct page **pvec;
-
-	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj, work->mm, work->task);
-
-	mutex_lock(&etnaviv_obj->lock);
-	if (IS_ERR(pvec)) {
-		etnaviv_obj->userptr.work = ERR_CAST(pvec);
-	} else {
-		etnaviv_obj->userptr.work = NULL;
-		etnaviv_obj->pages = pvec;
-	}
-
-	mutex_unlock(&etnaviv_obj->lock);
-	drm_gem_object_put_unlocked(&etnaviv_obj->base);
-
-	mmput(work->mm);
-	put_task_struct(work->task);
-	kfree(work);
-}
-
 static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
 {
 	struct page **pvec = NULL;
-	struct get_pages_work *work;
-	struct mm_struct *mm;
-	int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
+	struct etnaviv_gem_userptr *userptr = &etnaviv_obj->userptr;
+	int ret, pinned = 0, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
 
 	might_lock_read(&current->mm->mmap_sem);
 
-	if (etnaviv_obj->userptr.work) {
-		if (IS_ERR(etnaviv_obj->userptr.work)) {
-			ret = PTR_ERR(etnaviv_obj->userptr.work);
-			etnaviv_obj->userptr.work = NULL;
-		} else {
-			ret = -EAGAIN;
-		}
-		return ret;
-	}
+	if (userptr->mm != current->mm)
+		return -EPERM;
 
-	mm = get_task_mm(etnaviv_obj->userptr.task);
-	pinned = 0;
-	if (mm == current->mm) {
-		pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
-		if (!pvec) {
-			mmput(mm);
-			return -ENOMEM;
-		}
+	pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+	if (!pvec)
+		return -ENOMEM;
+
+	do {
+		unsigned num_pages = npages - pinned;
+		uint64_t ptr = userptr->ptr + pinned * PAGE_SIZE;
+		struct page **pages = pvec + pinned;
 
-		pinned = __get_user_pages_fast(etnaviv_obj->userptr.ptr, npages,
-					       !etnaviv_obj->userptr.ro, pvec);
-		if (pinned < 0) {
+		ret = get_user_pages_fast(ptr, num_pages,
+					  !userptr->ro ? FOLL_WRITE : 0, pages);
+		if (ret < 0) {
+			release_pages(pvec, pinned);
 			kvfree(pvec);
-			mmput(mm);
-			return pinned;
+			return ret;
 		}
 
-		if (pinned == npages) {
-			etnaviv_obj->pages = pvec;
-			mmput(mm);
-			return 0;
-		}
-	}
-
-	release_pages(pvec, pinned);
-	kvfree(pvec);
-
-	work = kmalloc(sizeof(*work), GFP_KERNEL);
-	if (!work) {
-		mmput(mm);
-		return -ENOMEM;
-	}
-
-	get_task_struct(current);
-	drm_gem_object_get(&etnaviv_obj->base);
-
-	work->mm = mm;
-	work->task = current;
-	work->etnaviv_obj = etnaviv_obj;
+		pinned += ret;
 
-	etnaviv_obj->userptr.work = &work->work;
-	INIT_WORK(&work->work, __etnaviv_gem_userptr_get_pages);
+	} while (pinned < npages);
 
-	etnaviv_queue_work(etnaviv_obj->base.dev, &work->work);
+	etnaviv_obj->pages = pvec;
 
-	return -EAGAIN;
+	return 0;
 }
 
 static void etnaviv_gem_userptr_release(struct etnaviv_gem_object *etnaviv_obj)
@@ -855,7 +755,6 @@ static void etnaviv_gem_userptr_release(struct etnaviv_gem_object *etnaviv_obj)
 		release_pages(etnaviv_obj->pages, npages);
 		kvfree(etnaviv_obj->pages);
 	}
-	put_task_struct(etnaviv_obj->userptr.task);
 }
 
 static int etnaviv_gem_userptr_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
@@ -885,9 +784,8 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
 	lockdep_set_class(&etnaviv_obj->lock, &etnaviv_userptr_lock_class);
 
 	etnaviv_obj->userptr.ptr = ptr;
-	etnaviv_obj->userptr.task = current;
+	etnaviv_obj->userptr.mm = current->mm;
 	etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
-	get_task_struct(current);
 
 	etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 00bd9c851a02..d1a7d040ac97 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -26,8 +26,7 @@ struct etnaviv_gem_object;
 
 struct etnaviv_gem_userptr {
 	uintptr_t ptr;
-	struct task_struct *task;
-	struct work_struct *work;
+	struct mm_struct *mm;
 	bool ro;
 };
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/27] drm/etnaviv: remove -EAGAIN handling from submit path

[Lucas Stach]

Now that the userptr BO handling doesn't rely on the userspace restarting
the submit after object population, there is no need to special case the
-EAGAIN return value anymore.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index ff911541a190..8fa31ab1fb0a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -534,14 +534,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 out:
 	submit_unpin_objects(submit);
 
-	/*
-	 * If we're returning -EAGAIN, it may be due to the userptr code
-	 * wanting to run its workqueue outside of any locks. Flush our
-	 * workqueue to ensure that it is run in a timely manner.
-	 */
-	if (ret == -EAGAIN)
-		flush_workqueue(priv->wq);
-
 err_submit_objects:
 	if (in_fence)
 		dma_fence_put(in_fence);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/27] drm/etnaviv: remove stale TODO in etnaviv_gpu_submit

[Lucas Stach]

Flush and prefetch are properly handled in the buffer code, data endianess
would need much wider changes than adding something to this single function.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index f0fae218e4aa..3dffccfcefce 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1391,15 +1391,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		return ret;
 
 	/*
-	 * TODO
-	 *
-	 * - flush
-	 * - data endian
-	 * - prefetch
-	 *
-	 */
-
-	/*
 	 * if there are performance monitor requests we need to have
 	 * - a sync point to re-configure gpu and process ETNA_PM_PROCESS_PRE
 	 *   requests.
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/27] drm/etnaviv: don't flush workqueue in etnaviv_gpu_wait_obj_inactive

[Lucas Stach]

There is no need to synchronize with oustanding retire jobs if the object
has gone idle. Retire jobs only ever change the object state from active to
idle, not the other way around.

The IOVA put race is uncritical, as the GEM_WAIT ioctl itself is holding
a reference to the GEM object, so the retire worker will not pull the
object into the CPU domain, which is the thing we are trying to guard
against with etnaviv_gpu_wait_obj_inactive.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 3dffccfcefce..ae152bb78f18 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1295,17 +1295,12 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
 	ret = wait_event_interruptible_timeout(gpu->fence_event,
 					       !is_active(etnaviv_obj),
 					       remaining);
-	if (ret > 0) {
-		struct etnaviv_drm_private *priv = gpu->drm->dev_private;
-
-		/* Synchronise with the retire worker */
-		flush_workqueue(priv->wq);
+	if (ret > 0)
 		return 0;
-	} else if (ret == -ERESTARTSYS) {
+	else if (ret == -ERESTARTSYS)
 		return -ERESTARTSYS;
-	} else {
+	else
 		return -ETIMEDOUT;
-	}
 }
 
 int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu)
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/27] drm/etnaviv: remove switch_context member from etnaviv_gpu

[Lucas Stach]

There is no need to store this in the gpu struct. MMU flushes are triggered
correctly in reaction to MMU maps and unmaps, independent of the current ctx
and required pipe switches can be infered from the current and the desired
GPU exec state.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 10 ++++++----
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c    |  8 +-------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h    |  1 -
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index 9e7098e3207f..6ad8972a59cc 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -294,6 +294,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 return_target, return_dwords;
 	u32 link_target, link_dwords;
+	bool switch_context = gpu->exec_state != cmdbuf->exec_state;
 
 	if (drm_debug & DRM_UT_DRIVER)
 		etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
@@ -306,7 +307,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 	 * need to append a mmu flush load state, followed by a new
 	 * link to this buffer - a total of four additional words.
 	 */
-	if (gpu->mmu->need_flush || gpu->switch_context) {
+	if (gpu->mmu->need_flush || switch_context) {
 		u32 target, extra_dwords;
 
 		/* link command */
@@ -321,7 +322,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 		}
 
 		/* pipe switch commands */
-		if (gpu->switch_context)
+		if (switch_context)
 			extra_dwords += 4;
 
 		target = etnaviv_buffer_reserve(gpu, buffer, extra_dwords);
@@ -349,10 +350,9 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 			gpu->mmu->need_flush = false;
 		}
 
-		if (gpu->switch_context) {
+		if (switch_context) {
 			etnaviv_cmd_select_pipe(gpu, buffer, cmdbuf->exec_state);
 			gpu->exec_state = cmdbuf->exec_state;
-			gpu->switch_context = false;
 		}
 
 		/* And the link to the submitted buffer */
@@ -421,4 +421,6 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 
 	if (drm_debug & DRM_UT_DRIVER)
 		etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
+
+	gpu->lastctx = cmdbuf->ctx;
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index ae152bb78f18..65eaa8c10ba2 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1416,12 +1416,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	submit->fence = dma_fence_get(fence);
 	gpu->active_fence = submit->fence->seqno;
 
-	if (gpu->lastctx != cmdbuf->ctx) {
-		gpu->mmu->need_flush = true;
-		gpu->switch_context = true;
-		gpu->lastctx = cmdbuf->ctx;
-	}
-
 	if (cmdbuf->nr_pmrs) {
 		gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;
 		gpu->event[event[1]].cmdbuf = cmdbuf;
@@ -1662,7 +1656,7 @@ static int etnaviv_gpu_hw_resume(struct etnaviv_gpu *gpu)
 	etnaviv_gpu_update_clock(gpu);
 	etnaviv_gpu_hw_init(gpu);
 
-	gpu->switch_context = true;
+	gpu->lastctx = NULL;
 	gpu->exec_state = -1;
 
 	mutex_unlock(&gpu->lock);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 4f10f147297a..15090bb68f5a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -106,7 +106,6 @@ struct etnaviv_gpu {
 	struct mutex lock;
 	struct etnaviv_chip_identity identity;
 	struct etnaviv_file_private *lastctx;
-	bool switch_context;
 
 	/* 'ring'-buffer: */
 	struct etnaviv_cmdbuf *buffer;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/27] drm/etnaviv: move workqueue to be per GPU

[Lucas Stach]

While the etnaviv workqueue needs to be ordered, as we rely on work items
being executed in queuing order, this is only true for a single GPU.
Having a shared workqueue for all GPUs in the system limits concurrency
artificially.

Getting each GPU its own ordered workqueue still meets our ordering
expectations and enables retire workers to run concurrently.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 12 ------------
 drivers/gpu/drm/etnaviv/etnaviv_drv.h | 10 ----------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 18 ++++++++++++++----
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
 4 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 491eddf9b150..ca03b5e4789b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -580,12 +580,6 @@ static int etnaviv_bind(struct device *dev)
 	}
 	drm->dev_private = priv;
 
-	priv->wq = alloc_ordered_workqueue("etnaviv", 0);
-	if (!priv->wq) {
-		ret = -ENOMEM;
-		goto out_wq;
-	}
-
 	mutex_init(&priv->gem_lock);
 	INIT_LIST_HEAD(&priv->gem_list);
 	priv->num_gpus = 0;
@@ -607,9 +601,6 @@ static int etnaviv_bind(struct device *dev)
 out_register:
 	component_unbind_all(dev, drm);
 out_bind:
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
-out_wq:
 	kfree(priv);
 out_unref:
 	drm_dev_unref(drm);
@@ -624,9 +615,6 @@ static void etnaviv_unbind(struct device *dev)
 
 	drm_dev_unregister(drm);
 
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
-
 	component_unbind_all(dev, drm);
 
 	drm->dev_private = NULL;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index d249acb6da08..8668bfd4abd5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -56,18 +56,8 @@ struct etnaviv_drm_private {
 	/* list of GEM objects: */
 	struct mutex gem_lock;
 	struct list_head gem_list;
-
-	struct workqueue_struct *wq;
 };
 
-static inline void etnaviv_queue_work(struct drm_device *dev,
-	struct work_struct *w)
-{
-	struct etnaviv_drm_private *priv = dev->dev_private;
-
-	queue_work(priv->wq, w);
-}
-
 int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		struct drm_file *file);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 65eaa8c10ba2..1e1e34adb07f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -958,7 +958,7 @@ static void recover_worker(struct work_struct *work)
 	pm_runtime_put_autosuspend(gpu->dev);
 
 	/* Retire the buffer objects in a work */
-	etnaviv_queue_work(gpu->drm, &gpu->retire_work);
+	queue_work(gpu->wq, &gpu->retire_work);
 }
 
 static void hangcheck_timer_reset(struct etnaviv_gpu *gpu)
@@ -994,7 +994,7 @@ static void hangcheck_handler(struct timer_list *t)
 		dev_err(gpu->dev, "     completed fence: %u\n", fence);
 		dev_err(gpu->dev, "     active fence: %u\n",
 			gpu->active_fence);
-		etnaviv_queue_work(gpu->drm, &gpu->recover_work);
+		queue_work(gpu->wq, &gpu->recover_work);
 	}
 
 	/* if still more pending work, reset the hangcheck timer: */
@@ -1526,7 +1526,7 @@ static irqreturn_t irq_handler(int irq, void *data)
 
 			if (gpu->event[event].sync_point) {
 				gpu->sync_point_event = event;
-				etnaviv_queue_work(gpu->drm, &gpu->sync_point_work);
+				queue_work(gpu->wq, &gpu->sync_point_work);
 			}
 
 			fence = gpu->event[event].fence;
@@ -1552,7 +1552,7 @@ static irqreturn_t irq_handler(int irq, void *data)
 		}
 
 		/* Retire the buffer objects in a work */
-		etnaviv_queue_work(gpu->drm, &gpu->retire_work);
+		queue_work(gpu->wq, &gpu->retire_work);
 
 		ret = IRQ_HANDLED;
 	}
@@ -1721,12 +1721,19 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 			return PTR_ERR(gpu->cooling);
 	}
 
+	gpu->wq = alloc_ordered_workqueue(dev_name(dev), 0);
+	if (!gpu->wq) {
+		thermal_cooling_device_unregister(gpu->cooling);
+		return -ENOMEM;
+	}
+
 #ifdef CONFIG_PM
 	ret = pm_runtime_get_sync(gpu->dev);
 #else
 	ret = etnaviv_gpu_clk_enable(gpu);
 #endif
 	if (ret < 0) {
+		destroy_workqueue(gpu->wq);
 		thermal_cooling_device_unregister(gpu->cooling);
 		return ret;
 	}
@@ -1760,6 +1767,9 @@ static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
 
 	hangcheck_disable(gpu);
 
+	flush_workqueue(gpu->wq);
+	destroy_workqueue(gpu->wq);
+
 #ifdef CONFIG_PM
 	pm_runtime_get_sync(gpu->dev);
 	pm_runtime_put_sync_suspend(gpu->dev);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 15090bb68f5a..ccef6139cf70 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -106,6 +106,7 @@ struct etnaviv_gpu {
 	struct mutex lock;
 	struct etnaviv_chip_identity identity;
 	struct etnaviv_file_private *lastctx;
+	struct workqueue_struct *wq;
 
 	/* 'ring'-buffer: */
 	struct etnaviv_cmdbuf *buffer;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/27] drm/etnaviv: hold GPU lock while inserting END command

[Lucas Stach]

Inserting the END command when suspending the GPU is changing the
command buffer state, which requires the GPU to be held.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 1e1e34adb07f..85f6ee1da016 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1631,7 +1631,9 @@ static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu)
 {
 	if (gpu->buffer) {
 		/* Replace the last WAIT with END */
+		mutex_lock(&gpu->lock);
 		etnaviv_buffer_end(gpu);
+		mutex_unlock(&gpu->lock);
 
 		/*
 		 * We know that only the FE is busy here, this should
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/27] drm/etnaviv: add lockdep annotations to buffer manipulation functions

[Lucas Stach]

When manipulating the kernel command buffer the GPU mutex must be held, as
otherwise different callers might try to replace the same part of the
buffer, wrecking havok in the GPU execution.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index 6ad8972a59cc..b0e046d8ad2d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -100,6 +100,8 @@ static void etnaviv_cmd_select_pipe(struct etnaviv_gpu *gpu,
 {
 	u32 flush = 0;
 
+	lockdep_assert_held(&gpu->lock);
+
 	/*
 	 * This assumes that if we're switching to 2D, we're switching
 	 * away from 3D, and vice versa.  Hence, if we're switching to
@@ -166,6 +168,8 @@ u16 etnaviv_buffer_init(struct etnaviv_gpu *gpu)
 {
 	struct etnaviv_cmdbuf *buffer = gpu->buffer;
 
+	lockdep_assert_held(&gpu->lock);
+
 	/* initialize buffer */
 	buffer->user_size = 0;
 
@@ -180,6 +184,8 @@ u16 etnaviv_buffer_config_mmuv2(struct etnaviv_gpu *gpu, u32 mtlb_addr, u32 safe
 {
 	struct etnaviv_cmdbuf *buffer = gpu->buffer;
 
+	lockdep_assert_held(&gpu->lock);
+
 	buffer->user_size = 0;
 
 	if (gpu->identity.features & chipFeatures_PIPE_3D) {
@@ -215,6 +221,8 @@ void etnaviv_buffer_end(struct etnaviv_gpu *gpu)
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 link_target, flush = 0;
 
+	lockdep_assert_held(&gpu->lock);
+
 	if (gpu->exec_state == ETNA_PIPE_2D)
 		flush = VIVS_GL_FLUSH_CACHE_PE2D;
 	else if (gpu->exec_state == ETNA_PIPE_3D)
@@ -257,6 +265,8 @@ void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int event)
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 dwords, target;
 
+	lockdep_assert_held(&gpu->lock);
+
 	/*
 	 * We need at most 3 dwords in the return target:
 	 * 1 event + 1 end + 1 wait + 1 link.
@@ -296,6 +306,8 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 	u32 link_target, link_dwords;
 	bool switch_context = gpu->exec_state != cmdbuf->exec_state;
 
+	lockdep_assert_held(&gpu->lock);
+
 	if (drm_debug & DRM_UT_DRIVER)
 		etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/27] drm/etnaviv: simplify submit_create

[Lucas Stach]

Use kzalloc so other code doesn't need to worry about uninitialized members.
Drop the non-standard GFP flags, as we really don't want to fail the submit
when under slight memory pressure. Remove one level of indentation by using
an early return if the allocation failed. Also remove the unused drm device
member.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 -
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 14 +++++---------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index d1a7d040ac97..dc478f014d29 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -101,7 +101,6 @@ struct etnaviv_gem_submit_bo {
  * lasts for the duration of the submit-ioctl.
  */
 struct etnaviv_gem_submit {
-	struct drm_device *dev;
 	struct etnaviv_gpu *gpu;
 	struct ww_acquire_ctx ticket;
 	struct dma_fence *fence;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 8fa31ab1fb0a..51ed34586c10 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -38,17 +38,13 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
 	struct etnaviv_gem_submit *submit;
 	size_t sz = size_vstruct(nr, sizeof(submit->bos[0]), sizeof(*submit));
 
-	submit = kmalloc(sz, GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
-	if (submit) {
-		submit->dev = dev;
-		submit->gpu = gpu;
+	submit = kzalloc(sz, GFP_KERNEL);
+	if (!submit)
+		return NULL;
 
-		/* initially, until copy_from_user() and bo lookup succeeds: */
-		submit->nr_bos = 0;
-		submit->fence = NULL;
+	submit->gpu = gpu;
 
-		ww_acquire_init(&submit->ticket, &reservation_ww_class);
-	}
+	ww_acquire_init(&submit->ticket, &reservation_ww_class);
 
 	return submit;
 }
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 15/27] drm/etnaviv: move object fence attachment to gem_submit path

[Lucas Stach]

The object fencing has nothing to do with the actual GPU buffer submit,
so move it to the gem submit path to have a cleaner split.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  7 -------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 51ed34586c10..72468f11dd16 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -180,6 +180,24 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
 	return ret;
 }
 
+static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
+{
+	int i;
+
+	for (i = 0; i < submit->nr_bos; i++) {
+		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
+
+		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
+			reservation_object_add_excl_fence(etnaviv_obj->resv,
+							  submit->fence);
+		else
+			reservation_object_add_shared_fence(etnaviv_obj->resv,
+							    submit->fence);
+
+		submit_unlock_object(submit, i);
+	}
+}
+
 static void submit_unpin_objects(struct etnaviv_gem_submit *submit)
 {
 	int i;
@@ -335,6 +353,7 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
 
+		/* if the GPU submit failed, objects might still be locked */
 		submit_unlock_object(submit, i);
 		drm_gem_object_put_unlocked(&etnaviv_obj->base);
 	}
@@ -507,6 +526,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto out;
 
+	submit_attach_object_fences(submit);
+
 	cmdbuf = NULL;
 
 	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 85f6ee1da016..d55a2137ee37 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1443,13 +1443,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		etnaviv_gem_mapping_reference(submit->bos[i].mapping);
 		cmdbuf->bo_map[i] = submit->bos[i].mapping;
 		atomic_inc(&etnaviv_obj->gpu_active);
-
-		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
-			reservation_object_add_excl_fence(etnaviv_obj->resv,
-							  fence);
-		else
-			reservation_object_add_shared_fence(etnaviv_obj->resv,
-							    fence);
 	}
 	cmdbuf->nr_bos = submit->nr_bos;
 	hangcheck_timer_reset(gpu);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 16/27] drm/etnaviv: rename submit fence to out_fence

[Lucas Stach]

This is the fence passed out on a sucessful GPU submit. Make the name
more clear.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 12 ++++++------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index dc478f014d29..848daf719cda 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -103,7 +103,7 @@ struct etnaviv_gem_submit_bo {
 struct etnaviv_gem_submit {
 	struct etnaviv_gpu *gpu;
 	struct ww_acquire_ctx ticket;
-	struct dma_fence *fence;
+	struct dma_fence *out_fence;
 	u32 flags;
 	unsigned int nr_bos;
 	struct etnaviv_gem_submit_bo bos[0];
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 72468f11dd16..930577c8794e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -189,10 +189,10 @@ static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
 
 		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
 			reservation_object_add_excl_fence(etnaviv_obj->resv,
-							  submit->fence);
+							  submit->out_fence);
 		else
 			reservation_object_add_shared_fence(etnaviv_obj->resv,
-							    submit->fence);
+							    submit->out_fence);
 
 		submit_unlock_object(submit, i);
 	}
@@ -359,8 +359,8 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	}
 
 	ww_acquire_fini(&submit->ticket);
-	if (submit->fence)
-		dma_fence_put(submit->fence);
+	if (submit->out_fence)
+		dma_fence_put(submit->out_fence);
 	kfree(submit);
 }
 
@@ -537,7 +537,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		 * fence to the sync file here, eliminating the ENOMEM
 		 * possibility at this stage.
 		 */
-		sync_file = sync_file_create(submit->fence);
+		sync_file = sync_file_create(submit->out_fence);
 		if (!sync_file) {
 			ret = -ENOMEM;
 			goto out;
@@ -546,7 +546,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	}
 
 	args->fence_fd = out_fence_fd;
-	args->fence = submit->fence->seqno;
+	args->fence = submit->out_fence->seqno;
 
 out:
 	submit_unpin_objects(submit);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index d55a2137ee37..2f09b746439e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1413,8 +1413,8 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	}
 
 	gpu->event[event[0]].fence = fence;
-	submit->fence = dma_fence_get(fence);
-	gpu->active_fence = submit->fence->seqno;
+	submit->out_fence = dma_fence_get(fence);
+	gpu->active_fence = submit->out_fence->seqno;
 
 	if (cmdbuf->nr_pmrs) {
 		gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 17/27] drm/etnaviv: attach in fence to submit and move fence wait to fence_sync

[Lucas Stach]

Simplifies the cleanup path and moves fence waiting to a central location.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 28 +++++++++++++---------------
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 848daf719cda..21cb3460046f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -103,7 +103,7 @@ struct etnaviv_gem_submit_bo {
 struct etnaviv_gem_submit {
 	struct etnaviv_gpu *gpu;
 	struct ww_acquire_ctx ticket;
-	struct dma_fence *out_fence;
+	struct dma_fence *out_fence, *in_fence;
 	u32 flags;
 	unsigned int nr_bos;
 	struct etnaviv_gem_submit_bo bos[0];
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 930577c8794e..20906c22998c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -177,6 +177,15 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
 			break;
 	}
 
+	if (submit->flags & ETNA_SUBMIT_FENCE_FD_IN) {
+		/*
+		 * Wait if the fence is from a foreign context, or if the fence
+		 * array contains any fence from a foreign context.
+		 */
+		if (!dma_fence_match_context(submit->in_fence, context))
+			ret = dma_fence_wait(submit->in_fence, true);
+	}
+
 	return ret;
 }
 
@@ -359,6 +368,8 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	}
 
 	ww_acquire_fini(&submit->ticket);
+	if (submit->in_fence)
+		dma_fence_put(submit->in_fence);
 	if (submit->out_fence)
 		dma_fence_put(submit->out_fence);
 	kfree(submit);
@@ -375,7 +386,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct etnaviv_gem_submit *submit;
 	struct etnaviv_cmdbuf *cmdbuf;
 	struct etnaviv_gpu *gpu;
-	struct dma_fence *in_fence = NULL;
 	struct sync_file *sync_file = NULL;
 	int out_fence_fd = -1;
 	void *stream;
@@ -485,21 +495,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
-		in_fence = sync_file_get_fence(args->fence_fd);
-		if (!in_fence) {
+		submit->in_fence = sync_file_get_fence(args->fence_fd);
+		if (!submit->in_fence) {
 			ret = -EINVAL;
 			goto err_submit_objects;
 		}
-
-		/*
-		 * Wait if the fence is from a foreign context, or if the fence
-		 * array contains any fence from a foreign context.
-		 */
-		if (!dma_fence_match_context(in_fence, gpu->fence_context)) {
-			ret = dma_fence_wait(in_fence, true);
-			if (ret)
-				goto err_submit_objects;
-		}
 	}
 
 	ret = submit_fence_sync(submit);
@@ -552,8 +552,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	submit_unpin_objects(submit);
 
 err_submit_objects:
-	if (in_fence)
-		dma_fence_put(in_fence);
 	submit_cleanup(submit);
 
 err_submit_cmds:
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 18/27] drm/etnaviv: move object unpinning to submit cleanup

[Lucas Stach]

This is safe to call in all paths, as the BO_PINNED flag tells us if the BO
needs unpinning.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 33 ++++++++++------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 20906c22998c..9b5541207d33 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -207,19 +207,6 @@ static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
 	}
 }
 
-static void submit_unpin_objects(struct etnaviv_gem_submit *submit)
-{
-	int i;
-
-	for (i = 0; i < submit->nr_bos; i++) {
-		if (submit->bos[i].flags & BO_PINNED)
-			etnaviv_gem_mapping_unreference(submit->bos[i].mapping);
-
-		submit->bos[i].mapping = NULL;
-		submit->bos[i].flags &= ~BO_PINNED;
-	}
-}
-
 static int submit_pin_objects(struct etnaviv_gem_submit *submit)
 {
 	int i, ret = 0;
@@ -362,6 +349,13 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
 
+		/* unpin all objects */
+		if (submit->bos[i].flags & BO_PINNED) {
+			etnaviv_gem_mapping_unreference(submit->bos[i].mapping);
+			submit->bos[i].mapping = NULL;
+			submit->bos[i].flags &= ~BO_PINNED;
+		}
+
 		/* if the GPU submit failed, objects might still be locked */
 		submit_unlock_object(submit, i);
 		drm_gem_object_put_unlocked(&etnaviv_obj->base);
@@ -508,23 +502,23 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 
 	ret = submit_pin_objects(submit);
 	if (ret)
-		goto out;
+		goto err_submit_objects;
 
 	ret = submit_reloc(submit, stream, args->stream_size / 4,
 			   relocs, args->nr_relocs);
 	if (ret)
-		goto out;
+		goto err_submit_objects;
 
 	ret = submit_perfmon_validate(submit, cmdbuf, pmrs, args->nr_pmrs);
 	if (ret)
-		goto out;
+		goto err_submit_objects;
 
 	memcpy(cmdbuf->vaddr, stream, args->stream_size);
 	cmdbuf->user_size = ALIGN(args->stream_size, 8);
 
 	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
 	if (ret)
-		goto out;
+		goto err_submit_objects;
 
 	submit_attach_object_fences(submit);
 
@@ -540,7 +534,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		sync_file = sync_file_create(submit->out_fence);
 		if (!sync_file) {
 			ret = -ENOMEM;
-			goto out;
+			goto err_submit_objects;
 		}
 		fd_install(out_fence_fd, sync_file->file);
 	}
@@ -548,9 +542,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	args->fence_fd = out_fence_fd;
 	args->fence = submit->out_fence->seqno;
 
-out:
-	submit_unpin_objects(submit);
-
 err_submit_objects:
 	submit_cleanup(submit);
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 19/27] drm/etnaviv: move ww_acquire_ctx out of submit object

[Lucas Stach]

The acquire_ctx is special in that it needs to be released from the same
thread as has been used to initialize it. This collides with the intention to
extend the submit lifetime beyond the gem_submit function with potentially
other threads doing the final cleanup.

Move the ww_acquire_ctx to the function local stack as suggested in the
documentation.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 -
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 22 +++++++++++++---------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 21cb3460046f..6b78d059ed2d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -102,7 +102,6 @@ struct etnaviv_gem_submit_bo {
  */
 struct etnaviv_gem_submit {
 	struct etnaviv_gpu *gpu;
-	struct ww_acquire_ctx ticket;
 	struct dma_fence *out_fence, *in_fence;
 	u32 flags;
 	unsigned int nr_bos;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 9b5541207d33..3090a46979af 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -44,8 +44,6 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
 
 	submit->gpu = gpu;
 
-	ww_acquire_init(&submit->ticket, &reservation_ww_class);
-
 	return submit;
 }
 
@@ -107,7 +105,8 @@ static void submit_unlock_object(struct etnaviv_gem_submit *submit, int i)
 	}
 }
 
-static int submit_lock_objects(struct etnaviv_gem_submit *submit)
+static int submit_lock_objects(struct etnaviv_gem_submit *submit,
+		struct ww_acquire_ctx *ticket)
 {
 	int contended, slow_locked = -1, i, ret = 0;
 
@@ -122,7 +121,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
 
 		if (!(submit->bos[i].flags & BO_LOCKED)) {
 			ret = ww_mutex_lock_interruptible(&etnaviv_obj->resv->lock,
-					&submit->ticket);
+							  ticket);
 			if (ret == -EALREADY)
 				DRM_ERROR("BO at index %u already on submit list\n",
 					  i);
@@ -132,7 +131,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
 		}
 	}
 
-	ww_acquire_done(&submit->ticket);
+	ww_acquire_done(ticket);
 
 	return 0;
 
@@ -150,7 +149,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
 
 		/* we lost out in a seqno race, lock and retry.. */
 		ret = ww_mutex_lock_slow_interruptible(&etnaviv_obj->resv->lock,
-				&submit->ticket);
+						       ticket);
 		if (!ret) {
 			submit->bos[contended].flags |= BO_LOCKED;
 			slow_locked = contended;
@@ -361,7 +360,6 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 		drm_gem_object_put_unlocked(&etnaviv_obj->base);
 	}
 
-	ww_acquire_fini(&submit->ticket);
 	if (submit->in_fence)
 		dma_fence_put(submit->in_fence);
 	if (submit->out_fence)
@@ -381,6 +379,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct etnaviv_cmdbuf *cmdbuf;
 	struct etnaviv_gpu *gpu;
 	struct sync_file *sync_file = NULL;
+	struct ww_acquire_ctx ticket;
 	int out_fence_fd = -1;
 	void *stream;
 	int ret;
@@ -466,10 +465,12 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		}
 	}
 
+	ww_acquire_init(&ticket, &reservation_ww_class);
+
 	submit = submit_create(dev, gpu, args->nr_bos);
 	if (!submit) {
 		ret = -ENOMEM;
-		goto err_submit_cmds;
+		goto err_submit_ww_acquire;
 	}
 
 	submit->flags = args->flags;
@@ -478,7 +479,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto err_submit_objects;
 
-	ret = submit_lock_objects(submit);
+	ret = submit_lock_objects(submit, &ticket);
 	if (ret)
 		goto err_submit_objects;
 
@@ -545,6 +546,9 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 err_submit_objects:
 	submit_cleanup(submit);
 
+err_submit_ww_acquire:
+	ww_acquire_fini(&ticket);
+
 err_submit_cmds:
 	if (ret && (out_fence_fd >= 0))
 		put_unused_fd(out_fence_fd);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 20/27] drm/etnaviv: refcount the submit object

[Lucas Stach]

The submit object lifetime will get extended to the actual GPU
execution. As multiple users will depend on this, add a kref to
properly control destuction of the object.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 +++
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 12 ++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 6b78d059ed2d..4238f8d8541d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -101,6 +101,7 @@ struct etnaviv_gem_submit_bo {
  * lasts for the duration of the submit-ioctl.
  */
 struct etnaviv_gem_submit {
+	struct kref refcount;
 	struct etnaviv_gpu *gpu;
 	struct dma_fence *out_fence, *in_fence;
 	u32 flags;
@@ -109,6 +110,8 @@ struct etnaviv_gem_submit {
 	/* No new members here, the previous one is variable-length! */
 };
 
+void etnaviv_submit_put(struct etnaviv_gem_submit * submit);
+
 int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
 	struct timespec *timeout);
 int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 3090a46979af..9b4436c380ea 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -43,6 +43,7 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
 		return NULL;
 
 	submit->gpu = gpu;
+	kref_init(&submit->refcount);
 
 	return submit;
 }
@@ -341,8 +342,10 @@ static int submit_perfmon_validate(struct etnaviv_gem_submit *submit,
 	return 0;
 }
 
-static void submit_cleanup(struct etnaviv_gem_submit *submit)
+static void submit_cleanup(struct kref *kref)
 {
+	struct etnaviv_gem_submit *submit =
+			container_of(kref, struct etnaviv_gem_submit, refcount);
 	unsigned i;
 
 	for (i = 0; i < submit->nr_bos; i++) {
@@ -367,6 +370,11 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	kfree(submit);
 }
 
+void etnaviv_submit_put(struct etnaviv_gem_submit *submit)
+{
+	kref_put(&submit->refcount, submit_cleanup);
+}
+
 int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
@@ -544,7 +552,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	args->fence = submit->out_fence->seqno;
 
 err_submit_objects:
-	submit_cleanup(submit);
+	etnaviv_submit_put(submit);
 
 err_submit_ww_acquire:
 	ww_acquire_fini(&ticket);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 21/27] drm/etnaviv: move PMRs to submit object

[Lucas Stach]

To make them available to the event worker even after the actual
command stream execution has finished.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c     | 14 +---------
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h     |  5 +---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  5 ++--
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 40 ++++++++++++++++------------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 27 ++++++++++---------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  2 +-
 6 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
index 66ac79558bbd..4ce4639d028d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
@@ -88,10 +88,9 @@ void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc)
 
 struct etnaviv_cmdbuf *
 etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
-		   size_t nr_bos, size_t nr_pmrs)
+		   size_t nr_bos)
 {
 	struct etnaviv_cmdbuf *cmdbuf;
-	struct etnaviv_perfmon_request *pmrs;
 	size_t sz = size_vstruct(nr_bos, sizeof(cmdbuf->bo_map[0]),
 				 sizeof(*cmdbuf));
 	int granule_offs, order, ret;
@@ -100,12 +99,6 @@ etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
 	if (!cmdbuf)
 		return NULL;
 
-	sz = sizeof(*pmrs) * nr_pmrs;
-	pmrs = kzalloc(sz, GFP_KERNEL);
-	if (!pmrs)
-		goto out_free_cmdbuf;
-
-	cmdbuf->pmrs = pmrs;
 	cmdbuf->suballoc = suballoc;
 	cmdbuf->size = size;
 
@@ -132,10 +125,6 @@ etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
 	cmdbuf->vaddr = suballoc->vaddr + cmdbuf->suballoc_offset;
 
 	return cmdbuf;
-
-out_free_cmdbuf:
-	kfree(cmdbuf);
-	return NULL;
 }
 
 void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
@@ -151,7 +140,6 @@ void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
 	suballoc->free_space = 1;
 	mutex_unlock(&suballoc->lock);
 	wake_up_all(&suballoc->free_event);
-	kfree(cmdbuf->pmrs);
 	kfree(cmdbuf);
 }
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
index b6348b9f2a9d..930b9c7f2249 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
@@ -39,9 +39,6 @@ struct etnaviv_cmdbuf {
 	u32 exec_state;
 	/* per GPU in-flight list */
 	struct list_head node;
-	/* perfmon requests */
-	unsigned int nr_pmrs;
-	struct etnaviv_perfmon_request *pmrs;
 	/* BOs attached to this command buffer */
 	unsigned int nr_bos;
 	struct etnaviv_vram_mapping *bo_map[0];
@@ -53,7 +50,7 @@ void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc);
 
 struct etnaviv_cmdbuf *
 etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
-		   size_t nr_bos, size_t nr_pmrs);
+		   size_t nr_bos);
 void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf);
 
 u32 etnaviv_cmdbuf_get_va(struct etnaviv_cmdbuf *buf);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 4238f8d8541d..f525fd1ef63c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -97,14 +97,15 @@ struct etnaviv_gem_submit_bo {
 
 /* Created per submit-ioctl, to track bo's and cmdstream bufs, etc,
  * associated with the cmdstream submission for synchronization (and
- * make it easier to unwind when things go wrong, etc).  This only
- * lasts for the duration of the submit-ioctl.
+ * make it easier to unwind when things go wrong, etc).
  */
 struct etnaviv_gem_submit {
 	struct kref refcount;
 	struct etnaviv_gpu *gpu;
 	struct dma_fence *out_fence, *in_fence;
 	u32 flags;
+	unsigned int nr_pmrs;
+	struct etnaviv_perfmon_request *pmrs;
 	unsigned int nr_bos;
 	struct etnaviv_gem_submit_bo bos[0];
 	/* No new members here, the previous one is variable-length! */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 9b4436c380ea..c8c576993c62 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -33,15 +33,23 @@
 #define BO_PINNED   0x2000
 
 static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
-		struct etnaviv_gpu *gpu, size_t nr)
+		struct etnaviv_gpu *gpu, size_t nr_bos, size_t nr_pmrs)
 {
 	struct etnaviv_gem_submit *submit;
-	size_t sz = size_vstruct(nr, sizeof(submit->bos[0]), sizeof(*submit));
+	size_t sz = size_vstruct(nr_bos, sizeof(submit->bos[0]), sizeof(*submit));
 
 	submit = kzalloc(sz, GFP_KERNEL);
 	if (!submit)
 		return NULL;
 
+	submit->pmrs = kcalloc(nr_pmrs, sizeof(struct etnaviv_perfmon_request),
+			       GFP_KERNEL);
+	if (!submit->pmrs) {
+		kfree(submit);
+		return NULL;
+	}
+	submit->nr_pmrs = nr_pmrs;
+
 	submit->gpu = gpu;
 	kref_init(&submit->refcount);
 
@@ -295,13 +303,11 @@ static int submit_reloc(struct etnaviv_gem_submit *submit, void *stream,
 }
 
 static int submit_perfmon_validate(struct etnaviv_gem_submit *submit,
-		struct etnaviv_cmdbuf *cmdbuf,
-		const struct drm_etnaviv_gem_submit_pmr *pmrs,
-		u32 nr_pms)
+		u32 exec_state, const struct drm_etnaviv_gem_submit_pmr *pmrs)
 {
 	u32 i;
 
-	for (i = 0; i < nr_pms; i++) {
+	for (i = 0; i < submit->nr_pmrs; i++) {
 		const struct drm_etnaviv_gem_submit_pmr *r = pmrs + i;
 		struct etnaviv_gem_submit_bo *bo;
 		int ret;
@@ -326,17 +332,17 @@ static int submit_perfmon_validate(struct etnaviv_gem_submit *submit,
 			return -EINVAL;
 		}
 
-		if (etnaviv_pm_req_validate(r, cmdbuf->exec_state)) {
+		if (etnaviv_pm_req_validate(r, exec_state)) {
 			DRM_ERROR("perfmon request: domain or signal not valid");
 			return -EINVAL;
 		}
 
-		cmdbuf->pmrs[i].flags = r->flags;
-		cmdbuf->pmrs[i].domain = r->domain;
-		cmdbuf->pmrs[i].signal = r->signal;
-		cmdbuf->pmrs[i].sequence = r->sequence;
-		cmdbuf->pmrs[i].offset = r->read_offset;
-		cmdbuf->pmrs[i].bo_vma = etnaviv_gem_vmap(&bo->obj->base);
+		submit->pmrs[i].flags = r->flags;
+		submit->pmrs[i].domain = r->domain;
+		submit->pmrs[i].signal = r->signal;
+		submit->pmrs[i].sequence = r->sequence;
+		submit->pmrs[i].offset = r->read_offset;
+		submit->pmrs[i].bo_vma = etnaviv_gem_vmap(&bo->obj->base);
 	}
 
 	return 0;
@@ -367,6 +373,7 @@ static void submit_cleanup(struct kref *kref)
 		dma_fence_put(submit->in_fence);
 	if (submit->out_fence)
 		dma_fence_put(submit->out_fence);
+	kfree(submit->pmrs);
 	kfree(submit);
 }
 
@@ -427,7 +434,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	stream = kvmalloc_array(1, args->stream_size, GFP_KERNEL);
 	cmdbuf = etnaviv_cmdbuf_new(gpu->cmdbuf_suballoc,
 				    ALIGN(args->stream_size, 8) + 8,
-				    args->nr_bos, args->nr_pmrs);
+				    args->nr_bos);
 	if (!bos || !relocs || !pmrs || !stream || !cmdbuf) {
 		ret = -ENOMEM;
 		goto err_submit_cmds;
@@ -456,7 +463,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		ret = -EFAULT;
 		goto err_submit_cmds;
 	}
-	cmdbuf->nr_pmrs = args->nr_pmrs;
 
 	ret = copy_from_user(stream, u64_to_user_ptr(args->stream),
 			     args->stream_size);
@@ -475,7 +481,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 
 	ww_acquire_init(&ticket, &reservation_ww_class);
 
-	submit = submit_create(dev, gpu, args->nr_bos);
+	submit = submit_create(dev, gpu, args->nr_bos, args->nr_pmrs);
 	if (!submit) {
 		ret = -ENOMEM;
 		goto err_submit_ww_acquire;
@@ -518,7 +524,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto err_submit_objects;
 
-	ret = submit_perfmon_validate(submit, cmdbuf, pmrs, args->nr_pmrs);
+	ret = submit_perfmon_validate(submit, args->exec_state, pmrs);
 	if (ret)
 		goto err_submit_objects;
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 2f09b746439e..6951ba560573 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -717,7 +717,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
 	}
 
 	/* Create buffer: */
-	gpu->buffer = etnaviv_cmdbuf_new(gpu->cmdbuf_suballoc, PAGE_SIZE, 0, 0);
+	gpu->buffer = etnaviv_cmdbuf_new(gpu->cmdbuf_suballoc, PAGE_SIZE, 0);
 	if (!gpu->buffer) {
 		ret = -ENOMEM;
 		dev_err(gpu->dev, "could not create command buffer\n");
@@ -1317,11 +1317,11 @@ void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu)
 static void sync_point_perfmon_sample(struct etnaviv_gpu *gpu,
 	struct etnaviv_event *event, unsigned int flags)
 {
-	const struct etnaviv_cmdbuf *cmdbuf = event->cmdbuf;
+	const struct etnaviv_gem_submit *submit = event->submit;
 	unsigned int i;
 
-	for (i = 0; i < cmdbuf->nr_pmrs; i++) {
-		const struct etnaviv_perfmon_request *pmr = cmdbuf->pmrs + i;
+	for (i = 0; i < submit->nr_pmrs; i++) {
+		const struct etnaviv_perfmon_request *pmr = submit->pmrs + i;
 
 		if (pmr->flags == flags)
 			etnaviv_perfmon_process(gpu, pmr);
@@ -1349,14 +1349,14 @@ static void sync_point_perfmon_sample_pre(struct etnaviv_gpu *gpu,
 static void sync_point_perfmon_sample_post(struct etnaviv_gpu *gpu,
 	struct etnaviv_event *event)
 {
-	const struct etnaviv_cmdbuf *cmdbuf = event->cmdbuf;
+	const struct etnaviv_gem_submit *submit = event->submit;
 	unsigned int i;
 	u32 val;
 
 	sync_point_perfmon_sample(gpu, event, ETNA_PM_PROCESS_POST);
 
-	for (i = 0; i < cmdbuf->nr_pmrs; i++) {
-		const struct etnaviv_perfmon_request *pmr = cmdbuf->pmrs + i;
+	for (i = 0; i < submit->nr_pmrs; i++) {
+		const struct etnaviv_perfmon_request *pmr = submit->pmrs + i;
 
 		*pmr->bo_vma = pmr->sequence;
 	}
@@ -1392,7 +1392,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	 * - a sync point to re-configure gpu, process ETNA_PM_PROCESS_POST requests
 	 *   and update the sequence number for userspace.
 	 */
-	if (cmdbuf->nr_pmrs)
+	if (submit->nr_pmrs)
 		nr_events = 3;
 
 	ret = event_alloc(gpu, nr_events, event);
@@ -1416,17 +1416,19 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	submit->out_fence = dma_fence_get(fence);
 	gpu->active_fence = submit->out_fence->seqno;
 
-	if (cmdbuf->nr_pmrs) {
+	if (submit->nr_pmrs) {
 		gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;
-		gpu->event[event[1]].cmdbuf = cmdbuf;
+		kref_get(&submit->refcount);
+		gpu->event[event[1]].submit = submit;
 		etnaviv_sync_point_queue(gpu, event[1]);
 	}
 
 	etnaviv_buffer_queue(gpu, event[0], cmdbuf);
 
-	if (cmdbuf->nr_pmrs) {
+	if (submit->nr_pmrs) {
 		gpu->event[event[2]].sync_point = &sync_point_perfmon_sample_post;
-		gpu->event[event[2]].cmdbuf = cmdbuf;
+		kref_get(&submit->refcount);
+		gpu->event[event[2]].submit = submit;
 		etnaviv_sync_point_queue(gpu, event[2]);
 	}
 
@@ -1465,6 +1467,7 @@ static void sync_point_worker(struct work_struct *work)
 	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
 
 	event->sync_point(gpu, event);
+	etnaviv_submit_put(event->submit);
 	event_free(gpu, gpu->sync_point_event);
 
 	/* restart FE last to avoid GPU and IRQ racing against this worker */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index ccef6139cf70..eea823838b5f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -89,7 +89,7 @@ struct etnaviv_chip_identity {
 
 struct etnaviv_event {
 	struct dma_fence *fence;
-	struct etnaviv_cmdbuf *cmdbuf;
+	struct etnaviv_gem_submit *submit;
 
 	void (*sync_point)(struct etnaviv_gpu *gpu, struct etnaviv_event *event);
 };
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 22/27] drm/etnaviv: move exec_state to submit object

[Lucas Stach]

We'll need this in some places where only the submit is available. Also
this is a first step at slimming down the cmdbuf object.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c     | 10 +++++-----
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h     |  2 --
 drivers/gpu/drm/etnaviv/etnaviv_drv.h        |  4 ++--
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 +
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  2 +-
 6 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index b0e046d8ad2d..ccb9562a3d82 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -297,14 +297,14 @@ void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int event)
 }
 
 /* Append a command buffer to the ring buffer. */
-void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
-	struct etnaviv_cmdbuf *cmdbuf)
+void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
+	unsigned int event, struct etnaviv_cmdbuf *cmdbuf)
 {
 	struct etnaviv_cmdbuf *buffer = gpu->buffer;
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 return_target, return_dwords;
 	u32 link_target, link_dwords;
-	bool switch_context = gpu->exec_state != cmdbuf->exec_state;
+	bool switch_context = gpu->exec_state != exec_state;
 
 	lockdep_assert_held(&gpu->lock);
 
@@ -363,8 +363,8 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
 		}
 
 		if (switch_context) {
-			etnaviv_cmd_select_pipe(gpu, buffer, cmdbuf->exec_state);
-			gpu->exec_state = cmdbuf->exec_state;
+			etnaviv_cmd_select_pipe(gpu, buffer, exec_state);
+			gpu->exec_state = exec_state;
 		}
 
 		/* And the link to the submitted buffer */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
index 930b9c7f2249..26460e5d252e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
@@ -35,8 +35,6 @@ struct etnaviv_cmdbuf {
 	u32 user_size;
 	/* fence after which this buffer is to be disposed */
 	struct dma_fence *fence;
-	/* target exec state */
-	u32 exec_state;
 	/* per GPU in-flight list */
 	struct list_head node;
 	/* BOs attached to this command buffer */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 8668bfd4abd5..a54f0b758a5c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -87,8 +87,8 @@ u16 etnaviv_buffer_init(struct etnaviv_gpu *gpu);
 u16 etnaviv_buffer_config_mmuv2(struct etnaviv_gpu *gpu, u32 mtlb_addr, u32 safe_addr);
 void etnaviv_buffer_end(struct etnaviv_gpu *gpu);
 void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int event);
-void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
-	struct etnaviv_cmdbuf *cmdbuf);
+void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
+	unsigned int event, struct etnaviv_cmdbuf *cmdbuf);
 void etnaviv_validate_init(void);
 bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu,
 	u32 *stream, unsigned int size,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index f525fd1ef63c..63285101850c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -103,6 +103,7 @@ struct etnaviv_gem_submit {
 	struct kref refcount;
 	struct etnaviv_gpu *gpu;
 	struct dma_fence *out_fence, *in_fence;
+	u32 exec_state;
 	u32 flags;
 	unsigned int nr_pmrs;
 	struct etnaviv_perfmon_request *pmrs;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index c8c576993c62..d87a7f00bca3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -440,7 +440,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_cmds;
 	}
 
-	cmdbuf->exec_state = args->exec_state;
 	cmdbuf->ctx = file->driver_priv;
 
 	ret = copy_from_user(bos, u64_to_user_ptr(args->bos),
@@ -487,6 +486,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_ww_acquire;
 	}
 
+	submit->exec_state = args->exec_state;
 	submit->flags = args->flags;
 
 	ret = submit_lookup_objects(submit, file, bos, args->nr_bos);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 6951ba560573..3d7fe6ed555d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1423,7 +1423,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		etnaviv_sync_point_queue(gpu, event[1]);
 	}
 
-	etnaviv_buffer_queue(gpu, event[0], cmdbuf);
+	etnaviv_buffer_queue(gpu, submit->exec_state, event[0], cmdbuf);
 
 	if (submit->nr_pmrs) {
 		gpu->event[event[2]].sync_point = &sync_point_perfmon_sample_post;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 23/27] drm/etnaviv: use submit exec_state for perfmon sampling

[Lucas Stach]

The GPU exec state may have changed at the time when the perfmon sampling
is done, as it reflects the state of the last submission, not the current
GPU execution state.

So for proper sampling we must use the submit exec_state.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c     | 2 +-
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 4 ++--
 drivers/gpu/drm/etnaviv/etnaviv_perfmon.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 3d7fe6ed555d..8059a12410fa 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1324,7 +1324,7 @@ static void sync_point_perfmon_sample(struct etnaviv_gpu *gpu,
 		const struct etnaviv_perfmon_request *pmr = submit->pmrs + i;
 
 		if (pmr->flags == flags)
-			etnaviv_perfmon_process(gpu, pmr);
+			etnaviv_perfmon_process(gpu, pmr, submit->exec_state);
 	}
 }
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
index 768f5aafdd18..26dddfc41aac 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
@@ -479,9 +479,9 @@ int etnaviv_pm_req_validate(const struct drm_etnaviv_gem_submit_pmr *r,
 }
 
 void etnaviv_perfmon_process(struct etnaviv_gpu *gpu,
-	const struct etnaviv_perfmon_request *pmr)
+	const struct etnaviv_perfmon_request *pmr, u32 exec_state)
 {
-	const struct etnaviv_pm_domain_meta *meta = &doms_meta[gpu->exec_state];
+	const struct etnaviv_pm_domain_meta *meta = &doms_meta[exec_state];
 	const struct etnaviv_pm_domain *dom;
 	const struct etnaviv_pm_signal *sig;
 	u32 *bo = pmr->bo_vma;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.h b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.h
index 35dce194cb00..c1653c64ab6b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.h
@@ -44,6 +44,6 @@ int etnaviv_pm_req_validate(const struct drm_etnaviv_gem_submit_pmr *r,
 	u32 exec_state);
 
 void etnaviv_perfmon_process(struct etnaviv_gpu *gpu,
-	const struct etnaviv_perfmon_request *pmr);
+	const struct etnaviv_perfmon_request *pmr, u32 exec_state);
 
 #endif /* __ETNAVIV_PERFMON_H__ */
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 24/27] drm/etnaviv: move cmdbuf into submit object

[Lucas Stach]

Less dynamic allocations and slims down the cmdbuf object to only the
required information, as everything else is already available in the
submit object.

This also simplifies buffer and mappings lifetime management, as they
are now exlusively attached to the submit object and not additionally
to the cmdbuf.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c     | 10 ++--
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c     | 17 ++-----
 drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h     | 13 ++----
 drivers/gpu/drm/etnaviv/etnaviv_drv.c        |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_dump.c       | 23 +++++-----
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 28 ++++++------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 68 +++++++++++-----------------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  7 +--
 drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c   |  2 +-
 10 files changed, 72 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index ccb9562a3d82..99ad2f073c6e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -166,7 +166,7 @@ static u32 etnaviv_buffer_reserve(struct etnaviv_gpu *gpu,
 
 u16 etnaviv_buffer_init(struct etnaviv_gpu *gpu)
 {
-	struct etnaviv_cmdbuf *buffer = gpu->buffer;
+	struct etnaviv_cmdbuf *buffer = &gpu->buffer;
 
 	lockdep_assert_held(&gpu->lock);
 
@@ -182,7 +182,7 @@ u16 etnaviv_buffer_init(struct etnaviv_gpu *gpu)
 
 u16 etnaviv_buffer_config_mmuv2(struct etnaviv_gpu *gpu, u32 mtlb_addr, u32 safe_addr)
 {
-	struct etnaviv_cmdbuf *buffer = gpu->buffer;
+	struct etnaviv_cmdbuf *buffer = &gpu->buffer;
 
 	lockdep_assert_held(&gpu->lock);
 
@@ -217,7 +217,7 @@ u16 etnaviv_buffer_config_mmuv2(struct etnaviv_gpu *gpu, u32 mtlb_addr, u32 safe
 
 void etnaviv_buffer_end(struct etnaviv_gpu *gpu)
 {
-	struct etnaviv_cmdbuf *buffer = gpu->buffer;
+	struct etnaviv_cmdbuf *buffer = &gpu->buffer;
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 link_target, flush = 0;
 
@@ -261,7 +261,7 @@ void etnaviv_buffer_end(struct etnaviv_gpu *gpu)
 /* Append a 'sync point' to the ring buffer. */
 void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int event)
 {
-	struct etnaviv_cmdbuf *buffer = gpu->buffer;
+	struct etnaviv_cmdbuf *buffer = &gpu->buffer;
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 dwords, target;
 
@@ -300,7 +300,7 @@ void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int event)
 void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
 	unsigned int event, struct etnaviv_cmdbuf *cmdbuf)
 {
-	struct etnaviv_cmdbuf *buffer = gpu->buffer;
+	struct etnaviv_cmdbuf *buffer = &gpu->buffer;
 	unsigned int waitlink_offset = buffer->user_size - 16;
 	u32 return_target, return_dwords;
 	u32 link_target, link_dwords;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
index 4ce4639d028d..3746827f45eb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
@@ -86,19 +86,11 @@ void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc)
 	kfree(suballoc);
 }
 
-struct etnaviv_cmdbuf *
-etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
-		   size_t nr_bos)
+int etnaviv_cmdbuf_init(struct etnaviv_cmdbuf_suballoc *suballoc,
+			struct etnaviv_cmdbuf *cmdbuf, u32 size)
 {
-	struct etnaviv_cmdbuf *cmdbuf;
-	size_t sz = size_vstruct(nr_bos, sizeof(cmdbuf->bo_map[0]),
-				 sizeof(*cmdbuf));
 	int granule_offs, order, ret;
 
-	cmdbuf = kzalloc(sz, GFP_KERNEL);
-	if (!cmdbuf)
-		return NULL;
-
 	cmdbuf->suballoc = suballoc;
 	cmdbuf->size = size;
 
@@ -116,7 +108,7 @@ etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
 		if (!ret) {
 			dev_err(suballoc->gpu->dev,
 				"Timeout waiting for cmdbuf space\n");
-			return NULL;
+			return -ETIMEDOUT;
 		}
 		goto retry;
 	}
@@ -124,7 +116,7 @@ etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
 	cmdbuf->suballoc_offset = granule_offs * SUBALLOC_GRANULE;
 	cmdbuf->vaddr = suballoc->vaddr + cmdbuf->suballoc_offset;
 
-	return cmdbuf;
+	return 0;
 }
 
 void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
@@ -140,7 +132,6 @@ void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
 	suballoc->free_space = 1;
 	mutex_unlock(&suballoc->lock);
 	wake_up_all(&suballoc->free_event);
-	kfree(cmdbuf);
 }
 
 u32 etnaviv_cmdbuf_get_va(struct etnaviv_cmdbuf *buf)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
index 26460e5d252e..ddc3f7ea169c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
@@ -33,22 +33,15 @@ struct etnaviv_cmdbuf {
 	void *vaddr;
 	u32 size;
 	u32 user_size;
-	/* fence after which this buffer is to be disposed */
-	struct dma_fence *fence;
-	/* per GPU in-flight list */
-	struct list_head node;
-	/* BOs attached to this command buffer */
-	unsigned int nr_bos;
-	struct etnaviv_vram_mapping *bo_map[0];
 };
 
 struct etnaviv_cmdbuf_suballoc *
 etnaviv_cmdbuf_suballoc_new(struct etnaviv_gpu * gpu);
 void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc);
 
-struct etnaviv_cmdbuf *
-etnaviv_cmdbuf_new(struct etnaviv_cmdbuf_suballoc *suballoc, u32 size,
-		   size_t nr_bos);
+
+int etnaviv_cmdbuf_init(struct etnaviv_cmdbuf_suballoc *suballoc,
+		struct etnaviv_cmdbuf *cmdbuf, u32 size);
 void etnaviv_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf);
 
 u32 etnaviv_cmdbuf_get_va(struct etnaviv_cmdbuf *buf);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index ca03b5e4789b..f2948885859d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -172,7 +172,7 @@ static int etnaviv_mmu_show(struct etnaviv_gpu *gpu, struct seq_file *m)
 
 static void etnaviv_buffer_dump(struct etnaviv_gpu *gpu, struct seq_file *m)
 {
-	struct etnaviv_cmdbuf *buf = gpu->buffer;
+	struct etnaviv_cmdbuf *buf = &gpu->buffer;
 	u32 size = buf->size;
 	u32 *ptr = buf->vaddr;
 	u32 i;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
index 2d955d7d7b6d..6d0909c589d1 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
@@ -120,7 +120,7 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
 	struct core_dump_iterator iter;
 	struct etnaviv_vram_mapping *vram;
 	struct etnaviv_gem_object *obj;
-	struct etnaviv_cmdbuf *cmd;
+	struct etnaviv_gem_submit *submit;
 	unsigned int n_obj, n_bomap_pages;
 	size_t file_size, mmu_size;
 	__le64 *bomap, *bomap_start;
@@ -132,11 +132,11 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
 	n_bomap_pages = 0;
 	file_size = ARRAY_SIZE(etnaviv_dump_registers) *
 			sizeof(struct etnaviv_dump_registers) +
-		    mmu_size + gpu->buffer->size;
+		    mmu_size + gpu->buffer.size;
 
 	/* Add in the active command buffers */
-	list_for_each_entry(cmd, &gpu->active_cmd_list, node) {
-		file_size += cmd->size;
+	list_for_each_entry(submit, &gpu->active_submit_list, node) {
+		file_size += submit->cmdbuf.size;
 		n_obj++;
 	}
 
@@ -176,13 +176,14 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
 
 	etnaviv_core_dump_registers(&iter, gpu);
 	etnaviv_core_dump_mmu(&iter, gpu, mmu_size);
-	etnaviv_core_dump_mem(&iter, ETDUMP_BUF_RING, gpu->buffer->vaddr,
-			      gpu->buffer->size,
-			      etnaviv_cmdbuf_get_va(gpu->buffer));
-
-	list_for_each_entry(cmd, &gpu->active_cmd_list, node)
-		etnaviv_core_dump_mem(&iter, ETDUMP_BUF_CMD, cmd->vaddr,
-				      cmd->size, etnaviv_cmdbuf_get_va(cmd));
+	etnaviv_core_dump_mem(&iter, ETDUMP_BUF_RING, gpu->buffer.vaddr,
+			      gpu->buffer.size,
+			      etnaviv_cmdbuf_get_va(&gpu->buffer));
+
+	list_for_each_entry(submit, &gpu->active_submit_list, node)
+		etnaviv_core_dump_mem(&iter, ETDUMP_BUF_CMD,
+				      submit->cmdbuf.vaddr, submit->cmdbuf.size,
+				      etnaviv_cmdbuf_get_va(&submit->cmdbuf));
 
 	/* Reserve space for the bomap */
 	if (n_bomap_pages) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 63285101850c..fe99f0c0d068 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -18,6 +18,7 @@
 #define __ETNAVIV_GEM_H__
 
 #include <linux/reservation.h>
+#include "etnaviv_cmdbuf.h"
 #include "etnaviv_drv.h"
 
 struct dma_fence;
@@ -103,6 +104,8 @@ struct etnaviv_gem_submit {
 	struct kref refcount;
 	struct etnaviv_gpu *gpu;
 	struct dma_fence *out_fence, *in_fence;
+	struct list_head node; /* GPU active submit list */
+	struct etnaviv_cmdbuf cmdbuf;
 	u32 exec_state;
 	u32 flags;
 	unsigned int nr_pmrs;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index d87a7f00bca3..db40cd1c14fd 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -354,6 +354,9 @@ static void submit_cleanup(struct kref *kref)
 			container_of(kref, struct etnaviv_gem_submit, refcount);
 	unsigned i;
 
+	if (submit->cmdbuf.suballoc)
+		etnaviv_cmdbuf_free(&submit->cmdbuf);
+
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
 
@@ -391,7 +394,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct drm_etnaviv_gem_submit_pmr *pmrs;
 	struct drm_etnaviv_gem_submit_bo *bos;
 	struct etnaviv_gem_submit *submit;
-	struct etnaviv_cmdbuf *cmdbuf;
 	struct etnaviv_gpu *gpu;
 	struct sync_file *sync_file = NULL;
 	struct ww_acquire_ctx ticket;
@@ -432,16 +434,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	relocs = kvmalloc_array(args->nr_relocs, sizeof(*relocs), GFP_KERNEL);
 	pmrs = kvmalloc_array(args->nr_pmrs, sizeof(*pmrs), GFP_KERNEL);
 	stream = kvmalloc_array(1, args->stream_size, GFP_KERNEL);
-	cmdbuf = etnaviv_cmdbuf_new(gpu->cmdbuf_suballoc,
-				    ALIGN(args->stream_size, 8) + 8,
-				    args->nr_bos);
-	if (!bos || !relocs || !pmrs || !stream || !cmdbuf) {
+	if (!bos || !relocs || !pmrs || !stream) {
 		ret = -ENOMEM;
 		goto err_submit_cmds;
 	}
 
-	cmdbuf->ctx = file->driver_priv;
-
 	ret = copy_from_user(bos, u64_to_user_ptr(args->bos),
 			     args->nr_bos * sizeof(*bos));
 	if (ret) {
@@ -486,6 +483,12 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_ww_acquire;
 	}
 
+	ret = etnaviv_cmdbuf_init(gpu->cmdbuf_suballoc, &submit->cmdbuf,
+				  ALIGN(args->stream_size, 8) + 8);
+	if (ret)
+		goto err_submit_objects;
+
+	submit->cmdbuf.ctx = file->driver_priv;
 	submit->exec_state = args->exec_state;
 	submit->flags = args->flags;
 
@@ -528,17 +531,15 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto err_submit_objects;
 
-	memcpy(cmdbuf->vaddr, stream, args->stream_size);
-	cmdbuf->user_size = ALIGN(args->stream_size, 8);
+	memcpy(submit->cmdbuf.vaddr, stream, args->stream_size);
+	submit->cmdbuf.user_size = ALIGN(args->stream_size, 8);
 
-	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
+	ret = etnaviv_gpu_submit(gpu, submit);
 	if (ret)
 		goto err_submit_objects;
 
 	submit_attach_object_fences(submit);
 
-	cmdbuf = NULL;
-
 	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
 		/*
 		 * This can be improved: ideally we want to allocate the sync
@@ -566,9 +567,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 err_submit_cmds:
 	if (ret && (out_fence_fd >= 0))
 		put_unused_fd(out_fence_fd);
-	/* if we still own the cmdbuf */
-	if (cmdbuf)
-		etnaviv_cmdbuf_free(cmdbuf);
 	if (stream)
 		kvfree(stream);
 	if (bos)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 8059a12410fa..93b9d2541502 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -644,7 +644,7 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
 	prefetch = etnaviv_buffer_init(gpu);
 
 	gpu_write(gpu, VIVS_HI_INTR_ENBL, ~0U);
-	etnaviv_gpu_start_fe(gpu, etnaviv_cmdbuf_get_va(gpu->buffer),
+	etnaviv_gpu_start_fe(gpu, etnaviv_cmdbuf_get_va(&gpu->buffer),
 			     prefetch);
 }
 
@@ -717,15 +717,15 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
 	}
 
 	/* Create buffer: */
-	gpu->buffer = etnaviv_cmdbuf_new(gpu->cmdbuf_suballoc, PAGE_SIZE, 0);
-	if (!gpu->buffer) {
-		ret = -ENOMEM;
+	ret = etnaviv_cmdbuf_init(gpu->cmdbuf_suballoc, &gpu->buffer,
+				  PAGE_SIZE);
+	if (ret) {
 		dev_err(gpu->dev, "could not create command buffer\n");
 		goto destroy_iommu;
 	}
 
 	if (gpu->mmu->version == ETNAVIV_IOMMU_V1 &&
-	    etnaviv_cmdbuf_get_va(gpu->buffer) > 0x80000000) {
+	    etnaviv_cmdbuf_get_va(&gpu->buffer) > 0x80000000) {
 		ret = -EINVAL;
 		dev_err(gpu->dev,
 			"command buffer outside valid memory window\n");
@@ -751,8 +751,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
 	return 0;
 
 free_buffer:
-	etnaviv_cmdbuf_free(gpu->buffer);
-	gpu->buffer = NULL;
+	etnaviv_cmdbuf_free(&gpu->buffer);
 destroy_iommu:
 	etnaviv_iommu_destroy(gpu->mmu);
 	gpu->mmu = NULL;
@@ -1201,27 +1200,20 @@ static void retire_worker(struct work_struct *work)
 	struct etnaviv_gpu *gpu = container_of(work, struct etnaviv_gpu,
 					       retire_work);
 	u32 fence = gpu->completed_fence;
-	struct etnaviv_cmdbuf *cmdbuf, *tmp;
+	struct etnaviv_gem_submit *submit, *tmp;
 	unsigned int i;
 
 	mutex_lock(&gpu->lock);
-	list_for_each_entry_safe(cmdbuf, tmp, &gpu->active_cmd_list, node) {
-		if (!dma_fence_is_signaled(cmdbuf->fence))
+	list_for_each_entry_safe(submit, tmp, &gpu->active_submit_list, node) {
+		if (!dma_fence_is_signaled(submit->out_fence))
 			break;
 
-		list_del(&cmdbuf->node);
-		dma_fence_put(cmdbuf->fence);
+		list_del(&submit->node);
 
-		for (i = 0; i < cmdbuf->nr_bos; i++) {
-			struct etnaviv_vram_mapping *mapping = cmdbuf->bo_map[i];
-			struct etnaviv_gem_object *etnaviv_obj = mapping->object;
-
-			atomic_dec(&etnaviv_obj->gpu_active);
-			/* drop the refcount taken in etnaviv_gpu_submit */
-			etnaviv_gem_mapping_unreference(mapping);
-		}
+		for (i = 0; i < submit->nr_bos; i++)
+			atomic_dec(&submit->bos[i].obj->gpu_active);
 
-		etnaviv_cmdbuf_free(cmdbuf);
+		etnaviv_submit_put(submit);
 		/*
 		 * We need to balance the runtime PM count caused by
 		 * each submission.  Upon submission, we increment
@@ -1375,9 +1367,8 @@ static void sync_point_perfmon_sample_post(struct etnaviv_gpu *gpu,
 
 /* add bo's to gpu's ring, and kick gpu: */
 int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
-	struct etnaviv_gem_submit *submit, struct etnaviv_cmdbuf *cmdbuf)
+	struct etnaviv_gem_submit *submit)
 {
-	struct dma_fence *fence;
 	unsigned int i, nr_events = 1, event[3];
 	int ret;
 
@@ -1403,8 +1394,8 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 
 	mutex_lock(&gpu->lock);
 
-	fence = etnaviv_gpu_fence_alloc(gpu);
-	if (!fence) {
+	submit->out_fence = etnaviv_gpu_fence_alloc(gpu);
+	if (!submit->out_fence) {
 		for (i = 0; i < nr_events; i++)
 			event_free(gpu, event[i]);
 
@@ -1412,8 +1403,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		goto out_unlock;
 	}
 
-	gpu->event[event[0]].fence = fence;
-	submit->out_fence = dma_fence_get(fence);
 	gpu->active_fence = submit->out_fence->seqno;
 
 	if (submit->nr_pmrs) {
@@ -1423,7 +1412,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		etnaviv_sync_point_queue(gpu, event[1]);
 	}
 
-	etnaviv_buffer_queue(gpu, submit->exec_state, event[0], cmdbuf);
+	kref_get(&submit->refcount);
+	gpu->event[event[0]].fence = submit->out_fence;
+	etnaviv_buffer_queue(gpu, submit->exec_state, event[0],
+			     &submit->cmdbuf);
 
 	if (submit->nr_pmrs) {
 		gpu->event[event[2]].sync_point = &sync_point_perfmon_sample_post;
@@ -1432,21 +1424,15 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		etnaviv_sync_point_queue(gpu, event[2]);
 	}
 
-	cmdbuf->fence = fence;
-	list_add_tail(&cmdbuf->node, &gpu->active_cmd_list);
+	list_add_tail(&submit->node, &gpu->active_submit_list);
 
 	/* We're committed to adding this command buffer, hold a PM reference */
 	pm_runtime_get_noresume(gpu->dev);
 
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
-
-		/* Each cmdbuf takes a refcount on the mapping */
-		etnaviv_gem_mapping_reference(submit->bos[i].mapping);
-		cmdbuf->bo_map[i] = submit->bos[i].mapping;
 		atomic_inc(&etnaviv_obj->gpu_active);
 	}
-	cmdbuf->nr_bos = submit->nr_bos;
 	hangcheck_timer_reset(gpu);
 	ret = 0;
 
@@ -1625,7 +1611,7 @@ int etnaviv_gpu_wait_idle(struct etnaviv_gpu *gpu, unsigned int timeout_ms)
 
 static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu)
 {
-	if (gpu->buffer) {
+	if (gpu->buffer.suballoc) {
 		/* Replace the last WAIT with END */
 		mutex_lock(&gpu->lock);
 		etnaviv_buffer_end(gpu);
@@ -1740,7 +1726,7 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	gpu->fence_context = dma_fence_context_alloc(1);
 	spin_lock_init(&gpu->fence_spinlock);
 
-	INIT_LIST_HEAD(&gpu->active_cmd_list);
+	INIT_LIST_HEAD(&gpu->active_submit_list);
 	INIT_WORK(&gpu->retire_work, retire_worker);
 	INIT_WORK(&gpu->sync_point_work, sync_point_worker);
 	INIT_WORK(&gpu->recover_work, recover_worker);
@@ -1775,10 +1761,8 @@ static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
 	etnaviv_gpu_hw_suspend(gpu);
 #endif
 
-	if (gpu->buffer) {
-		etnaviv_cmdbuf_free(gpu->buffer);
-		gpu->buffer = NULL;
-	}
+	if (gpu->buffer.suballoc)
+		etnaviv_cmdbuf_free(&gpu->buffer);
 
 	if (gpu->cmdbuf_suballoc) {
 		etnaviv_cmdbuf_suballoc_destroy(gpu->cmdbuf_suballoc);
@@ -1915,7 +1899,7 @@ static int etnaviv_gpu_rpm_resume(struct device *dev)
 		return ret;
 
 	/* Re-initialise the basic hardware state */
-	if (gpu->drm && gpu->buffer) {
+	if (gpu->drm && gpu->buffer.suballoc) {
 		ret = etnaviv_gpu_hw_resume(gpu);
 		if (ret) {
 			etnaviv_gpu_clk_disable(gpu);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index eea823838b5f..7623905210dc 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -20,6 +20,7 @@
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
 
+#include "etnaviv_cmdbuf.h"
 #include "etnaviv_drv.h"
 
 struct etnaviv_gem_submit;
@@ -109,7 +110,7 @@ struct etnaviv_gpu {
 	struct workqueue_struct *wq;
 
 	/* 'ring'-buffer: */
-	struct etnaviv_cmdbuf *buffer;
+	struct etnaviv_cmdbuf buffer;
 	int exec_state;
 
 	/* bus base address of memory  */
@@ -122,7 +123,7 @@ struct etnaviv_gpu {
 	spinlock_t event_spinlock;
 
 	/* list of currently in-flight command buffers */
-	struct list_head active_cmd_list;
+	struct list_head active_submit_list;
 
 	u32 idle_mask;
 
@@ -202,7 +203,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
 int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
 	struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout);
 int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
-	struct etnaviv_gem_submit *submit, struct etnaviv_cmdbuf *cmdbuf);
+	struct etnaviv_gem_submit *submit);
 int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu);
 void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_wait_idle(struct etnaviv_gpu *gpu, unsigned int timeout_ms);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c
index fc60fc8ddbf0..1e956e266aa3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c
@@ -229,7 +229,7 @@ void etnaviv_iommuv2_restore(struct etnaviv_gpu *gpu)
 	prefetch = etnaviv_buffer_config_mmuv2(gpu,
 				(u32)etnaviv_domain->mtlb_dma,
 				(u32)etnaviv_domain->base.bad_page_dma);
-	etnaviv_gpu_start_fe(gpu, (u32)etnaviv_cmdbuf_get_pa(gpu->buffer),
+	etnaviv_gpu_start_fe(gpu, (u32)etnaviv_cmdbuf_get_pa(&gpu->buffer),
 			     prefetch);
 	etnaviv_gpu_wait_idle(gpu, 100);
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 25/27] drm/etnaviv: move GPU active handling to bo pin/unpin

[Lucas Stach]

The active count is used to check if the BO is idle, where idle is defined
as not active on the GPU and all VM mappings and reference counts dropped
to the initial state. As the idling of the mappings and references now only
happens in the submit cleanup, the active state handling must be moved to
the same location in order to keep the userspace semantics.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  4 ++++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 10 ----------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index db40cd1c14fd..5a351b0f1087 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -229,6 +229,7 @@ static int submit_pin_objects(struct etnaviv_gem_submit *submit)
 			ret = PTR_ERR(mapping);
 			break;
 		}
+		atomic_inc(&etnaviv_obj->gpu_active);
 
 		submit->bos[i].flags |= BO_PINNED;
 		submit->bos[i].mapping = mapping;
@@ -363,6 +364,7 @@ static void submit_cleanup(struct kref *kref)
 		/* unpin all objects */
 		if (submit->bos[i].flags & BO_PINNED) {
 			etnaviv_gem_mapping_unreference(submit->bos[i].mapping);
+			atomic_dec(&etnaviv_obj->gpu_active);
 			submit->bos[i].mapping = NULL;
 			submit->bos[i].flags &= ~BO_PINNED;
 		}
@@ -372,6 +374,8 @@ static void submit_cleanup(struct kref *kref)
 		drm_gem_object_put_unlocked(&etnaviv_obj->base);
 	}
 
+	wake_up_all(&submit->gpu->fence_event);
+
 	if (submit->in_fence)
 		dma_fence_put(submit->in_fence);
 	if (submit->out_fence)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 93b9d2541502..b0f4c0e68645 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1201,7 +1201,6 @@ static void retire_worker(struct work_struct *work)
 					       retire_work);
 	u32 fence = gpu->completed_fence;
 	struct etnaviv_gem_submit *submit, *tmp;
-	unsigned int i;
 
 	mutex_lock(&gpu->lock);
 	list_for_each_entry_safe(submit, tmp, &gpu->active_submit_list, node) {
@@ -1210,9 +1209,6 @@ static void retire_worker(struct work_struct *work)
 
 		list_del(&submit->node);
 
-		for (i = 0; i < submit->nr_bos; i++)
-			atomic_dec(&submit->bos[i].obj->gpu_active);
-
 		etnaviv_submit_put(submit);
 		/*
 		 * We need to balance the runtime PM count caused by
@@ -1227,8 +1223,6 @@ static void retire_worker(struct work_struct *work)
 	gpu->retired_fence = fence;
 
 	mutex_unlock(&gpu->lock);
-
-	wake_up_all(&gpu->fence_event);
 }
 
 int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
@@ -1429,10 +1423,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	/* We're committed to adding this command buffer, hold a PM reference */
 	pm_runtime_get_noresume(gpu->dev);
 
-	for (i = 0; i < submit->nr_bos; i++) {
-		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
-		atomic_inc(&etnaviv_obj->gpu_active);
-	}
 	hangcheck_timer_reset(gpu);
 	ret = 0;
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 26/27] drm/etnaviv: couple runtime PM management to submit object lifetime

[Lucas Stach]

As long as there is an active submit, we want the GPU to stay awake. This
is slightly complicated by the fact that we really want to wake the GPU
at the last possible moment to achieve maximum power savings.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 +
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  3 +++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 30 +++-------------------------
 3 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index fe99f0c0d068..be72a9833f2b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -106,6 +106,7 @@ struct etnaviv_gem_submit {
 	struct dma_fence *out_fence, *in_fence;
 	struct list_head node; /* GPU active submit list */
 	struct etnaviv_cmdbuf cmdbuf;
+	bool runtime_resumed;
 	u32 exec_state;
 	u32 flags;
 	unsigned int nr_pmrs;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 5a351b0f1087..1f8202bca061 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -355,6 +355,9 @@ static void submit_cleanup(struct kref *kref)
 			container_of(kref, struct etnaviv_gem_submit, refcount);
 	unsigned i;
 
+	if (submit->runtime_resumed)
+		pm_runtime_put_autosuspend(submit->gpu->dev);
+
 	if (submit->cmdbuf.suballoc)
 		etnaviv_cmdbuf_free(&submit->cmdbuf);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index b0f4c0e68645..9becadb07efe 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1210,14 +1210,6 @@ static void retire_worker(struct work_struct *work)
 		list_del(&submit->node);
 
 		etnaviv_submit_put(submit);
-		/*
-		 * We need to balance the runtime PM count caused by
-		 * each submission.  Upon submission, we increment
-		 * the runtime PM counter, and allocate one event.
-		 * So here, we put the runtime PM count for each
-		 * completed event.
-		 */
-		pm_runtime_put_autosuspend(gpu->dev);
 	}
 
 	gpu->retired_fence = fence;
@@ -1289,17 +1281,6 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
 		return -ETIMEDOUT;
 }
 
-int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu)
-{
-	return pm_runtime_get_sync(gpu->dev);
-}
-
-void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu)
-{
-	pm_runtime_mark_last_busy(gpu->dev);
-	pm_runtime_put_autosuspend(gpu->dev);
-}
-
 static void sync_point_perfmon_sample(struct etnaviv_gpu *gpu,
 	struct etnaviv_event *event, unsigned int flags)
 {
@@ -1366,9 +1347,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	unsigned int i, nr_events = 1, event[3];
 	int ret;
 
-	ret = etnaviv_gpu_pm_get_sync(gpu);
+	ret = pm_runtime_get_sync(gpu->dev);
 	if (ret < 0)
 		return ret;
+	submit->runtime_resumed = true;
 
 	/*
 	 * if there are performance monitor requests we need to have
@@ -1383,7 +1365,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	ret = event_alloc(gpu, nr_events, event);
 	if (ret) {
 		DRM_ERROR("no free events\n");
-		goto out_pm_put;
+		return ret;
 	}
 
 	mutex_lock(&gpu->lock);
@@ -1420,18 +1402,12 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 
 	list_add_tail(&submit->node, &gpu->active_submit_list);
 
-	/* We're committed to adding this command buffer, hold a PM reference */
-	pm_runtime_get_noresume(gpu->dev);
-
 	hangcheck_timer_reset(gpu);
 	ret = 0;
 
 out_unlock:
 	mutex_unlock(&gpu->lock);
 
-out_pm_put:
-	etnaviv_gpu_pm_put(gpu);
-
 	return ret;
 }
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 27/27] drm/etnaviv: re-enable perfmon support

[Lucas Stach]

Now that the PMR lifetime issues are solved we can safely re-enable
performance counter profiling support.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index f2948885859d..6faf4042db23 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -459,9 +459,6 @@ static int etnaviv_ioctl_pm_query_dom(struct drm_device *dev, void *data,
 	struct drm_etnaviv_pm_domain *args = data;
 	struct etnaviv_gpu *gpu;
 
-	/* reject as long as the feature isn't stable */
-	return -EINVAL;
-
 	if (args->pipe >= ETNA_MAX_PIPES)
 		return -EINVAL;
 
@@ -479,9 +476,6 @@ static int etnaviv_ioctl_pm_query_sig(struct drm_device *dev, void *data,
 	struct drm_etnaviv_pm_signal *args = data;
 	struct etnaviv_gpu *gpu;
 
-	/* reject as long as the feature isn't stable */
-	return -EINVAL;
-
 	if (args->pipe >= ETNA_MAX_PIPES)
 		return -EINVAL;
 
@@ -556,7 +550,7 @@ static struct drm_driver etnaviv_drm_driver = {
 	.desc               = "etnaviv DRM",
 	.date               = "20151214",
 	.major              = 1,
-	.minor              = 1,
+	.minor              = 2,
 };
 
 /*
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/27] drm/etnaviv: fix GPU vs sync point race

[Philipp Zabel]

On Fri, 2017-12-01 at 11:35 +0100, Lucas Stach wrote:
> If the FE is restarted before the sync point event is cleared, the GPU
> might trigger a completion IRQ for the next sync point before corrupting
> the state of the currently running worker.

I don't understand this explanation. That sounds like it would be ok to
trigger a completion IRQ for the next sync point after corrupting the
state of the currently running worker. Maybe "s/before/, /"?

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

For the patch,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index e19cbe05da2a..f0fae218e4aa 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1484,22 +1484,18 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>  	return ret;
>  }
>  
> -static void etnaviv_process_sync_point(struct etnaviv_gpu *gpu,
> -	struct etnaviv_event *event)
> -{
> -	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
> -
> -	event->sync_point(gpu, event);
> -	etnaviv_gpu_start_fe(gpu, addr + 2, 2);
> -}
> -
>  static void sync_point_worker(struct work_struct *work)
>  {
>  	struct etnaviv_gpu *gpu = container_of(work, struct etnaviv_gpu,
>  					       sync_point_work);
> +	struct etnaviv_event *event = &gpu->event[gpu->sync_point_event];
> +	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
>  
> -	etnaviv_process_sync_point(gpu, &gpu->event[gpu->sync_point_event]);
> +	event->sync_point(gpu, event);
>  	event_free(gpu, gpu->sync_point_event);
> +
> +	/* restart FE last to avoid GPU and IRQ racing against this worker */
> +	etnaviv_gpu_start_fe(gpu, addr + 2, 2);
>  }
>  
>  /*
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/27] drm/etnaviv: split obj locks in different classes depending on the obj type

[Philipp Zabel]

On Fri, 2017-12-01 at 11:35 +0100, Lucas Stach wrote:
> Userptr, prime and shmem buffer objects have different lock ordering
> requirements. This is mostly due to the fact that we don't allow to mmap
> userptr buffers, so we won't ever end up in our fault handler for those,
> so some of the code pathes are never called with the mmap_sem held.
> 
> To avoid lockdep false positives, split them up into different lock classes.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/27] drm/etnaviv: fold __etnaviv_gem_new into caller

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> This function has only one caller and it isn't expected that there will
> be any more in the future. Folding this function into the caller is
> helping the readability.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/27] drm/etnaviv: change return type of etnaviv_gem_obj_add to void

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> This function never fails, as it does nothing more than adding the GEM
> object to the global device list. Making this explicit through the void
> return type allows to drop some unnecessary error handling.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> All code paths which populate userptr BOs are fine with the get_pages
> function taking the mmap_sem lock. This allows to get rid of the pretty
> involved architecture with a worker being scheduled if the mmap_sem
> needs to be taken, but instead call GUP directly and allow it to take
> the lock if necessary.
> 
> This simplifies the code a lot and removes the possibility of this
> function returning -EAGAIN, which complicates object population
> handling at the callers.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++-----------------------------
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
>  2 files changed, 23 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index a52220eeee45..fcc969fa0e69 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  	return 0;
>  }
>  
> -struct get_pages_work {
> -	struct work_struct work;
> -	struct mm_struct *mm;
> -	struct task_struct *task;
> -	struct etnaviv_gem_object *etnaviv_obj;
> -};
> -
> -static struct page **etnaviv_gem_userptr_do_get_pages(
> -	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct *mm, struct task_struct *task)
> -{
> -	int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> -	struct page **pvec;
> -	uintptr_t ptr;
> -	unsigned int flags = 0;
> -
> -	pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> -	if (!pvec)
> -		return ERR_PTR(-ENOMEM);
> -
> -	if (!etnaviv_obj->userptr.ro)
> -		flags |= FOLL_WRITE;
> -
> -	pinned = 0;
> -	ptr = etnaviv_obj->userptr.ptr;
> -
> -	down_read(&mm->mmap_sem);
> -	while (pinned < npages) {
> -		ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
> -					    flags, pvec + pinned, NULL, NULL);
> -		if (ret < 0)
> -			break;
> -
> -		ptr += ret * PAGE_SIZE;
> -		pinned += ret;
> -	}
> -	up_read(&mm->mmap_sem);
> -
> -	if (ret < 0) {
> -		release_pages(pvec, pinned);
> -		kvfree(pvec);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return pvec;
> -}
> -
> -static void __etnaviv_gem_userptr_get_pages(struct work_struct *_work)
> -{
> -	struct get_pages_work *work = container_of(_work, typeof(*work), work);
> -	struct etnaviv_gem_object *etnaviv_obj = work->etnaviv_obj;
> -	struct page **pvec;
> -
> -	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj, work->mm, work->task);
> -
> -	mutex_lock(&etnaviv_obj->lock);
> -	if (IS_ERR(pvec)) {
> -		etnaviv_obj->userptr.work = ERR_CAST(pvec);
> -	} else {
> -		etnaviv_obj->userptr.work = NULL;
> -		etnaviv_obj->pages = pvec;
> -	}
> -
> -	mutex_unlock(&etnaviv_obj->lock);
> -	drm_gem_object_put_unlocked(&etnaviv_obj->base);
> -
> -	mmput(work->mm);
> -	put_task_struct(work->task);
> -	kfree(work);
> -}
> -
>  static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
>  {
>  	struct page **pvec = NULL;
> -	struct get_pages_work *work;
> -	struct mm_struct *mm;
> -	int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> +	struct etnaviv_gem_userptr *userptr = &etnaviv_obj->userptr;
> +	int ret, pinned = 0, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
>  
>  	might_lock_read(&current->mm->mmap_sem);
>  
> -	if (etnaviv_obj->userptr.work) {
> -		if (IS_ERR(etnaviv_obj->userptr.work)) {
> -			ret = PTR_ERR(etnaviv_obj->userptr.work);
> -			etnaviv_obj->userptr.work = NULL;
> -		} else {
> -			ret = -EAGAIN;
> -		}
> -		return ret;
> -	}
> +	if (userptr->mm != current->mm)
> +		return -EPERM;

I don't pretend to understand the implications of this patch completely.
It looks mostly good to me, but this part seems to limit previously
allowed behaviour. I think this warrants a mention in the commit
message.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/27] drm/etnaviv: remove -EAGAIN handling from submit path

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> Now that the userptr BO handling doesn't rely on the userspace restarting
> the submit after object population, there is no need to special case the
> -EAGAIN return value anymore.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/27] drm/etnaviv: don't flush workqueue in etnaviv_gpu_wait_obj_inactive

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> There is no need to synchronize with oustanding retire jobs if the object
> has gone idle. Retire jobs only ever change the object state from active to
> idle, not the other way around.
> 
> The IOVA put race is uncritical, as the GEM_WAIT ioctl itself is holding
> a reference to the GEM object, so the retire worker will not pull the
> object into the CPU domain, which is the thing we are trying to guard
> against with etnaviv_gpu_wait_obj_inactive.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/27] drm/etnaviv: remove switch_context member from etnaviv_gpu

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> There is no need to store this in the gpu struct. MMU flushes are triggered
> correctly in reaction to MMU maps and unmaps, independent of the current ctx
> and required pipe switches can be infered from the current and the desired
> GPU exec state.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/27] drm/etnaviv: move workqueue to be per GPU

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> While the etnaviv workqueue needs to be ordered, as we rely on work items
> being executed in queuing order, this is only true for a single GPU.
> Having a shared workqueue for all GPUs in the system limits concurrency
> artificially.
> 
> Getting each GPU its own ordered workqueue still meets our ordering
> expectations and enables retire workers to run concurrently.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 12 ------------
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h | 10 ----------
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 18 ++++++++++++++----
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
>  4 files changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 491eddf9b150..ca03b5e4789b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -580,12 +580,6 @@ static int etnaviv_bind(struct device *dev)
>  	}
>  	drm->dev_private = priv;
>  
> -	priv->wq = alloc_ordered_workqueue("etnaviv", 0);
> -	if (!priv->wq) {
> -		ret = -ENOMEM;
> -		goto out_wq;
> -	}
> -
>  	mutex_init(&priv->gem_lock);
>  	INIT_LIST_HEAD(&priv->gem_list);
>  	priv->num_gpus = 0;
> @@ -607,9 +601,6 @@ static int etnaviv_bind(struct device *dev)
>  out_register:
>  	component_unbind_all(dev, drm);
>  out_bind:
> -	flush_workqueue(priv->wq);
> -	destroy_workqueue(priv->wq);
> -out_wq:
>  	kfree(priv);
>  out_unref:
>  	drm_dev_unref(drm);
> @@ -624,9 +615,6 @@ static void etnaviv_unbind(struct device *dev)
>  
>  	drm_dev_unregister(drm);
>  
> -	flush_workqueue(priv->wq);
> -	destroy_workqueue(priv->wq);
> -
>  	component_unbind_all(dev, drm);
>  
>  	drm->dev_private = NULL;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index d249acb6da08..8668bfd4abd5 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -56,18 +56,8 @@ struct etnaviv_drm_private {
>  	/* list of GEM objects: */
>  	struct mutex gem_lock;
>  	struct list_head gem_list;
> -
> -	struct workqueue_struct *wq;
>  };
>  
> -static inline void etnaviv_queue_work(struct drm_device *dev,
> -	struct work_struct *w)
> -{
> -	struct etnaviv_drm_private *priv = dev->dev_private;
> -
> -	queue_work(priv->wq, w);
> -}
> -
>  int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		struct drm_file *file);
>  
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 65eaa8c10ba2..1e1e34adb07f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -958,7 +958,7 @@ static void recover_worker(struct work_struct *work)
>  	pm_runtime_put_autosuspend(gpu->dev);
>  
>  	/* Retire the buffer objects in a work */
> -	etnaviv_queue_work(gpu->drm, &gpu->retire_work);
> +	queue_work(gpu->wq, &gpu->retire_work);
>  }
>  
>  static void hangcheck_timer_reset(struct etnaviv_gpu *gpu)
> @@ -994,7 +994,7 @@ static void hangcheck_handler(struct timer_list *t)
>  		dev_err(gpu->dev, "     completed fence: %u\n", fence);
>  		dev_err(gpu->dev, "     active fence: %u\n",
>  			gpu->active_fence);
> -		etnaviv_queue_work(gpu->drm, &gpu->recover_work);
> +		queue_work(gpu->wq, &gpu->recover_work);
>  	}
>  
>  	/* if still more pending work, reset the hangcheck timer: */
> @@ -1526,7 +1526,7 @@ static irqreturn_t irq_handler(int irq, void *data)
>  
>  			if (gpu->event[event].sync_point) {
>  				gpu->sync_point_event = event;
> -				etnaviv_queue_work(gpu->drm, &gpu->sync_point_work);
> +				queue_work(gpu->wq, &gpu->sync_point_work);
>  			}
>  
>  			fence = gpu->event[event].fence;
> @@ -1552,7 +1552,7 @@ static irqreturn_t irq_handler(int irq, void *data)
>  		}
>  
>  		/* Retire the buffer objects in a work */
> -		etnaviv_queue_work(gpu->drm, &gpu->retire_work);
> +		queue_work(gpu->wq, &gpu->retire_work);
>  
>  		ret = IRQ_HANDLED;
>  	}
> @@ -1721,12 +1721,19 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
>  			return PTR_ERR(gpu->cooling);
>  	}
>  
> +	gpu->wq = alloc_ordered_workqueue(dev_name(dev), 0);
> +	if (!gpu->wq) {
> +		thermal_cooling_device_unregister(gpu->cooling);

After the THERMAL selection patch this should change to:

		if (IS_ENABLED(CONFIG_DRM_ETNAVIV_THERMAL))
			thermal_cooling_device_unregister(gpu->cooling);

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/27] drm/etnaviv: hold GPU lock while inserting END command

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> Inserting the END command when suspending the GPU is changing the
> command buffer state, which requires the GPU to be held.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/27] drm/etnaviv: add lockdep annotations to buffer manipulation functions

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> When manipulating the kernel command buffer the GPU mutex must be held, as
> otherwise different callers might try to replace the same part of the
> buffer, wrecking havok in the GPU execution.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/27] drm/etnaviv: simplify submit_create

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> Use kzalloc so other code doesn't need to worry about uninitialized members.
> Drop the non-standard GFP flags, as we really don't want to fail the submit
> when under slight memory pressure. Remove one level of indentation by using
> an early return if the allocation failed. Also remove the unused drm device
> member.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Russell King - ARM Linux]

On Fri, Dec 01, 2017 at 05:38:51PM +0100, Philipp Zabel wrote:
> On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> > All code paths which populate userptr BOs are fine with the get_pages
> > function taking the mmap_sem lock. This allows to get rid of the pretty
> > involved architecture with a worker being scheduled if the mmap_sem
> > needs to be taken, but instead call GUP directly and allow it to take
> > the lock if necessary.
> > 
> > This simplifies the code a lot and removes the possibility of this
> > function returning -EAGAIN, which complicates object population
> > handling at the callers.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++-----------------------------
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
> >  2 files changed, 23 insertions(+), 126 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > index a52220eeee45..fcc969fa0e69 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > @@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
> >  	return 0;
> >  }
> >  
> > -struct get_pages_work {
> > -	struct work_struct work;
> > -	struct mm_struct *mm;
> > -	struct task_struct *task;
> > -	struct etnaviv_gem_object *etnaviv_obj;
> > -};
> > -
> > -static struct page **etnaviv_gem_userptr_do_get_pages(
> > -	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct *mm, struct task_struct *task)
> > -{
> > -	int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> > -	struct page **pvec;
> > -	uintptr_t ptr;
> > -	unsigned int flags = 0;
> > -
> > -	pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> > -	if (!pvec)
> > -		return ERR_PTR(-ENOMEM);
> > -
> > -	if (!etnaviv_obj->userptr.ro)
> > -		flags |= FOLL_WRITE;
> > -
> > -	pinned = 0;
> > -	ptr = etnaviv_obj->userptr.ptr;
> > -
> > -	down_read(&mm->mmap_sem);
> > -	while (pinned < npages) {
> > -		ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
> > -					    flags, pvec + pinned, NULL, NULL);
> > -		if (ret < 0)
> > -			break;
> > -
> > -		ptr += ret * PAGE_SIZE;
> > -		pinned += ret;
> > -	}
> > -	up_read(&mm->mmap_sem);
> > -
> > -	if (ret < 0) {
> > -		release_pages(pvec, pinned);
> > -		kvfree(pvec);
> > -		return ERR_PTR(ret);
> > -	}
> > -
> > -	return pvec;
> > -}
> > -
> > -static void __etnaviv_gem_userptr_get_pages(struct work_struct *_work)
> > -{
> > -	struct get_pages_work *work = container_of(_work, typeof(*work), work);
> > -	struct etnaviv_gem_object *etnaviv_obj = work->etnaviv_obj;
> > -	struct page **pvec;
> > -
> > -	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj, work->mm, work->task);
> > -
> > -	mutex_lock(&etnaviv_obj->lock);
> > -	if (IS_ERR(pvec)) {
> > -		etnaviv_obj->userptr.work = ERR_CAST(pvec);
> > -	} else {
> > -		etnaviv_obj->userptr.work = NULL;
> > -		etnaviv_obj->pages = pvec;
> > -	}
> > -
> > -	mutex_unlock(&etnaviv_obj->lock);
> > -	drm_gem_object_put_unlocked(&etnaviv_obj->base);
> > -
> > -	mmput(work->mm);
> > -	put_task_struct(work->task);
> > -	kfree(work);
> > -}
> > -
> >  static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
> >  {
> >  	struct page **pvec = NULL;
> > -	struct get_pages_work *work;
> > -	struct mm_struct *mm;
> > -	int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> > +	struct etnaviv_gem_userptr *userptr = &etnaviv_obj->userptr;
> > +	int ret, pinned = 0, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> >  
> >  	might_lock_read(&current->mm->mmap_sem);
> >  
> > -	if (etnaviv_obj->userptr.work) {
> > -		if (IS_ERR(etnaviv_obj->userptr.work)) {
> > -			ret = PTR_ERR(etnaviv_obj->userptr.work);
> > -			etnaviv_obj->userptr.work = NULL;
> > -		} else {
> > -			ret = -EAGAIN;
> > -		}
> > -		return ret;
> > -	}
> > +	if (userptr->mm != current->mm)
> > +		return -EPERM;
> 
> I don't pretend to understand the implications of this patch completely.
> It looks mostly good to me, but this part seems to limit previously
> allowed behaviour. I think this warrants a mention in the commit
> message.

The point of the complexity is to be able to grab the mmap_sem avoiding
any locking dependencies that would normally occur if it were done in
the ioctl.

However, drm locking has changed quite a bit over the last year, and
this is probably not be needed anymore - I assume that Lucas has
thoroughly verified the locking dependencies.  However, I notice i915
still retains this code though.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/27] drm/etnaviv: don't flush workqueue in etnaviv_gpu_wait_obj_inactive

[Russell King - ARM Linux]

On Fri, Dec 01, 2017 at 11:36:06AM +0100, Lucas Stach wrote:
> There is no need to synchronize with oustanding retire jobs if the object
> has gone idle. Retire jobs only ever change the object state from active to
> idle, not the other way around.
> 
> The IOVA put race is uncritical, as the GEM_WAIT ioctl itself is holding
> a reference to the GEM object, so the retire worker will not pull the
> object into the CPU domain, which is the thing we are trying to guard
> against with etnaviv_gpu_wait_obj_inactive.

I don't think this makes sense.

The point of GEM_WAIT is to ensure that the driver has completely
finished with the object, including making sure that the memory
(particularly usermem) is not going to get invalidated unexpectedly.

This is a very important property of GEM_WAIT, which, if violated,
leads to malloc() stack corruption in the Xorg DDX driver.

The explanation above seems to suggest that this condition has been
violated already by the etnaviv driver, which will lead to Xorg
segfaults.

The point to this is to ensure that, at this point in the DDX:

        if (bo->is_usermem)
                etna_bo_gem_wait(bo, VIV_WAIT_INDEFINITE);

        drmIoctl(conn->fd, DRM_IOCTL_GEM_CLOSE, &req);

once we reach here, the usermem object is completely free of any
meddling by the etnaviv driver.  This must *not* happen "at some
unknown point in the future".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Lucas Stach]

Am Freitag, den 01.12.2017, 16:51 +0000 schrieb Russell King - ARM
Linux:
> On Fri, Dec 01, 2017 at 05:38:51PM +0100, Philipp Zabel wrote:
> > On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> > > All code paths which populate userptr BOs are fine with the
> > > get_pages
> > > function taking the mmap_sem lock. This allows to get rid of the
> > > pretty
> > > involved architecture with a worker being scheduled if the
> > > mmap_sem
> > > needs to be taken, but instead call GUP directly and allow it to
> > > take
> > > the lock if necessary.
> > > 
> > > This simplifies the code a lot and removes the possibility of
> > > this
> > > function returning -EAGAIN, which complicates object population
> > > handling at the callers.
> > > 
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++---------------
> > > --------------
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
> > >  2 files changed, 23 insertions(+), 126 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > index a52220eeee45..fcc969fa0e69 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > @@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct
> > > drm_device *dev, size_t size, u32 flags,
> > >  	return 0;
> > >  }
> > >  
> > > -struct get_pages_work {
> > > -	struct work_struct work;
> > > -	struct mm_struct *mm;
> > > -	struct task_struct *task;
> > > -	struct etnaviv_gem_object *etnaviv_obj;
> > > -};
> > > -
> > > -static struct page **etnaviv_gem_userptr_do_get_pages(
> > > -	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct
> > > *mm, struct task_struct *task)
> > > -{
> > > -	int ret = 0, pinned, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > > -	struct page **pvec;
> > > -	uintptr_t ptr;
> > > -	unsigned int flags = 0;
> > > -
> > > -	pvec = kvmalloc_array(npages, sizeof(struct page *),
> > > GFP_KERNEL);
> > > -	if (!pvec)
> > > -		return ERR_PTR(-ENOMEM);
> > > -
> > > -	if (!etnaviv_obj->userptr.ro)
> > > -		flags |= FOLL_WRITE;
> > > -
> > > -	pinned = 0;
> > > -	ptr = etnaviv_obj->userptr.ptr;
> > > -
> > > -	down_read(&mm->mmap_sem);
> > > -	while (pinned < npages) {
> > > -		ret = get_user_pages_remote(task, mm, ptr,
> > > npages - pinned,
> > > -					    flags, pvec +
> > > pinned, NULL, NULL);
> > > -		if (ret < 0)
> > > -			break;
> > > -
> > > -		ptr += ret * PAGE_SIZE;
> > > -		pinned += ret;
> > > -	}
> > > -	up_read(&mm->mmap_sem);
> > > -
> > > -	if (ret < 0) {
> > > -		release_pages(pvec, pinned);
> > > -		kvfree(pvec);
> > > -		return ERR_PTR(ret);
> > > -	}
> > > -
> > > -	return pvec;
> > > -}
> > > -
> > > -static void __etnaviv_gem_userptr_get_pages(struct work_struct
> > > *_work)
> > > -{
> > > -	struct get_pages_work *work = container_of(_work,
> > > typeof(*work), work);
> > > -	struct etnaviv_gem_object *etnaviv_obj = work-
> > > >etnaviv_obj;
> > > -	struct page **pvec;
> > > -
> > > -	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj,
> > > work->mm, work->task);
> > > -
> > > -	mutex_lock(&etnaviv_obj->lock);
> > > -	if (IS_ERR(pvec)) {
> > > -		etnaviv_obj->userptr.work = ERR_CAST(pvec);
> > > -	} else {
> > > -		etnaviv_obj->userptr.work = NULL;
> > > -		etnaviv_obj->pages = pvec;
> > > -	}
> > > -
> > > -	mutex_unlock(&etnaviv_obj->lock);
> > > -	drm_gem_object_put_unlocked(&etnaviv_obj->base);
> > > -
> > > -	mmput(work->mm);
> > > -	put_task_struct(work->task);
> > > -	kfree(work);
> > > -}
> > > -
> > >  static int etnaviv_gem_userptr_get_pages(struct
> > > etnaviv_gem_object *etnaviv_obj)
> > >  {
> > >  	struct page **pvec = NULL;
> > > -	struct get_pages_work *work;
> > > -	struct mm_struct *mm;
> > > -	int ret, pinned, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > > +	struct etnaviv_gem_userptr *userptr = &etnaviv_obj-
> > > >userptr;
> > > +	int ret, pinned = 0, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > >  
> > >  	might_lock_read(&current->mm->mmap_sem);
> > >  
> > > -	if (etnaviv_obj->userptr.work) {
> > > -		if (IS_ERR(etnaviv_obj->userptr.work)) {
> > > -			ret = PTR_ERR(etnaviv_obj-
> > > >userptr.work);
> > > -			etnaviv_obj->userptr.work = NULL;
> > > -		} else {
> > > -			ret = -EAGAIN;
> > > -		}
> > > -		return ret;
> > > -	}
> > > +	if (userptr->mm != current->mm)
> > > +		return -EPERM;
> > 
> > I don't pretend to understand the implications of this patch
> > completely.
> > It looks mostly good to me, but this part seems to limit previously
> > allowed behaviour. I think this warrants a mention in the commit
> > message.
> 
> The point of the complexity is to be able to grab the mmap_sem
> avoiding
> any locking dependencies that would normally occur if it were done in
> the ioctl.
> 
> However, drm locking has changed quite a bit over the last year, and
> this is probably not be needed anymore - I assume that Lucas has
> thoroughly verified the locking dependencies.  However, I notice i915
> still retains this code though.

Yes, I have. Both by going through, reading a lot of the userptr
related core kernel code and by improving lockdeps ability to reason
about the locking used inside of etnaviv.

i915 still retains this code, as they are still using the "big DRM
lock" struct_mutex. Etnaviv has a much more fine-grained locking
scheme, allowing us to drop this complexity.

Philipp is right in that this change has a functional change by
rejecting attempts to populate userptr objects created by foreign
processes. Something which was allowed before, but would have been
pretty broken in practice if anyone dared to do so, as it violates the
coherency rules set by the etnaviv GEM object handling.

Regards,
Lucas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/27] drm/etnaviv: don't flush workqueue in etnaviv_gpu_wait_obj_inactive

[Lucas Stach]

Hi Russell,

Am Freitag, den 01.12.2017, 16:59 +0000 schrieb Russell King - ARM Linux:
> On Fri, Dec 01, 2017 at 11:36:06AM +0100, Lucas Stach wrote:
> > There is no need to synchronize with oustanding retire jobs if the object
> > has gone idle. Retire jobs only ever change the object state from active to
> > idle, not the other way around.
> > 
> > The IOVA put race is uncritical, as the GEM_WAIT ioctl itself is holding
> > a reference to the GEM object, so the retire worker will not pull the
> > object into the CPU domain, which is the thing we are trying to guard
> > against with etnaviv_gpu_wait_obj_inactive.
> 
> I don't think this makes sense.
> 
> The point of GEM_WAIT is to ensure that the driver has completely
> finished with the object, including making sure that the memory
> (particularly usermem) is not going to get invalidated unexpectedly.
> 
> This is a very important property of GEM_WAIT, which, if violated,
> leads to malloc() stack corruption in the Xorg DDX driver.

I'm aware of this property of GEM_WAIT. In fact it's the only reason this
ioctl exists at all.

> The explanation above seems to suggest that this condition has been
> violated already by the etnaviv driver, which will lead to Xorg
> segfaults.

The comment in the commit message refers to the fact that the active
count decrement and IOVA put are not atomic. Still execution of the
etnaviv_gpu_wait_obj_inactive function is synchronized through the
fence event. This gives the ioctl the exact semantics you require for
the X.Org driver to work correctly.

The user visible behavior is unchanged before and after this patch. I
should make this more clear in the commit message.

Regards,
Lucas

> The point to this is to ensure that, at this point in the DDX:
> 
>         if (bo->is_usermem)
>                 etna_bo_gem_wait(bo, VIV_WAIT_INDEFINITE);
> 
>         drmIoctl(conn->fd, DRM_IOCTL_GEM_CLOSE, &req);
> 
> once we reach here, the usermem object is completely free of any
> meddling by the etnaviv driver.  This must *not* happen "at some
> unknown point in the future".
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/27] drm/etnaviv: fix GPU vs sync point race

[Christian Gmeiner]

2017-12-01 11:35 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> If the FE is restarted before the sync point event is cleared, the GPU
> might trigger a completion IRQ for the next sync point before corrupting
> the state of the currently running worker.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index e19cbe05da2a..f0fae218e4aa 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1484,22 +1484,18 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>         return ret;
>  }
>
> -static void etnaviv_process_sync_point(struct etnaviv_gpu *gpu,
> -       struct etnaviv_event *event)
> -{
> -       u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
> -
> -       event->sync_point(gpu, event);
> -       etnaviv_gpu_start_fe(gpu, addr + 2, 2);
> -}
> -
>  static void sync_point_worker(struct work_struct *work)
>  {
>         struct etnaviv_gpu *gpu = container_of(work, struct etnaviv_gpu,
>                                                sync_point_work);
> +       struct etnaviv_event *event = &gpu->event[gpu->sync_point_event];
> +       u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
>
> -       etnaviv_process_sync_point(gpu, &gpu->event[gpu->sync_point_event]);
> +       event->sync_point(gpu, event);
>         event_free(gpu, gpu->sync_point_event);
> +
> +       /* restart FE last to avoid GPU and IRQ racing against this worker */
> +       etnaviv_gpu_start_fe(gpu, addr + 2, 2);
>  }
>
>  /*
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/27] drm/etnaviv: split obj locks in different classes depending on the obj type

[Christian Gmeiner]

2017-12-01 11:35 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> Userptr, prime and shmem buffer objects have different lock ordering
> requirements. This is mostly due to the fact that we don't allow to mmap
> userptr buffers, so we won't ever end up in our fault handler for those,
> so some of the code pathes are never called with the mmap_sem held.
>
> To avoid lockdep false positives, split them up into different lock classes.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 7 +++++++
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 3 +++
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index daee3f1196df..e3582507963d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -24,6 +24,9 @@
>  #include "etnaviv_gpu.h"
>  #include "etnaviv_mmu.h"
>
> +static struct lock_class_key etnaviv_shm_lock_class;
> +static struct lock_class_key etnaviv_userptr_lock_class;
> +
>  static void etnaviv_gem_scatter_map(struct etnaviv_gem_object *etnaviv_obj)
>  {
>         struct drm_device *dev = etnaviv_obj->base.dev;
> @@ -653,6 +656,8 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>         if (ret)
>                 goto fail;
>
> +       lockdep_set_class(&to_etnaviv_bo(obj)->lock, &etnaviv_shm_lock_class);
> +
>         ret = drm_gem_object_init(dev, obj, size);
>         if (ret == 0) {
>                 struct address_space *mapping;
> @@ -897,6 +902,8 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
>         if (ret)
>                 return ret;
>
> +       lockdep_set_class(&etnaviv_obj->lock, &etnaviv_userptr_lock_class);
> +
>         etnaviv_obj->userptr.ptr = ptr;
>         etnaviv_obj->userptr.task = current;
>         etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index ae884723e9b1..ea87bf87b187 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -19,6 +19,7 @@
>  #include "etnaviv_drv.h"
>  #include "etnaviv_gem.h"
>
> +static struct lock_class_key etnaviv_prime_lock_class;
>
>  struct sg_table *etnaviv_gem_prime_get_sg_table(struct drm_gem_object *obj)
>  {
> @@ -125,6 +126,8 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>         if (ret < 0)
>                 return ERR_PTR(ret);
>
> +       lockdep_set_class(&etnaviv_obj->lock, &etnaviv_prime_lock_class);
> +
>         npages = size / PAGE_SIZE;
>
>         etnaviv_obj->sgt = sgt;
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/27] drm/etnaviv: move object fence attachment to gem_submit path

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> The object fencing has nothing to do with the actual GPU buffer submit,
> so move it to the gem submit path to have a cleaner split.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  7 -------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 51ed34586c10..72468f11dd16 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -180,6 +180,24 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
>  	return ret;
>  }
>  
> +static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
> +{
> +	int i;
> +
> +	for (i = 0; i < submit->nr_bos; i++) {
> +		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
> +
> +		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
> +			reservation_object_add_excl_fence(etnaviv_obj->resv,
> +							  submit->fence);
> +		else
> +			reservation_object_add_shared_fence(etnaviv_obj->resv,
> +							    submit->fence);
> +
> +		submit_unlock_object(submit, i);
> +	}
> +}
> +
>  static void submit_unpin_objects(struct etnaviv_gem_submit *submit)
>  {
>  	int i;
> @@ -335,6 +353,7 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	for (i = 0; i < submit->nr_bos; i++) {
>  		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
>  
> +		/* if the GPU submit failed, objects might still be locked */
>  		submit_unlock_object(submit, i);
>  		drm_gem_object_put_unlocked(&etnaviv_obj->base);
>  	}
> @@ -507,6 +526,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto out;
>  
> +	submit_attach_object_fences(submit);
> +
>  	cmdbuf = NULL;
>  
>  	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 85f6ee1da016..d55a2137ee37 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1443,13 +1443,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>  		etnaviv_gem_mapping_reference(submit->bos[i].mapping);
>  		cmdbuf->bo_map[i] = submit->bos[i].mapping;
>  		atomic_inc(&etnaviv_obj->gpu_active);
> -
> -		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
> -			reservation_object_add_excl_fence(etnaviv_obj->resv,
> -							  fence);
> -		else
> -			reservation_object_add_shared_fence(etnaviv_obj->resv,
> -							    fence);
>  	}
>  	cmdbuf->nr_bos = submit->nr_bos;
>  	hangcheck_timer_reset(gpu);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/27] drm/etnaviv: attach in fence to submit and move fence wait to fence_sync

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> Simplifies the cleanup path and moves fence waiting to a central location.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 28 +++++++++++++---------------
>  2 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index 848daf719cda..21cb3460046f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -103,7 +103,7 @@ struct etnaviv_gem_submit_bo {
>  struct etnaviv_gem_submit {
>  	struct etnaviv_gpu *gpu;
>  	struct ww_acquire_ctx ticket;
> -	struct dma_fence *out_fence;
> +	struct dma_fence *out_fence, *in_fence;
>  	u32 flags;
>  	unsigned int nr_bos;
>  	struct etnaviv_gem_submit_bo bos[0];
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 930577c8794e..20906c22998c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -177,6 +177,15 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
>  			break;
>  	}
>  
> +	if (submit->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> +		/*
> +		 * Wait if the fence is from a foreign context, or if the fence
> +		 * array contains any fence from a foreign context.
> +		 */
> +		if (!dma_fence_match_context(submit->in_fence, context))
> +			ret = dma_fence_wait(submit->in_fence, true);
> +	}
> +
>  	return ret;
>  }
>  
> @@ -359,6 +368,8 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	}
>  
>  	ww_acquire_fini(&submit->ticket);
> +	if (submit->in_fence)
> +		dma_fence_put(submit->in_fence);
>  	if (submit->out_fence)
>  		dma_fence_put(submit->out_fence);
>  	kfree(submit);
> @@ -375,7 +386,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	struct etnaviv_gem_submit *submit;
>  	struct etnaviv_cmdbuf *cmdbuf;
>  	struct etnaviv_gpu *gpu;
> -	struct dma_fence *in_fence = NULL;
>  	struct sync_file *sync_file = NULL;
>  	int out_fence_fd = -1;
>  	void *stream;
> @@ -485,21 +495,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	}
>  
>  	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> -		in_fence = sync_file_get_fence(args->fence_fd);
> -		if (!in_fence) {
> +		submit->in_fence = sync_file_get_fence(args->fence_fd);
> +		if (!submit->in_fence) {
>  			ret = -EINVAL;
>  			goto err_submit_objects;
>  		}
> -
> -		/*
> -		 * Wait if the fence is from a foreign context, or if the fence
> -		 * array contains any fence from a foreign context.
> -		 */
> -		if (!dma_fence_match_context(in_fence, gpu->fence_context)) {
> -			ret = dma_fence_wait(in_fence, true);
> -			if (ret)
> -				goto err_submit_objects;
> -		}
>  	}
>  
>  	ret = submit_fence_sync(submit);
> @@ -552,8 +552,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	submit_unpin_objects(submit);
>  
>  err_submit_objects:
> -	if (in_fence)
> -		dma_fence_put(in_fence);
>  	submit_cleanup(submit);
>  
>  err_submit_cmds:
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 18/27] drm/etnaviv: move object unpinning to submit cleanup

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> This is safe to call in all paths, as the BO_PINNED flag tells us if the BO
> needs unpinning.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 33 ++++++++++------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 20906c22998c..9b5541207d33 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -207,19 +207,6 @@ static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
>  	}
>  }
>  
> -static void submit_unpin_objects(struct etnaviv_gem_submit *submit)
> -{
> -	int i;
> -
> -	for (i = 0; i < submit->nr_bos; i++) {
> -		if (submit->bos[i].flags & BO_PINNED)
> -			etnaviv_gem_mapping_unreference(submit->bos[i].mapping);
> -
> -		submit->bos[i].mapping = NULL;
> -		submit->bos[i].flags &= ~BO_PINNED;
> -	}
> -}
> -
>  static int submit_pin_objects(struct etnaviv_gem_submit *submit)
>  {
>  	int i, ret = 0;
> @@ -362,6 +349,13 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	for (i = 0; i < submit->nr_bos; i++) {
>  		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
>  
> +		/* unpin all objects */
> +		if (submit->bos[i].flags & BO_PINNED) {
> +			etnaviv_gem_mapping_unreference(submit->bos[i].mapping);
> +			submit->bos[i].mapping = NULL;
> +			submit->bos[i].flags &= ~BO_PINNED;
> +		}
> +
>  		/* if the GPU submit failed, objects might still be locked */
>  		submit_unlock_object(submit, i);
>  		drm_gem_object_put_unlocked(&etnaviv_obj->base);
> @@ -508,23 +502,23 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  
>  	ret = submit_pin_objects(submit);
>  	if (ret)
> -		goto out;
> +		goto err_submit_objects;
>  
>  	ret = submit_reloc(submit, stream, args->stream_size / 4,
>  			   relocs, args->nr_relocs);
>  	if (ret)
> -		goto out;
> +		goto err_submit_objects;
>  
>  	ret = submit_perfmon_validate(submit, cmdbuf, pmrs, args->nr_pmrs);
>  	if (ret)
> -		goto out;
> +		goto err_submit_objects;
>  
>  	memcpy(cmdbuf->vaddr, stream, args->stream_size);
>  	cmdbuf->user_size = ALIGN(args->stream_size, 8);
>  
>  	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
>  	if (ret)
> -		goto out;
> +		goto err_submit_objects;
>  
>  	submit_attach_object_fences(submit);
>  
> @@ -540,7 +534,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		sync_file = sync_file_create(submit->out_fence);
>  		if (!sync_file) {
>  			ret = -ENOMEM;
> -			goto out;
> +			goto err_submit_objects;
>  		}
>  		fd_install(out_fence_fd, sync_file->file);
>  	}
> @@ -548,9 +542,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	args->fence_fd = out_fence_fd;
>  	args->fence = submit->out_fence->seqno;
>  
> -out:
> -	submit_unpin_objects(submit);
> -
>  err_submit_objects:
>  	submit_cleanup(submit);
>  
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/27] drm/etnaviv: add lockdep annotation for userptr object population

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> The current userptr page population will defer work to a work item if
> needed to avoid ever taking the mmap_sem in the direct call path. With
> the more fine-grained locking in etnaviv this isn't needed anymore, so
> a future commit will simplify this code.
>
> Add a lockdep annotation to validate the assumption that the mmap_sem
> can be taken in the direct call path.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index e3582507963d..555a331c51a9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -804,6 +804,8 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
>         struct mm_struct *mm;
>         int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
>
> +       might_lock_read(&current->mm->mmap_sem);
> +
>         if (etnaviv_obj->userptr.work) {
>                 if (IS_ERR(etnaviv_obj->userptr.work)) {
>                         ret = PTR_ERR(etnaviv_obj->userptr.work);
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/27] drm/etnaviv: fold __etnaviv_gem_new into caller

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> This function has only one caller and it isn't expected that there will
> be any more in the future. Folding this function into the caller is
> helping the readability.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 555a331c51a9..f75105b7e1a4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -643,8 +643,9 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>         return 0;
>  }
>
> -static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
> -               u32 size, u32 flags)
> +/* convenience method to construct a GEM buffer object, and userspace handle */
> +int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> +       u32 size, u32 flags, u32 *handle)
>  {
>         struct drm_gem_object *obj = NULL;
>         int ret;
> @@ -665,7 +666,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>                 /*
>                  * Our buffers are kept pinned, so allocating them
>                  * from the MOVABLE zone is a really bad idea, and
> -                * conflicts with CMA.  See coments above new_inode()
> +                * conflicts with CMA. See comments above new_inode()
>                  * why this is required _and_ expected if you're
>                  * going to pin these pages.
>                  */
> @@ -677,24 +678,6 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev,
>         if (ret)
>                 goto fail;
>
> -       return obj;
> -
> -fail:
> -       drm_gem_object_put_unlocked(obj);
> -       return ERR_PTR(ret);
> -}
> -
> -/* convenience method to construct a GEM buffer object, and userspace handle */
> -int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> -               u32 size, u32 flags, u32 *handle)
> -{
> -       struct drm_gem_object *obj;
> -       int ret;
> -
> -       obj = __etnaviv_gem_new(dev, size, flags);
> -       if (IS_ERR(obj))
> -               return PTR_ERR(obj);
> -
>         ret = etnaviv_gem_obj_add(dev, obj);
>         if (ret < 0) {
>                 drm_gem_object_put_unlocked(obj);
> @@ -704,6 +687,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>         ret = drm_gem_handle_create(file, obj, handle);
>
>         /* drop reference from allocate - handle holds it now */
> +fail:
>         drm_gem_object_put_unlocked(obj);
>
>         return ret;
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/27] drm/etnaviv: change return type of etnaviv_gem_obj_add to void

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> This function never fails, as it does nothing more than adding the GEM
> object to the global device list. Making this explicit through the void
> return type allows to drop some unnecessary error handling.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 16 ++++------------
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h       |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  4 +---
>  3 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index f75105b7e1a4..a52220eeee45 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -586,7 +586,7 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj)
>         kfree(etnaviv_obj);
>  }
>
> -int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
> +void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
>  {
>         struct etnaviv_drm_private *priv = dev->dev_private;
>         struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
> @@ -594,8 +594,6 @@ int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
>         mutex_lock(&priv->gem_lock);
>         list_add_tail(&etnaviv_obj->gem_node, &priv->gem_list);
>         mutex_unlock(&priv->gem_lock);
> -
> -       return 0;
>  }
>
>  static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
> @@ -678,11 +676,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>         if (ret)
>                 goto fail;
>
> -       ret = etnaviv_gem_obj_add(dev, obj);
> -       if (ret < 0) {
> -               drm_gem_object_put_unlocked(obj);
> -               return ret;
> -       }
> +       etnaviv_gem_obj_add(dev, obj);
>
>         ret = drm_gem_handle_create(file, obj, handle);
>
> @@ -895,12 +889,10 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
>         etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
>         get_task_struct(current);
>
> -       ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
> -       if (ret)
> -               goto unreference;
> +       etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
>
>         ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
> -unreference:
> +
>         /* drop reference from allocate - handle holds it now */
>         drm_gem_object_put_unlocked(&etnaviv_obj->base);
>         return ret;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index e437fba1209d..00bd9c851a02 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -117,7 +117,7 @@ int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
>  int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>         struct reservation_object *robj, const struct etnaviv_gem_ops *ops,
>         struct etnaviv_gem_object **res);
> -int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
> +void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
>  struct page **etnaviv_gem_get_pages(struct etnaviv_gem_object *obj);
>  void etnaviv_gem_put_pages(struct etnaviv_gem_object *obj);
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index ea87bf87b187..5704305d41e6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -142,9 +142,7 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>         if (ret)
>                 goto fail;
>
> -       ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
> -       if (ret)
> -               goto fail;
> +       etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
>
>         return &etnaviv_obj->base;
>
> --
> 2.11.0
>
> _______________________________________________
> etnaviv mailing list
> etnaviv@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/27] drm/etnaviv: hold GPU lock while inserting END command

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> Inserting the END command when suspending the GPU is changing the
> command buffer state, which requires the GPU to be held.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 1e1e34adb07f..85f6ee1da016 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1631,7 +1631,9 @@ static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu)
>  {
>         if (gpu->buffer) {
>                 /* Replace the last WAIT with END */
> +               mutex_lock(&gpu->lock);
>                 etnaviv_buffer_end(gpu);
> +               mutex_unlock(&gpu->lock);
>
>                 /*
>                  * We know that only the FE is busy here, this should
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/27] drm/etnaviv: remove stale TODO in etnaviv_gpu_submit

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> Flush and prefetch are properly handled in the buffer code, data endianess
> would need much wider changes than adding something to this single function.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index f0fae218e4aa..3dffccfcefce 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1391,15 +1391,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>                 return ret;
>
>         /*
> -        * TODO
> -        *
> -        * - flush
> -        * - data endian
> -        * - prefetch
> -        *
> -        */
> -
> -       /*
>          * if there are performance monitor requests we need to have
>          * - a sync point to re-configure gpu and process ETNA_PM_PROCESS_PRE
>          *   requests.
> --
> 2.11.0
>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/27] drm/etnaviv: remove switch_context member from etnaviv_gpu

[Christian Gmeiner]

2017-12-01 11:36 GMT+01:00 Lucas Stach <l.stach@pengutronix.de>:
> There is no need to store this in the gpu struct. MMU flushes are triggered
> correctly in reaction to MMU maps and unmaps, independent of the current ctx
> and required pipe switches can be infered from the current and the desired
> GPU exec state.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 10 ++++++----
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c    |  8 +-------
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h    |  1 -
>  3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> index 9e7098e3207f..6ad8972a59cc 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> @@ -294,6 +294,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
>         unsigned int waitlink_offset = buffer->user_size - 16;
>         u32 return_target, return_dwords;
>         u32 link_target, link_dwords;
> +       bool switch_context = gpu->exec_state != cmdbuf->exec_state;
>
>         if (drm_debug & DRM_UT_DRIVER)
>                 etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
> @@ -306,7 +307,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
>          * need to append a mmu flush load state, followed by a new
>          * link to this buffer - a total of four additional words.
>          */
> -       if (gpu->mmu->need_flush || gpu->switch_context) {
> +       if (gpu->mmu->need_flush || switch_context) {
>                 u32 target, extra_dwords;
>
>                 /* link command */
> @@ -321,7 +322,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
>                 }
>
>                 /* pipe switch commands */
> -               if (gpu->switch_context)
> +               if (switch_context)
>                         extra_dwords += 4;
>
>                 target = etnaviv_buffer_reserve(gpu, buffer, extra_dwords);
> @@ -349,10 +350,9 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
>                         gpu->mmu->need_flush = false;
>                 }
>
> -               if (gpu->switch_context) {
> +               if (switch_context) {
>                         etnaviv_cmd_select_pipe(gpu, buffer, cmdbuf->exec_state);
>                         gpu->exec_state = cmdbuf->exec_state;
> -                       gpu->switch_context = false;
>                 }
>
>                 /* And the link to the submitted buffer */
> @@ -421,4 +421,6 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
>
>         if (drm_debug & DRM_UT_DRIVER)
>                 etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
> +
> +       gpu->lastctx = cmdbuf->ctx;
>  }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index ae152bb78f18..65eaa8c10ba2 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1416,12 +1416,6 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>         submit->fence = dma_fence_get(fence);
>         gpu->active_fence = submit->fence->seqno;
>
> -       if (gpu->lastctx != cmdbuf->ctx) {
> -               gpu->mmu->need_flush = true;
> -               gpu->switch_context = true;
> -               gpu->lastctx = cmdbuf->ctx;
> -       }
> -
>         if (cmdbuf->nr_pmrs) {
>                 gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;
>                 gpu->event[event[1]].cmdbuf = cmdbuf;
> @@ -1662,7 +1656,7 @@ static int etnaviv_gpu_hw_resume(struct etnaviv_gpu *gpu)
>         etnaviv_gpu_update_clock(gpu);
>         etnaviv_gpu_hw_init(gpu);
>
> -       gpu->switch_context = true;
> +       gpu->lastctx = NULL;
>         gpu->exec_state = -1;
>
>         mutex_unlock(&gpu->lock);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 4f10f147297a..15090bb68f5a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -106,7 +106,6 @@ struct etnaviv_gpu {
>         struct mutex lock;
>         struct etnaviv_chip_identity identity;
>         struct etnaviv_file_private *lastctx;
> -       bool switch_context;
>
>         /* 'ring'-buffer: */
>         struct etnaviv_cmdbuf *buffer;
> --
> 2.11.0
>
> _______________________________________________
> etnaviv mailing list
> etnaviv@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 20/27] drm/etnaviv: refcount the submit object

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> The submit object lifetime will get extended to the actual GPU
> execution. As multiple users will depend on this, add a kref to
> properly control destuction of the object.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

>  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 +++
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 12 ++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index 6b78d059ed2d..4238f8d8541d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -101,6 +101,7 @@ struct etnaviv_gem_submit_bo {
>   * lasts for the duration of the submit-ioctl.
>   */
>  struct etnaviv_gem_submit {
> +	struct kref refcount;
>  	struct etnaviv_gpu *gpu;
>  	struct dma_fence *out_fence, *in_fence;
>  	u32 flags;
> @@ -109,6 +110,8 @@ struct etnaviv_gem_submit {
>  	/* No new members here, the previous one is variable-length! */
>  };
>  
> +void etnaviv_submit_put(struct etnaviv_gem_submit * submit);
> +
>  int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
>  	struct timespec *timeout);
>  int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 3090a46979af..9b4436c380ea 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -43,6 +43,7 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
>  		return NULL;
>  
>  	submit->gpu = gpu;
> +	kref_init(&submit->refcount);
>  
>  	return submit;
>  }
> @@ -341,8 +342,10 @@ static int submit_perfmon_validate(struct etnaviv_gem_submit *submit,
>  	return 0;
>  }
>  
> -static void submit_cleanup(struct etnaviv_gem_submit *submit)
> +static void submit_cleanup(struct kref *kref)
>  {
> +	struct etnaviv_gem_submit *submit =
> +			container_of(kref, struct etnaviv_gem_submit, refcount);
>  	unsigned i;
>  
>  	for (i = 0; i < submit->nr_bos; i++) {
> @@ -367,6 +370,11 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	kfree(submit);
>  }
>  
> +void etnaviv_submit_put(struct etnaviv_gem_submit *submit)
> +{
> +	kref_put(&submit->refcount, submit_cleanup);
> +}
> +
>  int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> @@ -544,7 +552,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	args->fence = submit->out_fence->seqno;
>  
>  err_submit_objects:
> -	submit_cleanup(submit);
> +	etnaviv_submit_put(submit);
>  
>  err_submit_ww_acquire:
>  	ww_acquire_fini(&ticket);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 19/27] drm/etnaviv: move ww_acquire_ctx out of submit object

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> The acquire_ctx is special in that it needs to be released from the same
> thread as has been used to initialize it. This collides with the intention to
> extend the submit lifetime beyond the gem_submit function with potentially
> other threads doing the final cleanup.
> 
> Move the ww_acquire_ctx to the function local stack as suggested in the
> documentation.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 -
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 22 +++++++++++++---------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index 21cb3460046f..6b78d059ed2d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -102,7 +102,6 @@ struct etnaviv_gem_submit_bo {
>   */
>  struct etnaviv_gem_submit {
>  	struct etnaviv_gpu *gpu;
> -	struct ww_acquire_ctx ticket;
>  	struct dma_fence *out_fence, *in_fence;
>  	u32 flags;
>  	unsigned int nr_bos;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 9b5541207d33..3090a46979af 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -44,8 +44,6 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
>  
>  	submit->gpu = gpu;
>  
> -	ww_acquire_init(&submit->ticket, &reservation_ww_class);
> -
>  	return submit;
>  }
>  
> @@ -107,7 +105,8 @@ static void submit_unlock_object(struct etnaviv_gem_submit *submit, int i)
>  	}
>  }
>  
> -static int submit_lock_objects(struct etnaviv_gem_submit *submit)
> +static int submit_lock_objects(struct etnaviv_gem_submit *submit,
> +		struct ww_acquire_ctx *ticket)
>  {
>  	int contended, slow_locked = -1, i, ret = 0;
>  
> @@ -122,7 +121,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
>  
>  		if (!(submit->bos[i].flags & BO_LOCKED)) {
>  			ret = ww_mutex_lock_interruptible(&etnaviv_obj->resv->lock,
> -					&submit->ticket);
> +							  ticket);
>  			if (ret == -EALREADY)
>  				DRM_ERROR("BO at index %u already on submit list\n",
>  					  i);
> @@ -132,7 +131,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
>  		}
>  	}
>  
> -	ww_acquire_done(&submit->ticket);
> +	ww_acquire_done(ticket);
>  
>  	return 0;
>  
> @@ -150,7 +149,7 @@ static int submit_lock_objects(struct etnaviv_gem_submit *submit)
>  
>  		/* we lost out in a seqno race, lock and retry.. */
>  		ret = ww_mutex_lock_slow_interruptible(&etnaviv_obj->resv->lock,
> -				&submit->ticket);
> +						       ticket);
>  		if (!ret) {
>  			submit->bos[contended].flags |= BO_LOCKED;
>  			slow_locked = contended;
> @@ -361,7 +360,6 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  		drm_gem_object_put_unlocked(&etnaviv_obj->base);
>  	}
>  
> -	ww_acquire_fini(&submit->ticket);
>  	if (submit->in_fence)
>  		dma_fence_put(submit->in_fence);
>  	if (submit->out_fence)
> @@ -381,6 +379,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	struct etnaviv_cmdbuf *cmdbuf;
>  	struct etnaviv_gpu *gpu;
>  	struct sync_file *sync_file = NULL;
> +	struct ww_acquire_ctx ticket;
>  	int out_fence_fd = -1;
>  	void *stream;
>  	int ret;
> @@ -466,10 +465,12 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> +	ww_acquire_init(&ticket, &reservation_ww_class);
> +
>  	submit = submit_create(dev, gpu, args->nr_bos);
>  	if (!submit) {
>  		ret = -ENOMEM;
> -		goto err_submit_cmds;
> +		goto err_submit_ww_acquire;
>  	}
>  
>  	submit->flags = args->flags;
> @@ -478,7 +479,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto err_submit_objects;
>  
> -	ret = submit_lock_objects(submit);
> +	ret = submit_lock_objects(submit, &ticket);
>  	if (ret)
>  		goto err_submit_objects;
>  
> @@ -545,6 +546,9 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  err_submit_objects:
>  	submit_cleanup(submit);
>  
> +err_submit_ww_acquire:
> +	ww_acquire_fini(&ticket);
> +
>  err_submit_cmds:
>  	if (ret && (out_fence_fd >= 0))
>  		put_unused_fd(out_fence_fd);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel