[Intel-gfx] [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Daniel Vetter]

On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
> Precursor for fix to secure batch execution. We will need to be able to
> retrieve the batch VMA (as well as the batch itself) from the eb list,
> so this patch extracts that part of eb_get_batch() into a separate
> function, and moves both parts to a more logical place in the file, near
> where the eb list is created.
> 
> Also, it may not be obvious, but the current execbuffer2 ioctl interface
> requires that the buffer object containing the batch-to-be-executed be
> the LAST entry in the exec2_list[] array (I expected it to be the first!).
> 
> To clarify this, we can replace the rather obscure construct
> 	"list_entry(eb->vmas.prev, ...)"
> in the old version of eb_get_batch() with the equivalent but more explicit
> 	"list_last_entry(&eb->vmas,...)"
> in the new eb_get_batch_vma() and of course add an explanatory comment.
> 
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>

I have no context on the secure batch fix you're talking about, but this
here makes sense as an independent cleanup.

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 608fdc4..eea8b1f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -186,6 +186,35 @@ struct eb_vmas {
>  	return ret;
>  }
>  
> +static inline struct i915_vma *
> +eb_get_batch_vma(struct eb_vmas *eb)
> +{
> +	/* The batch is always the LAST item in the VMA list */
> +	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
> +
> +	return vma;
> +}
> +
> +static struct drm_i915_gem_object *
> +eb_get_batch(struct eb_vmas *eb)
> +{
> +	struct i915_vma *vma = eb_get_batch_vma(eb);
> +
> +	/*
> +	 * SNA is doing fancy tricks with compressing batch buffers, which leads
> +	 * to negative relocation deltas. Usually that works out ok since the
> +	 * relocate address is still positive, except when the batch is placed
> +	 * very low in the GTT. Ensure this doesn't happen.
> +	 *
> +	 * Note that actual hangs have only been observed on gen7, but for
> +	 * paranoia do it everywhere.
> +	 */
> +	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
> +		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
> +
> +	return vma->obj;
> +}
> +
>  static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
>  {
>  	if (eb->and < 0) {
> @@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
>  	return file_priv->bsd_ring;
>  }
>  
> -static struct drm_i915_gem_object *
> -eb_get_batch(struct eb_vmas *eb)
> -{
> -	struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list);
> -
> -	/*
> -	 * SNA is doing fancy tricks with compressing batch buffers, which leads
> -	 * to negative relocation deltas. Usually that works out ok since the
> -	 * relocate address is still positive, except when the batch is placed
> -	 * very low in the GTT. Ensure this doesn't happen.
> -	 *
> -	 * Note that actual hangs have only been observed on gen7, but for
> -	 * paranoia do it everywhere.
> -	 */
> -	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
> -		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
> -
> -	return vma->obj;
> -}
> -
>  #define I915_USER_RINGS (4)
>  
>  static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch