Re: [PATCH 14/56] drm/i915: Make retire condition check for requests not objects

[Tomas Elf <tomas.elf@intel.com>]

On 04/06/2015 19:23, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> 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@Intel.com>
> ---
>   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@niko.linuxsystems.it] 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@intel.com>

Thanks,
Tomas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx