Re: [Intel-gfx] [PATCH 0/4] mm/gup, drm/i915: refactor gup_fast, convert to pin_user_pages()

2020-05-22 Thread Souptick Joarder 
Hi John,


On Tue, May 19, 2020 at 5:51 AM John Hubbard  wrote:
>
> This needs to go through Andrew's -mm tree, due to adding a new gup.c
> routine. However, I would really love to have some testing from the
> drm/i915 folks, because I haven't been able to run-time test that part
> of it.
>
> Otherwise, though, the series has passed my basic run time testing:
> some LTP tests, some xfs and etx4 non-destructive xfstests, and an
> assortment of other smaller ones: vm selftests, io_uring_register, a
> few more. But that's only on one particular machine. Also, cross-compile
> tests for half a dozen arches all pass.
>
> Details:
>
> In order to convert the drm/i915 driver from get_user_pages() to
> pin_user_pages(), a FOLL_PIN equivalent of __get_user_pages_fast() was
> required. That led to refactoring __get_user_pages_fast(), with the
> following goals:
>
> 1) As above: provide a pin_user_pages*() routine for drm/i915 to call,
>in place of __get_user_pages_fast(),
>
> 2) Get rid of the gup.c duplicate code for walking page tables with
>interrupts disabled. This duplicate code is a minor maintenance
>problem anyway.
>
> 3) Make it easy for an upcoming patch from Souptick, which aims to
>convert __get_user_pages_fast() to use a gup_flags argument, instead
>of a bool writeable arg.  Also, if this series looks good, we can
>ask Souptick to change the name as well, to whatever the consensus
>is. My initial recommendation is: get_user_pages_fast_only(), to
>match the new pin_user_pages_only().

Shall I hold my changes till 5.8-rc1 , when this series will appear upstream ?
>
> John Hubbard (4):
>   mm/gup: move __get_user_pages_fast() down a few lines in gup.c
>   mm/gup: refactor and de-duplicate gup_fast() code
>   mm/gup: introduce pin_user_pages_fast_only()
>   drm/i915: convert get_user_pages() --> pin_user_pages()
>
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c |  22 +--
>  include/linux/mm.h  |   3 +
>  mm/gup.c| 150 
>  3 files changed, 107 insertions(+), 68 deletions(-)
>
>
> base-commit: 642b151f45dd54809ea00ecd3976a56c1ec9b53d
> --
> 2.26.2
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] gpu: drm: i915: Change return type to vm_fault_t

2018-05-30 Thread Souptick Joarder 
On Mon, May 21, 2018 at 4:48 PM, Souptick Joarder  wrote:
> On Thu, May 17, 2018 at 10:40 AM, Souptick Joarder  
> wrote:
>> On Thu, May 17, 2018 at 12:48 AM, Chris Wilson  
>> wrote:
>>> Quoting Souptick Joarder (2018-05-16 20:12:20)
>>>> Use new return type vm_fault_t for fault handler. For
>>>> now, this is just documenting that the function returns
>>>> a VM_FAULT value rather than an errno. Once all instances
>>>> are converted, vm_fault_t will become a distinct type.
>>>>
>>>> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>>>>
>>>> Fixed one checkpatch.pl warning inside WARN_ONCE.
>>>>
>>>> Signed-off-by: Souptick Joarder 
>>>> ---
>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>> index dd89abd..732abdf 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>>>   * The current feature set supported by i915_gem_fault() and thus GTT 
>>>> mmaps
>>>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
>>>> i915_gem_mmap_gtt_version).
>>>>   */
>>>> -int i915_gem_fault(struct vm_fault *vmf)
>>>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>>>  {
>>>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>>>> struct vm_area_struct *area = vmf->vma;
>>>> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
>>>> struct i915_vma *vma;
>>>> pgoff_t page_offset;
>>>> unsigned int flags;
>>>> -   int ret;
>>>> +   int err;
>>>> +   vm_fault_t ret = VM_FAULT_SIGBUS;
>>>>
>>>> /* We don't use vmf->pgoff since that has the fake offset */
>>>> page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>>>> @@ -2027,8 +2028,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>>>> ret = VM_FAULT_SIGBUS;
>>>> break;
>>>> default:
>>>> -   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", 
>>>> ret);
>>>> -   ret = VM_FAULT_SIGBUS;
>>>> +   WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, 
>>>> err);
>>>> break;
>>>
>>> Even simpler would be to use e.g. return VM_FAULT_SIGBUS for each case
>>> above. No early initialisation of use-once variables allowing the
>>> compiler to do it's job. For a smaller patch, you can even skip the
>>> s/ret/err/
>>> -Chris
>>
>> Chris,
>> I prefer to use return once at the end of the function rather than
>> writing multiple return statement (Current code is doing similar).
>> But if you think other way, I can make that change :)
>
> If no further comment, we would like to get this patch
> in queue for 4.18

We need to get this patch in queue for 4.18.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] gpu: drm: i915: Change return type to vm_fault_t

2018-05-21 Thread Souptick Joarder 
On Thu, May 17, 2018 at 10:40 AM, Souptick Joarder <jrdr.li...@gmail.com> wrote:
> On Thu, May 17, 2018 at 12:48 AM, Chris Wilson <ch...@chris-wilson.co.uk> 
> wrote:
>> Quoting Souptick Joarder (2018-05-16 20:12:20)
>>> Use new return type vm_fault_t for fault handler. For
>>> now, this is just documenting that the function returns
>>> a VM_FAULT value rather than an errno. Once all instances
>>> are converted, vm_fault_t will become a distinct type.
>>>
>>> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>>>
>>> Fixed one checkpatch.pl warning inside WARN_ONCE.
>>>
>>> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
>>> ---
>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index dd89abd..732abdf 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
>>> i915_gem_mmap_gtt_version).
>>>   */
>>> -int i915_gem_fault(struct vm_fault *vmf)
>>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>>  {
>>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>>> struct vm_area_struct *area = vmf->vma;
>>> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
>>> struct i915_vma *vma;
>>> pgoff_t page_offset;
>>> unsigned int flags;
>>> -   int ret;
>>> +   int err;
>>> +   vm_fault_t ret = VM_FAULT_SIGBUS;
>>>
>>> /* We don't use vmf->pgoff since that has the fake offset */
>>> page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>>> @@ -2027,8 +2028,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>>> ret = VM_FAULT_SIGBUS;
>>> break;
>>> default:
>>> -   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", 
>>> ret);
>>> -   ret = VM_FAULT_SIGBUS;
>>> +   WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, 
>>> err);
>>> break;
>>
>> Even simpler would be to use e.g. return VM_FAULT_SIGBUS for each case
>> above. No early initialisation of use-once variables allowing the
>> compiler to do it's job. For a smaller patch, you can even skip the
>> s/ret/err/
>> -Chris
>
> Chris,
> I prefer to use return once at the end of the function rather than
> writing multiple return statement (Current code is doing similar).
> But if you think other way, I can make that change :)

If no further comment, we would like to get this patch
in queue for 4.18
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] gpu: drm: i915: Change return type to vm_fault_t

2018-05-16 Thread Souptick Joarder 
On Thu, May 17, 2018 at 12:48 AM, Chris Wilson <ch...@chris-wilson.co.uk> wrote:
> Quoting Souptick Joarder (2018-05-16 20:12:20)
>> Use new return type vm_fault_t for fault handler. For
>> now, this is just documenting that the function returns
>> a VM_FAULT value rather than an errno. Once all instances
>> are converted, vm_fault_t will become a distinct type.
>>
>> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>>
>> Fixed one checkpatch.pl warning inside WARN_ONCE.
>>
>> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
>> ---
>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index dd89abd..732abdf 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
>> i915_gem_mmap_gtt_version).
>>   */
>> -int i915_gem_fault(struct vm_fault *vmf)
>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>  {
>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>> struct vm_area_struct *area = vmf->vma;
>> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
>> struct i915_vma *vma;
>> pgoff_t page_offset;
>> unsigned int flags;
>> -   int ret;
>> +   int err;
>> +   vm_fault_t ret = VM_FAULT_SIGBUS;
>>
>> /* We don't use vmf->pgoff since that has the fake offset */
>> page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>> @@ -2027,8 +2028,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>> ret = VM_FAULT_SIGBUS;
>> break;
>> default:
>> -   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", 
>> ret);
>> -   ret = VM_FAULT_SIGBUS;
>> +   WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, err);
>> break;
>
> Even simpler would be to use e.g. return VM_FAULT_SIGBUS for each case
> above. No early initialisation of use-once variables allowing the
> compiler to do it's job. For a smaller patch, you can even skip the
> s/ret/err/
> -Chris

Chris,
I prefer to use return once at the end of the function rather than
writing multiple return statement (Current code is doing similar).
But if you think other way, I can make that change :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4] gpu: drm: i915: Change return type to vm_fault_t

2018-05-16 Thread Souptick Joarder 
Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

commit 1c8f422059ae ("mm: change return type to vm_fault_t")

Fixed one checkpatch.pl warning inside WARN_ONCE.

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
v2: Updated the change log

v3: Corrected variable name to err

v4: Fixed patchwork error by initialized ret

 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 38 +++---
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a42deeb..95b0d50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "i915_params.h"
 #include "i915_reg.h"
@@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
*dev_priv,
   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
-int i915_gem_fault(struct vm_fault *vmf);
+vm_fault_t i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 unsigned int flags,
 long timeout,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd89abd..732abdf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
  * The current feature set supported by i915_gem_fault() and thus GTT mmaps
  * is exposed via I915_PARAM_MMAP_GTT_VERSION (see i915_gem_mmap_gtt_version).
  */
-int i915_gem_fault(struct vm_fault *vmf)
+vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 {
 #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
struct vm_area_struct *area = vmf->vma;
@@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
struct i915_vma *vma;
pgoff_t page_offset;
unsigned int flags;
-   int ret;
+   int err;
+   vm_fault_t ret = VM_FAULT_SIGBUS;
 
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
@@ -1906,26 +1907,26 @@ int i915_gem_fault(struct vm_fault *vmf)
 * repeat the flush holding the lock in the normal manner to catch cases
 * where we are gazumped.
 */
-   ret = i915_gem_object_wait(obj,
+   err = i915_gem_object_wait(obj,
   I915_WAIT_INTERRUPTIBLE,
   MAX_SCHEDULE_TIMEOUT,
   NULL);
-   if (ret)
+   if (err)
goto err;
 
-   ret = i915_gem_object_pin_pages(obj);
-   if (ret)
+   err = i915_gem_object_pin_pages(obj);
+   if (err)
goto err;
 
intel_runtime_pm_get(dev_priv);
 
-   ret = i915_mutex_lock_interruptible(dev);
-   if (ret)
+   err = i915_mutex_lock_interruptible(dev);
+   if (err)
goto err_rpm;
 
/* Access to snoopable pages through the GTT is incoherent. */
if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(dev_priv)) {
-   ret = -EFAULT;
+   err = -EFAULT;
goto err_unlock;
}
 
@@ -1952,25 +1953,25 @@ int i915_gem_fault(struct vm_fault *vmf)
vma = i915_gem_object_ggtt_pin(obj, , 0, 0, PIN_MAPPABLE);
}
if (IS_ERR(vma)) {
-   ret = PTR_ERR(vma);
+   err = PTR_ERR(vma);
goto err_unlock;
}
 
-   ret = i915_gem_object_set_to_gtt_domain(obj, write);
-   if (ret)
+   err = i915_gem_object_set_to_gtt_domain(obj, write);
+   if (err)
goto err_unpin;
 
-   ret = i915_vma_pin_fence(vma);
-   if (ret)
+   err = i915_vma_pin_fence(vma);
+   if (err)
goto err_unpin;
 
/* Finally, remap it using the new GTT offset */
-   ret = remap_io_mapping(area,
+   err = remap_io_mapping(area,
   area->vm_start + (vma->ggtt_view.partial.offset 
<< PAGE_SHIFT),
   (ggtt->gmadr.start + vma->node.start) >> 
PAGE_SHIFT,
   min_t(u64, vma->size, area->vm_end - 
area->vm_start),
   >iomap);
-   if (ret)
+   if (err)
goto err_fence;
 
/* Mark as being mmapped into userspace for later revocation */
@@ -1991,7 +1992,7 @@ int i915_gem_fault(struct vm_fault *vmf)
intel_runtime_pm_put(dev_priv)

[Intel-gfx] [PATCH v3] gpu: drm: i915: Change return type to vm_fault_t

2018-05-16 Thread Souptick Joarder 
Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

commit 1c8f422059ae ("mm: change return type to vm_fault_t")

Fixed one checkpatch.pl warning inside WARN_ONCE.

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
v2: Updated the change log

v3: Corrected variable name to err

 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 37 +++--
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a42deeb..95b0d50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "i915_params.h"
 #include "i915_reg.h"
@@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
*dev_priv,
   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
-int i915_gem_fault(struct vm_fault *vmf);
+vm_fault_t i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 unsigned int flags,
 long timeout,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd89abd..b81b785 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
  * The current feature set supported by i915_gem_fault() and thus GTT mmaps
  * is exposed via I915_PARAM_MMAP_GTT_VERSION (see i915_gem_mmap_gtt_version).
  */
-int i915_gem_fault(struct vm_fault *vmf)
+vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 {
 #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
struct vm_area_struct *area = vmf->vma;
@@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
struct i915_vma *vma;
pgoff_t page_offset;
unsigned int flags;
-   int ret;
+   int err;
+   vm_fault_t ret;
 
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
@@ -1906,26 +1907,26 @@ int i915_gem_fault(struct vm_fault *vmf)
 * repeat the flush holding the lock in the normal manner to catch cases
 * where we are gazumped.
 */
-   ret = i915_gem_object_wait(obj,
+   err = i915_gem_object_wait(obj,
   I915_WAIT_INTERRUPTIBLE,
   MAX_SCHEDULE_TIMEOUT,
   NULL);
-   if (ret)
+   if (err)
goto err;
 
-   ret = i915_gem_object_pin_pages(obj);
-   if (ret)
+   err = i915_gem_object_pin_pages(obj);
+   if (err)
goto err;
 
intel_runtime_pm_get(dev_priv);
 
-   ret = i915_mutex_lock_interruptible(dev);
-   if (ret)
+   err = i915_mutex_lock_interruptible(dev);
+   if (err)
goto err_rpm;
 
/* Access to snoopable pages through the GTT is incoherent. */
if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(dev_priv)) {
-   ret = -EFAULT;
+   err = -EFAULT;
goto err_unlock;
}
 
@@ -1952,25 +1953,25 @@ int i915_gem_fault(struct vm_fault *vmf)
vma = i915_gem_object_ggtt_pin(obj, , 0, 0, PIN_MAPPABLE);
}
if (IS_ERR(vma)) {
-   ret = PTR_ERR(vma);
+   err = PTR_ERR(vma);
goto err_unlock;
}
 
-   ret = i915_gem_object_set_to_gtt_domain(obj, write);
-   if (ret)
+   err = i915_gem_object_set_to_gtt_domain(obj, write);
+   if (err)
goto err_unpin;
 
-   ret = i915_vma_pin_fence(vma);
-   if (ret)
+   err = i915_vma_pin_fence(vma);
+   if (err)
goto err_unpin;
 
/* Finally, remap it using the new GTT offset */
-   ret = remap_io_mapping(area,
+   err = remap_io_mapping(area,
   area->vm_start + (vma->ggtt_view.partial.offset 
<< PAGE_SHIFT),
   (ggtt->gmadr.start + vma->node.start) >> 
PAGE_SHIFT,
   min_t(u64, vma->size, area->vm_end - 
area->vm_start),
   >iomap);
-   if (ret)
+   if (err)
goto err_fence;
 
/* Mark as being mmapped into userspace for later revocation */
@@ -1991,7 +1992,7 @@ int i915_gem_fault(struct vm_fault *vmf)
intel_runtime_pm_put(dev_priv);
i915_gem_object_unpin_pages(obj);
 err:
-   switch (ret) {
+

Re: [Intel-gfx] [PATCH v2] gpu: drm: i915: Change return type to vm_fault_t

2018-05-15 Thread Souptick Joarder 
>> >> default:
>> >> -   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", 
>> >> ret);
>> >> +   WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, 
>> >> ret);
>> >
>> > I don't see point in %x (which should be 0x%x, really), why change it?
>>
>> ret will return VM_FAULT_FOO which is actually a defined as hex value
>> so %x will be more meaningful to print. I think WARN_ONCE() is less
>> meaningful to print inside default.
>> Better to remove it ? Agree ?
>
> Apart from we don't want to see ret.
> -Chris

I look back to the code again. Printing "err" instead of "ret" will
reproduce the
original behaviour of the code. Will send v2.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] gpu: drm: i915: Change return type to vm_fault_t

2018-05-15 Thread Souptick Joarder 
On Tue, May 15, 2018 at 7:19 PM, Joonas Lahtinen
<joonas.lahti...@linux.intel.com> wrote:
> Quoting Souptick Joarder (2018-04-17 22:02:02)
>> Use new return type vm_fault_t for fault handler. For
>> now, this is just documenting that the function returns
>> a VM_FAULT value rather than an errno. Once all instances
>> are converted, vm_fault_t will become a distinct type.
>>
>> Reference id -> 1c8f422059ae ("mm: change return type to
>> vm_fault_t")
>>
>> Fixed one checkpatch.pl warning inside WARN_ONCE.
>>
>> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>>  drivers/gpu/drm/i915/i915_gem.c | 37 +++--
>>  2 files changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index a42deeb..95b0d50 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -51,6 +51,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "i915_params.h"
>>  #include "i915_reg.h"
>> @@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
>> *dev_priv,
>>unsigned int flags);
>>  int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
>>  void i915_gem_resume(struct drm_i915_private *dev_priv);
>> -int i915_gem_fault(struct vm_fault *vmf);
>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf);
>>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>>  unsigned int flags,
>>  long timeout,
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index dd89abd..61816e8 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
>> i915_gem_mmap_gtt_version).
>>   */
>> -int i915_gem_fault(struct vm_fault *vmf)
>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>  {
>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>> struct vm_area_struct *area = vmf->vma;
>> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
>> struct i915_vma *vma;
>> pgoff_t page_offset;
>> unsigned int flags;
>> -   int ret;
>> +   int error;
>> +   vm_fault_t ret;
>
> Just add "err" under the existing variable as shorter one. Just "err" name
> should suffice just like elsewhere in the code.

There is a goto level "err" inside this function due to which variable
is defined as error. I think better to keep "error" instead of modifying
both variable and level name.

>>  * We eat errors when the gpu is terminally wedged to avoid
>> @@ -2027,7 +2028,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>> ret = VM_FAULT_SIGBUS;
>> break;
>> default:
>> -   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", 
>> ret);
>> +   WARN_ONCE(ret, "unhandled error in %s: %x\n", __func__, ret);
>
> I don't see point in %x (which should be 0x%x, really), why change it?

ret will return VM_FAULT_FOO which is actually a defined as hex value
so %x will be more meaningful to print. I think WARN_ONCE() is less
meaningful to print inside default.
Better to remove it ? Agree ?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] gpu: drm: i915: Change return type to vm_fault_t

2018-05-10 Thread Souptick Joarder 
On Wed, Apr 18, 2018 at 12:32 AM, Souptick Joarder <jrdr.li...@gmail.com> wrote:
> Use new return type vm_fault_t for fault handler. For
> now, this is just documenting that the function returns
> a VM_FAULT value rather than an errno. Once all instances
> are converted, vm_fault_t will become a distinct type.
>
> Reference id -> 1c8f422059ae ("mm: change return type to
> vm_fault_t")
>
> Fixed one checkpatch.pl warning inside WARN_ONCE.
>
> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>  drivers/gpu/drm/i915/i915_gem.c | 37 +++--
>  2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a42deeb..95b0d50 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -51,6 +51,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "i915_params.h"
>  #include "i915_reg.h"
> @@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
> *dev_priv,
>unsigned int flags);
>  int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
>  void i915_gem_resume(struct drm_i915_private *dev_priv);
> -int i915_gem_fault(struct vm_fault *vmf);
> +vm_fault_t i915_gem_fault(struct vm_fault *vmf);
>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>  unsigned int flags,
>  long timeout,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dd89abd..61816e8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
> i915_gem_mmap_gtt_version).
>   */
> -int i915_gem_fault(struct vm_fault *vmf)
> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>  {
>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
> struct vm_area_struct *area = vmf->vma;
> @@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
> struct i915_vma *vma;
> pgoff_t page_offset;
> unsigned int flags;
> -   int ret;
> +   int error;
> +   vm_fault_t ret;
>
> /* We don't use vmf->pgoff since that has the fake offset */
> page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
> @@ -1906,26 +1907,26 @@ int i915_gem_fault(struct vm_fault *vmf)
>  * repeat the flush holding the lock in the normal manner to catch 
> cases
>  * where we are gazumped.
>  */
> -   ret = i915_gem_object_wait(obj,
> +   error = i915_gem_object_wait(obj,
>I915_WAIT_INTERRUPTIBLE,
>MAX_SCHEDULE_TIMEOUT,
>NULL);
> -   if (ret)
> +   if (error)
> goto err;
>
> -   ret = i915_gem_object_pin_pages(obj);
> -   if (ret)
> +   error = i915_gem_object_pin_pages(obj);
> +   if (error)
> goto err;
>
> intel_runtime_pm_get(dev_priv);
>
> -   ret = i915_mutex_lock_interruptible(dev);
> -   if (ret)
> +   error = i915_mutex_lock_interruptible(dev);
> +   if (error)
> goto err_rpm;
>
> /* Access to snoopable pages through the GTT is incoherent. */
> if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(dev_priv)) {
> -   ret = -EFAULT;
> +   error = -EFAULT;
> goto err_unlock;
> }
>
> @@ -1952,25 +1953,25 @@ int i915_gem_fault(struct vm_fault *vmf)
> vma = i915_gem_object_ggtt_pin(obj, , 0, 0, 
> PIN_MAPPABLE);
> }
> if (IS_ERR(vma)) {
> -   ret = PTR_ERR(vma);
> +   error = PTR_ERR(vma);
> goto err_unlock;
> }
>
> -   ret = i915_gem_object_set_to_gtt_domain(obj, write);
> -   if (ret)
> +   error = i915_gem_object_set_to_gtt_domain(obj, write);
> +   if (error)
> goto err_unpin;
>
> -   ret = i915_vma_pin_fence(vma);
> -   if (ret)
> +   error = i915_vma_pin_fence(vma);
> +   if (error)
> goto err_unpin;
>
> /* Finally, remap it using the new GTT offset */
> -   ret = remap_io_mapping(area,
> +   error = remap_io_mapping(area,
>

Re: [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for gpu: drm: i915: Change return type to vm_fault_t (rev2)

2018-04-18 Thread Souptick Joarder 
On Wed, Apr 18, 2018 at 9:24 PM, Patchwork
 wrote:
> == Series Details ==
>
> Series: gpu: drm: i915: Change return type to vm_fault_t (rev2)
> URL   : https://patchwork.freedesktop.org/series/41893/
> State : warning
>
> == Summary ==
>
> $ dim checkpatch origin/drm-tip
> 045e647f1204 gpu: drm: i915: Change return type to vm_fault_t
> -:11: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit 
> <12+ chars of sha1> ("")' - ie: 'commit 1c8f422059ae ("mm: change 
> return type to vm_fault_t")'

Does it mean I need to send v3 with correction ?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] gpu: drm: i915: Change return type to vm_fault_t

2018-04-18 Thread Souptick Joarder 
Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Reference id -> 1c8f422059ae ("mm: change return type to
vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 15 ---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a42deeb..95b0d50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "i915_params.h"
 #include "i915_reg.h"
@@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
*dev_priv,
   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
-int i915_gem_fault(struct vm_fault *vmf);
+vm_fault_t i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 unsigned int flags,
 long timeout,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd89abd..bdac690 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
  * The current feature set supported by i915_gem_fault() and thus GTT mmaps
  * is exposed via I915_PARAM_MMAP_GTT_VERSION (see i915_gem_mmap_gtt_version).
  */
-int i915_gem_fault(struct vm_fault *vmf)
+vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 {
 #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
struct vm_area_struct *area = vmf->vma;
@@ -1895,6 +1895,7 @@ int i915_gem_fault(struct vm_fault *vmf)
pgoff_t page_offset;
unsigned int flags;
int ret;
+   vm_fault_t retval;

/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
@@ -2000,7 +2001,7 @@ int i915_gem_fault(struct vm_fault *vmf)
 * and so needs to be reported.
 */
if (!i915_terminally_wedged(_priv->gpu_error)) {
-   ret = VM_FAULT_SIGBUS;
+   retval = VM_FAULT_SIGBUS;
break;
}
case -EAGAIN:
@@ -2017,21 +2018,21 @@ int i915_gem_fault(struct vm_fault *vmf)
 * EBUSY is ok: this just means that another thread
 * already did the job.
 */
-   ret = VM_FAULT_NOPAGE;
+   retval = VM_FAULT_NOPAGE;
break;
case -ENOMEM:
-   ret = VM_FAULT_OOM;
+   retval = VM_FAULT_OOM;
break;
case -ENOSPC:
case -EFAULT:
-   ret = VM_FAULT_SIGBUS;
+   retval = VM_FAULT_SIGBUS;
break;
default:
WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", ret);
-   ret = VM_FAULT_SIGBUS;
+   retval = VM_FAULT_SIGBUS;
break;
}
-   return ret;
+   return retval;
 }

 static void __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
--
1.9.1

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

Re: [Intel-gfx] [PATCH] gpu: drm: i915: Change return type to vm_fault_t

2018-04-18 Thread Souptick Joarder 
Not exactly. The plan for these patches is to introduce new vm_fault_t type
in vm_operations_struct fault handlers. It's now available in 4.17-rc1. We will
push all the required drivers/filesystem changes through different maintainers
to linus tree. Once everything is converted into vm_fault_t type then Changing
it from a signed to an unsigned int causes GCC to warn about an assignment
from an incompatible type -- int foo(void) is incompatible with
unsigned int foo(void).

Please refer 1c8f422059ae ("mm: change return type to vm_fault_t") in 4.17-rc1.

On Tue, Apr 17, 2018 at 8:59 PM, Jani Nikula
<jani.nik...@linux.intel.com> wrote:
> On Tue, 17 Apr 2018, Souptick Joarder <jrdr.li...@gmail.com> wrote:
>> Use new return type vm_fault_t for fault handler. For
>> now, this is just documenting that the function returns
>> a VM_FAULT value rather than an errno. Once all instances
>> are converted, vm_fault_t will become a distinct type.
>>
>> Reference id -> 1c8f422059ae ("mm: change return type to
>> vm_fault_t")
>>
>> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>>  drivers/gpu/drm/i915/i915_gem.c | 15 ---
>>  2 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index a42deeb..95b0d50 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -51,6 +51,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "i915_params.h"
>>  #include "i915_reg.h"
>> @@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
>> *dev_priv,
>>  unsigned int flags);
>>  int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
>>  void i915_gem_resume(struct drm_i915_private *dev_priv);
>> -int i915_gem_fault(struct vm_fault *vmf);
>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf);
>>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>>unsigned int flags,
>>long timeout,
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index dd89abd..bdac690 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
>>   * The current feature set supported by i915_gem_fault() and thus GTT mmaps
>>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see 
>> i915_gem_mmap_gtt_version).
>>   */
>> -int i915_gem_fault(struct vm_fault *vmf)
>> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
>>  {
>>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
>>   struct vm_area_struct *area = vmf->vma;
>> @@ -1895,6 +1895,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>>   pgoff_t page_offset;
>>   unsigned int flags;
>>   int ret;
>> + vm_fault_t retval;
>
> What's the point of changing the name? An unnecessary change.
>
> BR,
> Jani.
>
>>
>>   /* We don't use vmf->pgoff since that has the fake offset */
>>   page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>> @@ -2000,7 +2001,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>>* and so needs to be reported.
>>*/
>>   if (!i915_terminally_wedged(_priv->gpu_error)) {
>> - ret = VM_FAULT_SIGBUS;
>> + retval = VM_FAULT_SIGBUS;
>>   break;
>>   }
>>   case -EAGAIN:
>> @@ -2017,21 +2018,21 @@ int i915_gem_fault(struct vm_fault *vmf)
>>* EBUSY is ok: this just means that another thread
>>* already did the job.
>>*/
>> - ret = VM_FAULT_NOPAGE;
>> + retval = VM_FAULT_NOPAGE;
>>   break;
>>   case -ENOMEM:
>> - ret = VM_FAULT_OOM;
>> + retval = VM_FAULT_OOM;
>>   break;
>>   case -ENOSPC:
>>   case -EFAULT:
>> - ret = VM_FAULT_SIGBUS;
>> + retval = VM_FAULT_SIGBUS;
>>   break;
>>   default:
>>   WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", ret);
>> - ret = VM_FAULT_SIGBUS;
>> + retval = VM_FAULT_SIGBUS;
>>   break;
>>   }
>> - return ret;
>> + return retval;
>>  }
>>
>>  static void __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
>> --
>> 1.9.1
>>
>
> --
> Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] gpu: drm: i915: Change return type to vm_fault_t

2018-04-18 Thread Souptick Joarder 
On 17-Apr-2018 9:45 PM, "Matthew Wilcox" <wi...@infradead.org> wrote:
>
> On Tue, Apr 17, 2018 at 09:14:32PM +0530, Souptick Joarder wrote:
> > Not exactly. The plan for these patches is to introduce new vm_fault_t
type
> > in vm_operations_struct fault handlers. It's now available in 4.17-rc1.
We will
> > push all the required drivers/filesystem changes through different
maintainers
> > to linus tree. Once everything is converted into vm_fault_t type then
Changing
> > it from a signed to an unsigned int causes GCC to warn about an
assignment
> > from an incompatible type -- int foo(void) is incompatible with
> > unsigned int foo(void).
> >
> > Please refer 1c8f422059ae ("mm: change return type to vm_fault_t") in
4.17-rc1.
>
> I think this patch would be clearer if you did
>
> -   int ret;
> +   int err;
> +   vm_fault_t ret;
>
> Then it would be clearer to the maintainer that you're splitting apart the
> VM_FAULT and errno codes.
>
> Sorry for not catching this during initial review.

Ok, I will make required changes and send v2. Sorry, even I missed this :)
>
> > On Tue, Apr 17, 2018 at 8:59 PM, Jani Nikula
> > <jani.nik...@linux.intel.com> wrote:
> > > On Tue, 17 Apr 2018, Souptick Joarder <jrdr.li...@gmail.com> wrote:
> > >> Use new return type vm_fault_t for fault handler. For
> > >> now, this is just documenting that the function returns
> > >> a VM_FAULT value rather than an errno. Once all instances
> > >> are converted, vm_fault_t will become a distinct type.
> > >>
> > >> Reference id -> 1c8f422059ae ("mm: change return type to
> > >> vm_fault_t")
> > >>
> > >> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
> > >>  drivers/gpu/drm/i915/i915_gem.c | 15 ---
> > >>  2 files changed, 10 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
> > >> index a42deeb..95b0d50 100644
> > >> --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> @@ -51,6 +51,7 @@
> > >>  #include 
> > >>  #include 
> > >>  #include 
> > >> +#include 
> > >>
> > >>  #include "i915_params.h"
> > >>  #include "i915_reg.h"
> > >> @@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct
drm_i915_private *dev_priv,
> > >>  unsigned int flags);
> > >>  int __must_check i915_gem_suspend(struct drm_i915_private
*dev_priv);
> > >>  void i915_gem_resume(struct drm_i915_private *dev_priv);
> > >> -int i915_gem_fault(struct vm_fault *vmf);
> > >> +vm_fault_t i915_gem_fault(struct vm_fault *vmf);
> > >>  int i915_gem_object_wait(struct drm_i915_gem_object *obj,
> > >>unsigned int flags,
> > >>long timeout,
> > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
> > >> index dd89abd..bdac690 100644
> > >> --- a/drivers/gpu/drm/i915/i915_gem.c
> > >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> > >> @@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
> > >>   * The current feature set supported by i915_gem_fault() and thus
GTT mmaps
> > >>   * is exposed via I915_PARAM_MMAP_GTT_VERSION (see
i915_gem_mmap_gtt_version).
> > >>   */
> > >> -int i915_gem_fault(struct vm_fault *vmf)
> > >> +vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> > >>  {
> > >>  #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
> > >>   struct vm_area_struct *area = vmf->vma;
> > >> @@ -1895,6 +1895,7 @@ int i915_gem_fault(struct vm_fault *vmf)
> > >>   pgoff_t page_offset;
> > >>   unsigned int flags;
> > >>   int ret;
> > >> + vm_fault_t retval;
> > >
> > > What's the point of changing the name? An unnecessary change.
> > >
> > > BR,
> > > Jani.
> > >
> > >>
> > >>   /* We don't use vmf->pgoff since that has the fake offset */
> > >>   page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
> > >> @@ -2000,7 +2001,7 @@ int i915_gem_fault(struct vm_fault *vmf)
> > >>

[Intel-gfx] [PATCH v2] gpu: drm: i915: Change return type to vm_fault_t

2018-04-18 Thread Souptick Joarder 
Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Reference id -> 1c8f422059ae ("mm: change return type to
vm_fault_t")

Fixed one checkpatch.pl warning inside WARN_ONCE.

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 37 +++--
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a42deeb..95b0d50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "i915_params.h"
 #include "i915_reg.h"
@@ -3363,7 +3364,7 @@ int i915_gem_wait_for_idle(struct drm_i915_private 
*dev_priv,
   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
-int i915_gem_fault(struct vm_fault *vmf);
+vm_fault_t i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 unsigned int flags,
 long timeout,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd89abd..61816e8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1882,7 +1882,7 @@ int i915_gem_mmap_gtt_version(void)
  * The current feature set supported by i915_gem_fault() and thus GTT mmaps
  * is exposed via I915_PARAM_MMAP_GTT_VERSION (see i915_gem_mmap_gtt_version).
  */
-int i915_gem_fault(struct vm_fault *vmf)
+vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 {
 #define MIN_CHUNK_PAGES ((1 << 20) >> PAGE_SHIFT) /* 1 MiB */
struct vm_area_struct *area = vmf->vma;
@@ -1894,7 +1894,8 @@ int i915_gem_fault(struct vm_fault *vmf)
struct i915_vma *vma;
pgoff_t page_offset;
unsigned int flags;
-   int ret;
+   int error;
+   vm_fault_t ret;
 
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
@@ -1906,26 +1907,26 @@ int i915_gem_fault(struct vm_fault *vmf)
 * repeat the flush holding the lock in the normal manner to catch cases
 * where we are gazumped.
 */
-   ret = i915_gem_object_wait(obj,
+   error = i915_gem_object_wait(obj,
   I915_WAIT_INTERRUPTIBLE,
   MAX_SCHEDULE_TIMEOUT,
   NULL);
-   if (ret)
+   if (error)
goto err;
 
-   ret = i915_gem_object_pin_pages(obj);
-   if (ret)
+   error = i915_gem_object_pin_pages(obj);
+   if (error)
goto err;
 
intel_runtime_pm_get(dev_priv);
 
-   ret = i915_mutex_lock_interruptible(dev);
-   if (ret)
+   error = i915_mutex_lock_interruptible(dev);
+   if (error)
goto err_rpm;
 
/* Access to snoopable pages through the GTT is incoherent. */
if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(dev_priv)) {
-   ret = -EFAULT;
+   error = -EFAULT;
goto err_unlock;
}
 
@@ -1952,25 +1953,25 @@ int i915_gem_fault(struct vm_fault *vmf)
vma = i915_gem_object_ggtt_pin(obj, , 0, 0, PIN_MAPPABLE);
}
if (IS_ERR(vma)) {
-   ret = PTR_ERR(vma);
+   error = PTR_ERR(vma);
goto err_unlock;
}
 
-   ret = i915_gem_object_set_to_gtt_domain(obj, write);
-   if (ret)
+   error = i915_gem_object_set_to_gtt_domain(obj, write);
+   if (error)
goto err_unpin;
 
-   ret = i915_vma_pin_fence(vma);
-   if (ret)
+   error = i915_vma_pin_fence(vma);
+   if (error)
goto err_unpin;
 
/* Finally, remap it using the new GTT offset */
-   ret = remap_io_mapping(area,
+   error = remap_io_mapping(area,
   area->vm_start + (vma->ggtt_view.partial.offset 
<< PAGE_SHIFT),
   (ggtt->gmadr.start + vma->node.start) >> 
PAGE_SHIFT,
   min_t(u64, vma->size, area->vm_end - 
area->vm_start),
   >iomap);
-   if (ret)
+   if (error)
goto err_fence;
 
/* Mark as being mmapped into userspace for later revocation */
@@ -1991,7 +1992,7 @@ int i915_gem_fault(struct vm_fault *vmf)
intel_runtime_pm_put(dev_priv);
i915_gem_object_unpin_pages(obj);
 err:
-   switch (ret) {
+   switch (error) {