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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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
>>>>