Re: [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union

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

On 13/01/2017 10:33, Chris Wilson wrote:
> Reading the ggtt_views is much more pleasant without the extra
> characters from specifying the union (i.e. ggtt_view.partial rather than
> ggtt_view.params.partial). To make this work inside i915_vma_compare()
> with only a single memcmp requires us to ensure that there are no
> uninitialised bytes within each branch of the union (we make sure the
> structs are packed) and we need to store the size of each branch.
>
> v4: Rewrite changelog and add comments explaining the assert.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 20 ++++++++++----------
>  drivers/gpu/drm/i915/i915_gem.c      |  8 ++++----
>  drivers/gpu/drm/i915/i915_gem_gtt.c  |  9 ++++-----
>  drivers/gpu/drm/i915/i915_gem_gtt.h  |  2 +-
>  drivers/gpu/drm/i915/i915_vma.c      |  9 ++++-----
>  drivers/gpu/drm/i915/i915_vma.h      |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  7 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e367f06f5883..da13c4c3aa6b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -167,20 +167,20 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>
>  			case I915_GGTT_VIEW_PARTIAL:
>  				seq_printf(m, ", partial [%08llx+%x]",
> -					   vma->ggtt_view.params.partial.offset << PAGE_SHIFT,
> -					   vma->ggtt_view.params.partial.size << PAGE_SHIFT);
> +					   vma->ggtt_view.partial.offset << PAGE_SHIFT,
> +					   vma->ggtt_view.partial.size << PAGE_SHIFT);
>  				break;
>
>  			case I915_GGTT_VIEW_ROTATED:
>  				seq_printf(m, ", rotated [(%ux%u, stride=%u, offset=%u), (%ux%u, stride=%u, offset=%u)]",
> -					   vma->ggtt_view.params.rotated.plane[0].width,
> -					   vma->ggtt_view.params.rotated.plane[0].height,
> -					   vma->ggtt_view.params.rotated.plane[0].stride,
> -					   vma->ggtt_view.params.rotated.plane[0].offset,
> -					   vma->ggtt_view.params.rotated.plane[1].width,
> -					   vma->ggtt_view.params.rotated.plane[1].height,
> -					   vma->ggtt_view.params.rotated.plane[1].stride,
> -					   vma->ggtt_view.params.rotated.plane[1].offset);
> +					   vma->ggtt_view.rotated.plane[0].width,
> +					   vma->ggtt_view.rotated.plane[0].height,
> +					   vma->ggtt_view.rotated.plane[0].stride,
> +					   vma->ggtt_view.rotated.plane[0].offset,
> +					   vma->ggtt_view.rotated.plane[1].width,
> +					   vma->ggtt_view.rotated.plane[1].height,
> +					   vma->ggtt_view.rotated.plane[1].stride,
> +					   vma->ggtt_view.rotated.plane[1].offset);
>  				break;
>
>  			default:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f034d8d2dd4c..d8622fd23f5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1760,10 +1760,10 @@ compute_partial_view(struct drm_i915_gem_object *obj,
>  		chunk = roundup(chunk, tile_row_pages(obj));
>
>  	view.type = I915_GGTT_VIEW_PARTIAL;
> -	view.params.partial.offset = rounddown(page_offset, chunk);
> -	view.params.partial.size =
> +	view.partial.offset = rounddown(page_offset, chunk);
> +	view.partial.size =
>  		min_t(unsigned int, chunk,
> -		      (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset);
> +		      (obj->base.size >> PAGE_SHIFT) - view.partial.offset);
>
>  	/* If the partial covers the entire object, just create a normal VMA. */
>  	if (chunk >= obj->base.size >> PAGE_SHIFT)
> @@ -1879,7 +1879,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 + (vma->ggtt_view.params.partial.offset << PAGE_SHIFT),
> +			       area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT),
>  			       (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 6d2ff20ec973..06cfd6951a5e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3490,7 +3490,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>  {
>  	struct sg_table *st;
>  	struct scatterlist *sg, *iter;
> -	unsigned int count = view->params.partial.size;
> +	unsigned int count = view->partial.size;
>  	unsigned int offset;
>  	int ret = -ENOMEM;
>
> @@ -3502,9 +3502,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>  	if (ret)
>  		goto err_sg_alloc;
>
> -	iter = i915_gem_object_get_sg(obj,
> -				      view->params.partial.offset,
> -				      &offset);
> +	iter = i915_gem_object_get_sg(obj, view->partial.offset, &offset);
>  	GEM_BUG_ON(!iter);
>
>  	sg = st->sgl;
> @@ -3556,7 +3554,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 20c03c30ce4e..560fd8ddaf2c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -179,7 +179,7 @@ struct i915_ggtt_view {
>  		/* 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 b74eeb73ae41..e6c339b0ea37 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -97,15 +97,14 @@ __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,
> -						     view->params.partial.offset,
> -						     view->params.partial.size,
> +						     view->partial.offset,
> +						     view->partial.size,
>  						     obj->base.size >> PAGE_SHIFT));
> -			vma->size = view->params.partial.size;
> +			vma->size = view->partial.size;
>  			vma->size <<= PAGE_SHIFT;
>  			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 c803afa2d619..c4a2294ee42d 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -209,13 +209,20 @@ i915_vma_compare(struct i915_vma *vma,
>  		return cmp;
>
>  	BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED);
> +	BUILD_BUG_ON(offsetof(typeof(*view), rotated) !=
> +		     offsetof(typeof(*view), partial));
>
>  	/* ggtt_view.type also encodes its size so that we both distinguish
>  	 * different views using it as a "type" and also use a compact (no
>  	 * accessing of uninitialised padding bytes) memcmp without storing
>  	 * an extra parameter or adding more code.
> +	 *
> +	 * To ensure that the memcmp is valid for all branches of the union,
> +	 * even though the code looks like it is just comparing one branch,
> +	 * we assert above that all branches have the same address, and that
> +	 * each branch has a unique type/size.
>  	 */
> -	return memcmp(&vma->ggtt_view.params, &view->params, view->type);
> +	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 fd5fbc83c69e..f4be20f0198a 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;
>  	}
>

Looks ok. But please ask for some second opinions on the series.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In the meantime I came up with a softer "revert" idea, if it comes to 
play, which could be adding a params size field to the view structure.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx