[PATCH] drm/i915: don't double-restore ARB mode bits for kms

[PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Daniel Vetter]

Our init_clock_gating functions and related code should already take
care of this. And if they don't we'd better know.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index e6c90d1382b3..0b72fcd28d4d 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -348,12 +348,14 @@ int i915_save_state(struct drm_device *dev)
 
 	intel_disable_gt_powersave(dev);
 
-	/* Cache mode state */
-	if (INTEL_INFO(dev)->gen < 7)
-		dev_priv->regfile.saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Cache mode state */
+		if (INTEL_INFO(dev)->gen < 7)
+			dev_priv->regfile.saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
 
-	/* Memory Arbitration state */
-	dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
+		/* Memory Arbitration state */
+		dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
+	}
 
 	/* Scratch space */
 	for (i = 0; i < 16; i++) {
@@ -394,13 +396,15 @@ int i915_restore_state(struct drm_device *dev)
 		}
 	}
 
-	/* Cache mode state */
-	if (INTEL_INFO(dev)->gen < 7)
-		I915_WRITE(CACHE_MODE_0, dev_priv->regfile.saveCACHE_MODE_0 |
-			   0xffff0000);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Cache mode state */
+		if (INTEL_INFO(dev)->gen < 7)
+			I915_WRITE(CACHE_MODE_0, dev_priv->regfile.saveCACHE_MODE_0 |
+				   0xffff0000);
 
-	/* Memory arbitration state */
-	I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000);
+		/* Memory arbitration state */
+		I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000);
+	}
 
 	for (i = 0; i < 16; i++) {
 		I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]);
-- 
1.8.5.2

Re: [PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Paulo Zanoni]

2014/1/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Our init_clock_gating functions and related code should already take
> care of this. And if they don't we'd better know.

For both registers, I see functions applying specific workarounds, but
they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
don't see anybody explicitly fully initializing the registers
anywhere. So we go with whatever the BIOS gives us at boot + our
changes to a few specific registers. At resume, we don't know so far
what we'll get, so I fear this patch may in fact cause a regression.


>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index e6c90d1382b3..0b72fcd28d4d 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -348,12 +348,14 @@ int i915_save_state(struct drm_device *dev)
>
>         intel_disable_gt_powersave(dev);
>
> -       /* Cache mode state */
> -       if (INTEL_INFO(dev)->gen < 7)
> -               dev_priv->regfile.saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Cache mode state */
> +               if (INTEL_INFO(dev)->gen < 7)
> +                       dev_priv->regfile.saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
>
> -       /* Memory Arbitration state */
> -       dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
> +               /* Memory Arbitration state */
> +               dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
> +       }
>
>         /* Scratch space */
>         for (i = 0; i < 16; i++) {
> @@ -394,13 +396,15 @@ int i915_restore_state(struct drm_device *dev)
>                 }
>         }
>
> -       /* Cache mode state */
> -       if (INTEL_INFO(dev)->gen < 7)
> -               I915_WRITE(CACHE_MODE_0, dev_priv->regfile.saveCACHE_MODE_0 |
> -                          0xffff0000);
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Cache mode state */
> +               if (INTEL_INFO(dev)->gen < 7)
> +                       I915_WRITE(CACHE_MODE_0, dev_priv->regfile.saveCACHE_MODE_0 |
> +                                  0xffff0000);
>
> -       /* Memory arbitration state */
> -       I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000);
> +               /* Memory arbitration state */
> +               I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000);
> +       }
>
>         for (i = 0; i < 16; i++) {
>                 I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]);
> --
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Daniel Vetter]

On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014/1/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> Our init_clock_gating functions and related code should already take
>> care of this. And if they don't we'd better know.
>
> For both registers, I see functions applying specific workarounds, but
> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
> don't see anybody explicitly fully initializing the registers
> anywhere. So we go with whatever the BIOS gives us at boot + our
> changes to a few specific registers. At resume, we don't know so far
> what we'll get, so I fear this patch may in fact cause a regression.

That's part of the risk of it, but fixing those is a simple matter of
comparing register dumps. These two bits here are one of the very few
holdouts we have that depend upon the register save/restore code, and
I want to remove them. Since thus far this was a really good way to
hide random bugs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Jani Nikula]

On Thu, 16 Jan 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2014/1/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>> Our init_clock_gating functions and related code should already take
>>> care of this. And if they don't we'd better know.
>>
>> For both registers, I see functions applying specific workarounds, but
>> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
>> don't see anybody explicitly fully initializing the registers
>> anywhere. So we go with whatever the BIOS gives us at boot + our
>> changes to a few specific registers. At resume, we don't know so far
>> what we'll get, so I fear this patch may in fact cause a regression.
>
> That's part of the risk of it, but fixing those is a simple matter of
> comparing register dumps. These two bits here are one of the very few
> holdouts we have that depend upon the register save/restore code, and
> I want to remove them. Since thus far this was a really good way to
> hide random bugs.

I'm in favour of the patch in general, but I spotted at least

	/* On GEN3 we really need to make sure the ARB C3 LP bit is set */
	if (IS_GEN3(dev)) {
		I915_WRITE(MI_ARB_STATE,
			   _MASKED_BIT_ENABLE(MI_ARB_C3_LP_WRITE_ENABLE));
	}

in i915_gem_load() that won't be set on a suspend/resume cycle. Is that
going to be problem?

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Daniel Vetter]

On Wed, Feb 5, 2014 at 2:01 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 16 Jan 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>> 2014/1/16 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>>> Our init_clock_gating functions and related code should already take
>>>> care of this. And if they don't we'd better know.
>>>
>>> For both registers, I see functions applying specific workarounds, but
>>> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
>>> don't see anybody explicitly fully initializing the registers
>>> anywhere. So we go with whatever the BIOS gives us at boot + our
>>> changes to a few specific registers. At resume, we don't know so far
>>> what we'll get, so I fear this patch may in fact cause a regression.
>>
>> That's part of the risk of it, but fixing those is a simple matter of
>> comparing register dumps. These two bits here are one of the very few
>> holdouts we have that depend upon the register save/restore code, and
>> I want to remove them. Since thus far this was a really good way to
>> hide random bugs.
>
> I'm in favour of the patch in general, but I spotted at least
>
>         /* On GEN3 we really need to make sure the ARB C3 LP bit is set */
>         if (IS_GEN3(dev)) {
>                 I915_WRITE(MI_ARB_STATE,
>                            _MASKED_BIT_ENABLE(MI_ARB_C3_LP_WRITE_ENABLE));
>         }
>
> in i915_gem_load() that won't be set on a suspend/resume cycle. Is that
> going to be problem?

Yes, good catch. Somehow I've missed this when grepping.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch