Hi,
On 2/23/23 19:50, Alex Deucher wrote:
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.
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
