<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 30, 2014 at 10:53 AM, Paulo Zanoni <span dir="ltr"><<a href="mailto:przanoni@gmail.com" target="_blank">przanoni@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>>:<br>
<div><div class="h5">> GTIER and DEIER doesn't have same interface on HSW so this "or" operation<br>
> makes the information provided useless.<br>
><br>
> Cc: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br>
> Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>><br>
> ---<br>
> drivers/gpu/drm/i915/i915_drv.h | 1 +<br>
> drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------<br>
> 2 files changed, 11 insertions(+), 6 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h<br>
> index ef38c3b..ccb97f1 100644<br>
> --- a/drivers/gpu/drm/i915/i915_drv.h<br>
> +++ b/drivers/gpu/drm/i915/i915_drv.h<br>
> @@ -314,6 +314,7 @@ struct drm_i915_error_state {<br>
> u32 eir;<br>
> u32 pgtbl_er;<br>
> u32 ier;<br>
> + u32 gtier;<br>
> u32 ccid;<br>
> u32 derrmr;<br>
> u32 forcewake;<br>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c<br>
> index 0b3f694..372fea3 100644<br>
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c<br>
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c<br>
> @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,<br>
> err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device);<br>
> err_printf(m, "EIR: 0x%08x\n", error->eir);<br>
> err_printf(m, "IER: 0x%08x\n", error->ier);<br>
> + if (IS_HASWELL(dev))<br>
> + err_printf(m, "GTIER: 0x%08x\n", error->gtier);<br>
> err_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);<br>
> err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake);<br>
> err_printf(m, "DERRMR: 0x%08x\n", error->derrmr);<br>
> @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,<br>
> if (HAS_HW_CONTEXTS(dev))<br>
> error->ccid = I915_READ(CCID);<br>
><br>
> - if (HAS_PCH_SPLIT(dev))<br>
> + if (IS_HASWELL(dev)) {<br>
> + error->ier = I915_READ(DEIER);<br>
> + error->gtier = I915_READ(GTIER);<br>
> + } else if (HAS_PCH_SPLIT(dev)) {<br>
> error->ier = I915_READ(DEIER) | I915_READ(GTIER);<br>
<br>
</div></div>You did a change for HSW only, but we have these bits since Gen5. Why<br>
don't you do this change for the whole HAS_PCH_SPLIT chunk instead of<br>
adding a HSW-specific piece?<br>
<br>
I am not a huge user of these error state files, I can't really think<br>
why we would want to "or" the IER bits, so your patch looks correct to<br>
me.<br></blockquote><div><br></div><div>I believe before HSW they had the same interface both DEIR and GTIER, but since I'm splitting it for HSW you are right we can split everywhere else with or without common interface.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
<br>
> - else {<br>
> - if (IS_GEN2(dev))<br>
> - error->ier = I915_READ16(IER);<br>
> - else<br>
> - error->ier = I915_READ(IER);<br>
> + } else if (IS_GEN2(dev)) {<br>
> + error->ier = I915_READ16(IER);<br>
> + } else {<br>
> + error->ier = I915_READ(IER);<br>
<br>
</div>While reviewing your patch I also noticed that at the top of this<br>
function we set error->ier for VLV, but then at this point we just<br>
overwrite what was previously set. You could write another patch to<br>
fix VLV too :)<br></blockquote><div><br></div><div>Yeah, it is messy... I'll also split for VLV and organize a bit to avoid overwriting...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im HOEnZb"><br>
<br>
> }<br>
><br>
> /* 4: Everything else */<br>
> --<br>
> 1.9.3<br>
><br>
> _______________________________________________<br>
> Intel-gfx mailing list<br>
> <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
<br>
<br>
<br>
</div><span class="HOEnZb"><font color="#888888">--<br>
Paulo Zanoni<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
Intel-gfx mailing list<br>
<a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div></div>