[RFC,2/5] drm/i915: Extract code mapping errno to vm fault code
diff mbox

Message ID 1453820013-3908-3-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Jan. 26, 2016, 2:53 p.m. UTC
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(-)

Comments

Chris Wilson Jan. 26, 2016, 3:18 p.m. UTC | #1
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
Tvrtko Ursulin Jan. 26, 2016, 4:24 p.m. UTC | #2
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
Chris Wilson Jan. 26, 2016, 4:42 p.m. UTC | #3
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

Patch
diff mbox

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);
 }
 
 /**