Hi Paulo,
Thanks for comments.
1. Will make change related to #define for number of pipes and remove
the unnecessary ones.
2. vrefresh is almost same as "clock/(htotal*vtotal) if we round up
later. Will keep it vrefresh for now.
Thanks
Sagar
On 2/4/2016 1:55 AM, Zanoni, Paulo R wrote:
Em Qua, 2016-01-20 às 18:26 -0800, tom.orourke@xxxxxxxxx escreveu:
From: Tom O'Rourke <Tom.O'Rourke@xxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
