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.pack...@intel.com
pgpznYNPRTGZN.pgp
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
