Anonymouse ggtt_view params

Anonymouse ggtt_view params

[Chris Wilson]

No one else liked packing the partial parameters into a single u64, so we
are back to a full 32bit for the partial size, and using __packed and a
bunch of asserts to ensure we have no unused bits inside each struct of
the union.

Hopefully that's the last contentios point!
-Chris

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

[PATCH v2 1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view

[Chris Wilson]

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

[PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed

[Chris Wilson]

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 on that gcc hasn't
expanded the struct to include some padding bytes.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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

[PATCH v2 3/7] drm/i915: Compact memcmp in i915_vma_compare()

[Chris Wilson]

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

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +++++++-------
 drivers/gpu/drm/i915/i915_vma.h     | 15 +++++++++------
 2 files changed, 16 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..19f049cef9e3 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -199,15 +199,18 @@ 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;
+
+	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);
 
-	return memcmp(&vma->ggtt_view.params,
-		      &view->params,
-		      sizeof(view->params));
+	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

[PATCH v2 4/7] drm/i915: Stop clearing i915_ggtt_view

[Chris Wilson]

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

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

[Chris Wilson]

Save a lot of characters by making the union anonymous, with the
side-effect of ignoring unset bits when comparing views.

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@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));
 
-	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

[PATCH v2 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Chris Wilson]

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

[PATCH v2 7/7] drm/i915: Eliminate superfluous i915_ggtt_view_normal

[Chris Wilson]

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

✓ Fi.CI.BAT: success for series starting with [v2,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view

[Patchwork]

== Series Details ==

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

== Summary ==

Series 17923v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17923/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 

b72db2710635164c7b0495192fbf44e3a17362f6 drm-tip: 2017y-01m-12d-20h-39m-13s UTC integration manifest
6a1ebef drm/i915: Eliminate superfluous i915_ggtt_view_normal
d2a2989 drm/i915: Eliminate superfluous i915_ggtt_view_rotated
9abf337 drm/i915: Convert i915_ggtt_view to use an anonymous union
3951d61 drm/i915: Stop clearing i915_ggtt_view
f979633 drm/i915: Compact memcmp in i915_vma_compare()
c1ad4d2 drm/i915: Mark the ggtt_view structs as packed
85788a4 drm/i915: Name the anonymous structs inside i915_ggtt_view

== Logs ==

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

Re: [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed

[Tvrtko Ursulin]

On 12/01/2017 21:35, Chris Wilson wrote:
> 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 on that gcc hasn't
> expanded the struct to include some padding bytes.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  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];

Isn't packed theoretically needed on the intel_rotation_plane_info name 
in the previous patch as well? Otherwise there could be a hole between 
the array elements if the structure got changed in the future.

Regards,

Tvrtko

> -};
> +} __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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed

[Chris Wilson]

On Fri, Jan 13, 2017 at 08:44:34AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2017 21:35, Chris Wilson wrote:
> >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 on that gcc hasn't
> >expanded the struct to include some padding bytes.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > 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];
> 
> Isn't packed theoretically needed on the intel_rotation_plane_info
> name in the previous patch as well? Otherwise there could be a hole
> between the array elements if the structure got changed in the
> future.

Possibly, not too sure on the inner details of __packed. The assert
below will catch accidental holes in future (and if they change the
struct without changing the assert, a nuisance).

> >+static inline void assert_intel_rotation_info_is_packed(void)
> >+{
> >+	BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned int));
> >+}

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

Re: [PATCH v2 2/7] drm/i915: Mark the ggtt_view structs as packed

[Tvrtko Ursulin]

On 13/01/2017 08:47, Chris Wilson wrote:
> On Fri, Jan 13, 2017 at 08:44:34AM +0000, Tvrtko Ursulin wrote:
>>
>> On 12/01/2017 21:35, Chris Wilson wrote:
>>> 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 on that gcc hasn't
>>> expanded the struct to include some padding bytes.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>> 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];
>>
>> Isn't packed theoretically needed on the intel_rotation_plane_info
>> name in the previous patch as well? Otherwise there could be a hole
>> between the array elements if the structure got changed in the
>> future.
>
> Possibly, not too sure on the inner details of __packed. The assert
> below will catch accidental holes in future (and if they change the
> struct without changing the assert, a nuisance).
>
>>> +static inline void assert_intel_rotation_info_is_packed(void)
>>> +{
>>> +	BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 8*sizeof(unsigned int));
>>> +}
>

Somehow I only saw the assert for partial info.

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

Regards,

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

Re: [PATCH v2 3/7] drm/i915: Compact memcmp in i915_vma_compare()

[Tvrtko Ursulin]

On 12/01/2017 21:35, Chris Wilson wrote:
> 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).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +++++++-------
>  drivers/gpu/drm/i915/i915_vma.h     | 15 +++++++++------
>  2 files changed, 16 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..19f049cef9e3 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -199,15 +199,18 @@ 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;
> +
> +	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);
>
> -	return memcmp(&vma->ggtt_view.params,
> -		      &view->params,
> -		      sizeof(view->params));
> +	return memcmp(&vma->ggtt_view.params, &view->params, view->type);

Worth a comment at the memcmp callsite about how type for size is 
intended and why. Otherwise it is only in the commit message which is 
more digging.

>  }
>
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>

With the comment added,

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

Regards,

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

Re: [PATCH v2 4/7] drm/i915: Stop clearing i915_ggtt_view

[Tvrtko Ursulin]

On 12/01/2017 21:35, Chris Wilson wrote:
> 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>
> ---
>  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 =
>

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

Regards,

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

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

[Tvrtko Ursulin]

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

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

[Chris Wilson]

On Fri, Jan 13, 2017 at 09:04:46AM +0000, Tvrtko Ursulin wrote:
> 
> 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?

The changelog just needs rewriting so that the preconditions for this
patch (wanting to able to use a single memcmp and so no uninitialised
bits within the memcmp) are clear. In a sense, it's why I liked having
this in one patch, since the patches are not really standalone and only
make sense together :| Revert one, we have to revert them all.
-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