Re: [PATCH 1/2] drm/i915: Collect gtier properly on HSW.

[Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>]

On Wed, Jul 30, 2014 at 10:53 AM, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:

2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>:

> 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@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Paulo Zanoni

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--

Rodrigo Vivi

Blog: http://blog.vivi.eng.br

 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx