Re: [PATCH] drm/i915: Fix some NUM_RING iterators

[Daniel Vetter <daniel@ffwll.ch>]

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 Behalf
> > 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,
> > > > > >               * 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