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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1247181066.19542.5.camel@gaiman.anholt.net>
Date:	Thu, 09 Jul 2009 16:11:06 -0700
From:	Eric Anholt <eric@...olt.net>
To:	Chris Wilson <chris@...is-wilson.co.uk>
Cc:	Maxim Levitsky <maximlevitsky@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Dave Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
	dri-devel@...ts.sf.net
Subject: Re: [git pull] drm: previous pull req + 1.

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@...is-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@...is-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@...olt.net                         eric.anholt@...el.com



Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)