From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Vivi Subject: Re: [PATCH 1/2] drm/i915: Collect gtier properly on HSW. Date: Wed, 30 Jul 2014 11:17:39 -0700 Message-ID: References: <1406560776-29622-1-git-send-email-rodrigo.vivi@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1575555297==" Return-path: Received: from mail-vc0-f175.google.com (mail-vc0-f175.google.com [209.85.220.175]) by gabe.freedesktop.org (Postfix) with ESMTP id 9DD186E15C for ; Wed, 30 Jul 2014 11:17:40 -0700 (PDT) Received: by mail-vc0-f175.google.com with SMTP id ik5so2432897vcb.6 for ; Wed, 30 Jul 2014 11:17:40 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni , Rodrigo Vivi List-Id: intel-gfx@lists.freedesktop.org --===============1575555297== Content-Type: multipart/alternative; boundary=bcaec51a89bae729bc04ff6d2b83 --bcaec51a89bae729bc04ff6d2b83 Content-Type: text/plain; charset=UTF-8 On Wed, Jul 30, 2014 at 10:53 AM, Paulo Zanoni wrote: > 2014-07-28 12:19 GMT-03:00 Rodrigo Vivi : > > GTIER and DEIER doesn't have same interface on HSW so this "or" operation > > makes the information provided useless. > > > > Cc: Paulo Zanoni > > Signed-off-by: Rodrigo Vivi > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------ > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > > index ef38c3b..ccb97f1 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -314,6 +314,7 @@ struct drm_i915_error_state { > > u32 eir; > > u32 pgtbl_er; > > u32 ier; > > + u32 gtier; > > u32 ccid; > > u32 derrmr; > > u32 forcewake; > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > b/drivers/gpu/drm/i915/i915_gpu_error.c > > index 0b3f694..372fea3 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct > drm_i915_error_state_buf *m, > > err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device); > > err_printf(m, "EIR: 0x%08x\n", error->eir); > > err_printf(m, "IER: 0x%08x\n", error->ier); > > + if (IS_HASWELL(dev)) > > + err_printf(m, "GTIER: 0x%08x\n", error->gtier); > > err_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er); > > err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake); > > err_printf(m, "DERRMR: 0x%08x\n", error->derrmr); > > @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct > drm_i915_private *dev_priv, > > if (HAS_HW_CONTEXTS(dev)) > > error->ccid = I915_READ(CCID); > > > > - if (HAS_PCH_SPLIT(dev)) > > + if (IS_HASWELL(dev)) { > > + error->ier = I915_READ(DEIER); > > + error->gtier = I915_READ(GTIER); > > + } else if (HAS_PCH_SPLIT(dev)) { > > error->ier = I915_READ(DEIER) | I915_READ(GTIER); > > You did a change for HSW only, but we have these bits since Gen5. Why > don't you do this change for the whole HAS_PCH_SPLIT chunk instead of > adding a HSW-specific piece? > > I am not a huge user of these error state files, I can't really think > why we would want to "or" the IER bits, so your patch looks correct to > me. > 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. > > > > - else { > > - if (IS_GEN2(dev)) > > - error->ier = I915_READ16(IER); > > - else > > - error->ier = I915_READ(IER); > > + } else if (IS_GEN2(dev)) { > > + error->ier = I915_READ16(IER); > > + } else { > > + error->ier = I915_READ(IER); > > While reviewing your patch I also noticed that at the top of this > function we set error->ier for VLV, but then at this point we just > overwrite what was previously set. You could write another patch to > fix VLV too :) > Yeah, it is messy... I'll also split for VLV and organize a bit to avoid overwriting... > > > > } > > > > /* 4: Everything else */ > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Rodrigo Vivi Blog: http://blog.vivi.eng.br --bcaec51a89bae729bc04ff6d2b83 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



On Wed, Jul 30, 2014 at 10:53 AM, Paulo Zanoni &l= t;przanoni@gmail.co= m> wrote:
2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <= rodrigo.vivi@intel.com>:
> GTIER and DEIER doesn't have same interface= on HSW so this "or" operation
> makes the information provided useless.
>
> Cc: Paulo Zanoni <paulo= .r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> =C2=A0drivers/gpu/drm/i915/i915_drv.h =C2=A0 =C2=A0 =C2=A0 | =C2=A01 +=
> =C2=A0drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------
> =C2=A02 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i9= 15_drv.h
> index ef38c3b..ccb97f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -314,6 +314,7 @@ struct drm_i915_error_state {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 eir;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 pgtbl_er;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 ier;
> + =C2=A0 =C2=A0 =C2=A0 u32 gtier;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 ccid;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 derrmr;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 forcewake;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i= 915/i915_gpu_error.c
> index 0b3f694..372fea3 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct drm_i915_error_= state_buf *m,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "PCI ID: 0x%04x\n"= , dev->pdev->device);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "EIR: 0x%08x\n", e= rror->eir);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "IER: 0x%08x\n", e= rror->ier);
> + =C2=A0 =C2=A0 =C2=A0 if (IS_HASWELL(dev))
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "= ;GTIER: 0x%08x\n", error->gtier);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "PGTBL_ER: 0x%08x\n&quo= t;, error->pgtbl_er);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "FORCEWAKE: 0x%08x\n&qu= ot;, error->forcewake);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 err_printf(m, "DERRMR: 0x%08x\n"= , error->derrmr);
> @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct drm_= i915_private *dev_priv,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (HAS_HW_CONTEXTS(dev))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->ccid= =3D I915_READ(CCID);
>
> - =C2=A0 =C2=A0 =C2=A0 if (HAS_PCH_SPLIT(dev))
> + =C2=A0 =C2=A0 =C2=A0 if (IS_HASWELL(dev)) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->ier =3D I= 915_READ(DEIER);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->gtier =3D= I915_READ(GTIER);
> + =C2=A0 =C2=A0 =C2=A0 } else if (HAS_PCH_SPLIT(dev)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->ier = =3D I915_READ(DEIER) | I915_READ(GTIER);

You did a change for HSW only, but we have these bits since Gen= 5. Why
don't you do this change for the whole HAS_PCH_SPLIT chunk instead of adding a HSW-specific piece?

I am not a huge user of these error state files, I can't really think why we would want to "or" the IER bits, so your patch looks corre= ct to
me.

I believe before HSW they had the s= ame interface both DEIR and GTIER, but since I'm splitting it for HSW y= ou are right we can split everywhere else with or without common interface.=
=C2=A0


> - =C2=A0 =C2=A0 =C2=A0 else {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_GEN2(dev)) > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 error->ier =3D I915_READ16(IER);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 error->ier =3D I915_READ(IER);
> + =C2=A0 =C2=A0 =C2=A0 } else if (IS_GEN2(dev)) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->ier =3D I= 915_READ16(IER);
> + =C2=A0 =C2=A0 =C2=A0 } else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error->ier =3D I= 915_READ(IER);

While reviewing your patch I also noticed that at the top of this
function we set error->ier for VLV, but then at this point we just
overwrite what was previously set. You could write another patch to
fix VLV too :)

Yeah, it is messy... =C2= =A0I'll also split for VLV and organize a bit to avoid overwriting...
=C2=A0


> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* 4: Everything else */
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.fre= edesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx=



--
Paulo Zanoni
_____________________= __________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesk= top.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
=
Rodrigo Vivi
=C2=A0
--bcaec51a89bae729bc04ff6d2b83-- --===============1575555297== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1575555297==--