<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><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
mso-ligatures:standardcontextual;
mso-fareast-language:EN-US;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0cm;
margin-right:0cm;
margin-bottom:0cm;
margin-left:36.0pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
mso-ligatures:standardcontextual;
mso-fareast-language:EN-US;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:140732835;
mso-list-template-ids:-724284428;}
@list l1
{mso-list-id:2073506108;
mso-list-type:hybrid;
mso-list-template-ids:1821246718 536870927 536870937 536870939 536870927 536870937 536870939 536870927 536870937 536870939;}
@list l1:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l1:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="en-IL" link="#0563C1" vlink="#954F72" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US">+ Daniel Vetter<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="en-IL"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="mso-ligatures:none;mso-fareast-language:#2000">From:</span></b><span lang="EN-US" style="mso-ligatures:none;mso-fareast-language:#2000"> Ohad Sharabi
<br>
<b>Sent:</b> Thursday, 2 November 2023 12:02<br>
<b>To:</b> intel-xe@lists.freedesktop.org<br>
<b>Cc:</b> matthew.brost@intel.com; rodrigo.vivi@intel.com; Francois Dugast <francois.dugast@intel.com>; Hellstrom, Thomas <thomas.hellstrom@intel.com>; Oded Gabbay<br>
<b>Subject:</b> XE TTM review<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span lang="EN-US">Hi,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">I did the TTM-related review for the upstreamed XE driver.<br>
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></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<ol style="margin-top:0cm" start="1" type="1">
<li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>try_add_system</i> (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
<i>c</i> (in index) to avoid out-of-bound access?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">Maybe rename
<i>xe_bo_get_sg</i> -> <i>xe_bo_sg</i> as get implied refcount?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>xe_bo_move</i> we have <i>trace_printk</i>, shouldn’t we avoid such prints in production?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>xe_bo_restore_kernel</i>, 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></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>struct xe_bo</i>, I don’t think <i>preferred_mem_type</i> is being used.<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">When initializing XA flags to
<i>xe->usm.asid_to_vm</i> in the function <i>xe_device_create</i> you are using the
<i>flag XA_FLAGS_ALLOC1</i>. 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></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">Also, for the same XA, in the function
<i>xe_vm_create_ioctl</i>, 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></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">Minor one: in
<i>xe_exec_ioctl</i> can’t we use <i>num_syncs</i> as the iterator instead of <i>
i</i>?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">Looking at
<i>xe_ttm_vram_mgr_free_sgt</i> it seems this function has no relation with TTM/manager. Maybe it belongs in another place?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>xe_vm_create</i>, when shifting <i>xe->info.va_bits</i>, wouldn’t it be better to use
<i>BIT_ULL</i>? <o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>xe_vma_op_execute</i>, maybe get the VMA in each case and call once to <i>__xe_vma_op_execute</i>?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">In
<i>struct xe_vm</i>, maybe add doc to the flags (<i>XE_VM_FLAG_64K</i> et. al)?<o:p></o:p></span></li><li class="MsoListParagraph" style="margin-left:0cm;mso-list:l1 level1 lfo3"><span lang="EN-US">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></span></li></ol>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Ohad<o:p></o:p></span></p>
</div>
</body>
</html>