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

[CI 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 9f04c9febe4d..80f0cd534db6 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

[CI 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 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 80f0cd534db6..334b61b84376 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

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

v2: Add the rationale for why we encode size into ggtt_view.type as a
comment before the memcmp()
v3: Use a switch to also assert that no two i915_ggtt_view_type have the same
value.

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 | 28 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_vma.h     | 20 ++++++++++++++------
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 334b61b84376..35ea4a18dc77 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,30 @@ 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),
+};
+
+static inline void assert_i915_ggtt_view_type_is_unique(void)
+{
+	/* As we encode the size of each branch inside the union into its type,
+	 * we have to be careful that each branch has a unique size.
+	 */
+	switch ((enum i915_ggtt_view_type)0) {
+	case I915_GGTT_VIEW_NORMAL:
+	case I915_GGTT_VIEW_PARTIAL:
+	case I915_GGTT_VIEW_ROTATED:
+		/* gcc complains if these are identical cases */
+		break;
+	}
+}
+
 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 008cf115f38f..fdbacc036080 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));
+	/* 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.
+	 */
+	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL >= I915_GGTT_VIEW_PARTIAL);
+	BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL >= I915_GGTT_VIEW_ROTATED);
+	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

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

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

[Chris Wilson]

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 32ede342ab94..01fdbbf0fd43 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 7d21cdfc6b0e..e24b961c30c6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3511,7 +3511,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;
 
@@ -3523,9 +3523,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;
@@ -3577,7 +3575,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 35ea4a18dc77..71e7e0a7e2b6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -193,7 +193,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 379364b8fef9..fe93ed1e012f 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 fdbacc036080..86b60fb4e954 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -212,10 +212,17 @@ i915_vma_compare(struct i915_vma *vma,
 	 * 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.
 	 */
 	BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL >= I915_GGTT_VIEW_PARTIAL);
 	BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL >= I915_GGTT_VIEW_ROTATED);
-	return memcmp(&vma->ggtt_view.params, &view->params, view->type);
+	BUILD_BUG_ON(offsetof(typeof(*view), rotated) !=
+		     offsetof(typeof(*view), partial));
+	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

[CI 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 e24b961c30c6..169d10d81334 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 71e7e0a7e2b6..f673544e570e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -197,7 +197,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

Re: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Matthew Auld]

On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 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>
My machine is all of a sudden hanging just after boot and bisection
points to this as the culprit. When I revert back to the old behaviour
of copying from the zeroed struct for the normal case, then the issue
disappears, which doesn't make any sense. Any ideas?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Chris Wilson]

On Mon, Jan 23, 2017 at 01:51:54PM +0000, Matthew Auld wrote:
> On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 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>
> My machine is all of a sudden hanging just after boot and bisection
> points to this as the culprit. When I revert back to the old behaviour
> of copying from the zeroed struct for the normal case, then the issue
> disappears, which doesn't make any sense. Any ideas?

Quick diff of which clear you need?

s/view->type = NORMAL;/ view = *i915_ggtt_view_normal; / ?
-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

Re: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Matthew Auld]

On 23 January 2017 at 14:02, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jan 23, 2017 at 01:51:54PM +0000, Matthew Auld wrote:
>> On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > 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>
>> My machine is all of a sudden hanging just after boot and bisection
>> points to this as the culprit. When I revert back to the old behaviour
>> of copying from the zeroed struct for the normal case, then the issue
>> disappears, which doesn't make any sense. Any ideas?
>
> Quick diff of which clear you need?

-       view->type = I915_GGTT_VIEW_NORMAL;
        if (drm_rotation_90_or_270(rotation)) {
                view->type = I915_GGTT_VIEW_ROTATED;
                view->rotated = to_intel_framebuffer(fb)->rot_info;
+       } else {
+               *view = i915_ggtt_view_normal;
        }
 }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Chris Wilson]

On Mon, Jan 23, 2017 at 02:22:38PM +0000, Matthew Auld wrote:
> On 23 January 2017 at 14:02, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Jan 23, 2017 at 01:51:54PM +0000, Matthew Auld wrote:
> >> On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > 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>
> >> My machine is all of a sudden hanging just after boot and bisection
> >> points to this as the culprit. When I revert back to the old behaviour
> >> of copying from the zeroed struct for the normal case, then the issue
> >> disappears, which doesn't make any sense. Any ideas?
> >
> > Quick diff of which clear you need?
> 
> -       view->type = I915_GGTT_VIEW_NORMAL;
>         if (drm_rotation_90_or_270(rotation)) {
>                 view->type = I915_GGTT_VIEW_ROTATED;
>                 view->rotated = to_intel_framebuffer(fb)->rot_info;
> +       } else {
> +               *view = i915_ggtt_view_normal;
>         }
>  }

For no good reason, perhaps?
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 307b22ae7791..155906e84812 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -91,7 +91,7 @@ vma_create(struct drm_i915_gem_object *obj,
        vma->size = obj->base.size;
        vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
-       if (view) {
+       if (view && view->type != I915_GGTT_VIEW_NORMAL) {

-- 
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: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Chris Wilson]

On Mon, Jan 23, 2017 at 02:39:03PM +0000, Chris Wilson wrote:
> On Mon, Jan 23, 2017 at 02:22:38PM +0000, Matthew Auld wrote:
> > On 23 January 2017 at 14:02, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Jan 23, 2017 at 01:51:54PM +0000, Matthew Auld wrote:
> > >> On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >> > 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>
> > >> My machine is all of a sudden hanging just after boot and bisection
> > >> points to this as the culprit. When I revert back to the old behaviour
> > >> of copying from the zeroed struct for the normal case, then the issue
> > >> disappears, which doesn't make any sense. Any ideas?
> > >
> > > Quick diff of which clear you need?
> > 
> > -       view->type = I915_GGTT_VIEW_NORMAL;
> >         if (drm_rotation_90_or_270(rotation)) {
> >                 view->type = I915_GGTT_VIEW_ROTATED;
> >                 view->rotated = to_intel_framebuffer(fb)->rot_info;
> > +       } else {
> > +               *view = i915_ggtt_view_normal;
> >         }
> >  }
> 
> For no good reason, perhaps?
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 307b22ae7791..155906e84812 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -91,7 +91,7 @@ vma_create(struct drm_i915_gem_object *obj,
>         vma->size = obj->base.size;
>         vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
>  
> -       if (view) {
> +       if (view && view->type != I915_GGTT_VIEW_NORMAL) {

Found reason.

i915_gem_fault:
        ret = remap_io_mapping(area,
                               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);

So we depend upon ggtt_view being cleared for normal. Patch works, but we
still have a subtlety in that ROTATED cannot be mapped.
-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

Re: [CI 6/7] drm/i915: Eliminate superfluous i915_ggtt_view_rotated

[Chris Wilson]

On Mon, Jan 23, 2017 at 01:51:54PM +0000, Matthew Auld wrote:
> On 14 January 2017 at 00:28, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 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>
> My machine is all of a sudden hanging just after boot and bisection
> points to this as the culprit. When I revert back to the old behaviour
> of copying from the zeroed struct for the normal case, then the issue
> disappears, which doesn't make any sense. Any ideas?

kmemcheck is our friend here as well.
-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

[CI 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 169d10d81334..4c88745b6078 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 f673544e570e..3c5ef5358cef 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -196,8 +196,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 [CI,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view

[Patchwork]

== Series Details ==

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

== Summary ==

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

a957dfe613ab43c6af84833f2b88ea941320250f drm-tip: 2017y-01m-13d-18h-49m-04s UTC integration manifest
3a576ad drm/i915: Eliminate superfluous i915_ggtt_view_normal
2982533 drm/i915: Eliminate superfluous i915_ggtt_view_rotated
067f3f3 drm/i915: Convert i915_ggtt_view to use an anonymous union
3b0cf64 drm/i915: Stop clearing i915_ggtt_view
7d12c51 drm/i915: Compact memcmp in i915_vma_compare()
4cb8a27 drm/i915: Mark the ggtt_view structs as packed
5f20d78 drm/i915: Name the anonymous structs inside i915_ggtt_view

== Logs ==

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

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

[Chris Wilson]

On Sat, Jan 14, 2017 at 12:53:36AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [CI,1/7] drm/i915: Name the anonymous structs inside i915_ggtt_view
> URL   : https://patchwork.freedesktop.org/series/18006/
> State : success
> 
> == Summary ==
> 
> Series 18006v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/18006/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 

Applied because I want to take (have taken) advantage of the shorter name
and named structs for writing testcases. Thanks for the review and
finding a workable compromise.
-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