Re: [PATCH 2/6] drm/i915: Pack the partial view size and offset into a single u64

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

On 11/01/2017 21:51, Chris Wilson wrote:
> Since the partial offset must be page aligned, we can use those low 12
> bits to encode the size of the partial view (which then cannot be larger
> than 8MiB in pages). A requirement for avoiding unused bits inside the
> struct is imposed later by avoiding the clear of the struct (or of
> copying around static initialisers). This is easier to guarantee by
> manual packing into a single u64 - the presence of the u64 inside a
> struct causes gcc to pad the struct out to a u64 boundary, even if we
> use the __packed attribute.

Have you maybe tried bitfields? It would be much more readable if the 
generated code is roughly the same..

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c     | 14 +++++++++-----
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  6 +++---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 25 +++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_vma.c     |  9 ++++-----
>  4 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3bf517e2430a..3107fff970fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1696,6 +1696,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>
>  static unsigned int tile_row_pages(struct drm_i915_gem_object *obj)
>  {
> +	BUILD_BUG_ON(ilog2(GEN7_FENCE_MAX_PITCH_VAL*128*32 >> PAGE_SHIFT) >
> +		     INTEL_PARTIAL_SIZE_BITS);
> +
>  	return i915_gem_object_get_tile_row_size(obj) >> PAGE_SHIFT;
>  }
>
> @@ -1761,10 +1764,11 @@ compute_partial_view(struct drm_i915_gem_object *obj,
>
>  	memset(&view, 0, sizeof(view));
>  	view.type = I915_GGTT_VIEW_PARTIAL;
> -	view.params.partial.offset = rounddown(page_offset, chunk);
> -	view.params.partial.size =
> -		min_t(unsigned int, chunk,
> -		      (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset);
> +	view.params.partial.offset_size = rounddown(page_offset, chunk);
> +	view.params.partial.offset_size =
> +		(view.params.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
> +		(min_t(unsigned int, chunk,
> +		       (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset_size) - 1);
>
>  	/* If the partial covers the entire object, just create a normal VMA. */
>  	if (chunk >= obj->base.size >> PAGE_SHIFT)
> @@ -1880,7 +1884,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 + intel_partial_get_offset(&vma->ggtt_view.params.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 ed120a1e7f93..c36c196546f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3490,20 +3490,20 @@ 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 offset;
> +	unsigned int count, offset;
>  	int ret = -ENOMEM;
>
>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (!st)
>  		goto err_st_alloc;
>
> +	count = intel_partial_get_page_count(&view->params.partial);
>  	ret = sg_alloc_table(st, count, GFP_KERNEL);
>  	if (ret)
>  		goto err_sg_alloc;
>
>  	iter = i915_gem_object_get_sg(obj,
> -				      view->params.partial.offset,
> +				      intel_partial_get_page_offset(&view->params.partial),
>  				      &offset);
>  	GEM_BUG_ON(!iter);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 5dd3755a5a45..3187a260e6e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -159,10 +159,31 @@ struct intel_rotation_info {
>  };
>
>  struct intel_partial_info {
> -	u64 offset;
> -	unsigned int size;
> +	/* offset is page-aligned, leaving just enough bits for the size */
> +#define INTEL_PARTIAL_SIZE_BITS PAGE_SHIFT
> +	u64 offset_size;
>  };
>
> +static inline u32 intel_partial_get_page_count(const struct intel_partial_info *pi)
> +{
> +	return 1 + (pi->offset_size & GENMASK(INTEL_PARTIAL_SIZE_BITS-1, 0));
> +}
> +
> +static inline u32 intel_partial_get_size(const struct intel_partial_info *pi)
> +{
> +	return intel_partial_get_page_count(pi) << PAGE_SHIFT;
> +}
> +
> +static inline u64 intel_partial_get_offset(const struct intel_partial_info *pi)
> +{
> +	return pi->offset_size & GENMASK(63, INTEL_PARTIAL_SIZE_BITS);
> +}
> +
> +static inline u64 intel_partial_get_page_offset(const struct intel_partial_info *pi)
> +{
> +	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
> +}
> +
>  struct i915_ggtt_view {
>  	enum i915_ggtt_view_type type;
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index b74eeb73ae41..7226c5ef5410 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -97,11 +97,10 @@ __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,
> -						     obj->base.size >> PAGE_SHIFT));
> -			vma->size = view->params.partial.size;
> -			vma->size <<= PAGE_SHIFT;
> +						     intel_partial_get_offset(&view->params.partial),
> +						     intel_partial_get_size(&view->params.partial),
> +						     obj->base.size));
> +			vma->size = intel_partial_get_size(&view->params.partial);
>  			GEM_BUG_ON(vma->size >= obj->base.size);
>  		} else if (view->type == I915_GGTT_VIEW_ROTATED) {
>  			vma->size =
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx