[Intel-gfx] [PATCH 02/15] drm/i915: Accurately track flushed domains

[Keith Packard]

I'd love to see the API changes done in separate patches so they can be
reviewed without being mixed in with the actual fixes. This patch
conflates the changing of the sequence number management, ring
synchronization, elimination of the file argument to i915_add_request
with the changes in flush domains.

Also, a commit message which describes in detail what is changing and
why would really help verify that the patch does what it is supposed
to.

>  static inline u32
>  i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
>  {
> -	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	return ring->outstanding_lazy_request = dev_priv->next_seqno;
> +	if (ring->outstanding_lazy_request == 0)
> +		ring->outstanding_lazy_request = __i915_gem_get_seqno(ring->dev);
> +	return ring->outstanding_lazy_request;
>  }

Why did the code not allocate a seqno for this before? Why is it
necessary now?

> +	if (i915_ring_outstanding_dispatch(ring)) {
> +		struct drm_i915_gem_request *request;
> +
> +		request = kzalloc(sizeof(*request), GFP_KERNEL);
> +		if (request && i915_add_request(ring, request))
> +			kfree(request);
> +	}
> +

Why is it OK to not add this request when out of memory?

> +		if (!list_empty(&ring->gpu_write_list))
>  			ret = i915_gem_flush_ring(ring,
>  						  0, I915_GEM_GPU_DOMAINS);
> -			request = kzalloc(sizeof(*request), GFP_KERNEL);
> -			if (ret || request == NULL ||
> -			    i915_add_request(ring, NULL, request))
> -			    kfree(request);
> -		}
> -
> -		idle &= list_empty(&ring->request_list);
> +		(void)ret;

If you're going to ignore a __must_check return value, you might at
least justify it in a comment.

>  int
> -i915_gem_flush_ring(struct intel_ring_buffer *ring,
> -		    uint32_t invalidate_domains,
> -		    uint32_t flush_domains)
> +i915_gem_flush_ring(struct intel_ring_buffer *ring, u32 invalidate,
> u32 flush)

Seems like gratuitous variable renaming here.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20110323/6efc313f/attachment.sig>