[PATCH 1/2] drm/amdgpu: Skip drm_sched_entity realted ops for KIQ ring.

[PATCH 1/2] drm/amdgpu: Skip drm_sched_entity realted ops for KIQ ring.

[Andrey Grodzovsky]

Follwoing change 75fbed2 we need to skip KIQ ring when iterating
amdgpu_ctx's scheduler entites.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 6741a62..744519b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -173,9 +173,14 @@ static void amdgpu_ctx_do_release(struct kref *ref)
 
 	ctx = container_of(ref, struct amdgpu_ctx, refcount);
 
-	for (i = 0; i < ctx->adev->num_rings; i++)
+	for (i = 0; i < ctx->adev->num_rings; i++) {
+
+		if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
+			continue;
+
 		drm_sched_entity_fini(&ctx->adev->rings[i]->sched,
 			&ctx->rings[i].entity);
+	}
 
 	amdgpu_ctx_fini(ref);
 }
@@ -452,12 +457,17 @@ void amdgpu_ctx_mgr_entity_fini(struct amdgpu_ctx_mgr *mgr)
 		if (!ctx->adev)
 			return;
 
-		for (i = 0; i < ctx->adev->num_rings; i++)
+		for (i = 0; i < ctx->adev->num_rings; i++) {
+
+			if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
+				continue;
+
 			if (kref_read(&ctx->refcount) == 1)
 				drm_sched_entity_do_release(&ctx->adev->rings[i]->sched,
 						  &ctx->rings[i].entity);
 			else
 				DRM_ERROR("ctx %p is still alive\n", ctx);
+		}
 	}
 }
 
@@ -474,12 +484,17 @@ void amdgpu_ctx_mgr_entity_cleanup(struct amdgpu_ctx_mgr *mgr)
 		if (!ctx->adev)
 			return;
 
-		for (i = 0; i < ctx->adev->num_rings; i++)
+		for (i = 0; i < ctx->adev->num_rings; i++) {
+
+			if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
+				continue;
+
 			if (kref_read(&ctx->refcount) == 1)
 				drm_sched_entity_cleanup(&ctx->adev->rings[i]->sched,
 					&ctx->rings[i].entity);
 			else
 				DRM_ERROR("ctx %p is still alive\n", ctx);
+		}
 	}
 }
 
-- 
2.7.4

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

[PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey Grodzovsky]

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
 include/drm/gpu_scheduler.h               | 1 -
 2 files changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 1f1dd70..2569a63 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
 	entity->last_scheduled = NULL;
 
 	spin_lock_init(&entity->rq_lock);
-	spin_lock_init(&entity->queue_lock);
 	spsc_queue_init(&entity->job_queue);
 
 	atomic_set(&entity->fence_seq, 0);
@@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
 
 	trace_drm_sched_job(sched_job, entity);
 
-	spin_lock(&entity->queue_lock);
 	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
 
-	spin_unlock(&entity->queue_lock);
-
 	/* first job wakes up scheduler */
 	if (first) {
 		/* Add the entity to the run queue */
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 350a62c..683eb65 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -56,7 +56,6 @@ struct drm_sched_entity {
 	spinlock_t			rq_lock;
 	struct drm_gpu_scheduler	*sched;
 
-	spinlock_t			queue_lock;
 	struct spsc_queue		job_queue;
 
 	atomic_t			fence_seq;
-- 
2.7.4

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Alex Deucher]

On Tue, May 15, 2018 at 3:31 PM, Andrey Grodzovsky
<andrey.grodzovsky@amd.com> wrote:
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Please provide a better patch description.

Alex

> ---
>  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>  include/drm/gpu_scheduler.h               | 1 -
>  2 files changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 1f1dd70..2569a63 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
>         entity->last_scheduled = NULL;
>
>         spin_lock_init(&entity->rq_lock);
> -       spin_lock_init(&entity->queue_lock);
>         spsc_queue_init(&entity->job_queue);
>
>         atomic_set(&entity->fence_seq, 0);
> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
>
>         trace_drm_sched_job(sched_job, entity);
>
> -       spin_lock(&entity->queue_lock);
>         first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>
> -       spin_unlock(&entity->queue_lock);
> -
>         /* first job wakes up scheduler */
>         if (first) {
>                 /* Add the entity to the run queue */
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 350a62c..683eb65 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>         spinlock_t                      rq_lock;
>         struct drm_gpu_scheduler        *sched;
>
> -       spinlock_t                      queue_lock;
>         struct spsc_queue               job_queue;
>
>         atomic_t                        fence_seq;
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey Grodzovsky]

Yea, I might need to give another thought to whether this  spinlock can 
actually be removed.

Andrey

On 05/15/2018 03:38 PM, Alex Deucher wrote:
> On Tue, May 15, 2018 at 3:31 PM, Andrey Grodzovsky
> <andrey.grodzovsky@amd.com> wrote:
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Please provide a better patch description.
>
> Alex
>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>   include/drm/gpu_scheduler.h               | 1 -
>>   2 files changed, 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 1f1dd70..2569a63 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
>>          entity->last_scheduled = NULL;
>>
>>          spin_lock_init(&entity->rq_lock);
>> -       spin_lock_init(&entity->queue_lock);
>>          spsc_queue_init(&entity->job_queue);
>>
>>          atomic_set(&entity->fence_seq, 0);
>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
>>
>>          trace_drm_sched_job(sched_job, entity);
>>
>> -       spin_lock(&entity->queue_lock);
>>          first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>>
>> -       spin_unlock(&entity->queue_lock);
>> -
>>          /* first job wakes up scheduler */
>>          if (first) {
>>                  /* Add the entity to the run queue */
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 350a62c..683eb65 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>          spinlock_t                      rq_lock;
>>          struct drm_gpu_scheduler        *sched;
>>
>> -       spinlock_t                      queue_lock;
>>          struct spsc_queue               job_queue;
>>
>>          atomic_t                        fence_seq;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[zhoucm1]

On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>   include/drm/gpu_scheduler.h               | 1 -
>   2 files changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 1f1dd70..2569a63 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
>   	entity->last_scheduled = NULL;
>   
>   	spin_lock_init(&entity->rq_lock);
> -	spin_lock_init(&entity->queue_lock);
>   	spsc_queue_init(&entity->job_queue);
>   
>   	atomic_set(&entity->fence_seq, 0);
> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
>   
>   	trace_drm_sched_job(sched_job, entity);
>   
> -	spin_lock(&entity->queue_lock);
>   	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>   
> -	spin_unlock(&entity->queue_lock);
Is your spsc safely to be added simultaneously?

Regards,
David Zhou
> -
>   	/* first job wakes up scheduler */
>   	if (first) {
>   		/* Add the entity to the run queue */
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 350a62c..683eb65 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>   	spinlock_t			rq_lock;
>   	struct drm_gpu_scheduler	*sched;
>   
> -	spinlock_t			queue_lock;
>   	struct spsc_queue		job_queue;
>   
>   	atomic_t			fence_seq;

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Grodzovsky, Andrey]

Yeah, that what I am not sure about... It's lockless in a sense of single producer single consumer but not for multiple concurrent producers... So now I think this spinlock should stay there... It just looked useless to me at first sight...

Andrey

________________________________________
From: Zhou, David(ChunMing)
Sent: 15 May 2018 23:04:44
To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian
Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.



On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>   include/drm/gpu_scheduler.h               | 1 -
>   2 files changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 1f1dd70..2569a63 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
>       entity->last_scheduled = NULL;
>
>       spin_lock_init(&entity->rq_lock);
> -     spin_lock_init(&entity->queue_lock);
>       spsc_queue_init(&entity->job_queue);
>
>       atomic_set(&entity->fence_seq, 0);
> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
>
>       trace_drm_sched_job(sched_job, entity);
>
> -     spin_lock(&entity->queue_lock);
>       first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>
> -     spin_unlock(&entity->queue_lock);
Is your spsc safely to be added simultaneously?

Regards,
David Zhou
> -
>       /* first job wakes up scheduler */
>       if (first) {
>               /* Add the entity to the run queue */
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 350a62c..683eb65 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>       spinlock_t                      rq_lock;
>       struct drm_gpu_scheduler        *sched;
>
> -     spinlock_t                      queue_lock;
>       struct spsc_queue               job_queue;
>
>       atomic_t                        fence_seq;

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Christian König]

No, that spinlock is indeed incorrect.

See even when we protect the spsc queue with a spinlock that doesn't  
make it correct. It can happen that the jobs pushed to the queue are  
reversed in their sequence order and that can cause severe problems in  
the memory management.

Christian.

Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
> Yeah, that what I am not sure about... It's lockless in a sense of single producer single consumer but not for multiple concurrent producers... So now I think this spinlock should stay there... It just looked useless to me at first sight...
>
> Andrey
>
> ________________________________________
> From: Zhou, David(ChunMing)
> Sent: 15 May 2018 23:04:44
> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Koenig, Christian
> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>
>
>
> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>    include/drm/gpu_scheduler.h               | 1 -
>>    2 files changed, 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 1f1dd70..2569a63 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
>>        entity->last_scheduled = NULL;
>>
>>        spin_lock_init(&entity->rq_lock);
>> -     spin_lock_init(&entity->queue_lock);
>>        spsc_queue_init(&entity->job_queue);
>>
>>        atomic_set(&entity->fence_seq, 0);
>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job,
>>
>>        trace_drm_sched_job(sched_job, entity);
>>
>> -     spin_lock(&entity->queue_lock);
>>        first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>>
>> -     spin_unlock(&entity->queue_lock);
> Is your spsc safely to be added simultaneously?
>
> Regards,
> David Zhou
>> -
>>        /* first job wakes up scheduler */
>>        if (first) {
>>                /* Add the entity to the run queue */
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 350a62c..683eb65 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>        spinlock_t                      rq_lock;
>>        struct drm_gpu_scheduler        *sched;
>>
>> -     spinlock_t                      queue_lock;
>>        struct spsc_queue               job_queue;
>>
>>        atomic_t                        fence_seq;

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey Grodzovsky]

Can you please elaborate more, maybe give an example - I don't 
understand yet the problematic scenario.

Andrey


On 05/16/2018 02:50 AM, Christian König wrote:
> No, that spinlock is indeed incorrect. I
>
> See even when we protect the spsc queue with a spinlock that doesn't 
> make it correct. It can happen that the jobs pushed to the queue are 
> reversed in their sequence order and that can cause severe problems in 
> the memory management.
>
> Christian.
>
> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>> Yeah, that what I am not sure about... It's lockless in a sense of 
>> single producer single consumer but not for multiple concurrent 
>> producers... So now I think this spinlock should stay there... It 
>> just looked useless to me at first sight...
>>
>> Andrey
>>
>> ________________________________________
>> From: Zhou, David(ChunMing)
>> Sent: 15 May 2018 23:04:44
>> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; 
>> dri-devel@lists.freedesktop.org
>> Cc: Koenig, Christian
>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>>
>>
>>
>> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>    include/drm/gpu_scheduler.h               | 1 -
>>>    2 files changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> index 1f1dd70..2569a63 100644
>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
>>> drm_gpu_scheduler *sched,
>>>        entity->last_scheduled = NULL;
>>>
>>>        spin_lock_init(&entity->rq_lock);
>>> -     spin_lock_init(&entity->queue_lock);
>>>        spsc_queue_init(&entity->job_queue);
>>>
>>>        atomic_set(&entity->fence_seq, 0);
>>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct 
>>> drm_sched_job *sched_job,
>>>
>>>        trace_drm_sched_job(sched_job, entity);
>>>
>>> -     spin_lock(&entity->queue_lock);
>>>        first = spsc_queue_push(&entity->job_queue, 
>>> &sched_job->queue_node);
>>>
>>> -     spin_unlock(&entity->queue_lock);
>> Is your spsc safely to be added simultaneously?
>>
>> Regards,
>> David Zhou
>>> -
>>>        /* first job wakes up scheduler */
>>>        if (first) {
>>>                /* Add the entity to the run queue */
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 350a62c..683eb65 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>        spinlock_t                      rq_lock;
>>>        struct drm_gpu_scheduler        *sched;
>>>
>>> -     spinlock_t                      queue_lock;
>>>        struct spsc_queue               job_queue;
>>>
>>>        atomic_t                        fence_seq;
>

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Christian König]

drm_sched_fence_create() assigns a sequence number to the fence it creates.

Now drm_sched_fence_create() is called by drm_sched_job_init() to 
initialize the jobs we want to push on the scheduler queue.

When you now call drm_sched_entity_push_job() without a protecting lock 
it can happen that you push two (or even more) job with reversed 
sequence numbers.

Since the sequence numbers are used to determine job completion order 
reversing them can seriously mess things up.

So the spin lock should be superfluous, if it isn't we have a much 
larger bug we need to fix.

Christian.

Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
> Can you please elaborate more, maybe give an example - I don't 
> understand yet the problematic scenario.
>
> Andrey
>
>
> On 05/16/2018 02:50 AM, Christian König wrote:
>> No, that spinlock is indeed incorrect. I
>>
>> See even when we protect the spsc queue with a spinlock that doesn't 
>> make it correct. It can happen that the jobs pushed to the queue are 
>> reversed in their sequence order and that can cause severe problems 
>> in the memory management.
>>
>> Christian.
>>
>> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>>> Yeah, that what I am not sure about... It's lockless in a sense of 
>>> single producer single consumer but not for multiple concurrent 
>>> producers... So now I think this spinlock should stay there... It 
>>> just looked useless to me at first sight...
>>>
>>> Andrey
>>>
>>> ________________________________________
>>> From: Zhou, David(ChunMing)
>>> Sent: 15 May 2018 23:04:44
>>> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; 
>>> dri-devel@lists.freedesktop.org
>>> Cc: Koenig, Christian
>>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>>>
>>>
>>>
>>> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>>    include/drm/gpu_scheduler.h               | 1 -
>>>>    2 files changed, 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> index 1f1dd70..2569a63 100644
>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
>>>> drm_gpu_scheduler *sched,
>>>>        entity->last_scheduled = NULL;
>>>>
>>>>        spin_lock_init(&entity->rq_lock);
>>>> -     spin_lock_init(&entity->queue_lock);
>>>>        spsc_queue_init(&entity->job_queue);
>>>>
>>>>        atomic_set(&entity->fence_seq, 0);
>>>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct 
>>>> drm_sched_job *sched_job,
>>>>
>>>>        trace_drm_sched_job(sched_job, entity);
>>>>
>>>> -     spin_lock(&entity->queue_lock);
>>>>        first = spsc_queue_push(&entity->job_queue, 
>>>> &sched_job->queue_node);
>>>>
>>>> -     spin_unlock(&entity->queue_lock);
>>> Is your spsc safely to be added simultaneously?
>>>
>>> Regards,
>>> David Zhou
>>>> -
>>>>        /* first job wakes up scheduler */
>>>>        if (first) {
>>>>                /* Add the entity to the run queue */
>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>> index 350a62c..683eb65 100644
>>>> --- a/include/drm/gpu_scheduler.h
>>>> +++ b/include/drm/gpu_scheduler.h
>>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>>        spinlock_t                      rq_lock;
>>>>        struct drm_gpu_scheduler        *sched;
>>>>
>>>> -     spinlock_t                      queue_lock;
>>>>        struct spsc_queue               job_queue;
>>>>
>>>>        atomic_t                        fence_seq;
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey Grodzovsky]

So are you saying that you expect this to  be already in code for any 
usage of drm_sched_fence_create and drm_sched_entity_push_job ?

lock()

drm_sched_fence_create()

... (some code)

drm_sched_entity_push_job()

unlock()


Andrey

On 05/16/2018 07:23 AM, Christian König wrote:
> drm_sched_fence_create() assigns a sequence number to the fence it 
> creates.
>
> Now drm_sched_fence_create() is called by drm_sched_job_init() to 
> initialize the jobs we want to push on the scheduler queue.
>
> When you now call drm_sched_entity_push_job() without a protecting 
> lock it can happen that you push two (or even more) job with reversed 
> sequence numbers.
>
> Since the sequence numbers are used to determine job completion order 
> reversing them can seriously mess things up.
>
> So the spin lock should be superfluous, if it isn't we have a much 
> larger bug we need to fix.
>
> Christian.
>
> Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
>> Can you please elaborate more, maybe give an example - I don't 
>> understand yet the problematic scenario.
>>
>> Andrey
>>
>>
>> On 05/16/2018 02:50 AM, Christian König wrote:
>>> No, that spinlock is indeed incorrect. I
>>>
>>> See even when we protect the spsc queue with a spinlock that doesn't 
>>> make it correct. It can happen that the jobs pushed to the queue are 
>>> reversed in their sequence order and that can cause severe problems 
>>> in the memory management.
>>>
>>> Christian.
>>>
>>> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>>>> Yeah, that what I am not sure about... It's lockless in a sense of 
>>>> single producer single consumer but not for multiple concurrent 
>>>> producers... So now I think this spinlock should stay there... It 
>>>> just looked useless to me at first sight...
>>>>
>>>> Andrey
>>>>
>>>> ________________________________________
>>>> From: Zhou, David(ChunMing)
>>>> Sent: 15 May 2018 23:04:44
>>>> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; 
>>>> dri-devel@lists.freedesktop.org
>>>> Cc: Koenig, Christian
>>>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>>>>
>>>>
>>>>
>>>> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>>>    include/drm/gpu_scheduler.h               | 1 -
>>>>>    2 files changed, 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> index 1f1dd70..2569a63 100644
>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
>>>>> drm_gpu_scheduler *sched,
>>>>>        entity->last_scheduled = NULL;
>>>>>
>>>>>        spin_lock_init(&entity->rq_lock);
>>>>> -     spin_lock_init(&entity->queue_lock);
>>>>>        spsc_queue_init(&entity->job_queue);
>>>>>
>>>>>        atomic_set(&entity->fence_seq, 0);
>>>>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct 
>>>>> drm_sched_job *sched_job,
>>>>>
>>>>>        trace_drm_sched_job(sched_job, entity);
>>>>>
>>>>> -     spin_lock(&entity->queue_lock);
>>>>>        first = spsc_queue_push(&entity->job_queue, 
>>>>> &sched_job->queue_node);
>>>>>
>>>>> -     spin_unlock(&entity->queue_lock);
>>>> Is your spsc safely to be added simultaneously?
>>>>
>>>> Regards,
>>>> David Zhou
>>>>> -
>>>>>        /* first job wakes up scheduler */
>>>>>        if (first) {
>>>>>                /* Add the entity to the run queue */
>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>> b/include/drm/gpu_scheduler.h
>>>>> index 350a62c..683eb65 100644
>>>>> --- a/include/drm/gpu_scheduler.h
>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>>>        spinlock_t                      rq_lock;
>>>>>        struct drm_gpu_scheduler        *sched;
>>>>>
>>>>> -     spinlock_t                      queue_lock;
>>>>>        struct spsc_queue               job_queue;
>>>>>
>>>>>        atomic_t                        fence_seq;
>>>
>>
>> _______________________________________________
>> 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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Christian König]

Yes, exactly.

For normal user space command submission we should have tons of locks 
guaranteeing that (e.g. just the VM lock should do).

For kernel moves we have the mutex for the GTT windows which protects it.

The could be problems with the UVD/VCE queues to cleanup the handles 
when an application crashes.

Christian.

Am 16.05.2018 um 13:47 schrieb Andrey Grodzovsky:
> So are you saying that you expect this to  be already in code for any 
> usage of drm_sched_fence_create and drm_sched_entity_push_job ?
>
> lock()
>
> drm_sched_fence_create()
>
> ... (some code)
>
> drm_sched_entity_push_job()
>
> unlock()
>
>
> Andrey
>
> On 05/16/2018 07:23 AM, Christian König wrote:
>> drm_sched_fence_create() assigns a sequence number to the fence it 
>> creates.
>>
>> Now drm_sched_fence_create() is called by drm_sched_job_init() to 
>> initialize the jobs we want to push on the scheduler queue.
>>
>> When you now call drm_sched_entity_push_job() without a protecting 
>> lock it can happen that you push two (or even more) job with reversed 
>> sequence numbers.
>>
>> Since the sequence numbers are used to determine job completion order 
>> reversing them can seriously mess things up.
>>
>> So the spin lock should be superfluous, if it isn't we have a much 
>> larger bug we need to fix.
>>
>> Christian.
>>
>> Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
>>> Can you please elaborate more, maybe give an example - I don't 
>>> understand yet the problematic scenario.
>>>
>>> Andrey
>>>
>>>
>>> On 05/16/2018 02:50 AM, Christian König wrote:
>>>> No, that spinlock is indeed incorrect. I
>>>>
>>>> See even when we protect the spsc queue with a spinlock that 
>>>> doesn't make it correct. It can happen that the jobs pushed to the 
>>>> queue are reversed in their sequence order and that can cause 
>>>> severe problems in the memory management.
>>>>
>>>> Christian.
>>>>
>>>> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>>>>> Yeah, that what I am not sure about... It's lockless in a sense of 
>>>>> single producer single consumer but not for multiple concurrent 
>>>>> producers... So now I think this spinlock should stay there... It 
>>>>> just looked useless to me at first sight...
>>>>>
>>>>> Andrey
>>>>>
>>>>> ________________________________________
>>>>> From: Zhou, David(ChunMing)
>>>>> Sent: 15 May 2018 23:04:44
>>>>> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; 
>>>>> dri-devel@lists.freedesktop.org
>>>>> Cc: Koenig, Christian
>>>>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>>>>>
>>>>>
>>>>>
>>>>> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>>>>    include/drm/gpu_scheduler.h               | 1 -
>>>>>>    2 files changed, 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>>>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> index 1f1dd70..2569a63 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
>>>>>> drm_gpu_scheduler *sched,
>>>>>>        entity->last_scheduled = NULL;
>>>>>>
>>>>>>        spin_lock_init(&entity->rq_lock);
>>>>>> -     spin_lock_init(&entity->queue_lock);
>>>>>>        spsc_queue_init(&entity->job_queue);
>>>>>>
>>>>>>        atomic_set(&entity->fence_seq, 0);
>>>>>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct 
>>>>>> drm_sched_job *sched_job,
>>>>>>
>>>>>>        trace_drm_sched_job(sched_job, entity);
>>>>>>
>>>>>> -     spin_lock(&entity->queue_lock);
>>>>>>        first = spsc_queue_push(&entity->job_queue, 
>>>>>> &sched_job->queue_node);
>>>>>>
>>>>>> -     spin_unlock(&entity->queue_lock);
>>>>> Is your spsc safely to be added simultaneously?
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>> -
>>>>>>        /* first job wakes up scheduler */
>>>>>>        if (first) {
>>>>>>                /* Add the entity to the run queue */
>>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>>> b/include/drm/gpu_scheduler.h
>>>>>> index 350a62c..683eb65 100644
>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>>>>        spinlock_t                      rq_lock;
>>>>>>        struct drm_gpu_scheduler        *sched;
>>>>>>
>>>>>> -     spinlock_t                      queue_lock;
>>>>>>        struct spsc_queue               job_queue;
>>>>>>
>>>>>>        atomic_t                        fence_seq;
>>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Lucas Stach]

Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
> Yes, exactly.
> 
> For normal user space command submission we should have tons of
> locks 
> guaranteeing that (e.g. just the VM lock should do).
> 
> For kernel moves we have the mutex for the GTT windows which protects
> it.
> 
> The could be problems with the UVD/VCE queues to cleanup the handles 
> when an application crashes.

FWIW, etnaviv is currently completely unlocked in this path, but I
believe that this isn't an issue as the sched fence seq numbers are per
entity. So to actually end up with reversed seqnos one context has to
preempt itself to do another submit, while the current one hasn't
returned from kernel space, which I believe is a fairly theoretical
issue. Is my understanding correct?

Regards,
Lucas

> 
> Am 16.05.2018 um 13:47 schrieb Andrey Grodzovsky:
> > So are you saying that you expect this to  be already in code for
> > any 
> > usage of drm_sched_fence_create and drm_sched_entity_push_job ?
> > 
> > lock()
> > 
> > drm_sched_fence_create()
> > 
> > ... (some code)
> > 
> > drm_sched_entity_push_job()
> > 
> > unlock()
> > 
> > 
> > Andrey
> > 
> > On 05/16/2018 07:23 AM, Christian König wrote:
> > > drm_sched_fence_create() assigns a sequence number to the fence
> > > it 
> > > creates.
> > > 
> > > Now drm_sched_fence_create() is called by drm_sched_job_init()
> > > to 
> > > initialize the jobs we want to push on the scheduler queue.
> > > 
> > > When you now call drm_sched_entity_push_job() without a
> > > protecting 
> > > lock it can happen that you push two (or even more) job with
> > > reversed 
> > > sequence numbers.
> > > 
> > > Since the sequence numbers are used to determine job completion
> > > order 
> > > reversing them can seriously mess things up.
> > > 
> > > So the spin lock should be superfluous, if it isn't we have a
> > > much 
> > > larger bug we need to fix.
> > > 
> > > Christian.
> > > 
> > > Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
> > > > Can you please elaborate more, maybe give an example - I don't 
> > > > understand yet the problematic scenario.
> > > > 
> > > > Andrey
> > > > 
> > > > 
> > > > On 05/16/2018 02:50 AM, Christian König wrote:
> > > > > No, that spinlock is indeed incorrect. I
> > > > > 
> > > > > See even when we protect the spsc queue with a spinlock that 
> > > > > doesn't make it correct. It can happen that the jobs pushed
> > > > > to the 
> > > > > queue are reversed in their sequence order and that can
> > > > > cause 
> > > > > severe problems in the memory management.
> > > > > 
> > > > > Christian.
> > > > > 
> > > > > Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
> > > > > > Yeah, that what I am not sure about... It's lockless in a
> > > > > > sense of 
> > > > > > single producer single consumer but not for multiple
> > > > > > concurrent 
> > > > > > producers... So now I think this spinlock should stay
> > > > > > there... It 
> > > > > > just looked useless to me at first sight...
> > > > > > 
> > > > > > Andrey
> > > > > > 
> > > > > > ________________________________________
> > > > > > From: Zhou, David(ChunMing)
> > > > > > Sent: 15 May 2018 23:04:44
> > > > > > To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org; 
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > Cc: Koenig, Christian
> > > > > > Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete
> > > > > > spinlock.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
> > > > > > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.c
> > > > > > > om>
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
> > > > > > >    include/drm/gpu_scheduler.h               | 1 -
> > > > > > >    2 files changed, 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
> > > > > > > b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > > index 1f1dd70..2569a63 100644
> > > > > > > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > > @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
> > > > > > > drm_gpu_scheduler *sched,
> > > > > > >        entity->last_scheduled = NULL;
> > > > > > > 
> > > > > > >        spin_lock_init(&entity->rq_lock);
> > > > > > > -     spin_lock_init(&entity->queue_lock);
> > > > > > >        spsc_queue_init(&entity->job_queue);
> > > > > > > 
> > > > > > >        atomic_set(&entity->fence_seq, 0);
> > > > > > > @@ -424,11 +423,8 @@ void
> > > > > > > drm_sched_entity_push_job(struct 
> > > > > > > drm_sched_job *sched_job,
> > > > > > > 
> > > > > > >        trace_drm_sched_job(sched_job, entity);
> > > > > > > 
> > > > > > > -     spin_lock(&entity->queue_lock);
> > > > > > >        first = spsc_queue_push(&entity->job_queue, 
> > > > > > > &sched_job->queue_node);
> > > > > > > 
> > > > > > > -     spin_unlock(&entity->queue_lock);
> > > > > > 
> > > > > > Is your spsc safely to be added simultaneously?
> > > > > > 
> > > > > > Regards,
> > > > > > David Zhou
> > > > > > > -
> > > > > > >        /* first job wakes up scheduler */
> > > > > > >        if (first) {
> > > > > > >                /* Add the entity to the run queue */
> > > > > > > diff --git a/include/drm/gpu_scheduler.h 
> > > > > > > b/include/drm/gpu_scheduler.h
> > > > > > > index 350a62c..683eb65 100644
> > > > > > > --- a/include/drm/gpu_scheduler.h
> > > > > > > +++ b/include/drm/gpu_scheduler.h
> > > > > > > @@ -56,7 +56,6 @@ struct drm_sched_entity {
> > > > > > >        spinlock_t                      rq_lock;
> > > > > > >        struct drm_gpu_scheduler        *sched;
> > > > > > > 
> > > > > > > -     spinlock_t                      queue_lock;
> > > > > > >        struct spsc_queue               job_queue;
> > > > > > > 
> > > > > > >        atomic_t                        fence_seq;
> > > > 
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > amd-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Christian König]

Am 16.05.2018 um 14:28 schrieb Lucas Stach:
> Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
>> Yes, exactly.
>>
>> For normal user space command submission we should have tons of
>> locks
>> guaranteeing that (e.g. just the VM lock should do).
>>
>> For kernel moves we have the mutex for the GTT windows which protects
>> it.
>>
>> The could be problems with the UVD/VCE queues to cleanup the handles
>> when an application crashes.
> FWIW, etnaviv is currently completely unlocked in this path, but I
> believe that this isn't an issue as the sched fence seq numbers are per
> entity. So to actually end up with reversed seqnos one context has to
> preempt itself to do another submit, while the current one hasn't
> returned from kernel space, which I believe is a fairly theoretical
> issue. Is my understanding correct?

Yes. The problem is with the right timing this can be used to access 
freed up memory.

If you then manage to place a page table in that freed up memory taking 
over the system is just a typing exercise.

Regards,
Christian.

>
> Regards,
> Lucas
>
>> Am 16.05.2018 um 13:47 schrieb Andrey Grodzovsky:
>>> So are you saying that you expect this to  be already in code for
>>> any
>>> usage of drm_sched_fence_create and drm_sched_entity_push_job ?
>>>
>>> lock()
>>>
>>> drm_sched_fence_create()
>>>
>>> ... (some code)
>>>
>>> drm_sched_entity_push_job()
>>>
>>> unlock()
>>>
>>>
>>> Andrey
>>>
>>> On 05/16/2018 07:23 AM, Christian König wrote:
>>>> drm_sched_fence_create() assigns a sequence number to the fence
>>>> it
>>>> creates.
>>>>
>>>> Now drm_sched_fence_create() is called by drm_sched_job_init()
>>>> to
>>>> initialize the jobs we want to push on the scheduler queue.
>>>>
>>>> When you now call drm_sched_entity_push_job() without a
>>>> protecting
>>>> lock it can happen that you push two (or even more) job with
>>>> reversed
>>>> sequence numbers.
>>>>
>>>> Since the sequence numbers are used to determine job completion
>>>> order
>>>> reversing them can seriously mess things up.
>>>>
>>>> So the spin lock should be superfluous, if it isn't we have a
>>>> much
>>>> larger bug we need to fix.
>>>>
>>>> Christian.
>>>>
>>>> Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
>>>>> Can you please elaborate more, maybe give an example - I don't
>>>>> understand yet the problematic scenario.
>>>>>
>>>>> Andrey
>>>>>
>>>>>
>>>>> On 05/16/2018 02:50 AM, Christian König wrote:
>>>>>> No, that spinlock is indeed incorrect. I
>>>>>>
>>>>>> See even when we protect the spsc queue with a spinlock that
>>>>>> doesn't make it correct. It can happen that the jobs pushed
>>>>>> to the
>>>>>> queue are reversed in their sequence order and that can
>>>>>> cause
>>>>>> severe problems in the memory management.
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>>>>>>> Yeah, that what I am not sure about... It's lockless in a
>>>>>>> sense of
>>>>>>> single producer single consumer but not for multiple
>>>>>>> concurrent
>>>>>>> producers... So now I think this spinlock should stay
>>>>>>> there... It
>>>>>>> just looked useless to me at first sight...
>>>>>>>
>>>>>>> Andrey
>>>>>>>
>>>>>>> ________________________________________
>>>>>>> From: Zhou, David(ChunMing)
>>>>>>> Sent: 15 May 2018 23:04:44
>>>>>>> To: Grodzovsky, Andrey; amd-gfx@lists.freedesktop.org;
>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>> Cc: Koenig, Christian
>>>>>>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete
>>>>>>> spinlock.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2018年05月16日 03:31, Andrey Grodzovsky wrote:
>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.c
>>>>>>>> om>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>>>>>>     include/drm/gpu_scheduler.h               | 1 -
>>>>>>>>     2 files changed, 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> index 1f1dd70..2569a63 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct
>>>>>>>> drm_gpu_scheduler *sched,
>>>>>>>>         entity->last_scheduled = NULL;
>>>>>>>>
>>>>>>>>         spin_lock_init(&entity->rq_lock);
>>>>>>>> -     spin_lock_init(&entity->queue_lock);
>>>>>>>>         spsc_queue_init(&entity->job_queue);
>>>>>>>>
>>>>>>>>         atomic_set(&entity->fence_seq, 0);
>>>>>>>> @@ -424,11 +423,8 @@ void
>>>>>>>> drm_sched_entity_push_job(struct
>>>>>>>> drm_sched_job *sched_job,
>>>>>>>>
>>>>>>>>         trace_drm_sched_job(sched_job, entity);
>>>>>>>>
>>>>>>>> -     spin_lock(&entity->queue_lock);
>>>>>>>>         first = spsc_queue_push(&entity->job_queue,
>>>>>>>> &sched_job->queue_node);
>>>>>>>>
>>>>>>>> -     spin_unlock(&entity->queue_lock);
>>>>>>> Is your spsc safely to be added simultaneously?
>>>>>>>
>>>>>>> Regards,
>>>>>>> David Zhou
>>>>>>>> -
>>>>>>>>         /* first job wakes up scheduler */
>>>>>>>>         if (first) {
>>>>>>>>                 /* Add the entity to the run queue */
>>>>>>>> diff --git a/include/drm/gpu_scheduler.h
>>>>>>>> b/include/drm/gpu_scheduler.h
>>>>>>>> index 350a62c..683eb65 100644
>>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>>>>>>         spinlock_t                      rq_lock;
>>>>>>>>         struct drm_gpu_scheduler        *sched;
>>>>>>>>
>>>>>>>> -     spinlock_t                      queue_lock;
>>>>>>>>         struct spsc_queue               job_queue;
>>>>>>>>
>>>>>>>>         atomic_t                        fence_seq;
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Lucas Stach]

Am Mittwoch, den 16.05.2018, 14:32 +0200 schrieb Christian König:
> Am 16.05.2018 um 14:28 schrieb Lucas Stach:
> > Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
> > > Yes, exactly.
> > > 
> > > For normal user space command submission we should have tons of
> > > locks
> > > guaranteeing that (e.g. just the VM lock should do).
> > > 
> > > For kernel moves we have the mutex for the GTT windows which
> > > protects
> > > it.
> > > 
> > > The could be problems with the UVD/VCE queues to cleanup the
> > > handles
> > > when an application crashes.
> > 
> > FWIW, etnaviv is currently completely unlocked in this path, but I
> > believe that this isn't an issue as the sched fence seq numbers are
> > per
> > entity. So to actually end up with reversed seqnos one context has
> > to
> > preempt itself to do another submit, while the current one hasn't
> > returned from kernel space, which I believe is a fairly theoretical
> > issue. Is my understanding correct?
> 
> Yes. The problem is with the right timing this can be used to access 
> freed up memory.
> 
> If you then manage to place a page table in that freed up memory
> taking 
> over the system is just a typing exercise.

Thanks. I believe we don't have this problem in etnaviv, as memory
referencing is tied to the job and will only be unreferenced on
free_job, but I'll re-check this when I've got some time.

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Christian König]

Am 16.05.2018 um 15:00 schrieb Lucas Stach:
> Am Mittwoch, den 16.05.2018, 14:32 +0200 schrieb Christian König:
>> Am 16.05.2018 um 14:28 schrieb Lucas Stach:
>>> Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
>>>> Yes, exactly.
>>>>
>>>> For normal user space command submission we should have tons of
>>>> locks
>>>> guaranteeing that (e.g. just the VM lock should do).
>>>>
>>>> For kernel moves we have the mutex for the GTT windows which
>>>> protects
>>>> it.
>>>>
>>>> The could be problems with the UVD/VCE queues to cleanup the
>>>> handles
>>>> when an application crashes.
>>> FWIW, etnaviv is currently completely unlocked in this path, but I
>>> believe that this isn't an issue as the sched fence seq numbers are
>>> per
>>> entity. So to actually end up with reversed seqnos one context has
>>> to
>>> preempt itself to do another submit, while the current one hasn't
>>> returned from kernel space, which I believe is a fairly theoretical
>>> issue. Is my understanding correct?
>> Yes. The problem is with the right timing this can be used to access
>> freed up memory.
>>
>> If you then manage to place a page table in that freed up memory
>> taking
>> over the system is just a typing exercise.
> Thanks. I believe we don't have this problem in etnaviv, as memory
> referencing is tied to the job and will only be unreferenced on
> free_job, but I'll re-check this when I've got some time.

Well that depends on how you use the sequence numbers.

If you use them for job completion check somewhere then you certainly 
have a problem if job A gets the 1 and B the 2, but B completes before A.

At bare minimum that's still a bug we need to fix because it confuses 
functions like dma_fence_is_later() and dma_fence_later().

Christian.

>
> Regards,
> Lucas

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Lucas Stach]

Am Mittwoch, den 16.05.2018, 15:10 +0200 schrieb Christian König:
> Am 16.05.2018 um 15:00 schrieb Lucas Stach:
> > Am Mittwoch, den 16.05.2018, 14:32 +0200 schrieb Christian König:
> > > Am 16.05.2018 um 14:28 schrieb Lucas Stach:
> > > > Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
> > > > > Yes, exactly.
> > > > > 
> > > > > For normal user space command submission we should have tons of
> > > > > locks
> > > > > guaranteeing that (e.g. just the VM lock should do).
> > > > > 
> > > > > For kernel moves we have the mutex for the GTT windows which
> > > > > protects
> > > > > it.
> > > > > 
> > > > > The could be problems with the UVD/VCE queues to cleanup the
> > > > > handles
> > > > > when an application crashes.
> > > > 
> > > > FWIW, etnaviv is currently completely unlocked in this path, but I
> > > > believe that this isn't an issue as the sched fence seq numbers are
> > > > per
> > > > entity. So to actually end up with reversed seqnos one context has
> > > > to
> > > > preempt itself to do another submit, while the current one hasn't
> > > > returned from kernel space, which I believe is a fairly theoretical
> > > > issue. Is my understanding correct?
> > > 
> > > Yes. The problem is with the right timing this can be used to access
> > > freed up memory.
> > > 
> > > If you then manage to place a page table in that freed up memory
> > > taking
> > > over the system is just a typing exercise.
> > 
> > Thanks. I believe we don't have this problem in etnaviv, as memory
> > referencing is tied to the job and will only be unreferenced on
> > free_job, but I'll re-check this when I've got some time.
> 
> Well that depends on how you use the sequence numbers.
> 
> If you use them for job completion check somewhere then you certainly 
> have a problem if job A gets the 1 and B the 2, but B completes before A.

We don't do this anymore. All the etnaviv internals are driven by the
fence signal callbacks.

> At bare minimum that's still a bug we need to fix because it confuses 
> functions like dma_fence_is_later() and dma_fence_later().

Agreed. Also amending the documentation to state that
drm_sched_job_init() and drm_sched_entity_push_job() need to be called
under a common lock seems like a good idea.

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

Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey Grodzovsky]

On 05/16/2018 09:16 AM, Lucas Stach wrote:
> Am Mittwoch, den 16.05.2018, 15:10 +0200 schrieb Christian König:
>> Am 16.05.2018 um 15:00 schrieb Lucas Stach:
>>> Am Mittwoch, den 16.05.2018, 14:32 +0200 schrieb Christian König:
>>>> Am 16.05.2018 um 14:28 schrieb Lucas Stach:
>>>>> Am Mittwoch, den 16.05.2018, 14:08 +0200 schrieb Christian König:
>>>>>> Yes, exactly.
>>>>>>
>>>>>> For normal user space command submission we should have tons of
>>>>>> locks
>>>>>> guaranteeing that (e.g. just the VM lock should do).
>>>>>>
>>>>>> For kernel moves we have the mutex for the GTT windows which
>>>>>> protects
>>>>>> it.
>>>>>>
>>>>>> The could be problems with the UVD/VCE queues to cleanup the
>>>>>> handles
>>>>>> when an application crashes.
>>>>> FWIW, etnaviv is currently completely unlocked in this path, but I
>>>>> believe that this isn't an issue as the sched fence seq numbers are
>>>>> per
>>>>> entity. So to actually end up with reversed seqnos one context has
>>>>> to
>>>>> preempt itself to do another submit, while the current one hasn't
>>>>> returned from kernel space, which I believe is a fairly theoretical
>>>>> issue. Is my understanding correct?
>>>> Yes. The problem is with the right timing this can be used to access
>>>> freed up memory.
>>>>
>>>> If you then manage to place a page table in that freed up memory
>>>> taking
>>>> over the system is just a typing exercise.
>>> Thanks. I believe we don't have this problem in etnaviv, as memory
>>> referencing is tied to the job and will only be unreferenced on
>>> free_job, but I'll re-check this when I've got some time.
>> Well that depends on how you use the sequence numbers.
>>
>> If you use them for job completion check somewhere then you certainly
>> have a problem if job A gets the 1 and B the 2, but B completes before A.
> We don't do this anymore. All the etnaviv internals are driven by the
> fence signal callbacks.
>
>> At bare minimum that's still a bug we need to fix because it confuses
>> functions like dma_fence_is_later() and dma_fence_later().
> Agreed. Also amending the documentation to state that
> drm_sched_job_init() and drm_sched_entity_push_job() need to be called
> under a common lock seems like a good idea.

I will respin the patch with added documentation.

Andrey

> Regards,
> Lucas

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

Re: [PATCH 1/2] drm/amdgpu: Skip drm_sched_entity realted ops for KIQ ring.

[Alex Deucher]

On Tue, May 15, 2018 at 3:31 PM, Andrey Grodzovsky
<andrey.grodzovsky@amd.com> wrote:
> Follwoing change 75fbed2 we need to skip KIQ ring when iterating
> amdgpu_ctx's scheduler entites.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Typo in the title: realted -> related
Typo in the description: Follwoing -> Following
Also expand on the reasoning a bit in the patch description.  E.g., we
never initialize or use the GPU scheduler for KIQ.
With those things fixes:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 6741a62..744519b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -173,9 +173,14 @@ static void amdgpu_ctx_do_release(struct kref *ref)
>
>         ctx = container_of(ref, struct amdgpu_ctx, refcount);
>
> -       for (i = 0; i < ctx->adev->num_rings; i++)
> +       for (i = 0; i < ctx->adev->num_rings; i++) {
> +
> +               if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
> +                       continue;
> +
>                 drm_sched_entity_fini(&ctx->adev->rings[i]->sched,
>                         &ctx->rings[i].entity);
> +       }
>
>         amdgpu_ctx_fini(ref);
>  }
> @@ -452,12 +457,17 @@ void amdgpu_ctx_mgr_entity_fini(struct amdgpu_ctx_mgr *mgr)
>                 if (!ctx->adev)
>                         return;
>
> -               for (i = 0; i < ctx->adev->num_rings; i++)
> +               for (i = 0; i < ctx->adev->num_rings; i++) {
> +
> +                       if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
> +                               continue;
> +
>                         if (kref_read(&ctx->refcount) == 1)
>                                 drm_sched_entity_do_release(&ctx->adev->rings[i]->sched,
>                                                   &ctx->rings[i].entity);
>                         else
>                                 DRM_ERROR("ctx %p is still alive\n", ctx);
> +               }
>         }
>  }
>
> @@ -474,12 +484,17 @@ void amdgpu_ctx_mgr_entity_cleanup(struct amdgpu_ctx_mgr *mgr)
>                 if (!ctx->adev)
>                         return;
>
> -               for (i = 0; i < ctx->adev->num_rings; i++)
> +               for (i = 0; i < ctx->adev->num_rings; i++) {
> +
> +                       if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
> +                               continue;
> +
>                         if (kref_read(&ctx->refcount) == 1)
>                                 drm_sched_entity_cleanup(&ctx->adev->rings[i]->sched,
>                                         &ctx->rings[i].entity);
>                         else
>                                 DRM_ERROR("ctx %p is still alive\n", ctx);
> +               }
>         }
>  }
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: Skip drm_sched_entity realted ops for KIQ ring.

[Christian König]

Am 15.05.2018 um 21:42 schrieb Alex Deucher:
> On Tue, May 15, 2018 at 3:31 PM, Andrey Grodzovsky
> <andrey.grodzovsky@amd.com> wrote:
>> Follwoing change 75fbed2 we need to skip KIQ ring when iterating
>> amdgpu_ctx's scheduler entites.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Typo in the title: realted -> related
> Typo in the description: Follwoing -> Following
> Also expand on the reasoning a bit in the patch description.  E.g., we
> never initialize or use the GPU scheduler for KIQ.
> With those things fixes:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

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

Christian.

>
> Alex
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 21 ++++++++++++++++++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 6741a62..744519b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -173,9 +173,14 @@ static void amdgpu_ctx_do_release(struct kref *ref)
>>
>>          ctx = container_of(ref, struct amdgpu_ctx, refcount);
>>
>> -       for (i = 0; i < ctx->adev->num_rings; i++)
>> +       for (i = 0; i < ctx->adev->num_rings; i++) {
>> +
>> +               if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
>> +                       continue;
>> +
>>                  drm_sched_entity_fini(&ctx->adev->rings[i]->sched,
>>                          &ctx->rings[i].entity);
>> +       }
>>
>>          amdgpu_ctx_fini(ref);
>>   }
>> @@ -452,12 +457,17 @@ void amdgpu_ctx_mgr_entity_fini(struct amdgpu_ctx_mgr *mgr)
>>                  if (!ctx->adev)
>>                          return;
>>
>> -               for (i = 0; i < ctx->adev->num_rings; i++)
>> +               for (i = 0; i < ctx->adev->num_rings; i++) {
>> +
>> +                       if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
>> +                               continue;
>> +
>>                          if (kref_read(&ctx->refcount) == 1)
>>                                  drm_sched_entity_do_release(&ctx->adev->rings[i]->sched,
>>                                                    &ctx->rings[i].entity);
>>                          else
>>                                  DRM_ERROR("ctx %p is still alive\n", ctx);
>> +               }
>>          }
>>   }
>>
>> @@ -474,12 +484,17 @@ void amdgpu_ctx_mgr_entity_cleanup(struct amdgpu_ctx_mgr *mgr)
>>                  if (!ctx->adev)
>>                          return;
>>
>> -               for (i = 0; i < ctx->adev->num_rings; i++)
>> +               for (i = 0; i < ctx->adev->num_rings; i++) {
>> +
>> +                       if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring)
>> +                               continue;
>> +
>>                          if (kref_read(&ctx->refcount) == 1)
>>                                  drm_sched_entity_cleanup(&ctx->adev->rings[i]->sched,
>>                                          &ctx->rings[i].entity);
>>                          else
>>                                  DRM_ERROR("ctx %p is still alive\n", ctx);
>> +               }
>>          }
>>   }
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> 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