[Intel-gfx] [PATCH 08/11] drm/i915: Update write_domains on active list after flush.

[Chris Wilson]

On Mon,  1 Feb 2010 13:59:23 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Before changing the status of a buffer with a pending write we will await
> upon a new flush for that buffer. So we can take advantage of any flushes
> posted whilst the buffer is active and pending processing by the GPU, by
> clearing its write_domain and updating its last_rendering_seqno -- thus
> saving a potential flush in deep queues and improves flushing behaviour
> upon eviction for both GTT space and fences.
> 
> In order to reduce the time spent searching the active list for matching
> write_domains, we move those to a separate list whose elements are
> the buffers belong to the active/flushing list with pending writes.
> 
> Orignal patch by Chris Wilson <chris at chris-wilson.co.uk>, forward-ported
> by me.
> 
> In addition to better performance, this also fixes a real bug. Before
> this changes, i915_gem_evict_everything didn't work as advertised. When
> the gpu was actually busy and processing request, the flush and subsequent
> wait would not move active and dirty buffers to the inactive list, but
> just to the flushing list. Which triggered the BUG_ON at the end of this
> function. With the more tight dirty buffer tracking, all currently busy and
> dirty buffers get moved to the inactive list by one i915_gem_flush operation.
> 
> I've left the BUG_ON I've used to prove this in there.
> 
> References:
>   Bug 25911 - 2.10.0 causes kernel oops and system hangs
>   http://bugs.freedesktop.org/show_bug.cgi?id=25911
> 
>   Bug 26101 - [i915] xf86-video-intel 2.10.0 (and git) triggers kernel oops
>               within seconds after login
>   http://bugs.freedesktop.org/show_bug.cgi?id=26101
> 
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: stable at kernel.org

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

Just a single comment about something I've mulling over...

>  	for (i = 0; i < args->buffer_count; i++) {
>  		struct drm_gem_object *obj = object_list[i];
> +		struct drm_i915_gem_object *obj_priv = obj->driver_private;
>  		uint32_t old_write_domain = obj->write_domain;
>  
>  		obj->write_domain = obj->pending_write_domain;
> +		if (obj->write_domain)
> +			list_move_tail(&obj_priv->gpu_write_list,
> +				       &dev_priv->mm.gpu_write_list);
> +		else
> +			list_del_init(&obj_priv->gpu_write_list);
> +

I think the write list must be empty here if the write domain is clear..
Do you agree?
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre