From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Siluvery, Arun" Subject: Re: [PATCH 2/2] drm/i915/bdw: Export workaround data to debugfs Date: Sun, 31 Aug 2014 20:43:49 +0100 Message-ID: <54037AF5.5030304@linux.intel.com> References: <1409060691-4916-1-git-send-email-arun.siluvery@linux.intel.com> <1409060691-4916-3-git-send-email-arun.siluvery@linux.intel.com> <20140830151035.GA30509@strange.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 688516E19A for ; Sun, 31 Aug 2014 12:43:51 -0700 (PDT) In-Reply-To: <20140830151035.GA30509@strange.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 30/08/2014 16:10, Damien Lespiau wrote: > On Tue, Aug 26, 2014 at 02:44:51PM +0100, Arun Siluvery wrote: >> The workarounds that are applied are exported to a debugfs file; >> this is used to verify their state after the test case (reset or >> suspend/resume etc). This patch is only required to support i-g-t. > > I'm really, really confused. Please bear with me. I have reworked all the patches hopefully things will be more clear this time :) > > 1. We only deal with masked registers AFAICS. Those registers have the > high 16 bits masking the writes. > > 2. The values given to intel_ring_emit_wa() are the actual values we're going > to write in the register, so they include those mask bits. say: > > intel_ring_emit_wa(ring, GEN7_ROW_CHICKEN2, > _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE)); > > 3. We then record in intel_wa_regs the reg address and two fields named mask > and value. > > 3. a) mask > > intel_wa_regs[dev_priv->num_wa_regs].mask = value & 0xFFFF; > > You're selecting the low 16bits and put it in mask. But the masked bits are the > upper 16bits? This may work when the W/A is about setting bits, but we have a > bug if we ever have a W/A that is about clearing a bit. It would seem better to > me to grab the upper bits which are, after all, the bitmask we're interested in. > it is a valid issue, changed to use upper 16-bits as mask. > 3. b) value > > /* value is updated with the status of remaining bits of this > * register when it is read from debugfs file > */ > dev_priv->intel_wa_regs[dev_priv->num_wa_regs].value = value; > > I don't understand what the comment explains. The *why* we need to do that is > missing and, frankly, having to update the reference values we capture at > intel_ring_emit_wa() time sounds like a bug to me. > > I also take a note that, at this, point, intel_wa_regs.value contains both the > value and the mask. Weird. > I agree the why part is missing. The idea is value represents the status of other bits in this register besides w/a bit; this is actually redundant here, I guess I added because I wanted to initialize all members. This change is not applicable in the new patches. > 4. Time to expose that intel_wa_regs array to user space > > 4. a) mask > > mask = dev_priv->intel_wa_regs[i].mask > > Straigthforward enough, except that these are still the lower 16 bits, so the > value really. > > 4. b) value > > dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask; > > Hum? This really started my journey to dig futher. So we: > > - override the reference value from intel_ring_emit_wa() with whatever we > have in the register at that moment > > - Or it with a mask that's not really a mask (but the reference value) > > 5. igt test > > So you grab those mask and value fields from the debugs file and read the > register through mapped MMIO. and then > > status = (current_wa[i].value & current_wa[i].mask) != > (wa_regs[i].value & wa_regs[i].mask); > > So that's where I'm starting to put things back together and understand what > the intention is. I still think that's not quite right, especially how we get > the mask and why we read back the register in the debugfs file. > We read the register value after the test case (eg reset) and compare it with a known value that is exported to debugfs file. regards Arun > Or am I just missing something? In any case, having to spend that much time > trying to understand what's going on is a maintainability problem, we need code > that a least looks straightforward. > > HTH, >