On Thu, Feb 23, 2023 at 10:03 AM Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> 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 > >>
