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 <ch...@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 <ch...@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
e...@anholt.net                         eric.anh...@intel.com


Attachment: signature.asc
Description: This is a digitally signed message part

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Reply via email to