[PATCH] drm/i915: setup workarounds on reset

[PATCH] drm/i915: setup workarounds on reset

[Mika Kuoppala]

Large parts of hw initialization is behind per gen
clock gating functions. Including workarounds.

Call intel_modeset_init_hw after reset so that we
set these up correctly.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c2e00ed..2908f7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -798,6 +798,8 @@ int i915_reset(struct drm_device *dev)
 		drm_irq_uninstall(dev);
 		drm_irq_install(dev);
 		intel_hpd_init(dev);
+
+		intel_modeset_init_hw(dev);
 	} else {
 		mutex_unlock(&dev->struct_mutex);
 	}
-- 
1.7.9.5

Re: [PATCH] drm/i915: setup workarounds on reset

[Daniel Vetter]

On Mon, Nov 18, 2013 at 04:34:44PM +0200, Mika Kuoppala wrote:
> Large parts of hw initialization is behind per gen
> clock gating functions. Including workarounds.
> 
> Call intel_modeset_init_hw after reset so that we
> set these up correctly.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c2e00ed..2908f7f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -798,6 +798,8 @@ int i915_reset(struct drm_device *dev)
>  		drm_irq_uninstall(dev);
>  		drm_irq_install(dev);
>  		intel_hpd_init(dev);
> +
> +		intel_modeset_init_hw(dev);

Currently the idea is that w/as which get nuked by the gt reset should be
put into the respective ring_init function in intel_ringbuffer.c. This
disdinction is important since init_clock_gating gets called fairly early
in the resume/load sequence before most of the gem stuff is set up. And
the w/a in the ring_init functions are carefully ordered wrt the ring (re)
enabling.

So which bit/register is the culprit here?
-Daniel

>  	} else {
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: setup workarounds on reset

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

Hi Daniel,

> On Mon, Nov 18, 2013 at 04:34:44PM +0200, Mika Kuoppala wrote:
>> Large parts of hw initialization is behind per gen
>> clock gating functions. Including workarounds.
>> 
>> Call intel_modeset_init_hw after reset so that we
>> set these up correctly.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index c2e00ed..2908f7f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -798,6 +798,8 @@ int i915_reset(struct drm_device *dev)
>>  		drm_irq_uninstall(dev);
>>  		drm_irq_install(dev);
>>  		intel_hpd_init(dev);
>> +
>> +		intel_modeset_init_hw(dev);
>
> Currently the idea is that w/as which get nuked by the gt reset should be
> put into the respective ring_init function in intel_ringbuffer.c. This
> disdinction is important since init_clock_gating gets called fairly early
> in the resume/load sequence before most of the gem stuff is set up. And
> the w/a in the ring_init functions are carefully ordered wrt the ring (re)
> enabling.
>
> So which bit/register is the culprit here?

Here is output from the test on ivb, after drv_hangman:

FAIL     WaDisableEarlyCull:ivb
OK       WaDisableBackToBackFlipFix:ivb
FAIL     WaDisablePSDDualDispatchEnable:ivb
FAIL     WaDisableRHWOptimizationForRenderHang:ivb
FAIL     WaApplyL3ControlAndL3ChickenMode:ivb
OK       WaForceL3Serialization:ivb
OK       WaDisableRCZUnitClockGating:ivb
OK       WaCatErrorRejectionIssue:ivb
FAIL     WaVSRefCountFullforceMissDisable:ivb
FAIL     WaDisable4x2SubspanOptimization:ivb
10 workarounds tested, 4 passed, 6 failed
Test assertion failure function main, file drv_workarounds.c:119:
Failed assertion: check_workarounds(&ivb_workarounds[0], "ivb") == 0

Test can be found in here:
https://github.com/mkuoppal/intel-gpu-tools/tree/was

-Mika

Re: [PATCH] drm/i915: setup workarounds on reset

[Daniel Vetter]

On Wed, Nov 20, 2013 at 12:47 PM, Mika Kuoppala
<mika.kuoppala@linux.intel.com> wrote:
> FAIL     WaDisableEarlyCull:ivb
> OK       WaDisableBackToBackFlipFix:ivb
> FAIL     WaDisablePSDDualDispatchEnable:ivb
> FAIL     WaDisableRHWOptimizationForRenderHang:ivb
> FAIL     WaApplyL3ControlAndL3ChickenMode:ivb
> OK       WaForceL3Serialization:ivb
> OK       WaDisableRCZUnitClockGating:ivb
> OK       WaCatErrorRejectionIssue:ivb
> FAIL     WaVSRefCountFullforceMissDisable:ivb
> FAIL     WaDisable4x2SubspanOptimization:ivb


Looks like just a bunch of render related registers, so could make
sense to move them. I guess the bigger question is what happens if we
use per-ring reset. Do we loose all the same register settings as for
a full reset or is there some fancy split? Can you please check this
out with your per-ring reset patches if possible?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: setup workarounds on reset

[Paulo Zanoni]

Hi

2013/11/20 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
> Daniel Vetter <daniel@ffwll.ch> writes:
>
> Hi Daniel,
>
>> On Mon, Nov 18, 2013 at 04:34:44PM +0200, Mika Kuoppala wrote:
>>> Large parts of hw initialization is behind per gen
>>> clock gating functions. Including workarounds.
>>>
>>> Call intel_modeset_init_hw after reset so that we
>>> set these up correctly.
>>>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index c2e00ed..2908f7f 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -798,6 +798,8 @@ int i915_reset(struct drm_device *dev)
>>>              drm_irq_uninstall(dev);
>>>              drm_irq_install(dev);
>>>              intel_hpd_init(dev);
>>> +
>>> +            intel_modeset_init_hw(dev);
>>
>> Currently the idea is that w/as which get nuked by the gt reset should be
>> put into the respective ring_init function in intel_ringbuffer.c. This
>> disdinction is important since init_clock_gating gets called fairly early
>> in the resume/load sequence before most of the gem stuff is set up. And
>> the w/a in the ring_init functions are carefully ordered wrt the ring (re)
>> enabling.
>>
>> So which bit/register is the culprit here?
>
> Here is output from the test on ivb, after drv_hangman:
>
> FAIL     WaDisableEarlyCull:ivb
> OK       WaDisableBackToBackFlipFix:ivb
> FAIL     WaDisablePSDDualDispatchEnable:ivb
> FAIL     WaDisableRHWOptimizationForRenderHang:ivb
> FAIL     WaApplyL3ControlAndL3ChickenMode:ivb
> OK       WaForceL3Serialization:ivb
> OK       WaDisableRCZUnitClockGating:ivb
> OK       WaCatErrorRejectionIssue:ivb
> FAIL     WaVSRefCountFullforceMissDisable:ivb
> FAIL     WaDisable4x2SubspanOptimization:ivb
> 10 workarounds tested, 4 passed, 6 failed
> Test assertion failure function main, file drv_workarounds.c:119:
> Failed assertion: check_workarounds(&ivb_workarounds[0], "ivb") == 0
>
> Test can be found in here:
> https://github.com/mkuoppal/intel-gpu-tools/tree/was

I really like the idea of having a way to test bits that should always
be set to specific values, but I think that if we do this in IGT we'll
always be out of sync with the Kernel. We change these WAs way too
often, and I'm sure we'll always forget to keep IGT in sync.

One of the things I have discussed with Daniel was to create some sort
of data structure inside the Kernel where we explain which bits of
which registers should always have specific values, and which
function/stage should be responsible for setting the value. Then, at
init_clock_gating or ring_init or other places, our code would parse
this data structure and appropriately fix whatever is needed. As a
bonus, we would also be able to write functions that check if the
values in HW match the values on our data structure. The big benefit
of this model is that with a single change in the data structure we'd
guarantee that both the function that applies the WA and the function
that checks the WA are in sync. And if we need to expose the WA
checker to IGT, we could maybe write a debugfs file that triggers the
check.

That said, this was just an initial idea we discussed and I did not
develop more concrete plans regarding what exactly the data structure
should look like. I also have no plans to implement this soon. So if
you think it's a good idea, feel free to take the idea, improve it and
implement if you want.

Thanks,
Paulo

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



-- 
Paulo Zanoni