| Message ID | 1403803475-16337-13-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 26 Jun 2014 18:24:03 +0100 John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > Hardware sempahores require seqno values to be continuously incrementing. > However, the scheduler's reordering of batch buffers means that the seqno values > going through the hardware could be out of order. Thus semaphores can not be > used. > > On the other hand, the scheduler superceeds the need for hardware semaphores > anyway. Having one ring stall waiting for something to complete on another ring > is inefficient if that ring could be working on some other, independent task. > This is what the scheduler is meant to do - keep the hardware as busy as > possible by reordering batch buffers to avoid dependency stalls. > --- > drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++ > drivers/gpu/drm/i915/i915_scheduler.c | 9 +++++++++ > drivers/gpu/drm/i915/i915_scheduler.h | 1 + > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++++ > 4 files changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e2bfdda..748b13a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -33,6 +33,7 @@ > #include "i915_drv.h" > #include "i915_trace.h" > #include "intel_drv.h" > +#include "i915_scheduler.h" > > #include <linux/console.h> > #include <linux/module.h> > @@ -468,6 +469,14 @@ void intel_detect_pch(struct drm_device *dev) > > bool i915_semaphore_is_enabled(struct drm_device *dev) > { > + /* Hardware semaphores are not compatible with the scheduler due to the > + * seqno values being potentially out of order. However, semaphores are > + * also not required as the scheduler will handle interring dependencies > + * and try do so in a way that does not cause dead time on the hardware. > + */ > + if (i915_scheduler_is_enabled(dev)) > + return 0; > + > if (INTEL_INFO(dev)->gen < 6) > return false; > > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c > index e9aa566..d9c1879 100644 > --- a/drivers/gpu/drm/i915/i915_scheduler.c > +++ b/drivers/gpu/drm/i915/i915_scheduler.c > @@ -26,6 +26,15 @@ > #include "intel_drv.h" > #include "i915_scheduler.h" > > +bool i915_scheduler_is_enabled(struct drm_device *dev) > +{ > +#ifdef CONFIG_DRM_I915_SCHEDULER > + return true; > +#else > + return false; > +#endif > +} I think this should be: if (dev_priv->scheduler) return true; return false; instead? Otherwise looks fine.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e2bfdda..748b13a 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -33,6 +33,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "i915_scheduler.h" #include <linux/console.h> #include <linux/module.h> @@ -468,6 +469,14 @@ void intel_detect_pch(struct drm_device *dev) bool i915_semaphore_is_enabled(struct drm_device *dev) { + /* Hardware semaphores are not compatible with the scheduler due to the + * seqno values being potentially out of order. However, semaphores are + * also not required as the scheduler will handle interring dependencies + * and try do so in a way that does not cause dead time on the hardware. + */ + if (i915_scheduler_is_enabled(dev)) + return 0; + if (INTEL_INFO(dev)->gen < 6) return false; diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c index e9aa566..d9c1879 100644 --- a/drivers/gpu/drm/i915/i915_scheduler.c +++ b/drivers/gpu/drm/i915/i915_scheduler.c @@ -26,6 +26,15 @@ #include "intel_drv.h" #include "i915_scheduler.h" +bool i915_scheduler_is_enabled(struct drm_device *dev) +{ +#ifdef CONFIG_DRM_I915_SCHEDULER + return true; +#else + return false; +#endif +} + #ifdef CONFIG_DRM_I915_SCHEDULER int i915_scheduler_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h index 67260b7..4044b6e 100644 --- a/drivers/gpu/drm/i915/i915_scheduler.h +++ b/drivers/gpu/drm/i915/i915_scheduler.h @@ -25,6 +25,7 @@ #ifndef _I915_SCHEDULER_H_ #define _I915_SCHEDULER_H_ +bool i915_scheduler_is_enabled(struct drm_device *dev); int i915_scheduler_init(struct drm_device *dev); #ifdef CONFIG_DRM_I915_SCHEDULER diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index bad5db0..34d6d6e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -32,6 +32,7 @@ #include <drm/i915_drm.h> #include "i915_trace.h" #include "intel_drv.h" +#include "i915_scheduler.h" /* Early gen2 devices have a cacheline of just 32 bytes, using 64 is overkill, * but keeps the logic simple. Indeed, the whole purpose of this macro is just @@ -765,6 +766,9 @@ gen6_ring_sync(struct intel_engine_cs *waiter, u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id]; int ret; + /* Arithmetic on sequence numbers is unreliable with a scheduler. */ + BUG_ON(i915_scheduler_is_enabled(signaller->dev)); + /* Throughout all of the GEM code, seqno passed implies our current * seqno is >= the last seqno executed. However for hardware the * comparison is strictly greater than.
From: John Harrison <John.C.Harrison@Intel.com> Hardware sempahores require seqno values to be continuously incrementing. However, the scheduler's reordering of batch buffers means that the seqno values going through the hardware could be out of order. Thus semaphores can not be used. On the other hand, the scheduler superceeds the need for hardware semaphores anyway. Having one ring stall waiting for something to complete on another ring is inefficient if that ring could be working on some other, independent task. This is what the scheduler is meant to do - keep the hardware as busy as possible by reordering batch buffers to avoid dependency stalls. --- drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++ drivers/gpu/drm/i915/i915_scheduler.c | 9 +++++++++ drivers/gpu/drm/i915/i915_scheduler.h | 1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++++ 4 files changed, 23 insertions(+)