[Intel-gfx] [RFC 00/22] Add support for GuC-based SLPC

Wed Feb 3 20:25:24 UTC 2016

Em Qua, 2016-01-20 às 18:26 -0800, tom.orourke at intel.com escreveu:
> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
> 
> SLPC (Single Loop Power Controller) is a replacement for
> some host-based power management features.  The SLPC
> implemenation runs in firmware on GuC.
> 
> This series is a first request for comments.  This series
> is not expected to be merged.  After changes based on
> comments, a later patch series will be sent for merging.
>  
> This series has been tested with SKL guc firmware
> versions 4.3 and 4.7.  The graphics power management
> features in SLPC in those versions are DFPS (Dynamic FPS),
> Turbo, and DCC (Duty Cycle Control).  DFPS adjusts
> requested graphics frequency to maintain target framerate.
> Turbo adjusts requested graphics frequency to maintain
> target GT busyness.  DCC adjusts requested graphics
> frequency and stalls guc-scheduler to maintain actual
> graphics frequency in efficient range.
> 
> Patch 1/22 is included ihere for convenience and should be
> part of an earlier series.  SLPC assumes guc firmware has
> been loaded and GuC submission is enabled.
> 
> Patch 22/22 sets the flag to enable SLPC on SKL.  Without
> this patch, the previous patches should have no effect.
> 
> VIZ-6773, VIZ-6889

Hi

Some high-level comments for the whole series:

In many places there are 8 spaces instead of tabs.

There are also some lines containing just a tab character.

Lots of Yoda conditions: if (constant == variable).

Some functions would get much simpler if they had early returns.

I certainly wouldn't complain if you also extracted the relevant
rps/powersave code out of intel_pm.c to its own file with a nice
documentation. Of course, this could be done after the series.

Lots of ignored return values. It seems the inner functions all check
for errors and return them to their callers, but the top-most functions
added by the series just ignore the errors. See my previous comment on
patch 14 about this for suggestions.

There are no checks for GuC version here. We know that the SLPC ABI is
not stable and can change in new firmware versions, so I expect the
SLPC code to only run if it finds the specific _whitelisted_ GuC
versions. No "if (version >= num) use_slpc=true;", please.

I'm not 100% sure, but from what I could understand, it seems I'll get
a broken machine with no RPS at all if I don't have the GuC firmware
files - or if the GuC version is not the expected. IMHO this is a
regression since I currently don't have any firmware files and my SKL
machine works.

I see most of the functions are protected by "HAS_SLPC". Usually
HAS_SOMETHING is used for hardware features and is expected to be
constant on platforms. I suggest you to add a possible driver parameter
such as i915.enable_slpc, and also add a new "intel_slpc_enabled()" or
"intel_using_slpc()" function and replace all the HAS_SLPC checks with
these. This way, we'll be able to easily disable SLPC in case we don't
want it (such as due to a regression) or if there's no firmware or
incorrect firmware version, and revert back to the old (current)
behavior of driver-based turbo. The only HAS_SLPC check should be
during SLPC initialization. Having an easy way to enable/disable SLPC
will also be immensely helpful when we start running workloads to
decide if we want to enable it.

You could even go further and make the i915.enable_slpc param be a mask
where it's possible to select each sub-feature individually (dfps,
turbo, dcc).

Some of the checks for shared_data_obj could also become calls to an
inline function with a nice name such as intel_slpc_enabled() or
something else.

I see there's a specific pattern on the places that call
host2guc_action(). Perhaps we could move that common code to its own
function? It would also be nice to add a name to the 0xFF mask that we
return - and that gets ignored at the end of the call chains.

Patch 5 needs a commit message. Actually, when reviewing patch 4 I
thought it had broken RC6, until I saw patch 5, so maybe you could just
squash both commits into one.

On patch 13, those defines such as MAX_INTEL_PIPES are weird and
confusing (why do we check if they were already defined?), especially
since we already have I915_MAX_PIPES. And the only value that is
actually used is MAX_NUM_OF_PIPE. I would vote for you to only keep
this define, and prefix it with SLPC, such as SLPC_NUM_OF_PIPES (or any
other better name).

On patches 14/15, is mode->vrefresh accurate enough? Don't we want the
more-precise "clock/(htotal*vtotal)" value?

On patch 17. I'm not an expert here, but I'm not sure if we can call
kmap_atomic and then do those seq_printf calls without kunmap. 

Thanks,
Paulo

> 
> Dave Gordon (1):
>   drm/i915: Enable GuC submission, where supported
> 
> Sagar Arun Kamble (4):
>   drm/i915/slpc: Enable/Disable RC6 in SLPC flows
>   drm/i915/slpc: Add Display mode event related data structures
>   drm/i915/slpc: Notification of Display mode change
>   drm/i915/slpc: Notification of Refresh Rate change
> 
> Tom O'Rourke (17):
>   drm/i915/slpc: Add has_slpc capability flag
>   drm/i915/slpc: Expose guc functions for use with SLPC
>   drm/i915/slpc: Use intel_slpc_* functions if supported
>   drm/i915/slpc: If using SLPC, do not set frequency
>   drm/i915/slpc: Enable SLPC in guc if supported
>   drm/i915/slpc: Allocate/Release/Initialize SLPC shared data
>   drm/i915/slpc: Setup rps frequency values during SLPC init
>   drm/i915/slpc: Update current requested frequency
>   drm/i915/slpc: Send reset event
>   drm/i915/slpc: Send shutdown event
>   drm/i915/slpc: Add slpc_status enum values
>   drm/i915/slpc: Add i915_slpc_info to debugfs
>   drm/i915/slpc: Add dfps task info to i915_slpc_info
>   drm/i915/slpc: Add parameter unset/set/get functions
>   drm/i915/slpc: Add slpc support for max/min freq
>   drm/i915/slpc: Add enable/disable debugfs for slpc
>   drm/i915/slpc: Add has_slpc to skylake info
> 
>  drivers/gpu/drm/i915/Makefile              |   5 +-
>  drivers/gpu/drm/i915/i915_debugfs.c        | 436
> +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c            |   1 +
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +
>  drivers/gpu/drm/i915/i915_guc_submission.c |   6 +-
>  drivers/gpu/drm/i915/i915_params.c         |   4 +-
>  drivers/gpu/drm/i915/i915_sysfs.c          |  10 +
>  drivers/gpu/drm/i915/intel_display.c       |   2 +
>  drivers/gpu/drm/i915/intel_dp.c            |   2 +
>  drivers/gpu/drm/i915/intel_drv.h           |   1 +
>  drivers/gpu/drm/i915/intel_guc.h           |   7 +
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   3 +
>  drivers/gpu/drm/i915/intel_pm.c            |  43 ++-
>  drivers/gpu/drm/i915/intel_slpc.c          | 499
> +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_slpc.h          | 207 ++++++++++++
>  15 files changed, 1210 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_slpc.c
>  create mode 100644 drivers/gpu/drm/i915/intel_slpc.h
> 