Re: [Intel-xe] [PATCH v2 16/31] drm/xe: Port Xe to GPUVA

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

On 5/11/23 04:41, Matthew Brost wrote:
> On Tue, May 09, 2023 at 03:52:24PM +0200, Thomas Hellström wrote:
>> Hi, Matthew,
>>
>> On 5/2/23 02:17, Matthew Brost wrote:
>>> Rather than open coding VM binds and VMA tracking, use the GPUVA
>>> library. GPUVA provides a common infrastructure for VM binds to use mmap
>>> / munmap semantics and support for VK sparse bindings.
>>>
>>> The concepts are:
>>>
>>> 1) xe_vm inherits from drm_gpuva_manager
>>> 2) xe_vma inherits from drm_gpuva
>>> 3) xe_vma_op inherits from drm_gpuva_op
>>> 4) VM bind operations (MAP, UNMAP, PREFETCH, UNMAP_ALL) call into the
>>> GPUVA code to generate an VMA operations list which is parsed, commited,
>>> and executed.
>>>
>>> v2 (CI): Add break after default in case statement.
>>> v3: Rebase
>>> v4: Fix some error handling
>>>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> Before embarking on a second review of this code it would really be
>> beneficial if you could address some comments from the first review. In
>> particular splitting this huge patch up if possible (and I also think that
>> removing the async worker *before* this patch if at all possible would
>> really ease the review both for me and potential upcoming reviewers).
>>
> My bad that I missed your comments on the list, yes I will address your
> comments in the respin, expect it by Mondayish.

NP, basically this was a comment saying I'd rather wait for a respin 
before tackling this again :).

>
> Removing the async worker first doesn't make a ton of sense as GPUVA
> makes the error handling a lot easier plus that basically means a
> complete rewrite.

OK, yes I guess from a reviewer's point of view the other way around 
would be easier, but if you keep the ordering please split into separate 
patch series for GPUVA and async removal, because invasive changes in 
code that is later removed in the same series is typically not well 
received.

Thanks,

Thomas


>
> Matt
>
>> Thanks,
>>
>> Thomas
>>
>>