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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 24 Oct 2017, Tvrtko Ursulin <tursulin@xxxxxxxxxxx> wrote:
> On 24/10/17 18:48, Jani Nikula wrote:
>> On Tue, 24 Oct 2017, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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.009941972                716      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              4,927      i915/whoami/ 
>
>      50.016152690              4,886      i915/whoami/ 
>
>
> So.. I am not saying these particular registers are mega important, and 
> not even saying that these 5k/s conditionals are measurable (either as 
> branches or increased code size effect), but overall the situation is a 
> bit of.. bleurgh from the elegance point of view. :(
>
> If we have register sets which are 100% mutually exclusive, then I see 
> them as candidates to put them in some object at probe time. It doesn't 
> have to be device_info but I don't see why we wouldn't do it. It is just 
> a different flavour of the vfunc approach after all.

I think to fix something that is inelegant, you have to have a plan to
actually improve things in the long run. IMO adding a few random
registers to device info without a plan is less elegant and less
consistent than the status quo.

We currently have at least three ways to index pipe/port/transcoder/etc
based registers. Combine that with storing some register offsets in
device info, you'll have six ways. There's a chance we'll end up adding
the register offsets to device info both statically and
dynamically. We're already struggling with guiding new contributors to
defining registers in the existing schemes.

Now, I'm sure we could spend weeks on end devising a plan how to move
register offsets to device info or another structure, working out the
details and bikeshedding. After that, we could do weeks and weeks of
busywork converting registers, causing conflicts in all the work in our
internal trees and developers' own branches, not to mention making bug
fix and feature backports more painful.

I have a pretty strong feeling this is not a good use of our time.

BR,
Jani.


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux