[Intel-gfx] anholt/for-review 2008-11-14

[Keith Packard]

Overall, this patch sequence looks good, and we're starting to see a
more consistent set of invariants around domain settings and list
management.

74ff09cbf7178b9d363624076eb5c5dc50f23c98	ACK
42377ada91e674b5b31363e30a3fddb160e78e5b	ACK
35a2271ac627abe3516c1e20ac1ba2413153aa42	ACK
f52e168a76e26e3edf5151c1799f4f7503db8fd9

        Should move_to_flushing clear obj_priv->active?
        
        I still think we want some indication of which buffers are in
        the active list because they're being flushed, and which are in
        the list strictly because they're being read from. An
        optimization for CPU/GPU read sharing, to be sure.
        
0d4a8276c1883005d32c92ffb9fa0139f051d979	NAK

        I think we should prepare all of the state transitions that
        buffers will undergo and assign them atomically once we're past
        the point of no return. Flushing in the error path like this
        seems like an ugly kludge. Better to have precisely one place
        where
        the state is updated.
        

1fc8fbe7899d543b2fbf44d883a15fd5d32b37f7

        +	/* Figure out what GPU domains we need to flush or invalidate for
        +	 * moving to GTT.
        +	 */
        +	flush_domains = obj->write_domain & I915_GEM_GPU_DOMAINS;
        
I'd call this gpu_flush_domains instead; it doesn't include the cpu flush
which you do later in the function.

        +	/* If we're writing through the GTT domain, then CPU and GPU caches
        +	 * will need to be invalidated at next use.
        +	 */
        +	if (write)
        +		obj->read_domains &= ~(I915_GEM_GPU_DOMAINS |
        +					    I915_GEM_DOMAIN_CPU);

I think this can just be = 0

        +		obj->write_domain &= ~I915_GEM_DOMAIN_CPU;

And, here = 0?

98d1eedbcba2cbb9f455710e1c2b23247577ae81

In wait_rendering, you don't flush and you BUG_ON if there is dirty
data. Is this function really 'wait_active'?


        +	if ((obj->write_domain & I915_GEM_GPU_DOMAINS) == 0)
        +		return;
        
        vs
        
        +	if (obj->write_domain != I915_GEM_DOMAIN_CPU)
        +		return;

I think we should be consistent about how we test for write_domain
settings; or is that a silly complaint?

I like what set_to_gtt_domain looks like now. It's quite clear.

        +	/* No actual flushing is required for the GTT write domain */
        +	if (obj->write_domain == I915_GEM_DOMAIN_GTT)
        +		obj->write_domain = 0;

Do you want to add a flush_gtt_write_domain function, just for symmetry?
Would make a nice spot to stick that comment.

        +	/* If we have a partially-valid cache of the object in the CPU,
        +	 * finish invalidating it and free the per-page flags.

That needs to go in a separate function please.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081114/748ad4f4/attachment.sig>