<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="en-IL" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> Hi,<o:p></o:p> <o:p> </o:p> 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:<o:p></o:p> <o:p> </o:p> <ol style="margin-top:0cm" start="1" type="1"> <li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">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?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">Maybe rename xe_bo_get_sg -> xe_bo_sg as get implied refcount?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">In xe_bo_move we have trace_printk, shouldn’t we avoid such prints in production?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">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?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">In struct xe_bo, I don’t think preferred_mem_type is being used.<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">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?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">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?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">Minor one: in xe_exec_ioctl can’t we use num_syncs as the iterator instead of i?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">Looking at xe_ttm_vram_mgr_free_sgt it seems this function has no relation with TTM/manager. Maybe it belongs in another place?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">In xe_vm_create, when shifting xe->info.va_bits, wouldn’t it be better to use BIT_ULL? <o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">In xe_vma_op_execute, maybe get the VMA in each case and call once to __xe_vma_op_execute?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">In struct xe_vm, maybe add doc to the flags (XE_VM_FLAG_64K et. al)?<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l0 level1 lfo1">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). <o:p></o:p></li></ol> <o:p> </o:p> Regards,<o:p></o:p> Ohad<o:p></o:p> </div> </body> </html>