| Message ID | 1423828140-10653-8-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 13, 2015 at 11:48:16AM +0000, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > Start of explicit request management in the execbuffer code path. This patch > adds a call to allocate a request structure before all the actual hardware work > is done. Thus guaranteeing that all that work is tagged by a known request. At > present, nothing further is done with the request, the rest comes later in the > series. > > The only noticable change is that failure to get a request (e.g. due to lack of > memory) will be caught earlier in the sequence. It now occurs right at the start > before any un-undoable work has been done. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index ca85803..61471e9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > const u32 ctx_id = i915_execbuffer2_get_context_id(*args); > u32 dispatch_flags; > int ret; > - bool need_relocs; > + bool need_relocs, batch_pinned = false; > > if (!i915_gem_check_execbuffer(args)) > return -EINVAL; > @@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > if (ret) > goto err; > > + batch_pinned = true; > params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj); > } else > params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm); > > + /* Allocate a request for this batch buffer nice and early. */ > + ret = dev_priv->gt.alloc_request(ring, ctx); > + if (ret) > + goto err; > + > /* > * Save assorted stuff away to pass through to *_submission(). > * NB: This data should be 'persistent' and not local as it will > @@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas); > > +err: > /* > * FIXME: We crucially rely upon the active tracking for the (ppgtt) > * batch vma for correctness. For less ugly and less fragility this > * needs to be adjusted to also track the ggtt batch vma properly as > * active. > */ > - if (dispatch_flags & I915_DISPATCH_SECURE) > + if (batch_pinned) > i915_gem_object_ggtt_unpin(batch_obj); > -err: > + > /* the request owns the ref now */ > i915_gem_context_unreference(ctx); > eb_destroy(eb); This hunk here looks wrong, or maybe the context changed sufficiently already (but I can't find that in previous patches). Why do we need to change the pinning for the ggtt batch pin hack when allocating the request earlier? -Daniel
On 25/02/2015 22:22, Daniel Vetter wrote: > On Fri, Feb 13, 2015 at 11:48:16AM +0000, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> Start of explicit request management in the execbuffer code path. This patch >> adds a call to allocate a request structure before all the actual hardware work >> is done. Thus guaranteeing that all that work is tagged by a known request. At >> present, nothing further is done with the request, the rest comes later in the >> series. >> >> The only noticable change is that failure to get a request (e.g. due to lack of >> memory) will be caught earlier in the sequence. It now occurs right at the start >> before any un-undoable work has been done. >> >> For: VIZ-5115 >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index ca85803..61471e9 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> const u32 ctx_id = i915_execbuffer2_get_context_id(*args); >> u32 dispatch_flags; >> int ret; >> - bool need_relocs; >> + bool need_relocs, batch_pinned = false; >> >> if (!i915_gem_check_execbuffer(args)) >> return -EINVAL; >> @@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> if (ret) >> goto err; >> >> + batch_pinned = true; >> params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj); >> } else >> params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm); >> >> + /* Allocate a request for this batch buffer nice and early. */ >> + ret = dev_priv->gt.alloc_request(ring, ctx); >> + if (ret) >> + goto err; >> + >> /* >> * Save assorted stuff away to pass through to *_submission(). >> * NB: This data should be 'persistent' and not local as it will >> @@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, >> >> ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas); >> >> +err: >> /* >> * FIXME: We crucially rely upon the active tracking for the (ppgtt) >> * batch vma for correctness. For less ugly and less fragility this >> * needs to be adjusted to also track the ggtt batch vma properly as >> * active. >> */ >> - if (dispatch_flags & I915_DISPATCH_SECURE) >> + if (batch_pinned) >> i915_gem_object_ggtt_unpin(batch_obj); >> -err: >> + >> /* the request owns the ref now */ >> i915_gem_context_unreference(ctx); >> eb_destroy(eb); > This hunk here looks wrong, or maybe the context changed sufficiently > already (but I can't find that in previous patches). Why do we need to > change the pinning for the ggtt batch pin hack when allocating the request > earlier? > -Daniel It doesn't change the behaviour. It is just coping with the extra error path. If the alloc request call fails, we need to jump past the do_execbuf() call but not past the batch unpin. Hence the 'err:' tag is moved upwards. That means it is now possible to get to the batch unpin test from an error that occurred before the pin call actually happened. Hence it is no longer safe to just test the dispatch flag. Instead an explicit 'did this get pinned yet' boolean is required.
On Fri, Feb 27, 2015 at 12:27:15PM +0000, John Harrison wrote: > On 25/02/2015 22:22, Daniel Vetter wrote: > >On Fri, Feb 13, 2015 at 11:48:16AM +0000, John.C.Harrison@Intel.com wrote: > >>From: John Harrison <John.C.Harrison@Intel.com> > >> > >>Start of explicit request management in the execbuffer code path. This patch > >>adds a call to allocate a request structure before all the actual hardware work > >>is done. Thus guaranteeing that all that work is tagged by a known request. At > >>present, nothing further is done with the request, the rest comes later in the > >>series. > >> > >>The only noticable change is that failure to get a request (e.g. due to lack of > >>memory) will be caught earlier in the sequence. It now occurs right at the start > >>before any un-undoable work has been done. > >> > >>For: VIZ-5115 > >>Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 ++++++++++--- > >> 1 file changed, 10 insertions(+), 3 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>index ca85803..61471e9 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>@@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > >> const u32 ctx_id = i915_execbuffer2_get_context_id(*args); > >> u32 dispatch_flags; > >> int ret; > >>- bool need_relocs; > >>+ bool need_relocs, batch_pinned = false; > >> if (!i915_gem_check_execbuffer(args)) > >> return -EINVAL; > >>@@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > >> if (ret) > >> goto err; > >>+ batch_pinned = true; > >> params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj); > >> } else > >> params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm); > >>+ /* Allocate a request for this batch buffer nice and early. */ > >>+ ret = dev_priv->gt.alloc_request(ring, ctx); > >>+ if (ret) > >>+ goto err; > >>+ > >> /* > >> * Save assorted stuff away to pass through to *_submission(). > >> * NB: This data should be 'persistent' and not local as it will > >>@@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > >> ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas); > >>+err: > >> /* > >> * FIXME: We crucially rely upon the active tracking for the (ppgtt) > >> * batch vma for correctness. For less ugly and less fragility this > >> * needs to be adjusted to also track the ggtt batch vma properly as > >> * active. > >> */ > >>- if (dispatch_flags & I915_DISPATCH_SECURE) > >>+ if (batch_pinned) > >> i915_gem_object_ggtt_unpin(batch_obj); > >>-err: > >>+ > >> /* the request owns the ref now */ > >> i915_gem_context_unreference(ctx); > >> eb_destroy(eb); > >This hunk here looks wrong, or maybe the context changed sufficiently > >already (but I can't find that in previous patches). Why do we need to > >change the pinning for the ggtt batch pin hack when allocating the request > >earlier? > >-Daniel > > It doesn't change the behaviour. It is just coping with the extra error > path. If the alloc request call fails, we need to jump past the do_execbuf() > call but not past the batch unpin. Hence the 'err:' tag is moved upwards. > That means it is now possible to get to the batch unpin test from an error > that occurred before the pin call actually happened. Hence it is no longer > safe to just test the dispatch flag. Instead an explicit 'did this get > pinned yet' boolean is required. Ah I see what's going on. The usual pattern is to just duplicate goto targets, i.e. here: err_batch_unpin: /* * FIXME: We crucially rely upon the active tracking for the (ppgtt) * batch vma for correctness. For less ugly and less fragility this * needs to be adjusted to also track the ggtt batch vma properly as * active. */ if (dispatch_flags & I915_DISPATCH_SECURE) i915_gem_object_ggtt_unpin(batch_obj); err: /* the request owns the ref now */ i915_gem_context_unreference(ctx); eb_destroy(eb); Then you can let gcc deal with how to best structure the control flow for you. It means a bit larger diff since you need to change a pile of goto err; to goto err_batch_unpin, but imo that also makes it easier for reviewers to double-check there's no error case with funky requirements now broken. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index ca85803..61471e9 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, const u32 ctx_id = i915_execbuffer2_get_context_id(*args); u32 dispatch_flags; int ret; - bool need_relocs; + bool need_relocs, batch_pinned = false; if (!i915_gem_check_execbuffer(args)) return -EINVAL; @@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (ret) goto err; + batch_pinned = true; params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj); } else params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm); + /* Allocate a request for this batch buffer nice and early. */ + ret = dev_priv->gt.alloc_request(ring, ctx); + if (ret) + goto err; + /* * Save assorted stuff away to pass through to *_submission(). * NB: This data should be 'persistent' and not local as it will @@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas); +err: /* * FIXME: We crucially rely upon the active tracking for the (ppgtt) * batch vma for correctness. For less ugly and less fragility this * needs to be adjusted to also track the ggtt batch vma properly as * active. */ - if (dispatch_flags & I915_DISPATCH_SECURE) + if (batch_pinned) i915_gem_object_ggtt_unpin(batch_obj); -err: + /* the request owns the ref now */ i915_gem_context_unreference(ctx); eb_destroy(eb);