Re: [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
Hi! > This patch causes lockups in firefox. They appear like non-fatal hangs > of the webpage contents, "fixable" with alt-tab or a background system > load. I have verified that reverting the commit 754a254427 on top of > current Linus tree fixes the problem. This is still broken in v5.2-rc3. I have also verified that the particular commit, if backported to v5.1 release, breaks firefox there too in the same way. (for reference:) commit 754a25442705c4f90e0d05f1a7bd303ffe700ca9 Author: Chris Wilson Date: Thu Mar 21 16:19:08 2019 + drm/i915: Skip object locking around a no-op set-domain ioctl
Re: [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
Hi! This patch causes lockups in firefox. They appear like non-fatal hangs of the webpage contents, "fixable" with alt-tab or a background system load. I have verified that reverting the commit 754a254427 on top of current Linus tree fixes the problem.
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
Quoting Ville Syrjälä (2019-03-22 14:28:37)
> On Thu, Mar 21, 2019 at 04:19:08PM +, Chris Wilson wrote:
> > If we are already in the desired write domain of a set-domain ioctl,
> > then there is nothing for us to do and we can quickly return back to
> > userspace, avoiding any lock contention. By recognising that the
> > write_domain is always a subset of the read_domains, and excluding the
> > no-op case of requiring 0 read_domains in the ioctl, we can infer if the
> > current write_domain matches the target read_domains, there is nothing
> > for us to do.
> >
> > Secondary aspect of this is that we undo the arbitrary fetching and
> > potential flushing of all pages for a set-domain(.write=CPU) call on a
> > fresh object -- which was introduced simply because we do the get-pages
> > before taking the struct_mutex.
> >
> > References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of
> > struct_mutex in set-domain")
> > Signed-off-by: Chris Wilson
> > Cc: Joonas Lahtinen
> > Cc: Matthew Auld
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 26 +++---
> > 1 file changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 72374e952e4b..36f557002005 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev,
> > void *data,
> > if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> > return -EINVAL;
> >
> > - /* Having something in the write domain implies it's in the read
> > + /*
> > + * Having something in the write domain implies it's in the read
> >* domain, and only that read domain. Enforce that in the request.
> >*/
> > - if (write_domain != 0 && read_domains != write_domain)
> > + if (write_domain && read_domains != write_domain)
> > return -EINVAL;
> >
> > + if (!read_domains)
> > + return 0;
>
> Hopefully no one is relying on read_domains==0 meaning cpu domain.
> That seems to be how this was handled before.
Hopefully not. None of the userspace has tried that, and I hope that the
idea of write_domain==0 meaning don't set a write_domain has conditioned
everyone into not using it.
> Or maybe we want -EIVNAL here?
Introducing new -EINVAL is also risky.
Hmm. So in case of trouble we should
if (!read_domains)
read_domain = DOMAIN_CPU.
Hopefully no one even notices such a subtle ABI change. Bugzilla watch
out!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
On Thu, Mar 21, 2019 at 04:19:08PM +, Chris Wilson wrote:
> If we are already in the desired write domain of a set-domain ioctl,
> then there is nothing for us to do and we can quickly return back to
> userspace, avoiding any lock contention. By recognising that the
> write_domain is always a subset of the read_domains, and excluding the
> no-op case of requiring 0 read_domains in the ioctl, we can infer if the
> current write_domain matches the target read_domains, there is nothing
> for us to do.
>
> Secondary aspect of this is that we undo the arbitrary fetching and
> potential flushing of all pages for a set-domain(.write=CPU) call on a
> fresh object -- which was introduced simply because we do the get-pages
> before taking the struct_mutex.
>
> References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of
> struct_mutex in set-domain")
> Signed-off-by: Chris Wilson
> Cc: Joonas Lahtinen
> Cc: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_gem.c | 26 +++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 72374e952e4b..36f557002005 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev,
> void *data,
> if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> return -EINVAL;
>
> - /* Having something in the write domain implies it's in the read
> + /*
> + * Having something in the write domain implies it's in the read
>* domain, and only that read domain. Enforce that in the request.
>*/
> - if (write_domain != 0 && read_domains != write_domain)
> + if (write_domain && read_domains != write_domain)
> return -EINVAL;
>
> + if (!read_domains)
> + return 0;
Hopefully no one is relying on read_domains==0 meaning cpu domain.
That seems to be how this was handled before.
Or maybe we want -EIVNAL here?
> +
> obj = i915_gem_object_lookup(file, args->handle);
> if (!obj)
> return -ENOENT;
>
> - /* Try to flush the object off the GPU without holding the lock.
> + /*
> + * Already in the desired target write domain? Nothing for us to!
> + *
> + * We apply a little bit of cunning here to catch a broader set of
> + * no-ops. If obj->write_domain is set, we must be in the same
> + * obj->read_domains, and only that domain. Therefore, if that
> + * obj->write_domain matches the request read_domains, we are
> + * already in the same read/write domain and can skip the operation,
> + * without having to further check the requested write_domain.
> + */
> + if (READ_ONCE(obj->write_domain) == read_domains) {
> + err = 0;
> + goto out;
> + }
Hard to argue with that logic.
Haven't paid too much attention to this area lately but this
makes sense to me.
Reviewed-by: Ville Syrjälä
> +
> + /*
> + * Try to flush the object off the GPU without holding the lock.
>* We will repeat the flush holding the lock in the normal manner
>* to catch cases where we are gazumped.
>*/
> --
> 2.20.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
On Thu, 21 Mar 2019 at 13:43, Chris Wilson wrote:
>
> If we are already in the desired write domain of a set-domain ioctl,
> then there is nothing for us to do and we can quickly return back to
> userspace, avoiding any lock contention. By recognising that the
> write_domain is always a subset of the read_domains, and excluding the
> no-op case of requiring 0 read_domains in the ioctl, we can infer if the
> current write_domain matches the target read_domains, there is nothing
> for us to do.
>
> Secondary aspect of this is that we undo the arbitrary fetching and
> potential flushing of all pages for a set-domain(.write=CPU) call on a
> fresh object -- which was introduced simply because we do the get-pages
> before taking the struct_mutex.
>
> References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of
> struct_mutex in set-domain")
> Signed-off-by: Chris Wilson
> Cc: Joonas Lahtinen
> Cc: Matthew Auld
> ---
> drivers/gpu/drm/i915/i915_gem.c | 26 +++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 72374e952e4b..36f557002005 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev,
> void *data,
> if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> return -EINVAL;
>
> - /* Having something in the write domain implies it's in the read
> + /*
> +* Having something in the write domain implies it's in the read
> * domain, and only that read domain. Enforce that in the request.
> */
> - if (write_domain != 0 && read_domains != write_domain)
> + if (write_domain && read_domains != write_domain)
> return -EINVAL;
>
> + if (!read_domains)
> + return 0;
> +
> obj = i915_gem_object_lookup(file, args->handle);
> if (!obj)
> return -ENOENT;
>
> - /* Try to flush the object off the GPU without holding the lock.
> + /*
> +* Already in the desired target write domain? Nothing for us to!
for us to do ?
Reviewed-by: Matthew Auld
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
If we are already in the desired write domain of a set-domain ioctl,
then there is nothing for us to do and we can quickly return back to
userspace, avoiding any lock contention. By recognising that the
write_domain is always a subset of the read_domains, and excluding the
no-op case of requiring 0 read_domains in the ioctl, we can infer if the
current write_domain matches the target read_domains, there is nothing
for us to do.
Secondary aspect of this is that we undo the arbitrary fetching and
potential flushing of all pages for a set-domain(.write=CPU) call on a
fresh object -- which was introduced simply because we do the get-pages
before taking the struct_mutex.
References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of
struct_mutex in set-domain")
Signed-off-by: Chris Wilson
Cc: Joonas Lahtinen
Cc: Matthew Auld
---
drivers/gpu/drm/i915/i915_gem.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 72374e952e4b..36f557002005 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void
*data,
if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
return -EINVAL;
- /* Having something in the write domain implies it's in the read
+ /*
+* Having something in the write domain implies it's in the read
* domain, and only that read domain. Enforce that in the request.
*/
- if (write_domain != 0 && read_domains != write_domain)
+ if (write_domain && read_domains != write_domain)
return -EINVAL;
+ if (!read_domains)
+ return 0;
+
obj = i915_gem_object_lookup(file, args->handle);
if (!obj)
return -ENOENT;
- /* Try to flush the object off the GPU without holding the lock.
+ /*
+* Already in the desired target write domain? Nothing for us to!
+*
+* We apply a little bit of cunning here to catch a broader set of
+* no-ops. If obj->write_domain is set, we must be in the same
+* obj->read_domains, and only that domain. Therefore, if that
+* obj->write_domain matches the request read_domains, we are
+* already in the same read/write domain and can skip the operation,
+* without having to further check the requested write_domain.
+*/
+ if (READ_ONCE(obj->write_domain) == read_domains) {
+ err = 0;
+ goto out;
+ }
+
+ /*
+* Try to flush the object off the GPU without holding the lock.
* We will repeat the flush holding the lock in the normal manner
* to catch cases where we are gazumped.
*/
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Skip object locking around a no-op set-domain ioctl
If we are already in the desired write domain of a set-domain ioctl,
then there is nothing for us to do and we can quickly return back to
userspace, avoiding any lock contention. By recognising that the
write_domain is always a subset of the read_domains, and excluding the
no-op case of requiring 0 read_domains in the ioctl, we can infer if the
current write_domain matches the target read_domains, there is nothing
for us to do.
Secondary aspect of this is that we undo the arbitrary fetching and
potential flushing of all pages for a set-domain(.write=CPU) call on a
fresh object -- which was introduced simply because we do the get-pages
before taking the struct_mutex.
References: 40e62d5d6be8 ("drm/i915: Acquire the backing storage outside of
struct_mutex in set-domain")
Signed-off-by: Chris Wilson
Cc: Joonas Lahtinen
Cc: Matthew Auld
---
drivers/gpu/drm/i915/i915_gem.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 72374e952e4b..36f557002005 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1484,17 +1484,37 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void
*data,
if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
return -EINVAL;
- /* Having something in the write domain implies it's in the read
+ /*
+* Having something in the write domain implies it's in the read
* domain, and only that read domain. Enforce that in the request.
*/
- if (write_domain != 0 && read_domains != write_domain)
+ if (write_domain && read_domains != write_domain)
return -EINVAL;
+ if (!read_domains)
+ return 0;
+
obj = i915_gem_object_lookup(file, args->handle);
if (!obj)
return -ENOENT;
- /* Try to flush the object off the GPU without holding the lock.
+ /*
+* Already in the desired target write domain? Nothing for us to!
+*
+* We apply a little bit of cunning here to catch a broader set of
+* no-ops. If obj->write_domain is set, we must be in the same
+* obj->read_domains, and only that domain. Therefore, if that
+* obj->write_domain matches the request read_domains, we are
+* already in the same read/write domain and can skip the operation,
+* without having to further check the requested write_domain.
+*/
+ if (READ_ONCE(obj->write_domain) == read_domains) {
+ err = 0;
+ goto out;
+ }
+
+ /*
+* Try to flush the object off the GPU without holding the lock.
* We will repeat the flush holding the lock in the normal manner
* to catch cases where we are gazumped.
*/
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
