<div dir="auto"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 1, 2024, 03:37 Christian König <<a href="mailto:christian.koenig@amd.com">christian.koenig@amd.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u></u>

  
  <div>
    Am 01.08.24 um 08:53 schrieb Marek Olšák:<br>
    <blockquote type="cite">
      
      <div dir="auto">
        <div>
          <div class="gmail_quote">
            <div dir="ltr" class="gmail_attr">On Thu, Aug 1, 2024, 00:28
              Khatri, Sunil <<a href="mailto:sukhatri@amd.com" target="_blank" rel="noreferrer">sukhatri@amd.com</a>>
              wrote:<br>
            </div>
            <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
              On 8/1/2024 8:49 AM, Marek Olšák wrote:<br>
              >> +       /* Header is at index 0, followed by
              num_nops - 1 NOP packet's */<br>
              >> +       for (i = 1; i < num_nop; i++)<br>
              >> +               amdgpu_ring_write(ring,
              ring->funcs->nop);<br>
              > This loop should be removed. It's unnecessary CPU
              overhead and we<br>
              > should never get more than 0x3fff NOPs (maybe use
              BUG_ON). Leaving the<br>
              > whole packet body uninitialized is the fastest
              option.<br>
              That was the original intent to just move the WPTR for the
              no of nops <br>
              and tried too. Based on Christian inputs we should not let
              the nops packet<br>
              <br>
              as garbage or whatever was there originally as a
              threat/safety measure.</blockquote>
          </div>
        </div>
        <div dir="auto"><br>
        </div>
        <div dir="auto">It doesn't help safety. It can only be read by
          the GPU with kernel-level permissions.</div>
        <div dir="auto"><br>
        </div>
        <div dir="auto">Initializing the packet body is useless and adds
          CPU overhead, especially with the 256 NOPs or so that we use
          for no reason.</div>
      </div>
    </blockquote>
    <br>
    Not filling the remaining ring buffers with NOPs is a pretty clear
    NAK from my side. Leaving garbage in the ring buffer is not even
    remotely defensive.<br></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">What are you defending against? You know the ring is kernel-owned memory, right? </div><div dir="auto"><br></div><div dir="auto">Marek</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
    <br>
    What we can do is to optimize filling N DWs into the ring buffer
    without updating the WPTR each time.<br>
    <br>
    Regards,<br>
    Christian.<br>
    <br>
    <blockquote type="cite">
      <div dir="auto">
        <div dir="auto"><br>
        </div>
        <div dir="auto">Marek</div>
        <br>
      </div>
    </blockquote>
  </div>

</blockquote></div></div></div>