From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic Date: Wed, 07 Sep 2011 21:31:05 -0700 Message-ID: References: <1315437162-14312-1-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1551432645==" Return-path: Received: from keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 8997FA12B7 for ; Wed, 7 Sep 2011 21:31:09 -0700 (PDT) In-Reply-To: <1315437162-14312-1-git-send-email-ben@bwidawsk.net> 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: intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org --===============1551432645== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" --=-=-= Content-Transfer-Encoding: quoted-printable On Wed, 7 Sep 2011 16:12:41 -0700, Ben Widawsky wrote: > -update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno) > +update_mboxes(struct intel_ring_buffer *ring, > + u32 seqno, > + u32 mmio_offset) Yeah, definitely like this change; lots less magic here. > -static int > +/** > + * gen6_add_request - Update the semaphore mailbox registers > + *=20 > + * @ring - ring that is adding a request > + * @mbox1_reg - mailbox address for RCS or VCS ring > + * @mbox2_reg - mailbox address for VCS or BCS ring > + * > + * Update the mailbox registers in the *other* rings with the current se= qno. > + * This acts like a signal in the canonical semaphore. > + */ > +static u32 > gen6_add_request(struct intel_ring_buffer *ring, > - u32 *result) > + u32 mbox1_reg, > + u32 mbox2_reg) I think you're losing the ability to return errors from here. > u32 seqno; > int ret; > + seqno =3D i915_gem_get_seqno(ring->dev); >=20=20 > ret =3D intel_ring_begin(ring, 10); > if (ret) > return ret; >=20=20 > - seqno =3D i915_gem_get_seqno(ring->dev); Why change the ordering of get_seqno relative to ring_begin here? > +static int > +gen6_blt_add_request(struct intel_ring_buffer *ring, > + u32 *result) > +{ > + struct drm_device *dev =3D ring->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + *result =3D gen6_add_request(ring, > + dev_priv->ring[RCS].mmio_base + 0x44, > + dev_priv->ring[VCS].mmio_base + 0x40); > return 0; Why the magic constants? Can we have named values? And, note that this function never returns an error value, which is definitely not a good plan. > + temp |=3D MI_SEMAPHORE_REGISTER; temp is a constant, why is it being |=3D'd here? > +/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */ > +int > +render_ring_sync_to(struct intel_ring_buffer *waiter, > + struct intel_ring_buffer *signaller, > + u32 seqno) > +{ > + WARN_ON(signaller->semaphore_register[RCS] =3D=3D MI_SEMAPHORE_SYNC_INV= ALID); > + return intel_ring_sync(waiter, > + signaller, > + signaller->semaphore_register[RCS], Should you just pass the index instead of the register value itself? Otherwise, this seems like a reasonable change to me. =2D-=20 keith.packard@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iD8DBQFOaEUJQp8BWwlsTdMRAriSAJ4xPNvJOkVvrKSd580w8PbJkdGRhACeJBY+ 43x9fJDNa/9HtV+YUZE733M= =xt6h -----END PGP SIGNATURE----- --=-=-=-- --===============1551432645== 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 --===============1551432645==--