diff mbox

drm/i915/guc: fix mmio whitelist mmio_start offset and add reminder

Message ID 1483061567-7258-1-git-send-email-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniele Ceraolo Spurio Dec. 30, 2016, 1:32 a.m. UTC 
From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

The mmio_start offset for the whitelist is the first FORCE_TO_NONPRIV
register the GuC can use to restore the provided whitelist when an
engine reset via GuC (which we still don't support) is triggered.

We're currently adding the mmio_base of the engine to the absolute
address of the RCS version of the register, which results in the wrong
offset. Fix it by using the definition we already have instead of
re-defining it in the GuC FW header.

Also add a comment to avoid future issues with FORCE_TO_NONPRIV
registers, which are also used by the workaround framework.

Compile tested only as my SKL died early this week.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 9 +++++++--
 drivers/gpu/drm/i915/intel_guc_fwif.h      | 1 -
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Michał Winiarski Jan. 2, 2017, 12:18 p.m. UTC | #1 
On Thu, Dec 29, 2016 at 05:32:47PM -0800, daniele.ceraolospurio@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> The mmio_start offset for the whitelist is the first FORCE_TO_NONPRIV
> register the GuC can use to restore the provided whitelist when an
> engine reset via GuC (which we still don't support) is triggered.
> 
> We're currently adding the mmio_base of the engine to the absolute
> address of the RCS version of the register, which results in the wrong
> offset. Fix it by using the definition we already have instead of
> re-defining it in the GuC FW header.
> 
> Also add a comment to avoid future issues with FORCE_TO_NONPRIV
> registers, which are also used by the workaround framework.
> 
> Compile tested only as my SKL died early this week.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 9 +++++++--
>  drivers/gpu/drm/i915/intel_guc_fwif.h      | 1 -
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 30e012b..7a1c6a0 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1336,9 +1336,14 @@ static void guc_addon_create(struct intel_guc *guc)
>  
>  	for_each_engine(engine, dev_priv, id) {
>  		reg_state->mmio_white_list[engine->guc_id].mmio_start =
> -			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> +			i915_mmio_reg_offset(RING_FORCE_TO_NONPRIV(engine->mmio_base, 0));
>  
> -		/* Nothing to be saved or restored for now. */
> +		/*
> +		 * Nothing to be saved or restored for now.
> +		 * XXX: when adding registers to the whitelist, make sure to not
> +		 * conflict with what the workaround framework is doing with
> +		 * the FORCE_TO_NONPRIV registers.
> +		 */

Is this what will end up happening with GuC engine reset?
Shouldn't we just use the workaround framework to populate GuC whitelist instead
of maintaining separate one? (in which case, the comment is slightly misleading)

-Michał

>  		reg_state->mmio_white_list[engine->guc_id].count = 0;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 3202b32..2841d59 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -371,7 +371,6 @@ struct guc_policies {
>  #define GUC_REGSET_SAVE_CURRENT_VALUE	0x10
>  
>  #define GUC_REGSET_MAX_REGISTERS	25
> -#define GUC_MMIO_WHITE_LIST_START	0x24d0
>  #define GUC_MMIO_WHITE_LIST_MAX		12
>  #define GUC_S3_SAVE_SPACE_PAGES		10
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniele Ceraolo Spurio Jan. 2, 2017, 9:48 p.m. UTC | #2 
On 1/2/2017 4:18 AM, Michał Winiarski wrote:
> On Thu, Dec 29, 2016 at 05:32:47PM -0800, daniele.ceraolospurio@intel.com wrote:
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> The mmio_start offset for the whitelist is the first FORCE_TO_NONPRIV
>> register the GuC can use to restore the provided whitelist when an
>> engine reset via GuC (which we still don't support) is triggered.
>>
>> We're currently adding the mmio_base of the engine to the absolute
>> address of the RCS version of the register, which results in the wrong
>> offset. Fix it by using the definition we already have instead of
>> re-defining it in the GuC FW header.
>>
>> Also add a comment to avoid future issues with FORCE_TO_NONPRIV
>> registers, which are also used by the workaround framework.
>>
>> Compile tested only as my SKL died early this week.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 9 +++++++--
>>   drivers/gpu/drm/i915/intel_guc_fwif.h      | 1 -
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 30e012b..7a1c6a0 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -1336,9 +1336,14 @@ static void guc_addon_create(struct intel_guc *guc)
>>   
>>   	for_each_engine(engine, dev_priv, id) {
>>   		reg_state->mmio_white_list[engine->guc_id].mmio_start =
>> -			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>> +			i915_mmio_reg_offset(RING_FORCE_TO_NONPRIV(engine->mmio_base, 0));
>>   
>> -		/* Nothing to be saved or restored for now. */
>> +		/*
>> +		 * Nothing to be saved or restored for now.
>> +		 * XXX: when adding registers to the whitelist, make sure to not
>> +		 * conflict with what the workaround framework is doing with
>> +		 * the FORCE_TO_NONPRIV registers.
>> +		 */
> Is this what will end up happening with GuC engine reset?
> Shouldn't we just use the workaround framework to populate GuC whitelist instead
> of maintaining separate one? (in which case, the comment is slightly misleading)
>
> -Michał

I'm not sure which way we'll go with GuC engine as that will be an 
implementation detail, because AFAIK writing the registers manually using the 
workaround framework as we do now (with count=0 here) and moving the handling of 
post-reset restore of registers to GuC should functionally be identical. I agree 
with you that if we decide to use the GuC to do the restore we should use the 
workaround framework to populate this list, but since I'm not sure which way 
we're going I wanted to keep the comment generic. Does something like this sound 
better to you

/* Note: if the GuC whitelist management is enabled the values
  * should be filled using the workaround framework to avoid
  * inconsistencies with the handling of FORCE_TO_NONPRIV
  * registers
  */

I'm open to suggestions if you still feel that this is not clear enough ;).

Thanks,
Daniele

>
>>   		reg_state->mmio_white_list[engine->guc_id].count = 0;
>>   	}
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 3202b32..2841d59 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -371,7 +371,6 @@ struct guc_policies {
>>   #define GUC_REGSET_SAVE_CURRENT_VALUE	0x10
>>   
>>   #define GUC_REGSET_MAX_REGISTERS	25
>> -#define GUC_MMIO_WHITE_LIST_START	0x24d0
>>   #define GUC_MMIO_WHITE_LIST_MAX		12
>>   #define GUC_S3_SAVE_SPACE_PAGES		10
>>   
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 30e012b..7a1c6a0 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1336,9 +1336,14 @@  static void guc_addon_create(struct intel_guc *guc)
 
 	for_each_engine(engine, dev_priv, id) {
 		reg_state->mmio_white_list[engine->guc_id].mmio_start =
-			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
+			i915_mmio_reg_offset(RING_FORCE_TO_NONPRIV(engine->mmio_base, 0));
 
-		/* Nothing to be saved or restored for now. */
+		/*
+		 * Nothing to be saved or restored for now.
+		 * XXX: when adding registers to the whitelist, make sure to not
+		 * conflict with what the workaround framework is doing with
+		 * the FORCE_TO_NONPRIV registers.
+		 */
 		reg_state->mmio_white_list[engine->guc_id].count = 0;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 3202b32..2841d59 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -371,7 +371,6 @@  struct guc_policies {
 #define GUC_REGSET_SAVE_CURRENT_VALUE	0x10
 
 #define GUC_REGSET_MAX_REGISTERS	25
-#define GUC_MMIO_WHITE_LIST_START	0x24d0
 #define GUC_MMIO_WHITE_LIST_MAX		12
 #define GUC_S3_SAVE_SPACE_PAGES		10