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

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

[Ben Widawsky]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Check for get_pages instead of shmem (filp)

[Patchwork]

== Summary ==

Series 3145v1 drm/i915: Check for get_pages instead of shmem (filp)
http://patchwork.freedesktop.org/api/1.0/series/3145/revisions/1/mbox/


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:141  dwarn:0   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 
skl-i5k-2        total:164  pass:149  dwarn:1   dfail:0   fail:0   skip:14 
snb-dellxps      total:164  pass:142  dwarn:0   dfail:0   fail:0   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1377/

9ae71c139227311e70dcc91d16d5acba34ce9a71 drm-intel-nightly: 2016y-02m-08d-09h-39m-29s UTC integration manifest
48a3f49aa2256f9e1a071ff4a52c5c226cf01ee2 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

Re: [PATCH] drm/i915: Check for get_pages instead of shmem (filp)

[Jordan Justen]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for get_pages instead of shmem (filp)

[Kristian Høgsberg]

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check for get_pages instead of shmem (filp)

[Dave Gordon]

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.

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

Re: [PATCH] drm/i915: Check for get_pages instead of shmem (filp)

[Ben Widawsky]

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.


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Ben Widawsky]

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.

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

v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Kristian Høgsberg <krh@bitplanet.net>
Cc: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
---
 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 e9b19bc..7fd79b0 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 (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

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

✗ Fi.CI.BAT: warning for drm/i915: Check for get_pages instead of shmem (filp) (rev2)

[Patchwork]

== 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)
        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

Re: [PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Ben Widawsky]

Do you guys get the CI mails? This version has regressions. v1 did not. I don't
know what to trust.

On Tue, Feb 09, 2016 at 11:44:12AM -0800, 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.
> 
> NOTE: I manually retyped this from a test machine. So I haven't even compiled
> this exact patch.
> 
> v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> ---
>  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 e9b19bc..7fd79b0 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 (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
> 

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Chris Wilson]

On Wed, Feb 10, 2016 at 07:42:23AM -0800, Ben Widawsky wrote:
> Do you guys get the CI mails? This version has regressions. v1 did not. I don't
> know what to trust.

I didn't even see v2 itself!
 
> On Tue, Feb 09, 2016 at 11:44:12AM -0800, 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.
> > 
> > NOTE: I manually retyped this from a test machine. So I haven't even compiled
> > this exact patch.
> > 
> > v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Kristian Høgsberg <krh@bitplanet.net>
> > Cc: Dave Gordon <david.s.gordon@intel.com>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> > Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)

> > ---
> >  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 e9b19bc..7fd79b0 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 (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))

Don't use WARN_ON, there is code (or will be) where we use
prepare_shmem_read/write to determine if we can use the shmem paths.

Also i915_gem_obj_prepare_shmem_write() requires the same treatment.

My apologies I had this patch but appear to have accidentally squashed
it whilst rebasing. Thanks!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Ben Widawsky]

On Wed, Feb 10, 2016 at 04:23:08PM +0000, Chris Wilson wrote:
> On Wed, Feb 10, 2016 at 07:42:23AM -0800, Ben Widawsky wrote:
> > Do you guys get the CI mails? This version has regressions. v1 did not. I don't
> > know what to trust.
> 
> I didn't even see v2 itself!
>  
> > On Tue, Feb 09, 2016 at 11:44:12AM -0800, 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.
> > > 
> > > NOTE: I manually retyped this from a test machine. So I haven't even compiled
> > > this exact patch.
> > > 
> > > v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Kristian Høgsberg <krh@bitplanet.net>
> > > Cc: Dave Gordon <david.s.gordon@intel.com>
> > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > > Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> > > Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> 
> > > ---
> > >  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 e9b19bc..7fd79b0 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 (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
> 
> Don't use WARN_ON, there is code (or will be) where we use
> prepare_shmem_read/write to determine if we can use the shmem paths.
> 
> Also i915_gem_obj_prepare_shmem_write() requires the same treatment.
> 
> My apologies I had this patch but appear to have accidentally squashed
> it whilst rebasing. Thanks!
> -Chris
> 

So... is someone going to land this fix? We need it.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: Check for get_pages instead of shmem (filp) (rev2)

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Daniel Vetter]

On Wed, Feb 10, 2016 at 09:39:33AM -0800, Ben Widawsky wrote:
> On Wed, Feb 10, 2016 at 04:23:08PM +0000, Chris Wilson wrote:
> > On Wed, Feb 10, 2016 at 07:42:23AM -0800, Ben Widawsky wrote:
> > > Do you guys get the CI mails? This version has regressions. v1 did not. I don't
> > > know what to trust.
> > 
> > I didn't even see v2 itself!
> >  
> > > On Tue, Feb 09, 2016 at 11:44:12AM -0800, 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.
> > > > 
> > > > NOTE: I manually retyped this from a test machine. So I haven't even compiled
> > > > this exact patch.
> > > > 
> > > > v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)
> > > > 
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Kristian Høgsberg <krh@bitplanet.net>
> > > > Cc: Dave Gordon <david.s.gordon@intel.com>
> > > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> > > > Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
> > 
> > > > ---
> > > >  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 e9b19bc..7fd79b0 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 (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))
> > 
> > Don't use WARN_ON, there is code (or will be) where we use
> > prepare_shmem_read/write to determine if we can use the shmem paths.
> > 
> > Also i915_gem_obj_prepare_shmem_write() requires the same treatment.
> > 
> > My apologies I had this patch but appear to have accidentally squashed
> > it whilst rebasing. Thanks!
> > -Chris
> > 
> 
> So... is someone going to land this fix? We need it.

Queued for -next, thanks for the patch. Aside: We have about 15 committers
by now, Chris being one of them. No need to hang in there waiting for lazy
me to undig myself from the latest CI fire.
-Daniel
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Check for get_pages instead of shmem (filp)

[Dave Gordon]

On 10/02/16 17:39, Ben Widawsky wrote:
> On Wed, Feb 10, 2016 at 04:23:08PM +0000, Chris Wilson wrote:
>> On Wed, Feb 10, 2016 at 07:42:23AM -0800, Ben Widawsky wrote:
>>> Do you guys get the CI mails? This version has regressions. v1 did not. I don't
>>> know what to trust.
>>
>> I didn't even see v2 itself!
>>
>>> On Tue, Feb 09, 2016 at 11:44:12AM -0800, 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.
>>>>
>>>> NOTE: I manually retyped this from a test machine. So I haven't even compiled
>>>> this exact patch.
>>>>
>>>> v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon)
>>>>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Kristian Høgsberg <krh@bitplanet.net>
>>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>>> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
>>>> Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
>>>> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
>>
>>>> ---
>>>>   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 e9b19bc..7fd79b0 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 (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0))

It looks like the above code can't be reached? 'filp' is tested in 
i915_gem_pread_ioctl(), before i915_gem_obj_prepare_shmem_read() is called!

>> Don't use WARN_ON, there is code (or will be) where we use
>> prepare_shmem_read/write to determine if we can use the shmem paths.
>>
>> Also i915_gem_obj_prepare_shmem_write() requires the same treatment.

No such function, but there's a 'filp' test in i915_gem_pwrite_ioctl().

Also, what about i915_gem_mmap_ioctl() ? Is mmap() also going to be 
legitimate without a file pointer?

> > My apologies I had this patch but appear to have accidentally squashed
>> it whilst rebasing. Thanks!
>> -Chris
>
> So... is someone going to land this fix? We need it.

Looks like Chris wants this to read

     if (!(obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE))
         return -EINVAL;

leaving it to the caller to decide whether to log a complaint.
I'm happy with that.

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