| Message ID | 1470581141-14432-9-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > request->batch_obj is only set by execbuffer for the convenience of > debugging hangs. By moving that operation to the callsite, we can > simplify all other callers and future patches. We also move the > complications of reference handling of the request->batch_obj next to > where the active tracking is set up for the request. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++- > drivers/gpu/drm/i915/i915_gem_request.c | 12 +----------- > drivers/gpu/drm/i915/i915_gem_request.h | 8 +++----- > 3 files changed, 13 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index c494b79ded20..c8d13fea4b25 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1702,6 +1702,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > goto err_batch_unpin; > } > > + /* Whilst this request exists, batch_obj will be on the > + * active_list, and so will hold the active reference. Only when this > + * request is retired will the the batch_obj be moved onto the > + * inactive_list and lose its active reference. Hence we do not need > + * to explicitly hold another reference here. > + */ The comment here might or might not need revisiting. I can't say yet. But when I tried to learn how the current code works, I found that there are comments referencing __i915_gem_active_get_request_rcu() which does not exist. -Mika > + params->request->batch_obj = params->batch->obj; > + > ret = i915_gem_request_add_to_client(params->request, file); > if (ret) > goto err_request; > @@ -1720,7 +1728,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > ret = execbuf_submit(params, args, &eb->vmas); > err_request: > - __i915_add_request(params->request, params->batch->obj, ret == 0); > + __i915_add_request(params->request, ret == 0); > > err_batch_unpin: > /* > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c > index b7ffde002a62..c6f523e2879c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.c > +++ b/drivers/gpu/drm/i915/i915_gem_request.c > @@ -461,9 +461,7 @@ static void i915_gem_mark_busy(const struct intel_engine_cs *engine) > * request is not being tracked for completion but the work itself is > * going to happen on the hardware. This would be a Bad Thing(tm). > */ > -void __i915_add_request(struct drm_i915_gem_request *request, > - struct drm_i915_gem_object *obj, > - bool flush_caches) > +void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches) > { > struct intel_engine_cs *engine; > struct intel_ring *ring; > @@ -504,14 +502,6 @@ void __i915_add_request(struct drm_i915_gem_request *request, > > request->head = request_start; > > - /* Whilst this request exists, batch_obj will be on the > - * active_list, and so will hold the active reference. Only when this > - * request is retired will the the batch_obj be moved onto the > - * inactive_list and lose its active reference. Hence we do not need > - * to explicitly hold another reference here. > - */ > - request->batch_obj = obj; > - > /* Seal the request and mark it as pending execution. Note that > * we may inspect this state, without holding any locks, during > * hangcheck. Hence we apply the barrier to ensure that we do not > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h > index 721eb8cbce9b..d5176f9cc22f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.h > +++ b/drivers/gpu/drm/i915/i915_gem_request.h > @@ -225,13 +225,11 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst, > *pdst = src; > } > > -void __i915_add_request(struct drm_i915_gem_request *req, > - struct drm_i915_gem_object *batch_obj, > - bool flush_caches); > +void __i915_add_request(struct drm_i915_gem_request *req, bool flush_caches); > #define i915_add_request(req) \ > - __i915_add_request(req, NULL, true) > + __i915_add_request(req, true) > #define i915_add_request_no_flush(req) \ > - __i915_add_request(req, NULL, false) > + __i915_add_request(req, false) > > struct intel_rps_client; > #define NO_WAITBOOST ERR_PTR(-1) > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 09, 2016 at 06:53:16PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > request->batch_obj is only set by execbuffer for the convenience of > > debugging hangs. By moving that operation to the callsite, we can > > simplify all other callers and future patches. We also move the > > complications of reference handling of the request->batch_obj next to > > where the active tracking is set up for the request. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++- > > drivers/gpu/drm/i915/i915_gem_request.c | 12 +----------- > > drivers/gpu/drm/i915/i915_gem_request.h | 8 +++----- > > 3 files changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index c494b79ded20..c8d13fea4b25 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -1702,6 +1702,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > goto err_batch_unpin; > > } > > > > + /* Whilst this request exists, batch_obj will be on the > > + * active_list, and so will hold the active reference. Only when this > > + * request is retired will the the batch_obj be moved onto the > > + * inactive_list and lose its active reference. Hence we do not need > > + * to explicitly hold another reference here. > > + */ > > The comment here might or might not need revisiting. I can't say yet. That's still true. Active objects have a reference that prevents them from being freed whilst in use by the GPU - currently managed by i915_gem_object_retire__read() iirc. -Chris
On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote: > request->batch_obj is only set by execbuffer for the convenience of > debugging hangs. By moving that operation to the callsite, we can > simplify all other callers and future patches. We also move the > complications of reference handling of the request->batch_obj next to > where the active tracking is set up for the request. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c494b79ded20..c8d13fea4b25 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1702,6 +1702,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, goto err_batch_unpin; } + /* Whilst this request exists, batch_obj will be on the + * active_list, and so will hold the active reference. Only when this + * request is retired will the the batch_obj be moved onto the + * inactive_list and lose its active reference. Hence we do not need + * to explicitly hold another reference here. + */ + params->request->batch_obj = params->batch->obj; + ret = i915_gem_request_add_to_client(params->request, file); if (ret) goto err_request; @@ -1720,7 +1728,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = execbuf_submit(params, args, &eb->vmas); err_request: - __i915_add_request(params->request, params->batch->obj, ret == 0); + __i915_add_request(params->request, ret == 0); err_batch_unpin: /* diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index b7ffde002a62..c6f523e2879c 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -461,9 +461,7 @@ static void i915_gem_mark_busy(const struct intel_engine_cs *engine) * request is not being tracked for completion but the work itself is * going to happen on the hardware. This would be a Bad Thing(tm). */ -void __i915_add_request(struct drm_i915_gem_request *request, - struct drm_i915_gem_object *obj, - bool flush_caches) +void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches) { struct intel_engine_cs *engine; struct intel_ring *ring; @@ -504,14 +502,6 @@ void __i915_add_request(struct drm_i915_gem_request *request, request->head = request_start; - /* Whilst this request exists, batch_obj will be on the - * active_list, and so will hold the active reference. Only when this - * request is retired will the the batch_obj be moved onto the - * inactive_list and lose its active reference. Hence we do not need - * to explicitly hold another reference here. - */ - request->batch_obj = obj; - /* Seal the request and mark it as pending execution. Note that * we may inspect this state, without holding any locks, during * hangcheck. Hence we apply the barrier to ensure that we do not diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h index 721eb8cbce9b..d5176f9cc22f 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.h +++ b/drivers/gpu/drm/i915/i915_gem_request.h @@ -225,13 +225,11 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst, *pdst = src; } -void __i915_add_request(struct drm_i915_gem_request *req, - struct drm_i915_gem_object *batch_obj, - bool flush_caches); +void __i915_add_request(struct drm_i915_gem_request *req, bool flush_caches); #define i915_add_request(req) \ - __i915_add_request(req, NULL, true) + __i915_add_request(req, true) #define i915_add_request_no_flush(req) \ - __i915_add_request(req, NULL, false) + __i915_add_request(req, false) struct intel_rps_client; #define NO_WAITBOOST ERR_PTR(-1)
request->batch_obj is only set by execbuffer for the convenience of debugging hangs. By moving that operation to the callsite, we can simplify all other callers and future patches. We also move the complications of reference handling of the request->batch_obj next to where the active tracking is set up for the request. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++- drivers/gpu/drm/i915/i915_gem_request.c | 12 +----------- drivers/gpu/drm/i915/i915_gem_request.h | 8 +++----- 3 files changed, 13 insertions(+), 17 deletions(-)