diff mbox series

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

Message ID 20190614002838.3072-2-robert.m.fosha@intel.com (mailing list archive)
State New, archived
Headers show
Series Update whitelist support for new hardware | expand

Commit Message

Fosha, Robert M June 14, 2019, 12:28 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 14, 2019, 6:48 a.m. UTC | #1 
On 14/06/2019 01:28, Robert M. Fosha 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 edf9f93934a1..10fea5ab3fc3 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+ */

Shouldn't this comment be against the RD and WR flags, while the above 
is the legacy one?

> +#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)
> 

Regards,

Tvrtko
John Harrison June 18, 2019, 1:13 a.m. UTC | #2 
On 6/13/2019 23:48, Tvrtko Ursulin wrote:
>
> On 14/06/2019 01:28, Robert M. Fosha 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 edf9f93934a1..10fea5ab3fc3 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+ */
>
> Shouldn't this comment be against the RD and WR flags, while the above 
> is the legacy one?

Technically, this field does not exist at all on older hardware. However 
these bits are MBZ hence are equivalent to having the value of RW being 
set. To me, it makes more sense to be accurate and label the entire 
field as newer hardware only.

John.
Tvrtko Ursulin June 18, 2019, 6:51 a.m. UTC | #3 
On 18/06/2019 02:13, John Harrison wrote:
> On 6/13/2019 23:48, Tvrtko Ursulin wrote:
>>
>> On 14/06/2019 01:28, Robert M. Fosha 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 edf9f93934a1..10fea5ab3fc3 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+ */
>>
>> Shouldn't this comment be against the RD and WR flags, while the above 
>> is the legacy one?
> 
> Technically, this field does not exist at all on older hardware. However 
> these bits are MBZ hence are equivalent to having the value of RW being 
> set. To me, it makes more sense to be accurate and label the entire 
> field as newer hardware only.

Did not see your reply initially due not being copied.

Okay then I guess. Just going forward make sure the patches for old 
workarounds don't use RING_FORCE_TO_NONPRIV_RW like the previous round 
did. (Can't have it both ways. ;)

Regards,

Tvrtko
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 edf9f93934a1..10fea5ab3fc3 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)