[v7,13/16] drm/scheduler: Fix hang when sched_entity released

Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because drm_sched_entity_is_idle
never becomes false.

Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.

v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.

v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
 drivers/gpu/drm/scheduler/sched_main.c   | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

In a separate discussion with Daniel we once more iterated over the 
dma_resv requirements and I came to the conclusion that this approach 
here won't work reliable.

The problem is as following:
1. device A schedules some rendering with into a buffer and exports it 
as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering, for 
the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer scheduled.

The problem is now that we can't do this since the operation of device A 
is still running and by signaling our fences we run into the problem of 
potential memory corruption.

Not sure how to handle that case. One possibility would be to wait for 
all dependencies of unscheduled jobs before signaling their fences as 
canceled.

Christian.

Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
> Problem: If scheduler is already stopped by the time sched_entity
> is released and entity's job_queue not empty I encountred
> a hang in drm_sched_entity_flush. This is because drm_sched_entity_is_idle
> never becomes false.
>
> Fix: In drm_sched_fini detach all sched_entities from the
> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
> Also wakeup all those processes stuck in sched_entity flushing
> as the scheduler main thread which wakes them up is stopped by now.
>
> v2:
> Reverse order of drm_sched_rq_remove_entity and marking
> s_entity as stopped to prevent reinserion back to rq due
> to race.
>
> v3:
> Drop drm_sched_rq_remove_entity, only modify entity->stopped
> and check for it in drm_sched_entity_is_idle
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>   drivers/gpu/drm/scheduler/sched_main.c   | 24 ++++++++++++++++++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 0249c7450188..2e93e881b65f 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>   	rmb(); /* for list_empty to work without lock */
>   
>   	if (list_empty(&entity->list) ||
> -	    spsc_queue_count(&entity->job_queue) == 0)
> +	    spsc_queue_count(&entity->job_queue) == 0 ||
> +	    entity->stopped)
>   		return true;
>   
>   	return false;
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 8d1211e87101..a2a953693b45 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>    */
>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>   {
> +	struct drm_sched_entity *s_entity;
> +	int i;
> +
>   	if (sched->thread)
>   		kthread_stop(sched->thread);
>   
> +	for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
> +		struct drm_sched_rq *rq = &sched->sched_rq[i];
> +
> +		if (!rq)
> +			continue;
> +
> +		spin_lock(&rq->lock);
> +		list_for_each_entry(s_entity, &rq->entities, list)
> +			/*
> +			 * Prevents reinsertion and marks job_queue as idle,
> +			 * it will removed from rq in drm_sched_entity_fini
> +			 * eventually
> +			 */
> +			s_entity->stopped = true;
> +		spin_unlock(&rq->lock);
> +
> +	}
> +
> +	/* Wakeup everyone stuck in drm_sched_entity_flush for this scheduler */
> +	wake_up_all(&sched->job_scheduled);
> +
>   	/* Confirm no work left behind accessing device structures */
>   	cancel_delayed_work_sync(&sched->work_tdr);
>

On 2021-05-18 10:07 a.m., Christian König wrote:
> In a separate discussion with Daniel we once more iterated over the 
> dma_resv requirements and I came to the conclusion that this approach 
> here won't work reliable.
> 
> The problem is as following:
> 1. device A schedules some rendering with into a buffer and exports it 
> as DMA-buf.
> 2. device B imports the DMA-buf and wants to consume the rendering, for 
> the the fence of device A is replaced with a new operation.
> 3. device B is hot plugged and the new operation canceled/newer scheduled.
> 
> The problem is now that we can't do this since the operation of device A 
> is still running and by signaling our fences we run into the problem of 
> potential memory corruption.


I am not sure this problem you describe above is related to this patch.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'

Also, in the patch series as it is now we only signal HW fences for the
extracted device B, we are not touching any other fences. In fact as you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.

Andrey

> 
> Not sure how to handle that case. One possibility would be to wait for 
> all dependencies of unscheduled jobs before signaling their fences as 
> canceled.
> 
> Christian.
> 
> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>> Problem: If scheduler is already stopped by the time sched_entity
>> is released and entity's job_queue not empty I encountred
>> a hang in drm_sched_entity_flush. This is because 
>> drm_sched_entity_is_idle
>> never becomes false.
>>
>> Fix: In drm_sched_fini detach all sched_entities from the
>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>> Also wakeup all those processes stuck in sched_entity flushing
>> as the scheduler main thread which wakes them up is stopped by now.
>>
>> v2:
>> Reverse order of drm_sched_rq_remove_entity and marking
>> s_entity as stopped to prevent reinserion back to rq due
>> to race.
>>
>> v3:
>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>> and check for it in drm_sched_entity_is_idle
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 ++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 0249c7450188..2e93e881b65f 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>> drm_sched_entity *entity)
>>       rmb(); /* for list_empty to work without lock */
>>       if (list_empty(&entity->list) ||
>> -        spsc_queue_count(&entity->job_queue) == 0)
>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>> +        entity->stopped)
>>           return true;
>>       return false;
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 8d1211e87101..a2a953693b45 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>    */
>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>   {
>> +    struct drm_sched_entity *s_entity;
>> +    int i;
>> +
>>       if (sched->thread)
>>           kthread_stop(sched->thread);
>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>> DRM_SCHED_PRIORITY_MIN; i--) {
>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>> +
>> +        if (!rq)
>> +            continue;
>> +
>> +        spin_lock(&rq->lock);
>> +        list_for_each_entry(s_entity, &rq->entities, list)
>> +            /*
>> +             * Prevents reinsertion and marks job_queue as idle,
>> +             * it will removed from rq in drm_sched_entity_fini
>> +             * eventually
>> +             */
>> +            s_entity->stopped = true;
>> +        spin_unlock(&rq->lock);
>> +
>> +    }
>> +
>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>> scheduler */
>> +    wake_up_all(&sched->job_scheduled);
>> +
>>       /* Confirm no work left behind accessing device structures */
>>       cancel_delayed_work_sync(&sched->work_tdr);
>

Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>
> On 2021-05-18 10:07 a.m., Christian König wrote:
>> In a separate discussion with Daniel we once more iterated over the 
>> dma_resv requirements and I came to the conclusion that this approach 
>> here won't work reliable.
>>
>> The problem is as following:
>> 1. device A schedules some rendering with into a buffer and exports 
>> it as DMA-buf.
>> 2. device B imports the DMA-buf and wants to consume the rendering, 
>> for the the fence of device A is replaced with a new operation.
>> 3. device B is hot plugged and the new operation canceled/newer 
>> scheduled.
>>
>> The problem is now that we can't do this since the operation of 
>> device A is still running and by signaling our fences we run into the 
>> problem of potential memory corruption.
>
>
> I am not sure this problem you describe above is related to this patch.

Well it is kind of related.

> Here we purely expand the criteria for when sched_entity is
> considered idle in order to prevent a hang on device remove.

And exactly that is problematic. See the jobs on the entity need to 
cleanly wait for their dependencies before they can be completed.

drm_sched_entity_kill_jobs() is also not handling that correctly at the 
moment, we only wait for the last scheduled fence but not for the 
dependencies of the job.

> Were you addressing the patch from yesterday in which you commented
> that you found a problem with how we finish fences ? It was
> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>
> Also, in the patch series as it is now we only signal HW fences for the
> extracted device B, we are not touching any other fences. In fact as you
> may remember, I dropped all new logic to forcing fence completion in
> this patch series and only call amdgpu_fence_driver_force_completion
> for the HW fences of the extracted device as it's done today anyway.

Signaling hardware fences is unproblematic since they are emitted when 
the software scheduling is already completed.

Christian.

>
> Andrey
>
>>
>> Not sure how to handle that case. One possibility would be to wait 
>> for all dependencies of unscheduled jobs before signaling their 
>> fences as canceled.
>>
>> Christian.
>>
>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>> Problem: If scheduler is already stopped by the time sched_entity
>>> is released and entity's job_queue not empty I encountred
>>> a hang in drm_sched_entity_flush. This is because 
>>> drm_sched_entity_is_idle
>>> never becomes false.
>>>
>>> Fix: In drm_sched_fini detach all sched_entities from the
>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>> Also wakeup all those processes stuck in sched_entity flushing
>>> as the scheduler main thread which wakes them up is stopped by now.
>>>
>>> v2:
>>> Reverse order of drm_sched_rq_remove_entity and marking
>>> s_entity as stopped to prevent reinserion back to rq due
>>> to race.
>>>
>>> v3:
>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>> and check for it in drm_sched_entity_is_idle
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>> ++++++++++++++++++++++++
>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 0249c7450188..2e93e881b65f 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>> drm_sched_entity *entity)
>>>       rmb(); /* for list_empty to work without lock */
>>>       if (list_empty(&entity->list) ||
>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>> +        entity->stopped)
>>>           return true;
>>>       return false;
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 8d1211e87101..a2a953693b45 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>    */
>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>   {
>>> +    struct drm_sched_entity *s_entity;
>>> +    int i;
>>> +
>>>       if (sched->thread)
>>>           kthread_stop(sched->thread);
>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>> +
>>> +        if (!rq)
>>> +            continue;
>>> +
>>> +        spin_lock(&rq->lock);
>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>> +            /*
>>> +             * Prevents reinsertion and marks job_queue as idle,
>>> +             * it will removed from rq in drm_sched_entity_fini
>>> +             * eventually
>>> +             */
>>> +            s_entity->stopped = true;
>>> +        spin_unlock(&rq->lock);
>>> +
>>> +    }
>>> +
>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>> scheduler */
>>> +    wake_up_all(&sched->job_scheduled);
>>> +
>>>       /* Confirm no work left behind accessing device structures */
>>>       cancel_delayed_work_sync(&sched->work_tdr);
>>

On 2021-05-18 11:15 a.m., Christian König wrote:
> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>
>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>> In a separate discussion with Daniel we once more iterated over the 
>>> dma_resv requirements and I came to the conclusion that this approach 
>>> here won't work reliable.
>>>
>>> The problem is as following:
>>> 1. device A schedules some rendering with into a buffer and exports 
>>> it as DMA-buf.
>>> 2. device B imports the DMA-buf and wants to consume the rendering, 
>>> for the the fence of device A is replaced with a new operation.
>>> 3. device B is hot plugged and the new operation canceled/newer 
>>> scheduled.
>>>
>>> The problem is now that we can't do this since the operation of 
>>> device A is still running and by signaling our fences we run into the 
>>> problem of potential memory corruption.

By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?

Andrey

>>
>>
>> I am not sure this problem you describe above is related to this patch.
> 
> Well it is kind of related.
> 
>> Here we purely expand the criteria for when sched_entity is
>> considered idle in order to prevent a hang on device remove.
> 
> And exactly that is problematic. See the jobs on the entity need to 
> cleanly wait for their dependencies before they can be completed.
> 
> drm_sched_entity_kill_jobs() is also not handling that correctly at the 
> moment, we only wait for the last scheduled fence but not for the 
> dependencies of the job.
> 
>> Were you addressing the patch from yesterday in which you commented
>> that you found a problem with how we finish fences ? It was
>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>
>> Also, in the patch series as it is now we only signal HW fences for the
>> extracted device B, we are not touching any other fences. In fact as you
>> may remember, I dropped all new logic to forcing fence completion in
>> this patch series and only call amdgpu_fence_driver_force_completion
>> for the HW fences of the extracted device as it's done today anyway.
> 
> Signaling hardware fences is unproblematic since they are emitted when 
> the software scheduling is already completed.
> 
> Christian.
> 
>>
>> Andrey
>>
>>>
>>> Not sure how to handle that case. One possibility would be to wait 
>>> for all dependencies of unscheduled jobs before signaling their 
>>> fences as canceled.
>>>
>>> Christian.
>>>
>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>> is released and entity's job_queue not empty I encountred
>>>> a hang in drm_sched_entity_flush. This is because 
>>>> drm_sched_entity_is_idle
>>>> never becomes false.
>>>>
>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>
>>>> v2:
>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>> s_entity as stopped to prevent reinserion back to rq due
>>>> to race.
>>>>
>>>> v3:
>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>> and check for it in drm_sched_entity_is_idle
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>> ++++++++++++++++++++++++
>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> index 0249c7450188..2e93e881b65f 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>>> drm_sched_entity *entity)
>>>>       rmb(); /* for list_empty to work without lock */
>>>>       if (list_empty(&entity->list) ||
>>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>>> +        entity->stopped)
>>>>           return true;
>>>>       return false;
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index 8d1211e87101..a2a953693b45 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>    */
>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>   {
>>>> +    struct drm_sched_entity *s_entity;
>>>> +    int i;
>>>> +
>>>>       if (sched->thread)
>>>>           kthread_stop(sched->thread);
>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>> +
>>>> +        if (!rq)
>>>> +            continue;
>>>> +
>>>> +        spin_lock(&rq->lock);
>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>> +            /*
>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>> +             * eventually
>>>> +             */
>>>> +            s_entity->stopped = true;
>>>> +        spin_unlock(&rq->lock);
>>>> +
>>>> +    }
>>>> +
>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>>> scheduler */
>>>> +    wake_up_all(&sched->job_scheduled);
>>>> +
>>>>       /* Confirm no work left behind accessing device structures */
>>>>       cancel_delayed_work_sync(&sched->work_tdr);
>>>
>

Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>
>
> On 2021-05-18 11:15 a.m., Christian König wrote:
>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>
>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>> In a separate discussion with Daniel we once more iterated over the 
>>>> dma_resv requirements and I came to the conclusion that this 
>>>> approach here won't work reliable.
>>>>
>>>> The problem is as following:
>>>> 1. device A schedules some rendering with into a buffer and exports 
>>>> it as DMA-buf.
>>>> 2. device B imports the DMA-buf and wants to consume the rendering, 
>>>> for the the fence of device A is replaced with a new operation.
>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>> scheduled.
>>>>
>>>> The problem is now that we can't do this since the operation of 
>>>> device A is still running and by signaling our fences we run into 
>>>> the problem of potential memory corruption.
>
> By signaling s_fence->finished of the canceled operation from the
> removed device B we in fact cause memory corruption for the uncompleted
> job still running on device A ? Because there is someone waiting to
> read write from the imported buffer on device B and he only waits for
> the s_fence->finished of device B we signaled
> in drm_sched_entity_kill_jobs ?

Exactly that, yes.

In other words when you have a dependency chain like A->B->C then memory 
management only waits for C before freeing up the memory for example.

When C now signaled because the device is hot-plugged before A or B are 
finished they are essentially accessing freed up memory.

Christian.

>
> Andrey
>
>>>
>>>
>>> I am not sure this problem you describe above is related to this patch.
>>
>> Well it is kind of related.
>>
>>> Here we purely expand the criteria for when sched_entity is
>>> considered idle in order to prevent a hang on device remove.
>>
>> And exactly that is problematic. See the jobs on the entity need to 
>> cleanly wait for their dependencies before they can be completed.
>>
>> drm_sched_entity_kill_jobs() is also not handling that correctly at 
>> the moment, we only wait for the last scheduled fence but not for the 
>> dependencies of the job.
>>
>>> Were you addressing the patch from yesterday in which you commented
>>> that you found a problem with how we finish fences ? It was
>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>
>>> Also, in the patch series as it is now we only signal HW fences for the
>>> extracted device B, we are not touching any other fences. In fact as 
>>> you
>>> may remember, I dropped all new logic to forcing fence completion in
>>> this patch series and only call amdgpu_fence_driver_force_completion
>>> for the HW fences of the extracted device as it's done today anyway.
>>
>> Signaling hardware fences is unproblematic since they are emitted 
>> when the software scheduling is already completed.
>>
>> Christian.
>>
>>>
>>> Andrey
>>>
>>>>
>>>> Not sure how to handle that case. One possibility would be to wait 
>>>> for all dependencies of unscheduled jobs before signaling their 
>>>> fences as canceled.
>>>>
>>>> Christian.
>>>>
>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>> is released and entity's job_queue not empty I encountred
>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>> drm_sched_entity_is_idle
>>>>> never becomes false.
>>>>>
>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>
>>>>> v2:
>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>> to race.
>>>>>
>>>>> v3:
>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>> and check for it in drm_sched_entity_is_idle
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>> ++++++++++++++++++++++++
>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>>>> drm_sched_entity *entity)
>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>       if (list_empty(&entity->list) ||
>>>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>>>> +        entity->stopped)
>>>>>           return true;
>>>>>       return false;
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>    */
>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>   {
>>>>> +    struct drm_sched_entity *s_entity;
>>>>> +    int i;
>>>>> +
>>>>>       if (sched->thread)
>>>>>           kthread_stop(sched->thread);
>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>> +
>>>>> +        if (!rq)
>>>>> +            continue;
>>>>> +
>>>>> +        spin_lock(&rq->lock);
>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>> +            /*
>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>> +             * eventually
>>>>> +             */
>>>>> +            s_entity->stopped = true;
>>>>> +        spin_unlock(&rq->lock);
>>>>> +
>>>>> +    }
>>>>> +
>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>>>> scheduler */
>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>> +
>>>>>       /* Confirm no work left behind accessing device structures */
>>>>>       cancel_delayed_work_sync(&sched->work_tdr);
>>>>
>>

On 2021-05-18 12:33 p.m., Christian König wrote:
> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>
>>
>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>
>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>> In a separate discussion with Daniel we once more iterated over the 
>>>>> dma_resv requirements and I came to the conclusion that this 
>>>>> approach here won't work reliable.
>>>>>
>>>>> The problem is as following:
>>>>> 1. device A schedules some rendering with into a buffer and exports 
>>>>> it as DMA-buf.
>>>>> 2. device B imports the DMA-buf and wants to consume the rendering, 
>>>>> for the the fence of device A is replaced with a new operation.
>>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>>> scheduled.
>>>>>
>>>>> The problem is now that we can't do this since the operation of 
>>>>> device A is still running and by signaling our fences we run into 
>>>>> the problem of potential memory corruption.
>>
>> By signaling s_fence->finished of the canceled operation from the
>> removed device B we in fact cause memory corruption for the uncompleted
>> job still running on device A ? Because there is someone waiting to
>> read write from the imported buffer on device B and he only waits for
>> the s_fence->finished of device B we signaled
>> in drm_sched_entity_kill_jobs ?
> 
> Exactly that, yes.
> 
> In other words when you have a dependency chain like A->B->C then memory 
> management only waits for C before freeing up the memory for example.
> 
> When C now signaled because the device is hot-plugged before A or B are 
> finished they are essentially accessing freed up memory.

But didn't C imported the BO form B or A in this case ? Why would he be
the one releasing that memory ? He would be just dropping his reference
to the BO, no ?

Also in the general case,
drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
the one who signals the 'C' fence with error code are as far
as I looked called from when the user of that BO is stopping
the usage anyway (e.g. drm_driver.postclose callback for when use
process closes his device file) who would then access and corrupt
the exported memory on device A where the job hasn't completed yet ?

Andrey

> 
> Christian.
> 
>>
>> Andrey
>>
>>>>
>>>>
>>>> I am not sure this problem you describe above is related to this patch.
>>>
>>> Well it is kind of related.
>>>
>>>> Here we purely expand the criteria for when sched_entity is
>>>> considered idle in order to prevent a hang on device remove.
>>>
>>> And exactly that is problematic. See the jobs on the entity need to 
>>> cleanly wait for their dependencies before they can be completed.
>>>
>>> drm_sched_entity_kill_jobs() is also not handling that correctly at 
>>> the moment, we only wait for the last scheduled fence but not for the 
>>> dependencies of the job.
>>>
>>>> Were you addressing the patch from yesterday in which you commented
>>>> that you found a problem with how we finish fences ? It was
>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>
>>>> Also, in the patch series as it is now we only signal HW fences for the
>>>> extracted device B, we are not touching any other fences. In fact as 
>>>> you
>>>> may remember, I dropped all new logic to forcing fence completion in
>>>> this patch series and only call amdgpu_fence_driver_force_completion
>>>> for the HW fences of the extracted device as it's done today anyway.
>>>
>>> Signaling hardware fences is unproblematic since they are emitted 
>>> when the software scheduling is already completed.
>>>
>>> Christian.
>>>
>>>>
>>>> Andrey
>>>>
>>>>>
>>>>> Not sure how to handle that case. One possibility would be to wait 
>>>>> for all dependencies of unscheduled jobs before signaling their 
>>>>> fences as canceled.
>>>>>
>>>>> Christian.
>>>>>
>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>> is released and entity's job_queue not empty I encountred
>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>> drm_sched_entity_is_idle
>>>>>> never becomes false.
>>>>>>
>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>>
>>>>>> v2:
>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>> to race.
>>>>>>
>>>>>> v3:
>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>
>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>> ++++++++++++++++++++++++
>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>>>>> drm_sched_entity *entity)
>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>       if (list_empty(&entity->list) ||
>>>>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>>>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>> +        entity->stopped)
>>>>>>           return true;
>>>>>>       return false;
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>    */
>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>   {
>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>> +    int i;
>>>>>> +
>>>>>>       if (sched->thread)
>>>>>>           kthread_stop(sched->thread);
>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>> +
>>>>>> +        if (!rq)
>>>>>> +            continue;
>>>>>> +
>>>>>> +        spin_lock(&rq->lock);
>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>> +            /*
>>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>> +             * eventually
>>>>>> +             */
>>>>>> +            s_entity->stopped = true;
>>>>>> +        spin_unlock(&rq->lock);
>>>>>> +
>>>>>> +    }
>>>>>> +
>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>>>>> scheduler */
>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>> +
>>>>>>       /* Confirm no work left behind accessing device structures */
>>>>>>       cancel_delayed_work_sync(&sched->work_tdr);
>>>>>
>>>
>

Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
> On 2021-05-18 12:33 p.m., Christian König wrote:
>> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>>
>>>
>>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>>
>>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>>> In a separate discussion with Daniel we once more iterated over 
>>>>>> the dma_resv requirements and I came to the conclusion that this 
>>>>>> approach here won't work reliable.
>>>>>>
>>>>>> The problem is as following:
>>>>>> 1. device A schedules some rendering with into a buffer and 
>>>>>> exports it as DMA-buf.
>>>>>> 2. device B imports the DMA-buf and wants to consume the 
>>>>>> rendering, for the the fence of device A is replaced with a new 
>>>>>> operation.
>>>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>>>> scheduled.
>>>>>>
>>>>>> The problem is now that we can't do this since the operation of 
>>>>>> device A is still running and by signaling our fences we run into 
>>>>>> the problem of potential memory corruption.
>>>
>>> By signaling s_fence->finished of the canceled operation from the
>>> removed device B we in fact cause memory corruption for the uncompleted
>>> job still running on device A ? Because there is someone waiting to
>>> read write from the imported buffer on device B and he only waits for
>>> the s_fence->finished of device B we signaled
>>> in drm_sched_entity_kill_jobs ?
>>
>> Exactly that, yes.
>>
>> In other words when you have a dependency chain like A->B->C then 
>> memory management only waits for C before freeing up the memory for 
>> example.
>>
>> When C now signaled because the device is hot-plugged before A or B 
>> are finished they are essentially accessing freed up memory.
>
> But didn't C imported the BO form B or A in this case ? Why would he be
> the one releasing that memory ? He would be just dropping his reference
> to the BO, no ?

Well freeing the memory was just an example. The BO could also move back 
to VRAM because of the dropped reference.

> Also in the general case,
> drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
> the one who signals the 'C' fence with error code are as far
> as I looked called from when the user of that BO is stopping
> the usage anyway (e.g. drm_driver.postclose callback for when use
> process closes his device file) who would then access and corrupt
> the exported memory on device A where the job hasn't completed yet ?

Key point is that memory management only waits for the last added fence, 
that is the design of the dma_resv object. How that happens is irrelevant.

Because of this we at least need to wait for all dependencies of the job 
before signaling the fence, even if we cancel the job for some reason.

Christian.

>
> Andrey
>
>>
>> Christian.
>>
>>>
>>> Andrey
>>>
>>>>>
>>>>>
>>>>> I am not sure this problem you describe above is related to this 
>>>>> patch.
>>>>
>>>> Well it is kind of related.
>>>>
>>>>> Here we purely expand the criteria for when sched_entity is
>>>>> considered idle in order to prevent a hang on device remove.
>>>>
>>>> And exactly that is problematic. See the jobs on the entity need to 
>>>> cleanly wait for their dependencies before they can be completed.
>>>>
>>>> drm_sched_entity_kill_jobs() is also not handling that correctly at 
>>>> the moment, we only wait for the last scheduled fence but not for 
>>>> the dependencies of the job.
>>>>
>>>>> Were you addressing the patch from yesterday in which you commented
>>>>> that you found a problem with how we finish fences ? It was
>>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>>
>>>>> Also, in the patch series as it is now we only signal HW fences 
>>>>> for the
>>>>> extracted device B, we are not touching any other fences. In fact 
>>>>> as you
>>>>> may remember, I dropped all new logic to forcing fence completion in
>>>>> this patch series and only call amdgpu_fence_driver_force_completion
>>>>> for the HW fences of the extracted device as it's done today anyway.
>>>>
>>>> Signaling hardware fences is unproblematic since they are emitted 
>>>> when the software scheduling is already completed.
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> Andrey
>>>>>
>>>>>>
>>>>>> Not sure how to handle that case. One possibility would be to 
>>>>>> wait for all dependencies of unscheduled jobs before signaling 
>>>>>> their fences as canceled.
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>>> drm_sched_entity_is_idle
>>>>>>> never becomes false.
>>>>>>>
>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>>>
>>>>>>> v2:
>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>> to race.
>>>>>>>
>>>>>>> v3:
>>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>>
>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>>> ++++++++++++++++++++++++
>>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>>>>>> drm_sched_entity *entity)
>>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>>       if (list_empty(&entity->list) ||
>>>>>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>>>>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>>> +        entity->stopped)
>>>>>>>           return true;
>>>>>>>       return false;
>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>>    */
>>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>>   {
>>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>>> +    int i;
>>>>>>> +
>>>>>>>       if (sched->thread)
>>>>>>>           kthread_stop(sched->thread);
>>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>> +
>>>>>>> +        if (!rq)
>>>>>>> +            continue;
>>>>>>> +
>>>>>>> +        spin_lock(&rq->lock);
>>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>>> +            /*
>>>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>>> +             * eventually
>>>>>>> +             */
>>>>>>> +            s_entity->stopped = true;
>>>>>>> +        spin_unlock(&rq->lock);
>>>>>>> +
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>>>>>> scheduler */
>>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>>> +
>>>>>>>       /* Confirm no work left behind accessing device structures */
>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>
>>>>
>>

On 2021-05-18 2:02 p.m., Christian König wrote:
> Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
>> On 2021-05-18 12:33 p.m., Christian König wrote:
>>> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>>>
>>>>
>>>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>>>
>>>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>>>> In a separate discussion with Daniel we once more iterated over 
>>>>>>> the dma_resv requirements and I came to the conclusion that this 
>>>>>>> approach here won't work reliable.
>>>>>>>
>>>>>>> The problem is as following:
>>>>>>> 1. device A schedules some rendering with into a buffer and 
>>>>>>> exports it as DMA-buf.
>>>>>>> 2. device B imports the DMA-buf and wants to consume the 
>>>>>>> rendering, for the the fence of device A is replaced with a new 
>>>>>>> operation.
>>>>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>>>>> scheduled.
>>>>>>>
>>>>>>> The problem is now that we can't do this since the operation of 
>>>>>>> device A is still running and by signaling our fences we run into 
>>>>>>> the problem of potential memory corruption.
>>>>
>>>> By signaling s_fence->finished of the canceled operation from the
>>>> removed device B we in fact cause memory corruption for the uncompleted
>>>> job still running on device A ? Because there is someone waiting to
>>>> read write from the imported buffer on device B and he only waits for
>>>> the s_fence->finished of device B we signaled
>>>> in drm_sched_entity_kill_jobs ?
>>>
>>> Exactly that, yes.
>>>
>>> In other words when you have a dependency chain like A->B->C then 
>>> memory management only waits for C before freeing up the memory for 
>>> example.
>>>
>>> When C now signaled because the device is hot-plugged before A or B 
>>> are finished they are essentially accessing freed up memory.
>>
>> But didn't C imported the BO form B or A in this case ? Why would he be
>> the one releasing that memory ? He would be just dropping his reference
>> to the BO, no ?
> 
> Well freeing the memory was just an example. The BO could also move back 
> to VRAM because of the dropped reference.
> 
>> Also in the general case,
>> drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
>> the one who signals the 'C' fence with error code are as far
>> as I looked called from when the user of that BO is stopping
>> the usage anyway (e.g. drm_driver.postclose callback for when use
>> process closes his device file) who would then access and corrupt
>> the exported memory on device A where the job hasn't completed yet ?
> 
> Key point is that memory management only waits for the last added fence, 
> that is the design of the dma_resv object. How that happens is irrelevant.
> 
> Because of this we at least need to wait for all dependencies of the job 
> before signaling the fence, even if we cancel the job for some reason.
> 
> Christian.

Would this be the right way to do it ?

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index 2e93e881b65f..10f784874b63 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct 
drm_sched_entity *entity)
  {
         struct drm_sched_job *job;
         int r;
+       struct dma_fence *f;

         while ((job = 
to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
                 struct drm_sched_fence *s_fence = job->s_fence;

+               while (f = sched->ops->dependency(sched_job, entity))
+                       dma_fence_wait(f);
+
                 drm_sched_fence_scheduled(s_fence);
                 dma_fence_set_error(&s_fence->finished, -ESRCH);

Andrey



> 
>>
>> Andrey
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Andrey
>>>>
>>>>>>
>>>>>>
>>>>>> I am not sure this problem you describe above is related to this 
>>>>>> patch.
>>>>>
>>>>> Well it is kind of related.
>>>>>
>>>>>> Here we purely expand the criteria for when sched_entity is
>>>>>> considered idle in order to prevent a hang on device remove.
>>>>>
>>>>> And exactly that is problematic. See the jobs on the entity need to 
>>>>> cleanly wait for their dependencies before they can be completed.
>>>>>
>>>>> drm_sched_entity_kill_jobs() is also not handling that correctly at 
>>>>> the moment, we only wait for the last scheduled fence but not for 
>>>>> the dependencies of the job.
>>>>>
>>>>>> Were you addressing the patch from yesterday in which you commented
>>>>>> that you found a problem with how we finish fences ? It was
>>>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>>>
>>>>>> Also, in the patch series as it is now we only signal HW fences 
>>>>>> for the
>>>>>> extracted device B, we are not touching any other fences. In fact 
>>>>>> as you
>>>>>> may remember, I dropped all new logic to forcing fence completion in
>>>>>> this patch series and only call amdgpu_fence_driver_force_completion
>>>>>> for the HW fences of the extracted device as it's done today anyway.
>>>>>
>>>>> Signaling hardware fences is unproblematic since they are emitted 
>>>>> when the software scheduling is already completed.
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Andrey
>>>>>>
>>>>>>>
>>>>>>> Not sure how to handle that case. One possibility would be to 
>>>>>>> wait for all dependencies of unscheduled jobs before signaling 
>>>>>>> their fences as canceled.
>>>>>>>
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>>>> drm_sched_entity_is_idle
>>>>>>>> never becomes false.
>>>>>>>>
>>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>>> to race.
>>>>>>>>
>>>>>>>> v3:
>>>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>>>
>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct 
>>>>>>>> drm_sched_entity *entity)
>>>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>>>       if (list_empty(&entity->list) ||
>>>>>>>> -        spsc_queue_count(&entity->job_queue) == 0)
>>>>>>>> +        spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>>>> +        entity->stopped)
>>>>>>>>           return true;
>>>>>>>>       return false;
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>>>    */
>>>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>>>   {
>>>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>>>> +    int i;
>>>>>>>> +
>>>>>>>>       if (sched->thread)
>>>>>>>>           kthread_stop(sched->thread);
>>>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>>> +
>>>>>>>> +        if (!rq)
>>>>>>>> +            continue;
>>>>>>>> +
>>>>>>>> +        spin_lock(&rq->lock);
>>>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>>>> +            /*
>>>>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>>>> +             * eventually
>>>>>>>> +             */
>>>>>>>> +            s_entity->stopped = true;
>>>>>>>> +        spin_unlock(&rq->lock);
>>>>>>>> +
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for this 
>>>>>>>> scheduler */
>>>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>>>> +
>>>>>>>>       /* Confirm no work left behind accessing device structures */
>>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>>
>>>>>
>>>
>

Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
> On 2021-05-18 2:02 p.m., Christian König wrote:
>> Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
>>> On 2021-05-18 12:33 p.m., Christian König wrote:
>>>> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>>>>
>>>>>
>>>>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>>>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>>>>
>>>>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>>>>> In a separate discussion with Daniel we once more iterated over 
>>>>>>>> the dma_resv requirements and I came to the conclusion that 
>>>>>>>> this approach here won't work reliable.
>>>>>>>>
>>>>>>>> The problem is as following:
>>>>>>>> 1. device A schedules some rendering with into a buffer and 
>>>>>>>> exports it as DMA-buf.
>>>>>>>> 2. device B imports the DMA-buf and wants to consume the 
>>>>>>>> rendering, for the the fence of device A is replaced with a new 
>>>>>>>> operation.
>>>>>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>>>>>> scheduled.
>>>>>>>>
>>>>>>>> The problem is now that we can't do this since the operation of 
>>>>>>>> device A is still running and by signaling our fences we run 
>>>>>>>> into the problem of potential memory corruption.
>>>>>
>>>>> By signaling s_fence->finished of the canceled operation from the
>>>>> removed device B we in fact cause memory corruption for the 
>>>>> uncompleted
>>>>> job still running on device A ? Because there is someone waiting to
>>>>> read write from the imported buffer on device B and he only waits for
>>>>> the s_fence->finished of device B we signaled
>>>>> in drm_sched_entity_kill_jobs ?
>>>>
>>>> Exactly that, yes.
>>>>
>>>> In other words when you have a dependency chain like A->B->C then 
>>>> memory management only waits for C before freeing up the memory for 
>>>> example.
>>>>
>>>> When C now signaled because the device is hot-plugged before A or B 
>>>> are finished they are essentially accessing freed up memory.
>>>
>>> But didn't C imported the BO form B or A in this case ? Why would he be
>>> the one releasing that memory ? He would be just dropping his reference
>>> to the BO, no ?
>>
>> Well freeing the memory was just an example. The BO could also move 
>> back to VRAM because of the dropped reference.
>>
>>> Also in the general case,
>>> drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
>>> the one who signals the 'C' fence with error code are as far
>>> as I looked called from when the user of that BO is stopping
>>> the usage anyway (e.g. drm_driver.postclose callback for when use
>>> process closes his device file) who would then access and corrupt
>>> the exported memory on device A where the job hasn't completed yet ?
>>
>> Key point is that memory management only waits for the last added 
>> fence, that is the design of the dma_resv object. How that happens is 
>> irrelevant.
>>
>> Because of this we at least need to wait for all dependencies of the 
>> job before signaling the fence, even if we cancel the job for some 
>> reason.
>>
>> Christian.
>
> Would this be the right way to do it ?

Yes, it is at least a start. Question is if we can wait blocking here or 
not.

We install a callback a bit lower to avoid blocking, so I'm pretty sure 
that won't work as expected.

Christian.

>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
> b/drivers/gpu/drm/scheduler/sched_entity.c
> index 2e93e881b65f..10f784874b63 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct 
> drm_sched_entity *entity)
>  {
>         struct drm_sched_job *job;
>         int r;
> +       struct dma_fence *f;
>
>         while ((job = 
> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>                 struct drm_sched_fence *s_fence = job->s_fence;
>
> +               while (f = sched->ops->dependency(sched_job, entity))
> +                       dma_fence_wait(f);
> +
>                 drm_sched_fence_scheduled(s_fence);
>                 dma_fence_set_error(&s_fence->finished, -ESRCH);
>
> Andrey
>
>
>
>>
>>>
>>> Andrey
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> Andrey
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I am not sure this problem you describe above is related to this 
>>>>>>> patch.
>>>>>>
>>>>>> Well it is kind of related.
>>>>>>
>>>>>>> Here we purely expand the criteria for when sched_entity is
>>>>>>> considered idle in order to prevent a hang on device remove.
>>>>>>
>>>>>> And exactly that is problematic. See the jobs on the entity need 
>>>>>> to cleanly wait for their dependencies before they can be completed.
>>>>>>
>>>>>> drm_sched_entity_kill_jobs() is also not handling that correctly 
>>>>>> at the moment, we only wait for the last scheduled fence but not 
>>>>>> for the dependencies of the job.
>>>>>>
>>>>>>> Were you addressing the patch from yesterday in which you commented
>>>>>>> that you found a problem with how we finish fences ? It was
>>>>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>>>>
>>>>>>> Also, in the patch series as it is now we only signal HW fences 
>>>>>>> for the
>>>>>>> extracted device B, we are not touching any other fences. In 
>>>>>>> fact as you
>>>>>>> may remember, I dropped all new logic to forcing fence 
>>>>>>> completion in
>>>>>>> this patch series and only call 
>>>>>>> amdgpu_fence_driver_force_completion
>>>>>>> for the HW fences of the extracted device as it's done today 
>>>>>>> anyway.
>>>>>>
>>>>>> Signaling hardware fences is unproblematic since they are emitted 
>>>>>> when the software scheduling is already completed.
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Andrey
>>>>>>>
>>>>>>>>
>>>>>>>> Not sure how to handle that case. One possibility would be to 
>>>>>>>> wait for all dependencies of unscheduled jobs before signaling 
>>>>>>>> their fences as canceled.
>>>>>>>>
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>>>>> drm_sched_entity_is_idle
>>>>>>>>> never becomes false.
>>>>>>>>>
>>>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>>>> scheduler's run queues. This will satisfy 
>>>>>>>>> drm_sched_entity_is_idle.
>>>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>>>> as the scheduler main thread which wakes them up is stopped by 
>>>>>>>>> now.
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>>>> to race.
>>>>>>>>>
>>>>>>>>> v3:
>>>>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>>>>
>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>> @@ -116,7 +116,8 @@ static bool 
>>>>>>>>> drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>>>>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>>>>       if (list_empty(&entity->list) ||
>>>>>>>>> - spsc_queue_count(&entity->job_queue) == 0)
>>>>>>>>> + spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>>>>> +        entity->stopped)
>>>>>>>>>           return true;
>>>>>>>>>       return false;
>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>>>>    */
>>>>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>>>>   {
>>>>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>>>>> +    int i;
>>>>>>>>> +
>>>>>>>>>       if (sched->thread)
>>>>>>>>>           kthread_stop(sched->thread);
>>>>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>>>> +
>>>>>>>>> +        if (!rq)
>>>>>>>>> +            continue;
>>>>>>>>> +
>>>>>>>>> +        spin_lock(&rq->lock);
>>>>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>>>>> +            /*
>>>>>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>>>>> +             * eventually
>>>>>>>>> +             */
>>>>>>>>> +            s_entity->stopped = true;
>>>>>>>>> +        spin_unlock(&rq->lock);
>>>>>>>>> +
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for 
>>>>>>>>> this scheduler */
>>>>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>>>>> +
>>>>>>>>>       /* Confirm no work left behind accessing device 
>>>>>>>>> structures */
>>>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>>>
>>>>>>
>>>>
>>

On 2021-05-18 2:13 p.m., Christian König wrote:
> 
> Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
>> On 2021-05-18 2:02 p.m., Christian König wrote:
>>> Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
>>>> On 2021-05-18 12:33 p.m., Christian König wrote:
>>>>> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>>>>>
>>>>>>
>>>>>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>>>>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>>>>>
>>>>>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>>>>>> In a separate discussion with Daniel we once more iterated over 
>>>>>>>>> the dma_resv requirements and I came to the conclusion that 
>>>>>>>>> this approach here won't work reliable.
>>>>>>>>>
>>>>>>>>> The problem is as following:
>>>>>>>>> 1. device A schedules some rendering with into a buffer and 
>>>>>>>>> exports it as DMA-buf.
>>>>>>>>> 2. device B imports the DMA-buf and wants to consume the 
>>>>>>>>> rendering, for the the fence of device A is replaced with a new 
>>>>>>>>> operation.
>>>>>>>>> 3. device B is hot plugged and the new operation canceled/newer 
>>>>>>>>> scheduled.
>>>>>>>>>
>>>>>>>>> The problem is now that we can't do this since the operation of 
>>>>>>>>> device A is still running and by signaling our fences we run 
>>>>>>>>> into the problem of potential memory corruption.
>>>>>>
>>>>>> By signaling s_fence->finished of the canceled operation from the
>>>>>> removed device B we in fact cause memory corruption for the 
>>>>>> uncompleted
>>>>>> job still running on device A ? Because there is someone waiting to
>>>>>> read write from the imported buffer on device B and he only waits for
>>>>>> the s_fence->finished of device B we signaled
>>>>>> in drm_sched_entity_kill_jobs ?
>>>>>
>>>>> Exactly that, yes.
>>>>>
>>>>> In other words when you have a dependency chain like A->B->C then 
>>>>> memory management only waits for C before freeing up the memory for 
>>>>> example.
>>>>>
>>>>> When C now signaled because the device is hot-plugged before A or B 
>>>>> are finished they are essentially accessing freed up memory.
>>>>
>>>> But didn't C imported the BO form B or A in this case ? Why would he be
>>>> the one releasing that memory ? He would be just dropping his reference
>>>> to the BO, no ?
>>>
>>> Well freeing the memory was just an example. The BO could also move 
>>> back to VRAM because of the dropped reference.
>>>
>>>> Also in the general case,
>>>> drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
>>>> the one who signals the 'C' fence with error code are as far
>>>> as I looked called from when the user of that BO is stopping
>>>> the usage anyway (e.g. drm_driver.postclose callback for when use
>>>> process closes his device file) who would then access and corrupt
>>>> the exported memory on device A where the job hasn't completed yet ?
>>>
>>> Key point is that memory management only waits for the last added 
>>> fence, that is the design of the dma_resv object. How that happens is 
>>> irrelevant.
>>>
>>> Because of this we at least need to wait for all dependencies of the 
>>> job before signaling the fence, even if we cancel the job for some 
>>> reason.
>>>
>>> Christian.
>>
>> Would this be the right way to do it ?
> 
> Yes, it is at least a start. Question is if we can wait blocking here or 
> not.
> 
> We install a callback a bit lower to avoid blocking, so I'm pretty sure 
> that won't work as expected.
> 
> Christian.

I can't see why this would create problems, as long as the dependencies
complete or force competed if they are from same device (extracted) but
on a different ring then looks to me it should work. I will give it
a try.

Andrey

> 
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 2e93e881b65f..10f784874b63 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct 
>> drm_sched_entity *entity)
>>  {
>>         struct drm_sched_job *job;
>>         int r;
>> +       struct dma_fence *f;
>>
>>         while ((job = 
>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>                 struct drm_sched_fence *s_fence = job->s_fence;
>>
>> +               while (f = sched->ops->dependency(sched_job, entity))
>> +                       dma_fence_wait(f);
>> +
>>                 drm_sched_fence_scheduled(s_fence);
>>                 dma_fence_set_error(&s_fence->finished, -ESRCH);
>>
>> Andrey
>>
>>
>>
>>>
>>>>
>>>> Andrey
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Andrey
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I am not sure this problem you describe above is related to this 
>>>>>>>> patch.
>>>>>>>
>>>>>>> Well it is kind of related.
>>>>>>>
>>>>>>>> Here we purely expand the criteria for when sched_entity is
>>>>>>>> considered idle in order to prevent a hang on device remove.
>>>>>>>
>>>>>>> And exactly that is problematic. See the jobs on the entity need 
>>>>>>> to cleanly wait for their dependencies before they can be completed.
>>>>>>>
>>>>>>> drm_sched_entity_kill_jobs() is also not handling that correctly 
>>>>>>> at the moment, we only wait for the last scheduled fence but not 
>>>>>>> for the dependencies of the job.
>>>>>>>
>>>>>>>> Were you addressing the patch from yesterday in which you commented
>>>>>>>> that you found a problem with how we finish fences ? It was
>>>>>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>>>>>
>>>>>>>> Also, in the patch series as it is now we only signal HW fences 
>>>>>>>> for the
>>>>>>>> extracted device B, we are not touching any other fences. In 
>>>>>>>> fact as you
>>>>>>>> may remember, I dropped all new logic to forcing fence 
>>>>>>>> completion in
>>>>>>>> this patch series and only call 
>>>>>>>> amdgpu_fence_driver_force_completion
>>>>>>>> for the HW fences of the extracted device as it's done today 
>>>>>>>> anyway.
>>>>>>>
>>>>>>> Signaling hardware fences is unproblematic since they are emitted 
>>>>>>> when the software scheduling is already completed.
>>>>>>>
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>> Andrey
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Not sure how to handle that case. One possibility would be to 
>>>>>>>>> wait for all dependencies of unscheduled jobs before signaling 
>>>>>>>>> their fences as canceled.
>>>>>>>>>
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>>>>>> drm_sched_entity_is_idle
>>>>>>>>>> never becomes false.
>>>>>>>>>>
>>>>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>>>>> scheduler's run queues. This will satisfy 
>>>>>>>>>> drm_sched_entity_is_idle.
>>>>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>>>>> as the scheduler main thread which wakes them up is stopped by 
>>>>>>>>>> now.
>>>>>>>>>>
>>>>>>>>>> v2:
>>>>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>>>>> to race.
>>>>>>>>>>
>>>>>>>>>> v3:
>>>>>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>> @@ -116,7 +116,8 @@ static bool 
>>>>>>>>>> drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>>>>>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>>>>>       if (list_empty(&entity->list) ||
>>>>>>>>>> - spsc_queue_count(&entity->job_queue) == 0)
>>>>>>>>>> + spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>>>>>> +        entity->stopped)
>>>>>>>>>>           return true;
>>>>>>>>>>       return false;
>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>>>>>    */
>>>>>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>>>>>   {
>>>>>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>>>>>> +    int i;
>>>>>>>>>> +
>>>>>>>>>>       if (sched->thread)
>>>>>>>>>>           kthread_stop(sched->thread);
>>>>>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>>>>> +
>>>>>>>>>> +        if (!rq)
>>>>>>>>>> +            continue;
>>>>>>>>>> +
>>>>>>>>>> +        spin_lock(&rq->lock);
>>>>>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>>>>>> +            /*
>>>>>>>>>> +             * Prevents reinsertion and marks job_queue as idle,
>>>>>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>>>>>> +             * eventually
>>>>>>>>>> +             */
>>>>>>>>>> +            s_entity->stopped = true;
>>>>>>>>>> +        spin_unlock(&rq->lock);
>>>>>>>>>> +
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for 
>>>>>>>>>> this scheduler */
>>>>>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>>>>>> +
>>>>>>>>>>       /* Confirm no work left behind accessing device 
>>>>>>>>>> structures */
>>>>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>

On 2021-05-18 2:48 p.m., Andrey Grodzovsky wrote:
> 
> 
> On 2021-05-18 2:13 p.m., Christian König wrote:
>>
>> Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
>>> On 2021-05-18 2:02 p.m., Christian König wrote:
>>>> Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
>>>>> On 2021-05-18 12:33 p.m., Christian König wrote:
>>>>>> Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
>>>>>>>
>>>>>>>
>>>>>>> On 2021-05-18 11:15 a.m., Christian König wrote:
>>>>>>>> Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
>>>>>>>>>
>>>>>>>>> On 2021-05-18 10:07 a.m., Christian König wrote:
>>>>>>>>>> In a separate discussion with Daniel we once more iterated 
>>>>>>>>>> over the dma_resv requirements and I came to the conclusion 
>>>>>>>>>> that this approach here won't work reliable.
>>>>>>>>>>
>>>>>>>>>> The problem is as following:
>>>>>>>>>> 1. device A schedules some rendering with into a buffer and 
>>>>>>>>>> exports it as DMA-buf.
>>>>>>>>>> 2. device B imports the DMA-buf and wants to consume the 
>>>>>>>>>> rendering, for the the fence of device A is replaced with a 
>>>>>>>>>> new operation.
>>>>>>>>>> 3. device B is hot plugged and the new operation 
>>>>>>>>>> canceled/newer scheduled.
>>>>>>>>>>
>>>>>>>>>> The problem is now that we can't do this since the operation 
>>>>>>>>>> of device A is still running and by signaling our fences we 
>>>>>>>>>> run into the problem of potential memory corruption.
>>>>>>>
>>>>>>> By signaling s_fence->finished of the canceled operation from the
>>>>>>> removed device B we in fact cause memory corruption for the 
>>>>>>> uncompleted
>>>>>>> job still running on device A ? Because there is someone waiting to
>>>>>>> read write from the imported buffer on device B and he only waits 
>>>>>>> for
>>>>>>> the s_fence->finished of device B we signaled
>>>>>>> in drm_sched_entity_kill_jobs ?
>>>>>>
>>>>>> Exactly that, yes.
>>>>>>
>>>>>> In other words when you have a dependency chain like A->B->C then 
>>>>>> memory management only waits for C before freeing up the memory 
>>>>>> for example.
>>>>>>
>>>>>> When C now signaled because the device is hot-plugged before A or 
>>>>>> B are finished they are essentially accessing freed up memory.
>>>>>
>>>>> But didn't C imported the BO form B or A in this case ? Why would 
>>>>> he be
>>>>> the one releasing that memory ? He would be just dropping his 
>>>>> reference
>>>>> to the BO, no ?
>>>>
>>>> Well freeing the memory was just an example. The BO could also move 
>>>> back to VRAM because of the dropped reference.
>>>>
>>>>> Also in the general case,
>>>>> drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
>>>>> the one who signals the 'C' fence with error code are as far
>>>>> as I looked called from when the user of that BO is stopping
>>>>> the usage anyway (e.g. drm_driver.postclose callback for when use
>>>>> process closes his device file) who would then access and corrupt
>>>>> the exported memory on device A where the job hasn't completed yet ?
>>>>
>>>> Key point is that memory management only waits for the last added 
>>>> fence, that is the design of the dma_resv object. How that happens 
>>>> is irrelevant.
>>>>
>>>> Because of this we at least need to wait for all dependencies of the 
>>>> job before signaling the fence, even if we cancel the job for some 
>>>> reason.
>>>>
>>>> Christian.
>>>
>>> Would this be the right way to do it ?
>>
>> Yes, it is at least a start. Question is if we can wait blocking here 
>> or not.
>>
>> We install a callback a bit lower to avoid blocking, so I'm pretty 
>> sure that won't work as expected.
>>
>> Christian.
> 
> I can't see why this would create problems, as long as the dependencies
> complete or force competed if they are from same device (extracted) but
> on a different ring then looks to me it should work. I will give it
> a try.
> 
> Andrey

Well, i gave it a try with my usual tests like IGT hot unplug while
rapid command submissions and unplug the card while glxgears runs
with DRI_PRIME=1 and haven't seen issues.

Andrey

> 
>>
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 2e93e881b65f..10f784874b63 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct 
>>> drm_sched_entity *entity)
>>>  {
>>>         struct drm_sched_job *job;
>>>         int r;
>>> +       struct dma_fence *f;
>>>
>>>         while ((job = 
>>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>>                 struct drm_sched_fence *s_fence = job->s_fence;
>>>
>>> +               while (f = sched->ops->dependency(sched_job, entity))
>>> +                       dma_fence_wait(f);
>>> +
>>>                 drm_sched_fence_scheduled(s_fence);
>>>                 dma_fence_set_error(&s_fence->finished, -ESRCH);
>>>
>>> Andrey
>>>
>>>
>>>
>>>>
>>>>>
>>>>> Andrey
>>>>>
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Andrey
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am not sure this problem you describe above is related to 
>>>>>>>>> this patch.
>>>>>>>>
>>>>>>>> Well it is kind of related.
>>>>>>>>
>>>>>>>>> Here we purely expand the criteria for when sched_entity is
>>>>>>>>> considered idle in order to prevent a hang on device remove.
>>>>>>>>
>>>>>>>> And exactly that is problematic. See the jobs on the entity need 
>>>>>>>> to cleanly wait for their dependencies before they can be 
>>>>>>>> completed.
>>>>>>>>
>>>>>>>> drm_sched_entity_kill_jobs() is also not handling that correctly 
>>>>>>>> at the moment, we only wait for the last scheduled fence but not 
>>>>>>>> for the dependencies of the job.
>>>>>>>>
>>>>>>>>> Were you addressing the patch from yesterday in which you 
>>>>>>>>> commented
>>>>>>>>> that you found a problem with how we finish fences ? It was
>>>>>>>>> '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
>>>>>>>>>
>>>>>>>>> Also, in the patch series as it is now we only signal HW fences 
>>>>>>>>> for the
>>>>>>>>> extracted device B, we are not touching any other fences. In 
>>>>>>>>> fact as you
>>>>>>>>> may remember, I dropped all new logic to forcing fence 
>>>>>>>>> completion in
>>>>>>>>> this patch series and only call 
>>>>>>>>> amdgpu_fence_driver_force_completion
>>>>>>>>> for the HW fences of the extracted device as it's done today 
>>>>>>>>> anyway.
>>>>>>>>
>>>>>>>> Signaling hardware fences is unproblematic since they are 
>>>>>>>> emitted when the software scheduling is already completed.
>>>>>>>>
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrey
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Not sure how to handle that case. One possibility would be to 
>>>>>>>>>> wait for all dependencies of unscheduled jobs before signaling 
>>>>>>>>>> their fences as canceled.
>>>>>>>>>>
>>>>>>>>>> Christian.
>>>>>>>>>>
>>>>>>>>>> Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
>>>>>>>>>>> Problem: If scheduler is already stopped by the time 
>>>>>>>>>>> sched_entity
>>>>>>>>>>> is released and entity's job_queue not empty I encountred
>>>>>>>>>>> a hang in drm_sched_entity_flush. This is because 
>>>>>>>>>>> drm_sched_entity_is_idle
>>>>>>>>>>> never becomes false.
>>>>>>>>>>>
>>>>>>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>>>>>>> scheduler's run queues. This will satisfy 
>>>>>>>>>>> drm_sched_entity_is_idle.
>>>>>>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>>>>>>> as the scheduler main thread which wakes them up is stopped 
>>>>>>>>>>> by now.
>>>>>>>>>>>
>>>>>>>>>>> v2:
>>>>>>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>>>>>>> to race.
>>>>>>>>>>>
>>>>>>>>>>> v3:
>>>>>>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>>>>>>> ---
>>>>>>>>>>>   drivers/gpu/drm/scheduler/sched_entity.c |  3 ++-
>>>>>>>>>>>   drivers/gpu/drm/scheduler/sched_main.c   | 24 
>>>>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>>>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>>> index 0249c7450188..2e93e881b65f 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>>>>> @@ -116,7 +116,8 @@ static bool 
>>>>>>>>>>> drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>>>>>>>>>>>       rmb(); /* for list_empty to work without lock */
>>>>>>>>>>>       if (list_empty(&entity->list) ||
>>>>>>>>>>> - spsc_queue_count(&entity->job_queue) == 0)
>>>>>>>>>>> + spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>>>>>>> +        entity->stopped)
>>>>>>>>>>>           return true;
>>>>>>>>>>>       return false;
>>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>>> index 8d1211e87101..a2a953693b45 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>>>>> @@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>>>>>>>    */
>>>>>>>>>>>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>>>>>>>   {
>>>>>>>>>>> +    struct drm_sched_entity *s_entity;
>>>>>>>>>>> +    int i;
>>>>>>>>>>> +
>>>>>>>>>>>       if (sched->thread)
>>>>>>>>>>>           kthread_stop(sched->thread);
>>>>>>>>>>> +    for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= 
>>>>>>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>>>>>>> +        struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>>>>>>> +
>>>>>>>>>>> +        if (!rq)
>>>>>>>>>>> +            continue;
>>>>>>>>>>> +
>>>>>>>>>>> +        spin_lock(&rq->lock);
>>>>>>>>>>> +        list_for_each_entry(s_entity, &rq->entities, list)
>>>>>>>>>>> +            /*
>>>>>>>>>>> +             * Prevents reinsertion and marks job_queue as 
>>>>>>>>>>> idle,
>>>>>>>>>>> +             * it will removed from rq in drm_sched_entity_fini
>>>>>>>>>>> +             * eventually
>>>>>>>>>>> +             */
>>>>>>>>>>> +            s_entity->stopped = true;
>>>>>>>>>>> +        spin_unlock(&rq->lock);
>>>>>>>>>>> +
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    /* Wakeup everyone stuck in drm_sched_entity_flush for 
>>>>>>>>>>> this scheduler */
>>>>>>>>>>> +    wake_up_all(&sched->job_scheduled);
>>>>>>>>>>> +
>>>>>>>>>>>       /* Confirm no work left behind accessing device 
>>>>>>>>>>> structures */
>>>>>>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>

Am 18.05.21 um 20:48 schrieb Andrey Grodzovsky:
> [SNIP]
>>>
>>> Would this be the right way to do it ?
>>
>> Yes, it is at least a start. Question is if we can wait blocking here 
>> or not.
>>
>> We install a callback a bit lower to avoid blocking, so I'm pretty 
>> sure that won't work as expected.
>>
>> Christian.
>
> I can't see why this would create problems, as long as the dependencies
> complete or force competed if they are from same device (extracted) but
> on a different ring then looks to me it should work. I will give it
> a try.

Ok, but please also test the case for a killed process.

Christian.

>
> Andrey

On 2021-05-19 6:57 a.m., Christian König wrote:
> Am 18.05.21 um 20:48 schrieb Andrey Grodzovsky:
>> [SNIP]
>>>>
>>>> Would this be the right way to do it ?
>>>
>>> Yes, it is at least a start. Question is if we can wait blocking here 
>>> or not.
>>>
>>> We install a callback a bit lower to avoid blocking, so I'm pretty 
>>> sure that won't work as expected.
>>>
>>> Christian.
>>
>> I can't see why this would create problems, as long as the dependencies
>> complete or force competed if they are from same device (extracted) but
>> on a different ring then looks to me it should work. I will give it
>> a try.
> 
> Ok, but please also test the case for a killed process.
> 
> Christian.

You mean something like run glxgears and then simply
terminate it ? Because I done that. Or something more ?

Andrey


> 
>>
>> Andrey
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cce1252e55fae4338710d08d91ab4de01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637570186393107071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vGqxY5sxpEIiQGFBNn2PWkKqVjviM29r34Yjv0wujf4%3D&reserved=0 
>

Am 19.05.21 um 13:03 schrieb Andrey Grodzovsky:
>
>
> On 2021-05-19 6:57 a.m., Christian König wrote:
>> Am 18.05.21 um 20:48 schrieb Andrey Grodzovsky:
>>> [SNIP]
>>>>>
>>>>> Would this be the right way to do it ?
>>>>
>>>> Yes, it is at least a start. Question is if we can wait blocking 
>>>> here or not.
>>>>
>>>> We install a callback a bit lower to avoid blocking, so I'm pretty 
>>>> sure that won't work as expected.
>>>>
>>>> Christian.
>>>
>>> I can't see why this would create problems, as long as the dependencies
>>> complete or force competed if they are from same device (extracted) but
>>> on a different ring then looks to me it should work. I will give it
>>> a try.
>>
>> Ok, but please also test the case for a killed process.
>>
>> Christian.
>
> You mean something like run glxgears and then simply
> terminate it ? Because I done that. Or something more ?

Well glxgears is a bit to lightweight for that.

You need at least some test which is limited by the rendering pipeline.

Christian.

>
> Andrey
>
>
>>
>>>
>>> Andrey
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cce1252e55fae4338710d08d91ab4de01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637570186393107071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vGqxY5sxpEIiQGFBNn2PWkKqVjviM29r34Yjv0wujf4%3D&reserved=0 
>>

On 2021-05-19 7:46 a.m., Christian König wrote:
> Am 19.05.21 um 13:03 schrieb Andrey Grodzovsky:
>>
>>
>> On 2021-05-19 6:57 a.m., Christian König wrote:
>>> Am 18.05.21 um 20:48 schrieb Andrey Grodzovsky:
>>>> [SNIP]
>>>>>>
>>>>>> Would this be the right way to do it ?
>>>>>
>>>>> Yes, it is at least a start. Question is if we can wait blocking 
>>>>> here or not.
>>>>>
>>>>> We install a callback a bit lower to avoid blocking, so I'm pretty 
>>>>> sure that won't work as expected.
>>>>>
>>>>> Christian.
>>>>
>>>> I can't see why this would create problems, as long as the dependencies
>>>> complete or force competed if they are from same device (extracted) but
>>>> on a different ring then looks to me it should work. I will give it
>>>> a try.
>>>
>>> Ok, but please also test the case for a killed process.
>>>
>>> Christian.
>>
>> You mean something like run glxgears and then simply
>> terminate it ? Because I done that. Or something more ?
> 
> Well glxgears is a bit to lightweight for that.
> 
> You need at least some test which is limited by the rendering pipeline.
> 
> Christian.

You mean something that fill the entity queue faster then sched thread
empties it so when we kill the process we actually need to explicitly go
through remaining jobs termination ? I done that too by inserting
artificial delay in drm_sched_main.

Andrey

> 
>>
>> Andrey
>>
>>
>>>
>>>>
>>>> Andrey
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cce1252e55fae4338710d08d91ab4de01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637570186393107071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vGqxY5sxpEIiQGFBNn2PWkKqVjviM29r34Yjv0wujf4%3D&reserved=0 
>>>
>

Am 19.05.21 um 13:51 schrieb Andrey Grodzovsky:
>
>
> On 2021-05-19 7:46 a.m., Christian König wrote:
>> Am 19.05.21 um 13:03 schrieb Andrey Grodzovsky:
>>>
>>>
>>> On 2021-05-19 6:57 a.m., Christian König wrote:
>>>> Am 18.05.21 um 20:48 schrieb Andrey Grodzovsky:
>>>>> [SNIP]
>>>>>>>
>>>>>>> Would this be the right way to do it ?
>>>>>>
>>>>>> Yes, it is at least a start. Question is if we can wait blocking 
>>>>>> here or not.
>>>>>>
>>>>>> We install a callback a bit lower to avoid blocking, so I'm 
>>>>>> pretty sure that won't work as expected.
>>>>>>
>>>>>> Christian.
>>>>>
>>>>> I can't see why this would create problems, as long as the 
>>>>> dependencies
>>>>> complete or force competed if they are from same device 
>>>>> (extracted) but
>>>>> on a different ring then looks to me it should work. I will give it
>>>>> a try.
>>>>
>>>> Ok, but please also test the case for a killed process.
>>>>
>>>> Christian.
>>>
>>> You mean something like run glxgears and then simply
>>> terminate it ? Because I done that. Or something more ?
>>
>> Well glxgears is a bit to lightweight for that.
>>
>> You need at least some test which is limited by the rendering pipeline.
>>
>> Christian.
>
> You mean something that fill the entity queue faster then sched thread
> empties it so when we kill the process we actually need to explicitly go
> through remaining jobs termination ? I done that too by inserting
> artificial delay in drm_sched_main.

Yeah, something like that.

Ok in that case I would say that this should work then.

Christian.

>
> Andrey
>
>>
>>>
>>> Andrey
>>>
>>>
>>>>
>>>>>
>>>>> Andrey
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cce1252e55fae4338710d08d91ab4de01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637570186393107071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vGqxY5sxpEIiQGFBNn2PWkKqVjviM29r34Yjv0wujf4%3D&reserved=0 
>>>>
>>