Re: [git pull] drm: previous pull req + 1.

Eric Anholt
2009-07-10T18:03:46Z

[-- Attachment #1: Type: text/plain, Size: 4299 bytes --]

On Mon, 2009-06-29 at 08:57 +0100, Chris Wilson wrote:
> On Mon, 2009-06-22 at 21:09 +0300, Maxim Levitsky wrote:
> > Nope, same thing.
> >
> > I use commit 87ef92092fd092936535ba057ee19b97bb6a709a + this patch
> > Note that GE doesn't hang the system when maximizing it.
> >
> > It is for sure tiled textures accessed incorrectly, same behavior
> > observed in many games (they still run though)
>
> Sorry for the delay, I was travelling last week and was sure I had
> nailed the problem. Aside from the missing flush on i965 when a batch
> buffer might be using fenced commands, the only other issue I found was
> that we did not zap the mapping range on clear - which could also cause
> tiling errors if textures were being written via a GTT mmap.
>
> So please can you try this patch:
>
> >From 20b7c9322914213bb589035afbbc03faf1ae3bf0 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Mon, 29 Jun 2009 08:45:31 +0100
> Subject: [PATCH] drm/i915: Remove mappings on clearing fence register
>
> As the fence register is not locked whilst the user has mmaped the buffer
> through the GTT, in order for the buffer to reacquire a fence register we
> need to cause a fresh page-fault on the next user access. In order to
> cause the page fault, we zap the current mapping on clearing the register.
> We also ensure that all potential outstanding access via the fence
> register is flushed before release as well.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 41 ++++++++++++++++++--------------------
> 1 files changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 685a876..6dc74c8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1946,12 +1946,6 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
> obj_priv->agp_mem = NULL;
> }
>
> -
> - /* blow away mappings if mapped through GTT */
> - if (obj_priv->mmap_offset && dev->dev_mapping)
> - unmap_mapping_range(dev->dev_mapping,
> - obj_priv->mmap_offset, obj->size, 1);
> -

Err, so now untiled things wouldn't fault to rebind? Seems wrong.

> @@ -2456,6 +2451,11 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
>
> dev_priv->fence_regs[obj_priv->fence_reg].obj = NULL;
> obj_priv->fence_reg = I915_FENCE_REG_NONE;
> +
> + /* blow away mappings if mapped through GTT */
> + if (obj_priv->mmap_offset && dev->dev_mapping)
> + unmap_mapping_range(dev->dev_mapping,
> + obj_priv->mmap_offset, obj->size, 1);
> }

This part seems good.

>
> /**
> @@ -2469,27 +2469,24 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
> int
> i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
> {
> - struct drm_device *dev = obj->dev;
> struct drm_i915_gem_object *obj_priv = obj->driver_private;
> + int ret;
>
> if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
> return 0;
>
> - /* On the i915, GPU access to tiled buffers is via a fence,
> - * therefore we must wait for any outstanding access to complete
> - * before clearing the fence.
> + /* If there is outstanding activity on the buffer whilst it holds
> + * a fence register we must assume that it requires that fence for
> + * correct operation. Therefore we must wait for any outstanding
> + * access to complete before clearing the fence.
> */
> - if (!IS_I965G(dev)) {
> - int ret;
> -
> - i915_gem_object_flush_gpu_write_domain(obj);
> - i915_gem_object_flush_gtt_write_domain(obj);
> - ret = i915_gem_object_wait_rendering(obj);
> - if (ret != 0)
> - return ret;
> - }
> + i915_gem_object_flush_gpu_write_domain(obj);
> + i915_gem_object_flush_gtt_write_domain(obj);
> + ret = i915_gem_object_wait_rendering(obj);
> + if (ret != 0)
> + return ret;
>
> - i915_gem_clear_fence_reg (obj);
> + i915_gem_clear_fence_reg(obj);
>
> return 0;
> }

This part doesn't make sense to me. There should be nothing in the 965
rendering path that's using fences. Did you identify something that
was?

--
Eric Anholt
eric@anholt.net eric.anholt@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]