Re: [RFC 00/22] Add support for GuC-based SLPC

["Kamble, Sagar A" <sagar.a.kamble@intel.com>]

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@intel.com escreveu:
>> From: Tom O'Rourke <Tom.O'Rourke@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx