| Message ID | 1453820013-3908-3-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 26, 2016 at 02:53:30PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Will be used from multiple callers in a following patch. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 91 ++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index af15c290c71d..dacf6a0013c5 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1772,6 +1772,53 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > return 0; > } > > +static int > +i915_gem_ret_to_vm_ret(struct drm_i915_private *dev_priv, int ret) > +{ > + switch (ret) { > + case -EIO: > + /* > + * We eat errors when the gpu is terminally wedged to avoid > + * userspace unduly crashing (gl has no provisions for mmaps to > + * fail). But any other -EIO isn't ours (e.g. swap in failure) > + * and so needs to be reported. > + */ > + if (!i915_terminally_wedged(&dev_priv->gpu_error)) { > + ret = VM_FAULT_SIGBUS; > + break; > + } > + case -EAGAIN: > + /* > + * EAGAIN means the gpu is hung and we'll wait for the error > + * handler to reset everything when re-faulting in > + * i915_mutex_lock_interruptible. > + */ > + case 0: > + case -ERESTARTSYS: > + case -EINTR: > + case -EBUSY: > + /* > + * EBUSY is ok: this just means that another thread > + * already did the job. > + */ > + ret = VM_FAULT_NOPAGE; > + break; > + case -ENOMEM: > + ret = VM_FAULT_OOM; > + break; > + case -ENOSPC: > + case -EFAULT: > + ret = VM_FAULT_SIGBUS; > + break; > + default: > + WARN_ONCE(ret, "unhandled error in page fault\n"); > + ret = VM_FAULT_SIGBUS; > + break; > + } So without having to pin (plus a few other similar changes), we only need to report -ENOMEM, -ENOSPC, -EIO (from shmemfs) and -EFAULT (get_pages). Given that I'd rather have the reasoning behind each explicit. -Chris
On 26/01/16 15:18, Chris Wilson wrote: > On Tue, Jan 26, 2016 at 02:53:30PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Will be used from multiple callers in a following patch. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 91 ++++++++++++++++++++++------------------- >> 1 file changed, 49 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index af15c290c71d..dacf6a0013c5 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1772,6 +1772,53 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, >> return 0; >> } >> >> +static int >> +i915_gem_ret_to_vm_ret(struct drm_i915_private *dev_priv, int ret) >> +{ >> + switch (ret) { >> + case -EIO: >> + /* >> + * We eat errors when the gpu is terminally wedged to avoid >> + * userspace unduly crashing (gl has no provisions for mmaps to >> + * fail). But any other -EIO isn't ours (e.g. swap in failure) >> + * and so needs to be reported. >> + */ >> + if (!i915_terminally_wedged(&dev_priv->gpu_error)) { >> + ret = VM_FAULT_SIGBUS; >> + break; >> + } >> + case -EAGAIN: >> + /* >> + * EAGAIN means the gpu is hung and we'll wait for the error >> + * handler to reset everything when re-faulting in >> + * i915_mutex_lock_interruptible. >> + */ >> + case 0: >> + case -ERESTARTSYS: >> + case -EINTR: >> + case -EBUSY: >> + /* >> + * EBUSY is ok: this just means that another thread >> + * already did the job. >> + */ >> + ret = VM_FAULT_NOPAGE; >> + break; >> + case -ENOMEM: >> + ret = VM_FAULT_OOM; >> + break; >> + case -ENOSPC: >> + case -EFAULT: >> + ret = VM_FAULT_SIGBUS; >> + break; >> + default: >> + WARN_ONCE(ret, "unhandled error in page fault\n"); >> + ret = VM_FAULT_SIGBUS; >> + break; >> + } > > So without having to pin (plus a few other similar changes), we only need > to report > > -ENOMEM, -ENOSPC, -EIO (from shmemfs) and -EFAULT (get_pages). > > Given that I'd rather have the reasoning behind each explicit. This is just existing code factored out so I am not sure what you are suggesting? Regards, Tvrtko
On Tue, Jan 26, 2016 at 04:24:46PM +0000, Tvrtko Ursulin wrote: > > > On 26/01/16 15:18, Chris Wilson wrote: > >On Tue, Jan 26, 2016 at 02:53:30PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Will be used from multiple callers in a following patch. > >> > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_gem.c | 91 ++++++++++++++++++++++------------------- > >> 1 file changed, 49 insertions(+), 42 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>index af15c290c71d..dacf6a0013c5 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem.c > >>+++ b/drivers/gpu/drm/i915/i915_gem.c > >>@@ -1772,6 +1772,53 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > >> return 0; > >> } > >> > >>+static int > >>+i915_gem_ret_to_vm_ret(struct drm_i915_private *dev_priv, int ret) > >>+{ > >>+ switch (ret) { > >>+ case -EIO: > >>+ /* > >>+ * We eat errors when the gpu is terminally wedged to avoid > >>+ * userspace unduly crashing (gl has no provisions for mmaps to > >>+ * fail). But any other -EIO isn't ours (e.g. swap in failure) > >>+ * and so needs to be reported. > >>+ */ > >>+ if (!i915_terminally_wedged(&dev_priv->gpu_error)) { > >>+ ret = VM_FAULT_SIGBUS; > >>+ break; > >>+ } > >>+ case -EAGAIN: > >>+ /* > >>+ * EAGAIN means the gpu is hung and we'll wait for the error > >>+ * handler to reset everything when re-faulting in > >>+ * i915_mutex_lock_interruptible. > >>+ */ > >>+ case 0: > >>+ case -ERESTARTSYS: > >>+ case -EINTR: > >>+ case -EBUSY: > >>+ /* > >>+ * EBUSY is ok: this just means that another thread > >>+ * already did the job. > >>+ */ > >>+ ret = VM_FAULT_NOPAGE; > >>+ break; > >>+ case -ENOMEM: > >>+ ret = VM_FAULT_OOM; > >>+ break; > >>+ case -ENOSPC: > >>+ case -EFAULT: > >>+ ret = VM_FAULT_SIGBUS; > >>+ break; > >>+ default: > >>+ WARN_ONCE(ret, "unhandled error in page fault\n"); > >>+ ret = VM_FAULT_SIGBUS; > >>+ break; > >>+ } > > > >So without having to pin (plus a few other similar changes), we only need > >to report > > > >-ENOMEM, -ENOSPC, -EIO (from shmemfs) and -EFAULT (get_pages). > > > >Given that I'd rather have the reasoning behind each explicit. > > This is just existing code factored out so I am not sure what you > are suggesting? I'm looking at the next user and wondering how many of these comments should (and do) apply. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index af15c290c71d..dacf6a0013c5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1772,6 +1772,53 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, return 0; } +static int +i915_gem_ret_to_vm_ret(struct drm_i915_private *dev_priv, int ret) +{ + switch (ret) { + case -EIO: + /* + * We eat errors when the gpu is terminally wedged to avoid + * userspace unduly crashing (gl has no provisions for mmaps to + * fail). But any other -EIO isn't ours (e.g. swap in failure) + * and so needs to be reported. + */ + if (!i915_terminally_wedged(&dev_priv->gpu_error)) { + ret = VM_FAULT_SIGBUS; + break; + } + case -EAGAIN: + /* + * EAGAIN means the gpu is hung and we'll wait for the error + * handler to reset everything when re-faulting in + * i915_mutex_lock_interruptible. + */ + case 0: + case -ERESTARTSYS: + case -EINTR: + case -EBUSY: + /* + * EBUSY is ok: this just means that another thread + * already did the job. + */ + ret = VM_FAULT_NOPAGE; + break; + case -ENOMEM: + ret = VM_FAULT_OOM; + break; + case -ENOSPC: + case -EFAULT: + ret = VM_FAULT_SIGBUS; + break; + default: + WARN_ONCE(ret, "unhandled error in page fault\n"); + ret = VM_FAULT_SIGBUS; + break; + } + + return ret; +} + /** * i915_gem_fault - fault a page into the GTT * @vma: VMA in question @@ -1902,49 +1949,9 @@ unpin: unlock: mutex_unlock(&dev->struct_mutex); out: - switch (ret) { - case -EIO: - /* - * We eat errors when the gpu is terminally wedged to avoid - * userspace unduly crashing (gl has no provisions for mmaps to - * fail). But any other -EIO isn't ours (e.g. swap in failure) - * and so needs to be reported. - */ - if (!i915_terminally_wedged(&dev_priv->gpu_error)) { - ret = VM_FAULT_SIGBUS; - break; - } - case -EAGAIN: - /* - * EAGAIN means the gpu is hung and we'll wait for the error - * handler to reset everything when re-faulting in - * i915_mutex_lock_interruptible. - */ - case 0: - case -ERESTARTSYS: - case -EINTR: - case -EBUSY: - /* - * EBUSY is ok: this just means that another thread - * already did the job. - */ - ret = VM_FAULT_NOPAGE; - break; - case -ENOMEM: - ret = VM_FAULT_OOM; - break; - case -ENOSPC: - case -EFAULT: - ret = VM_FAULT_SIGBUS; - break; - default: - WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", ret); - ret = VM_FAULT_SIGBUS; - break; - } - intel_runtime_pm_put(dev_priv); - return ret; + + return i915_gem_ret_to_vm_ret(dev_priv, ret); } /**