[Intel-gfx] [RFC 00/22] Add support for GuC-based SLPC
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Thu Feb 11 20:10:26 UTC 2016
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 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
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list