WAs in init_clock_gating?

WAs in init_clock_gating?

[Mateo Lozano, Oscar]

Is there any reason why the WAs are applied in *_init_clock_gating? We are finding that some of them are lost during reset, and also the default context ends up with wrong values because the render context is restored & saved before we get to gen8_init_clok_gating (at least with Execlists, I´m not sure this happens with MI_SET_CONTEXT because the context won´t be saved until the next switch).

I believe this have been brought to the mailing list a couple of times, like:

	drm/i916: Init chv workarounds at render ring init
	My bsw is an unhappy camper if we delay the workaround init until init_clock_gating(). Move a bunch of it to the render ring init.

	FIXME: need to do this for all platforms since some of the registers
       	also get clobbered at reset. Just need to figure out which
      	 registers those actually are. This patch is based on a
       	slightly educated guess, but verifying on actual hw would
       	be a good idea. Also should maybe move the init_clock_gating
       	earlier too since we set up a bunch of clock gating stuff
       	there that might be important for a properly working GT.

	Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And also:

http://lists.freedesktop.org/archives/intel-gfx/2013-November/036482.html

-- Oscar

Re: WAs in init_clock_gating?

[Daniel Vetter]

On Tue, Jul 01, 2014 at 04:51:07PM +0000, Mateo Lozano, Oscar wrote:
> Is there any reason why the WAs are applied in *_init_clock_gating? We
> are finding that some of them are lost during reset, and also the
> default context ends up with wrong values because the render context is
> restored & saved before we get to gen8_init_clok_gating (at least with
> Execlists, I´m not sure this happens with MI_SET_CONTEXT because the
> context won´t be saved until the next switch).

It's a historical accident since _very_ old hw only needed a bit of
frobbing of the display clock gating bits.

> I believe this have been brought to the mailing list a couple of times, like:
> 
> 	drm/i916: Init chv workarounds at render ring init
> 	My bsw is an unhappy camper if we delay the workaround init until init_clock_gating(). Move a bunch of it to the render ring init.
> 
> 	FIXME: need to do this for all platforms since some of the registers
>        	also get clobbered at reset. Just need to figure out which
>       	 registers those actually are. This patch is based on a
>        	slightly educated guess, but verifying on actual hw would
>        	be a good idea. Also should maybe move the init_clock_gating
>        	earlier too since we set up a bunch of clock gating stuff
>        	there that might be important for a properly working GT.
> 
> 	Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> And also:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2013-November/036482.html

My concerns still apply. We need to move all work-arounds to the right
places (a bunch of them also might need to get moved into the runtime pm
code ...), and then we also need some test to make sure this all works.

Since maintaining the full list of all w/a bits is currently out of the
question (our code is too unstructured for this) I think we should have a
per-platform list of w/a relevant registers + maybe bitmasks with stuff to
ignore (e.g. the ring registers where the ring base addr might differ).

Then the test would grab snapshots before after all the following
operations and complain loud if anything changes:
- gpu hangs (on all rings as prep for per-engine reset)
- runtime pm (actually all the different power wells really, e.g. lpsp
  mode)
- system suspend/resum
- module reload

That should at least catch all the bugs we've seen thus far. If it later
on turns out that's not good enough we can go more fancy, but for now I
prefer something simpler ...

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

Re: WAs in init_clock_gating?

[Jesse Barnes]

On Mon, 7 Jul 2014 22:50:08 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jul 01, 2014 at 04:51:07PM +0000, Mateo Lozano, Oscar wrote:
> > Is there any reason why the WAs are applied in *_init_clock_gating? We
> > are finding that some of them are lost during reset, and also the
> > default context ends up with wrong values because the render context is
> > restored & saved before we get to gen8_init_clok_gating (at least with
> > Execlists, I´m not sure this happens with MI_SET_CONTEXT because the
> > context won´t be saved until the next switch).
> 
> It's a historical accident since _very_ old hw only needed a bit of
> frobbing of the display clock gating bits.
> 
> > I believe this have been brought to the mailing list a couple of times, like:
> > 
> > 	drm/i916: Init chv workarounds at render ring init
> > 	My bsw is an unhappy camper if we delay the workaround init until init_clock_gating(). Move a bunch of it to the render ring init.
> > 
> > 	FIXME: need to do this for all platforms since some of the registers
> >        	also get clobbered at reset. Just need to figure out which
> >       	 registers those actually are. This patch is based on a
> >        	slightly educated guess, but verifying on actual hw would
> >        	be a good idea. Also should maybe move the init_clock_gating
> >        	earlier too since we set up a bunch of clock gating stuff
> >        	there that might be important for a properly working GT.
> > 
> > 	Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > And also:
> > 
> > http://lists.freedesktop.org/archives/intel-gfx/2013-November/036482.html
> 
> My concerns still apply. We need to move all work-arounds to the right
> places (a bunch of them also might need to get moved into the runtime pm
> code ...), and then we also need some test to make sure this all works.
> 
> Since maintaining the full list of all w/a bits is currently out of the
> question (our code is too unstructured for this) I think we should have a
> per-platform list of w/a relevant registers + maybe bitmasks with stuff to
> ignore (e.g. the ring registers where the ring base addr might differ).

I don't think it's unreasonable to use a macro that checks a global
list for whether to apply a given WA.  They'll be scattered all over,
but at least it'll be easy to see:
  1) whether we implement a given workaround
and
  2) which platforms & steppings it applies to based on the table.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: WAs in init_clock_gating?

[Daniel Vetter]

On Mon, Jul 7, 2014 at 11:16 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> I don't think it's unreasonable to use a macro that checks a global
> list for whether to apply a given WA.  They'll be scattered all over,
> but at least it'll be easy to see:
>   1) whether we implement a given workaround
> and
>   2) which platforms & steppings it applies to based on the table.

Oh, I agree it's not unreasonable. But I'm kinda begging for the
simple solution since months (years?) and haven't gotten it, while
still getting a steady stream of bug reports and issues. So I've
readjusted my expectations ;-)

If someone delivers the real deal I'll certainly won't reject it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: WAs in init_clock_gating?

[Siluvery, Arun]

On 07/07/2014 22:24, Daniel Vetter wrote:
> On Mon, Jul 7, 2014 at 11:16 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> I don't think it's unreasonable to use a macro that checks a global
>> list for whether to apply a given WA.  They'll be scattered all over,
>> but at least it'll be easy to see:
>>    1) whether we implement a given workaround
>> and
>>    2) which platforms & steppings it applies to based on the table.
>
> Oh, I agree it's not unreasonable. But I'm kinda begging for the
> simple solution since months (years?) and haven't gotten it, while
> still getting a steady stream of bug reports and issues. So I've
> readjusted my expectations ;-)
>
> If someone delivers the real deal I'll certainly won't reject it.
> -Daniel
>

I am moving bdw workarounds from clock_gating fn to render ring init fn 
and testing this before and after gpu reset.
One of the workaround is to disable STC optimization, reg CACHE_MODE_1 
bit6 set to 1. I observed that some times after boot this gets reset to 
0 (default value) even after applying workarounds; other than 
workarounds no one else seems to write to this function.
Any ideas about this behaviour?

regards
Arun

Re: WAs in init_clock_gating?

[Daniel Vetter]

On Thu, Jul 24, 2014 at 11:43:11AM +0100, Siluvery, Arun wrote:
> On 07/07/2014 22:24, Daniel Vetter wrote:
> >On Mon, Jul 7, 2014 at 11:16 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>I don't think it's unreasonable to use a macro that checks a global
> >>list for whether to apply a given WA.  They'll be scattered all over,
> >>but at least it'll be easy to see:
> >>   1) whether we implement a given workaround
> >>and
> >>   2) which platforms & steppings it applies to based on the table.
> >
> >Oh, I agree it's not unreasonable. But I'm kinda begging for the
> >simple solution since months (years?) and haven't gotten it, while
> >still getting a steady stream of bug reports and issues. So I've
> >readjusted my expectations ;-)
> >
> >If someone delivers the real deal I'll certainly won't reject it.
> >-Daniel
> >
> 
> I am moving bdw workarounds from clock_gating fn to render ring init fn and
> testing this before and after gpu reset.

Testing = with an igt? Because I'll ask for this ;-)

> One of the workaround is to disable STC optimization, reg CACHE_MODE_1 bit6
> set to 1. I observed that some times after boot this gets reset to 0
> (default value) even after applying workarounds; other than workarounds no
> one else seems to write to this function.
> Any ideas about this behaviour?

gpu init tends to do this, since clock_gating is run before that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: WAs in init_clock_gating?

[Siluvery, Arun]

On 24/07/2014 13:33, Daniel Vetter wrote:
> On Thu, Jul 24, 2014 at 11:43:11AM +0100, Siluvery, Arun wrote:
>> On 07/07/2014 22:24, Daniel Vetter wrote:
>>> On Mon, Jul 7, 2014 at 11:16 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>> I don't think it's unreasonable to use a macro that checks a global
>>>> list for whether to apply a given WA.  They'll be scattered all over,
>>>> but at least it'll be easy to see:
>>>>    1) whether we implement a given workaround
>>>> and
>>>>    2) which platforms & steppings it applies to based on the table.
>>>
>>> Oh, I agree it's not unreasonable. But I'm kinda begging for the
>>> simple solution since months (years?) and haven't gotten it, while
>>> still getting a steady stream of bug reports and issues. So I've
>>> readjusted my expectations ;-)
>>>
>>> If someone delivers the real deal I'll certainly won't reject it.
>>> -Daniel
>>>
>>
>> I am moving bdw workarounds from clock_gating fn to render ring init fn and
>> testing this before and after gpu reset.
>
> Testing = with an igt? Because I'll ask for this ;-)

Yes, triggering gpu reset with igt, at the moment the test fails because 
of this register.
>
>> One of the workaround is to disable STC optimization, reg CACHE_MODE_1 bit6
>> set to 1. I observed that some times after boot this gets reset to 0
>> (default value) even after applying workarounds; other than workarounds no
>> one else seems to write to this function.
>> Any ideas about this behaviour?
>
> gpu init tends to do this, since clock_gating is run before that.

thanks, I will take a look.

regards
Arun

> -Daniel
>