Em Ter, 2016-02-09 às 12:33 +0530, Kamble, Sagar A escreveu: > 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. I was under the impression that the VSyncFTUsec field wanted the more precise solution. Maybe Tom or someone else could clarify this to us. > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
