[Intel-gfx] anholt/for-review 2008-11-14
keithp at keithp.com
Fri Nov 14 22:09:49 PST 2008
Overall, this patch sequence looks good, and we're starting to see a
more consistent set of invariants around domain settings and list
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.
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
the state is updated.
+ /* 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 |
I think this can just be = 0
+ obj->write_domain &= ~I915_GEM_DOMAIN_CPU;
And, here = 0?
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)
+ if (obj->write_domain != I915_GEM_DOMAIN_CPU)
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: not available
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/intel-gfx/attachments/20081114/748ad4f4/attachment.pgp
More information about the Intel-gfx