On 02/18/2016 06:26 AM, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
