| Message ID | 1307369924-3601-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 6 Jun 2011 15:18:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > case I915_PARAM_HAS_RELAXED_FENCING: > - value = 1; > + value = 2; This looks like a change in ABI to me. I think this means you want a new ioctl so that applications using the existing HAS_RELAXED_FENCING ioctl and expecting a boolean will continue to work. > 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); ... > - i915_gem_get_unfenced_gtt_alignment(obj); > + i915_gem_get_unfenced_gtt_alignment(obj, > + args->tiling_mode); This combination looks like a simple bug fix -- not using the new tiling mode when computing the required alignment. Can you separate out this From the power-of-two change?
On Mon, 06 Jun 2011 09:03:30 -0700, Keith Packard <keithp@keithp.com> wrote: > On Mon, 6 Jun 2011 15:18:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > case I915_PARAM_HAS_RELAXED_FENCING: > > - value = 1; > > + value = 2; > > This looks like a change in ABI to me. I think this means you want a new > ioctl so that applications using the existing HAS_RELAXED_FENCING ioctl > and expecting a boolean will continue to work. Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken with any per-surface tiling on gen2/gen3. > > 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); > ... > > - i915_gem_get_unfenced_gtt_alignment(obj); > > + i915_gem_get_unfenced_gtt_alignment(obj, > > + args->tiling_mode); > > This combination looks like a simple bug fix -- not using the new tiling > mode when computing the required alignment. Can you separate out this > From the power-of-two change? Yes, I can. But the simple bug fix doesn't fix anything without the other chunk. Do you still want it split? -Chris
On Mon, 06 Jun 2011 18:50:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken > with any per-surface tiling on gen2/gen3. Right, seems like we need to signal user space that tiling works now, which should involve a new ioctl of some form? > Yes, I can. But the simple bug fix doesn't fix anything without the other > chunk. Do you still want it split? Isn't it a general bug? We're asking for an alignment value using the wrong tiling mode when changing tiling modes.
On Mon, 06 Jun 2011 11:09:30 -0700, Keith Packard <keithp@keithp.com> wrote: > On Mon, 06 Jun 2011 18:50:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken > > with any per-surface tiling on gen2/gen3. > > Right, seems like we need to signal user space that tiling works now, > which should involve a new ioctl of some form? So PARAM_HAS_PER_SURFACE_TILING and re-enable fencing for render operations on gen2/3. > > Yes, I can. But the simple bug fix doesn't fix anything without the other > > chunk. Do you still want it split? > > Isn't it a general bug? We're asking for an alignment value using the > wrong tiling mode when changing tiling modes. Indeed. I just can't test the patch independently since the hangs look identical. -Chris
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 403e69f..1a5bd0f 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -771,7 +771,7 @@ static int i915_getparam(struct drm_device *dev, void *data, value = HAS_BLT(dev); break; case I915_PARAM_HAS_RELAXED_FENCING: - value = 1; + value = 2; break; case I915_PARAM_HAS_COHERENT_RINGS: value = 1; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b492779..a030e38 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1188,7 +1188,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); /* i915_gem_gtt.c */ void i915_gem_restore_gtt_mappings(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7ce3f35..896aac7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1430,35 +1430,32 @@ 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) + dev = obj->base.dev; + 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) || - (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 @@ -2752,7 +2749,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 281ad3d..c775f03 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. 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> Cc: stable@kernel.org --- drivers/gpu/drm/i915/i915_dma.c | 2 +- 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 ++- 4 files changed, 19 insertions(+), 19 deletions(-)