[Intel-xe] XE TTM review

[Ohad Sharabi]

+ Daniel Vetter

From: Ohad Sharabi
Sent: Thursday, 2 November 2023 12:02
To: intel-xe at lists.freedesktop.org
Cc: matthew.brost at intel.com; rodrigo.vivi at intel.com; Francois Dugast <francois.dugast at intel.com>; Hellstrom, Thomas <thomas.hellstrom at intel.com>; Oded Gabbay
Subject: XE TTM review

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.  In try_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 on c (in index) to avoid out-of-bound access?
  2.  Maybe rename xe_bo_get_sg -> xe_bo_sg as get implied refcount?
  3.  In xe_bo_move we have trace_printk, shouldn’t we avoid such prints in production?
  4.  In xe_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.  In struct xe_bo, I don’t think preferred_mem_type is being used.
  6.  When initializing XA flags to xe->usm.asid_to_vm in the function xe_device_create you are using the flag 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 function xe_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: in xe_exec_ioctl can’t we use num_syncs as the iterator instead of i?
  9.  Looking at xe_ttm_vram_mgr_free_sgt it seems this function has no relation with TTM/manager. Maybe it belongs in another place?
  10. In xe_vm_create, when shifting xe->info.va_bits, wouldn’t it be better to use BIT_ULL?
  11. In xe_vma_op_execute, maybe get the VMA in each case and call once to __xe_vma_op_execute?
  12. In struct 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20231102/238b739c/attachment-0001.htm>