Re: [PATCH 3/6] drm/i915: Compact memcmp in i915_vma_compare()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/01/2017 21:51, Chris Wilson wrote:
> In preparation for the next patch to convert to using an anonymous union
> and leaving the excess bytes in the union uninitialised, we first need
> to make sure we do not compare using those uninitialised bytes. We also
> want to preserve the compactness of the code, avoiding a second call to
> memcmp or introducing a switch, so we take advantage of using the type
> as an encoded size (as well as a unique identifier for each type of view).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +++++++-------
>  drivers/gpu/drm/i915/i915_vma.h     | 15 +++++++++------
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 3187a260e6e1..36d85599ffc9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -145,12 +145,6 @@ typedef uint64_t gen8_ppgtt_pml4e_t;
>
>  struct sg_table;
>
> -enum i915_ggtt_view_type {
> -	I915_GGTT_VIEW_NORMAL = 0,
> -	I915_GGTT_VIEW_ROTATED,
> -	I915_GGTT_VIEW_PARTIAL,
> -};
> -
>  struct intel_rotation_info {
>  	struct intel_rotation_plane_info {
>  		/* tiles */
> @@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
>  	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>  }
>
> +enum i915_ggtt_view_type {
> +	I915_GGTT_VIEW_NORMAL = 0,
> +	I915_GGTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
> +	I915_GGTT_VIEW_PARTIAL = sizeof(struct intel_partial_info),
> +};

I just realized this will uglify the debugfs output. In the light of 
that I think we should leave the enum human readable and instead you 
could have a static map in i915_vma_compare. Hopefully code generation 
wouldn't be affected by it.

Regards,

Tvrtko

> +
>  struct i915_ggtt_view {
>  	enum i915_ggtt_view_type type;
> -
>  	union {
> +		/* Members need to contain no holes/padding */
>  		struct intel_partial_info partial;
>  		struct intel_rotation_info rotated;
>  	} params;
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index a969bbb65871..19f049cef9e3 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -199,15 +199,18 @@ i915_vma_compare(struct i915_vma *vma,
>  	if (cmp)
>  		return cmp;
>
> +	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL != 0);
> +	cmp = vma->ggtt_view.type;
>  	if (!view)
> -		return vma->ggtt_view.type;
> +		return cmp;
> +
> +	cmp -= view->type;
> +	if (cmp)
> +		return cmp;
>
> -	if (vma->ggtt_view.type != view->type)
> -		return vma->ggtt_view.type - view->type;
> +	BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED);
>
> -	return memcmp(&vma->ggtt_view.params,
> -		      &view->params,
> -		      sizeof(view->params));
> +	return memcmp(&vma->ggtt_view.params, &view->params, view->type);
>  }
>
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx