| Message ID | 1455047053-2644-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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.
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
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.
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)) {
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(-)