| Message ID | 554CB99A.3090501@zoho.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Rebecca N. Palmer" <rebecca_palmer@zoho.com> writes: Hi, >> where cmdparser is disabled, batch_obj is >> left dangling > Sorry! Fixed now. > There is absolutely nothing to be sorry about. > This version also brings exec_start = 0 inside this check, as it > appears to be there because the copying (i915_cmd_parser.c:1054) > removes any offset the original might have had. > > When tested on next-20150508 (675b3fb), it passed my checks > (libva tests, vlc video, glxgears, beignet tests), and didn't > show the "missing window title bar" problem [0-1] in 3 attempts, > but given the intermittent nature of that I can't be sure. > > I still can't give useful i-g-t results, as it works on 3.16 > but reports "GPU HANG" for most tests on 4.0 and (both patched and > unpatched) next (scripts/run-tests.sh at the recovery-mode > (single-user-mode) prompt, both i-g-t 1.10 and latest git). > > [0] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065906.html > > --- > > i915_gem_execbuffer_parse returns the original batch_obj on batches > it can't check (currently, chained batches). Don't set the secure > bit in this case. > > v2 (thanks to Mika Kuoppala): > Don't leave batch_obj unset when the parser is not run. > Only do exec_start = 0 on parsed batches. > Add comments. > > Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 7ab63d9..2fb6dc1 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1540,28 +1540,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > } > > if (i915_needs_cmd_parser(ring) && args->batch_len) { > - batch_obj = i915_gem_execbuffer_parse(ring, > + struct drm_i915_gem_object *parsed_batch_obj; > + > + parsed_batch_obj = i915_gem_execbuffer_parse(ring, > &shadow_exec_entry, > eb, > batch_obj, > args->batch_start_offset, > args->batch_len, > file->is_master); > - if (IS_ERR(batch_obj)) { > - ret = PTR_ERR(batch_obj); > + if (IS_ERR(parsed_batch_obj)) { > + /* Batch rejected by parser, or an error > occurred */ This comment should be omitted as the rejection part is not valid in here and the error part is redudant. Daniel can squash it while applying I think. For a first patch, stellar work! Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > + ret = PTR_ERR(parsed_batch_obj); > goto err; > } > > - /* > - * Set the DISPATCH_SECURE bit to remove the NON_SECURE > - * bit from MI_BATCH_BUFFER_START commands issued in the > - * dispatch_execbuffer implementations. We specifically > - * don't want that set when the command parser is > - * enabled. > - */ > - dispatch_flags |= I915_DISPATCH_SECURE; > - > - exec_start = 0; > + /* parsed_batch_obj == batch_obj means batch not fully parsed: > + * accept, but don't promote to secure */ > + > + if (parsed_batch_obj != batch_obj) { > + /* > + * Batch parsed and accepted: > + * > + * Set the DISPATCH_SECURE bit to remove the NON_SECURE > + * bit from MI_BATCH_BUFFER_START commands issued in > + * the dispatch_execbuffer implementations. We > + * specifically don't want that set on batches the > + * command parser has accepted. > + */ > + dispatch_flags |= I915_DISPATCH_SECURE; > + exec_start = 0; > + batch_obj = parsed_batch_obj; > + } > } > > batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND; > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 08, 2015 at 05:04:48PM +0300, Mika Kuoppala wrote: > "Rebecca N. Palmer" <rebecca_palmer@zoho.com> writes: > > Hi, > > >> where cmdparser is disabled, batch_obj is > >> left dangling > > Sorry! Fixed now. > > > > There is absolutely nothing to be sorry about. > > > This version also brings exec_start = 0 inside this check, as it > > appears to be there because the copying (i915_cmd_parser.c:1054) > > removes any offset the original might have had. > > > > When tested on next-20150508 (675b3fb), it passed my checks > > (libva tests, vlc video, glxgears, beignet tests), and didn't > > show the "missing window title bar" problem [0-1] in 3 attempts, > > but given the intermittent nature of that I can't be sure. > > > > I still can't give useful i-g-t results, as it works on 3.16 > > but reports "GPU HANG" for most tests on 4.0 and (both patched and > > unpatched) next (scripts/run-tests.sh at the recovery-mode > > (single-user-mode) prompt, both i-g-t 1.10 and latest git). > > > > [0] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065906.html > > > > --- Just an side: git drops everything _below_ the --- line from the commit message when applying the patch. Hence comments like the above should go below that line, and the real commit message above. Also your commit message for v2 doesn't mention how this bug was introduced, which is crucial information. I've stitched something together. > > i915_gem_execbuffer_parse returns the original batch_obj on batches > > it can't check (currently, chained batches). Don't set the secure > > bit in this case. > > > > v2 (thanks to Mika Kuoppala): > > Don't leave batch_obj unset when the parser is not run. > > Only do exec_start = 0 on parsed batches. > > Add comments. > > > > Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 7ab63d9..2fb6dc1 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -1540,28 +1540,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > } > > > > if (i915_needs_cmd_parser(ring) && args->batch_len) { > > - batch_obj = i915_gem_execbuffer_parse(ring, > > + struct drm_i915_gem_object *parsed_batch_obj; > > + > > + parsed_batch_obj = i915_gem_execbuffer_parse(ring, > > &shadow_exec_entry, > > eb, > > batch_obj, > > args->batch_start_offset, > > args->batch_len, > > file->is_master); > > - if (IS_ERR(batch_obj)) { > > - ret = PTR_ERR(batch_obj); > > + if (IS_ERR(parsed_batch_obj)) { > > + /* Batch rejected by parser, or an error > > occurred */ > > This comment should be omitted as the rejection part is not > valid in here and the error part is redudant. Daniel can squash it while > applying I think. Done while applying. > > For a first patch, stellar work! Indeed. Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 7ab63d9..2fb6dc1 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1540,28 +1540,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, } if (i915_needs_cmd_parser(ring) && args->batch_len) { - batch_obj = i915_gem_execbuffer_parse(ring, + struct drm_i915_gem_object *parsed_batch_obj; + + parsed_batch_obj = i915_gem_execbuffer_parse(ring, &shadow_exec_entry, eb, batch_obj, args->batch_start_offset, args->batch_len, file->is_master); - if (IS_ERR(batch_obj)) { - ret = PTR_ERR(batch_obj); + if (IS_ERR(parsed_batch_obj)) { + /* Batch rejected by parser, or an error occurred */ + ret = PTR_ERR(parsed_batch_obj); goto err; } - /* - * Set the DISPATCH_SECURE bit to remove the NON_SECURE - * bit from MI_BATCH_BUFFER_START commands issued in the - * dispatch_execbuffer implementations. We specifically - * don't want that set when the command parser is - * enabled. - */ - dispatch_flags |= I915_DISPATCH_SECURE; - - exec_start = 0; + /* parsed_batch_obj == batch_obj means batch not fully parsed: + * accept, but don't promote to secure */ + + if (parsed_batch_obj != batch_obj) { + /* + * Batch parsed and accepted: + * + * Set the DISPATCH_SECURE bit to remove the NON_SECURE + * bit from MI_BATCH_BUFFER_START commands issued in + * the dispatch_execbuffer implementations. We + * specifically don't want that set on batches the + * command parser has accepted. + */ + dispatch_flags |= I915_DISPATCH_SECURE; + exec_start = 0; + batch_obj = parsed_batch_obj; + } } batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;