| Message ID | 20220907205705.934688-1-James.Zhu@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/sched: returns struct drm_gpu_scheduler ** for drm_sched_pick_best | expand |
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); >
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); >
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);
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);
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);
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);
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);
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);
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);
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(-)