diff mbox series

[1/4] drm/i915: Support flags in whitlist WAs

Message ID 20190618010108.27499-2-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Update whitelist support for new hardware | expand

Commit Message

John Harrison June 18, 2019, 1:01 a.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

Newer hardware adds flags to the whitelist work-around register. These
allow per access direction privileges and ranges.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Tvrtko Ursulin June 18, 2019, 6:27 a.m. UTC | #1 
On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;
>   
> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */

Repeating the question from previous round - isn't the RW legacy and the 
two new flags below are CFL+ & Gen11+?

> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 

If I am right that the comment should be moved a line down, then with 
that, or if I am not right like it is:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
Tvrtko Ursulin June 18, 2019, 6:35 a.m. UTC | #2 
On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;

Actually how about we add somewhere around here:

GEM_BUG_ON(hweight32(flags & (..RD | .. WR)) > 1);

To ensure correct usage of the flags?

Regards,

Tvrtko

> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>
John Harrison June 18, 2019, 1:48 p.m. UTC | #3 
On 6/17/2019 23:35, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Newer hardware adds flags to the whitelist work-around register. These
>> allow per access direction privileges and ranges.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 165b0a45e009..ae82340fff45 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct 
>> drm_i915_private *i915,
>>   }
>>     static void
>> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>>   {
>>       struct i915_wa wa = {
>>           .reg = reg
>> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, 
>> i915_reg_t reg)
>>       if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>>           return;
>
> Actually how about we add somewhere around here:
>
> GEM_BUG_ON(hweight32(flags & (..RD | .. WR)) > 1);
>
> To ensure correct usage of the flags?
>

It should probably be more like BUG_ON((flags & ACCESS_MASK) > 
ACCESS_MAX). It is intended to be an access enum with three valid values 
rather than a pair of flags. But yes, such a check could be added in the 
next version of the patch series along with the selftest updates.

John.


> Regards,
>
> Tvrtko
>
>> +    wa.reg.reg |= flags;
>>       _wa_add(wal, &wa);
>>   }
>>   +static void
>> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +{
>> +    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>> +}
>> +
>>   static void gen9_whitelist_build(struct i915_wa_list *w)
>>   {
>>       /* 
>> WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 7a26766ba84d..cc295a4f6e92 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>>   #define   RING_WAIT_SEMAPHORE    (1 << 10) /* gen6+ */
>>     #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + 
>> (i) * 4)
>> +#define   RING_FORCE_TO_NONPRIV_RW        (0 << 28)    /* CFL+ & 
>> Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RD        (1 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_WR        (2 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_1        (0 << 0)     /* CFL+ 
>> & Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_4        (1 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_16    (2 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_64    (3 << 0)
>>   #define   RING_MAX_NONPRIV_SLOTS  12
>>     #define GEN7_TLB_RD_ADDR    _MMIO(0x4700)
>>
Tvrtko Ursulin June 18, 2019, 4:10 p.m. UTC | #4 
On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;
>   
> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 165b0a45e009..ae82340fff45 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1012,7 +1012,7 @@  bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
 }
 
 static void
-whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
+whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
 {
 	struct i915_wa wa = {
 		.reg = reg
@@ -1021,9 +1021,16 @@  whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
 	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
 		return;
 
+	wa.reg.reg |= flags;
 	_wa_add(wal, &wa);
 }
 
+static void
+whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
+{
+	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
+}
+
 static void gen9_whitelist_build(struct i915_wa_list *w)
 {
 	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7a26766ba84d..cc295a4f6e92 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2513,6 +2513,13 @@  enum i915_power_well_id {
 #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
 
 #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
+#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
+#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
+#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
+#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
+#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
+#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
+#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
 #define   RING_MAX_NONPRIV_SLOTS  12
 
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)