From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 17/29] drm/i915: plumb VM into bind/unbind
 code
Date: Tue, 6 Aug 2013 20:54:52 +0200
Message-ID: <20130806185452.GE22035@phenom.ffwll.local>
References: <1375315222-4785-1-git-send-email-ben@bwidawsk.net>
	<1375315222-4785-18-git-send-email-ben@bwidawsk.net>
	<20130806182947.GB22035@phenom.ffwll.local>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mail-wi0-f181.google.com (mail-wi0-f181.google.com
	[209.85.212.181])
	by gabe.freedesktop.org (Postfix) with ESMTP id 5D763E678E
	for <intel-gfx@lists.freedesktop.org>;
	Tue,  6 Aug 2013 11:54:46 -0700 (PDT)
Received: by mail-wi0-f181.google.com with SMTP id en1so789966wid.14
	for <intel-gfx@lists.freedesktop.org>;
	Tue, 06 Aug 2013 11:54:45 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20130806182947.GB22035@phenom.ffwll.local>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.org

On Tue, Aug 06, 2013 at 08:29:47PM +0200, Daniel Vetter wrote:
> On Wed, Jul 31, 2013 at 05:00:10PM -0700, Ben Widawsky wrote:

[snip]

> > @@ -3643,33 +3672,39 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >  		    bool map_and_fenceable,
> >  		    bool nonblocking)
> >  {
> > +	struct i915_vma *vma;
> >  	int ret;
> >  
> >  	if (WARN_ON(obj->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> >  		return -EBUSY;
> >  
> > -	if (i915_gem_obj_ggtt_bound(obj)) {
> > -		if ((alignment && i915_gem_obj_ggtt_offset(obj) & (alignment - 1)) ||
> > +	WARN_ON(map_and_fenceable && !i915_is_ggtt(vm));
> > +
> > +	vma = i915_gem_obj_to_vma(obj, vm);
> > +
> > +	if (vma) {
> > +		if ((alignment &&
> > +		     vma->node.start & (alignment - 1)) ||
> >  		    (map_and_fenceable && !obj->map_and_fenceable)) {
> >  			WARN(obj->pin_count,
> >  			     "bo is already pinned with incorrect alignment:"
> >  			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
> >  			     " obj->map_and_fenceable=%d\n",
> > -			     i915_gem_obj_ggtt_offset(obj), alignment,
> > +			     i915_gem_obj_offset(obj, vm), alignment,
> >  			     map_and_fenceable,
> >  			     obj->map_and_fenceable);
> > -			ret = i915_gem_object_unbind(obj);
> > +			ret = i915_vma_unbind(vma);
> 
> If I read this correctly then we wont' call i915_gem_vma_destroy anymore
> and so will leak the vma. Is that correct? If so I guess a new slab for
> vmas could be handy to easily detect such bugs.

On re-reading all seems to be fine here since object_unbind was converted
to vma_unbind and so inherited the call to vma_destroy. So no leak here.
The other stuff isn't really critical, so I'll merge this patch (and the
next one).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch