All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH v2 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
Date: Fri, 13 Jan 2017 09:04:46 +0000	[thread overview]
Message-ID: <2b4a7368-8be8-b0fb-f370-d3b752dd4031@linux.intel.com> (raw)
In-Reply-To: <20170112213533.16614-6-chris@chris-wilson.co.uk>


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

Side-effect is not happening in this patch. Always tricky what to do 
with commit messages for split patches. :) Maybe just rewrite the commit 
message.

>
> v2: Roll up the memcmps back into one.
> v3: And split again as Ville points out we can't trust the compiler.

Not sure what is split, memcmps? But there is only one. Needs v4? Or I 
am missing something?

> 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      |  4 +++-
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  7 files changed, 27 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 19f049cef9e3..9d6913b10f30 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -209,8 +209,10 @@ 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));

Nice!

>
> -	return memcmp(&vma->ggtt_view.params, &view->params, view->type);
> +	return memcmp(&vma->ggtt_view.partial, &view->partial, view->type);

Here you could expand on the comment from the earlier patch explaining 
the above BUILD_BUG_ON in words.

>  }
>
>  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;
>  	}
>

Regards,

Tvrtko

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

  reply	other threads:[~2017-01-13  9:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-12 21:35 Anonymouse ggtt_view params Chris Wilson
2017-01-12 21:35 ` [PATCH v2 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
2017-01-12 21:35 ` [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed Chris Wilson
2017-01-13  8:44   ` Tvrtko Ursulin
2017-01-13  8:47     ` Chris Wilson
2017-01-13  8:54       ` Tvrtko Ursulin
2017-01-12 21:35 ` [PATCH v2 3/7] drm/i915: Compact memcmp in i915_vma_compare() Chris Wilson
2017-01-13  8:57   ` Tvrtko Ursulin
2017-01-12 21:35 ` [PATCH v2 4/7] drm/i915: Stop clearing i915_ggtt_view Chris Wilson
2017-01-13  8:58   ` Tvrtko Ursulin
2017-01-12 21:35 ` [PATCH v2 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
2017-01-13  9:04   ` Tvrtko Ursulin [this message]
2017-01-13  9:16     ` Chris Wilson
2017-01-12 21:35 ` [PATCH v2 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
2017-01-12 21:35 ` [PATCH v2 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
2017-01-12 22:23 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2b4a7368-8be8-b0fb-f370-d3b752dd4031@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.