[PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[James Zhu]

drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
struct drm_gpu_scheduler *

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 include/drm/gpu_scheduler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 0fca8f38bee4..011f70a43397 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct drm_sched_fence *fence);
 unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
 void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
 		                unsigned long remaining);
-struct drm_gpu_scheduler *
+struct drm_gpu_scheduler **
 drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
 		     unsigned int num_sched_list);
 
-- 
2.25.1

[PATCH 2/4] drm/sched: implement new drm_sched_pick_best

[James Zhu]

drm_sched_pick_best return best selecetd ring schedul list's address.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c   | 14 ++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 191c56064f19..f5595607995b 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -475,7 +475,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		return;
 
 	spin_lock(&entity->rq_lock);
-	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
+	sched = *drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
 	rq = sched ? &sched->sched_rq[entity->priority] : NULL;
 	if (rq != entity->rq) {
 		drm_sched_rq_remove_entity(entity->rq, entity);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 68317d3a7a27..111277f6c53c 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -863,12 +863,12 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
  * Returns pointer of the sched with the least load or NULL if none of the
  * drm_gpu_schedulers are ready
  */
-struct drm_gpu_scheduler *
+struct drm_gpu_scheduler **
 drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
 		     unsigned int num_sched_list)
 {
-	struct drm_gpu_scheduler *sched, *picked_sched = NULL;
-	int i;
+	struct drm_gpu_scheduler *sched;
+	int i, picked_idx = -1;
 	unsigned int min_score = UINT_MAX, num_score;
 
 	for (i = 0; i < num_sched_list; ++i) {
@@ -883,11 +883,13 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
 		num_score = atomic_read(sched->score);
 		if (num_score < min_score) {
 			min_score = num_score;
-			picked_sched = sched;
+			picked_idx = i;
 		}
 	}
-
-	return picked_sched;
+	if (picked_idx != -1)
+		return &(sched_list[picked_idx]);
+	else
+		return (struct drm_gpu_scheduler **)(NULL);
 }
 EXPORT_SYMBOL(drm_sched_pick_best);
 
-- 
2.25.1

[PATCH 3/4] drm/sched: always keep selecetd ring sched list in ctx entity

[James Zhu]

Always keep selecetd ring sched list in ctx entity.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index f5595607995b..39dca9cb8e0d 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
-	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+	entity->sched_list = sched_list;
 	entity->last_scheduled = NULL;
 
 	if(num_sched_list)
@@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_sched_rq *rq;
 
 	/* single possible engine and already selected */
-	if (!entity->sched_list)
+	if (entity->num_sched_list <= 1)
 		return;
 
 	/* queue non-empty, stay on the same engine */
@@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		entity->rq = rq;
 	}
 	spin_unlock(&entity->rq_lock);
-
-	if (entity->num_sched_list == 1)
-		entity->sched_list = NULL;
 }
 
 /**
-- 
2.25.1

[PATCH 4/4] drm/amdgpu: update amdgpu_ctx_init_entity

[James Zhu]

update amdgpu_ctx_init_entity with new drm_sched_pick_best.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 6ea8980c8ad7..3a1cb0a70392 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -201,7 +201,7 @@ static ktime_t amdgpu_ctx_entity_time(struct amdgpu_ctx *ctx,
 static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
 				  const u32 ring)
 {
-	struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
+	struct drm_gpu_scheduler **scheds = NULL;
 	struct amdgpu_device *adev = ctx->mgr->adev;
 	struct amdgpu_ctx_entity *entity;
 	enum drm_sched_priority drm_prio;
@@ -230,8 +230,7 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
 	    hw_ip == AMDGPU_HW_IP_VCN_DEC ||
 	    hw_ip == AMDGPU_HW_IP_UVD_ENC ||
 	    hw_ip == AMDGPU_HW_IP_UVD) {
-		sched = drm_sched_pick_best(scheds, num_scheds);
-		scheds = &sched;
+		scheds = drm_sched_pick_best(scheds, num_scheds);
 		num_scheds = 1;
 	}
 
-- 
2.25.1

[PATCH v2 3/4] drm/sched: always keep selected ring sched list in ctx entity

[James Zhu]

Always keep selected ring sched list in ctx entity.

v2: fixed typo

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index f5595607995b..39dca9cb8e0d 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
-	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+	entity->sched_list = sched_list;
 	entity->last_scheduled = NULL;
 
 	if(num_sched_list)
@@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_sched_rq *rq;
 
 	/* single possible engine and already selected */
-	if (!entity->sched_list)
+	if (entity->num_sched_list <= 1)
 		return;
 
 	/* queue non-empty, stay on the same engine */
@@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		entity->rq = rq;
 	}
 	spin_unlock(&entity->rq_lock);
-
-	if (entity->num_sched_list == 1)
-		entity->sched_list = NULL;
 }
 
 /**
-- 
2.25.1

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[Christian König]

Hui? That's certainly not correct.

Christian.

Am 07.09.22 um 22:57 schrieb James Zhu:
> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
> struct drm_gpu_scheduler *
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   include/drm/gpu_scheduler.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 0fca8f38bee4..011f70a43397 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct drm_sched_fence *fence);
>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>   		                unsigned long remaining);
> -struct drm_gpu_scheduler *
> +struct drm_gpu_scheduler **
>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>   		     unsigned int num_sched_list);
>   

Re: [PATCH 3/4] drm/sched: always keep selecetd ring sched list in ctx entity

[Christian König]

Am 07.09.22 um 22:57 schrieb James Zhu:
> Always keep selecetd ring sched list in ctx entity.

I have no idea what you are doing here, but this certainly doesn't make 
sense.

Please explain a bit more.

Thanks,
Christian.

>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index f5595607995b..39dca9cb8e0d 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>   	entity->guilty = guilty;
>   	entity->num_sched_list = num_sched_list;
>   	entity->priority = priority;
> -	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
> +	entity->sched_list = sched_list;
>   	entity->last_scheduled = NULL;
>   
>   	if(num_sched_list)
> @@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>   	struct drm_sched_rq *rq;
>   
>   	/* single possible engine and already selected */
> -	if (!entity->sched_list)
> +	if (entity->num_sched_list <= 1)
>   		return;
>   
>   	/* queue non-empty, stay on the same engine */
> @@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>   		entity->rq = rq;
>   	}
>   	spin_unlock(&entity->rq_lock);
> -
> -	if (entity->num_sched_list == 1)
> -		entity->sched_list = NULL;
>   }
>   
>   /**

Re: [PATCH 3/4] drm/sched: always keep selecetd ring sched list in ctx entity

[James Zhu]

Hi Christian

I need use entity->sched_list to track ring (ring = container_of(sched, 
struct amdgpu_ring, sched))

during amdgpu_ctx_fini_entity.

I think change here to keep selected ring sched list in 
entity->sched_list won't change the original logic too much.

Best Regards!

James


On 2022-09-08 2:15 a.m., Christian König wrote:
> Am 07.09.22 um 22:57 schrieb James Zhu:
>> Always keep selecetd ring sched list in ctx entity.
>
> I have no idea what you are doing here, but this certainly doesn't 
> make sense.
>
> Please explain a bit more.
>
> Thanks,
> Christian.
>
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index f5595607995b..39dca9cb8e0d 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity 
>> *entity,
>>       entity->guilty = guilty;
>>       entity->num_sched_list = num_sched_list;
>>       entity->priority = priority;
>> -    entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>> +    entity->sched_list = sched_list;
>>       entity->last_scheduled = NULL;
>>         if(num_sched_list)
>> @@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct 
>> drm_sched_entity *entity)
>>       struct drm_sched_rq *rq;
>>         /* single possible engine and already selected */
>> -    if (!entity->sched_list)
>> +    if (entity->num_sched_list <= 1)
>>           return;
>>         /* queue non-empty, stay on the same engine */
>> @@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct 
>> drm_sched_entity *entity)
>>           entity->rq = rq;
>>       }
>>       spin_unlock(&entity->rq_lock);
>> -
>> -    if (entity->num_sched_list == 1)
>> -        entity->sched_list = NULL;
>>   }
>>     /**
>

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[Andrey Grodzovsky]

What's the reason for this entire patch set ?

Andrey

On 2022-09-07 16:57, James Zhu wrote:
> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
> struct drm_gpu_scheduler *
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   include/drm/gpu_scheduler.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 0fca8f38bee4..011f70a43397 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct drm_sched_fence *fence);
>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched);
>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>   		                unsigned long remaining);
> -struct drm_gpu_scheduler *
> +struct drm_gpu_scheduler **
>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>   		     unsigned int num_sched_list);
>   

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[James Zhu]

Hi Andrey

Basically this entire patch set are derived from patch [3/4]: 
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;

I think no special reason to treat single and multiple schedule list here.

Best Regards!

James

On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
> What's the reason for this entire patch set ?
>
> Andrey
>
> On 2022-09-07 16:57, James Zhu wrote:
>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>> struct drm_gpu_scheduler *
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   include/drm/gpu_scheduler.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 0fca8f38bee4..011f70a43397 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>> drm_sched_fence *fence);
>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler 
>> *sched);
>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>                           unsigned long remaining);
>> -struct drm_gpu_scheduler *
>> +struct drm_gpu_scheduler **
>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>                unsigned int num_sched_list);

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[Andrey Grodzovsky]

I guess it's an option but i don't really see what's the added value  ? 
You saved a few lines in this patch
but added a few lines in another. In total seems to me no to much 
difference ?

Andrey

On 2022-09-08 10:17, James Zhu wrote:
> Hi Andrey
>
> Basically this entire patch set are derived from patch [3/4]: 
> entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>
> I think no special reason to treat single and multiple schedule list 
> here.
>
> Best Regards!
>
> James
>
> On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
>> What's the reason for this entire patch set ?
>>
>> Andrey
>>
>> On 2022-09-07 16:57, James Zhu wrote:
>>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>>> struct drm_gpu_scheduler *
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>> ---
>>>   include/drm/gpu_scheduler.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 0fca8f38bee4..011f70a43397 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>>> drm_sched_fence *fence);
>>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler 
>>> *sched);
>>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>>                           unsigned long remaining);
>>> -struct drm_gpu_scheduler *
>>> +struct drm_gpu_scheduler **
>>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>                unsigned int num_sched_list);

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[James Zhu]

To save lines is not the purpose.

Also I want to use entity->sched_list to track ring which is used in 
this ctx in amdgpu_ctx_fini_entity

Best Regards!

James

On 2022-09-08 10:38 a.m., Andrey Grodzovsky wrote:
> I guess it's an option but i don't really see what's the added value  
> ? You saved a few lines in this patch
> but added a few lines in another. In total seems to me no to much 
> difference ?
>
> Andrey
>
> On 2022-09-08 10:17, James Zhu wrote:
>> Hi Andrey
>>
>> Basically this entire patch set are derived from patch [3/4]: 
>> entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>
>> I think no special reason to treat single and multiple schedule list 
>> here.
>>
>> Best Regards!
>>
>> James
>>
>> On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
>>> What's the reason for this entire patch set ?
>>>
>>> Andrey
>>>
>>> On 2022-09-07 16:57, James Zhu wrote:
>>>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>>>> struct drm_gpu_scheduler *
>>>>
>>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>>> ---
>>>>   include/drm/gpu_scheduler.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>> index 0fca8f38bee4..011f70a43397 100644
>>>> --- a/include/drm/gpu_scheduler.h
>>>> +++ b/include/drm/gpu_scheduler.h
>>>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>>>> drm_sched_fence *fence);
>>>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler 
>>>> *sched);
>>>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>>>                           unsigned long remaining);
>>>> -struct drm_gpu_scheduler *
>>>> +struct drm_gpu_scheduler **
>>>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>>                unsigned int num_sched_list);

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[Andrey Grodzovsky]

So this is the real need of this patch-set, but this explanation doesn't 
appear anywhere in the description.
It's always good to add a short 0 RFC patch which describes the 
intention of the patchset if the code is
not self explanatory.

And I still don't understand the need - i don't see anything in 
amdgpu_ctx_fini_entity regarding
rings tracking ? Is it a new code you plan to add and not included in 
this patcheset ? Did i miss an
earlier patch maybe ?

Andrey

On 2022-09-08 10:45, James Zhu wrote:
> To save lines is not the purpose.
>
> Also I want to use entity->sched_list to track ring which is used in 
> this ctx in amdgpu_ctx_fini_entity
>
> Best Regards!
>
> James
>
> On 2022-09-08 10:38 a.m., Andrey Grodzovsky wrote:
>> I guess it's an option but i don't really see what's the added value  
>> ? You saved a few lines in this patch
>> but added a few lines in another. In total seems to me no to much 
>> difference ?
>>
>> Andrey
>>
>> On 2022-09-08 10:17, James Zhu wrote:
>>> Hi Andrey
>>>
>>> Basically this entire patch set are derived from patch [3/4]: 
>>> entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>>
>>> I think no special reason to treat single and multiple schedule list 
>>> here.
>>>
>>> Best Regards!
>>>
>>> James
>>>
>>> On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
>>>> What's the reason for this entire patch set ?
>>>>
>>>> Andrey
>>>>
>>>> On 2022-09-07 16:57, James Zhu wrote:
>>>>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>>>>> struct drm_gpu_scheduler *
>>>>>
>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>>>> ---
>>>>>   include/drm/gpu_scheduler.h | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>> b/include/drm/gpu_scheduler.h
>>>>> index 0fca8f38bee4..011f70a43397 100644
>>>>> --- a/include/drm/gpu_scheduler.h
>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>>>>> drm_sched_fence *fence);
>>>>>   unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler 
>>>>> *sched);
>>>>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>>>>                           unsigned long remaining);
>>>>> -struct drm_gpu_scheduler *
>>>>> +struct drm_gpu_scheduler **
>>>>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>>>                unsigned int num_sched_list);

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[James Zhu]

Yes, it  is for NPI design. I will send out patches for review soon.

Thanks!

James

On 2022-09-08 11:05 a.m., Andrey Grodzovsky wrote:
> So this is the real need of this patch-set, but this explanation 
> doesn't appear anywhere in the description.
> It's always good to add a short 0 RFC patch which describes the 
> intention of the patchset if the code is
> not self explanatory.
>
> And I still don't understand the need - i don't see anything in 
> amdgpu_ctx_fini_entity regarding
> rings tracking ? Is it a new code you plan to add and not included in 
> this patcheset ? Did i miss an
> earlier patch maybe ?
>
> Andrey
>
> On 2022-09-08 10:45, James Zhu wrote:
>> To save lines is not the purpose.
>>
>> Also I want to use entity->sched_list to track ring which is used in 
>> this ctx in amdgpu_ctx_fini_entity
>>
>> Best Regards!
>>
>> James
>>
>> On 2022-09-08 10:38 a.m., Andrey Grodzovsky wrote:
>>> I guess it's an option but i don't really see what's the added 
>>> value  ? You saved a few lines in this patch
>>> but added a few lines in another. In total seems to me no to much 
>>> difference ?
>>>
>>> Andrey
>>>
>>> On 2022-09-08 10:17, James Zhu wrote:
>>>> Hi Andrey
>>>>
>>>> Basically this entire patch set are derived from patch [3/4]: 
>>>> entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>>>
>>>> I think no special reason to treat single and multiple schedule 
>>>> list here.
>>>>
>>>> Best Regards!
>>>>
>>>> James
>>>>
>>>> On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
>>>>> What's the reason for this entire patch set ?
>>>>>
>>>>> Andrey
>>>>>
>>>>> On 2022-09-07 16:57, James Zhu wrote:
>>>>>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>>>>>> struct drm_gpu_scheduler *
>>>>>>
>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>>>>> ---
>>>>>>   include/drm/gpu_scheduler.h | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>>> b/include/drm/gpu_scheduler.h
>>>>>> index 0fca8f38bee4..011f70a43397 100644
>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>>>>>> drm_sched_fence *fence);
>>>>>>   unsigned long drm_sched_suspend_timeout(struct 
>>>>>> drm_gpu_scheduler *sched);
>>>>>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>>>>>                           unsigned long remaining);
>>>>>> -struct drm_gpu_scheduler *
>>>>>> +struct drm_gpu_scheduler **
>>>>>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>>>>                unsigned int num_sched_list);

[PATCH v3 3/4] drm/sched: always keep selected ring sched list in ctx entity

[James Zhu]

Always keep selected ring sched list in ctx entity.
Later entity->sched_list can always be used to track ring which
is used in this ctx in amdgpu_ctx_fini_entity.

v2: fixed typo
v3. Update comments

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index f5595607995b..39dca9cb8e0d 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
-	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+	entity->sched_list = sched_list;
 	entity->last_scheduled = NULL;
 
 	if(num_sched_list)
@@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_sched_rq *rq;
 
 	/* single possible engine and already selected */
-	if (!entity->sched_list)
+	if (entity->num_sched_list <= 1)
 		return;
 
 	/* queue non-empty, stay on the same engine */
@@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		entity->rq = rq;
 	}
 	spin_unlock(&entity->rq_lock);
-
-	if (entity->num_sched_list == 1)
-		entity->sched_list = NULL;
 }
 
 /**
-- 
2.25.1

Re: [PATCH 1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best

[Andrey Grodzovsky]

Please send everything together because otherwise it's not clear why we 
need this.

Andrey

On 2022-09-08 11:09, James Zhu wrote:
> Yes, it is for NPI design. I will send out patches for review soon.
>
> Thanks!
>
> James
>
> On 2022-09-08 11:05 a.m., Andrey Grodzovsky wrote:
>> So this is the real need of this patch-set, but this explanation 
>> doesn't appear anywhere in the description.
>> It's always good to add a short 0 RFC patch which describes the 
>> intention of the patchset if the code is
>> not self explanatory.
>>
>> And I still don't understand the need - i don't see anything in 
>> amdgpu_ctx_fini_entity regarding
>> rings tracking ? Is it a new code you plan to add and not included in 
>> this patcheset ? Did i miss an
>> earlier patch maybe ?
>>
>> Andrey
>>
>> On 2022-09-08 10:45, James Zhu wrote:
>>> To save lines is not the purpose.
>>>
>>> Also I want to use entity->sched_list to track ring which is used in 
>>> this ctx in amdgpu_ctx_fini_entity
>>>
>>> Best Regards!
>>>
>>> James
>>>
>>> On 2022-09-08 10:38 a.m., Andrey Grodzovsky wrote:
>>>> I guess it's an option but i don't really see what's the added 
>>>> value  ? You saved a few lines in this patch
>>>> but added a few lines in another. In total seems to me no to much 
>>>> difference ?
>>>>
>>>> Andrey
>>>>
>>>> On 2022-09-08 10:17, James Zhu wrote:
>>>>> Hi Andrey
>>>>>
>>>>> Basically this entire patch set are derived from patch [3/4]: 
>>>>> entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>>>>
>>>>> I think no special reason to treat single and multiple schedule 
>>>>> list here.
>>>>>
>>>>> Best Regards!
>>>>>
>>>>> James
>>>>>
>>>>> On 2022-09-08 10:08 a.m., Andrey Grodzovsky wrote:
>>>>>> What's the reason for this entire patch set ?
>>>>>>
>>>>>> Andrey
>>>>>>
>>>>>> On 2022-09-07 16:57, James Zhu wrote:
>>>>>>> drm_sched_pick_best returns struct drm_gpu_scheduler ** instead of
>>>>>>> struct drm_gpu_scheduler *
>>>>>>>
>>>>>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>>>>>> ---
>>>>>>>   include/drm/gpu_scheduler.h | 2 +-
>>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>>>> b/include/drm/gpu_scheduler.h
>>>>>>> index 0fca8f38bee4..011f70a43397 100644
>>>>>>> --- a/include/drm/gpu_scheduler.h
>>>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>>>> @@ -529,7 +529,7 @@ void drm_sched_fence_finished(struct 
>>>>>>> drm_sched_fence *fence);
>>>>>>>   unsigned long drm_sched_suspend_timeout(struct 
>>>>>>> drm_gpu_scheduler *sched);
>>>>>>>   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
>>>>>>>                           unsigned long remaining);
>>>>>>> -struct drm_gpu_scheduler *
>>>>>>> +struct drm_gpu_scheduler **
>>>>>>>   drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>>>>>                unsigned int num_sched_list);

Re: [PATCH 3/4] drm/sched: always keep selecetd ring sched list in ctx entity

[Christian König]

Hi James,

please use to_amdgpu_ring(entity->rq->sched) for this.

That's the scheduler which was actually picked for this entity.

Regards,
Christian.

Am 08.09.22 um 15:19 schrieb James Zhu:
> Hi Christian
>
> I need use entity->sched_list to track ring (ring = 
> container_of(sched, struct amdgpu_ring, sched))
>
> during amdgpu_ctx_fini_entity.
>
> I think change here to keep selected ring sched list in 
> entity->sched_list won't change the original logic too much.
>
> Best Regards!
>
> James
>
>
> On 2022-09-08 2:15 a.m., Christian König wrote:
>> Am 07.09.22 um 22:57 schrieb James Zhu:
>>> Always keep selecetd ring sched list in ctx entity.
>>
>> I have no idea what you are doing here, but this certainly doesn't 
>> make sense.
>>
>> Please explain a bit more.
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index f5595607995b..39dca9cb8e0d 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -71,7 +71,7 @@ int drm_sched_entity_init(struct drm_sched_entity 
>>> *entity,
>>>       entity->guilty = guilty;
>>>       entity->num_sched_list = num_sched_list;
>>>       entity->priority = priority;
>>> -    entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>> +    entity->sched_list = sched_list;
>>>       entity->last_scheduled = NULL;
>>>         if(num_sched_list)
>>> @@ -453,7 +453,7 @@ void drm_sched_entity_select_rq(struct 
>>> drm_sched_entity *entity)
>>>       struct drm_sched_rq *rq;
>>>         /* single possible engine and already selected */
>>> -    if (!entity->sched_list)
>>> +    if (entity->num_sched_list <= 1)
>>>           return;
>>>         /* queue non-empty, stay on the same engine */
>>> @@ -482,9 +482,6 @@ void drm_sched_entity_select_rq(struct 
>>> drm_sched_entity *entity)
>>>           entity->rq = rq;
>>>       }
>>>       spin_unlock(&entity->rq_lock);
>>> -
>>> -    if (entity->num_sched_list == 1)
>>> -        entity->sched_list = NULL;
>>>   }
>>>     /**
>>