Em Qua, 2016-01-20 às 18:26 -0800, tom.orou...@intel.com escreveu: > From: Tom O'Rourke <Tom.O'rou...@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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
