From mboxrd@z Thu Jan 1 00:00:00 1970 From: Damien Lespiau Subject: Re: [PATCH 2/2] drm/i915/bdw: Export workaround data to debugfs Date: Sat, 30 Aug 2014 16:10:35 +0100 Message-ID: <20140830151035.GA30509@strange.ger.corp.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 5FCD66E085 for ; Sat, 30 Aug 2014 08:11:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1409060691-4916-3-git-send-email-arun.siluvery@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Arun Siluvery Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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. 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. 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. 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. 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, -- Damien