subject:"\[Intel\-gfx\] \[PATCH v2 1\/1\] drm\/i915\: Save PM interrupt register offsets in device info"

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-26 Thread Jani Nikula

On Thu, 26 Oct 2017, Jani Nikula  wrote:
> On Thu, 26 Oct 2017, Tvrtko Ursulin  wrote:
>> On 25/10/2017 08:45, Jani Nikula wrote:
>>> On Tue, 24 Oct 2017, Tvrtko Ursulin  wrote:
 On 24/10/17 18:48, Jani Nikula wrote:
> On Tue, 24 Oct 2017, Chris Wilson  wrote:
>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13)
>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>>> b/drivers/gpu/drm/i915/intel_device_info.c
>>> index 875d428..d1a4911 100644
>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
>>> drm_i915_private *dev_priv)
>>>info->sseu.has_subslice_pg ? "y" : "n");
>>>   DRM_DEBUG_DRIVER("has EU power gating: %s\n",
>>>info->sseu.has_eu_pg ? "y" : "n");
>>> +
>>> +   /* Initialize PM interrupt register offsets */
>>> +   if (INTEL_GEN(dev_priv) >= 8) {
>>> +   info->pm_iir_offset = GEN8_GT_IIR(2);
>>> +   info->pm_imr_offset = GEN8_GT_IMR(2);
>>> +   info->pm_ier_offset = GEN8_GT_IER(2);
>>> +   } else {
>>> +   info->pm_iir_offset = GEN6_PMIIR;
>>> +   info->pm_imr_offset = GEN6_PMIMR;
>>> +   info->pm_ier_offset = GEN6_PMIER;
>>> +   }
>>
>> If you are going to take another pass at this, move these into the
>> static tables in i915_pci.c
>>
>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
>> individual platform defines.
>
> Like I wrote in reply to v1, I'm not convinced we should do this at all.
>
> What makes *these* registers so important they must be in device info?
> What makes most of i915_reg.h so unimportant they don't deserve the same
> treatment? Where do you draw the line?
>
> I'd draw the line at, no registers at device info.

 I suggested to Sagar this change during review so feel responsible to
 chime in.

 So in general I just find the amount of times our driver asks itself
 what it's running on a bit tasteless. :(

 I did quick and dirty check by bumping a counter in all the
 IS_this|or|that checks, all which can be known at driver probe time, and
 wired it up to the PMU so I can check their frequency. The annotated
 perf stat output:

 root@e31:~# perf stat -a -e i915/whoami/ -I 1000
 #   time counts unit events

 # idle system no X running

1.000298100 10  i915/whoami/

2.000750955  8  i915/whoami/

3.001104193 10  i915/whoami/

4.001333433 10  i915/whoami/

5.001703162 10  i915/whoami/

6.002122721 10  i915/whoami/


 # starting X now..

7.002266228  2,203  i915/whoami/

8.002392598  4,682  i915/whoami/

9.002764398  0  i915/whoami/

   10.003027119  0  i915/whoami/

   11.003486048 42  i915/whoami/


 # X idling..

   12.003854660  0  i915/whoami/

   13.004221680  0  i915/whoami/

   14.004622571  0  i915/whoami/

   15.004968110  0  i915/whoami/

   16.005372363  0  i915/whoami/

   17.005778034  0  i915/whoami/

   18.005941970  0  i915/whoami/

   19.006313427  0  i915/whoami/

   20.006676048  0  i915/whoami/

   21.007059927  0  i915/whoami/

   22.007507818  0  i915/whoami/

   23.007887628  0  i915/whoami/

   24.008207035  0  i915/whoami/

   25.008580496  0  i915/whoami/

 #   time counts unit events
   26.008949236  0  i915/whoami/

   27.009433473  0  i915/whoami/


 # gfxbench trex starting up

   28.009677600  2,605  i915/whoami/

   29.009941972716  i915/whoami/

   30.010127588  2,190  i915/whoami/

   31.010249535 46  i915/whoami/

   32.010383565 36

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-26 Thread Jani Nikula

On Thu, 26 Oct 2017, Tvrtko Ursulin  wrote:
> On 25/10/2017 08:45, Jani Nikula wrote:
>> On Tue, 24 Oct 2017, Tvrtko Ursulin  wrote:
>>> On 24/10/17 18:48, Jani Nikula wrote:
 On Tue, 24 Oct 2017, Chris Wilson  wrote:
> Quoting Sagar Arun Kamble (2017-10-24 11:41:13)
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index 875d428..d1a4911 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
>> drm_i915_private *dev_priv)
>>info->sseu.has_subslice_pg ? "y" : "n");
>>   DRM_DEBUG_DRIVER("has EU power gating: %s\n",
>>info->sseu.has_eu_pg ? "y" : "n");
>> +
>> +   /* Initialize PM interrupt register offsets */
>> +   if (INTEL_GEN(dev_priv) >= 8) {
>> +   info->pm_iir_offset = GEN8_GT_IIR(2);
>> +   info->pm_imr_offset = GEN8_GT_IMR(2);
>> +   info->pm_ier_offset = GEN8_GT_IER(2);
>> +   } else {
>> +   info->pm_iir_offset = GEN6_PMIIR;
>> +   info->pm_imr_offset = GEN6_PMIMR;
>> +   info->pm_ier_offset = GEN6_PMIER;
>> +   }
>
> If you are going to take another pass at this, move these into the
> static tables in i915_pci.c
>
> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
> individual platform defines.

 Like I wrote in reply to v1, I'm not convinced we should do this at all.

 What makes *these* registers so important they must be in device info?
 What makes most of i915_reg.h so unimportant they don't deserve the same
 treatment? Where do you draw the line?

 I'd draw the line at, no registers at device info.
>>>
>>> I suggested to Sagar this change during review so feel responsible to
>>> chime in.
>>>
>>> So in general I just find the amount of times our driver asks itself
>>> what it's running on a bit tasteless. :(
>>>
>>> I did quick and dirty check by bumping a counter in all the
>>> IS_this|or|that checks, all which can be known at driver probe time, and
>>> wired it up to the PMU so I can check their frequency. The annotated
>>> perf stat output:
>>>
>>> root@e31:~# perf stat -a -e i915/whoami/ -I 1000
>>> #   time counts unit events
>>>
>>> # idle system no X running
>>>
>>>1.000298100 10  i915/whoami/
>>>
>>>2.000750955  8  i915/whoami/
>>>
>>>3.001104193 10  i915/whoami/
>>>
>>>4.001333433 10  i915/whoami/
>>>
>>>5.001703162 10  i915/whoami/
>>>
>>>6.002122721 10  i915/whoami/
>>>
>>>
>>> # starting X now..
>>>
>>>7.002266228  2,203  i915/whoami/
>>>
>>>8.002392598  4,682  i915/whoami/
>>>
>>>9.002764398  0  i915/whoami/
>>>
>>>   10.003027119  0  i915/whoami/
>>>
>>>   11.003486048 42  i915/whoami/
>>>
>>>
>>> # X idling..
>>>
>>>   12.003854660  0  i915/whoami/
>>>
>>>   13.004221680  0  i915/whoami/
>>>
>>>   14.004622571  0  i915/whoami/
>>>
>>>   15.004968110  0  i915/whoami/
>>>
>>>   16.005372363  0  i915/whoami/
>>>
>>>   17.005778034  0  i915/whoami/
>>>
>>>   18.005941970  0  i915/whoami/
>>>
>>>   19.006313427  0  i915/whoami/
>>>
>>>   20.006676048  0  i915/whoami/
>>>
>>>   21.007059927  0  i915/whoami/
>>>
>>>   22.007507818  0  i915/whoami/
>>>
>>>   23.007887628  0  i915/whoami/
>>>
>>>   24.008207035  0  i915/whoami/
>>>
>>>   25.008580496  0  i915/whoami/
>>>
>>> #   time counts unit events
>>>   26.008949236  0  i915/whoami/
>>>
>>>   27.009433473  0  i915/whoami/
>>>
>>>
>>> # gfxbench trex starting up
>>>
>>>   28.009677600  2,605  i915/whoami/
>>>
>>>   29.009941972716  i915/whoami/
>>>
>>>   30.010127588  2,190  i915/whoami/
>>>
>>>   31.010249535 46  i915/whoami/
>>>
>>>   32.010383565 36  i915/whoami/
>>>
>>>   33.010527674  0  i915/whoami/
>>>
>>>
>>> # trex running
>>>
>>>   34.010760584  4,709  i915/whoami/
>>>
>>>   35.011079891

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-26 Thread Tvrtko Ursulin



On 25/10/2017 08:45, Jani Nikula wrote:

On Tue, 24 Oct 2017, Tvrtko Ursulin  wrote:

On 24/10/17 18:48, Jani Nikula wrote:

On Tue, 24 Oct 2017, Chris Wilson  wrote:

Quoting Sagar Arun Kamble (2017-10-24 11:41:13)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c
index 875d428..d1a4911 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
drm_i915_private *dev_priv)
   info->sseu.has_subslice_pg ? "y" : "n");
  DRM_DEBUG_DRIVER("has EU power gating: %s\n",
   info->sseu.has_eu_pg ? "y" : "n");
+
+   /* Initialize PM interrupt register offsets */
+   if (INTEL_GEN(dev_priv) >= 8) {
+   info->pm_iir_offset = GEN8_GT_IIR(2);
+   info->pm_imr_offset = GEN8_GT_IMR(2);
+   info->pm_ier_offset = GEN8_GT_IER(2);
+   } else {
+   info->pm_iir_offset = GEN6_PMIIR;
+   info->pm_imr_offset = GEN6_PMIMR;
+   info->pm_ier_offset = GEN6_PMIER;
+   }


If you are going to take another pass at this, move these into the
static tables in i915_pci.c

Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
individual platform defines.


Like I wrote in reply to v1, I'm not convinced we should do this at all.

What makes *these* registers so important they must be in device info?
What makes most of i915_reg.h so unimportant they don't deserve the same
treatment? Where do you draw the line?

I'd draw the line at, no registers at device info.


I suggested to Sagar this change during review so feel responsible to
chime in.

So in general I just find the amount of times our driver asks itself
what it's running on a bit tasteless. :(

I did quick and dirty check by bumping a counter in all the
IS_this|or|that checks, all which can be known at driver probe time, and
wired it up to the PMU so I can check their frequency. The annotated
perf stat output:

root@e31:~# perf stat -a -e i915/whoami/ -I 1000
#   time counts unit events

# idle system no X running

   1.000298100 10  i915/whoami/

   2.000750955  8  i915/whoami/

   3.001104193 10  i915/whoami/

   4.001333433 10  i915/whoami/

   5.001703162 10  i915/whoami/

   6.002122721 10  i915/whoami/


# starting X now..

   7.002266228  2,203  i915/whoami/

   8.002392598  4,682  i915/whoami/

   9.002764398  0  i915/whoami/

  10.003027119  0  i915/whoami/

  11.003486048 42  i915/whoami/


# X idling..

  12.003854660  0  i915/whoami/

  13.004221680  0  i915/whoami/

  14.004622571  0  i915/whoami/

  15.004968110  0  i915/whoami/

  16.005372363  0  i915/whoami/

  17.005778034  0  i915/whoami/

  18.005941970  0  i915/whoami/

  19.006313427  0  i915/whoami/

  20.006676048  0  i915/whoami/

  21.007059927  0  i915/whoami/

  22.007507818  0  i915/whoami/

  23.007887628  0  i915/whoami/

  24.008207035  0  i915/whoami/

  25.008580496  0  i915/whoami/

#   time counts unit events
  26.008949236  0  i915/whoami/

  27.009433473  0  i915/whoami/


# gfxbench trex starting up

  28.009677600  2,605  i915/whoami/

  29.009941972716  i915/whoami/

  30.010127588  2,190  i915/whoami/

  31.010249535 46  i915/whoami/

  32.010383565 36  i915/whoami/

  33.010527674  0  i915/whoami/


# trex running

  34.010760584  4,709  i915/whoami/

  35.011079891  5,381  i915/whoami/

  36.011280234  5,306  i915/whoami/

  37.011719986  5,505  i915/whoami/

  38.012017531  5,386  i915/whoami/

  39.012529241  5,687  i915/whoami/

  40.012922986  6,009  i915/whoami/

  41.013120143  5,791  i915/whoami/

  42.013399982  5,296  i915/whoami/

  43.013712979  5,349  i915/whoami/

  44.014107375  5,127  i915/whoami/

  45.014553950  5,387  i915/whoami/

  46.014953020  5,364

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-25 Thread Jani Nikula

On Tue, 24 Oct 2017, Tvrtko Ursulin  wrote:
> On 24/10/17 18:48, Jani Nikula wrote:
>> On Tue, 24 Oct 2017, Chris Wilson  wrote:
>>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13)
 diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
 b/drivers/gpu/drm/i915/intel_device_info.c
 index 875d428..d1a4911 100644
 --- a/drivers/gpu/drm/i915/intel_device_info.c
 +++ b/drivers/gpu/drm/i915/intel_device_info.c
 @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
 drm_i915_private *dev_priv)
   info->sseu.has_subslice_pg ? "y" : "n");
  DRM_DEBUG_DRIVER("has EU power gating: %s\n",
   info->sseu.has_eu_pg ? "y" : "n");
 +
 +   /* Initialize PM interrupt register offsets */
 +   if (INTEL_GEN(dev_priv) >= 8) {
 +   info->pm_iir_offset = GEN8_GT_IIR(2);
 +   info->pm_imr_offset = GEN8_GT_IMR(2);
 +   info->pm_ier_offset = GEN8_GT_IER(2);
 +   } else {
 +   info->pm_iir_offset = GEN6_PMIIR;
 +   info->pm_imr_offset = GEN6_PMIMR;
 +   info->pm_ier_offset = GEN6_PMIER;
 +   }
>>>
>>> If you are going to take another pass at this, move these into the
>>> static tables in i915_pci.c
>>>
>>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
>>> individual platform defines.
>> 
>> Like I wrote in reply to v1, I'm not convinced we should do this at all.
>> 
>> What makes *these* registers so important they must be in device info?
>> What makes most of i915_reg.h so unimportant they don't deserve the same
>> treatment? Where do you draw the line?
>> 
>> I'd draw the line at, no registers at device info.
>
> I suggested to Sagar this change during review so feel responsible to 
> chime in.
>
> So in general I just find the amount of times our driver asks itself 
> what it's running on a bit tasteless. :(
>
> I did quick and dirty check by bumping a counter in all the 
> IS_this|or|that checks, all which can be known at driver probe time, and 
> wired it up to the PMU so I can check their frequency. The annotated 
> perf stat output:
>
> root@e31:~# perf stat -a -e i915/whoami/ -I 1000
> #   time counts unit events
>
> # idle system no X running
>
>   1.000298100 10  i915/whoami/ 
>
>   2.000750955  8  i915/whoami/ 
>
>   3.001104193 10  i915/whoami/ 
>
>   4.001333433 10  i915/whoami/ 
>
>   5.001703162 10  i915/whoami/ 
>
>   6.002122721 10  i915/whoami/ 
>
>
> # starting X now..
>
>   7.002266228  2,203  i915/whoami/ 
>
>   8.002392598  4,682  i915/whoami/ 
>
>   9.002764398  0  i915/whoami/ 
>
>  10.003027119  0  i915/whoami/ 
>
>  11.003486048 42  i915/whoami/ 
>
>
> # X idling..
>
>  12.003854660  0  i915/whoami/ 
>
>  13.004221680  0  i915/whoami/ 
>
>  14.004622571  0  i915/whoami/ 
>
>  15.004968110  0  i915/whoami/ 
>
>  16.005372363  0  i915/whoami/ 
>
>  17.005778034  0  i915/whoami/ 
>
>  18.005941970  0  i915/whoami/ 
>
>  19.006313427  0  i915/whoami/ 
>
>  20.006676048  0  i915/whoami/ 
>
>  21.007059927  0  i915/whoami/ 
>
>  22.007507818  0  i915/whoami/ 
>
>  23.007887628  0  i915/whoami/ 
>
>  24.008207035  0  i915/whoami/ 
>
>  25.008580496  0  i915/whoami/ 
>
> #   time counts unit events
>  26.008949236  0  i915/whoami/ 
>
>  27.009433473  0  i915/whoami/ 
>
>
> # gfxbench trex starting up
>
>  28.009677600  2,605  i915/whoami/ 
>
>  29.009941972716  i915/whoami/ 
>
>  30.010127588  2,190  i915/whoami/ 
>
>  31.010249535 46  i915/whoami/ 
>
>  32.010383565 36  i915/whoami/ 
>
>  33.010527674  0  i915/whoami/ 
>
>
> # trex running
>
>  34.010760584  4,709  i915/whoami/ 
>
>  35.011079891  5,381  i915/whoami/ 
>
>  36.011280234  5,306  i915/whoami/ 
>
>  37.011719986  5,505  i915/whoami/ 
>
>  38.012017531  5,386  i915/whoami/ 
>
>  39.012529241  5,687  i915/whoami/ 
>
>  40.012922986  6,009  i915/whoami/ 
>
>  41.013120143  5,791  i915/whoami/ 
>

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Tvrtko Ursulin




On 24/10/17 18:48, Jani Nikula wrote:

On Tue, 24 Oct 2017, Chris Wilson  wrote:

Quoting Sagar Arun Kamble (2017-10-24 11:41:13)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c
index 875d428..d1a4911 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
drm_i915_private *dev_priv)
  info->sseu.has_subslice_pg ? "y" : "n");
 DRM_DEBUG_DRIVER("has EU power gating: %s\n",
  info->sseu.has_eu_pg ? "y" : "n");
+
+   /* Initialize PM interrupt register offsets */
+   if (INTEL_GEN(dev_priv) >= 8) {
+   info->pm_iir_offset = GEN8_GT_IIR(2);
+   info->pm_imr_offset = GEN8_GT_IMR(2);
+   info->pm_ier_offset = GEN8_GT_IER(2);
+   } else {
+   info->pm_iir_offset = GEN6_PMIIR;
+   info->pm_imr_offset = GEN6_PMIMR;
+   info->pm_ier_offset = GEN6_PMIER;
+   }


If you are going to take another pass at this, move these into the
static tables in i915_pci.c

Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
individual platform defines.


Like I wrote in reply to v1, I'm not convinced we should do this at all.

What makes *these* registers so important they must be in device info?
What makes most of i915_reg.h so unimportant they don't deserve the same
treatment? Where do you draw the line?

I'd draw the line at, no registers at device info.


I suggested to Sagar this change during review so feel responsible to 
chime in.


So in general I just find the amount of times our driver asks itself 
what it's running on a bit tasteless. :(


I did quick and dirty check by bumping a counter in all the 
IS_this|or|that checks, all which can be known at driver probe time, and 
wired it up to the PMU so I can check their frequency. The annotated 
perf stat output:


root@e31:~# perf stat -a -e i915/whoami/ -I 1000
#   time counts unit events

# idle system no X running

 1.000298100 10  i915/whoami/ 

 2.000750955  8  i915/whoami/ 

 3.001104193 10  i915/whoami/ 

 4.001333433 10  i915/whoami/ 

 5.001703162 10  i915/whoami/ 

 6.002122721 10  i915/whoami/ 



# starting X now..

 7.002266228  2,203  i915/whoami/ 

 8.002392598  4,682  i915/whoami/ 

 9.002764398  0  i915/whoami/ 

10.003027119  0  i915/whoami/ 

11.003486048 42  i915/whoami/ 



# X idling..

12.003854660  0  i915/whoami/ 

13.004221680  0  i915/whoami/ 

14.004622571  0  i915/whoami/ 

15.004968110  0  i915/whoami/ 

16.005372363  0  i915/whoami/ 

17.005778034  0  i915/whoami/ 

18.005941970  0  i915/whoami/ 

19.006313427  0  i915/whoami/ 

20.006676048  0  i915/whoami/ 

21.007059927  0  i915/whoami/ 

22.007507818  0  i915/whoami/ 

23.007887628  0  i915/whoami/ 

24.008207035  0  i915/whoami/ 

25.008580496  0  i915/whoami/ 


#   time counts unit events
26.008949236  0  i915/whoami/ 

27.009433473  0  i915/whoami/ 



# gfxbench trex starting up

28.009677600  2,605  i915/whoami/ 

29.009941972716  i915/whoami/ 

30.010127588  2,190  i915/whoami/ 

31.010249535 46  i915/whoami/ 

32.010383565 36  i915/whoami/ 

33.010527674  0  i915/whoami/ 



# trex running

34.010760584  4,709  i915/whoami/ 

35.011079891  5,381  i915/whoami/ 

36.011280234  5,306  i915/whoami/ 

37.011719986  5,505  i915/whoami/ 

38.012017531  5,386  i915/whoami/ 

39.012529241  5,687  i915/whoami/ 

40.012922986  6,009  i915/whoami/ 

41.013120143  5,791  i915/whoami/ 

42.013399982  5,296  i915/whoami/ 

43.013712979  5,349  i915/whoami/ 

44.014107375  5,127  i915/whoami/ 

45.014553950  5,387  i915/whoami/ 

46.014953020  5,364  i915/whoami/ 

47.015243748  4,738  i915/whoami/ 

48.015560460  4,788  i915/whoami/ 

49.015867395

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Jani Nikula

On Tue, 24 Oct 2017, Chris Wilson  wrote:
> Quoting Sagar Arun Kamble (2017-10-24 11:41:13)
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index 875d428..d1a4911 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
>> drm_i915_private *dev_priv)
>>  info->sseu.has_subslice_pg ? "y" : "n");
>> DRM_DEBUG_DRIVER("has EU power gating: %s\n",
>>  info->sseu.has_eu_pg ? "y" : "n");
>> +
>> +   /* Initialize PM interrupt register offsets */
>> +   if (INTEL_GEN(dev_priv) >= 8) {
>> +   info->pm_iir_offset = GEN8_GT_IIR(2);
>> +   info->pm_imr_offset = GEN8_GT_IMR(2);
>> +   info->pm_ier_offset = GEN8_GT_IER(2);
>> +   } else {
>> +   info->pm_iir_offset = GEN6_PMIIR;
>> +   info->pm_imr_offset = GEN6_PMIMR;
>> +   info->pm_ier_offset = GEN6_PMIER;
>> +   }
>
> If you are going to take another pass at this, move these into the
> static tables in i915_pci.c
>
> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
> individual platform defines.

Like I wrote in reply to v1, I'm not convinced we should do this at all.

What makes *these* registers so important they must be in device info?
What makes most of i915_reg.h so unimportant they don't deserve the same
treatment? Where do you draw the line?

I'd draw the line at, no registers at device info.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Chris Wilson

Quoting Sagar Arun Kamble (2017-10-24 11:41:13)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
> b/drivers/gpu/drm/i915/intel_device_info.c
> index 875d428..d1a4911 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
> drm_i915_private *dev_priv)
>  info->sseu.has_subslice_pg ? "y" : "n");
> DRM_DEBUG_DRIVER("has EU power gating: %s\n",
>  info->sseu.has_eu_pg ? "y" : "n");
> +
> +   /* Initialize PM interrupt register offsets */
> +   if (INTEL_GEN(dev_priv) >= 8) {
> +   info->pm_iir_offset = GEN8_GT_IIR(2);
> +   info->pm_imr_offset = GEN8_GT_IMR(2);
> +   info->pm_ier_offset = GEN8_GT_IER(2);
> +   } else {
> +   info->pm_iir_offset = GEN6_PMIIR;
> +   info->pm_imr_offset = GEN6_PMIMR;
> +   info->pm_ier_offset = GEN6_PMIER;
> +   }

If you are going to take another pass at this, move these into the
static tables in i915_pci.c

Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into
individual platform defines.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Sagar Arun Kamble




On 10/24/2017 4:16 PM, Michal Wajdeczko wrote:
On Tue, 24 Oct 2017 12:41:13 +0200, Sagar Arun Kamble 
 wrote:


PM interrupt register offsets are constant per platforms and saving 
those

in device info is more appropriate than getting those through functions.
This patch removes functions gen6_pm_iir/imr/ier and saves those offsets
in device info.

v2: Use INTEL_INFO() to access device info. (Chris)

Suggested-by: Tvrtko Ursulin 
Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_drv.h  |  5 +
 drivers/gpu/drm/i915/i915_irq.c  | 31 
+--

 drivers/gpu/drm/i915/intel_device_info.c | 11 +++
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h

index 54b5d4c..2f77d26 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -888,6 +888,11 @@ struct intel_device_info {
 u16 degamma_lut_size;
 u16 gamma_lut_size;
 } color;
+
+    /* PM interrupt register offsets */
+    i915_reg_t pm_iir_offset;
+    i915_reg_t pm_imr_offset;
+    i915_reg_t pm_ier_offset;
 };
struct intel_display_error_state;
diff --git a/drivers/gpu/drm/i915/i915_irq.c 
b/drivers/gpu/drm/i915/i915_irq.c

index b1296a5..5d448af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private 
*dev_priv, uint32_t mask)

 ilk_update_gt_irq(dev_priv, mask, 0);
 }
-static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
-{
-    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
-}
-
-static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv)
-{
-    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR;
-}
-
-static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv)
-{
-    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER;
-}
-


btw, if you keep these functions but modify them into:

return INTEL_INFO(dev_priv)->pm_xxx_offset;

then most of below changes will not be needed

Yes. Will keep these functions.



 /**
  * snb_update_pm_irq - update GEN6_PMIMR
  * @dev_priv: driver private
@@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct 
drm_i915_private *dev_priv,

   uint32_t enabled_irq_mask)
 {
 uint32_t new_val;
+    i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset;


s/reg/imr ?

will remove this change since we are keeping gen6_pm* functions.



WARN_ON(enabled_irq_mask & ~interrupt_mask);
@@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct 
drm_i915_private *dev_priv,

if (new_val != dev_priv->pm_imr) {
 dev_priv->pm_imr = new_val;
-    I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
-    POSTING_READ(gen6_pm_imr(dev_priv));
+    I915_WRITE(reg, dev_priv->pm_imr);
+    POSTING_READ(reg);
 }
 }
@@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private 
*dev_priv, u32 mask)
static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 
reset_mask)

 {
-    i915_reg_t reg = gen6_pm_iir(dev_priv);
+    i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset;


s/reg/iir ?

will remove this as well.



lockdep_assert_held(_priv->irq_lock);
@@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct 
drm_i915_private *dev_priv, u32 enable_mas

 lockdep_assert_held(_priv->irq_lock);
dev_priv->pm_ier |= enable_mask;
-    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+    I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
 gen6_unmask_pm_irq(dev_priv, enable_mask);
 /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
 }
@@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct 
drm_i915_private *dev_priv, u32 disable_m

dev_priv->pm_ier &= ~disable_mask;
 __gen6_mask_pm_irq(dev_priv, disable_mask);
-    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+    I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
 /* though a barrier is missing here, but don't really need a one */
 }
@@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct 
drm_i915_private *dev_priv)

spin_lock_irq(_priv->irq_lock);
 WARN_ON_ONCE(rps->pm_iir);
-    WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & 
dev_priv->pm_rps_events);

+ WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
+   dev_priv->pm_rps_events);


Can you define separate iir_reg variable as in above functions to 
simplify

this nested statement ?

and this as well.



 rps->interrupts_enabled = true;
 gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
@@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct 
drm_i915_private

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Michal Wajdeczko

On Tue, 24 Oct 2017 12:41:13 +0200, Sagar Arun Kamble  
 wrote:



PM interrupt register offsets are constant per platforms and saving those
in device info is more appropriate than getting those through functions.
This patch removes functions gen6_pm_iir/imr/ier and saves those offsets
in device info.

v2: Use INTEL_INFO() to access device info. (Chris)

Suggested-by: Tvrtko Ursulin 
Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_drv.h  |  5 +
 drivers/gpu/drm/i915/i915_irq.c  | 31  
+--

 drivers/gpu/drm/i915/intel_device_info.c | 11 +++
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h  
b/drivers/gpu/drm/i915/i915_drv.h

index 54b5d4c..2f77d26 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -888,6 +888,11 @@ struct intel_device_info {
u16 degamma_lut_size;
u16 gamma_lut_size;
} color;
+
+   /* PM interrupt register offsets */
+   i915_reg_t pm_iir_offset;
+   i915_reg_t pm_imr_offset;
+   i915_reg_t pm_ier_offset;
 };
struct intel_display_error_state;
diff --git a/drivers/gpu/drm/i915/i915_irq.c  
b/drivers/gpu/drm/i915/i915_irq.c

index b1296a5..5d448af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private  
*dev_priv, uint32_t mask)

ilk_update_gt_irq(dev_priv, mask, 0);
 }
-static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
-}
-
-static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR;
-}
-
-static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER;
-}
-


btw, if you keep these functions but modify them into:

return INTEL_INFO(dev_priv)->pm_xxx_offset;

then most of below changes will not be needed


 /**
  * snb_update_pm_irq - update GEN6_PMIMR
  * @dev_priv: driver private
@@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct  
drm_i915_private *dev_priv,

  uint32_t enabled_irq_mask)
 {
uint32_t new_val;
+   i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset;


s/reg/imr ?


WARN_ON(enabled_irq_mask & ~interrupt_mask);
@@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct  
drm_i915_private *dev_priv,

if (new_val != dev_priv->pm_imr) {
dev_priv->pm_imr = new_val;
-   I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
-   POSTING_READ(gen6_pm_imr(dev_priv));
+   I915_WRITE(reg, dev_priv->pm_imr);
+   POSTING_READ(reg);
}
 }
@@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private  
*dev_priv, u32 mask)
static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32  
reset_mask)

 {
-   i915_reg_t reg = gen6_pm_iir(dev_priv);
+   i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset;


s/reg/iir ?


lockdep_assert_held(_priv->irq_lock);
@@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct  
drm_i915_private *dev_priv, u32 enable_mas

lockdep_assert_held(_priv->irq_lock);
dev_priv->pm_ier |= enable_mask;
-   I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+   I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
gen6_unmask_pm_irq(dev_priv, enable_mask);
/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
 }
@@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct  
drm_i915_private *dev_priv, u32 disable_m

dev_priv->pm_ier &= ~disable_mask;
__gen6_mask_pm_irq(dev_priv, disable_mask);
-   I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+   I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
/* though a barrier is missing here, but don't really need a one */
 }
@@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct  
drm_i915_private *dev_priv)

spin_lock_irq(_priv->irq_lock);
WARN_ON_ONCE(rps->pm_iir);
-	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &  
dev_priv->pm_rps_events);

+   WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
+  dev_priv->pm_rps_events);


Can you define separate iir_reg variable as in above functions to simplify
this nested statement ?


rps->interrupts_enabled = true;
gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
@@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct  
drm_i915_private *dev_priv)

[Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

2017-10-24 Thread Sagar Arun Kamble

PM interrupt register offsets are constant per platforms and saving those
in device info is more appropriate than getting those through functions.
This patch removes functions gen6_pm_iir/imr/ier and saves those offsets
in device info.

v2: Use INTEL_INFO() to access device info. (Chris)

Suggested-by: Tvrtko Ursulin 
Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_drv.h  |  5 +
 drivers/gpu/drm/i915/i915_irq.c  | 31 +--
 drivers/gpu/drm/i915/intel_device_info.c | 11 +++
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 54b5d4c..2f77d26 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -888,6 +888,11 @@ struct intel_device_info {
u16 degamma_lut_size;
u16 gamma_lut_size;
} color;
+
+   /* PM interrupt register offsets */
+   i915_reg_t pm_iir_offset;
+   i915_reg_t pm_imr_offset;
+   i915_reg_t pm_ier_offset;
 };
 
 struct intel_display_error_state;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1296a5..5d448af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private 
*dev_priv, uint32_t mask)
ilk_update_gt_irq(dev_priv, mask, 0);
 }
 
-static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
-}
-
-static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR;
-}
-
-static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv)
-{
-   return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER;
-}
-
 /**
  * snb_update_pm_irq - update GEN6_PMIMR
  * @dev_priv: driver private
@@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct drm_i915_private 
*dev_priv,
  uint32_t enabled_irq_mask)
 {
uint32_t new_val;
+   i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset;
 
WARN_ON(enabled_irq_mask & ~interrupt_mask);
 
@@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct drm_i915_private 
*dev_priv,
 
if (new_val != dev_priv->pm_imr) {
dev_priv->pm_imr = new_val;
-   I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
-   POSTING_READ(gen6_pm_imr(dev_priv));
+   I915_WRITE(reg, dev_priv->pm_imr);
+   POSTING_READ(reg);
}
 }
 
@@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, 
u32 mask)
 
 static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 
reset_mask)
 {
-   i915_reg_t reg = gen6_pm_iir(dev_priv);
+   i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset;
 
lockdep_assert_held(_priv->irq_lock);
 
@@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct drm_i915_private 
*dev_priv, u32 enable_mas
lockdep_assert_held(_priv->irq_lock);
 
dev_priv->pm_ier |= enable_mask;
-   I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+   I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
gen6_unmask_pm_irq(dev_priv, enable_mask);
/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
 }
@@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct drm_i915_private 
*dev_priv, u32 disable_m
 
dev_priv->pm_ier &= ~disable_mask;
__gen6_mask_pm_irq(dev_priv, disable_mask);
-   I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+   I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
/* though a barrier is missing here, but don't really need a one */
 }
 
@@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct drm_i915_private 
*dev_priv)
 
spin_lock_irq(_priv->irq_lock);
WARN_ON_ONCE(rps->pm_iir);
-   WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & 
dev_priv->pm_rps_events);
+   WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
+  dev_priv->pm_rps_events);
rps->interrupts_enabled = true;
gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
 
@@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct drm_i915_private 
*dev_priv)
 {
spin_lock_irq(_priv->irq_lock);
if (!dev_priv->guc.interrupts_enabled) {
-   WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+   WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
   dev_priv->pm_guc_events);
dev_priv->guc.interrupts_enabled = true;

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

[Intel-gfx] [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

10 matches

Site Navigation

Mail list logo

Footer information