Re: [PATCH v5 08/35] drm/i915: Disable hardware semaphores when GPU scheduler is enabled

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx