On Tue, Jun 09, 2015 at 04:56:01PM +0100, Tomas Elf wrote: > On 04/06/2015 19:23, John.C.Harrison@xxxxxxxxx wrote: > >From: John Harrison <John.C.Harrison@xxxxxxxxx> > > > >A previous patch (read-read optimisation) changed the early exit > >condition in i915_gem_retire_requests_ring() from checking the request > >list to checking the active list. This assumes that all requests have > >objects associated with them which are placed on the active list. The > >removal of the OLR means that non-batch buffer work is no longer > >tagged onto the nearest batch buffer submission and thus there are > >requests going through the system which do not have objects associated > >with them. This can therefore lead to the situation where an > >outstanding request never gets retired. > > > >This change reverts the early exit condition to check for requests. > >Given that the purpose of the function is to retire requests, this > >does seem to make much more sense. > > > >For: VIZ-5190 > >Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index 7117659..4c5a6cd 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -2859,7 +2859,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > > { > > WARN_ON(i915_verify_lists(ring->dev)); > > > >- if (list_empty(&ring->active_list)) > >+ if (list_empty(&ring->request_list)) > > return; > > > > /* Retire requests first as we use it above for the early return. > > > > Note to whoever is integrating this patch: This patch can either be applied > or we could drop the request_list check entirely. This according to Chris > Wilson in the following conversation: > > 3:26:09 PM - ickle: we just kill the check > 3:26:25 PM - ickle: the final function is just request_list + trace_request > 3:26:37 PM - ickle: adding a test to save one isn't a great tradeoff > 3:26:58 PM - siglesias has left the room (Quit: Ping timeout: 256 seconds). > 3:27:28 PM - twnqx has left the room (Quit: Ping timeout: 272 seconds). > 3:28:04 PM - tomas_elf: fine > 3:28:20 PM - tomas_elf: anyway, good to know > 3:29:32 PM - ickle: there's actually one more, it's also where the > execlists_retire should be > 3:30:48 PM - tomas_elf: maybe you can just submit a patch (unless you've > already done so) that removes all of the references > 3:31:09 PM - JohnHarrison Joryn > 3:31:23 PM - ickle: there's like a backlog of 50 patches before we even get > to that point > 3:31:29 PM - tomas_elf: ok, cool > 3:31:51 PM - tomas_elf: at any rate, JohnHarrison's patch can be accepted > either with the request_list check or no check at all > 3:33:14 PM - ickle: I thought vsyrjala sent the patch to completely kill it > along with a bug citation > 3:34:50 PM - doome_ [~doome@82.150.48.146] entered the room. > 3:35:25 PM - ickle: 0aedb1626566efd72b369c01992ee7413c82a0c5 > 3:35:39 PM - darkbasic_ [~quassel@xxxxxxxxxxxxxxxxxxxx] entered the room. > 3:36:13 PM - darkbasic has left the room (Quit: Read error: Connection reset > by peer). > 3:39:05 PM - tomas_elf: has it been merged? > 3:39:43 PM - ickle: it is in drm-intel-fixes > 3:40:02 PM - tomas_elf: ah, ok > > As long as the active_list check is removed since it breaks things. > > Reviewed-by: Tomas Elf <tomas.elf@xxxxxxxxx> Jani already picked up Ville's version of this for dinf: commit 11ee9615f9bbc9c0c2dbd9f5eb275459b76f032a Author: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Date: Thu May 28 18:32:36 2015 +0300 drm/i915: Don't skip request retirement if the active list is empty We should be covered here I think. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx