From a996838af55a12632e64e2f1c0725a18ca3e586f Mon Sep 17 00:00:00 2001 From: Oded Gabbay <ogabbay@habana.ai> Date: Thu, 1 Jun 2023 15:32:09 +0300 Subject: [PATCH] more comments for 1.6 Signed-off-by: Oded Gabbay <ogabbay@habana.ai> --- cr.txt | 45 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/cr.txt b/cr.txt index 1a48e2ddfbdc..36a0eb804457 100644 --- a/cr.txt +++ b/cr.txt @@ -55,9 +55,6 @@ xe_mmio.h: - xe_mmio_wait32 reg & val arguments are not in the same order as intel_wait_for_register in i915. We are setting us up to fail when copying stuff from i915. -xe_bo.c: -- - xe_bo.h: - PPAT_CACHED_PDE is defined as 0. Is this correct or it should be BIT_ULL(0) ? If it is 0, code in gen8_pde_encode is confusing. @@ -81,11 +78,51 @@ xe_vm.c: -------- End of comments sent on 24/5 ------------- +-------- Start of comments sent on 1/6 ------------- + +xe_vm_types.h: +- Line 175: Either XE_VM_FLAGS_64K should be XE_VM_FLAG_64K or all other XE_VM_FLAG_* should be + XE_VM_FLAGS_* + I would also love to have some documentation on what each flag means. It's not trivial + to understand that from the code. + +xe_bo.h: +- Line 23: Where is the STOLEN define being used? I only found code that checks if that flag is + set + +xe_bo.c: +- __xe_bo_create_locked(): Are you sure the bo will always be destroyed (kfree) if a failure + happens during this function ? AFAICS, the bo can either be destroyed + by a direct call to xe_bo_free(), or through callback from ttm called + xe_ttm_bo_destroy(). However, xe_ttm_bo_destroy is assigned only when + calling ttm_bo_init_reserved(). However, you can exit __xe_bo_create_locked + in line 1009 (which is before ttm_bo_init_reserved). In addition, what + happens if the call to ttm_bo_init_reserved fails ? + +- Line 1096: after returning from __xe_bo_create_locked, if there was an error and the bo was + allocated in xe_bo_create_locked_range(), don't we need to make sure it is freed ? + +xe_vm.c: +- Line 1182: I have no idea what this comment means... Could you please explain what the migrate + vm code is doing there and WHY do we need it in case the migration flag is not set ? + +- number_gts: This local variable appears in 3 different functions in this file and each time is + calculated in a different manner :) Why not keep this number in the device data + structure and set it once during init ? (Maybe it also happens in other files, idk) + +- Line 1888: what is this asid ? Is it related to PASID when using ATS or is this something else ? + +xe_drm.h: +- Line 356: You need to add comment for the flags, in case there is some dependency or limitation. + e.g. afaics, you can't create two vms, one with DRM_XE_VM_CREATE_FAULT_MODE and + the other without. Either all vms are in fault mode or not. This should be written + here explicitly in the uapi file. +-------- End of comments sent on 1/6 ------------- -------- END OF CODE REVIEW ------------- -------- For Oded, please ignore ---------- TODO: - xe_driver_fops -- \ No newline at end of file +- preempt_rebind_work_func \ No newline at end of file -- GitLab