All of lore.kernel.org
 help / color / mirror / Atom feed
* Anonymoose ggtt_view_params
@ 2017-01-13 10:33 Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
                   ` (8 more replies)
  0 siblings, 9 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

Ok, ok, this cover note only exists to continue the run on joke of my
mispellings!

Everything but
[5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
has a r-b, so this is a good time to complain if this is too much of a
hack.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 2/7] drm/i915: Mark the ggtt_view structs as packed Chris Wilson
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

Naming this pair will become useful shortly...

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 6c40088f8cf4..5dd3755a5a45 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -152,20 +152,22 @@ enum i915_ggtt_view_type {
 };
 
 struct intel_rotation_info {
-	struct {
+	struct intel_rotation_plane_info {
 		/* tiles */
 		unsigned int width, height, stride, offset;
 	} plane[2];
 };
 
+struct intel_partial_info {
+	u64 offset;
+	unsigned int size;
+};
+
 struct i915_ggtt_view {
 	enum i915_ggtt_view_type type;
 
 	union {
-		struct {
-			u64 offset;
-			unsigned int size;
-		} partial;
+		struct intel_partial_info partial;
 		struct intel_rotation_info rotated;
 	} params;
 };
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/7] drm/i915: Mark the ggtt_view structs as packed
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 3/7] drm/i915: Compact memcmp in i915_vma_compare() Chris Wilson
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

In the next few patches, we will depend upon there being no
uninitialised bits inside the ggtt_view. To ensure this we add the
__packed attribute and double check with a build bug that gcc hasn't
expanded the struct to include some padding bytes.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 5dd3755a5a45..57cbd532dae1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -156,12 +156,22 @@ struct intel_rotation_info {
 		/* tiles */
 		unsigned int width, height, stride, offset;
 	} plane[2];
-};
+} __packed;
+
+static inline void assert_intel_rotation_info_is_packed(void)
+{
+	BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned int));
+}
 
 struct intel_partial_info {
 	u64 offset;
 	unsigned int size;
-};
+} __packed;
+
+static inline void assert_intel_partial_info_is_packed(void)
+{
+	BUILD_BUG_ON(sizeof(struct intel_partial_info) != sizeof(u64) + sizeof(unsigned int));
+}
 
 struct i915_ggtt_view {
 	enum i915_ggtt_view_type type;
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 3/7] drm/i915: Compact memcmp in i915_vma_compare()
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 2/7] drm/i915: Mark the ggtt_view structs as packed Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 4/7] drm/i915: Stop clearing i915_ggtt_view Chris Wilson
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

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).

v2: Add the rationale for why we encode size into ggtt_view.type as a
comment before the memcmp()

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +++++++-------
 drivers/gpu/drm/i915/i915_vma.h     | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 57cbd532dae1..20c03c30ce4e 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 */
@@ -173,10 +167,16 @@ static inline void assert_intel_partial_info_is_packed(void)
 	BUILD_BUG_ON(sizeof(struct intel_partial_info) != sizeof(u64) + sizeof(unsigned int));
 }
 
+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;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index a969bbb65871..c803afa2d619 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -199,15 +199,23 @@ 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;
 
-	if (vma->ggtt_view.type != view->type)
-		return vma->ggtt_view.type - view->type;
+	cmp -= view->type;
+	if (cmp)
+		return cmp;
 
-	return memcmp(&vma->ggtt_view.params,
-		      &view->params,
-		      sizeof(view->params));
+	BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED);
+
+	/* 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.
+	 */
+	return memcmp(&vma->ggtt_view.params, &view->params, view->type);
 }
 
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 4/7] drm/i915: Stop clearing i915_ggtt_view
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (2 preceding siblings ...)
  2017-01-13 10:33 ` [PATCH v3 3/7] drm/i915: Compact memcmp in i915_vma_compare() Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

As we now use a compact memcmp in i915_vma_compare(), we can forgo
clearing the entire view and only set the precise parameters used in
this view.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3bf517e2430a..f034d8d2dd4c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1759,7 +1759,6 @@ compute_partial_view(struct drm_i915_gem_object *obj,
 	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 = rounddown(page_offset, chunk);
 	view.params.partial.size =
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (3 preceding siblings ...)
  2017-01-13 10:33 ` [PATCH v3 4/7] drm/i915: Stop clearing i915_ggtt_view Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 11:12   ` Tvrtko Ursulin
  2017-01-13 11:26   ` Joonas Lahtinen
  2017-01-13 10:33 ` [PATCH v3 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx; +Cc: Daniel Vetter

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

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (4 preceding siblings ...)
  2017-01-13 10:33 ` [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:33 ` [PATCH v3 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

It is only being used to clear a struct and set the type, after which it
is overwritten. Since we no longer check the unset bits of the union,
skipping the clear is permissible.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 3 ---
 drivers/gpu/drm/i915/i915_gem_gtt.h  | 1 -
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 06cfd6951a5e..46e04676e011 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -106,9 +106,6 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma);
 const struct i915_ggtt_view i915_ggtt_view_normal = {
 	.type = I915_GGTT_VIEW_NORMAL,
 };
-const struct i915_ggtt_view i915_ggtt_view_rotated = {
-	.type = I915_GGTT_VIEW_ROTATED,
-};
 
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 560fd8ddaf2c..2a8b25742d72 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -183,7 +183,6 @@ struct i915_ggtt_view {
 };
 
 extern const struct i915_ggtt_view i915_ggtt_view_normal;
-extern const struct i915_ggtt_view i915_ggtt_view_rotated;
 
 enum i915_cache_level;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f4be20f0198a..f523256ef77c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2137,11 +2137,10 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
 			const struct drm_framebuffer *fb,
 			unsigned int rotation)
 {
+	view->type = I915_GGTT_VIEW_NORMAL;
 	if (drm_rotation_90_or_270(rotation)) {
-		*view = i915_ggtt_view_rotated;
+		view->type = I915_GGTT_VIEW_ROTATED;
 		view->rotated = to_intel_framebuffer(fb)->rot_info;
-	} else {
-		*view = i915_ggtt_view_normal;
 	}
 }
 
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (5 preceding siblings ...)
  2017-01-13 10:33 ` [PATCH v3 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
@ 2017-01-13 10:33 ` Chris Wilson
  2017-01-13 10:59 ` Anonymoose ggtt_view_params Tvrtko Ursulin
  2017-01-13 11:23 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Patchwork
  8 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 10:33 UTC (permalink / raw)
  To: intel-gfx

Since commit 058d88c4330f ("drm/i915: Track pinned VMA"), there is only
one user of i915_ggtt_view_normal rodate. Just treat NULL as no special
view in pin_to_display() like everywhere else.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c      | 2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 4 ----
 drivers/gpu/drm/i915/i915_gem_gtt.h  | 2 --
 drivers/gpu/drm/i915/intel_overlay.c | 3 +--
 4 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d8622fd23f5d..d4c59b53532e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3436,7 +3436,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	 * try to preserve the existing ABI).
 	 */
 	vma = ERR_PTR(-ENOSPC);
-	if (view->type == I915_GGTT_VIEW_NORMAL)
+	if (!view || view->type == I915_GGTT_VIEW_NORMAL)
 		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
 					       PIN_MAPPABLE | PIN_NONBLOCK);
 	if (IS_ERR(vma)) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 46e04676e011..d7f2aa82d810 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -103,10 +103,6 @@
 static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
-const struct i915_ggtt_view i915_ggtt_view_normal = {
-	.type = I915_GGTT_VIEW_NORMAL,
-};
-
 static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
 {
 	/* Note that as an uncached mmio write, this should flush the
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 2a8b25742d72..4b351a2d812b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -182,8 +182,6 @@ struct i915_ggtt_view {
 	};
 };
 
-extern const struct i915_ggtt_view i915_ggtt_view_normal;
-
 enum i915_cache_level;
 
 struct i915_vma;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 4473a611c664..0608fad7f593 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -811,8 +811,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	vma = i915_gem_object_pin_to_display_plane(new_bo, 0,
-						   &i915_ggtt_view_normal);
+	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: Anonymoose ggtt_view_params
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (6 preceding siblings ...)
  2017-01-13 10:33 ` [PATCH v3 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
@ 2017-01-13 10:59 ` Tvrtko Ursulin
  2017-01-13 11:11   ` Chris Wilson
  2017-01-13 11:23 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Patchwork
  8 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-01-13 10:59 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 13/01/2017 10:33, Chris Wilson wrote:
> Ok, ok, this cover note only exists to continue the run on joke of my
> mispellings!
>
> Everything but
> [5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
> has a r-b, so this is a good time to complain if this is too much of a
> hack.

If you could polish your clouded crystal ball to see if more view types 
might be coming, which then might have a colliding parameters size and 
foil the whole idea.

I do think it is a little bit of hack with a questionable benefit. And I 
think I asked a few times if you really see a performance difference for 
a few bytes smaller memcmp? Presumably it would be some test case with a 
huge number of partial views which could theoretically maybe show something?

Downside is if we need to revert it would be relatively "churny".

Regards,

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Anonymoose ggtt_view_params
  2017-01-13 10:59 ` Anonymoose ggtt_view_params Tvrtko Ursulin
@ 2017-01-13 11:11   ` Chris Wilson
  2017-01-13 11:23     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 11:11 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Fri, Jan 13, 2017 at 10:59:46AM +0000, Tvrtko Ursulin wrote:
> 
> On 13/01/2017 10:33, Chris Wilson wrote:
> >Ok, ok, this cover note only exists to continue the run on joke of my
> >mispellings!
> >
> >Everything but
> >[5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
> >has a r-b, so this is a good time to complain if this is too much of a
> >hack.
> 
> If you could polish your clouded crystal ball to see if more view
> types might be coming, which then might have a colliding parameters
> size and foil the whole idea.
> 
> I do think it is a little bit of hack with a questionable benefit.
> And I think I asked a few times if you really see a performance
> difference for a few bytes smaller memcmp? Presumably it would be
> some test case with a huge number of partial views which could
> theoretically maybe show something?

It was the doubling code size of i915_vma_compare() that struck me as
objectionable.

> Downside is if we need to revert it would be relatively "churny".

We just split the type into two fields, the enum and size.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
  2017-01-13 10:33 ` [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
@ 2017-01-13 11:12   ` Tvrtko Ursulin
  2017-01-13 11:26   ` Joonas Lahtinen
  1 sibling, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-01-13 11:12 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v3,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view
  2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
                   ` (7 preceding siblings ...)
  2017-01-13 10:59 ` Anonymoose ggtt_view_params Tvrtko Ursulin
@ 2017-01-13 11:23 ` Patchwork
  8 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-01-13 11:23 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v3,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view
URL   : https://patchwork.freedesktop.org/series/17960/
State : success

== Summary ==

Series 17960v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17960/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

ae1fc35150f6aaadf190a8a881adc9aeb78fd7c9 drm-tip: 2017y-01m-13d-00h-42m-24s UTC integration manifest
dc1bbb4 drm/i915: Eliminate superfluous i915_ggtt_view_normal
35dc25c drm/i915: Eliminate superfluous i915_ggtt_view_rotated
096579b drm/i915: Convert i915_ggtt_view to use an anonymous union
85ed27f drm/i915: Stop clearing i915_ggtt_view
d1c89f0 drm/i915: Compact memcmp in i915_vma_compare()
fde4b74 drm/i915: Mark the ggtt_view structs as packed
cbb3967 drm/i915: Name the anonymous structs inside i915_ggtt_view

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3509/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Anonymoose ggtt_view_params
  2017-01-13 11:11   ` Chris Wilson
@ 2017-01-13 11:23     ` Tvrtko Ursulin
  2017-01-13 11:34       ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-01-13 11:23 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 13/01/2017 11:11, Chris Wilson wrote:
> On Fri, Jan 13, 2017 at 10:59:46AM +0000, Tvrtko Ursulin wrote:
>>
>> On 13/01/2017 10:33, Chris Wilson wrote:
>>> Ok, ok, this cover note only exists to continue the run on joke of my
>>> mispellings!
>>>
>>> Everything but
>>> [5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
>>> has a r-b, so this is a good time to complain if this is too much of a
>>> hack.
>>
>> If you could polish your clouded crystal ball to see if more view
>> types might be coming, which then might have a colliding parameters
>> size and foil the whole idea.
>>
>> I do think it is a little bit of hack with a questionable benefit.
>> And I think I asked a few times if you really see a performance
>> difference for a few bytes smaller memcmp? Presumably it would be
>> some test case with a huge number of partial views which could
>> theoretically maybe show something?
>
> It was the doubling code size of i915_vma_compare() that struck me as
> objectionable.

Why does this series shrink i915_vma_compare? Was it getting inlined in 
your build? For me it doesn't.

>> Downside is if we need to revert it would be relatively "churny".
>
> We just split the type into two fields, the enum and size.

Yes realized that a bit later.

Regards,

Tvrtko


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
  2017-01-13 10:33 ` [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
  2017-01-13 11:12   ` Tvrtko Ursulin
@ 2017-01-13 11:26   ` Joonas Lahtinen
  1 sibling, 0 replies; 15+ messages in thread
From: Joonas Lahtinen @ 2017-01-13 11:26 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter

On pe, 2017-01-13 at 10:33 +0000, 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Anonymoose ggtt_view_params
  2017-01-13 11:23     ` Tvrtko Ursulin
@ 2017-01-13 11:34       ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-01-13 11:34 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Fri, Jan 13, 2017 at 11:23:47AM +0000, Tvrtko Ursulin wrote:
> 
> On 13/01/2017 11:11, Chris Wilson wrote:
> >On Fri, Jan 13, 2017 at 10:59:46AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 13/01/2017 10:33, Chris Wilson wrote:
> >>>Ok, ok, this cover note only exists to continue the run on joke of my
> >>>mispellings!
> >>>
> >>>Everything but
> >>>[5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union
> >>>has a r-b, so this is a good time to complain if this is too much of a
> >>>hack.
> >>
> >>If you could polish your clouded crystal ball to see if more view
> >>types might be coming, which then might have a colliding parameters
> >>size and foil the whole idea.
> >>
> >>I do think it is a little bit of hack with a questionable benefit.
> >>And I think I asked a few times if you really see a performance
> >>difference for a few bytes smaller memcmp? Presumably it would be
> >>some test case with a huge number of partial views which could
> >>theoretically maybe show something?
> >
> >It was the doubling code size of i915_vma_compare() that struck me as
> >objectionable.
> 
> Why does this series shrink i915_vma_compare? Was it getting inlined
> in your build? For me it doesn't.

It's inlined for me. From my build, we only maintain the size of the
i915_vma_lookup. Hmm, this might also explain why changing the return of
i915_vma_compare() had significance for your build and not mine. Weird
gcc is weird.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-01-13 11:34 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-13 10:33 Anonymoose ggtt_view_params Chris Wilson
2017-01-13 10:33 ` [PATCH v3 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
2017-01-13 10:33 ` [PATCH v3 2/7] drm/i915: Mark the ggtt_view structs as packed Chris Wilson
2017-01-13 10:33 ` [PATCH v3 3/7] drm/i915: Compact memcmp in i915_vma_compare() Chris Wilson
2017-01-13 10:33 ` [PATCH v3 4/7] drm/i915: Stop clearing i915_ggtt_view Chris Wilson
2017-01-13 10:33 ` [PATCH v3 5/7] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
2017-01-13 11:12   ` Tvrtko Ursulin
2017-01-13 11:26   ` Joonas Lahtinen
2017-01-13 10:33 ` [PATCH v3 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
2017-01-13 10:33 ` [PATCH v3 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
2017-01-13 10:59 ` Anonymoose ggtt_view_params Tvrtko Ursulin
2017-01-13 11:11   ` Chris Wilson
2017-01-13 11:23     ` Tvrtko Ursulin
2017-01-13 11:34       ` Chris Wilson
2017-01-13 11:23 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view Patchwork

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.