Re: [PATCH 02/15] drm/i915: Accurately track flushed domains

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2019 bytes --]


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@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx