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@xxxxxxxxxxxxxxxxxxxxx] On Behalf > > Of Ben Widawsky > > Sent: Saturday, June 28, 2014 10:56 PM > > To: Chris Wilson; Rodrigo Vivi; Widawsky, Benjamin; Intel GFX > > Subject: Re: [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, > > > > > > * synchronization commands which almost always appear in > > the > > > > > > case > > > > > > * strictly a client bug. Use instdone to differentiate those > > > > > > some. > > > > > > */ > > > > > > - for (i = 0; i < I915_NUM_RINGS; i++) { > > > > > > + for (i = 0; i < I915_ACTIVE_RINGS(dev_priv->dev); i++) { > > > > > > if (error->ring[i].hangcheck_action == > > HANGCHECK_HUNG) { > > > > > > if (ring_id) > > > > > > *ring_id = 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 intel_engine_cs { > > > > > > } id; > > > > > > #define I915_NUM_RINGS 5 > > > > > > #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 bugs > > introduced in patches I've not yet submitted, so I think for that reason, 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´t > really justify it on the Execlists series anymore (on the latest > versions we don´t 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
