| Message ID | 1424366285-29232-6-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > For some reason, the i915_add_request() call in > i915_gem_execbuffer_retire_commands() was explicitly having its return code > ignored. The _retire_commands() function itself was 'void'. Given that > _add_request() can fail without dispatching the batch buffer, this seems odd. > > Also shrunk the parameter list to a single structure as everything it requires > is available in the execbuff_params object. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 +---- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++++--------- > drivers/gpu/drm/i915/intel_lrc.c | 3 +-- > 3 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index fdd63ab..b350910 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2640,10 +2640,7 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > void i915_gem_execbuffer_move_to_active(struct list_head *vmas, > struct intel_engine_cs *ring); > -void i915_gem_execbuffer_retire_commands(struct drm_device *dev, > - struct drm_file *file, > - struct intel_engine_cs *ring, > - struct drm_i915_gem_object *obj); > +int i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params); > int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > struct drm_i915_gem_execbuffer2 *args, > struct list_head *vmas); > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 93b0ef0..ca85803 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -989,17 +989,15 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, > } > } > > -void > -i915_gem_execbuffer_retire_commands(struct drm_device *dev, > - struct drm_file *file, > - struct intel_engine_cs *ring, > - struct drm_i915_gem_object *obj) > +int > +i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params) > { > /* Unconditionally force add_request to emit a full flush. */ > - ring->gpu_caches_dirty = true; > + params->ring->gpu_caches_dirty = true; > > /* Add a breadcrumb for the completion of the batch buffer */ > - (void)__i915_add_request(ring, file, obj); > + return __i915_add_request(params->ring, params->file, > + params->batch_obj); > } > > static int > @@ -1282,8 +1280,8 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); > > i915_gem_execbuffer_move_to_active(vmas, ring); > - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, > - params->batch_obj); > + > + ret = i915_gem_execbuffer_retire_commands(params); > > error: > kfree(cliprects); > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index f6a24e8a2..dc474b4 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -715,9 +715,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); > > i915_gem_execbuffer_move_to_active(vmas, ring); > - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, params->batch_obj); > > - return 0; > + return i915_gem_execbuffer_retire_commands(params); > } > > void intel_execlists_retire_requests(struct intel_engine_cs *ring) > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Thanks, Tomas
On Thu, Mar 05, 2015 at 02:45:11PM +0000, Tomas Elf wrote: > On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote: > >From: John Harrison <John.C.Harrison@Intel.com> > > > >For some reason, the i915_add_request() call in > >i915_gem_execbuffer_retire_commands() was explicitly having its return code > >ignored. The _retire_commands() function itself was 'void'. Given that > >_add_request() can fail without dispatching the batch buffer, this seems odd. > > > >Also shrunk the parameter list to a single structure as everything it requires > >is available in the execbuff_params object. > > > >For: VIZ-5115 > >Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >--- > > drivers/gpu/drm/i915/i915_drv.h | 5 +---- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++++--------- > > drivers/gpu/drm/i915/intel_lrc.c | 3 +-- > > 3 files changed, 9 insertions(+), 15 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index fdd63ab..b350910 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -2640,10 +2640,7 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv); > > void i915_gem_execbuffer_move_to_active(struct list_head *vmas, > > struct intel_engine_cs *ring); > >-void i915_gem_execbuffer_retire_commands(struct drm_device *dev, > >- struct drm_file *file, > >- struct intel_engine_cs *ring, > >- struct drm_i915_gem_object *obj); > >+int i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params); > > int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > > struct drm_i915_gem_execbuffer2 *args, > > struct list_head *vmas); > >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >index 93b0ef0..ca85803 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >@@ -989,17 +989,15 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, > > } > > } > > > >-void > >-i915_gem_execbuffer_retire_commands(struct drm_device *dev, > >- struct drm_file *file, > >- struct intel_engine_cs *ring, > >- struct drm_i915_gem_object *obj) > >+int > >+i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params) > > { > > /* Unconditionally force add_request to emit a full flush. */ > >- ring->gpu_caches_dirty = true; > >+ params->ring->gpu_caches_dirty = true; > > > > /* Add a breadcrumb for the completion of the batch buffer */ > >- (void)__i915_add_request(ring, file, obj); > >+ return __i915_add_request(params->ring, params->file, > >+ params->batch_obj); > > } > > > > static int > >@@ -1282,8 +1280,8 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > > trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); > > > > i915_gem_execbuffer_move_to_active(vmas, ring); > >- i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, > >- params->batch_obj); > >+ > >+ ret = i915_gem_execbuffer_retire_commands(params); > > > > error: > > kfree(cliprects); > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index f6a24e8a2..dc474b4 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -715,9 +715,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > > trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); > > > > i915_gem_execbuffer_move_to_active(vmas, ring); > >- i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, params->batch_obj); > > > >- return 0; > >+ return i915_gem_execbuffer_retire_commands(params); > > } > > > > void intel_execlists_retire_requests(struct intel_engine_cs *ring) > > > > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Patch discussion unfortunately split since I've made all my comments on the earlier iteration. Anyway there's a big discussion there, you might want to look into that and reconsder your r-b ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fdd63ab..b350910 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2640,10 +2640,7 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); void i915_gem_execbuffer_move_to_active(struct list_head *vmas, struct intel_engine_cs *ring); -void i915_gem_execbuffer_retire_commands(struct drm_device *dev, - struct drm_file *file, - struct intel_engine_cs *ring, - struct drm_i915_gem_object *obj); +int i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params); int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, struct drm_i915_gem_execbuffer2 *args, struct list_head *vmas); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 93b0ef0..ca85803 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -989,17 +989,15 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, } } -void -i915_gem_execbuffer_retire_commands(struct drm_device *dev, - struct drm_file *file, - struct intel_engine_cs *ring, - struct drm_i915_gem_object *obj) +int +i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params) { /* Unconditionally force add_request to emit a full flush. */ - ring->gpu_caches_dirty = true; + params->ring->gpu_caches_dirty = true; /* Add a breadcrumb for the completion of the batch buffer */ - (void)__i915_add_request(ring, file, obj); + return __i915_add_request(params->ring, params->file, + params->batch_obj); } static int @@ -1282,8 +1280,8 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); i915_gem_execbuffer_move_to_active(vmas, ring); - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, - params->batch_obj); + + ret = i915_gem_execbuffer_retire_commands(params); error: kfree(cliprects); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f6a24e8a2..dc474b4 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -715,9 +715,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), params->dispatch_flags); i915_gem_execbuffer_move_to_active(vmas, ring); - i915_gem_execbuffer_retire_commands(params->dev, params->file, ring, params->batch_obj); - return 0; + return i915_gem_execbuffer_retire_commands(params); } void intel_execlists_retire_requests(struct intel_engine_cs *ring)