From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Fix some NUM_RING iterators Date: Mon, 7 Jul 2014 21:37:17 +0200 Message-ID: <20140707193717.GH5821@phenom.ffwll.local> References: <1403906985-11045-1-git-send-email-benjamin.widawsky@intel.com> <20140628062038.GA25660@nuc-i3427.alporthouse.com> <20140628152615.GC17803@bwidawsk.net> <20140628192855.GA5122@nuc-i3427.alporthouse.com> <20140628215545.GD17803@bwidawsk.net> <92648605EABDA246B775AAB04C95A7A3137E8A8A@IRSMSX103.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f46.google.com (mail-wg0-f46.google.com [74.125.82.46]) by gabe.freedesktop.org (Postfix) with ESMTP id B5CE76E476 for ; Mon, 7 Jul 2014 12:37:07 -0700 (PDT) Received: by mail-wg0-f46.google.com with SMTP id l18so1525157wgh.29 for ; Mon, 07 Jul 2014 12:37:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: <92648605EABDA246B775AAB04C95A7A3137E8A8A@IRSMSX103.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Mateo Lozano, Oscar" Cc: Ben Widawsky , "Widawsky, Benjamin" , Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Mon, Jun 30, 2014 at 11:27:25AM +0000, Mateo Lozano, Oscar wrote: > = > = > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > = > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Beh= alf > > Of Ben Widawsky > > Sent: Saturday, June 28, 2014 10:56 PM > > To: Chris Wilson; Rodrigo Vivi; Widawsky, Benjamin; Intel GFX > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix some NUM_RING iterators > > = > > On Sat, Jun 28, 2014 at 08:28:55PM +0100, Chris Wilson wrote: > > > On Sat, Jun 28, 2014 at 08:26:15AM -0700, Ben Widawsky wrote: > > > > On Sat, Jun 28, 2014 at 07:20:38AM +0100, Chris Wilson wrote: > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > > > > index 86362de..6e5250d 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > > > > @@ -848,7 +848,7 @@ static uint32_t > > i915_error_generate_code(struct > > > > > > drm_i915_private *dev_priv, > > > > > > =A0 =A0 =A0 =A0 =A0* synchronization commands which almost= always appear in > > the > > > > > > case > > > > > > =A0 =A0 =A0 =A0 =A0* strictly a client bug. Use instdone t= o differentiate those > > > > > > some. > > > > > > =A0 =A0 =A0 =A0 =A0*/ > > > > > > - =A0 =A0 =A0 for (i =3D 0; i < I915_NUM_RINGS; i++) { > > > > > > + =A0 =A0 =A0 for (i =3D 0; i < I915_ACTIVE_RINGS(dev_priv= ->dev); i++) { > > > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (error->ring[i].hangche= ck_action =3D=3D > > HANGCHECK_HUNG) { > > > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ring_i= d) > > > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 *ring_id =3D i; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > > > b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > > > index e72017b..67e2919 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > > > @@ -90,6 +90,8 @@ struct =A0intel_engine_cs { > > > > > > =A0 =A0 =A0 =A0 } id; > > > > > > =A0#define I915_NUM_RINGS 5 > > > > > > =A0#define LAST_USER_RING (VECS + 1) > > > > > > +#define I915_ACTIVE_RINGS(dev) > > > > > > hweight8(INTEL_INFO(dev)->ring_mask) > > > > > > > > > > What does the popcount of the mask have to do with the validity of > > > > > the arrays being iterated over in this patch? > > > > > -Chris > > > > > > > > The popcount of the mask represents the number of rings available on > > > > the specific SKU, as opposed to the total number of rings on any SKU > > ever. > > > > It is not always correct to iterate on all rings in the system. > > > > Please note, the patch is incomplete. I have a couple of other, > > > > perhaps more interesting, cases which I've missed. > > > > > > You still iterate over holes in the ring mask, and the iteration here > > > is over a completely different array, not rings. > > > -Chris > > = > > For the holes, I mentioned that in the commit message of the yet to be > > submitted v2; it's not really an issue in the way things are today. > > When/if we add a new ring, it will be. > > = > > What you're asking for has already been submitted multiple times with > > seemingly no traction. I do realize the fixes (with my v2) are due to b= ugs > > introduced in patches I've not yet submitted, so I think for that reaso= n, it's > > fair to drop this patch. > > = > > I'd rather the other patch get in (for_each_active_ring), but it's tied= up with > > execlists atm, and I continue to think this is a useful way to iterate = over the > > rings in error conditions and during reset. > = > I dropped that patch, since it received some resistance and I couldn=B4t > really justify it on the Execlists series anymore (on the latest > versions we don=B4t introduce new for i < I915_NUM_RINGS). I imagine the > patch could be sent again as a standalone? With Chris' patch to no longer tear down ring structures over reset/system suspend we should be able to always use ring_for_each. If not that means we still have some fun to look at. In any case I'm always happy to merge such drive-by cleanup patches, no need to have a big patch series to justify it. Well as long as it's indeed a step forward, which occasionally is a contentions topic ... -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch