[Intel-xe] [PATCH 0/1] drm: Add a gpu page-table walker

[Alex Deucher]

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