Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-21 Thread Michel Thierry 



On 21/04/17 13:21, Chris Wilson wrote:

On Fri, Apr 21, 2017 at 01:10:37PM -0700, Daniele Ceraolo Spurio wrote:



On 21/04/17 13:07, Michel Thierry wrote:



On 20/04/17 10:29, Michel Thierry wrote:



On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:



On 20/04/17 04:33, Joonas Lahtinen wrote:

On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are
saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save
and
restore. This is not an issue in case of engine reset as driver
initializes
most of them following an engine reset, but in case of media reset
(aka
watchdog reset) which is completely internal to GuC (including
resubmission
of hung workload), it is necessary to provide this list, otherwise
GuC won't
be able to schedule further workloads after a reset. This is the
minimal
set of registers identified for things to work as expected but if we
see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC
know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use
REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers
(Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 





@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
intel_engine_cs *engine)

int ret;


/* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
-I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
+I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
+
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));


To make grepping easier, how about?

   I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
  ...);

Regards, Joonas



GUC_REG makes it sound like it is somehow related to GuC, while it
isn't, we just want GuC to restore its value. What about
GUC_REG_RESTORE?



Honestly, I dont care about names, pick one and I add it.
Just a reminder, we not only need the reg offset, we want to save the
value too.



I915_WRITE_GUC_RESTORE(reg, value) ?

That would be inline to the others we have, e.g. I915_WRITE_FW,
I915_WRITE_CTL, I915_WRITE_HEAD/TAIL.


I915_WRITE_FW is not the same class as I915_WRITE_CTL/HEAD/TAIL, and I
can say from experience the I915_*_CTL/HEAD/TAIL were a mistake (special
casing one particular access to the ring mmio, but we often deviate from
that pattern).

Looking at the above I see you are falling for the same trap as the ring
shorthand... So are you sure the convenience will not be lost later? And
in particular avoid using I915_WRITE_*() naming style as I would rather
that was earmarked for the different mmio accessors.


Ok, then can follow the pattern of the other workarounds & whitelist reg 
code?


E.g. WA_REG_GUC_RESTORE or WA_MMIO_REG_GUC_RESTORE (to make it clearer 
that these are not registers in the ctx image).




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

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-21 Thread Chris Wilson 
On Fri, Apr 21, 2017 at 01:10:37PM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 21/04/17 13:07, Michel Thierry wrote:
> >
> >
> >On 20/04/17 10:29, Michel Thierry wrote:
> >>
> >>
> >>On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:
> >>>
> >>>
> >>>On 20/04/17 04:33, Joonas Lahtinen wrote:
> On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:
> >From: Arun Siluvery 
> >
> >GuC expects a list of registers from the driver which are
> >saved/restored
> >during engine reset. The type of value to be saved is controlled by
> >flags. We provide a minimal set of registers that we want GuC to save
> >and
> >restore. This is not an issue in case of engine reset as driver
> >initializes
> >most of them following an engine reset, but in case of media reset
> >(aka
> >watchdog reset) which is completely internal to GuC (including
> >resubmission
> >of hung workload), it is necessary to provide this list, otherwise
> >GuC won't
> >be able to schedule further workloads after a reset. This is the
> >minimal
> >set of registers identified for things to work as expected but if we
> >see
> >any new issues, this register list can be expanded.
> >
> >In order to not loose any existing workarounds, we have to let GuC
> >know
> >the registers and its values. These will be reapplied after the reset.
> >Note that we can't just read the current value because most of these
> >registers are masked (so we have a workaround for a workaround for a
> >workaround).
> >
> >v2: REGSET_MASKED is too difficult for GuC, use
> >REGSET_SAVE_DEFAULT_VALUE
> >and current value from RING_MODE reg instead; no need to preserve
> >head/tail either, be extra paranoid and save whitelisted registers
> >(Daniele).
> >
> >v3: Workarounds added only once during _init_workarounds also have to
> >been restored, or we risk loosing them after internal GuC reset
> >(Daniele).
> >
> >Cc: Daniele Ceraolo Spurio 
> >Signed-off-by: Arun Siluvery 
> >Signed-off-by: Jeff McGee 
> >Signed-off-by: Michel Thierry 
> 
> 
> 
> >@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
> >intel_engine_cs *engine)
> >> int ret;
> >
> > /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
> >-I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
> >_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
> >+I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
> >+
> >_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
> 
> To make grepping easier, how about?
> 
> I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
>    ...);
> 
> Regards, Joonas
> 
> >>>
> >>>GUC_REG makes it sound like it is somehow related to GuC, while it
> >>>isn't, we just want GuC to restore its value. What about
> >>>GUC_REG_RESTORE?
> >>>
> >>
> >>Honestly, I dont care about names, pick one and I add it.
> >>Just a reminder, we not only need the reg offset, we want to save the
> >>value too.
> >>
> >
> >I915_WRITE_GUC_RESTORE(reg, value) ?
> >
> >That would be inline to the others we have, e.g. I915_WRITE_FW,
> >I915_WRITE_CTL, I915_WRITE_HEAD/TAIL.

I915_WRITE_FW is not the same class as I915_WRITE_CTL/HEAD/TAIL, and I
can say from experience the I915_*_CTL/HEAD/TAIL were a mistake (special
casing one particular access to the ring mmio, but we often deviate from
that pattern).

Looking at the above I see you are falling for the same trap as the ring
shorthand... So are you sure the convenience will not be lost later? And
in particular avoid using I915_WRITE_*() naming style as I would rather
that was earmarked for the different mmio accessors.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-21 Thread Daniele Ceraolo Spurio 



On 21/04/17 13:07, Michel Thierry wrote:



On 20/04/17 10:29, Michel Thierry wrote:



On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:



On 20/04/17 04:33, Joonas Lahtinen wrote:

On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are
saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save
and
restore. This is not an issue in case of engine reset as driver
initializes
most of them following an engine reset, but in case of media reset
(aka
watchdog reset) which is completely internal to GuC (including
resubmission
of hung workload), it is necessary to provide this list, otherwise
GuC won't
be able to schedule further workloads after a reset. This is the
minimal
set of registers identified for things to work as expected but if we
see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC
know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use
REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers
(Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 





@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
intel_engine_cs *engine)

 int ret;


 /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
-I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
+I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
+
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));


To make grepping easier, how about?

I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
   ...);

Regards, Joonas



GUC_REG makes it sound like it is somehow related to GuC, while it
isn't, we just want GuC to restore its value. What about
GUC_REG_RESTORE?



Honestly, I dont care about names, pick one and I add it.
Just a reminder, we not only need the reg offset, we want to save the
value too.



I915_WRITE_GUC_RESTORE(reg, value) ?

That would be inline to the others we have, e.g. I915_WRITE_FW,
I915_WRITE_CTL, I915_WRITE_HEAD/TAIL.

-Michel


LGTM.

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

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-21 Thread Michel Thierry 



On 20/04/17 10:29, Michel Thierry wrote:



On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:



On 20/04/17 04:33, Joonas Lahtinen wrote:

On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are
saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save
and
restore. This is not an issue in case of engine reset as driver
initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including
resubmission
of hung workload), it is necessary to provide this list, otherwise
GuC won't
be able to schedule further workloads after a reset. This is the
minimal
set of registers identified for things to work as expected but if we
see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use
REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers
(Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 





@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
intel_engine_cs *engine)

 int ret;


 /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
-I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
+I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
+
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));


To make grepping easier, how about?

I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
   ...);

Regards, Joonas



GUC_REG makes it sound like it is somehow related to GuC, while it
isn't, we just want GuC to restore its value. What about GUC_REG_RESTORE?



Honestly, I dont care about names, pick one and I add it.
Just a reminder, we not only need the reg offset, we want to save the
value too.



I915_WRITE_GUC_RESTORE(reg, value) ?

That would be inline to the others we have, e.g. I915_WRITE_FW, 
I915_WRITE_CTL, I915_WRITE_HEAD/TAIL.


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

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-20 Thread Michel Thierry 



On 20/04/17 09:39, Daniele Ceraolo Spurio wrote:



On 20/04/17 04:33, Joonas Lahtinen wrote:

On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save
and
restore. This is not an issue in case of engine reset as driver
initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including
resubmission
of hung workload), it is necessary to provide this list, otherwise
GuC won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use
REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers
(Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 





@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct
intel_engine_cs *engine)

 int ret;


 /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
-I915_WRITE(GEN9_CSFE_CHICKEN1_RCS,
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
+I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
+
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));


To make grepping easier, how about?

I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
   ...);

Regards, Joonas



GUC_REG makes it sound like it is somehow related to GuC, while it
isn't, we just want GuC to restore its value. What about GUC_REG_RESTORE?



Honestly, I dont care about names, pick one and I add it.
Just a reminder, we not only need the reg offset, we want to save the 
value too.


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

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-20 Thread Daniele Ceraolo Spurio 



On 20/04/17 04:33, Joonas Lahtinen wrote:

On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save and
restore. This is not an issue in case of engine reset as driver initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including resubmission
of hung workload), it is necessary to provide this list, otherwise GuC won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers (Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 





@@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct intel_engine_cs 
*engine)

int ret;


/* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
-   I915_WRITE(GEN9_CSFE_CHICKEN1_RCS, 
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
+   I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
+  
_MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));


To make grepping easier, how about?

I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
   ...);

Regards, Joonas



GUC_REG makes it sound like it is somehow related to GuC, while it 
isn't, we just want GuC to restore its value. What about GUC_REG_RESTORE?


Thanks,
Daniele
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-20 Thread Joonas Lahtinen 
On ke, 2017-04-19 at 11:35 -0700, Michel Thierry wrote:
> From: Arun Siluvery 
> 
> GuC expects a list of registers from the driver which are saved/restored
> during engine reset. The type of value to be saved is controlled by
> flags. We provide a minimal set of registers that we want GuC to save and
> restore. This is not an issue in case of engine reset as driver initializes
> most of them following an engine reset, but in case of media reset (aka
> watchdog reset) which is completely internal to GuC (including resubmission
> of hung workload), it is necessary to provide this list, otherwise GuC won't
> be able to schedule further workloads after a reset. This is the minimal
> set of registers identified for things to work as expected but if we see
> any new issues, this register list can be expanded.
> 
> In order to not loose any existing workarounds, we have to let GuC know
> the registers and its values. These will be reapplied after the reset.
> Note that we can't just read the current value because most of these
> registers are masked (so we have a workaround for a workaround for a
> workaround).
> 
> v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
> and current value from RING_MODE reg instead; no need to preserve
> head/tail either, be extra paranoid and save whitelisted registers (Daniele).
> 
> v3: Workarounds added only once during _init_workarounds also have to
> been restored, or we risk loosing them after internal GuC reset
> (Daniele).
> 
> Cc: Daniele Ceraolo Spurio 
> Signed-off-by: Arun Siluvery 
> Signed-off-by: Jeff McGee 
> Signed-off-by: Michel Thierry 



> @@ -732,15 +755,16 @@ static int gen9_init_workarounds(struct intel_engine_cs 
> *engine)
> >     int ret;
>  
>   /* WaConextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk */
> - I915_WRITE(GEN9_CSFE_CHICKEN1_RCS, 
> _MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));
> + I915_GUC_REG_WRITE(GEN9_CSFE_CHICKEN1_RCS,
> +    
> _MASKED_BIT_ENABLE(GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE));

To make grepping easier, how about?

I915_WRITE(GUC_REG(GEN9_CSFE_CHICKEN1_RCS),
   ...);

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

[Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-19 Thread Michel Thierry 
From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save and
restore. This is not an issue in case of engine reset as driver initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including resubmission
of hung workload), it is necessary to provide this list, otherwise GuC won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

In order to not loose any existing workarounds, we have to let GuC know
the registers and its values. These will be reapplied after the reset.
Note that we can't just read the current value because most of these
registers are masked (so we have a workaround for a workaround for a
workaround).

v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers (Daniele).

v3: Workarounds added only once during _init_workarounds also have to
been restored, or we risk loosing them after internal GuC reset
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_drv.h|  3 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 68 +-
 drivers/gpu/drm/i915/intel_engine_cs.c | 65 +++-
 3 files changed, 114 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fa3988c5033b..1ba1ac016973 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1913,7 +1913,10 @@ struct i915_wa_reg {
 
 struct i915_workarounds {
struct i915_wa_reg reg[I915_MAX_WA_REGS];
+   /* list of registers (and their values) that GuC will have to restore */
+   struct i915_wa_reg guc_reg[GUC_REGSET_MAX_REGISTERS];
u32 count;
+   u32 guc_count;
u32 hw_whitelist_count[I915_NUM_ENGINES];
 };
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1ea36a88d2fb..f4081da88df2 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1003,6 +1003,24 @@ static void guc_policies_init(struct guc_policies 
*policies)
policies->is_valid = 1;
 }
 
+/*
+ * In this macro it is highly unlikely to exceed max value but even if we did
+ * it is not an error so just throw a warning and continue. Only side effect
+ * in continuing further means some registers won't be added to save/restore
+ * list.
+ */
+#define GUC_ADD_MMIO_REG_ADS(node, reg_addr, _flags, defvalue) \
+   do {\
+   u32 __count = node->number_of_registers;\
+   if (WARN_ON(__count >= GUC_REGSET_MAX_REGISTERS))   \
+   continue;   \
+   node->registers[__count].offset = reg_addr.reg; \
+   node->registers[__count].flags = (_flags);  \
+   if (defvalue)   \
+   node->registers[__count].value = (defvalue);\
+   node->number_of_registers++;\
+   } while (0)
+
 static int guc_ads_create(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1016,6 +1034,7 @@ static int guc_ads_create(struct intel_guc *guc)
u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
} __packed *blob;
struct intel_engine_cs *engine;
+   struct i915_workarounds *workarounds = _priv->workarounds;
enum intel_engine_id id;
u32 base;
 
@@ -1035,6 +1054,47 @@ static int guc_ads_create(struct intel_guc *guc)
 
/* MMIO reg state */
for_each_engine(engine, dev_priv, id) {
+   u32 i;
+   struct guc_mmio_regset *eng_reg =
+   >reg_state.engine_reg[engine->guc_id];
+
+   /*
+* Provide a list of registers to be saved/restored during gpu
+* reset. This is mainly required for Media reset (aka watchdog
+* timeout) which is completely under the control of GuC
+* (resubmission of hung workload is handled inside GuC).
+*/
+   GUC_ADD_MMIO_REG_ADS(eng_reg,

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-18 Thread Michel Thierry 

On 18/04/17 17:26, Daniele Ceraolo Spurio wrote:



On 18/04/17 13:23, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save and
restore. This is not an issue in case of engine reset as driver
initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including
resubmission
of hung workload), it is necessary to provide this list, otherwise GuC
won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers
(Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 60
+-
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1ea36a88d2fb..d772718861df 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1003,6 +1003,24 @@ static void guc_policies_init(struct
guc_policies *policies)
 policies->is_valid = 1;
 }

+/*
+ * In this macro it is highly unlikely to exceed max value but even
if we did
+ * it is not an error so just throw a warning and continue. Only side
effect
+ * in continuing further means some registers won't be added to
save/restore
+ * list.
+ */
+#define GUC_ADD_MMIO_REG_ADS(node, reg_addr, _flags, defvalue)\
+do {\
+u32 __count = node->number_of_registers;\
+if (WARN_ON(__count >= GUC_REGSET_MAX_REGISTERS))\
+continue;\
+node->registers[__count].offset = reg_addr.reg;\
+node->registers[__count].flags = (_flags);\
+if (defvalue)\
+node->registers[__count].value = (defvalue);\
+node->number_of_registers++;\
+} while (0)
+
 static int guc_ads_create(struct intel_guc *guc)
 {
 struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1016,6 +1034,7 @@ static int guc_ads_create(struct intel_guc *guc)
 u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
 } __packed *blob;
 struct intel_engine_cs *engine;
+struct i915_workarounds *workarounds = _priv->workarounds;
 enum intel_engine_id id;
 u32 base;

@@ -1035,6 +1054,39 @@ static int guc_ads_create(struct intel_guc *guc)

 /* MMIO reg state */
 for_each_engine(engine, dev_priv, id) {
+u32 i;
+struct guc_mmio_regset *eng_reg =
+>reg_state.engine_reg[engine->guc_id];
+
+/*
+ * Provide a list of registers to be saved/restored during gpu
+ * reset. This is mainly required for Media reset (aka watchdog
+ * timeout) which is completely under the control of GuC
+ * (resubmission of hung workload is handled inside GuC).
+ */
+GUC_ADD_MMIO_REG_ADS(eng_reg, RING_HWS_PGA(engine->mmio_base),
+ GUC_REGSET_ENGINERESET |
+ GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+
+/*
+ * Workaround the guc issue with masked registers, note that
+ * at this point guc submission is still disabled and the mode
+ * register doesnt have the irq_steering bit set, which we
+ * need to fwd irqs to GuC.
+ */
+GUC_ADD_MMIO_REG_ADS(eng_reg, RING_MODE_GEN7(engine),
+ GUC_REGSET_ENGINERESET |
+ GUC_REGSET_SAVE_DEFAULT_VALUE,
+ I915_READ(RING_MODE_GEN7(engine)) |
+ GFX_INTERRUPT_STEERING | (0x<<16));
+
+GUC_ADD_MMIO_REG_ADS(eng_reg, RING_IMR(engine->mmio_base),
+ GUC_REGSET_ENGINERESET |
+ GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+


I might just be too paranoid, but I think that we also have to add the
registers that we use for WAs via mmio (i.e. not using an LRI in the
ringbuffer). I did a quick test for the registers in the
gen9_init_workarounds and skl_init_workarounds functions that we write
using I915_WRITE and it looks like some of them lose their value after
and RCS media reset:

 REG   WA BITSVAL PRE-MRVAL POST-MR
0x20D4(1<<2)0x00040x

Re: [Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-18 Thread Daniele Ceraolo Spurio 



On 18/04/17 13:23, Michel Thierry wrote:

From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save and
restore. This is not an issue in case of engine reset as driver initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including resubmission
of hung workload), it is necessary to provide this list, otherwise GuC won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers (Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 60 +-
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1ea36a88d2fb..d772718861df 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1003,6 +1003,24 @@ static void guc_policies_init(struct guc_policies 
*policies)
policies->is_valid = 1;
 }

+/*
+ * In this macro it is highly unlikely to exceed max value but even if we did
+ * it is not an error so just throw a warning and continue. Only side effect
+ * in continuing further means some registers won't be added to save/restore
+ * list.
+ */
+#define GUC_ADD_MMIO_REG_ADS(node, reg_addr, _flags, defvalue) \
+   do {\
+   u32 __count = node->number_of_registers; \
+   if (WARN_ON(__count >= GUC_REGSET_MAX_REGISTERS))\
+   continue;   \
+   node->registers[__count].offset = reg_addr.reg;  \
+   node->registers[__count].flags = (_flags);   \
+   if (defvalue)   \
+   node->registers[__count].value = (defvalue); \
+   node->number_of_registers++; \
+   } while (0)
+
 static int guc_ads_create(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1016,6 +1034,7 @@ static int guc_ads_create(struct intel_guc *guc)
u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
} __packed *blob;
struct intel_engine_cs *engine;
+   struct i915_workarounds *workarounds = _priv->workarounds;
enum intel_engine_id id;
u32 base;

@@ -1035,6 +1054,39 @@ static int guc_ads_create(struct intel_guc *guc)

/* MMIO reg state */
for_each_engine(engine, dev_priv, id) {
+   u32 i;
+   struct guc_mmio_regset *eng_reg =
+   >reg_state.engine_reg[engine->guc_id];
+
+   /*
+* Provide a list of registers to be saved/restored during gpu
+* reset. This is mainly required for Media reset (aka watchdog
+* timeout) which is completely under the control of GuC
+* (resubmission of hung workload is handled inside GuC).
+*/
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_HWS_PGA(engine->mmio_base),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+
+   /*
+* Workaround the guc issue with masked registers, note that
+* at this point guc submission is still disabled and the mode
+* register doesnt have the irq_steering bit set, which we
+* need to fwd irqs to GuC.
+*/
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_MODE_GEN7(engine),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_DEFAULT_VALUE,
+I915_READ(RING_MODE_GEN7(engine)) |
+GFX_INTERRUPT_STEERING | (0x<<16));
+
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_IMR(engine->mmio_base),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+


I might just be too paranoid, but I think that we also have to

[Intel-gfx] [PATCH v6 13/20] drm/i915/guc: Provide register list to be saved/restored during engine reset

2017-04-18 Thread Michel Thierry 
From: Arun Siluvery 

GuC expects a list of registers from the driver which are saved/restored
during engine reset. The type of value to be saved is controlled by
flags. We provide a minimal set of registers that we want GuC to save and
restore. This is not an issue in case of engine reset as driver initializes
most of them following an engine reset, but in case of media reset (aka
watchdog reset) which is completely internal to GuC (including resubmission
of hung workload), it is necessary to provide this list, otherwise GuC won't
be able to schedule further workloads after a reset. This is the minimal
set of registers identified for things to work as expected but if we see
any new issues, this register list can be expanded.

v2: REGSET_MASKED is too difficult for GuC, use REGSET_SAVE_DEFAULT_VALUE
and current value from RING_MODE reg instead; no need to preserve
head/tail either, be extra paranoid and save whitelisted registers (Daniele).

Cc: Daniele Ceraolo Spurio 
Signed-off-by: Arun Siluvery 
Signed-off-by: Jeff McGee 
Signed-off-by: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 60 +-
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1ea36a88d2fb..d772718861df 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1003,6 +1003,24 @@ static void guc_policies_init(struct guc_policies 
*policies)
policies->is_valid = 1;
 }
 
+/*
+ * In this macro it is highly unlikely to exceed max value but even if we did
+ * it is not an error so just throw a warning and continue. Only side effect
+ * in continuing further means some registers won't be added to save/restore
+ * list.
+ */
+#define GUC_ADD_MMIO_REG_ADS(node, reg_addr, _flags, defvalue) \
+   do {\
+   u32 __count = node->number_of_registers;\
+   if (WARN_ON(__count >= GUC_REGSET_MAX_REGISTERS))   \
+   continue;   \
+   node->registers[__count].offset = reg_addr.reg; \
+   node->registers[__count].flags = (_flags);  \
+   if (defvalue)   \
+   node->registers[__count].value = (defvalue);\
+   node->number_of_registers++;\
+   } while (0)
+
 static int guc_ads_create(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1016,6 +1034,7 @@ static int guc_ads_create(struct intel_guc *guc)
u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
} __packed *blob;
struct intel_engine_cs *engine;
+   struct i915_workarounds *workarounds = _priv->workarounds;
enum intel_engine_id id;
u32 base;
 
@@ -1035,6 +1054,39 @@ static int guc_ads_create(struct intel_guc *guc)
 
/* MMIO reg state */
for_each_engine(engine, dev_priv, id) {
+   u32 i;
+   struct guc_mmio_regset *eng_reg =
+   >reg_state.engine_reg[engine->guc_id];
+
+   /*
+* Provide a list of registers to be saved/restored during gpu
+* reset. This is mainly required for Media reset (aka watchdog
+* timeout) which is completely under the control of GuC
+* (resubmission of hung workload is handled inside GuC).
+*/
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_HWS_PGA(engine->mmio_base),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+
+   /*
+* Workaround the guc issue with masked registers, note that
+* at this point guc submission is still disabled and the mode
+* register doesnt have the irq_steering bit set, which we
+* need to fwd irqs to GuC.
+*/
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_MODE_GEN7(engine),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_DEFAULT_VALUE,
+I915_READ(RING_MODE_GEN7(engine)) |
+GFX_INTERRUPT_STEERING | (0x<<16));
+
+   GUC_ADD_MMIO_REG_ADS(eng_reg, RING_IMR(engine->mmio_base),
+GUC_REGSET_ENGINERESET |
+GUC_REGSET_SAVE_CURRENT_VALUE, 0);
+
+   DRM_DEBUG_DRIVER("%s register save/restore count: %u\n",
+