Re: [RFC 17/44] drm/i915: Prelude to splitting i915_gem_do_execbuffer in two

Jesse Barnes
2014-07-02T18:33:39Z

On Thu, 26 Jun 2014 18:24:08 +0100
John.C.Harrison@Intel.com wrote:

> From: John Harrison <John.C.Harrison@Intel.com>
>
> The scheduler decouples the submission of batch buffers to the driver with their
> submission to the hardware. This basically means splitting the execbuffer()
> function in half. This change rearranges some code ready for the split to occur.
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ec274ef..fda9187 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -32,6 +32,7 @@
> #include "i915_trace.h"
> #include "intel_drv.h"
> #include <linux/dma_remapping.h>
> +#include "i915_scheduler.h"
>
> #define __EXEC_OBJECT_HAS_PIN (1<<31)
> #define __EXEC_OBJECT_HAS_FENCE (1<<30)
> @@ -874,10 +875,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
> if (flush_domains & I915_GEM_DOMAIN_GTT)
> wmb();
>
> - /* Unconditionally invalidate gpu caches and ensure that we do flush
> - * any residual writes from the previous batch.
> - */
> - return intel_ring_invalidate_all_caches(ring);
> + return 0;
> }
>
> static bool
> @@ -1219,8 +1217,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> }
> }
>
> - intel_runtime_pm_get(dev_priv);
> -
> ret = i915_mutex_lock_interruptible(dev);
> if (ret)
> goto pre_mutex_err;
> @@ -1331,6 +1327,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> goto err;
>
> + i915_gem_execbuffer_move_to_active(&eb->vmas, ring);
> +
> + /* To be split into two functions here... */
> +
> + intel_runtime_pm_get(dev_priv);
> +
> + /* Unconditionally invalidate gpu caches and ensure that we do flush
> + * any residual writes from the previous batch.
> + */
> + ret = intel_ring_invalidate_all_caches(ring);
> + if (ret)
> + goto err;
> +
> + /* Switch to the correct context for the batch */
> ret = i915_switch_context(ring, ctx);
> if (ret)
> goto err;
> @@ -1381,7 +1391,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>
> trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
>
> - i915_gem_execbuffer_move_to_active(&eb->vmas, ring);
> i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
>
> err:

I'd like Chris to take a look too, but it looks safe afaict.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

--
Jesse Barnes, Intel Open Source Technology Center