[PATCH] drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[PATCH] drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[Chris Wilson]

Actually transferring from shmemfs to the physically contiguous set of
pages should be wholly guarded by its obj->mm.lock!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index aff449807399..db168e395e8e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -574,7 +574,8 @@ int
 i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
 			    int align)
 {
-	int ret;
+	struct sg_table *pages;
+	int err;
 
 	if (align > obj->base.size)
 		return -EINVAL;
@@ -582,32 +583,44 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
 	if (obj->ops == &i915_gem_phys_ops)
 		return 0;
 
-	if (obj->mm.madv != I915_MADV_WILLNEED)
-		return -EFAULT;
-
-	if (obj->base.filp == NULL)
+	if (obj->ops != &i915_gem_object_ops)
 		return -EINVAL;
 
-	ret = i915_gem_object_unbind(obj);
-	if (ret)
-		return ret;
+	err = i915_gem_object_unbind(obj);
+	if (err)
+		return err;
+
+	mutex_lock(&obj->mm.lock);
+
+	if (obj->mm.quirked) {
+		err = -EFAULT;
+		goto err_unlock;
+	}
 
-	__i915_gem_object_put_pages(obj, I915_MM_NORMAL);
-	if (obj->mm.pages)
-		return -EBUSY;
+	if (obj->mm.mapping) {
+		err = -EBUSY;
+		goto err_unlock;
+	}
 
-	GEM_BUG_ON(obj->ops != &i915_gem_object_ops);
+	pages = obj->mm.pages;
 	obj->ops = &i915_gem_phys_ops;
 
-	ret = i915_gem_object_pin_pages(obj);
-	if (ret)
+	err = __i915_gem_object_get_pages(obj);
+	if (err)
 		goto err_xfer;
 
+	/* Perma-pin (until release) the physical set of pages */
+	__i915_gem_object_pin_pages(obj);
+
+	mutex_unlock(&obj->mm.lock);
 	return 0;
 
 err_xfer:
 	obj->ops = &i915_gem_object_ops;
-	return ret;
+	obj->mm.pages = pages;
+err_unlock:
+	mutex_unlock(&obj->mm.lock);
+	return err;
 }
 
 static int
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[Patchwork]

== Series Details ==

Series: drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately
URL   : https://patchwork.freedesktop.org/series/25469/
State : warning

== Summary ==

Series 25469v1 drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately
https://patchwork.freedesktop.org/api/1.0/series/25469/revisions/1/mbox/


24db004734623b21acb25f58084e5b8eadc4f908 drm-tip: 2017y-06m-08d-09h-22m-33s UTC integration manifest
0483b65 drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4907/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[Patchwork]

== Series Details ==

Series: drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately
URL   : https://patchwork.freedesktop.org/series/25469/
State : success

== Summary ==

Series 25469v1 drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately
https://patchwork.freedesktop.org/api/1.0/series/25469/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-r) fdo#100125
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:447s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:432s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:578s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:513s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:479s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:594s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:429s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:416s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:462s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:571s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:581s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:469s
fi-skl-6700hq    total:278  pass:228  dwarn:1   dfail:0   fail:27  skip:22  time:400s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:465s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:404s

24db004734623b21acb25f58084e5b8eadc4f908 drm-tip: 2017y-06m-08d-09h-22m-33s UTC integration manifest
0483b65 drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4907/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[Ville Syrjälä]

On Thu, Jun 08, 2017 at 11:53:44AM +0100, Chris Wilson wrote:
> Actually transferring from shmemfs to the physically contiguous set of
> pages should be wholly guarded by its obj->mm.lock!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index aff449807399..db168e395e8e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -574,7 +574,8 @@ int
>  i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>  			    int align)
>  {
> -	int ret;
> +	struct sg_table *pages;
> +	int err;
>  
>  	if (align > obj->base.size)
>  		return -EINVAL;
> @@ -582,32 +583,44 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>  	if (obj->ops == &i915_gem_phys_ops)
>  		return 0;
>  
> -	if (obj->mm.madv != I915_MADV_WILLNEED)
> -		return -EFAULT;
> -
> -	if (obj->base.filp == NULL)
> +	if (obj->ops != &i915_gem_object_ops)
>  		return -EINVAL;
>  
> -	ret = i915_gem_object_unbind(obj);
> -	if (ret)
> -		return ret;
> +	err = i915_gem_object_unbind(obj);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&obj->mm.lock);
> +
> +	if (obj->mm.quirked) {
> +		err = -EFAULT;
> +		goto err_unlock;
> +	}
>  
> -	__i915_gem_object_put_pages(obj, I915_MM_NORMAL);

Are we now leaking the old mm.pages?

> -	if (obj->mm.pages)
> -		return -EBUSY;
> +	if (obj->mm.mapping) {
> +		err = -EBUSY;
> +		goto err_unlock;
> +	}
>  
> -	GEM_BUG_ON(obj->ops != &i915_gem_object_ops);
> +	pages = obj->mm.pages;
>  	obj->ops = &i915_gem_phys_ops;
>  
> -	ret = i915_gem_object_pin_pages(obj);
> -	if (ret)
> +	err = __i915_gem_object_get_pages(obj);
> +	if (err)
>  		goto err_xfer;
>  
> +	/* Perma-pin (until release) the physical set of pages */
> +	__i915_gem_object_pin_pages(obj);
> +
> +	mutex_unlock(&obj->mm.lock);
>  	return 0;
>  
>  err_xfer:
>  	obj->ops = &i915_gem_object_ops;
> -	return ret;
> +	obj->mm.pages = pages;
> +err_unlock:
> +	mutex_unlock(&obj->mm.lock);
> +	return err;
>  }
>  
>  static int
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

[Chris Wilson]

Quoting Ville Syrjälä (2017-06-12 14:46:40)
> On Thu, Jun 08, 2017 at 11:53:44AM +0100, Chris Wilson wrote:
> > Actually transferring from shmemfs to the physically contiguous set of
> > pages should be wholly guarded by its obj->mm.lock!
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index aff449807399..db168e395e8e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -574,7 +574,8 @@ int
> >  i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> >                           int align)
> >  {
> > -     int ret;
> > +     struct sg_table *pages;
> > +     int err;
> >  
> >       if (align > obj->base.size)
> >               return -EINVAL;
> > @@ -582,32 +583,44 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> >       if (obj->ops == &i915_gem_phys_ops)
> >               return 0;
> >  
> > -     if (obj->mm.madv != I915_MADV_WILLNEED)
> > -             return -EFAULT;
> > -
> > -     if (obj->base.filp == NULL)
> > +     if (obj->ops != &i915_gem_object_ops)
> >               return -EINVAL;
> >  
> > -     ret = i915_gem_object_unbind(obj);
> > -     if (ret)
> > -             return ret;
> > +     err = i915_gem_object_unbind(obj);
> > +     if (err)
> > +             return err;
> > +
> > +     mutex_lock(&obj->mm.lock);
> > +
> > +     if (obj->mm.quirked) {
> > +             err = -EFAULT;
> > +             goto err_unlock;
> > +     }
> >  
> > -     __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
> 
> Are we now leaking the old mm.pages?

Hmm, I thought I had replace it with a direct call to free them on
completion. All that is needed is a call to
i915_gem_object_put_pages_gtt() which we can rely since we know this is
i915_gem_object_ops. Make that i915_gem_object_ops.put_pages().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx