| Message ID | 1455805644-6450-9-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/18/2016 06:26 AM, 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. > > v4: Downgraded a BUG_ON to WARN_ON as the latter is preferred. > > v5: Squashed the i915_scheduler.c portions down into the 'start of > scheduler' patch. [Joonas Lahtinen] > > For: VIZ-1587 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 975af35..5760a17 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -34,6 +34,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> > @@ -517,6 +518,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 false; > + > if (INTEL_INFO(dev)->gen < 6) > return false; > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 9d4f19d..ca7b8af 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -33,6 +33,7 @@ > #include <drm/i915_drm.h> > #include "i915_trace.h" > #include "intel_drv.h" > +#include "i915_scheduler.h" > > int __intel_ring_space(int head, int tail, int size) > { > @@ -1400,6 +1401,9 @@ gen6_ring_sync(struct drm_i915_gem_request *waiter_req, > u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id]; > int ret; > > + /* Arithmetic on sequence numbers is unreliable with a scheduler. */ > + WARN_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. > I'd rather get rid of this altogether, but I guess we'll need it for the older gens. Another option would be to make the sync_to hook NULL in the scheduler case, though I guess the failure mode is less desirable there. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 975af35..5760a17 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -34,6 +34,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> @@ -517,6 +518,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 false; + if (INTEL_INFO(dev)->gen < 6) return false; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 9d4f19d..ca7b8af 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -33,6 +33,7 @@ #include <drm/i915_drm.h> #include "i915_trace.h" #include "intel_drv.h" +#include "i915_scheduler.h" int __intel_ring_space(int head, int tail, int size) { @@ -1400,6 +1401,9 @@ gen6_ring_sync(struct drm_i915_gem_request *waiter_req, u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id]; int ret; + /* Arithmetic on sequence numbers is unreliable with a scheduler. */ + WARN_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.