Re: [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 06/01/2017 15:25, Chris Wilson wrote:
> Save a lot of characters by making the union anonymous, with the
> side-effect of ignoring unset bits when comparing views.

I'll just repeat for the record, since the earlier discussion was on the 
trybot list.

I think the anonymizing of the union should be a separate patch from 
storing the struct sizes in the enum. The latter is what enables the 
memcpy reduction trick, so the commit message is also incorrect in that 
respect it is not the anonymizing which enables it.

Other than that I have no complaints on this patch, apart it depends on 
the preceding struct partial packing which I think is possibly going to far.

Regards,

Tvrtko

> v2: Roll up the memcmps back into one.
> v3: And split again as Ville points out we can't trust the compiler.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 11 +++++------
>  drivers/gpu/drm/i915/i915_gem_gtt.c  |  7 ++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.h  | 16 ++++++++--------
>  drivers/gpu/drm/i915/i915_vma.c      |  9 ++++-----
>  drivers/gpu/drm/i915/i915_vma.h      | 16 ++++++++++------
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  6 files changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2c1631190a60..43233069ccc8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1841,13 +1841,12 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>  		if (i915_gem_object_is_tiled(obj))
>  			chunk = roundup(chunk, tile_row_pages(obj));
>
> -		memset(&view, 0, sizeof(view));
>  		view.type = I915_GGTT_VIEW_PARTIAL;
> -		view.params.partial.offset_size = rounddown(page_offset, chunk);
> -		view.params.partial.offset_size =
> -			(view.params.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
> +		view.partial.offset_size = rounddown(page_offset, chunk);
> +		view.partial.offset_size =
> +			(view.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
>  			(min_t(unsigned int, chunk,
> -			      vma_pages(area) - view.params.partial.offset_size) - 1);
> +			      vma_pages(area) - view.partial.offset_size) - 1);
>
>  		/* If the partial covers the entire object, just create a
>  		 * normal VMA.
> @@ -1882,7 +1881,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>
>  	/* Finally, remap it using the new GTT offset */
>  	ret = remap_io_mapping(area,
> -			       area->vm_start + intel_partial_get_offset(&vma->ggtt_view.params.partial),
> +			       area->vm_start + intel_partial_get_offset(&vma->ggtt_view.partial),
>  			       (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT,
>  			       min_t(u64, vma->size, area->vm_end - area->vm_start),
>  			       &ggtt->mappable);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 45ab54e71dc0..578de1d21b5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3499,13 +3499,13 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>  	if (!st)
>  		goto err_st_alloc;
>
> -	count = intel_partial_get_page_count(&view->params.partial);
> +	count = intel_partial_get_page_count(&view->partial);
>  	ret = sg_alloc_table(st, count, GFP_KERNEL);
>  	if (ret)
>  		goto err_sg_alloc;
>
>  	iter = i915_gem_object_get_sg(obj,
> -				      intel_partial_get_page_offset(&view->params.partial),
> +				      intel_partial_get_page_offset(&view->partial),
>  				      &offset);
>  	GEM_BUG_ON(!iter);
>
> @@ -3558,7 +3558,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  		vma->pages = vma->obj->mm.pages;
>  	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
>  		vma->pages =
> -			intel_rotate_fb_obj_pages(&vma->ggtt_view.params.rotated, vma->obj);
> +			intel_rotate_fb_obj_pages(&vma->ggtt_view.rotated,
> +						  vma->obj);
>  	else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL)
>  		vma->pages = intel_partial_pages(&vma->ggtt_view, vma->obj);
>  	else
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index e2b8e5e1ed88..1376a53bb9a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -142,12 +142,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 */
> @@ -181,13 +175,19 @@ 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),
> +};
> +
>  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;
> +	};
>  };
>
>  extern const struct i915_ggtt_view i915_ggtt_view_normal;
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 65770b7109c0..284273056ea2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -96,14 +96,13 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
>  		vma->ggtt_view = *view;
>  		if (view->type == I915_GGTT_VIEW_PARTIAL) {
>  			GEM_BUG_ON(range_overflows_t(u64,
> -						     intel_partial_get_offset(&view->params.partial),
> -						     intel_partial_get_size(&view->params.partial),
> +						     intel_partial_get_offset(&view->partial),
> +						     intel_partial_get_size(&view->partial),
>  						     obj->base.size));
> -			vma->size = intel_partial_get_size(&view->params.partial);
> +			vma->size = intel_partial_get_size(&view->partial);
>  			GEM_BUG_ON(vma->size >= obj->base.size);
>  		} else if (view->type == I915_GGTT_VIEW_ROTATED) {
> -			vma->size =
> -				intel_rotation_info_size(&view->params.rotated);
> +			vma->size = intel_rotation_info_size(&view->rotated);
>  			vma->size <<= PAGE_SHIFT;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index e3b2b3b1e056..6d936009f919 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -196,15 +196,19 @@ i915_vma_compare(struct i915_vma *vma,
>  	if (cmp)
>  		return cmp;
>
> +	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);
> +	BUILD_BUG_ON(offsetof(typeof(*view), rotated) !=
> +		     offsetof(typeof(*view), partial));
>
> -	return memcmp(&vma->ggtt_view.params,
> -		      &view->params,
> -		      sizeof(view->params));
> +	return memcmp(&vma->ggtt_view.partial, &view->partial, view->type);
>  }
>
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e2150a64860c..f5718f99d0d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2139,7 +2139,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>  {
>  	if (drm_rotation_90_or_270(rotation)) {
>  		*view = i915_ggtt_view_rotated;
> -		view->params.rotated = to_intel_framebuffer(fb)->rot_info;
> +		view->rotated = to_intel_framebuffer(fb)->rot_info;
>  	} else {
>  		*view = i915_ggtt_view_normal;
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx