| Message ID | 1310200285-17692-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
It sucks a bit to see the relaxed fencing idea go down the toilet for pre-g33,
but bleh, hw sometimes just hates us.
Cc: stable@kernel.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwl.ch>
On Sat, 9 Jul 2011 09:31:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > uint32_t > -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj) > +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj, > + int tiling_mode) ... > + return i915_gem_get_gtt_size(obj); I think you want to pass the new tiling mode to this function rather than using the object's existing tiling mode. Seems like most of the issues could easily be explained by using the stale value when trying to change tiling modes.
On Mon, 18 Jul 2011 09:17:16 -0700, Keith Packard <keithp@keithp.com> wrote: Non-text part: multipart/signed > On Sat, 9 Jul 2011 09:31:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > uint32_t > > -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj) > > +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj, > > + int tiling_mode) > ... > > + return i915_gem_get_gtt_size(obj); > > I think you want to pass the new tiling mode to this function rather > than using the object's existing tiling mode. Seems like most of the > issues could easily be explained by using the stale value when trying to > change tiling modes. So help me, I made that change at one time as Daniel pointed that out. -Chris
On Mon, 18 Jul 2011 09:17:16 -0700, Keith Packard <keithp@keithp.com> wrote: Non-text part: multipart/signed > On Sat, 9 Jul 2011 09:31:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > uint32_t > > -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj) > > +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj, > > + int tiling_mode) > ... > > + return i915_gem_get_gtt_size(obj); > > I think you want to pass the new tiling mode to this function rather > than using the object's existing tiling mode. Seems like most of the > issues could easily be explained by using the stale value when trying to > change tiling modes. Actually, given that the only thing you need from the object is the size, it would be better to just create functions which take just the size and tiling mode and computes the gtt size required to map that. Like: i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev, size_t size, unsigned tiling_mode) i915_gem_get_gtt_size(struct drm_device *dev, size_t size, unsigned tiling_mode) Then you can be sure you're using the correct tiling mode in all of the computations.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2798d27..a9492d9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1216,7 +1216,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev); void i915_gem_release(struct drm_device *dev, struct drm_file *file); uint32_t -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj); +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj, + int tiling_mode); 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 e357c73..4fc9738 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1426,36 +1426,31 @@ i915_gem_get_gtt_alignment(struct drm_i915_gem_object *obj) * i915_gem_get_unfenced_gtt_alignment - return required GTT alignment for an * unfenced object * @obj: object to check + * @tiling_mode: tiling mode of the object * * Return the required GTT alignment for an object, only taking into account * unfenced tiled surface requirements. */ uint32_t -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj) +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj, + int tiling_mode) { struct drm_device *dev = obj->base.dev; - int tile_height; + + if (tiling_mode == I915_TILING_NONE) + return 4096; /* * Minimum alignment is 4k (GTT page size) for sane hw. */ - if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev) || - obj->tiling_mode == I915_TILING_NONE) + if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev)) return 4096; - /* - * Older chips need unfenced tiled buffers to be aligned to the left - * edge of an even tile row (where tile rows are counted as if the bo is - * placed in a fenced gtt region). + /* Previous hardware however needs to be aligned to a power-of-two + * tile height. The simplest method for determining this is to reuse + * the power-of-tile object size. */ - if (IS_GEN2(dev)) - tile_height = 16; - else if (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)) - tile_height = 32; - else - tile_height = 8; - - return tile_height * obj->stride * 2; + return i915_gem_get_gtt_size(obj); } int @@ -2781,7 +2776,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, fence_size = i915_gem_get_gtt_size(obj); fence_alignment = i915_gem_get_gtt_alignment(obj); - unfenced_alignment = i915_gem_get_unfenced_gtt_alignment(obj); + unfenced_alignment = + i915_gem_get_unfenced_gtt_alignment(obj, obj->tiling_mode); if (alignment == 0) alignment = map_and_fenceable ? fence_alignment : diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 82d70fd..8433b97 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -348,7 +348,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, /* Rebind if we need a change of alignment */ if (!obj->map_and_fenceable) { u32 unfenced_alignment = - i915_gem_get_unfenced_gtt_alignment(obj); + i915_gem_get_unfenced_gtt_alignment(obj, + args->tiling_mode); if (obj->gtt_offset & (unfenced_alignment - 1)) ret = i915_gem_object_unbind(obj); }
Align unfenced buffers on older hardware to the power-of-two object size. The docs suggest that it should be possible to align only to a power-of-two tile height, but using the already computed fence size is easier and always correct. We also have to make sure that we unbind misaligned buffers upon tiling changes. Reported-and-tested-by: Sitosfe Wheeler <sitsofe@yahoo.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36326 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 3 ++- drivers/gpu/drm/i915/i915_gem.c | 30 +++++++++++++----------------- drivers/gpu/drm/i915/i915_gem_tiling.c | 3 ++- 3 files changed, 17 insertions(+), 19 deletions(-)