| Message ID | 20230216162729.219979-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Add a gpu page-table walker | expand |
On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: > A slightly unusual cover letter for a single patch. > > The page table walker is currently used by the xe driver only, > but the code is generic so we can be good citizens and add it to drm > as a helper, for possible use by other drivers, > If so we can merge the commit when we merge the xe driver. > > The question raised here is > *) Should it be a generic drm helper or xe-specific with changed > prefixes? I think if there's some other drivers interested in using this, then this sounds like a good idea. Maybe more useful if it's also integrated into the vm/vma helpers that are being discussed as an optional part? Maybe some good old sales pitching here to convince people would be good. Maybe one of the new accel drivers is interested in this too? > *) If a drm helper, should we use a config option? I am no fan of Kconfig things tbh. Maybe just include it in the vma helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, drm/sched, this one here, vm/vma helpers or whatever they will be and so on? Kinda like we have modeset helpers. I'd definitely not go for a Kconfig per individual file, that's just excessive. -Daniel > > For usage examples, see xe_pt.c > https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c > > Thanks, > Thomas > > Thomas Hellström (1): > drm: Add a gpu page-table walker helper > > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ > include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ > 3 files changed, 321 insertions(+) > create mode 100644 drivers/gpu/drm/drm_pt_walk.c > create mode 100644 include/drm/drm_pt_walk.h > > -- > 2.34.1 >
Hi, Daniel, On 2/16/23 21:18, Daniel Vetter wrote: > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: >> A slightly unusual cover letter for a single patch. >> >> The page table walker is currently used by the xe driver only, >> but the code is generic so we can be good citizens and add it to drm >> as a helper, for possible use by other drivers, >> If so we can merge the commit when we merge the xe driver. >> >> The question raised here is >> *) Should it be a generic drm helper or xe-specific with changed >> prefixes? > I think if there's some other drivers interested in using this, then this > sounds like a good idea. Maybe more useful if it's also integrated into > the vm/vma helpers that are being discussed as an optional part? > > Maybe some good old sales pitching here to convince people would be good. > > Maybe one of the new accel drivers is interested in this too? Thanks for your thoughts on this. Yeah, I think it's a bit awkward to push for having code generic when there is only one user, and the prospect of having other drivers rewrite their page-table building code based on this helper in the near future is probably small. Perhaps more of interest to new drivers. I think what will happen otherwise is that during some future cleanup this will be pushed down to xe claiming it's the only user. I wonder whether it might be an idea to maintain a small document where driver writers can list suggestions for code that could be lifted to core drm and be reused by others. That way both reviewers and writers of other drivers can keep an eye on that document and use it to avoid duplicating code. The procedure would then be to lift it to core drm and fix up prefixes as soon as we have two or more users. Thoughts? Thomas > >> *) If a drm helper, should we use a config option? > I am no fan of Kconfig things tbh. Maybe just include it in the vma > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, > drm/sched, this one here, vm/vma helpers or whatever they will be and so > on? Kinda like we have modeset helpers. > > I'd definitely not go for a Kconfig per individual file, that's just > excessive. > -Daniel > >> For usage examples, see xe_pt.c >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c >> >> Thanks, >> Thomas >> >> Thomas Hellström (1): >> drm: Add a gpu page-table walker helper >> >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ >> include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 321 insertions(+) >> create mode 100644 drivers/gpu/drm/drm_pt_walk.c >> create mode 100644 include/drm/drm_pt_walk.h >> >> -- >> 2.34.1 >>
On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > > Hi, Daniel, > > On 2/16/23 21:18, Daniel Vetter wrote: > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: > >> A slightly unusual cover letter for a single patch. > >> > >> The page table walker is currently used by the xe driver only, > >> but the code is generic so we can be good citizens and add it to drm > >> as a helper, for possible use by other drivers, > >> If so we can merge the commit when we merge the xe driver. > >> > >> The question raised here is > >> *) Should it be a generic drm helper or xe-specific with changed > >> prefixes? > > I think if there's some other drivers interested in using this, then this > > sounds like a good idea. Maybe more useful if it's also integrated into > > the vm/vma helpers that are being discussed as an optional part? > > > > Maybe some good old sales pitching here to convince people would be good. > > > > Maybe one of the new accel drivers is interested in this too? > > Thanks for your thoughts on this. Yeah, I think it's a bit awkward to > push for having code generic when there is only one user, and the > prospect of having other drivers rewrite their page-table building code > based on this helper in the near future is probably small. Perhaps more > of interest to new drivers. I think what will happen otherwise is that > during some future cleanup this will be pushed down to xe claiming it's > the only user. > > I wonder whether it might be an idea to maintain a small document where > driver writers can list suggestions for code that could be lifted to > core drm and be reused by others. That way both reviewers and writers of > other drivers can keep an eye on that document and use it to avoid > duplicating code. The procedure would then be to lift it to core drm and > fix up prefixes as soon as we have two or more users. > > Thoughts? FWIW, when we originally wrote the GPU scheduler it was part of amdgpu, but we consciously kept any AMD-isms out of it so it could be lifted up to a core component when another user came along. Maybe some comments in the top of those files to that effect to maintain the separation. Alex > > Thomas > > > > > >> *) If a drm helper, should we use a config option? > > I am no fan of Kconfig things tbh. Maybe just include it in the vma > > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, > > drm/sched, this one here, vm/vma helpers or whatever they will be and so > > on? Kinda like we have modeset helpers. > > > > I'd definitely not go for a Kconfig per individual file, that's just > > excessive. > > -Daniel > > > >> For usage examples, see xe_pt.c > >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c > >> > >> Thanks, > >> Thomas > >> > >> Thomas Hellström (1): > >> drm: Add a gpu page-table walker helper > >> > >> drivers/gpu/drm/Makefile | 1 + > >> drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ > >> include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ > >> 3 files changed, 321 insertions(+) > >> create mode 100644 drivers/gpu/drm/drm_pt_walk.c > >> create mode 100644 include/drm/drm_pt_walk.h > >> > >> -- > >> 2.34.1 > >>
On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote: > > On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström > <thomas.hellstrom@linux.intel.com> wrote: > > > > Hi, Daniel, > > > > On 2/16/23 21:18, Daniel Vetter wrote: > > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: > > >> A slightly unusual cover letter for a single patch. > > >> > > >> The page table walker is currently used by the xe driver only, > > >> but the code is generic so we can be good citizens and add it to drm > > >> as a helper, for possible use by other drivers, > > >> If so we can merge the commit when we merge the xe driver. > > >> > > >> The question raised here is > > >> *) Should it be a generic drm helper or xe-specific with changed > > >> prefixes? > > > I think if there's some other drivers interested in using this, then this > > > sounds like a good idea. Maybe more useful if it's also integrated into > > > the vm/vma helpers that are being discussed as an optional part? > > > > > > Maybe some good old sales pitching here to convince people would be good. > > > > > > Maybe one of the new accel drivers is interested in this too? Hi, As the habanalabs driver is not really a new driver, I currently don't see the benefit of moving to this code. Our pgt code is quite mature and was tested extensively in deployment in the past couple of years. Nevertheless, I'll try to offer this code for any new/future driver that will want to join accel. Stanislaw, I'm adding you here in case you missed this. Might be of an interest to you. Thanks, Oded - Oded > > > > Thanks for your thoughts on this. Yeah, I think it's a bit awkward to > > push for having code generic when there is only one user, and the > > prospect of having other drivers rewrite their page-table building code > > based on this helper in the near future is probably small. Perhaps more > > of interest to new drivers. I think what will happen otherwise is that > > during some future cleanup this will be pushed down to xe claiming it's > > the only user. > > > > I wonder whether it might be an idea to maintain a small document where > > driver writers can list suggestions for code that could be lifted to > > core drm and be reused by others. That way both reviewers and writers of > > other drivers can keep an eye on that document and use it to avoid > > duplicating code. The procedure would then be to lift it to core drm and > > fix up prefixes as soon as we have two or more users. > > > > Thoughts? > > FWIW, when we originally wrote the GPU scheduler it was part of > amdgpu, but we consciously kept any AMD-isms out of it so it could be > lifted up to a core component when another user came along. Maybe > some comments in the top of those files to that effect to maintain the > separation. > > Alex > > > > > > Thomas > > > > > > > > > >> *) If a drm helper, should we use a config option? > > > I am no fan of Kconfig things tbh. Maybe just include it in the vma > > > helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, > > > drm/sched, this one here, vm/vma helpers or whatever they will be and so > > > on? Kinda like we have modeset helpers. > > > > > > I'd definitely not go for a Kconfig per individual file, that's just > > > excessive. > > > -Daniel > > > > > >> For usage examples, see xe_pt.c > > >> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c > > >> > > >> Thanks, > > >> Thomas > > >> > > >> Thomas Hellström (1): > > >> drm: Add a gpu page-table walker helper > > >> > > >> drivers/gpu/drm/Makefile | 1 + > > >> drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ > > >> include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ > > >> 3 files changed, 321 insertions(+) > > >> create mode 100644 drivers/gpu/drm/drm_pt_walk.c > > >> create mode 100644 include/drm/drm_pt_walk.h > > >> > > >> -- > > >> 2.34.1 > > >>
Hi, On 2/23/23 19:50, Alex Deucher wrote: > On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström > <thomas.hellstrom@linux.intel.com> wrote: >> Hi, Daniel, >> >> On 2/16/23 21:18, Daniel Vetter wrote: >>> On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: >>>> A slightly unusual cover letter for a single patch. >>>> >>>> The page table walker is currently used by the xe driver only, >>>> but the code is generic so we can be good citizens and add it to drm >>>> as a helper, for possible use by other drivers, >>>> If so we can merge the commit when we merge the xe driver. >>>> >>>> The question raised here is >>>> *) Should it be a generic drm helper or xe-specific with changed >>>> prefixes? >>> I think if there's some other drivers interested in using this, then this >>> sounds like a good idea. Maybe more useful if it's also integrated into >>> the vm/vma helpers that are being discussed as an optional part? >>> >>> Maybe some good old sales pitching here to convince people would be good. >>> >>> Maybe one of the new accel drivers is interested in this too? >> Thanks for your thoughts on this. Yeah, I think it's a bit awkward to >> push for having code generic when there is only one user, and the >> prospect of having other drivers rewrite their page-table building code >> based on this helper in the near future is probably small. Perhaps more >> of interest to new drivers. I think what will happen otherwise is that >> during some future cleanup this will be pushed down to xe claiming it's >> the only user. >> >> I wonder whether it might be an idea to maintain a small document where >> driver writers can list suggestions for code that could be lifted to >> core drm and be reused by others. That way both reviewers and writers of >> other drivers can keep an eye on that document and use it to avoid >> duplicating code. The procedure would then be to lift it to core drm and >> fix up prefixes as soon as we have two or more users. >> >> Thoughts? > FWIW, when we originally wrote the GPU scheduler it was part of > amdgpu, but we consciously kept any AMD-isms out of it so it could be > lifted up to a core component when another user came along. Maybe > some comments in the top of those files to that effect to maintain the > separation. Sure. I'll do that. It sounds like we'll keep this in xe for now. Thanks, /Thomas > Alex > > >> Thomas >> >> >>>> *) If a drm helper, should we use a config option? >>> I am no fan of Kconfig things tbh. Maybe just include it in the vma >>> helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, >>> drm/sched, this one here, vm/vma helpers or whatever they will be and so >>> on? Kinda like we have modeset helpers. >>> >>> I'd definitely not go for a Kconfig per individual file, that's just >>> excessive. >>> -Daniel >>> >>>> For usage examples, see xe_pt.c >>>> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c >>>> >>>> Thanks, >>>> Thomas >>>> >>>> Thomas Hellström (1): >>>> drm: Add a gpu page-table walker helper >>>> >>>> drivers/gpu/drm/Makefile | 1 + >>>> drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ >>>> include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 321 insertions(+) >>>> create mode 100644 drivers/gpu/drm/drm_pt_walk.c >>>> create mode 100644 include/drm/drm_pt_walk.h >>>> >>>> -- >>>> 2.34.1 >>>>
Hi, Oded. On 2/26/23 19:56, Oded Gabbay wrote: > On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote: >> On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström >> <thomas.hellstrom@linux.intel.com> wrote: >>> Hi, Daniel, >>> >>> On 2/16/23 21:18, Daniel Vetter wrote: >>>> On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: >>>>> A slightly unusual cover letter for a single patch. >>>>> >>>>> The page table walker is currently used by the xe driver only, >>>>> but the code is generic so we can be good citizens and add it to drm >>>>> as a helper, for possible use by other drivers, >>>>> If so we can merge the commit when we merge the xe driver. >>>>> >>>>> The question raised here is >>>>> *) Should it be a generic drm helper or xe-specific with changed >>>>> prefixes? >>>> I think if there's some other drivers interested in using this, then this >>>> sounds like a good idea. Maybe more useful if it's also integrated into >>>> the vm/vma helpers that are being discussed as an optional part? >>>> >>>> Maybe some good old sales pitching here to convince people would be good. >>>> >>>> Maybe one of the new accel drivers is interested in this too? > Hi, > As the habanalabs driver is not really a new driver, I currently don't > see the benefit of moving > to this code. Our pgt code is quite mature and was tested extensively > in deployment in the > past couple of years. > > Nevertheless, I'll try to offer this code for any new/future driver > that will want to join accel. > > Stanislaw, I'm adding you here in case you missed this. Might be of an > interest to you. Thanks for taking a look. Yes, as also mentioned to Alex, I think we'll keep this in xe for now. /Thomas > > Thanks, > Oded > > - Oded > > > >>> Thanks for your thoughts on this. Yeah, I think it's a bit awkward to >>> push for having code generic when there is only one user, and the >>> prospect of having other drivers rewrite their page-table building code >>> based on this helper in the near future is probably small. Perhaps more >>> of interest to new drivers. I think what will happen otherwise is that >>> during some future cleanup this will be pushed down to xe claiming it's >>> the only user. >>> >>> I wonder whether it might be an idea to maintain a small document where >>> driver writers can list suggestions for code that could be lifted to >>> core drm and be reused by others. That way both reviewers and writers of >>> other drivers can keep an eye on that document and use it to avoid >>> duplicating code. The procedure would then be to lift it to core drm and >>> fix up prefixes as soon as we have two or more users. >>> >>> Thoughts? >> FWIW, when we originally wrote the GPU scheduler it was part of >> amdgpu, but we consciously kept any AMD-isms out of it so it could be >> lifted up to a core component when another user came along. Maybe >> some comments in the top of those files to that effect to maintain the >> separation. >> >> Alex >> >> >>> Thomas >>> >>> >>>>> *) If a drm helper, should we use a config option? >>>> I am no fan of Kconfig things tbh. Maybe just include it in the vma >>>> helpers, or perhaps we want to do a drm-accel-helpers with gem helpers, >>>> drm/sched, this one here, vm/vma helpers or whatever they will be and so >>>> on? Kinda like we have modeset helpers. >>>> >>>> I'd definitely not go for a Kconfig per individual file, that's just >>>> excessive. >>>> -Daniel >>>> >>>>> For usage examples, see xe_pt.c >>>>> https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_pt.c >>>>> >>>>> Thanks, >>>>> Thomas >>>>> >>>>> Thomas Hellström (1): >>>>> drm: Add a gpu page-table walker helper >>>>> >>>>> drivers/gpu/drm/Makefile | 1 + >>>>> drivers/gpu/drm/drm_pt_walk.c | 159 +++++++++++++++++++++++++++++++++ >>>>> include/drm/drm_pt_walk.h | 161 ++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 321 insertions(+) >>>>> create mode 100644 drivers/gpu/drm/drm_pt_walk.c >>>>> create mode 100644 include/drm/drm_pt_walk.h >>>>> >>>>> -- >>>>> 2.34.1 >>>>>
On Mon, Feb 27, 2023 at 09:09:14AM +0100, Thomas Hellström wrote: > Hi, Oded. > > On 2/26/23 19:56, Oded Gabbay wrote: > > On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher@gmail.com> wrote: > > > On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström > > > <thomas.hellstrom@linux.intel.com> wrote: > > > > Hi, Daniel, > > > > > > > > On 2/16/23 21:18, Daniel Vetter wrote: > > > > > On Thu, Feb 16, 2023 at 05:27:28PM +0100, Thomas Hellström wrote: > > > > > > A slightly unusual cover letter for a single patch. > > > > > > > > > > > > The page table walker is currently used by the xe driver only, > > > > > > but the code is generic so we can be good citizens and add it to drm > > > > > > as a helper, for possible use by other drivers, > > > > > > If so we can merge the commit when we merge the xe driver. > > > > > > > > > > > > The question raised here is > > > > > > *) Should it be a generic drm helper or xe-specific with changed > > > > > > prefixes? > > > > > I think if there's some other drivers interested in using this, then this > > > > > sounds like a good idea. Maybe more useful if it's also integrated into > > > > > the vm/vma helpers that are being discussed as an optional part? > > > > > > > > > > Maybe some good old sales pitching here to convince people would be good. > > > > > > > > > > Maybe one of the new accel drivers is interested in this too? > > Hi, > > As the habanalabs driver is not really a new driver, I currently don't > > see the benefit of moving > > to this code. Our pgt code is quite mature and was tested extensively > > in deployment in the > > past couple of years. > > > > Nevertheless, I'll try to offer this code for any new/future driver > > that will want to join accel. > > > > Stanislaw, I'm adding you here in case you missed this. Might be of an > > interest to you. Rewrite table walk will not give the ivpu driver much, perhaps one function would be smaller. Nothing that would justify the effort IMO. > Thanks for taking a look. Yes, as also mentioned to Alex, I think we'll keep > this in xe for now. Sounds good :-) Regards Stanislaw