* [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-06 15:43 ` Tvrtko Ursulin
2017-01-06 15:25 ` [PATCH 03/10] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
` (8 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
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 | 16 ++++++++--------
drivers/gpu/drm/i915/i915_vma.c | 9 ++++-----
drivers/gpu/drm/i915/i915_vma.h | 16 ++++++++++------
drivers/gpu/drm/i915/intel_display.c | 2 +-
6 files changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c1631190a60..43233069ccc8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1841,13 +1841,12 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
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,
- vma_pages(area) - view.params.partial.offset_size) - 1);
+ vma_pages(area) - view.partial.offset_size) - 1);
/* If the partial covers the entire object, just create a
* normal VMA.
@@ -1882,7 +1881,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 45ab54e71dc0..578de1d21b5d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3499,13 +3499,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);
@@ -3558,7 +3558,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 e2b8e5e1ed88..1376a53bb9a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -142,12 +142,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 */
@@ -181,13 +175,19 @@ 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;
+ };
};
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 65770b7109c0..284273056ea2 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -96,14 +96,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 e3b2b3b1e056..6d936009f919 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -196,15 +196,19 @@ i915_vma_compare(struct i915_vma *vma,
if (cmp)
return cmp;
+ 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);
+ BUILD_BUG_ON(offsetof(typeof(*view), rotated) !=
+ offsetof(typeof(*view), partial));
- return memcmp(&vma->ggtt_view.params,
- &view->params,
- sizeof(view->params));
+ 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 e2150a64860c..f5718f99d0d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2139,7 +2139,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
{
if (drm_rotation_90_or_270(rotation)) {
*view = i915_ggtt_view_rotated;
- view->params.rotated = to_intel_framebuffer(fb)->rot_info;
+ view->rotated = to_intel_framebuffer(fb)->rot_info;
} else {
*view = i915_ggtt_view_normal;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union
2017-01-06 15:25 ` [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
@ 2017-01-06 15:43 ` Tvrtko Ursulin
0 siblings, 0 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2017-01-06 15:43 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter
On 06/01/2017 15:25, Chris Wilson wrote:
> Save a lot of characters by making the union anonymous, with the
> side-effect of ignoring unset bits when comparing views.
I'll just repeat for the record, since the earlier discussion was on the
trybot list.
I think the anonymizing of the union should be a separate patch from
storing the struct sizes in the enum. The latter is what enables the
memcpy reduction trick, so the commit message is also incorrect in that
respect it is not the anonymizing which enables it.
Other than that I have no complaints on this patch, apart it depends on
the preceding struct partial packing which I think is possibly going to far.
Regards,
Tvrtko
> 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 | 16 ++++++++--------
> drivers/gpu/drm/i915/i915_vma.c | 9 ++++-----
> drivers/gpu/drm/i915/i915_vma.h | 16 ++++++++++------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 6 files changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2c1631190a60..43233069ccc8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1841,13 +1841,12 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
> 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,
> - vma_pages(area) - view.params.partial.offset_size) - 1);
> + vma_pages(area) - view.partial.offset_size) - 1);
>
> /* If the partial covers the entire object, just create a
> * normal VMA.
> @@ -1882,7 +1881,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 45ab54e71dc0..578de1d21b5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3499,13 +3499,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);
>
> @@ -3558,7 +3558,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 e2b8e5e1ed88..1376a53bb9a7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -142,12 +142,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 */
> @@ -181,13 +175,19 @@ 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;
> + };
> };
>
> 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 65770b7109c0..284273056ea2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -96,14 +96,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 e3b2b3b1e056..6d936009f919 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -196,15 +196,19 @@ i915_vma_compare(struct i915_vma *vma,
> if (cmp)
> return cmp;
>
> + 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);
> + BUILD_BUG_ON(offsetof(typeof(*view), rotated) !=
> + offsetof(typeof(*view), partial));
>
> - return memcmp(&vma->ggtt_view.params,
> - &view->params,
> - sizeof(view->params));
> + 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 e2150a64860c..f5718f99d0d4 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;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 03/10] drm/i915: Eliminate superfluous i915_ggtt_view_rotated
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
2017-01-06 15:25 ` [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-06 15:25 ` [PATCH 04/10] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
It is only being used to clear a struct and set the type, after which it
is overwritten. Since we no longer check the unset bits of the union,
skipping the clear is permissible.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
drivers/gpu/drm/i915/i915_gem_gtt.h | 1 -
drivers/gpu/drm/i915/intel_display.c | 5 ++---
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 578de1d21b5d..1164fc3d5bda 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -102,9 +102,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,
-};
int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
int enable_ppgtt)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 1376a53bb9a7..2006b83c6a96 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -191,7 +191,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 f5718f99d0d4..aec65a3025c2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2137,11 +2137,10 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
const struct drm_framebuffer *fb,
unsigned int rotation)
{
+ view->type = I915_GGTT_VIEW_NORMAL;
if (drm_rotation_90_or_270(rotation)) {
- *view = i915_ggtt_view_rotated;
+ view->type = I915_GGTT_VIEW_ROTATED;
view->rotated = to_intel_framebuffer(fb)->rot_info;
- } else {
- *view = i915_ggtt_view_normal;
}
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 04/10] drm/i915: Eliminate superfluous i915_ggtt_view_normal
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
2017-01-06 15:25 ` [PATCH 02/10] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
2017-01-06 15:25 ` [PATCH 03/10] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-06 15:25 ` [PATCH 05/10] drm/i915: Extact compute_partial_view() Chris Wilson
` (6 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
Since commit 058d88c4330f ("drm/i915: Track pinned VMA"), there is only
one user of i915_ggtt_view_normal rodate. Just treat NULL as no special
view in pin_to_display() like everywhere else.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ----
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 --
drivers/gpu/drm/i915/intel_overlay.c | 3 +--
4 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 43233069ccc8..51c009303c77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3499,7 +3499,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 1164fc3d5bda..05771556b960 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -99,10 +99,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,
-};
-
int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
int enable_ppgtt)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 2006b83c6a96..9ecf83cb04ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -190,8 +190,6 @@ struct i915_ggtt_view {
};
};
-extern const struct i915_ggtt_view i915_ggtt_view_normal;
-
enum i915_cache_level;
struct i915_vma;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 4473a611c664..0608fad7f593 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -811,8 +811,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
if (ret != 0)
return ret;
- vma = i915_gem_object_pin_to_display_plane(new_bo, 0,
- &i915_ggtt_view_normal);
+ vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
if (IS_ERR(vma))
return PTR_ERR(vma);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 05/10] drm/i915: Extact compute_partial_view()
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (2 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 04/10] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 11:39 ` Joonas Lahtinen
2017-01-06 15:25 ` [PATCH 06/10] drm/i915: Clip the partial view against the object not vma Chris Wilson
` (5 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
In order to reuse the partial view for selftesting, extract the common
function for computing the view.
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 | 49 +++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 51c009303c77..5c5cf31ef40f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1757,6 +1757,33 @@ int i915_gem_mmap_gtt_version(void)
return 1;
}
+static inline struct i915_ggtt_view
+compute_partial_view(struct drm_i915_gem_object *obj,
+ struct vm_area_struct *area,
+ pgoff_t page_offset,
+ unsigned int chunk)
+{
+ struct i915_ggtt_view view;
+
+ if (i915_gem_object_is_tiled(obj))
+ chunk = roundup(chunk, tile_row_pages(obj));
+
+ view.type = I915_GGTT_VIEW_PARTIAL;
+ 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,
+ vma_pages(area) - view.partial.offset_size) - 1);
+
+ /* If the partial covers the entire object, just create a
+ * normal VMA.
+ */
+ if (chunk >= obj->base.size >> PAGE_SHIFT)
+ view.type = I915_GGTT_VIEW_NORMAL;
+
+ return view;
+}
+
/**
* i915_gem_fault - fault a page into the GTT
* @area: CPU VMA in question
@@ -1833,26 +1860,10 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
/* Now pin it into the GTT as needed */
vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, flags);
if (IS_ERR(vma)) {
- struct i915_ggtt_view view;
- unsigned int chunk;
-
/* Use a partial view if it is bigger than available space */
- chunk = MIN_CHUNK_PAGES;
- if (i915_gem_object_is_tiled(obj))
- chunk = roundup(chunk, tile_row_pages(obj));
-
- view.type = I915_GGTT_VIEW_PARTIAL;
- 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,
- vma_pages(area) - view.partial.offset_size) - 1);
-
- /* If the partial covers the entire object, just create a
- * normal VMA.
- */
- if (chunk >= obj->base.size >> PAGE_SHIFT)
- view.type = I915_GGTT_VIEW_NORMAL;
+ struct i915_ggtt_view view =
+ compute_partial_view(obj, area,
+ page_offset, MIN_CHUNK_PAGES);
/* Userspace is now writing through an untracked VMA, abandon
* all hope that the hardware is able to track future writes.
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 06/10] drm/i915: Clip the partial view against the object not vma
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (3 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 05/10] drm/i915: Extact compute_partial_view() Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 14:08 ` Joonas Lahtinen
2017-01-06 15:25 ` [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row Chris Wilson
` (4 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
The VMA is later clipped against the vm_area_struct before insertion of
the faulting PTE so we are free to create the partial view as we desire.
If we use the object as the extents rather than the area, this partial
can then be used for other areas.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5c5cf31ef40f..981227cabc58 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1759,7 +1759,6 @@ int i915_gem_mmap_gtt_version(void)
static inline struct i915_ggtt_view
compute_partial_view(struct drm_i915_gem_object *obj,
- struct vm_area_struct *area,
pgoff_t page_offset,
unsigned int chunk)
{
@@ -1773,7 +1772,7 @@ compute_partial_view(struct drm_i915_gem_object *obj,
view.partial.offset_size =
(view.partial.offset_size << INTEL_PARTIAL_SIZE_BITS) |
(min_t(unsigned int, chunk,
- vma_pages(area) - view.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.
@@ -1862,8 +1861,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
if (IS_ERR(vma)) {
/* Use a partial view if it is bigger than available space */
struct i915_ggtt_view view =
- compute_partial_view(obj, area,
- page_offset, MIN_CHUNK_PAGES);
+ compute_partial_view(obj, page_offset, MIN_CHUNK_PAGES);
/* Userspace is now writing through an untracked VMA, abandon
* all hope that the hardware is able to track future writes.
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (4 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 06/10] drm/i915: Clip the partial view against the object not vma Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 13:21 ` Joonas Lahtinen
2017-01-06 15:25 ` [PATCH 08/10] drm/i915: Replace WARNs in fence register writes with extensive asserts Chris Wilson
` (3 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
Ensure the view occupies the full tile row so that reads/writes into the
VMA do not escape (via fenced detiling) into neighbouring objects - we
will pad the object with scratch pages to satisfy the fence. This
applies the lazy-tiling we employed on gen2/3 to gen4+.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 5 +++--
drivers/gpu/drm/i915/i915_gem.c | 27 +++++++++++++++++++--------
drivers/gpu/drm/i915/i915_gem_tiling.c | 18 +++++++++---------
drivers/gpu/drm/i915/i915_vma.c | 10 ++++++++--
4 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f66eeedecf97..9a70a25a5ba4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3361,9 +3361,10 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file);
void i915_gem_release(struct drm_device *dev, struct drm_file *file);
u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u64 size,
- int tiling_mode);
+ int tiling_mode, unsigned int stride);
u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
- int tiling_mode, bool fenced);
+ int tiling_mode, unsigned int stride,
+ bool fenced);
int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 981227cabc58..dc200c465ff7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2038,21 +2038,29 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
* @dev_priv: i915 device
* @size: object size
* @tiling_mode: tiling mode
+ * @stride: tiling stride
*
* Return the required global GTT size for an object, taking into account
* potential fence register mapping.
*/
u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
- u64 size, int tiling_mode)
+ u64 size, int tiling_mode, unsigned int stride)
{
u64 ggtt_size;
- GEM_BUG_ON(size == 0);
+ GEM_BUG_ON(!size);
- if (INTEL_GEN(dev_priv) >= 4 ||
- tiling_mode == I915_TILING_NONE)
+ if (tiling_mode == I915_TILING_NONE)
return size;
+ GEM_BUG_ON(!stride);
+
+ if (INTEL_GEN(dev_priv) >= 4) {
+ stride *= tiling_mode == I915_TILING_Y ? 32 : 8;
+ GEM_BUG_ON(stride & 4095);
+ return roundup(size, stride);
+ }
+
/* Previous chips need a power-of-two fence region when tiling */
if (IS_GEN3(dev_priv))
ggtt_size = 1024*1024;
@@ -2070,15 +2078,17 @@ u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
* @dev_priv: i915 device
* @size: object size
* @tiling_mode: tiling mode
+ * @stride: tiling stride
* @fenced: is fenced alignment required or not
*
* Return the required global GTT alignment for an object, taking into account
* potential fence register mapping.
*/
u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
- int tiling_mode, bool fenced)
+ int tiling_mode, unsigned int stride,
+ bool fenced)
{
- GEM_BUG_ON(size == 0);
+ GEM_BUG_ON(!size);
/*
* Minimum alignment is 4k (GTT page size), but might be greater
@@ -2093,7 +2103,7 @@ u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
* Previous chips need to be aligned to the size of the smallest
* fence register that can contain the object.
*/
- return i915_gem_get_ggtt_size(dev_priv, size, tiling_mode);
+ return i915_gem_get_ggtt_size(dev_priv, size, tiling_mode, stride);
}
static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -3715,7 +3725,8 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
u32 fence_size;
fence_size = i915_gem_get_ggtt_size(dev_priv, vma->size,
- i915_gem_object_get_tiling(obj));
+ i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj));
/* If the required space is larger than the available
* aperture, we will not able to find a slot for the
* object and unbinding the object now will be in
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 62ad375de6ca..51b8d71876b7 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -117,7 +117,8 @@ i915_tiling_ok(struct drm_i915_private *dev_priv,
return true;
}
-static bool i915_vma_fence_prepare(struct i915_vma *vma, int tiling_mode)
+static bool i915_vma_fence_prepare(struct i915_vma *vma,
+ int tiling_mode, unsigned int stride)
{
struct drm_i915_private *dev_priv = vma->vm->i915;
u32 size;
@@ -133,7 +134,7 @@ static bool i915_vma_fence_prepare(struct i915_vma *vma, int tiling_mode)
return false;
}
- size = i915_gem_get_ggtt_size(dev_priv, vma->size, tiling_mode);
+ size = i915_gem_get_ggtt_size(dev_priv, vma->size, tiling_mode, stride);
if (vma->node.size < size)
return false;
@@ -145,20 +146,17 @@ static bool i915_vma_fence_prepare(struct i915_vma *vma, int tiling_mode)
/* Make the current GTT allocation valid for the change in tiling. */
static int
-i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj, int tiling_mode)
+i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj,
+ int tiling_mode, unsigned int stride)
{
- struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
struct i915_vma *vma;
int ret;
if (tiling_mode == I915_TILING_NONE)
return 0;
- if (INTEL_GEN(dev_priv) >= 4)
- return 0;
-
list_for_each_entry(vma, &obj->vma_list, obj_link) {
- if (i915_vma_fence_prepare(vma, tiling_mode))
+ if (i915_vma_fence_prepare(vma, tiling_mode, stride))
continue;
ret = i915_vma_unbind(vma);
@@ -255,7 +253,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
* whilst executing a fenced command for an untiled object.
*/
- err = i915_gem_object_fence_prepare(obj, args->tiling_mode);
+ err = i915_gem_object_fence_prepare(obj,
+ args->tiling_mode,
+ args->stride);
if (!err) {
struct i915_vma *vma;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 284273056ea2..6c6965cc3747 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -282,11 +282,14 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
fence_size = i915_gem_get_ggtt_size(dev_priv,
vma->size,
- i915_gem_object_get_tiling(obj));
+ i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj));
fence_alignment = i915_gem_get_ggtt_alignment(dev_priv,
vma->size,
i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj),
true);
+ GEM_BUG_ON(!is_power_of_2(fence_alignment));
fenceable = (vma->node.size == fence_size &&
(vma->node.start & (fence_alignment - 1)) == 0);
@@ -368,12 +371,15 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
size = max(size, vma->size);
if (flags & PIN_MAPPABLE)
size = i915_gem_get_ggtt_size(dev_priv, size,
- i915_gem_object_get_tiling(obj));
+ i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj));
alignment = max(max(alignment, vma->display_alignment),
i915_gem_get_ggtt_alignment(dev_priv, size,
i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj),
flags & PIN_MAPPABLE));
+ GEM_BUG_ON(!is_power_of_2(alignment));
start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row
2017-01-06 15:25 ` [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row Chris Wilson
@ 2017-01-09 13:21 ` Joonas Lahtinen
2017-01-09 13:30 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Joonas Lahtinen @ 2017-01-09 13:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Auld, Matthew
On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
> Ensure the view occupies the full tile row so that reads/writes into the
> VMA do not escape (via fenced detiling) into neighbouring objects - we
> will pad the object with scratch pages to satisfy the fence. This
> applies the lazy-tiling we employed on gen2/3 to gen4+.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
<SNIP>
> u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
> - u64 size, int tiling_mode)
> + u64 size, int tiling_mode, unsigned int stride)
> {
> u64 ggtt_size;
>
> - GEM_BUG_ON(size == 0);
> + GEM_BUG_ON(!size);
>
> - if (INTEL_GEN(dev_priv) >= 4 ||
> - tiling_mode == I915_TILING_NONE)
> + if (tiling_mode == I915_TILING_NONE)
> return size;
>
> + GEM_BUG_ON(!stride);
> +
> + if (INTEL_GEN(dev_priv) >= 4) {
> + stride *= tiling_mode == I915_TILING_Y ? 32 : 8;
(Split from tile_row_pages and) use tile_row_size() here? I915_TILING_Y
? 32 : 8 should really be in one place.
> + GEM_BUG_ON(stride & 4095);
~PAGE_MASK? Or even (4096 - 1) is better, so that it gets caught when
converting to non-hardcoded page sizes. CC'ing Matt.
> + return roundup(size, stride);
> + }
> +
> /* Previous chips need a power-of-two fence region when tiling */
> if (IS_GEN3(dev_priv))
> ggtt_size = 1024*1024;
Other than that,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row
2017-01-09 13:21 ` Joonas Lahtinen
@ 2017-01-09 13:30 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2017-01-09 13:30 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx, Auld, Matthew
On Mon, Jan 09, 2017 at 03:21:43PM +0200, Joonas Lahtinen wrote:
> On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
> > Ensure the view occupies the full tile row so that reads/writes into the
> > VMA do not escape (via fenced detiling) into neighbouring objects - we
> > will pad the object with scratch pages to satisfy the fence. This
> > applies the lazy-tiling we employed on gen2/3 to gen4+.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> <SNIP>
>
> > u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
> > - u64 size, int tiling_mode)
> > + u64 size, int tiling_mode, unsigned int stride)
> > {
> > u64 ggtt_size;
> >
> > - GEM_BUG_ON(size == 0);
> > + GEM_BUG_ON(!size);
> >
> > - if (INTEL_GEN(dev_priv) >= 4 ||
> > - tiling_mode == I915_TILING_NONE)
> > + if (tiling_mode == I915_TILING_NONE)
> > return size;
> >
> > + GEM_BUG_ON(!stride);
> > +
> > + if (INTEL_GEN(dev_priv) >= 4) {
> > + stride *= tiling_mode == I915_TILING_Y ? 32 : 8;
>
> (Split from tile_row_pages and) use tile_row_size() here? I915_TILING_Y
> ? 32 : 8 should really be in one place.
But, it's repeated so often, it must be good! :)
> > + GEM_BUG_ON(stride & 4095);
>
> ~PAGE_MASK? Or even (4096 - 1) is better, so that it gets caught when
> converting to non-hardcoded page sizes. CC'ing Matt.
I've a patch for later (after this one) to do conversions to
PAGE_SIZE, I915_GTT_PAGE_SIZE, I915_GTT_MIN_ALIGNMENT and use
IS_ALIGNED().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 08/10] drm/i915: Replace WARNs in fence register writes with extensive asserts
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (5 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 07/10] drm/i915: Align GGTT sizes to a fence tile row Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 13:37 ` Joonas Lahtinen
2017-01-06 15:25 ` [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma Chris Wilson
` (2 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
All of these conditions are prechecked by i915_tiling_ok() before we
allow setting the tiling/stride on the object and so we should never
fail asserting those conditions before writing the register.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 50 ++++++++++++++-----------------
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 775059e19ab9..d4dc8241fcc0 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -83,8 +83,13 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
u32 row_size = stride * (is_y_tiled ? 32 : 8);
u32 size = rounddown((u32)vma->node.size, row_size);
- val = ((vma->node.start + size - 4096) & 0xfffff000) << 32;
- val |= vma->node.start & 0xfffff000;
+ GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
+ GEM_BUG_ON(vma->node.start & 4095);
+ GEM_BUG_ON(vma->node.size & 4095);
+ GEM_BUG_ON(stride & 127);
+
+ val = (vma->node.start + size - 4096) << 32;
+ val |= vma->node.start;
val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
if (is_y_tiled)
val |= BIT(I965_FENCE_TILING_Y_SHIFT);
@@ -122,31 +127,24 @@ static void i915_write_fence_reg(struct drm_i915_fence_reg *fence,
unsigned int tiling = i915_gem_object_get_tiling(vma->obj);
bool is_y_tiled = tiling == I915_TILING_Y;
unsigned int stride = i915_gem_object_get_stride(vma->obj);
- int pitch_val;
- int tile_width;
- WARN((vma->node.start & ~I915_FENCE_START_MASK) ||
- !is_power_of_2(vma->node.size) ||
- (vma->node.start & (vma->node.size - 1)),
- "object 0x%08llx [fenceable? %d] not 1M or pot-size (0x%08llx) aligned\n",
- vma->node.start,
- i915_vma_is_map_and_fenceable(vma),
- vma->node.size);
+ GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
+ GEM_BUG_ON(vma->node.start & ~I915_FENCE_START_MASK);
+ GEM_BUG_ON(!is_power_of_2(vma->node.size));
+ GEM_BUG_ON(vma->node.start & (vma->node.size - 1));
if (is_y_tiled && HAS_128_BYTE_Y_TILING(fence->i915))
- tile_width = 128;
+ stride /= 128;
else
- tile_width = 512;
-
- /* Note: pitch better be a power of two tile widths */
- pitch_val = stride / tile_width;
- pitch_val = ffs(pitch_val) - 1;
+ stride /= 512;
+ GEM_BUG_ON(!is_power_of_2(stride));
val = vma->node.start;
if (is_y_tiled)
val |= BIT(I830_FENCE_TILING_Y_SHIFT);
val |= I915_FENCE_SIZE_BITS(vma->node.size);
- val |= pitch_val << I830_FENCE_PITCH_SHIFT;
+ val |= ilog2(stride) << I830_FENCE_PITCH_SHIFT;
+
val |= I830_FENCE_REG_VALID;
}
@@ -169,22 +167,18 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *fence,
unsigned int tiling = i915_gem_object_get_tiling(vma->obj);
bool is_y_tiled = tiling == I915_TILING_Y;
unsigned int stride = i915_gem_object_get_stride(vma->obj);
- u32 pitch_val;
-
- WARN((vma->node.start & ~I830_FENCE_START_MASK) ||
- !is_power_of_2(vma->node.size) ||
- (vma->node.start & (vma->node.size - 1)),
- "object 0x%08llx not 512K or pot-size 0x%08llx aligned\n",
- vma->node.start, vma->node.size);
- pitch_val = stride / 128;
- pitch_val = ffs(pitch_val) - 1;
+ GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
+ GEM_BUG_ON(vma->node.start & ~I830_FENCE_START_MASK);
+ GEM_BUG_ON(!is_power_of_2(vma->node.size));
+ GEM_BUG_ON(!is_power_of_2(stride / 128));
+ GEM_BUG_ON(vma->node.start & (vma->node.size - 1));
val = vma->node.start;
if (is_y_tiled)
val |= BIT(I830_FENCE_TILING_Y_SHIFT);
val |= I830_FENCE_SIZE_BITS(vma->node.size);
- val |= pitch_val << I830_FENCE_PITCH_SHIFT;
+ val |= ilog2(stride / 128) << I830_FENCE_PITCH_SHIFT;
val |= I830_FENCE_REG_VALID;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (6 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 08/10] drm/i915: Replace WARNs in fence register writes with extensive asserts Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 14:05 ` Joonas Lahtinen
2017-01-06 15:25 ` [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region Chris Wilson
2017-01-09 14:13 ` [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Joonas Lahtinen
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 7 ++--
drivers/gpu/drm/i915/i915_gem.c | 27 +++++---------
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 16 ++++----
drivers/gpu/drm/i915/i915_gem_tiling.c | 36 ++++++++++--------
drivers/gpu/drm/i915/i915_vma.c | 61 +++++++++++++++----------------
drivers/gpu/drm/i915/i915_vma.h | 3 ++
6 files changed, 72 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9a70a25a5ba4..ee651cb3b8a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3360,11 +3360,10 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
int i915_gem_open(struct drm_device *dev, struct drm_file *file);
void i915_gem_release(struct drm_device *dev, struct drm_file *file);
-u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u64 size,
+u32 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u32 size,
int tiling_mode, unsigned int stride);
-u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
- int tiling_mode, unsigned int stride,
- bool fenced);
+u32 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u32 size,
+ int tiling_mode, unsigned int stride);
int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dc200c465ff7..a55283da8fb1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2043,10 +2043,10 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
* Return the required global GTT size for an object, taking into account
* potential fence register mapping.
*/
-u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
- u64 size, int tiling_mode, unsigned int stride)
+u32 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
+ u32 size, int tiling_mode, unsigned int stride)
{
- u64 ggtt_size;
+ u32 ggtt_size;
GEM_BUG_ON(!size);
@@ -2079,14 +2079,12 @@ u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv,
* @size: object size
* @tiling_mode: tiling mode
* @stride: tiling stride
- * @fenced: is fenced alignment required or not
*
* Return the required global GTT alignment for an object, taking into account
* potential fence register mapping.
*/
-u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
- int tiling_mode, unsigned int stride,
- bool fenced)
+u32 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u32 size,
+ int tiling_mode, unsigned int stride)
{
GEM_BUG_ON(!size);
@@ -2094,9 +2092,7 @@ u64 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u64 size,
* Minimum alignment is 4k (GTT page size), but might be greater
* if a fence register is needed for the object.
*/
- if (INTEL_GEN(dev_priv) >= 4 ||
- (!fenced && (IS_G33(dev_priv) || IS_PINEVIEW(dev_priv))) ||
- tiling_mode == I915_TILING_NONE)
+ if (INTEL_GEN(dev_priv) >= 4 || tiling_mode == I915_TILING_NONE)
return 4096;
/*
@@ -3577,7 +3573,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
return;
if (--vma->obj->pin_display == 0)
- vma->display_alignment = 0;
+ vma->display_alignment = 4096;
/* Bump the LRU to try and avoid premature eviction whilst flipping */
if (!i915_vma_is_active(vma))
@@ -3722,11 +3718,6 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
return ERR_PTR(-ENOSPC);
if (flags & PIN_MAPPABLE) {
- u32 fence_size;
-
- fence_size = i915_gem_get_ggtt_size(dev_priv, vma->size,
- i915_gem_object_get_tiling(obj),
- i915_gem_object_get_stride(obj));
/* If the required space is larger than the available
* aperture, we will not able to find a slot for the
* object and unbinding the object now will be in
@@ -3734,7 +3725,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
* the object in and out of the Global GTT and
* waste a lot of cycles under the mutex.
*/
- if (fence_size > dev_priv->ggtt.mappable_end)
+ if (vma->fence_size > dev_priv->ggtt.mappable_end)
return ERR_PTR(-E2BIG);
/* If NONBLOCK is set the caller is optimistically
@@ -3753,7 +3744,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
* we could try to minimise harm to others.
*/
if (flags & PIN_NONBLOCK &&
- fence_size > dev_priv->ggtt.mappable_end / 2)
+ vma->fence_size > dev_priv->ggtt.mappable_end / 2)
return ERR_PTR(-ENOSPC);
}
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index d4dc8241fcc0..84ed59b6a45d 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -81,11 +81,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
bool is_y_tiled = tiling == I915_TILING_Y;
unsigned int stride = i915_gem_object_get_stride(vma->obj);
u32 row_size = stride * (is_y_tiled ? 32 : 8);
- u32 size = rounddown((u32)vma->node.size, row_size);
+ u32 size = rounddown((u32)vma->fence_size, row_size);
GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
GEM_BUG_ON(vma->node.start & 4095);
- GEM_BUG_ON(vma->node.size & 4095);
+ GEM_BUG_ON(vma->fence_size & 4095);
GEM_BUG_ON(stride & 127);
val = (vma->node.start + size - 4096) << 32;
@@ -130,8 +130,8 @@ static void i915_write_fence_reg(struct drm_i915_fence_reg *fence,
GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
GEM_BUG_ON(vma->node.start & ~I915_FENCE_START_MASK);
- GEM_BUG_ON(!is_power_of_2(vma->node.size));
- GEM_BUG_ON(vma->node.start & (vma->node.size - 1));
+ GEM_BUG_ON(!is_power_of_2(vma->fence_size));
+ GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
if (is_y_tiled && HAS_128_BYTE_Y_TILING(fence->i915))
stride /= 128;
@@ -142,7 +142,7 @@ static void i915_write_fence_reg(struct drm_i915_fence_reg *fence,
val = vma->node.start;
if (is_y_tiled)
val |= BIT(I830_FENCE_TILING_Y_SHIFT);
- val |= I915_FENCE_SIZE_BITS(vma->node.size);
+ val |= I915_FENCE_SIZE_BITS(vma->fence_size);
val |= ilog2(stride) << I830_FENCE_PITCH_SHIFT;
val |= I830_FENCE_REG_VALID;
@@ -170,14 +170,14 @@ static void i830_write_fence_reg(struct drm_i915_fence_reg *fence,
GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
GEM_BUG_ON(vma->node.start & ~I830_FENCE_START_MASK);
- GEM_BUG_ON(!is_power_of_2(vma->node.size));
+ GEM_BUG_ON(!is_power_of_2(vma->fence_size));
GEM_BUG_ON(!is_power_of_2(stride / 128));
- GEM_BUG_ON(vma->node.start & (vma->node.size - 1));
+ GEM_BUG_ON(vma->node.start & (vma->fence_size - 1));
val = vma->node.start;
if (is_y_tiled)
val |= BIT(I830_FENCE_TILING_Y_SHIFT);
- val |= I830_FENCE_SIZE_BITS(vma->node.size);
+ val |= I830_FENCE_SIZE_BITS(vma->fence_size);
val |= ilog2(stride / 128) << I830_FENCE_PITCH_SHIFT;
val |= I830_FENCE_REG_VALID;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 51b8d71876b7..23a896cd934f 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -120,25 +120,18 @@ i915_tiling_ok(struct drm_i915_private *dev_priv,
static bool i915_vma_fence_prepare(struct i915_vma *vma,
int tiling_mode, unsigned int stride)
{
- struct drm_i915_private *dev_priv = vma->vm->i915;
- u32 size;
+ struct drm_i915_private *i915 = vma->vm->i915;
+ u32 size, alignment;
if (!i915_vma_is_map_and_fenceable(vma))
return true;
- if (INTEL_GEN(dev_priv) == 3) {
- if (vma->node.start & ~I915_FENCE_START_MASK)
- return false;
- } else {
- if (vma->node.start & ~I830_FENCE_START_MASK)
- return false;
- }
-
- size = i915_gem_get_ggtt_size(dev_priv, vma->size, tiling_mode, stride);
+ size = i915_gem_get_ggtt_size(i915, vma->size, tiling_mode, stride);
if (vma->node.size < size)
return false;
- if (vma->node.start & (size - 1))
+ alignment = i915_gem_get_ggtt_alignment(i915, vma->size, tiling_mode, stride);
+ if (vma->node.start & (alignment - 1))
return false;
return true;
@@ -156,6 +149,9 @@ i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj,
return 0;
list_for_each_entry(vma, &obj->vma_list, obj_link) {
+ if (!i915_vma_is_ggtt(vma))
+ break;
+
if (i915_vma_fence_prepare(vma, tiling_mode, stride))
continue;
@@ -277,10 +273,18 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
mutex_unlock(&obj->mm.lock);
list_for_each_entry(vma, &obj->vma_list, obj_link) {
- if (!vma->fence)
- continue;
-
- vma->fence->dirty = true;
+ if (!i915_vma_is_ggtt(vma))
+ break;
+
+ vma->fence_size = i915_gem_get_ggtt_size(dev_priv, vma->size,
+ args->tiling_mode,
+ args->stride);
+ vma->fence_alignment = i915_gem_get_ggtt_alignment(dev_priv, vma->size,
+ args->tiling_mode,
+ args->stride);
+
+ if (vma->fence)
+ vma->fence->dirty = true;
}
obj->tiling_and_stride =
args->stride | args->tiling_mode;
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 6c6965cc3747..777ef92494e6 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -91,6 +91,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
vma->vm = vm;
vma->obj = obj;
vma->size = obj->base.size;
+ vma->display_alignment = 4096;
if (view) {
vma->ggtt_view = *view;
@@ -108,6 +109,17 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
}
if (i915_is_ggtt(vm)) {
+ GEM_BUG_ON(overflows_type(vma->size, u32));
+ vma->fence_size = i915_gem_get_ggtt_size(vm->i915, vma->size,
+ i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj));
+ GEM_BUG_ON(vma->fence_size & 4095);
+
+ vma->fence_alignment = i915_gem_get_ggtt_alignment(vm->i915, vma->size,
+ i915_gem_object_get_tiling(obj),
+ i915_gem_object_get_stride(obj));
+ GEM_BUG_ON(!is_power_of_2(vma->fence_alignment));
+
vma->flags |= I915_VMA_GGTT;
list_add(&vma->obj_link, &obj->vma_list);
} else {
@@ -275,34 +287,24 @@ i915_vma_misplaced(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
{
- struct drm_i915_gem_object *obj = vma->obj;
- struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
bool mappable, fenceable;
- u32 fence_size, fence_alignment;
- fence_size = i915_gem_get_ggtt_size(dev_priv,
- vma->size,
- i915_gem_object_get_tiling(obj),
- i915_gem_object_get_stride(obj));
- fence_alignment = i915_gem_get_ggtt_alignment(dev_priv,
- vma->size,
- i915_gem_object_get_tiling(obj),
- i915_gem_object_get_stride(obj),
- true);
- GEM_BUG_ON(!is_power_of_2(fence_alignment));
-
- fenceable = (vma->node.size == fence_size &&
- (vma->node.start & (fence_alignment - 1)) == 0);
-
- mappable = (vma->node.start + fence_size <=
- dev_priv->ggtt.mappable_end);
+ GEM_BUG_ON(!i915_vma_is_ggtt(vma));
+ GEM_BUG_ON(!vma->fence_size);
/*
* Explicitly disable for rotated VMA since the display does not
* need the fence and the VMA is not accessible to other users.
*/
- if (mappable && fenceable &&
- vma->ggtt_view.type != I915_GGTT_VIEW_ROTATED)
+ if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
+ return;
+
+ fenceable = (vma->node.size >= vma->fence_size &&
+ (vma->node.start & (vma->fence_alignment - 1)) == 0);
+
+ mappable = vma->node.start + vma->fence_size <= i915_vm_to_ggtt(vma->vm)->mappable_end;
+
+ if (mappable && fenceable)
vma->flags |= I915_VMA_CAN_FENCE;
else
vma->flags &= ~I915_VMA_CAN_FENCE;
@@ -369,17 +371,12 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
size = max(size, vma->size);
- if (flags & PIN_MAPPABLE)
- size = i915_gem_get_ggtt_size(dev_priv, size,
- i915_gem_object_get_tiling(obj),
- i915_gem_object_get_stride(obj));
-
- alignment = max(max(alignment, vma->display_alignment),
- i915_gem_get_ggtt_alignment(dev_priv, size,
- i915_gem_object_get_tiling(obj),
- i915_gem_object_get_stride(obj),
- flags & PIN_MAPPABLE));
- GEM_BUG_ON(!is_power_of_2(alignment));
+ alignment = max(alignment, vma->display_alignment);
+ if (flags & PIN_MAPPABLE) {
+ size = max_t(typeof(size), size, vma->fence_size);
+ alignment = max_t(typeof(alignment),
+ alignment, vma->fence_alignment);
+ }
start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 6d936009f919..8193ad34a1fe 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -55,6 +55,9 @@ struct i915_vma {
u64 size;
u64 display_alignment;
+ u32 fence_size;
+ u32 fence_alignment;
+
unsigned int flags;
/**
* How many users have pinned this object in GTT space. The following
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma
2017-01-06 15:25 ` [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma Chris Wilson
@ 2017-01-09 14:05 ` Joonas Lahtinen
2017-01-09 14:14 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Joonas Lahtinen @ 2017-01-09 14:05 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
Commit message missing.
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
<SNIP>
> @@ -3360,11 +3360,10 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> void i915_gem_release(struct drm_device *dev, struct drm_file *file);
>
> -u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u64 size,
> +u32 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u32 size,
We still seem to have some type bouncing going on.
> @@ -3577,7 +3573,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
> return;
>
> if (--vma->obj->pin_display == 0)
> - vma->display_alignment = 0;
> + vma->display_alignment = 4096;
Is there a case when the max() become zero? Do we have Bugzilla or is
this just preventive action.
> @@ -277,10 +273,18 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
> mutex_unlock(&obj->mm.lock);
>
> list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!vma->fence)
> - continue;
> -
> - vma->fence->dirty = true;
> + if (!i915_vma_is_ggtt(vma))
> + break;
> +
> + vma->fence_size = i915_gem_get_ggtt_size(dev_priv, vma->size,
> + args->tiling_mode,
> + args->stride);
It was called fence_size previously in the context which helped to
understand the variable name. I think without the context, it should
either be vma->ggtt_size or rename i915_gem_get_ggtt_size to fence_size
too.
> @@ -91,6 +91,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
> vma->vm = vm;
> vma->obj = obj;
> vma->size = obj->base.size;
> + vma->display_alignment = 4096;
Same question as above. But regardless;
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma
2017-01-09 14:05 ` Joonas Lahtinen
@ 2017-01-09 14:14 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2017-01-09 14:14 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Mon, Jan 09, 2017 at 04:05:01PM +0200, Joonas Lahtinen wrote:
> On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
>
> Commit message missing.
>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> <SNIP>
>
> > @@ -3360,11 +3360,10 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> > void i915_gem_release(struct drm_device *dev, struct drm_file *file);
> >
> > -u64 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u64 size,
> > +u32 i915_gem_get_ggtt_size(struct drm_i915_private *dev_priv, u32 size,
>
> We still seem to have some type bouncing going on.
Since we have restricted ggtt to only be u32, I'm correcting the
interfaces as I go.
> > @@ -3577,7 +3573,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
> > return;
> >
> > if (--vma->obj->pin_display == 0)
> > - vma->display_alignment = 0;
> > + vma->display_alignment = 4096;
>
> Is there a case when the max() become zero? Do we have Bugzilla or is
> this just preventive action.
It's a change to accommodate this patch to simplify the alignment
compuation as a series of max().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (7 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 09/10] drm/i915: Store required fence size/alignment for GGTT vma Chris Wilson
@ 2017-01-06 15:25 ` Chris Wilson
2017-01-09 11:46 ` Joonas Lahtinen
2017-01-09 14:13 ` [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Joonas Lahtinen
9 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2017-01-06 15:25 UTC (permalink / raw)
To: intel-gfx
Restricting the fence to the end of the previous tile-row breaks access
to the final portion of the object. On gen2/3 we employed lazy fencing
to pad out the fence with scratch page to provide access to the tail,
and now we also pad out the object on gen4+ we can apply the same fix.
Fixes: af1a7301c7cf ("drm/i915: Only fence tiled region of object.")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem_fence_reg.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 84ed59b6a45d..ff9ad6232591 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -78,20 +78,17 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
val = 0;
if (vma) {
unsigned int tiling = i915_gem_object_get_tiling(vma->obj);
- bool is_y_tiled = tiling == I915_TILING_Y;
unsigned int stride = i915_gem_object_get_stride(vma->obj);
- u32 row_size = stride * (is_y_tiled ? 32 : 8);
- u32 size = rounddown((u32)vma->fence_size, row_size);
GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
GEM_BUG_ON(vma->node.start & 4095);
GEM_BUG_ON(vma->fence_size & 4095);
GEM_BUG_ON(stride & 127);
- val = (vma->node.start + size - 4096) << 32;
+ val = (vma->node.start + vma->fence_size - 4096) << 32;
val |= vma->node.start;
val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
- if (is_y_tiled)
+ if (tiling == I915_TILING_Y)
val |= BIT(I965_FENCE_TILING_Y_SHIFT);
val |= I965_FENCE_REG_VALID;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region
2017-01-06 15:25 ` [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region Chris Wilson
@ 2017-01-09 11:46 ` Joonas Lahtinen
2017-01-09 12:09 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Joonas Lahtinen @ 2017-01-09 11:46 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
> Restricting the fence to the end of the previous tile-row breaks access
> to the final portion of the object. On gen2/3 we employed lazy fencing
> to pad out the fence with scratch page to provide access to the tail,
> and now we also pad out the object on gen4+ we can apply the same fix.
>
> Fixes: af1a7301c7cf ("drm/i915: Only fence tiled region of object.")
Need to prepare for backporting if this gets sent to stable, code has
changed quite a bit.
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
T-b would be great.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region
2017-01-09 11:46 ` Joonas Lahtinen
@ 2017-01-09 12:09 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2017-01-09 12:09 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Mon, Jan 09, 2017 at 01:46:52PM +0200, Joonas Lahtinen wrote:
> On pe, 2017-01-06 at 15:25 +0000, Chris Wilson wrote:
> > Restricting the fence to the end of the previous tile-row breaks access
> > to the final portion of the object. On gen2/3 we employed lazy fencing
> > to pad out the fence with scratch page to provide access to the tail,
> > and now we also pad out the object on gen4+ we can apply the same fix.
> >
> > Fixes: af1a7301c7cf ("drm/i915: Only fence tiled region of object.")
>
> Need to prepare for backporting if this gets sent to stable, code has
> changed quite a bit.
Exactly why I didn't propose a stable tag.
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> T-b would be great.
Testcase: igt/drv_selftest/objects
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64
2017-01-06 15:25 [PATCH 01/10] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
` (8 preceding siblings ...)
2017-01-06 15:25 ` [PATCH 10/10] drm/i915: Remove the rounding down of the gen4+ fence region Chris Wilson
@ 2017-01-09 14:13 ` Joonas Lahtinen
9 siblings, 0 replies; 21+ messages in thread
From: Joonas Lahtinen @ 2017-01-09 14:13 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On pe, 2017-01-06 at 15:25 +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).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Lets just not, it makes the code unnecessarily hard to read (and for
$DEITY's sake, I wrote the initial partial code). I think we have
enough bugs in the MMIO parts that have to deal with packed registers.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread