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.
As for your second point, assuming it's the code above, I don't quite
follow what you mean. Error code generation shouldn't be based upon
inactive rings. As for whether it changes any of the functionality, it
does not - but that wasn't the point of that hunk.
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
