From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH 02/15] drm/i915: Accurately track flushed domains Date: Wed, 23 Mar 2011 06:07:52 +0900 Message-ID: References: <1300801920-23130-1-git-send-email-chris@chris-wilson.co.uk> <1300801920-23130-3-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1880147343==" Return-path: Received: from keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 0494C9EF53 for ; Tue, 22 Mar 2011 14:08:12 -0700 (PDT) In-Reply-To: <1300801920-23130-3-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1880147343== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" --=-=-= Content-Transfer-Encoding: quoted-printable 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 =3D ring->dev->dev_private; > - return ring->outstanding_lazy_request =3D dev_priv->next_seqno; > + if (ring->outstanding_lazy_request =3D=3D 0) > + ring->outstanding_lazy_request =3D __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 =3D 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 =3D i915_gem_flush_ring(ring, > 0, I915_GEM_GPU_DOMAINS); > - request =3D kzalloc(sizeof(*request), GFP_KERNEL); > - if (ret || request =3D=3D NULL || > - i915_add_request(ring, NULL, request)) > - kfree(request); > - } > - > - idle &=3D 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. =2D-=20 keith.packard@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iD8DBQFNiQ+oQp8BWwlsTdMRAmM8AKC6f/zfd9Ah9dTHnHJRp5LdpKv6vACeNRV1 V6DvRrdtFfAYT4FXZM8PEQU= =MQOB -----END PGP SIGNATURE----- --=-=-=-- --===============1880147343== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1880147343==--