1. 17 May, 2019 6 commits
    • Chris Wilson's avatar
      drm/i915: Downgrade NEWCLIENT to non-preemptive · 68fc728b
      Chris Wilson authored
      Commit 1413b2bc ("drm/i915: Trim NEWCLIENT boosting") had the
      intended consequence of not allowing a sequence of work that merely
      crossed into a new engine the privilege to be promoted to NEWCLIENT
      status. It also had the unintended consequence of actually making
      NEWCLIENT effective on heavily oversubscribed transcode machines and
      impacting upon their throughput.
      
      If we consider a client packet composed of (rcsA, rcsB, vcs) and 30 of
      those clients, using the NEWCLIENT boost that will be scheduled as
      
      	rcsA x 30, (rcsB, vcs) x 30
      
      where as before it would have been
      
      	(rcsA, rcsB, vcs) x 30
      
      That is with NEWCLIENT only boosting the first request of each client,
      we would execute all rcsA requests prior to running on the vcs engines;
      acruing a lot of dead time as compared to the previous case where the
      vcs engine would be started in parallel to processing the second client.
      
      The previous patch has the effect of delaying submission until it is
      required by a third party (either the user with an explicit wait, or by
      another client/engine). We reduce the NEWCLIENT bump to a mere WAIT,
      which has the effect of removing its preemptive grant and reducing it to
      the same level as any other user interaction -- that it will not be
      promoted above the interengine dependencies, and so preventing NEWCLIENTS
      from starving other engines. This a large nerf to the rrul properties of
      the current NEWCLIENT, but it still does give prioritised submission to
      new requests from light workloads.
      
      References: b16c7651 ("drm/i915: Priority boost for new clients")
      Fixes: 1413b2bc ("drm/i915: Trim NEWCLIENT boosting") # customer impact
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
      Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
      Cc: Dmitry Ermilov <dmitry.ermilov@intel.com>
      Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190515130052.4475-4-chris@chris-wilson.co.uk
      68fc728b
    • Chris Wilson's avatar
      drm/i915: Bump signaler priority on adding a waiter · 6e7eb7a8
      Chris Wilson authored
      The handling of the no-preemption priority level imposes the restriction
      that we need to maintain the implied ordering even though preemption is
      disabled. Otherwise we may end up with an AB-BA deadlock across multiple
      engine due to a real preemption event reordering the no-preemption
      WAITs. To resolve this issue we currently promote all requests to WAIT
      on unsubmission, however this interferes with the timeslicing
      requirement that we do not apply any implicit promotion that will defeat
      the round-robin timeslice list. (If we automatically promote the active
      request it will go back to the head of the queue and not the tail!)
      
      So we need implicit promotion to prevent reordering around semaphores
      where we are not allowed to preempt, and we must avoid implicit
      promotion on unsubmission. So instead of at unsubmit, if we apply that
      implicit promotion on adding the dependency, we avoid the semaphore
      deadlock and we also reduce the gains made by the promotion for user
      space waiting. Furthermore, by keeping the earlier dependencies at a
      higher level, we reduce the search space for timeslicing without
      altering runtime scheduling too badly (no dependencies at all will be
      assigned a higher priority for rrul).
      
      v2: Limit the bump to external edges (as originally intended) i.e.
      between contexts and out to the user.
      
      Testcase: igt/gem_concurrent_blit
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190515130052.4475-3-chris@chris-wilson.co.uk
      6e7eb7a8
    • Chris Wilson's avatar
      drm/i915/hdcp: Use both bits for device_count · af461ff3
      Chris Wilson authored
      Smatch spotted:
      drivers/gpu/drm/i915//intel_hdcp.c:1406 hdcp2_authenticate_repeater_topology() warn: should this be a bitwise op?
      
      and indeed looks to be suspect that we do need to use a bitwise or to
      combine the two register fields into one counter.
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Ramalingam C <ramalingam.c@intel.com>
      Reviewed-by: default avatarRamalingam C <ramalingam.c@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190517102225.3069-3-chris@chris-wilson.co.uk
      af461ff3
    • Chris Wilson's avatar
      drm/i915/dp: Initialise locals for static analysis · 96ac0813
      Chris Wilson authored
      Just to squelch an smatch warning that doesn't see the with_() being
      taken unconditionally:
      drivers/gpu/drm/i915//intel_dp.c:230 intel_dp_get_fia_supported_lane_count() error: uninitialized symbol 'lane_info'.
      drivers/gpu/drm/i915//intel_dp.c:5338 intel_digital_port_connected() error: uninitialized symbol 'is_connected'.
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Imre Deak <imre.deak@intel.com>
      Reviewed-by: default avatarImre Deak <imre.deak@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190517102225.3069-2-chris@chris-wilson.co.uk
      96ac0813
    • Chris Wilson's avatar
      drm/i915: Truly bump ready tasks ahead of busywaits · 17db337f
      Chris Wilson authored
      In commit b7404c7e ("drm/i915: Bump ready tasks ahead of
      busywaits"), I tried cutting a corner in order to not install a signal
      for each of our dependencies, and only listened to requests on which we
      were intending to busywait. The compromise that was made was that
      instead of then being able to promote the request with a full
      NOSEMAPHORE like its non-busywaiting brethren, as we had not ensured we
      had cleared the semaphore chain, we settled for only using the NEWCLIENT
      boost. With an over saturated system with multiple NEWCLIENTS in flight
      at any time, this was found to be an inadequate promotion and left us
      with a much poorer scheduling order than prior to using semaphores.
      
      The outcome of this patch, is that all requests have NOSEMAPHORE
      priority when they have no dependencies and are ready to run and not
      busywait, restoring the pre-semaphore ordering on saturated systems.
      
      We can demonstrate the effect of poor scheduling order by oversaturating
      the system using gem_wsim on a system with multiple vcs engines
      (i.e running the same workloads across more clients than required for
      peak throughput, e.g. media_load_balance_17i7.wsim -c4 -b context):
      
      x v5.1 (normalized)
      + tip
      * fix
      +------------------------------------------------------------------------+
      |                                                                    x   |
      |                                                                    x   |
      |                                                                    x   |
      |                                                                    x   |
      |                                                                   %x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %%x   |
      |                                                                  %#x   |
      |                                                                  %#x   |
      |                                                                  %#x   |
      |                                                                  %#x   |
      |                                                                  %#x   |
      |         +                                                        %#xx  |
      |         +                                                        %#xx  |
      |         +                                                       %%#xx  |
      |         +                                                       %%#xx  |
      |         +                                                       %%#xx  |
      |         +                                                       %%#xx  |
      |         +                                                       %%##x  |
      |         +++                                                     %%##x  |
      |         +++                                                     %%##x  |
      |         +++                                                     %%##x  |
      |        ++++                                                     %%##x  |
      |        ++++                                                     %%##x  |
      |        ++++                                                     %%##xx |
      |        ++++                                                     %###xx |
      |        ++++                                                     %###xx |
      |        ++++                                                     %###xx |
      |        ++++                                                     %###xx |
      |        ++++ +                                                   %#O#xx |
      |        ++++ +                                                   %#O#xx |
      |        ++++++ +                                                 %#O#xx |
      |       ++++++++++                                                %OOOxxx|
      |       ++++++++++       +                                       %#OOO#xx|
      |     + ++++++++++++ ++ +++++    +                        ++    @@OOOO#xx|
      |                                                                   |A_| |
      ||__________M_______A____________________|                               |
      |                                                                 |A_|   |
      +------------------------------------------------------------------------+
          N           Min           Max        Median           Avg        Stddev
      x 120       0.99456       1.00628      0.999985     1.0001545  0.0024387139
      + 120      0.873021       1.00037      0.884134    0.90148752   0.039190862
      Difference at 99.5% confidence
      	-0.098667 +/- 0.0110762
      	-9.86517% +/- 1.10745%
      	(Student's t, pooled s = 0.0277657)
      % 120      0.990207       1.00165     0.9970265    0.99699748     0.0021024
      Difference at 99.5% confidence
      	-0.003157 +/- 0.000908245
      	-0.315651% +/- 0.0908105%
      	(Student's t, pooled s = 0.00227678)
      
      Fixes: b7404c7e ("drm/i915: Bump ready tasks ahead of busywaits")
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
      Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
      Cc: Dmitry Ermilov <dmitry.ermilov@intel.com>
      Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190515130052.4475-2-chris@chris-wilson.co.uk
      17db337f
    • Chris Wilson's avatar
      drm/i915: Mark semaphores as complete on unsubmit out if payload was started · dba5a7f3
      Chris Wilson authored
      Avoid charging us for the presumed busywait if the request was preempted
      after successfully using semaphores to reduce inter-engine latency.
      
      v2: Bump the priority to reflect the lack of semaphores now required.
      
      References: ca6e56f6 ("drm/i915: Disable semaphore busywaits on saturated systems")
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
      Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190515130052.4475-1-chris@chris-wilson.co.uk
      dba5a7f3
  2. 14 May, 2019 16 commits
  3. 13 May, 2019 4 commits
  4. 09 May, 2019 9 commits
  5. 08 May, 2019 3 commits
    • Chris Wilson's avatar
      drm/i915: Seal races between async GPU cancellation, retirement and signaling · 0152b3b3
      Chris Wilson authored
      Currently there is an underlying assumption that i915_request_unsubmit()
      is synchronous wrt the GPU -- that is the request is no longer in flight
      as we remove it. In the near future that may change, and this may upset
      our signaling as we can process an interrupt for that request while it
      is no longer in flight.
      
      CPU0					CPU1
      intel_engine_breadcrumbs_irq
      (queue request completion)
      					i915_request_cancel_signaling
      ...					...
      					i915_request_enable_signaling
      dma_fence_signal
      
      Hence in the time it took us to drop the lock to signal the request, a
      preemption event may have occurred and re-queued the request. In the
      process, that request would have seen I915_FENCE_FLAG_SIGNAL clear and
      so reused the rq->signal_link that was in use on CPU0, leading to bad
      pointer chasing in intel_engine_breadcrumbs_irq.
      
      A related issue was that if someone started listening for a signal on a
      completed but no longer in-flight request, we missed the opportunity to
      immediately signal that request.
      
      Furthermore, as intel_contexts may be immediately released during
      request retirement, in order to be entirely sure that
      intel_engine_breadcrumbs_irq may no longer dereference the intel_context
      (ce->signals and ce->signal_link), we must wait for irq spinlock.
      
      In order to prevent the race, we use a bit in the fence.flags to signal
      the transfer onto the signal list inside intel_engine_breadcrumbs_irq.
      For simplicity, we use the DMA_FENCE_FLAG_SIGNALED_BIT as it then
      quickly signals to any outside observer that the fence is indeed signaled.
      
      v2: Sketch out potential dma-fence API for manual signaling
      v3: And the test_and_set_bit()
      
      Fixes: 52c0fdb2 ("drm/i915: Replace global breadcrumbs with per-context interrupt tracking")
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
      Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190508112452.18942-1-chris@chris-wilson.co.uk
      0152b3b3
    • Chris Wilson's avatar
      drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD · 519a0194
      Chris Wilson authored
      After realising we need to sample RING_START to detect context switches
      from preemption events that do not allow for the seqno to advance, we
      can also realise that the seqno itself is just a distance along the ring
      and so can be replaced by sampling RING_HEAD.
      
      v2: Bonus comment for the mystery separate CS_STALL before MI_USER_INTERRUPT
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
      Reviewed-by: default avatarMika Kuoppala <mika.kuoppala@linux.intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190508080704.24223-1-chris@chris-wilson.co.uk
      519a0194
    • Chris Wilson's avatar
      drm/i915: Reboot CI if forcewake fails · 18ecc6c5
      Chris Wilson authored
      If the HW fails to ack a change in forcewake status, the machine is as
      good as dead -- it may recover, but in reality it missed the mmio
      updates and is now in a very inconsistent state. If it happens, we can't
      trust the CI results (or at least the fails may be genuine but due to
      the HW being dead and not the actual test!) so reboot the machine (CI
      checks for a kernel taint in between each test and reboots if the
      machine is tainted).
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
      Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
      Reviewed-by: default avatarMika Kuoppala <mika.kuoppala@linux.intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20190508115245.27790-1-chris@chris-wilson.co.uk
      18ecc6c5
  6. 07 May, 2019 2 commits