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

[Eric Anholt]

On Fri, 2008-11-14 at 22:09 -0800, Keith Packard wrote:
> 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?

No, obj_priv->active is used for the test for whether we have a refcount
on it due to being in the ringbuffer (active list) or needing to make it
back onto the ringbuffer before freeing (flushing list)

> 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.

Yeah, we can discern that with testing both active and
last_rendering_seqno, but it's not pretty.

> 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.

Sounds like we've got resolution on this with your new patch series.

> 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.

Which you have, I'll pull that.

>         +	/* 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

Replaced with &= DOMAIN_GTT (even though = 0 would be harmless)

>         +		obj->write_domain &= ~I915_GEM_DOMAIN_CPU;
> 
> And, here = 0?

Fixed

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

Since the implementation is "wait for it to drop off the active list",
and all our consumers only want to do that if they're actually waiting
for it to go to inactive, that's what I asserted.  It appears to be safe
in my review.

There actually is a need for a distinction between "active because it's
being rendered to" and "active because it's hasn't been flushed yet".
Right now, a finished occlusion query on an idle system might not end up
returning active == 0 since there's no pressure to get it flushed,
despite the spec saying that apps can expect it to happen eventually.
That's outside of the scope of these patches, but it does justify close
attention to these issues, and probably the right thing is to move
obj_priv->active to being obj_priv->state = {active,flushing,inactive}
to encourage care.

>         +	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?

Not sure if it's silly, not sure how much I care either.  I tend to just
type it how I'm thinking about it -- "is it something outside of this
set" or "is it not this exact one?"

> 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.

Yeah, that's kind of pretty.  Fixed.

>         +	/* 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.

Fixed.

-- 
Eric Anholt
eric at anholt.net                         eric.anholt at intel.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081125/0f81afa7/attachment.sig>