drm/i915: Check for get_pages instead of shmem (filp)

Submitted by Ben Widawsky on Feb. 6, 2016, 1:48 a.m.

Details

Message ID 1454723317-31605-1-git-send-email-benjamin.widawsky@intel.com
State New
Headers show
Series "drm/i915: Check for get_pages instead of shmem (filp)" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ben Widawsky Feb. 6, 2016, 1:48 a.m.
This behavior of checking for a shmem backed GEM object was introduced here:
commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab
Author: Brad Volkin <bradley.d.volkin@intel.com>
Date:   Tue Feb 18 10:15:45 2014 -0800

    drm/i915: Refactor shmem pread setup

It is possible for an object to not be a shmem backed GEM object (for example
userptr objects). An example of how we hit this failure can be found through
copy_batch() in the command parser because we allocate a userptr object for the
batch which contains privileged instructions. Userptr calls
drm_gem_private_object_init() which explicitly sets the filp to none.

It is equally feasible to simply remove the check altogether. You'll probably
oops with get_pages somewhere, but that's okay IMO because this condition
should be a driver bug, and not trigger-able by userspace. On this note, the
function name could probably benefit from a change, but whatever.

NOTE: I manually retyped this from a test machine. So I haven't even compiled
this exact patch.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Kristian Høgsberg <krh@bitplanet.net>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 66b1705..a198928 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -489,7 +489,7 @@  int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
 
 	*needs_clflush = 0;
 
-	if (!obj->base.filp)
+	if (!obj->ops->get_pages)
 		return -EINVAL;
 
 	if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) {

Comments

Applied to a 4.4.1 tree:
Tested-by: Jordan Justen <jordan.l.justen@intel.com>

And, FWIW:
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On 2016-02-05 17:48:37, Ben Widawsky wrote:
> This behavior of checking for a shmem backed GEM object was introduced here:
> commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab
> Author: Brad Volkin <bradley.d.volkin@intel.com>
> Date:   Tue Feb 18 10:15:45 2014 -0800
> 
>     drm/i915: Refactor shmem pread setup
> 
> It is possible for an object to not be a shmem backed GEM object (for example
> userptr objects). An example of how we hit this failure can be found through
> copy_batch() in the command parser because we allocate a userptr object for the
> batch which contains privileged instructions. Userptr calls
> drm_gem_private_object_init() which explicitly sets the filp to none.
> 
> It is equally feasible to simply remove the check altogether. You'll probably
> oops with get_pages somewhere, but that's okay IMO because this condition
> should be a driver bug, and not trigger-able by userspace. On this note, the
> function name could probably benefit from a change, but whatever.
> 
> NOTE: I manually retyped this from a test machine. So I haven't even compiled
> this exact patch.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 66b1705..a198928 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -489,7 +489,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
>  
>         *needs_clflush = 0;
>  
> -       if (!obj->base.filp)
> +       if (!obj->ops->get_pages)
>                 return -EINVAL;
>  
>         if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) {
> -- 
> 2.7.0
>
On Fri, Feb 5, 2016 at 5:48 PM, Ben Widawsky
<benjamin.widawsky@intel.com> wrote:
> This behavior of checking for a shmem backed GEM object was introduced here:
> commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab
> Author: Brad Volkin <bradley.d.volkin@intel.com>
> Date:   Tue Feb 18 10:15:45 2014 -0800
>
>     drm/i915: Refactor shmem pread setup
>
> It is possible for an object to not be a shmem backed GEM object (for example
> userptr objects). An example of how we hit this failure can be found through
> copy_batch() in the command parser because we allocate a userptr object for the
> batch which contains privileged instructions. Userptr calls
> drm_gem_private_object_init() which explicitly sets the filp to none.
>
> It is equally feasible to simply remove the check altogether. You'll probably
> oops with get_pages somewhere, but that's okay IMO because this condition
> should be a driver bug, and not trigger-able by userspace. On this note, the
> function name could probably benefit from a change, but whatever.
>
> NOTE: I manually retyped this from a test machine. So I haven't even compiled
> this exact patch.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 66b1705..a198928 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -489,7 +489,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
>
>         *needs_clflush = 0;
>
> -       if (!obj->base.filp)
> +       if (!obj->ops->get_pages)

Do we want to do what Chris did in
a2a4f916c2f344d4e596c875dd1e66764afec8b8 (on drm-intel-fixes):

+       if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))

?

>                 return -EINVAL;
>
>         if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) {
> --
> 2.7.0
>
On 09/02/16 00:20, Kristian Høgsberg wrote:
> On Fri, Feb 5, 2016 at 5:48 PM, Ben Widawsky
> <benjamin.widawsky@intel.com> wrote:
>> This behavior of checking for a shmem backed GEM object was introduced here:
>> commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab
>> Author: Brad Volkin <bradley.d.volkin@intel.com>
>> Date:   Tue Feb 18 10:15:45 2014 -0800
>>
>>      drm/i915: Refactor shmem pread setup
>>
>> It is possible for an object to not be a shmem backed GEM object (for example
>> userptr objects). An example of how we hit this failure can be found through
>> copy_batch() in the command parser because we allocate a userptr object for the
>> batch which contains privileged instructions. Userptr calls
>> drm_gem_private_object_init() which explicitly sets the filp to none.
>>
>> It is equally feasible to simply remove the check altogether. You'll probably
>> oops with get_pages somewhere, but that's okay IMO because this condition
>> should be a driver bug, and not trigger-able by userspace. On this note, the
>> function name could probably benefit from a change, but whatever.
>>
>> NOTE: I manually retyped this from a test machine. So I haven't even compiled
>> this exact patch.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Kristian Høgsberg <krh@bitplanet.net>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 66b1705..a198928 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -489,7 +489,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
>>
>>          *needs_clflush = 0;
>>
>> -       if (!obj->base.filp)
>> +       if (!obj->ops->get_pages)

Don't all subclasses have a get_pages() function?

> Do we want to do what Chris did in
> a2a4f916c2f344d4e596c875dd1e66764afec8b8 (on drm-intel-fixes):
>
> +       if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
>
> ?

Yes, I think so; i915_gem_shmem_pread() is going to walk the sglist, so 
the object had better have a page array for it to iterate over.

.Dave.
On Tue, Feb 09, 2016 at 11:30:34AM +0000, Dave Gordon wrote:
> On 09/02/16 00:20, Kristian Høgsberg wrote:
> >On Fri, Feb 5, 2016 at 5:48 PM, Ben Widawsky
> ><benjamin.widawsky@intel.com> wrote:
> >>This behavior of checking for a shmem backed GEM object was introduced here:
> >>commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab
> >>Author: Brad Volkin <bradley.d.volkin@intel.com>
> >>Date:   Tue Feb 18 10:15:45 2014 -0800
> >>
> >>     drm/i915: Refactor shmem pread setup
> >>
> >>It is possible for an object to not be a shmem backed GEM object (for example
> >>userptr objects). An example of how we hit this failure can be found through
> >>copy_batch() in the command parser because we allocate a userptr object for the
> >>batch which contains privileged instructions. Userptr calls
> >>drm_gem_private_object_init() which explicitly sets the filp to none.
> >>
> >>It is equally feasible to simply remove the check altogether. You'll probably
> >>oops with get_pages somewhere, but that's okay IMO because this condition
> >>should be a driver bug, and not trigger-able by userspace. On this note, the
> >>function name could probably benefit from a change, but whatever.
> >>
> >>NOTE: I manually retyped this from a test machine. So I haven't even compiled
> >>this exact patch.
> >>
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: Kristian Høgsberg <krh@bitplanet.net>
> >>Cc: Jordan Justen <jordan.l.justen@intel.com>
> >>Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 66b1705..a198928 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -489,7 +489,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
> >>
> >>         *needs_clflush = 0;
> >>
> >>-       if (!obj->base.filp)
> >>+       if (!obj->ops->get_pages)
> 
> Don't all subclasses have a get_pages() function?

Yeah, as I said in the commit message, I didn't think this was particularly
appealing, but I figured people would say something if I removed everything. I
wasn't aware of Chris' patch.

> 
> >Do we want to do what Chris did in
> >a2a4f916c2f344d4e596c875dd1e66764afec8b8 (on drm-intel-fixes):
> >
> >+       if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
> >
> >?
> 
> Yes, I think so; i915_gem_shmem_pread() is going to walk the sglist, so the
> object had better have a page array for it to iterate over.
> 
> .Dave.
> 

Okay. I'll send v2. Thanks.
On Wed, Feb 10, 2016 at 07:14:36AM -0000, Patchwork wrote:
> == Summary ==
> 
> Series 3145v2 drm/i915: Check for get_pages instead of shmem (filp)
> http://patchwork.freedesktop.org/api/1.0/series/3145/revisions/2/mbox/
> 
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 fail       -> PASS       (hsw-gt2)
>                 pass       -> DMESG-WARN (byt-nuc)

Known fail: https://bugs.freedesktop.org/show_bug.cgi?id=93121

I really should simply merge the patch to shut this all up.
-Daniel

>         Subgroup basic-rte:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
>                 dmesg-warn -> PASS       (byt-nuc) UNSTABLE
> 
> bdw-nuci7        total:161  pass:152  dwarn:0   dfail:0   fail:0   skip:9  
> bdw-ultra        total:164  pass:152  dwarn:0   dfail:0   fail:0   skip:12 
> bsw-nuc-2        total:164  pass:136  dwarn:0   dfail:0   fail:0   skip:28 
> byt-nuc          total:164  pass:140  dwarn:1   dfail:0   fail:0   skip:23 
> hsw-brixbox      total:164  pass:151  dwarn:0   dfail:0   fail:0   skip:13 
> hsw-gt2          total:164  pass:154  dwarn:0   dfail:0   fail:0   skip:10 
> ilk-hp8440p      total:164  pass:116  dwarn:0   dfail:0   fail:0   skip:48 
> ivb-t430s        total:164  pass:150  dwarn:0   dfail:0   fail:0   skip:14 
> snb-dellxps      total:164  pass:142  dwarn:0   dfail:0   fail:0   skip:22 
> snb-x220t        total:164  pass:142  dwarn:0   dfail:0   fail:1   skip:21 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1385/
> 
> d9bd337b4b2d46f73005fcdf0e7049e7f8ed5c04 drm-intel-nightly: 2016y-02m-09d-15h-42m-46s UTC integration manifest
> cb1f5a551af8b6b2f3f75a322a70df5891eee393 drm/i915: Check for get_pages instead of shmem (filp)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx