[Intel-gfx] [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-20 12:12:02)
> Joonas Lahtinen <joonas.lahtinen at linux.intel.com> writes:
> >> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> >>                      }
> >>  
> >>                      /* After the final element, the hw should be idle */
> >> -                    GEM_BUG_ON(port_count(port) == 0 &&
> >> +                    GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
> >>                                 !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
> >
> > Why did you stop trusting port variable here?
> >
> 
> We did assing it pre loop before. Now we do it inside the loop. Also
> I thought I made a favour for reader (and for the bug triager
> as GEM_BUG_ON might soon show condition in dmesg) 
> to note that it is always the first port count we assert
> for idleness.

I would favour assert(port == el_port_head(el)); where appropriate.
The intent is that we comment upon the rationale of the GEM_BUG_ON() so
that we have an aide-memoire as to where to begin the hunt. For
triaging, we just need to be able to recognise the same BUG_ON as you
only need to dupe to the first one to catch up with the bug analysis.

We should err on making GEM_BUG_ON() fit the code better than a
potential bug report. We should be reading the code far more often...
-Chris