On 19/06/15 18:02, Dave Gordon wrote: > On 15/06/15 22:32, Chris Wilson wrote: [snip] >> Try to keep comments to explain why rather than what. Most of the >> comments here fall into the "i++; // postincrement i" category. >> -Chris > > Most of the "what" comments in this file are associated with accesses to > specific h/w registers, which therefore have semantic effect beyond what > is explicit in the code. For example this comment: > > /* tell all command streamers to forward interrupts and vblank to GuC */ > irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK,GFX_FORWARD_VBLANK_ALWAYS); > irqs |= _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING); > for_each_ring(ring, dev_priv, i) > I915_WRITE(RING_MODE_GEN7(ring), irqs); > > helps the reader what the /effect/ of the writes is intended to be. It's > quite different from: > > /* write bitmask to GEN7 ring mode register */ > I915_WRITE(RING_MODE_GEN7(ring),MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING)); > > and means you may not have to dig through the BSpec to find out what the > less helpfully-named bits actually do. And this: > > I915_WRITE(DE_GUCRMR, ~0); > > would be incomprehensible without reading the BSpec ... or the comment > > /* tell DE to send nothing to GuC */ > > .Dave. Oops, those comments aren't actually in this patch, they're in a later one. But they *will* be in this file by the end of the patchset ... .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
