[PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Chunming Zhou]

this is an improvement for previous patch, the sched_sync is to store fence
that could be skipped as scheduled, when job is executed, we didn't need
pipeline_sync if all fences in sched_sync are signalled, otherwise insert
pipeline_sync still.

Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 787acd7..ef018bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1162,6 +1162,7 @@ struct amdgpu_job {
 	struct amdgpu_vm	*vm;
 	struct amdgpu_ring	*ring;
 	struct amdgpu_sync	sync;
+	struct amdgpu_sync	sched_sync;
 	struct amdgpu_ib	*ibs;
 	struct fence		*fence; /* the hw fence */
 	uint32_t		preamble_status;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 2c6624d..86ad507 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 {
 	struct amdgpu_device *adev = ring->adev;
 	struct amdgpu_ib *ib = &ibs[0];
+	struct fence *tmp;
 	bool skip_preamble, need_ctx_switch;
 	unsigned patch_offset = ~0;
 	struct amdgpu_vm *vm;
@@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 		return r;
 	}
 
-	if (ring->funcs->emit_pipeline_sync && job && job->need_pipeline_sync)
+	if (ring->funcs->emit_pipeline_sync && job &&
+	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
+		job->need_pipeline_sync = true;
 		amdgpu_ring_emit_pipeline_sync(ring);
+		fence_put(tmp);
+	}
 	if (vm) {
 		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast than DE */
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index cfa97ab..fa0c8b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
 	(*job)->need_pipeline_sync = false;
 
 	amdgpu_sync_create(&(*job)->sync);
+	amdgpu_sync_create(&(*job)->sched_sync);
 
 	return 0;
 }
@@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct amd_sched_job *s_job)
 
 	fence_put(job->fence);
 	amdgpu_sync_free(&job->sync);
+	amdgpu_sync_free(&job->sched_sync);
 	kfree(job);
 }
 
@@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
 
 	fence_put(job->fence);
 	amdgpu_sync_free(&job->sync);
+	amdgpu_sync_free(&job->sched_sync);
 	kfree(job);
 }
 
@@ -154,7 +157,7 @@ static struct fence *amdgpu_job_dependency(struct amd_sched_job *sched_job)
 	}
 
 	if (amd_sched_dependency_optimized(fence, sched_job->s_entity))
-		job->need_pipeline_sync = true;
+		amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
 
 	return fence;
 }
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] drm/amdgpu: make pipeline sync be in same place

[Chunming Zhou]

Change-Id: I0ccfa0e6de0cddbcca8dd85f2862240bc5ca02b3
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  |  6 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 34 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ef018bf..a583aab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1170,7 +1170,6 @@ struct amdgpu_job {
 	void			*owner;
 	uint64_t		fence_ctx; /* the fence_context this job uses */
 	bool                    vm_needs_flush;
-	bool			need_pipeline_sync;
 	unsigned		vm_id;
 	uint64_t		vm_pd_addr;
 	uint32_t		gds_base, gds_size;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 86ad507..dc250d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -121,7 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 {
 	struct amdgpu_device *adev = ring->adev;
 	struct amdgpu_ib *ib = &ibs[0];
-	struct fence *tmp;
+	struct fence *tmp = NULL;
 	bool skip_preamble, need_ctx_switch;
 	unsigned patch_offset = ~0;
 	struct amdgpu_vm *vm;
@@ -169,8 +169,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	}
 
 	if (ring->funcs->emit_pipeline_sync && job &&
-	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
-		job->need_pipeline_sync = true;
+	    ((tmp = amdgpu_sync_get_fence(&job->sched_sync)) ||
+	     amdgpu_vm_need_pipeline_sync(ring, job))) {
 		amdgpu_ring_emit_pipeline_sync(ring);
 		fence_put(tmp);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index fa0c8b1..adb7901 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -57,7 +57,6 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
 	(*job)->vm = vm;
 	(*job)->ibs = (void *)&(*job)[1];
 	(*job)->num_ibs = num_ibs;
-	(*job)->need_pipeline_sync = false;
 
 	amdgpu_sync_create(&(*job)->sync);
 	amdgpu_sync_create(&(*job)->sched_sync);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b4f83fc..58cde30 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -700,6 +700,37 @@ static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
 	return addr;
 }
 
+bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
+				  struct amdgpu_job *job)
+{
+	struct amdgpu_device *adev = ring->adev;
+	unsigned vmhub = ring->funcs->vmhub;
+	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
+	struct amdgpu_vm_id *id;
+	bool gds_switch_needed;
+	bool vm_flush_needed = job->vm_needs_flush ||
+		amdgpu_vm_ring_has_compute_vm_bug(ring);
+
+	if (job->vm_id == 0)
+		return false;
+	id = &id_mgr->ids[job->vm_id];
+	gds_switch_needed = ring->funcs->emit_gds_switch && (
+		id->gds_base != job->gds_base ||
+		id->gds_size != job->gds_size ||
+		id->gws_base != job->gws_base ||
+		id->gws_size != job->gws_size ||
+		id->oa_base != job->oa_base ||
+		id->oa_size != job->oa_size);
+
+	if (amdgpu_vm_had_gpu_reset(adev, id)) {
+		gds_switch_needed = true;
+		vm_flush_needed = true;
+	}
+	if (!vm_flush_needed && !gds_switch_needed)
+		return false;
+	return true;
+}
+
 /**
  * amdgpu_vm_flush - hardware flush the vm
  *
@@ -738,9 +769,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
 	if (ring->funcs->init_cond_exec)
 		patch_offset = amdgpu_ring_init_cond_exec(ring);
 
-	if (ring->funcs->emit_pipeline_sync && !job->need_pipeline_sync)
-		amdgpu_ring_emit_pipeline_sync(ring);
-
 	if (ring->funcs->emit_vm_flush && vm_flush_needed) {
 		u64 pd_addr = amdgpu_vm_adjust_mc_addr(adev, job->vm_pd_addr);
 		struct fence *fence;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 9828fcd..3d16169 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -254,5 +254,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 		      struct amdgpu_bo_va *bo_va);
 void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint64_t vm_size);
 int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
+bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
+				  struct amdgpu_job *job);
 
 #endif
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/amdgpu: id reset count only is updated when used end

[Chunming Zhou]

before that, we have function to check if reset happens by using reset count.

Change-Id: I2e941dd35295d4210d57a9593d39b5ee9021be9f
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 58cde30..bbb3587 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -450,7 +450,6 @@ static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
 		id->flushed_updates = fence_get(updates);
 	}
 	id->pd_gpu_addr = job->vm_pd_addr;
-	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
 	atomic64_set(&id->owner, vm->client_id);
 	job->vm_needs_flush = needs_flush;
 	if (needs_flush) {
@@ -598,7 +597,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 	id->pd_gpu_addr = job->vm_pd_addr;
 	fence_put(id->flushed_updates);
 	id->flushed_updates = fence_get(updates);
-	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
 	atomic64_set(&id->owner, vm->client_id);
 
 needs_flush:
@@ -783,6 +781,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
 		mutex_lock(&id_mgr->lock);
 		fence_put(id->last_flush);
 		id->last_flush = fence;
+		if (amdgpu_vm_had_gpu_reset(adev, id))
+			id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
 		mutex_unlock(&id_mgr->lock);
 	}
 
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] drm/amdgpu: check whether the vmid can be reused first

[Chunming Zhou]

Change-Id: If24a62b9c3097c9b040225ab0e768145b7a3db1e
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 92 +++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index bbb3587..6259608 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -492,51 +492,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 		mutex_unlock(&id_mgr->lock);
 		return r;
 	}
-	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
-	if (!fences) {
-		mutex_unlock(&id_mgr->lock);
-		return -ENOMEM;
-	}
-	/* Check if we have an idle VMID */
-	i = 0;
-	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
-		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
-		if (!fences[i])
-			break;
-		++i;
-	}
-
-	/* If we can't find a idle VMID to use, wait till one becomes available */
-	if (&idle->list == &id_mgr->ids_lru) {
-		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
-		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
-		struct fence_array *array;
-		unsigned j;
-
-		for (j = 0; j < i; ++j)
-			fence_get(fences[j]);
-
-		array = fence_array_create(i, fences, fence_context,
-					   seqno, true);
-		if (!array) {
-			for (j = 0; j < i; ++j)
-				fence_put(fences[j]);
-			kfree(fences);
-			r = -ENOMEM;
-			goto error;
-		}
-
-
-		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
-		fence_put(&array->base);
-		if (r)
-			goto error;
-
-		mutex_unlock(&id_mgr->lock);
-		return 0;
-
-	}
-	kfree(fences);
 
 	job->vm_needs_flush = false;
 	/* Check if we can use a VMID already assigned to this VM */
@@ -586,6 +541,53 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 
 	};
 
+	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
+	if (!fences) {
+		mutex_unlock(&id_mgr->lock);
+		return -ENOMEM;
+	}
+	/* Check if we have an idle VMID */
+	i = 0;
+	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
+		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
+		if (!fences[i])
+			break;
+		++i;
+	}
+
+	/* If we can't find a idle VMID to use, wait till one becomes available */
+	if (&idle->list == &id_mgr->ids_lru) {
+		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
+		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
+		struct fence_array *array;
+		unsigned j;
+
+		for (j = 0; j < i; ++j)
+			fence_get(fences[j]);
+
+		array = fence_array_create(i, fences, fence_context,
+					   seqno, true);
+		if (!array) {
+			for (j = 0; j < i; ++j)
+				fence_put(fences[j]);
+			kfree(fences);
+			r = -ENOMEM;
+			goto error;
+		}
+
+
+		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
+		fence_put(&array->base);
+		if (r)
+			goto error;
+
+		mutex_unlock(&id_mgr->lock);
+		return 0;
+
+	}
+	kfree(fences);
+
+
 	/* Still no ID to use? Then use the idle one found earlier */
 	id = idle;
 
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Zhang, Jerry (Junwei)]

On 05/10/2017 03:31 PM, Chunming Zhou wrote:
> this is an improvement for previous patch, the sched_sync is to store fence
> that could be skipped as scheduled, when job is executed, we didn't need
> pipeline_sync if all fences in sched_sync are signalled, otherwise insert
> pipeline_sync still.
>
> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>   3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 787acd7..ef018bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>   	struct amdgpu_vm	*vm;
>   	struct amdgpu_ring	*ring;
>   	struct amdgpu_sync	sync;
> +	struct amdgpu_sync	sched_sync;
>   	struct amdgpu_ib	*ibs;
>   	struct fence		*fence; /* the hw fence */
>   	uint32_t		preamble_status;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 2c6624d..86ad507 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   {
>   	struct amdgpu_device *adev = ring->adev;
>   	struct amdgpu_ib *ib = &ibs[0];
> +	struct fence *tmp;
>   	bool skip_preamble, need_ctx_switch;
>   	unsigned patch_offset = ~0;
>   	struct amdgpu_vm *vm;
> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   		return r;
>   	}
>
> -	if (ring->funcs->emit_pipeline_sync && job && job->need_pipeline_sync)
> +	if (ring->funcs->emit_pipeline_sync && job &&
> +	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
> +		job->need_pipeline_sync = true;
>   		amdgpu_ring_emit_pipeline_sync(ring);
> +		fence_put(tmp);
> +	}
>   	if (vm) {
>   		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast than DE */
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index cfa97ab..fa0c8b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>   	(*job)->need_pipeline_sync = false;
>
>   	amdgpu_sync_create(&(*job)->sync);
> +	amdgpu_sync_create(&(*job)->sched_sync);
>
>   	return 0;
>   }
> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct amd_sched_job *s_job)
>
>   	fence_put(job->fence);
>   	amdgpu_sync_free(&job->sync);
> +	amdgpu_sync_free(&job->sched_sync);
>   	kfree(job);
>   }
>
> @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>
>   	fence_put(job->fence);
>   	amdgpu_sync_free(&job->sync);
> +	amdgpu_sync_free(&job->sched_sync);
>   	kfree(job);
>   }
>
> @@ -154,7 +157,7 @@ static struct fence *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>   	}
>
>   	if (amd_sched_dependency_optimized(fence, sched_job->s_entity))
> -		job->need_pipeline_sync = true;
> +		amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>
>   	return fence;
>   }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amdgpu: make pipeline sync be in same place

[Zhang, Jerry (Junwei)]

On 05/10/2017 03:31 PM, Chunming Zhou wrote:
> Change-Id: I0ccfa0e6de0cddbcca8dd85f2862240bc5ca02b3
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  |  6 +++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 34 ++++++++++++++++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
>   5 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index ef018bf..a583aab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1170,7 +1170,6 @@ struct amdgpu_job {
>   	void			*owner;
>   	uint64_t		fence_ctx; /* the fence_context this job uses */
>   	bool                    vm_needs_flush;
> -	bool			need_pipeline_sync;
>   	unsigned		vm_id;
>   	uint64_t		vm_pd_addr;
>   	uint32_t		gds_base, gds_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 86ad507..dc250d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -121,7 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   {
>   	struct amdgpu_device *adev = ring->adev;
>   	struct amdgpu_ib *ib = &ibs[0];
> -	struct fence *tmp;
> +	struct fence *tmp = NULL;
>   	bool skip_preamble, need_ctx_switch;
>   	unsigned patch_offset = ~0;
>   	struct amdgpu_vm *vm;
> @@ -169,8 +169,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	}
>
>   	if (ring->funcs->emit_pipeline_sync && job &&
> -	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
> -		job->need_pipeline_sync = true;
> +	    ((tmp = amdgpu_sync_get_fence(&job->sched_sync)) ||
> +	     amdgpu_vm_need_pipeline_sync(ring, job))) {
>   		amdgpu_ring_emit_pipeline_sync(ring);
>   		fence_put(tmp);
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index fa0c8b1..adb7901 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -57,7 +57,6 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>   	(*job)->vm = vm;
>   	(*job)->ibs = (void *)&(*job)[1];
>   	(*job)->num_ibs = num_ibs;
> -	(*job)->need_pipeline_sync = false;
>
>   	amdgpu_sync_create(&(*job)->sync);
>   	amdgpu_sync_create(&(*job)->sched_sync);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b4f83fc..58cde30 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -700,6 +700,37 @@ static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
>   	return addr;
>   }
>
> +bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> +				  struct amdgpu_job *job)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	unsigned vmhub = ring->funcs->vmhub;
> +	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> +	struct amdgpu_vm_id *id;
> +	bool gds_switch_needed;
> +	bool vm_flush_needed = job->vm_needs_flush ||
> +		amdgpu_vm_ring_has_compute_vm_bug(ring);
> +
> +	if (job->vm_id == 0)
> +		return false;
> +	id = &id_mgr->ids[job->vm_id];
> +	gds_switch_needed = ring->funcs->emit_gds_switch && (
> +		id->gds_base != job->gds_base ||
> +		id->gds_size != job->gds_size ||
> +		id->gws_base != job->gws_base ||
> +		id->gws_size != job->gws_size ||
> +		id->oa_base != job->oa_base ||
> +		id->oa_size != job->oa_size);
> +
> +	if (amdgpu_vm_had_gpu_reset(adev, id)) {
> +		gds_switch_needed = true;
> +		vm_flush_needed = true;
> +	}
> +	if (!vm_flush_needed && !gds_switch_needed)
> +		return false;
> +	return true;
> +}
> +
>   /**
>    * amdgpu_vm_flush - hardware flush the vm
>    *
> @@ -738,9 +769,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
>   	if (ring->funcs->init_cond_exec)
>   		patch_offset = amdgpu_ring_init_cond_exec(ring);
>
> -	if (ring->funcs->emit_pipeline_sync && !job->need_pipeline_sync)
> -		amdgpu_ring_emit_pipeline_sync(ring);
> -

Just confirm:
this pipeline sync will be done when vm_flush job is scheduled?

If yes,
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Jerry

>   	if (ring->funcs->emit_vm_flush && vm_flush_needed) {
>   		u64 pd_addr = amdgpu_vm_adjust_mc_addr(adev, job->vm_pd_addr);
>   		struct fence *fence;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9828fcd..3d16169 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -254,5 +254,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>   		      struct amdgpu_bo_va *bo_va);
>   void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint64_t vm_size);
>   int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
> +bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> +				  struct amdgpu_job *job);
>
>   #endif
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdgpu: check whether the vmid can be reused first

[Zhang, Jerry (Junwei)]

On 05/10/2017 03:31 PM, Chunming Zhou wrote:
> Change-Id: If24a62b9c3097c9b040225ab0e768145b7a3db1e
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
I had same idea when read this code before.

Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 92 +++++++++++++++++-----------------
>   1 file changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bbb3587..6259608 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -492,51 +492,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   		mutex_unlock(&id_mgr->lock);
>   		return r;
>   	}
> -	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
> -	if (!fences) {
> -		mutex_unlock(&id_mgr->lock);
> -		return -ENOMEM;
> -	}
> -	/* Check if we have an idle VMID */
> -	i = 0;
> -	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
> -		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
> -		if (!fences[i])
> -			break;
> -		++i;
> -	}
> -
> -	/* If we can't find a idle VMID to use, wait till one becomes available */
> -	if (&idle->list == &id_mgr->ids_lru) {
> -		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
> -		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
> -		struct fence_array *array;
> -		unsigned j;
> -
> -		for (j = 0; j < i; ++j)
> -			fence_get(fences[j]);
> -
> -		array = fence_array_create(i, fences, fence_context,
> -					   seqno, true);
> -		if (!array) {
> -			for (j = 0; j < i; ++j)
> -				fence_put(fences[j]);
> -			kfree(fences);
> -			r = -ENOMEM;
> -			goto error;
> -		}
> -
> -
> -		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
> -		fence_put(&array->base);
> -		if (r)
> -			goto error;
> -
> -		mutex_unlock(&id_mgr->lock);
> -		return 0;
> -
> -	}
> -	kfree(fences);
>
>   	job->vm_needs_flush = false;
>   	/* Check if we can use a VMID already assigned to this VM */
> @@ -586,6 +541,53 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>
>   	};
>
> +	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
> +	if (!fences) {
> +		mutex_unlock(&id_mgr->lock);
> +		return -ENOMEM;
> +	}
> +	/* Check if we have an idle VMID */
> +	i = 0;
> +	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
> +		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
> +		if (!fences[i])
> +			break;
> +		++i;
> +	}
> +
> +	/* If we can't find a idle VMID to use, wait till one becomes available */
> +	if (&idle->list == &id_mgr->ids_lru) {
> +		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
> +		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
> +		struct fence_array *array;
> +		unsigned j;
> +
> +		for (j = 0; j < i; ++j)
> +			fence_get(fences[j]);
> +
> +		array = fence_array_create(i, fences, fence_context,
> +					   seqno, true);
> +		if (!array) {
> +			for (j = 0; j < i; ++j)
> +				fence_put(fences[j]);
> +			kfree(fences);
> +			r = -ENOMEM;
> +			goto error;
> +		}
> +
> +
> +		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
> +		fence_put(&array->base);
> +		if (r)
> +			goto error;
> +
> +		mutex_unlock(&id_mgr->lock);
> +		return 0;
> +
> +	}
> +	kfree(fences);
> +
> +
>   	/* Still no ID to use? Then use the idle one found earlier */
>   	id = idle;
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/4] drm/amdgpu: id reset count only is updated when used end

[Zhang, Jerry (Junwei)]

Could you elaborate the reason? bug fix, or sth else?

Jerry

On 05/10/2017 03:31 PM, Chunming Zhou wrote:
> before that, we have function to check if reset happens by using reset count.
>
> Change-Id: I2e941dd35295d4210d57a9593d39b5ee9021be9f
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 58cde30..bbb3587 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -450,7 +450,6 @@ static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
>   		id->flushed_updates = fence_get(updates);
>   	}
>   	id->pd_gpu_addr = job->vm_pd_addr;
> -	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
>   	atomic64_set(&id->owner, vm->client_id);
>   	job->vm_needs_flush = needs_flush;
>   	if (needs_flush) {
> @@ -598,7 +597,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	id->pd_gpu_addr = job->vm_pd_addr;
>   	fence_put(id->flushed_updates);
>   	id->flushed_updates = fence_get(updates);
> -	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
>   	atomic64_set(&id->owner, vm->client_id);
>
>   needs_flush:
> @@ -783,6 +781,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
>   		mutex_lock(&id_mgr->lock);
>   		fence_put(id->last_flush);
>   		id->last_flush = fence;
> +		if (amdgpu_vm_had_gpu_reset(adev, id))
> +			id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
>   		mutex_unlock(&id_mgr->lock);
>   	}
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdgpu: check whether the vmid can be reused first

[Christian König]

NAK, it is intentionally done like this to avoid starvation of processes.

In other words we assign a VMID only when there is at least one free.

Regards,
Christian.

Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
> Change-Id: If24a62b9c3097c9b040225ab0e768145b7a3db1e
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 92 +++++++++++++++++-----------------
>   1 file changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bbb3587..6259608 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -492,51 +492,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   		mutex_unlock(&id_mgr->lock);
>   		return r;
>   	}
> -	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
> -	if (!fences) {
> -		mutex_unlock(&id_mgr->lock);
> -		return -ENOMEM;
> -	}
> -	/* Check if we have an idle VMID */
> -	i = 0;
> -	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
> -		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
> -		if (!fences[i])
> -			break;
> -		++i;
> -	}
> -
> -	/* If we can't find a idle VMID to use, wait till one becomes available */
> -	if (&idle->list == &id_mgr->ids_lru) {
> -		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
> -		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
> -		struct fence_array *array;
> -		unsigned j;
> -
> -		for (j = 0; j < i; ++j)
> -			fence_get(fences[j]);
> -
> -		array = fence_array_create(i, fences, fence_context,
> -					   seqno, true);
> -		if (!array) {
> -			for (j = 0; j < i; ++j)
> -				fence_put(fences[j]);
> -			kfree(fences);
> -			r = -ENOMEM;
> -			goto error;
> -		}
> -
> -
> -		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
> -		fence_put(&array->base);
> -		if (r)
> -			goto error;
> -
> -		mutex_unlock(&id_mgr->lock);
> -		return 0;
> -
> -	}
> -	kfree(fences);
>   
>   	job->vm_needs_flush = false;
>   	/* Check if we can use a VMID already assigned to this VM */
> @@ -586,6 +541,53 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   
>   	};
>   
> +	fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
> +	if (!fences) {
> +		mutex_unlock(&id_mgr->lock);
> +		return -ENOMEM;
> +	}
> +	/* Check if we have an idle VMID */
> +	i = 0;
> +	list_for_each_entry(idle, &id_mgr->ids_lru, list) {
> +		fences[i] = amdgpu_sync_peek_fence(&idle->active, ring);
> +		if (!fences[i])
> +			break;
> +		++i;
> +	}
> +
> +	/* If we can't find a idle VMID to use, wait till one becomes available */
> +	if (&idle->list == &id_mgr->ids_lru) {
> +		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
> +		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
> +		struct fence_array *array;
> +		unsigned j;
> +
> +		for (j = 0; j < i; ++j)
> +			fence_get(fences[j]);
> +
> +		array = fence_array_create(i, fences, fence_context,
> +					   seqno, true);
> +		if (!array) {
> +			for (j = 0; j < i; ++j)
> +				fence_put(fences[j]);
> +			kfree(fences);
> +			r = -ENOMEM;
> +			goto error;
> +		}
> +
> +
> +		r = amdgpu_sync_fence(ring->adev, sync, &array->base);
> +		fence_put(&array->base);
> +		if (r)
> +			goto error;
> +
> +		mutex_unlock(&id_mgr->lock);
> +		return 0;
> +
> +	}
> +	kfree(fences);
> +
> +
>   	/* Still no ID to use? Then use the idle one found earlier */
>   	id = idle;
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/4] drm/amdgpu: id reset count only is updated when used end

[Christian König]

Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
> before that, we have function to check if reset happens by using reset count.
>
> Change-Id: I2e941dd35295d4210d57a9593d39b5ee9021be9f
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 58cde30..bbb3587 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -450,7 +450,6 @@ static int amdgpu_vm_grab_reserved_vmid_locked(struct amdgpu_vm *vm,
>   		id->flushed_updates = fence_get(updates);
>   	}
>   	id->pd_gpu_addr = job->vm_pd_addr;
> -	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
>   	atomic64_set(&id->owner, vm->client_id);
>   	job->vm_needs_flush = needs_flush;
>   	if (needs_flush) {
> @@ -598,7 +597,6 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	id->pd_gpu_addr = job->vm_pd_addr;
>   	fence_put(id->flushed_updates);
>   	id->flushed_updates = fence_get(updates);
> -	id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);
>   	atomic64_set(&id->owner, vm->client_id);
>   
>   needs_flush:
> @@ -783,6 +781,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
>   		mutex_lock(&id_mgr->lock);
>   		fence_put(id->last_flush);
>   		id->last_flush = fence;
> +		if (amdgpu_vm_had_gpu_reset(adev, id))
> +			id->current_gpu_reset_count = atomic_read(&adev->gpu_reset_counter);

Just drop the "if", checking the memory location first to avoid the 
write is rather pointless with an atomic.

With that fixed the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

>   		mutex_unlock(&id_mgr->lock);
>   	}
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Christian König]

Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
> this is an improvement for previous patch, the sched_sync is to store fence
> that could be skipped as scheduled, when job is executed, we didn't need
> pipeline_sync if all fences in sched_sync are signalled, otherwise insert
> pipeline_sync still.
>
> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>   3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 787acd7..ef018bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>   	struct amdgpu_vm	*vm;
>   	struct amdgpu_ring	*ring;
>   	struct amdgpu_sync	sync;
> +	struct amdgpu_sync	sched_sync;
>   	struct amdgpu_ib	*ibs;
>   	struct fence		*fence; /* the hw fence */
>   	uint32_t		preamble_status;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 2c6624d..86ad507 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   {
>   	struct amdgpu_device *adev = ring->adev;
>   	struct amdgpu_ib *ib = &ibs[0];
> +	struct fence *tmp;
>   	bool skip_preamble, need_ctx_switch;
>   	unsigned patch_offset = ~0;
>   	struct amdgpu_vm *vm;
> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   		return r;
>   	}
>   
> -	if (ring->funcs->emit_pipeline_sync && job && job->need_pipeline_sync)
> +	if (ring->funcs->emit_pipeline_sync && job &&
> +	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
> +		job->need_pipeline_sync = true;
>   		amdgpu_ring_emit_pipeline_sync(ring);
> +		fence_put(tmp);
> +	}
>   	if (vm) {
>   		amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go too fast than DE */
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index cfa97ab..fa0c8b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>   	(*job)->need_pipeline_sync = false;
>   
>   	amdgpu_sync_create(&(*job)->sync);
> +	amdgpu_sync_create(&(*job)->sched_sync);
>   
>   	return 0;
>   }
> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct amd_sched_job *s_job)
>   
>   	fence_put(job->fence);
>   	amdgpu_sync_free(&job->sync);
> +	amdgpu_sync_free(&job->sched_sync);
>   	kfree(job);
>   }
>   
> @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>   
>   	fence_put(job->fence);
>   	amdgpu_sync_free(&job->sync);
> +	amdgpu_sync_free(&job->sched_sync);
>   	kfree(job);
>   }
>   
> @@ -154,7 +157,7 @@ static struct fence *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>   	}
>   
>   	if (amd_sched_dependency_optimized(fence, sched_job->s_entity))
> -		job->need_pipeline_sync = true;
> +		amdgpu_sync_fence(job->adev, &job->sched_sync, fence);

This can result in an -ENOMEM and additional to that we only need to 
remember the last fence optimized like this, not all of them.

So just keep the last one found here in job->sched_fence instead.

Regards,
Christian.

>   
>   	return fence;
>   }


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amdgpu: make pipeline sync be in same place

[Christian König]

Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
> Change-Id: I0ccfa0e6de0cddbcca8dd85f2862240bc5ca02b3
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  |  6 +++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 34 ++++++++++++++++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
>   5 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index ef018bf..a583aab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1170,7 +1170,6 @@ struct amdgpu_job {
>   	void			*owner;
>   	uint64_t		fence_ctx; /* the fence_context this job uses */
>   	bool                    vm_needs_flush;
> -	bool			need_pipeline_sync;
>   	unsigned		vm_id;
>   	uint64_t		vm_pd_addr;
>   	uint32_t		gds_base, gds_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 86ad507..dc250d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -121,7 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   {
>   	struct amdgpu_device *adev = ring->adev;
>   	struct amdgpu_ib *ib = &ibs[0];
> -	struct fence *tmp;
> +	struct fence *tmp = NULL;
>   	bool skip_preamble, need_ctx_switch;
>   	unsigned patch_offset = ~0;
>   	struct amdgpu_vm *vm;
> @@ -169,8 +169,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	}
>   
>   	if (ring->funcs->emit_pipeline_sync && job &&
> -	    (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
> -		job->need_pipeline_sync = true;
> +	    ((tmp = amdgpu_sync_get_fence(&job->sched_sync)) ||
> +	     amdgpu_vm_need_pipeline_sync(ring, job))) {
>   		amdgpu_ring_emit_pipeline_sync(ring);
>   		fence_put(tmp);
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index fa0c8b1..adb7901 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -57,7 +57,6 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>   	(*job)->vm = vm;
>   	(*job)->ibs = (void *)&(*job)[1];
>   	(*job)->num_ibs = num_ibs;
> -	(*job)->need_pipeline_sync = false;
>   
>   	amdgpu_sync_create(&(*job)->sync);
>   	amdgpu_sync_create(&(*job)->sched_sync);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b4f83fc..58cde30 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -700,6 +700,37 @@ static u64 amdgpu_vm_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
>   	return addr;
>   }
>   
> +bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> +				  struct amdgpu_job *job)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	unsigned vmhub = ring->funcs->vmhub;
> +	struct amdgpu_vm_id_manager *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> +	struct amdgpu_vm_id *id;
> +	bool gds_switch_needed;
> +	bool vm_flush_needed = job->vm_needs_flush ||
> +		amdgpu_vm_ring_has_compute_vm_bug(ring);
> +
> +	if (job->vm_id == 0)
> +		return false;
> +	id = &id_mgr->ids[job->vm_id];
> +	gds_switch_needed = ring->funcs->emit_gds_switch && (
> +		id->gds_base != job->gds_base ||
> +		id->gds_size != job->gds_size ||
> +		id->gws_base != job->gws_base ||
> +		id->gws_size != job->gws_size ||
> +		id->oa_base != job->oa_base ||
> +		id->oa_size != job->oa_size);
> +
> +	if (amdgpu_vm_had_gpu_reset(adev, id)) {
> +		gds_switch_needed = true;
> +		vm_flush_needed = true;

You can shortcut here, e.g. just return true. Same above if you use an "if".

Apart from that looks good to me.

Christian.

> +	}
> +	if (!vm_flush_needed && !gds_switch_needed)
> +		return false;
> +	return true;
> +}
> +
>   /**
>    * amdgpu_vm_flush - hardware flush the vm
>    *
> @@ -738,9 +769,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job)
>   	if (ring->funcs->init_cond_exec)
>   		patch_offset = amdgpu_ring_init_cond_exec(ring);
>   
> -	if (ring->funcs->emit_pipeline_sync && !job->need_pipeline_sync)
> -		amdgpu_ring_emit_pipeline_sync(ring);
> -
>   	if (ring->funcs->emit_vm_flush && vm_flush_needed) {
>   		u64 pd_addr = amdgpu_vm_adjust_mc_addr(adev, job->vm_pd_addr);
>   		struct fence *fence;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9828fcd..3d16169 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -254,5 +254,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>   		      struct amdgpu_bo_va *bo_va);
>   void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint64_t vm_size);
>   int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
> +bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> +				  struct amdgpu_job *job);
>   
>   #endif


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[zhoucm1]

On 2017年05月10日 16:26, Christian König wrote:
> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>> this is an improvement for previous patch, the sched_sync is to store 
>> fence
>> that could be skipped as scheduled, when job is executed, we didn't need
>> pipeline_sync if all fences in sched_sync are signalled, otherwise 
>> insert
>> pipeline_sync still.
>>
>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 787acd7..ef018bf 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>       struct amdgpu_vm    *vm;
>>       struct amdgpu_ring    *ring;
>>       struct amdgpu_sync    sync;
>> +    struct amdgpu_sync    sched_sync;
>>       struct amdgpu_ib    *ibs;
>>       struct fence        *fence; /* the hw fence */
>>       uint32_t        preamble_status;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> index 2c6624d..86ad507 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>   {
>>       struct amdgpu_device *adev = ring->adev;
>>       struct amdgpu_ib *ib = &ibs[0];
>> +    struct fence *tmp;
>>       bool skip_preamble, need_ctx_switch;
>>       unsigned patch_offset = ~0;
>>       struct amdgpu_vm *vm;
>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>           return r;
>>       }
>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>> job->need_pipeline_sync)
>> +    if (ring->funcs->emit_pipeline_sync && job &&
>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>> +        job->need_pipeline_sync = true;
>>           amdgpu_ring_emit_pipeline_sync(ring);
>> +        fence_put(tmp);
>> +    }
>>       if (vm) {
>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go 
>> too fast than DE */
>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> index cfa97ab..fa0c8b1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, 
>> unsigned num_ibs,
>>       (*job)->need_pipeline_sync = false;
>>         amdgpu_sync_create(&(*job)->sync);
>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>         return 0;
>>   }
>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct amd_sched_job 
>> *s_job)
>>         fence_put(job->fence);
>>       amdgpu_sync_free(&job->sync);
>> +    amdgpu_sync_free(&job->sched_sync);
>>       kfree(job);
>>   }
>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>         fence_put(job->fence);
>>       amdgpu_sync_free(&job->sync);
>> +    amdgpu_sync_free(&job->sched_sync);
>>       kfree(job);
>>   }
>>   @@ -154,7 +157,7 @@ static struct fence 
>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>       }
>>         if (amd_sched_dependency_optimized(fence, sched_job->s_entity))
>> -        job->need_pipeline_sync = true;
>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>
> This can result in an -ENOMEM 
will handle it.
> and additional to that we only need to remember the last fence 
> optimized like this, not all of them.
>
> So just keep the last one found here in job->sched_fence instead.
I guess this isn't enough.
The dependency is not in order when calling, so the last one is not 
always the last scheduled fence.
And they could be sched fence not hw fence, although they are handled by 
same hw ring, but the sched fence context isn't same.
so we still need sched_sync here, right?

Regards,
David zhou
>
> Regards,
> Christian.
>
>>         return fence;
>>   }
>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Christian König]

Am 10.05.2017 um 10:38 schrieb zhoucm1:
>
>
> On 2017年05月10日 16:26, Christian König wrote:
>> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>>> this is an improvement for previous patch, the sched_sync is to 
>>> store fence
>>> that could be skipped as scheduled, when job is executed, we didn't 
>>> need
>>> pipeline_sync if all fences in sched_sync are signalled, otherwise 
>>> insert
>>> pipeline_sync still.
>>>
>>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 787acd7..ef018bf 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>>       struct amdgpu_vm    *vm;
>>>       struct amdgpu_ring    *ring;
>>>       struct amdgpu_sync    sync;
>>> +    struct amdgpu_sync    sched_sync;
>>>       struct amdgpu_ib    *ibs;
>>>       struct fence        *fence; /* the hw fence */
>>>       uint32_t        preamble_status;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> index 2c6624d..86ad507 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>>> unsigned num_ibs,
>>>   {
>>>       struct amdgpu_device *adev = ring->adev;
>>>       struct amdgpu_ib *ib = &ibs[0];
>>> +    struct fence *tmp;
>>>       bool skip_preamble, need_ctx_switch;
>>>       unsigned patch_offset = ~0;
>>>       struct amdgpu_vm *vm;
>>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>> *ring, unsigned num_ibs,
>>>           return r;
>>>       }
>>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>>> job->need_pipeline_sync)
>>> +    if (ring->funcs->emit_pipeline_sync && job &&
>>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>>> +        job->need_pipeline_sync = true;
>>>           amdgpu_ring_emit_pipeline_sync(ring);
>>> +        fence_put(tmp);
>>> +    }
>>>       if (vm) {
>>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go 
>>> too fast than DE */
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index cfa97ab..fa0c8b1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, 
>>> unsigned num_ibs,
>>>       (*job)->need_pipeline_sync = false;
>>>         amdgpu_sync_create(&(*job)->sync);
>>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>>         return 0;
>>>   }
>>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct 
>>> amd_sched_job *s_job)
>>>         fence_put(job->fence);
>>>       amdgpu_sync_free(&job->sync);
>>> +    amdgpu_sync_free(&job->sched_sync);
>>>       kfree(job);
>>>   }
>>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>>         fence_put(job->fence);
>>>       amdgpu_sync_free(&job->sync);
>>> +    amdgpu_sync_free(&job->sched_sync);
>>>       kfree(job);
>>>   }
>>>   @@ -154,7 +157,7 @@ static struct fence 
>>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>>       }
>>>         if (amd_sched_dependency_optimized(fence, sched_job->s_entity))
>>> -        job->need_pipeline_sync = true;
>>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>>
>> This can result in an -ENOMEM 
> will handle it.
>> and additional to that we only need to remember the last fence 
>> optimized like this, not all of them.
>>
>> So just keep the last one found here in job->sched_fence instead.
> I guess this isn't enough.
> The dependency is not in order when calling, so the last one is not 
> always the last scheduled fence.
> And they could be sched fence not hw fence, although they are handled 
> by same hw ring, but the sched fence context isn't same.
> so we still need sched_sync here, right?

No, amdgpu_job_dependency is only called again when the returned fence 
is signaled (or scheduled on the same ring).

So when this is called and you find that you need to wait for another 
fence the order is guaranteed.

Regards,
Christian.

>
> Regards,
> David zhou
>>
>> Regards,
>> Christian.
>>
>>>         return fence;
>>>   }
>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[zhoucm1]

On 2017年05月10日 16:50, Christian König wrote:
> Am 10.05.2017 um 10:38 schrieb zhoucm1:
>>
>>
>> On 2017年05月10日 16:26, Christian König wrote:
>>> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>>>> this is an improvement for previous patch, the sched_sync is to 
>>>> store fence
>>>> that could be skipped as scheduled, when job is executed, we didn't 
>>>> need
>>>> pipeline_sync if all fences in sched_sync are signalled, otherwise 
>>>> insert
>>>> pipeline_sync still.
>>>>
>>>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index 787acd7..ef018bf 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>>>       struct amdgpu_vm    *vm;
>>>>       struct amdgpu_ring    *ring;
>>>>       struct amdgpu_sync    sync;
>>>> +    struct amdgpu_sync    sched_sync;
>>>>       struct amdgpu_ib    *ibs;
>>>>       struct fence        *fence; /* the hw fence */
>>>>       uint32_t        preamble_status;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>> index 2c6624d..86ad507 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>> *ring, unsigned num_ibs,
>>>>   {
>>>>       struct amdgpu_device *adev = ring->adev;
>>>>       struct amdgpu_ib *ib = &ibs[0];
>>>> +    struct fence *tmp;
>>>>       bool skip_preamble, need_ctx_switch;
>>>>       unsigned patch_offset = ~0;
>>>>       struct amdgpu_vm *vm;
>>>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>> *ring, unsigned num_ibs,
>>>>           return r;
>>>>       }
>>>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>>>> job->need_pipeline_sync)
>>>> +    if (ring->funcs->emit_pipeline_sync && job &&
>>>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>>>> +        job->need_pipeline_sync = true;
>>>>           amdgpu_ring_emit_pipeline_sync(ring);
>>>> +        fence_put(tmp);
>>>> +    }
>>>>       if (vm) {
>>>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE go 
>>>> too fast than DE */
>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> index cfa97ab..fa0c8b1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, 
>>>> unsigned num_ibs,
>>>>       (*job)->need_pipeline_sync = false;
>>>>         amdgpu_sync_create(&(*job)->sync);
>>>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>>>         return 0;
>>>>   }
>>>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct 
>>>> amd_sched_job *s_job)
>>>>         fence_put(job->fence);
>>>>       amdgpu_sync_free(&job->sync);
>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>       kfree(job);
>>>>   }
>>>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>>>         fence_put(job->fence);
>>>>       amdgpu_sync_free(&job->sync);
>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>       kfree(job);
>>>>   }
>>>>   @@ -154,7 +157,7 @@ static struct fence 
>>>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>>>       }
>>>>         if (amd_sched_dependency_optimized(fence, 
>>>> sched_job->s_entity))
>>>> -        job->need_pipeline_sync = true;
>>>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>>>
>>> This can result in an -ENOMEM 
>> will handle it.
>>> and additional to that we only need to remember the last fence 
>>> optimized like this, not all of them.
>>>
>>> So just keep the last one found here in job->sched_fence instead.
>> I guess this isn't enough.
>> The dependency is not in order when calling, so the last one is not 
>> always the last scheduled fence.
>> And they could be sched fence not hw fence, although they are handled 
>> by same hw ring, but the sched fence context isn't same.
>> so we still need sched_sync here, right?
>
> No, amdgpu_job_dependency is only called again when the returned fence 
> is signaled (or scheduled on the same ring).
Let use give an example for it:
Assume job->sync has two fences(fenceA and fenceB) which could be 
scheduled. fenceA is from entity1, fenceB is from entity2, but both for 
gfx engine, but fenceA could be submitted to hw ring behind fenceB.
the order in job->sync list is: others---->fenceA---->fenceB--->others.
when calling amdgpu_job_dependency, fenceA will be checked first, and 
then fenceB.

If following your proposal, we only store fenceB, but fenceA is the 
later. Which isn't  expected.

Regards,
David Zhou
>
> So when this is called and you find that you need to wait for another 
> fence the order is guaranteed.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David zhou
>>>
>>> Regards,
>>> Christian.
>>>
>>>>         return fence;
>>>>   }
>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[zhoucm1]

On 2017年05月10日 17:21, Christian König wrote:
> Am 10.05.2017 um 11:00 schrieb zhoucm1:
>>
>>
>> On 2017年05月10日 16:50, Christian König wrote:
>>> Am 10.05.2017 um 10:38 schrieb zhoucm1:
>>>>
>>>>
>>>> On 2017年05月10日 16:26, Christian König wrote:
>>>>> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>>>>>> this is an improvement for previous patch, the sched_sync is to 
>>>>>> store fence
>>>>>> that could be skipped as scheduled, when job is executed, we 
>>>>>> didn't need
>>>>>> pipeline_sync if all fences in sched_sync are signalled, 
>>>>>> otherwise insert
>>>>>> pipeline_sync still.
>>>>>>
>>>>>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>>>>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> index 787acd7..ef018bf 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>>>>>       struct amdgpu_vm    *vm;
>>>>>>       struct amdgpu_ring    *ring;
>>>>>>       struct amdgpu_sync    sync;
>>>>>> +    struct amdgpu_sync    sched_sync;
>>>>>>       struct amdgpu_ib    *ibs;
>>>>>>       struct fence        *fence; /* the hw fence */
>>>>>>       uint32_t        preamble_status;
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>> index 2c6624d..86ad507 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>>> *ring, unsigned num_ibs,
>>>>>>   {
>>>>>>       struct amdgpu_device *adev = ring->adev;
>>>>>>       struct amdgpu_ib *ib = &ibs[0];
>>>>>> +    struct fence *tmp;
>>>>>>       bool skip_preamble, need_ctx_switch;
>>>>>>       unsigned patch_offset = ~0;
>>>>>>       struct amdgpu_vm *vm;
>>>>>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>>> *ring, unsigned num_ibs,
>>>>>>           return r;
>>>>>>       }
>>>>>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>>>>>> job->need_pipeline_sync)
>>>>>> +    if (ring->funcs->emit_pipeline_sync && job &&
>>>>>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>>>>>> +        job->need_pipeline_sync = true;
>>>>>>           amdgpu_ring_emit_pipeline_sync(ring);
>>>>>> +        fence_put(tmp);
>>>>>> +    }
>>>>>>       if (vm) {
>>>>>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE 
>>>>>> go too fast than DE */
>>>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> index cfa97ab..fa0c8b1 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device 
>>>>>> *adev, unsigned num_ibs,
>>>>>>       (*job)->need_pipeline_sync = false;
>>>>>>         amdgpu_sync_create(&(*job)->sync);
>>>>>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>>>>>         return 0;
>>>>>>   }
>>>>>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct 
>>>>>> amd_sched_job *s_job)
>>>>>>         fence_put(job->fence);
>>>>>>       amdgpu_sync_free(&job->sync);
>>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>>       kfree(job);
>>>>>>   }
>>>>>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>>>>>         fence_put(job->fence);
>>>>>>       amdgpu_sync_free(&job->sync);
>>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>>       kfree(job);
>>>>>>   }
>>>>>>   @@ -154,7 +157,7 @@ static struct fence 
>>>>>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>>>>>       }
>>>>>>         if (amd_sched_dependency_optimized(fence, 
>>>>>> sched_job->s_entity))
>>>>>> -        job->need_pipeline_sync = true;
>>>>>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>>>>>
>>>>> This can result in an -ENOMEM 
>>>> will handle it.
>>>>> and additional to that we only need to remember the last fence 
>>>>> optimized like this, not all of them.
>>>>>
>>>>> So just keep the last one found here in job->sched_fence instead.
>>>> I guess this isn't enough.
>>>> The dependency is not in order when calling, so the last one is not 
>>>> always the last scheduled fence.
>>>> And they could be sched fence not hw fence, although they are 
>>>> handled by same hw ring, but the sched fence context isn't same.
>>>> so we still need sched_sync here, right?
>>>
>>> No, amdgpu_job_dependency is only called again when the returned 
>>> fence is signaled (or scheduled on the same ring).
>> Let use give an example for it:
>> Assume job->sync has two fences(fenceA and fenceB) which could be 
>> scheduled. fenceA is from entity1, fenceB is from entity2, but both 
>> for gfx engine, but fenceA could be submitted to hw ring behind fenceB.
>> the order in job->sync list is: others---->fenceA---->fenceB--->others.
>> when calling amdgpu_job_dependency, fenceA will be checked first, and 
>> then fenceB.
>>
>> If following your proposal, we only store fenceB, but fenceA is the 
>> later. Which isn't  expected.
>
> Ah! Indeed, I didn't realized that the dependent fence could have 
> already been scheduled.
>
> Mhm, how are we going to handle the out of memory situation then? Sine 
> we are inside a kernel thread we are not supposed to fail at this point.
like grab vmid failed case, add DRM_ERROR, is it ok?

Regards,
David Zhou
>
> Regards,
> Christian.
>
>>
>>
>> Regards,
>> David Zhou
>>>
>>> So when this is called and you find that you need to wait for 
>>> another fence the order is guaranteed.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> David zhou
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>         return fence;
>>>>>>   }
>>>>>
>>>>>
>>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Christian König]

Am 10.05.2017 um 11:00 schrieb zhoucm1:
>
>
> On 2017年05月10日 16:50, Christian König wrote:
>> Am 10.05.2017 um 10:38 schrieb zhoucm1:
>>>
>>>
>>> On 2017年05月10日 16:26, Christian König wrote:
>>>> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>>>>> this is an improvement for previous patch, the sched_sync is to 
>>>>> store fence
>>>>> that could be skipped as scheduled, when job is executed, we 
>>>>> didn't need
>>>>> pipeline_sync if all fences in sched_sync are signalled, otherwise 
>>>>> insert
>>>>> pipeline_sync still.
>>>>>
>>>>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>>>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> index 787acd7..ef018bf 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>>>>       struct amdgpu_vm    *vm;
>>>>>       struct amdgpu_ring    *ring;
>>>>>       struct amdgpu_sync    sync;
>>>>> +    struct amdgpu_sync    sched_sync;
>>>>>       struct amdgpu_ib    *ibs;
>>>>>       struct fence        *fence; /* the hw fence */
>>>>>       uint32_t        preamble_status;
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>> index 2c6624d..86ad507 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>> *ring, unsigned num_ibs,
>>>>>   {
>>>>>       struct amdgpu_device *adev = ring->adev;
>>>>>       struct amdgpu_ib *ib = &ibs[0];
>>>>> +    struct fence *tmp;
>>>>>       bool skip_preamble, need_ctx_switch;
>>>>>       unsigned patch_offset = ~0;
>>>>>       struct amdgpu_vm *vm;
>>>>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>> *ring, unsigned num_ibs,
>>>>>           return r;
>>>>>       }
>>>>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>>>>> job->need_pipeline_sync)
>>>>> +    if (ring->funcs->emit_pipeline_sync && job &&
>>>>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>>>>> +        job->need_pipeline_sync = true;
>>>>>           amdgpu_ring_emit_pipeline_sync(ring);
>>>>> +        fence_put(tmp);
>>>>> +    }
>>>>>       if (vm) {
>>>>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE 
>>>>> go too fast than DE */
>>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> index cfa97ab..fa0c8b1 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, 
>>>>> unsigned num_ibs,
>>>>>       (*job)->need_pipeline_sync = false;
>>>>>         amdgpu_sync_create(&(*job)->sync);
>>>>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>>>>         return 0;
>>>>>   }
>>>>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct 
>>>>> amd_sched_job *s_job)
>>>>>         fence_put(job->fence);
>>>>>       amdgpu_sync_free(&job->sync);
>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>       kfree(job);
>>>>>   }
>>>>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>>>>         fence_put(job->fence);
>>>>>       amdgpu_sync_free(&job->sync);
>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>       kfree(job);
>>>>>   }
>>>>>   @@ -154,7 +157,7 @@ static struct fence 
>>>>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>>>>       }
>>>>>         if (amd_sched_dependency_optimized(fence, 
>>>>> sched_job->s_entity))
>>>>> -        job->need_pipeline_sync = true;
>>>>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>>>>
>>>> This can result in an -ENOMEM 
>>> will handle it.
>>>> and additional to that we only need to remember the last fence 
>>>> optimized like this, not all of them.
>>>>
>>>> So just keep the last one found here in job->sched_fence instead.
>>> I guess this isn't enough.
>>> The dependency is not in order when calling, so the last one is not 
>>> always the last scheduled fence.
>>> And they could be sched fence not hw fence, although they are 
>>> handled by same hw ring, but the sched fence context isn't same.
>>> so we still need sched_sync here, right?
>>
>> No, amdgpu_job_dependency is only called again when the returned 
>> fence is signaled (or scheduled on the same ring).
> Let use give an example for it:
> Assume job->sync has two fences(fenceA and fenceB) which could be 
> scheduled. fenceA is from entity1, fenceB is from entity2, but both 
> for gfx engine, but fenceA could be submitted to hw ring behind fenceB.
> the order in job->sync list is: others---->fenceA---->fenceB--->others.
> when calling amdgpu_job_dependency, fenceA will be checked first, and 
> then fenceB.
>
> If following your proposal, we only store fenceB, but fenceA is the 
> later. Which isn't  expected.

Ah! Indeed, I didn't realized that the dependent fence could have 
already been scheduled.

Mhm, how are we going to handle the out of memory situation then? Sine 
we are inside a kernel thread we are not supposed to fail at this point.

Regards,
Christian.

>
>
> Regards,
> David Zhou
>>
>> So when this is called and you find that you need to wait for another 
>> fence the order is guaranteed.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David zhou
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>         return fence;
>>>>>   }
>>>>
>>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] drm/amdgpu: add sched sync for amdgpu job

[Christian König]

Am 10.05.2017 um 11:20 schrieb zhoucm1:
>
>
> On 2017年05月10日 17:21, Christian König wrote:
>> Am 10.05.2017 um 11:00 schrieb zhoucm1:
>>>
>>>
>>> On 2017年05月10日 16:50, Christian König wrote:
>>>> Am 10.05.2017 um 10:38 schrieb zhoucm1:
>>>>>
>>>>>
>>>>> On 2017年05月10日 16:26, Christian König wrote:
>>>>>> Am 10.05.2017 um 09:31 schrieb Chunming Zhou:
>>>>>>> this is an improvement for previous patch, the sched_sync is to 
>>>>>>> store fence
>>>>>>> that could be skipped as scheduled, when job is executed, we 
>>>>>>> didn't need
>>>>>>> pipeline_sync if all fences in sched_sync are signalled, 
>>>>>>> otherwise insert
>>>>>>> pipeline_sync still.
>>>>>>>
>>>>>>> Change-Id: I26d3a2794272ba94b25753d4bf367326d12f6939
>>>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 1 +
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 7 ++++++-
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 ++++-
>>>>>>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> index 787acd7..ef018bf 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>> @@ -1162,6 +1162,7 @@ struct amdgpu_job {
>>>>>>>       struct amdgpu_vm    *vm;
>>>>>>>       struct amdgpu_ring    *ring;
>>>>>>>       struct amdgpu_sync    sync;
>>>>>>> +    struct amdgpu_sync    sched_sync;
>>>>>>>       struct amdgpu_ib    *ibs;
>>>>>>>       struct fence        *fence; /* the hw fence */
>>>>>>>       uint32_t        preamble_status;
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>>> index 2c6624d..86ad507 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>>>>>> @@ -121,6 +121,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>>>> *ring, unsigned num_ibs,
>>>>>>>   {
>>>>>>>       struct amdgpu_device *adev = ring->adev;
>>>>>>>       struct amdgpu_ib *ib = &ibs[0];
>>>>>>> +    struct fence *tmp;
>>>>>>>       bool skip_preamble, need_ctx_switch;
>>>>>>>       unsigned patch_offset = ~0;
>>>>>>>       struct amdgpu_vm *vm;
>>>>>>> @@ -167,8 +168,12 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>>>>>> *ring, unsigned num_ibs,
>>>>>>>           return r;
>>>>>>>       }
>>>>>>>   -    if (ring->funcs->emit_pipeline_sync && job && 
>>>>>>> job->need_pipeline_sync)
>>>>>>> +    if (ring->funcs->emit_pipeline_sync && job &&
>>>>>>> +        (tmp = amdgpu_sync_get_fence(&job->sched_sync))) {
>>>>>>> +        job->need_pipeline_sync = true;
>>>>>>>           amdgpu_ring_emit_pipeline_sync(ring);
>>>>>>> +        fence_put(tmp);
>>>>>>> +    }
>>>>>>>       if (vm) {
>>>>>>>           amdgpu_ring_insert_nop(ring, extra_nop); /* prevent CE 
>>>>>>> go too fast than DE */
>>>>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> index cfa97ab..fa0c8b1 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> @@ -60,6 +60,7 @@ int amdgpu_job_alloc(struct amdgpu_device 
>>>>>>> *adev, unsigned num_ibs,
>>>>>>>       (*job)->need_pipeline_sync = false;
>>>>>>>         amdgpu_sync_create(&(*job)->sync);
>>>>>>> +    amdgpu_sync_create(&(*job)->sched_sync);
>>>>>>>         return 0;
>>>>>>>   }
>>>>>>> @@ -98,6 +99,7 @@ static void amdgpu_job_free_cb(struct 
>>>>>>> amd_sched_job *s_job)
>>>>>>>         fence_put(job->fence);
>>>>>>>       amdgpu_sync_free(&job->sync);
>>>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>>>       kfree(job);
>>>>>>>   }
>>>>>>>   @@ -107,6 +109,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
>>>>>>>         fence_put(job->fence);
>>>>>>>       amdgpu_sync_free(&job->sync);
>>>>>>> +    amdgpu_sync_free(&job->sched_sync);
>>>>>>>       kfree(job);
>>>>>>>   }
>>>>>>>   @@ -154,7 +157,7 @@ static struct fence 
>>>>>>> *amdgpu_job_dependency(struct amd_sched_job *sched_job)
>>>>>>>       }
>>>>>>>         if (amd_sched_dependency_optimized(fence, 
>>>>>>> sched_job->s_entity))
>>>>>>> -        job->need_pipeline_sync = true;
>>>>>>> +        amdgpu_sync_fence(job->adev, &job->sched_sync, fence);
>>>>>>
>>>>>> This can result in an -ENOMEM 
>>>>> will handle it.
>>>>>> and additional to that we only need to remember the last fence 
>>>>>> optimized like this, not all of them.
>>>>>>
>>>>>> So just keep the last one found here in job->sched_fence instead.
>>>>> I guess this isn't enough.
>>>>> The dependency is not in order when calling, so the last one is 
>>>>> not always the last scheduled fence.
>>>>> And they could be sched fence not hw fence, although they are 
>>>>> handled by same hw ring, but the sched fence context isn't same.
>>>>> so we still need sched_sync here, right?
>>>>
>>>> No, amdgpu_job_dependency is only called again when the returned 
>>>> fence is signaled (or scheduled on the same ring).
>>> Let use give an example for it:
>>> Assume job->sync has two fences(fenceA and fenceB) which could be 
>>> scheduled. fenceA is from entity1, fenceB is from entity2, but both 
>>> for gfx engine, but fenceA could be submitted to hw ring behind fenceB.
>>> the order in job->sync list is: others---->fenceA---->fenceB--->others.
>>> when calling amdgpu_job_dependency, fenceA will be checked first, 
>>> and then fenceB.
>>>
>>> If following your proposal, we only store fenceB, but fenceA is the 
>>> later. Which isn't  expected.
>>
>> Ah! Indeed, I didn't realized that the dependent fence could have 
>> already been scheduled.
>>
>> Mhm, how are we going to handle the out of memory situation then? 
>> Sine we are inside a kernel thread we are not supposed to fail at 
>> this point.
> like grab vmid failed case, add DRM_ERROR, is it ok?

Not ideal, but should at least work for the moment.

Christian.

>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> Regards,
>>> David Zhou
>>>>
>>>> So when this is called and you find that you need to wait for 
>>>> another fence the order is guaranteed.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> David zhou
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>         return fence;
>>>>>>>   }
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx