On Wed, Jan 29, 2014 at 05:05:39PM +0200, Mika Kuoppala wrote: > As we seek the guilty batch using request and hangcheck > score, this code is not needed anymore. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_gem.c | 91 ++------------------------------------- > 1 file changed, 4 insertions(+), 87 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index a46a1a7..8637898 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2241,70 +2241,6 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request) > spin_unlock(&file_priv->mm.lock); > } > > -static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj, > - struct i915_address_space *vm) > -{ > - if (acthd >= i915_gem_obj_offset(obj, vm) && > - acthd < i915_gem_obj_offset(obj, vm) + obj->base.size) > - return true; > - > - return false; > -} > - > -static bool i915_head_inside_request(const u32 acthd_unmasked, > - const u32 request_start, > - const u32 request_end) > -{ > - const u32 acthd = acthd_unmasked & HEAD_ADDR; > - > - if (request_start < request_end) { > - if (acthd >= request_start && acthd < request_end) > - return true; > - } else if (request_start > request_end) { > - if (acthd >= request_start || acthd < request_end) > - return true; > - } > - > - return false; > -} > - > -static struct i915_address_space * > -request_to_vm(struct drm_i915_gem_request *request) > -{ > - struct drm_i915_private *dev_priv = request->ring->dev->dev_private; > - struct i915_address_space *vm; > - > - if (request->ctx) > - vm = request->ctx->vm; > - else > - vm = &dev_priv->gtt.base; > - > - return vm; > -} > - > -static bool i915_request_guilty(struct drm_i915_gem_request *request, > - const u32 acthd, bool *inside) > -{ > - /* There is a possibility that unmasked head address > - * pointing inside the ring, matches the batch_obj address range. > - * However this is extremely unlikely. > - */ > - if (request->batch_obj) { > - if (i915_head_inside_object(acthd, request->batch_obj, > - request_to_vm(request))) { > - *inside = true; > - return true; > - } > - } > - > - if (i915_head_inside_request(acthd, request->head, request->tail)) { > - *inside = false; > - return true; > - } > - > - return false; > -} > - > static bool i915_context_is_banned(const struct i915_hw_context *ctx) > { > struct drm_i915_private *dev_priv; > @@ -2329,30 +2265,11 @@ static bool i915_context_is_banned(const struct i915_hw_context *ctx) > return false; > } > > -static void i915_set_reset_status(struct intel_ring_buffer *ring, > - struct drm_i915_gem_request *request, > - const bool guilty) > +static void i915_set_reset_status(struct i915_hw_context *ctx, > + const bool guilty) > { > - const u32 acthd = intel_ring_get_active_head(ring); > - bool inside; > - unsigned long offset = 0; > - struct i915_hw_context *ctx = request->ctx; > struct i915_ctx_hang_stats *hs; > > - if (request->batch_obj) > - offset = i915_gem_obj_offset(request->batch_obj, > - request_to_vm(request)); > - > - if (guilty && > - i915_request_guilty(request, acthd, &inside)) { > - DRM_DEBUG("%s hung %s bo (0x%lx ctx %d) at 0x%x\n", > - ring->name, > - inside ? "inside" : "flushing", > - offset, > - ctx ? ctx->id : 0, > - acthd); > - } > - > if (WARN_ON(!ctx)) > return; > > @@ -2407,10 +2324,10 @@ static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv, > > ring_hung = ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG; > > - i915_set_reset_status(ring, request, ring_hung); > + i915_set_reset_status(request->ctx, ring_hung); > > list_for_each_entry_continue(request, &ring->request_list, list) > - i915_set_reset_status(ring, request, false); > + i915_set_reset_status(request->ctx, false); > } > > static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, I can't say that I'm a huge fan of calling i915_set_reset_status twice (I bit my lip while reading the last patch). To me it suggests that the interface probably ended up a bit poorly designed. I can live with it though, I just couldn't bite my lip for 2 patches in a row :-) I guess I've missed how this solves the issue I poked about in the original series. However, the code overall is a big improvement, and even in the unlikely case that I am not just being blind to your solution- the odds of having multiple hung rings are slim enough that I can live with that either way. I did put some requests in the patches. Each already had an unconditional r-b. Therefore, the series is: Reviewed-by: Ben Widawsky <ben@xxxxxxxxxxxx> Last word: As I've discussed with Chris too, I am overall a bit wary of removing any use upon hardware for doing a lot of these error triage, detection and collection functions. I really like that no matter how bonghits our driver gets, we can read certain registers to try to figure things out. I say this now since I think after this series I will no longer have a leg to stand on in the, we shouldn't use requests for error collection, discussion. Thanks for reading my rant. -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx