Re: [PATCH 09/15] drm/i915: GuC submission setup, phase 1

From: Dave Gordon <david.s.gordon@intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/15] drm/i915: GuC submission setup, phase 1
Date: Fri, 19 Jun 2015 18:22:24 +0100	[thread overview]
Message-ID: <55844FD0.4040301@intel.com> (raw)
In-Reply-To: <55844B29.3040605@intel.com>

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx