[Intel-xe] XE TTM review

[Hellstrom, Thomas]

Hi, Ohad

On Thu, 2023-11-02 at 10:01 +0000, Ohad Sharabi wrote:
> Hi,
>  
> I did the TTM-related review for the upstreamed XE driver.
> Below I’ll summarize comments/questions I had, please note that some
> comments are on code not directly related but within the same flow of
> TTM:
>  
>    1. Intry_add_system (or the other variants), I know that for now
> we’re good regarding space allocated in places, but isn’t it worth to
> having some assertion onc (in index) to avoid out-of-bound access?
>    2. Maybe renamexe_bo_get_sg -> xe_bo_sg as get implied refcount?
>    3. Inxe_bo_move we have trace_printk, shouldn’t we avoid such
> prints in production?
>    4. Inxe_bo_restore_kernel, if the restore fails, is it OK to leave
> BO in the present list? If so, is it because the resume failure will
> handle it?
>    5. Instruct xe_bo, I don’t think preferred_mem_type is being used.
>    6. When initializing XA flags toxe->usm.asid_to_vm in the function
> xe_device_create you are using theflag XA_FLAGS_ALLOC1. Later this XA
> is used for cyclic allocation. But the Doc says that for Cyc alloc
> one shouldn’t use ALLOC1. Am I missing something? Is it still OK in
> this case?
>    7. Also, for the same XA, in the functionxe_vm_create_ioctl, note
> that, according to the documentation return value of 1 isn’t a
> failure but merely an indication of a wrap. Are you prohibiting wrap
> on purpose?
>    8. Minor one: inxe_exec_ioctl can’t we use num_syncs as the
> iterator instead of i?
>    9. Looking atxe_ttm_vram_mgr_free_sgt it seems this function has
> no relation with TTM/manager. Maybe it belongs in another place?
>   10. Inxe_vm_create, when shifting xe->info.va_bits, wouldn’t it be
> better to useBIT_ULL? 
>   11. Inxe_vma_op_execute, maybe get the VMA in each case and call
> once to __xe_vma_op_execute?
>   12. Instruct xe_vm, maybe add doc to the flags (XE_VM_FLAG_64K et.
> al)?
>   13. Side note: I know that it may not be the scope of the review,
> but I couldn’t ignore the fact that the XE code is using a lot of
> inverted logic (e.g., if (!cond)… else). 
>  
> Regards,
> Ohad

I'll take a look at these and get back.

/Thomas