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

[Thomas Hellström]

Hi, Oded.

On 2/26/23 19:56, Oded Gabbay wrote:
> On Thu, Feb 23, 2023 at 8:50 PM Alex Deucher <alexdeucher at gmail.com> wrote:
>> 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?
> 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
>>>>>