[PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Oscar Mateo]

RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
global privileged MMIO registers that happen to be powercontext saved and restored
(meaning only they can survive RC6). Therefore, there is absolutely no need to save
them so that they can be restored everytime we create a new logical context.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a28e2a8..a75f5e8 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
 	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
 		return -EINVAL;
 
-	WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
-		 i915_mmio_reg_offset(reg));
+	I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
+		   i915_mmio_reg_offset(reg));
 	wa->hw_whitelist_count[engine->id]++;
 
 	return 0;
-- 
1.9.1

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

✓ Fi.CI.BAT: success for drm/i915: Transform whitelisting WAs into a simple reg write

[Patchwork]

== Series Details ==

Series: drm/i915: Transform whitelisting WAs into a simple reg write
URL   : https://patchwork.freedesktop.org/series/31099/
State : success

== Summary ==

Series 31099v1 drm/i915: Transform whitelisting WAs into a simple reg write
https://patchwork.freedesktop.org/api/1.0/series/31099/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> PASS       (fi-glk-1) fdo#102777

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:475s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:416s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:508s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:495s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:509s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:495s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:487s
fi-cfl-s         total:289  pass:256  dwarn:1   dfail:0   fail:0   skip:32  time:547s
fi-cnl-y         total:289  pass:260  dwarn:1   dfail:0   fail:1   skip:27  time:633s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:419s
fi-glk-1         total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:565s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:429s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:404s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:467s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:577s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:545s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:761s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:488s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:477s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:564s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:415s

0369ecdbb55495dd4671ad33d375f37d0707b629 drm-tip: 2017y-09m-28d-20h-01m-55s UTC integration manifest
34540f12b1bc drm/i915: Transform whitelisting WAs into a simple reg write

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5854/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Michel Thierry]

On 28/09/17 15:40, Oscar Mateo wrote:
> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> global privileged MMIO registers that happen to be powercontext saved and restored
> (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> them so that they can be restored everytime we create a new logical context.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a28e2a8..a75f5e8 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
>          if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>                  return -EINVAL;
> 
> -       WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> -                i915_mmio_reg_offset(reg));
> +       I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> +                  i915_mmio_reg_offset(reg));
>          wa->hw_whitelist_count[engine->id]++;
> 
>          return 0;
> --
> 1.9.1
> 

I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in 
the "Register State Context", so

Acked-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Transform whitelisting WAs into a simple reg write

[Patchwork]

== Series Details ==

Series: drm/i915: Transform whitelisting WAs into a simple reg write
URL   : https://patchwork.freedesktop.org/series/31099/
State : success

== Summary ==

Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2429 pass:1330 dwarn:5   dfail:0   fail:11  skip:1083 time:9966s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5854/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Chris Wilson]

Quoting Oscar Mateo (2017-09-28 23:40:39)
> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> global privileged MMIO registers that happen to be powercontext saved and restored
> (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> them so that they can be restored everytime we create a new logical context.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk> #bxt

Now we just need Mika for the full set of tags! :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Joonas Lahtinen]

On Thu, 2017-09-28 at 16:47 -0700, Michel Thierry wrote:
> On 28/09/17 15:40, Oscar Mateo wrote:
> > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> > global privileged MMIO registers that happen to be powercontext saved and restored
> > (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> > them so that they can be restored everytime we create a new logical context.
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a28e2a8..a75f5e8 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
> >          if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> >                  return -EINVAL;
> > 
> > -       WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> > -                i915_mmio_reg_offset(reg));
> > +       I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> > +                  i915_mmio_reg_offset(reg));
> >          wa->hw_whitelist_count[engine->id]++;
> > 
> >          return 0;
> > --
> > 1.9.1
> > 
> 
> I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in 
> the "Register State Context", so
> 
> Acked-by: Michel Thierry <michel.thierry@intel.com>

You reviewed the spec and the code, so should be Reviewed-by :)

Then this could be merged, too.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Michel Thierry]

On 9/29/2017 3:25 AM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 16:47 -0700, Michel Thierry wrote:
>> On 28/09/17 15:40, Oscar Mateo wrote:
>>> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
>>> global privileged MMIO registers that happen to be powercontext saved and restored
>>> (meaning only they can survive RC6). Therefore, there is absolutely no need to save
>>> them so that they can be restored everytime we create a new logical context.
>>>
>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index a28e2a8..a75f5e8 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
>>>           if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>>>                   return -EINVAL;
>>>
>>> -       WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
>>> -                i915_mmio_reg_offset(reg));
>>> +       I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
>>> +                  i915_mmio_reg_offset(reg));
>>>           wa->hw_whitelist_count[engine->id]++;
>>>
>>>           return 0;
>>> --
>>> 1.9.1
>>>
>>
>> I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in
>> the "Register State Context", so
>>
>> Acked-by: Michel Thierry <michel.thierry@intel.com>
> 
> You reviewed the spec and the code, so should be Reviewed-by :)
> 
> Then this could be merged, too.
> 
> Regards, Joonas
> 

I thought Chris would be adding his. Swap my acked-by to,

Reviewed-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Joonas Lahtinen]

On Fri, 2017-09-29 at 09:38 +0100, Chris Wilson wrote:
> Quoting Oscar Mateo (2017-09-28 23:40:39)
> > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> > global privileged MMIO registers that happen to be powercontext saved and restored
> > (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> > them so that they can be restored everytime we create a new logical context.
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> #bxt
> 
> Now we just need Mika for the full set of tags! :)

Mika, ping?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Mika Kuoppala]

Oscar Mateo <oscar.mateo@intel.com> writes:

> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> global privileged MMIO registers that happen to be powercontext saved and restored
> (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> them so that they can be restored everytime we create a new logical context.
>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index a28e2a8..a75f5e8 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
>  	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>  		return -EINVAL;
>  
> -	WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> -		 i915_mmio_reg_offset(reg));
> +	I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> +		   i915_mmio_reg_offset(reg));

#define WA_WRITE should also been removed as it is clearly
dangerous. Chris pointed out that anything with nonmasked access
is not part of context image, and this seems to hold true in
atleast with current cases.

But removing of define can be a followup.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  	wa->hw_whitelist_count[engine->id]++;
>  
>  	return 0;
> -- 
> 1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-04 13:39:13)
> Oscar Mateo <oscar.mateo@intel.com> writes:
> 
> > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
> > global privileged MMIO registers that happen to be powercontext saved and restored
> > (meaning only they can survive RC6). Therefore, there is absolutely no need to save
> > them so that they can be restored everytime we create a new logical context.
> >
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index a28e2a8..a75f5e8 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
> >       if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> >               return -EINVAL;
> >  
> > -     WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> > -              i915_mmio_reg_offset(reg));
> > +     I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
> > +                i915_mmio_reg_offset(reg));
> 
> #define WA_WRITE should also been removed as it is clearly
> dangerous. Chris pointed out that anything with nonmasked access
> is not part of context image, and this seems to hold true in
> atleast with current cases.
> 
> But removing of define can be a followup.

I've picked up this patch and I'll squash in the -WA_WRITE into the
removal of WA_SET_BIT and push all 3 patches at once (when the shards
report back).

Thanks for the patch, review, testing, debate and keep having fun.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Transform whitelisting WAs into a simple reg write

[Oscar Mateo]

On 10/04/2017 06:17 AM, Chris Wilson wrote:
> Quoting Mika Kuoppala (2017-10-04 13:39:13)
>> Oscar Mateo <oscar.mateo@intel.com> writes:
>>
>>> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply
>>> global privileged MMIO registers that happen to be powercontext saved and restored
>>> (meaning only they can survive RC6). Therefore, there is absolutely no need to save
>>> them so that they can be restored everytime we create a new logical context.
>>>
>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index a28e2a8..a75f5e8 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine,
>>>        if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>>>                return -EINVAL;
>>>   
>>> -     WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
>>> -              i915_mmio_reg_offset(reg));
>>> +     I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index),
>>> +                i915_mmio_reg_offset(reg));
>> #define WA_WRITE should also been removed as it is clearly
>> dangerous. Chris pointed out that anything with nonmasked access
>> is not part of context image, and this seems to hold true in
>> atleast with current cases.
>>
>> But removing of define can be a followup.
> I've picked up this patch and I'll squash in the -WA_WRITE into the
> removal of WA_SET_BIT and push all 3 patches at once (when the shards
> report back).
>
> Thanks for the patch, review, testing, debate and keep having fun.
> -Chris

We can also remove RING_MAX_NONPRIV_SLOTS from here:

#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)

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