<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body>
    Am 27.10.23 um 10:22 schrieb Boris Brezillon:<br>
    <blockquote type="cite" cite="mid:20231027102237.0cdb85af@collabora.com">
      <pre class="moz-quote-pre" wrap="">On Fri, 27 Oct 2023 09:44:13 +0200
Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> wrote:

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Am 27.10.23 um 09:39 schrieb Boris Brezillon:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">On Fri, 27 Oct 2023 09:35:01 +0200
Christian König<a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a>  wrote:
 
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">Am 27.10.23 um 09:32 schrieb Boris Brezillon:  
</pre>
            <blockquote type="cite">
              <pre class="moz-quote-pre" wrap="">On Fri, 27 Oct 2023 09:22:12 +0200
Christian König<a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a>  wrote:
    
</pre>
              <blockquote type="cite">
                <blockquote type="cite">
                  <pre class="moz-quote-pre" wrap="">+
+       /**
+        * @update_job_credits: Called once the scheduler is considering this
+        * job for execution.
+        *
+        * Drivers may use this to update the job's submission credits, which is
+        * useful to e.g. deduct the number of native fences which have been
+        * signaled meanwhile.
+        *
+        * The callback must either return the new number of submission credits
+        * for the given job, or zero if no update is required.
+        *
+        * This callback is optional.
+        */
+       u32 (*update_job_credits)(struct drm_sched_job *sched_job);  
</pre>
                </blockquote>
                <pre class="moz-quote-pre" wrap="">Why do we need an extra callback for this?

Just document that prepare_job() is allowed to reduce the number of
credits the job might need.
</pre>
              </blockquote>
              <pre class="moz-quote-pre" wrap="">->prepare_job() is called only once if the returned fence is NULL, but  
we need this credit-update to happen every time a job is considered for
execution by the scheduler.  
</pre>
            </blockquote>
            <pre class="moz-quote-pre" wrap="">But the job is only considered for execution once. How do you see that
this is called multiple times?  
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">Nope, it's not. If drm_sched_can_queue() returns false, the scheduler
will go look for another entity that has a job ready for execution, and
get back to this entity later, and test drm_sched_can_queue() again.
Basically, any time drm_sched_can_queue() is called, the job credits
update should happen, so we have an accurate view of how many credits
this job needs.  
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
Well, that is the handling which I already rejected because it creates 
unfairness between processes. When you consider the credits needed 
*before* scheduling jobs with a lower credit count are always preferred 
over jobs with a higher credit count.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
My bad, it doesn't pick another entity when an entity with a
ready job that doesn't fit the queue is found, it just bails out from
drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity
found). But we still want to update the job credits before checking if
the job fits or not (next time this entity is tested).</pre>
    </blockquote>
    <br>
    Why? I only see a few possibility here:<br>
    <br>
    1. You need to wait for submissions to the same scheduler to finish
    before the one you want to push to the ring.<br>
        This case can be avoided by trying to cast the dependency fences
    to drm_sched_fences and looking if they are already scheduled.<br>
    <br>
    2. You need to wait for submissions to a different scheduler
    instance and in this case you should probably return that as
    dependency instead.<br>
    <br>
    So to me it looks like when prepare_job() is called because it is
    selected as next job for submission you should already know how many
    credits it needs.<br>
    <br>
    <span style="white-space: pre-wrap">
</span>
    <blockquote type="cite" cite="mid:20231027102237.0cdb85af@collabora.com">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">What you can do is to look at the credits of a job *after* it was picked 
up for scheduling so that you can scheduler more jobs.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Sure, but then you might further delay your job if something made it
smaller (ie. native fences got signaled) between ->prepare_job() and
drm_sched_can_queue(). And any new drm_sched_can_queue() test would
just see the old credits value.

Out of curiosity, what are you worried about with this optional
->update_job_credits() call in the drm_sched_can_queue() path? Is the
if (sched->update_job_credits) overhead considered too high for drivers
that don't need it?</pre>
    </blockquote>
    <br>
    Since the dma_fences are also used for resource management the
    scheduler is vital for correct system operation.<br>
    <br>
    We had massively problems because people tried to over-optimize the
    dma_fence handling which lead to very hard to narrow down memory
    corruptions.<br>
    <br>
    So for every change you come up here you need to have a very very
    good justification. And just saving a bit size of your ring buffer
    is certainly not one of them.<br>
    <br>
    Regards,<br>
    Christian.<br>
  </body>
</html>