Anonymous ggtt_view params

Anonymous ggtt_view params

[Chris Wilson]

It makes reading the vma->ggtt_view code more pleasant, but at an
unfortunate cost in header complexity to ensure that no unwanted bits are
in the struct and that gcc doesn't double the size of a few inlined
functions.

I like it, but I am biased.
-Chris

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

[PATCH 1/6] 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 2/6] drm/i915: Pack the partial view size and offset into a single u64

[Chris Wilson]

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

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c     | 14 +++++++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c |  6 +++---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_vma.c     |  9 ++++-----
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3bf517e2430a..3107fff970fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1696,6 +1696,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 
 static unsigned int tile_row_pages(struct drm_i915_gem_object *obj)
 {
+	BUILD_BUG_ON(ilog2(GEN7_FENCE_MAX_PITCH_VAL*128*32 >> PAGE_SHIFT) >
+		     INTEL_PARTIAL_SIZE_BITS);
+
 	return i915_gem_object_get_tile_row_size(obj) >> PAGE_SHIFT;
 }
 
@@ -1761,10 +1764,11 @@ compute_partial_view(struct drm_i915_gem_object *obj,
 
 	memset(&view, 0, sizeof(view));
 	view.type = I915_GGTT_VIEW_PARTIAL;
-	view.params.partial.offset = rounddown(page_offset, chunk);
-	view.params.partial.size =
-		min_t(unsigned int, chunk,
-		      (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset);
+	view.params.partial.offset_size = rounddown(page_offset, chunk);
+	view.params.partial.offset_size =
+		(view.params.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
+		(min_t(unsigned int, chunk,
+		       (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset_size) - 1);
 
 	/* If the partial covers the entire object, just create a normal VMA. */
 	if (chunk >= obj->base.size >> PAGE_SHIFT)
@@ -1880,7 +1884,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
 
 	/* Finally, remap it using the new GTT offset */
 	ret = remap_io_mapping(area,
-			       area->vm_start + (vma->ggtt_view.params.partial.offset << PAGE_SHIFT),
+			       area->vm_start + intel_partial_get_offset(&vma->ggtt_view.params.partial),
 			       (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT,
 			       min_t(u64, vma->size, area->vm_end - area->vm_start),
 			       &ggtt->mappable);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ed120a1e7f93..c36c196546f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3490,20 +3490,20 @@ intel_partial_pages(const struct i915_ggtt_view *view,
 {
 	struct sg_table *st;
 	struct scatterlist *sg, *iter;
-	unsigned int count = view->params.partial.size;
-	unsigned int offset;
+	unsigned int count, offset;
 	int ret = -ENOMEM;
 
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (!st)
 		goto err_st_alloc;
 
+	count = intel_partial_get_page_count(&view->params.partial);
 	ret = sg_alloc_table(st, count, GFP_KERNEL);
 	if (ret)
 		goto err_sg_alloc;
 
 	iter = i915_gem_object_get_sg(obj,
-				      view->params.partial.offset,
+				      intel_partial_get_page_offset(&view->params.partial),
 				      &offset);
 	GEM_BUG_ON(!iter);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 5dd3755a5a45..3187a260e6e1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -159,10 +159,31 @@ struct intel_rotation_info {
 };
 
 struct intel_partial_info {
-	u64 offset;
-	unsigned int size;
+	/* offset is page-aligned, leaving just enough bits for the size */
+#define INTEL_PARTIAL_SIZE_BITS PAGE_SHIFT
+	u64 offset_size;
 };
 
+static inline u32 intel_partial_get_page_count(const struct intel_partial_info *pi)
+{
+	return 1 + (pi->offset_size & GENMASK(INTEL_PARTIAL_SIZE_BITS-1, 0));
+}
+
+static inline u32 intel_partial_get_size(const struct intel_partial_info *pi)
+{
+	return intel_partial_get_page_count(pi) << PAGE_SHIFT;
+}
+
+static inline u64 intel_partial_get_offset(const struct intel_partial_info *pi)
+{
+	return pi->offset_size & GENMASK(63, INTEL_PARTIAL_SIZE_BITS);
+}
+
+static inline u64 intel_partial_get_page_offset(const struct intel_partial_info *pi)
+{
+	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
+}
+
 struct i915_ggtt_view {
 	enum i915_ggtt_view_type type;
 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index b74eeb73ae41..7226c5ef5410 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -97,11 +97,10 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
 		vma->ggtt_view = *view;
 		if (view->type == I915_GGTT_VIEW_PARTIAL) {
 			GEM_BUG_ON(range_overflows_t(u64,
-						     view->params.partial.offset,
-						     view->params.partial.size,
-						     obj->base.size >> PAGE_SHIFT));
-			vma->size = view->params.partial.size;
-			vma->size <<= PAGE_SHIFT;
+						     intel_partial_get_offset(&view->params.partial),
+						     intel_partial_get_size(&view->params.partial),
+						     obj->base.size));
+			vma->size = intel_partial_get_size(&view->params.partial);
 			GEM_BUG_ON(vma->size >= obj->base.size);
 		} else if (view->type == I915_GGTT_VIEW_ROTATED) {
 			vma->size =
-- 
2.11.0

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

[PATCH 3/6] 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 3187a260e6e1..36d85599ffc9 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 */
@@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
 	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
 }
 
+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 4/6] 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_gem.c      | 11 +++++------
 drivers/gpu/drm/i915/i915_gem_gtt.c  |  7 ++++---
 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 +-
 6 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3107fff970fd..04e569d5ac40 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1762,13 +1762,12 @@ 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_size = rounddown(page_offset, chunk);
-	view.params.partial.offset_size =
-		(view.params.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
+	view.partial.offset_size = rounddown(page_offset, chunk);
+	view.partial.offset_size =
+		(view.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
 		(min_t(unsigned int, chunk,
-		       (obj->base.size >> PAGE_SHIFT) - view.params.partial.offset_size) - 1);
+		       (obj->base.size >> PAGE_SHIFT) - view.partial.offset_size) - 1);
 
 	/* If the partial covers the entire object, just create a normal VMA. */
 	if (chunk >= obj->base.size >> PAGE_SHIFT)
@@ -1884,7 +1883,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 + intel_partial_get_offset(&vma->ggtt_view.params.partial),
+			       area->vm_start + intel_partial_get_offset(&vma->ggtt_view.partial),
 			       (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT,
 			       min_t(u64, vma->size, area->vm_end - area->vm_start),
 			       &ggtt->mappable);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c36c196546f8..73581d1b2da6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3497,13 +3497,13 @@ intel_partial_pages(const struct i915_ggtt_view *view,
 	if (!st)
 		goto err_st_alloc;
 
-	count = intel_partial_get_page_count(&view->params.partial);
+	count = intel_partial_get_page_count(&view->partial);
 	ret = sg_alloc_table(st, count, GFP_KERNEL);
 	if (ret)
 		goto err_sg_alloc;
 
 	iter = i915_gem_object_get_sg(obj,
-				      intel_partial_get_page_offset(&view->params.partial),
+				      intel_partial_get_page_offset(&view->partial),
 				      &offset);
 	GEM_BUG_ON(!iter);
 
@@ -3556,7 +3556,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 36d85599ffc9..0ddb5f6731ce 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -190,7 +190,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 7226c5ef5410..2307e1610743 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -97,14 +97,13 @@ __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,
-						     intel_partial_get_offset(&view->params.partial),
-						     intel_partial_get_size(&view->params.partial),
+						     intel_partial_get_offset(&view->partial),
+						     intel_partial_get_size(&view->partial),
 						     obj->base.size));
-			vma->size = intel_partial_get_size(&view->params.partial);
+			vma->size = intel_partial_get_size(&view->partial);
 			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 56047018391c..f9d4077b5c58 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 5/6] 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 73581d1b2da6..70b7ab747a98 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 0ddb5f6731ce..4b91b009e14c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -194,7 +194,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 f9d4077b5c58..5c69e27fd92c 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 6/6] 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 04e569d5ac40..0d9fb51aa3cc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3440,7 +3440,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 70b7ab747a98..b2595c0cf8e9 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 4b91b009e14c..7ce4e2aaa159 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -193,8 +193,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 [1/6] drm/i915: Name the anonymous structs inside i915_ggtt_view

[Patchwork]

== Series Details ==

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

== Summary ==

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

60f8884d35facd41e1b085a19444205ec13a5da0 drm-tip: 2017y-01m-11d-20h-53m-23s UTC integration manifest
4e8ec77 drm/i915: Convert i915_ggtt_view to use an anonymous union
2470d28 drm/i915: Compact memcmp in i915_vma_compare()
623177a drm/i915: Pack the partial view size and offset into a single u64
c82de988 drm/i915: Name the anonymous structs inside i915_ggtt_view

== Logs ==

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

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

[Tvrtko Ursulin]

On 11/01/2017 21:51, 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 3187a260e6e1..36d85599ffc9 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 */
> @@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
>  	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>  }
>
> +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),
> +};

I just realized this will uglify the debugfs output. In the light of 
that I think we should leave the enum human readable and instead you 
could have a static map in i915_vma_compare. Hopefully code generation 
wouldn't be affected by it.

Regards,

Tvrtko

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

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

[Tvrtko Ursulin]

On 12/01/2017 07:08, Tvrtko Ursulin wrote:
>
> On 11/01/2017 21:51, 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 3187a260e6e1..36d85599ffc9 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 */
>> @@ -184,10 +178,16 @@ static inline u64
>> intel_partial_get_page_offset(const struct intel_partial_info
>>      return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>>  }
>>
>> +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),
>> +};
>
> I just realized this will uglify the debugfs output. In the light of
> that I think we should leave the enum human readable and instead you
> could have a static map in i915_vma_compare. Hopefully code generation
> wouldn't be affected by it.

Another alternative, perhaps better, a map of view type to printable 
names in i915_debugfs.c.

Regards,

Tvrtko

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

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

[Tvrtko Ursulin]

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

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

Regards,

Tvrtko

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

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

[Chris Wilson]

On Thu, Jan 12, 2017 at 07:14:55AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2017 07:08, Tvrtko Ursulin wrote:
> >
> >On 11/01/2017 21:51, 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 3187a260e6e1..36d85599ffc9 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 */
> >>@@ -184,10 +178,16 @@ static inline u64
> >>intel_partial_get_page_offset(const struct intel_partial_info
> >>     return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
> >> }
> >>
> >>+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),
> >>+};
> >
> >I just realized this will uglify the debugfs output. In the light of
> >that I think we should leave the enum human readable and instead you
> >could have a static map in i915_vma_compare. Hopefully code generation
> >wouldn't be affected by it.
> 
> Another alternative, perhaps better, a map of view type to printable
> names in i915_debugfs.c.

Yeah, when in doubt remove guess work from the reader.
-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: [PATCH 3/6] drm/i915: Compact memcmp in i915_vma_compare()

[Tvrtko Ursulin]

On 12/01/2017 10:05, Chris Wilson wrote:
> On Thu, Jan 12, 2017 at 07:14:55AM +0000, Tvrtko Ursulin wrote:
>> On 12/01/2017 07:08, Tvrtko Ursulin wrote:
>>>
>>> On 11/01/2017 21:51, 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 3187a260e6e1..36d85599ffc9 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 */
>>>> @@ -184,10 +178,16 @@ static inline u64
>>>> intel_partial_get_page_offset(const struct intel_partial_info
>>>>     return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>>>> }
>>>>
>>>> +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),
>>>> +};
>>>
>>> I just realized this will uglify the debugfs output. In the light of
>>> that I think we should leave the enum human readable and instead you
>>> could have a static map in i915_vma_compare. Hopefully code generation
>>> wouldn't be affected by it.
>>
>> Another alternative, perhaps better, a map of view type to printable
>> names in i915_debugfs.c.
>
> Yeah, when in doubt remove guess work from the reader.

Remembered one downside of the enum->string map - the wasted space due 
sparseness. Not sure then, if it keeps code generation in check perhaps 
enum->size map would be better.

Regards,

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

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

[Chris Wilson]

On Wed, Jan 11, 2017 at 09:51:03PM +0000, 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 3187a260e6e1..36d85599ffc9 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 */
> @@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
>  	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>  }
>  
> +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),

One alternative to packing intel_partial_info into a u64 is to use

I915_GGTT_VIEW_PARTIAL = 12, /* ideal sizeof(struct intel_partial_info) */

How horrible is that?
-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: [PATCH 2/6] drm/i915: Pack the partial view size and offset into a single u64

[Chris Wilson]

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

I made a mistake in my testing. :|

I assked gcc whether struct {u64; u32;} was the same size as a u64.
If we ask gcc whether struct {u64; u32;} is 12 bytes, all we need is the
__packed attribute.
-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: [PATCH 3/6] drm/i915: Compact memcmp in i915_vma_compare()

[Tvrtko Ursulin]

On 12/01/2017 11:33, Chris Wilson wrote:
> On Wed, Jan 11, 2017 at 09:51:03PM +0000, 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 3187a260e6e1..36d85599ffc9 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 */
>> @@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
>>  	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>>  }
>>
>> +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),
>
> One alternative to packing intel_partial_info into a u64 is to use
>
> I915_GGTT_VIEW_PARTIAL = 12, /* ideal sizeof(struct intel_partial_info) */
>
> How horrible is that?

Really horrible, nightmare to debug. :)

You are happy with 12 packed bytes I gather so no need for it?

Regards,

Tvrtko

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

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

[Chris Wilson]

On Thu, Jan 12, 2017 at 03:00:20PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2017 11:33, Chris Wilson wrote:
> >On Wed, Jan 11, 2017 at 09:51:03PM +0000, 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 3187a260e6e1..36d85599ffc9 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 */
> >>@@ -184,10 +178,16 @@ static inline u64 intel_partial_get_page_offset(const struct intel_partial_info
> >> 	return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
> >> }
> >>
> >>+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),
> >
> >One alternative to packing intel_partial_info into a u64 is to use
> >
> >I915_GGTT_VIEW_PARTIAL = 12, /* ideal sizeof(struct intel_partial_info) */
> >
> >How horrible is that?
> 
> Really horrible, nightmare to debug. :)
> 
> You are happy with 12 packed bytes I gather so no need for it?

I'd prefer the u64, but currently baking:

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

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