Hi Vinay,

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Add macros to check for slpc support. This feature is currently supported for gen12+ and enabled whenever guc submission is enabled/selected.

please try to use consistent names across all patches:

s/slpc/SLPC s/gen12/Gen12 s/guc/GuC

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.c | 1 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 ++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 21 +++++++++++++++++++ .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 16 ++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 ++++-- drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 + 6 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 979128e28372..b9a809f2d221 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -157,6 +157,7 @@ void intel_guc_init_early(struct intel_guc *guc) intel_guc_ct_init_early(&guc->ct); intel_guc_log_init_early(&guc->log); intel_guc_submission_init_early(guc);

• intel_guc_slpc_init_early(guc);

  mutex_init(&guc->send_mutex); spin_lock_init(&guc->irq_lock);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 5d94cf482516..e5a456918b88 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -57,6 +57,8 @@ struct intel_guc {

bool submission_supported; bool submission_selected;

• bool slpc_supported;

• bool slpc_selected;

  struct i915_vma *ads_vma; struct __guc_ads_blob *ads_blob;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 9c102bf0c8e3..e2644a05f298 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2351,6 +2351,27 @@ void intel_guc_submission_init_early(struct intel_guc *guc) guc->submission_selected = __guc_submission_selected(guc); }

+static bool __guc_slpc_supported(struct intel_guc *guc)

hmm, easy to confuse with intel_guc_slpc_is_supported, so maybe:

__detect_slpc_supported()

(yes, I know you were following code above)

+{

• /* GuC slpc is unavailable for pre-Gen12 */

s/slpc/SLPC

• return guc->submission_supported &&

• GRAPHICS_VER(guc_to_gt(guc)->i915) >= 12;
  

+}

+static bool __guc_slpc_selected(struct intel_guc *guc) +{

• if (!intel_guc_slpc_is_supported(guc))

• return false;
  

• return guc->submission_selected;

+}

+void intel_guc_slpc_init_early(struct intel_guc *guc) +{

• guc->slpc_supported = __guc_slpc_supported(guc);
• guc->slpc_selected = __guc_slpc_selected(guc);

+}

in patch 4/16 you are introducing intel_guc_slpc.c|h so to have proper encapsulation better to define this function as

void intel_guc_slpc_init_early(struct intel_guc_slpc *slpc) { }

and move it to intel_guc_slpc.c

static inline struct intel_context * g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) { diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h index be767eb6ff71..7ae5fd052faf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h @@ -13,6 +13,7 @@ struct drm_printer; struct intel_engine_cs;

+void intel_guc_slpc_init_early(struct intel_guc *guc);

it really does not belong to this .h

void intel_guc_submission_init_early(struct intel_guc *guc); int intel_guc_submission_init(struct intel_guc *guc); void intel_guc_submission_enable(struct intel_guc *guc); @@ -50,4 +51,19 @@ static inline bool intel_guc_submission_is_used(struct intel_guc *guc) return intel_guc_is_used(guc) && intel_guc_submission_is_wanted(guc); }

+static inline bool intel_guc_slpc_is_supported(struct intel_guc *guc) +{

• return guc->slpc_supported;

+}

+static inline bool intel_guc_slpc_is_wanted(struct intel_guc *guc) +{

• return guc->slpc_selected;

+}

+static inline bool intel_guc_slpc_is_used(struct intel_guc *guc) +{

• return intel_guc_submission_is_used(guc) && intel_guc_slpc_is_wanted(guc);

+}

did you try to define them in intel_guc_slpc.h ?

note that to avoid circular dependencies you can define slpc struct in intel_guc_slpc_types.h and then

in intel_guc.h: #include "intel_guc_slpc_types.h" instead of intel_guc_slpc.h

in intel_guc_slpc.h: #include "intel_guc.h" #include "intel_guc_slpc_types.h" #include "intel_guc_submission.h"

#endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 61be0aa81492..dca5f6d0641b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -76,16 +76,18 @@ static void __confirm_options(struct intel_uc *uc) struct drm_i915_private *i915 = uc_to_gt(uc)->i915;

drm_dbg(&i915->drm,

• "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
  

• "enable_guc=%d (guc:%s submission:%s huc:%s slpc:%s)\n",
  

  i915->params.enable_guc, yesno(intel_uc_wants_guc(uc)), yesno(intel_uc_wants_guc_submission(uc)),

• yesno(intel_uc_wants_huc(uc)));
  

• yesno(intel_uc_wants_huc(uc)),
  

• yesno(intel_uc_wants_guc_slpc(uc)));
  
  

  if (i915->params.enable_guc == 0) { GEM_BUG_ON(intel_uc_wants_guc(uc)); GEM_BUG_ON(intel_uc_wants_guc_submission(uc)); GEM_BUG_ON(intel_uc_wants_huc(uc));

• GEM_BUG_ON(intel_uc_wants_guc_slpc(uc));
  

  return; }

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index e2da2b6e76e1..38e465fd8a0c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -83,6 +83,7 @@ __uc_state_checker(x, func, uses, used) uc_state_checkers(guc, guc); uc_state_checkers(huc, huc); uc_state_checkers(guc, guc_submission); +uc_state_checkers(guc, guc_slpc);

#undef uc_state_checkers #undef __uc_state_checker

On 7/10/2021 7:27 AM, Michal Wajdeczko wrote:

Hi Vinay,

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Add macros to check for slpc support. This feature is currently supported for gen12+ and enabled whenever guc submission is enabled/selected.

please try to use consistent names across all patches:

s/slpc/SLPC s/gen12/Gen12 s/guc/GuC

Ok.

...

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.c | 1 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 ++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 21 +++++++++++++++++++ .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 16 ++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 ++++-- drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 + 6 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 979128e28372..b9a809f2d221 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -157,6 +157,7 @@ void intel_guc_init_early(struct intel_guc *guc) intel_guc_ct_init_early(&guc->ct); intel_guc_log_init_early(&guc->log); intel_guc_submission_init_early(guc);

• intel_guc_slpc_init_early(guc);

  mutex_init(&guc->send_mutex); spin_lock_init(&guc->irq_lock);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 5d94cf482516..e5a456918b88 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -57,6 +57,8 @@ struct intel_guc {

bool submission_supported; bool submission_selected;

• bool slpc_supported;

• bool slpc_selected;

  struct i915_vma *ads_vma; struct __guc_ads_blob *ads_blob;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 9c102bf0c8e3..e2644a05f298 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2351,6 +2351,27 @@ void intel_guc_submission_init_early(struct intel_guc *guc) guc->submission_selected = __guc_submission_selected(guc); }

+static bool __guc_slpc_supported(struct intel_guc *guc)

hmm, easy to confuse with intel_guc_slpc_is_supported, so maybe:

__detect_slpc_supported()

(yes, I know you were following code above)

...

+{

• /* GuC slpc is unavailable for pre-Gen12 */

s/slpc/SLPC

Ok.

...

• return guc->submission_supported &&

• GRAPHICS_VER(guc_to_gt(guc)->i915) >= 12;
  

+}

+static bool __guc_slpc_selected(struct intel_guc *guc) +{

• if (!intel_guc_slpc_is_supported(guc))

• return false;
  

• return guc->submission_selected;

+}

+void intel_guc_slpc_init_early(struct intel_guc *guc) +{

• guc->slpc_supported = __guc_slpc_supported(guc);
• guc->slpc_selected = __guc_slpc_selected(guc);

+}

in patch 4/16 you are introducing intel_guc_slpc.c|h so to have proper encapsulation better to define this function as

void intel_guc_slpc_init_early(struct intel_guc_slpc *slpc) { }

and move it to intel_guc_slpc.c

done.

...

• static inline struct intel_context * g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) {

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h index be767eb6ff71..7ae5fd052faf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h @@ -13,6 +13,7 @@ struct drm_printer; struct intel_engine_cs;

+void intel_guc_slpc_init_early(struct intel_guc *guc);

it really does not belong to this .h

...

void intel_guc_submission_init_early(struct intel_guc *guc); int intel_guc_submission_init(struct intel_guc *guc); void intel_guc_submission_enable(struct intel_guc *guc); @@ -50,4 +51,19 @@ static inline bool intel_guc_submission_is_used(struct intel_guc *guc) return intel_guc_is_used(guc) && intel_guc_submission_is_wanted(guc); }

+static inline bool intel_guc_slpc_is_supported(struct intel_guc *guc) +{

• return guc->slpc_supported;

+}

+static inline bool intel_guc_slpc_is_wanted(struct intel_guc *guc) +{

• return guc->slpc_selected;

+}

+static inline bool intel_guc_slpc_is_used(struct intel_guc *guc) +{

• return intel_guc_submission_is_used(guc) && intel_guc_slpc_is_wanted(guc);

+}

did you try to define them in intel_guc_slpc.h ?

note that to avoid circular dependencies you can define slpc struct in intel_guc_slpc_types.h and then

in intel_guc.h: #include "intel_guc_slpc_types.h" instead of intel_guc_slpc.h

in intel_guc_slpc.h: #include "intel_guc.h" #include "intel_guc_slpc_types.h" #include "intel_guc_submission.h"

that worked.

Thanks, Vinay.

...

• #endif

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 61be0aa81492..dca5f6d0641b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -76,16 +76,18 @@ static void __confirm_options(struct intel_uc *uc) struct drm_i915_private *i915 = uc_to_gt(uc)->i915;

drm_dbg(&i915->drm,

• "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
  

• "enable_guc=%d (guc:%s submission:%s huc:%s slpc:%s)\n",
  

  i915->params.enable_guc, yesno(intel_uc_wants_guc(uc)), yesno(intel_uc_wants_guc_submission(uc)),

• yesno(intel_uc_wants_huc(uc)));
  

• yesno(intel_uc_wants_huc(uc)),
  

• yesno(intel_uc_wants_guc_slpc(uc)));
  
  

  if (i915->params.enable_guc == 0) { GEM_BUG_ON(intel_uc_wants_guc(uc)); GEM_BUG_ON(intel_uc_wants_guc_submission(uc)); GEM_BUG_ON(intel_uc_wants_huc(uc));

• GEM_BUG_ON(intel_uc_wants_guc_slpc(uc));
  

  return; }

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index e2da2b6e76e1..38e465fd8a0c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -83,6 +83,7 @@ __uc_state_checker(x, func, uses, used) uc_state_checkers(guc, guc); uc_state_checkers(huc, huc); uc_state_checkers(guc, guc_submission); +uc_state_checkers(guc, guc_slpc);

#undef uc_state_checkers #undef __uc_state_checker

On 7/10/2021 7:35 AM, Michal Wajdeczko wrote:

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Declare header and source files for SLPC, along with init and enable/disable function templates.

later you claim that "disable" is not needed

Changed.

...

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 34 +++++++++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 16 ++++++++++ 4 files changed, 53 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index ab7679957623..d8eac4468df9 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -186,6 +186,7 @@ i915-y += gt/uc/intel_uc.o \ gt/uc/intel_guc_fw.o \ gt/uc/intel_guc_log.o \ gt/uc/intel_guc_log_debugfs.o \

•  gt/uc/intel_guc_slpc.o \
  

  gt/uc/intel_guc_submission.o \ gt/uc/intel_huc.o \ gt/uc/intel_huc_debugfs.o \

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index e5a456918b88..0dbbd9cf553f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -15,6 +15,7 @@ #include "intel_guc_ct.h" #include "intel_guc_log.h" #include "intel_guc_reg.h" +#include "intel_guc_slpc.h" #include "intel_uc_fw.h" #include "i915_utils.h" #include "i915_vma.h" @@ -30,6 +31,7 @@ struct intel_guc { struct intel_uc_fw fw; struct intel_guc_log log; struct intel_guc_ct ct;

• struct intel_guc_slpc slpc;

  /* Global engine used to submit requests to GuC */ struct i915_sched_engine *sched_engine;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c new file mode 100644 index 000000000000..c1f569d2300d --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -0,0 +1,34 @@ +/*

• • SPDX-License-Identifier: MIT

SPDX tag shall be in very first line, for .c:

// SPDX-License-Identifier: MIT

...

• • Copyright © 2020 Intel Corporation

2021

done.

...

• */

+#include "intel_guc_slpc.h"

+int intel_guc_slpc_init(struct intel_guc_slpc *slpc) +{

• return 0;

+}

+/*

• • intel_guc_slpc_enable() - Start SLPC
• • @slpc: pointer to intel_guc_slpc.
• • SLPC is enabled by setting up the shared data structure and
• • sending reset event to GuC SLPC. Initial data is setup in
• • intel_guc_slpc_init. Here we send the reset event. We do
• • not currently need a slpc_disable since this is taken care
• • of automatically when a reset/suspend occurs and the guc

s/guc/GuC

...

• • channels are destroyed.

you mean CTB ?

yes, fixed.

...

• • Return: 0 on success, non-zero error code on failure.
• */

+int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) +{

• return 0;

+}

+void intel_guc_slpc_fini(struct intel_guc_slpc *slpc) +{ +} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h new file mode 100644 index 000000000000..74fd86769163 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -0,0 +1,16 @@ +/*

• • SPDX-License-Identifier: MIT

SPDX tag shall be in very first line, for .h:

/* SPDX-License-Identifier: MIT */

...

• • Copyright © 2020 Intel Corporation

2021

...

• */

+#ifndef _INTEL_GUC_SLPC_H_ +#define _INTEL_GUC_SLPC_H_

+struct intel_guc_slpc { +};

move all data definitions to intel_guc_slpc_types.h and include it here

...

+int intel_guc_slpc_init(struct intel_guc_slpc *slpc); +int intel_guc_slpc_enable(struct intel_guc_slpc *slpc); +void intel_guc_slpc_fini(struct intel_guc_slpc *slpc);

+#endif

and as suggested in comment to 2/14 you should likely move this patch to the front of the series

Yes, squashed with the first patch.

Thanks, Vinay.

Michal

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Replicate the SLPC header file in GuC for the most part. There are

what you mean by "replicate" here?

some SLPC mode based parameters which haven't been included since we are not using them.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.c | 4 + drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 2 + .../gpu/drm/i915/gt/uc/intel_guc_slpc_fwif.h | 255 ++++++++++++++++++ 4 files changed, 263 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_fwif.h

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index b9a809f2d221..9d61b2d54de4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -202,11 +202,15 @@ static u32 guc_ctl_debug_flags(struct intel_guc *guc)

static u32 guc_ctl_feature_flags(struct intel_guc *guc) {

• struct intel_gt *gt = guc_to_gt(guc); u32 flags = 0;

  if (!intel_guc_submission_is_used(guc)) flags |= GUC_CTL_DISABLE_SCHEDULER;

• if (intel_uc_uses_guc_slpc(&gt->uc))

• flags |= GUC_CTL_ENABLE_SLPC;
  

• return flags;

}

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index 94bb1ca6f889..19e2504d7a36 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -114,6 +114,8 @@ #define GUC_ADS_ADDR_SHIFT 1 #define GUC_ADS_ADDR_MASK (0xFFFFF << GUC_ADS_ADDR_SHIFT)

+#define GUC_CTL_ENABLE_SLPC BIT(2)

this should be defined closer to GUC_CTL_FEATURE

#define GUC_CTL_MAX_DWORDS (SOFT_SCRATCH_COUNT - 2) /* [1..14] */

/* Generic GT SysInfo data types */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index 74fd86769163..98036459a1a3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -6,6 +6,8 @@ #ifndef _INTEL_GUC_SLPC_H_ #define _INTEL_GUC_SLPC_H_

+#include "intel_guc_slpc_fwif.h"

doesn't seem to be needed right now

struct intel_guc_slpc { };

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_fwif.h new file mode 100644 index 000000000000..2a5e71428374 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_fwif.h

I've started to move all pure ABI definitions to files in abi/ folder, leaving in guc_fwif.h only our next level helpers/wrappers.

Can you move these SLPC definition there too ? maybe as dedicated:

abi/guc_slpc_abi.h

@@ -0,0 +1,255 @@ +/*

• • SPDX-License-Identifier: MIT

use proper format

• • Copyright © 2020 Intel Corporation

2021

• */

+#ifndef _INTEL_GUC_SLPC_FWIF_H_ +#define _INTEL_GUC_SLPC_FWIF_H_

+#include <linux/types.h>

+/* This file replicates the header in GuC code for handling SLPC related

• • data structures and sizes
• */

use proper format for multi-line comments:

/* * blah blah * blah blah */

+/* SLPC exposes certain parameters for global configuration by the host.

• • These are referred to as override parameters, because in most cases
• • the host will not need to modify the default values used by SLPC.
• • SLPC remembers the default values which allows the host to easily restore
• • them by simply unsetting the override. The host can set or unset override
• • parameters during SLPC (re-)initialization using the SLPC Reset event.
• • The host can also set or unset override parameters on the fly using the
• • Parameter Set and Parameter Unset events
• */

+#define SLPC_MAX_OVERRIDE_PARAMETERS 256 +#define SLPC_OVERRIDE_BITFIELD_SIZE \

• (SLPC_MAX_OVERRIDE_PARAMETERS / 32)
  

+#define SLPC_PAGE_SIZE_BYTES 4096 +#define SLPC_CACHELINE_SIZE_BYTES 64 +#define SLPC_SHARE_DATA_SIZE_BYTE_HEADER SLPC_CACHELINE_SIZE_BYTES +#define SLPC_SHARE_DATA_SIZE_BYTE_PLATFORM_INFO SLPC_CACHELINE_SIZE_BYTES +#define SLPC_SHARE_DATA_SIZE_BYTE_TASK_STATE SLPC_CACHELINE_SIZE_BYTES +#define SLPC_SHARE_DATA_MODE_DEFN_TABLE_SIZE SLPC_PAGE_SIZE_BYTES

can you put some simply diagram that would describe this layout ?

+#define SLPC_SHARE_DATA_SIZE_BYTE_MAX (2 * SLPC_PAGE_SIZE_BYTES)

+/* Cacheline size aligned (Total size needed for

• • SLPM_KMD_MAX_OVERRIDE_PARAMETERS=256 is 1088 bytes)
• */

+#define SLPC_SHARE_DATA_SIZE_BYTE_PARAM (((((SLPC_MAX_OVERRIDE_PARAMETERS * 4) \

• 				+ ((SLPC_MAX_OVERRIDE_PARAMETERS / 32) * 4)) \
  

• + (SLPC_CACHELINE_SIZE_BYTES-1)) / SLPC_CACHELINE_SIZE_BYTES)*SLPC_CACHELINE_SIZE_BYTES)
  

+#define SLPC_SHARE_DATA_SIZE_BYTE_OTHER (SLPC_SHARE_DATA_SIZE_BYTE_MAX - \

• 			(SLPC_SHARE_DATA_SIZE_BYTE_HEADER \
  

• 			+ SLPC_SHARE_DATA_SIZE_BYTE_PLATFORM_INFO \
  

• 			+ SLPC_SHARE_DATA_SIZE_BYTE_TASK_STATE \
  

• 			+ SLPC_SHARE_DATA_SIZE_BYTE_PARAM \
  

• 			+ SLPC_SHARE_DATA_MODE_DEFN_TABLE_SIZE))
  

+#define SLPC_EVENT(id, argc) ((u32)(id) << 8 | (argc))

+#define SLPC_PARAM_TASK_DEFAULT 0 +#define SLPC_PARAM_TASK_ENABLED 1 +#define SLPC_PARAM_TASK_DISABLED 2 +#define SLPC_PARAM_TASK_UNKNOWN 3

many values below are defined as enum, why these values are #defines ?

and is there any relation to these ones defined below (look similar)?

+ SLPC_PARAM_TASK_ENABLE_GTPERF = 0, + SLPC_PARAM_TASK_DISABLE_GTPERF = 1, + SLPC_PARAM_TASK_ENABLE_BALANCER = 2, + SLPC_PARAM_TASK_DISABLE_BALANCER = 3, + SLPC_PARAM_TASK_ENABLE_DCC = 4, + SLPC_PARAM_TASK_DISABLE_DCC = 5,

+enum slpc_status {

• SLPC_STATUS_OK = 0,
• SLPC_STATUS_ERROR = 1,
• SLPC_STATUS_ILLEGAL_COMMAND = 2,
• SLPC_STATUS_INVALID_ARGS = 3,
• SLPC_STATUS_INVALID_PARAMS = 4,
• SLPC_STATUS_INVALID_DATA = 5,
• SLPC_STATUS_OUT_OF_RANGE = 6,
• SLPC_STATUS_NOT_SUPPORTED = 7,
• SLPC_STATUS_NOT_IMPLEMENTED = 8,
• SLPC_STATUS_NO_DATA = 9,
• SLPC_STATUS_EVENT_NOT_REGISTERED = 10,
• SLPC_STATUS_REGISTER_LOCKED = 11,
• SLPC_STATUS_TEMPORARILY_UNAVAILABLE = 12,
• SLPC_STATUS_VALUE_ALREADY_SET = 13,
• SLPC_STATUS_VALUE_ALREADY_UNSET = 14,
• SLPC_STATUS_VALUE_NOT_CHANGED = 15,
• SLPC_STATUS_MEMIO_ERROR = 16,
• SLPC_STATUS_EVENT_QUEUED_REQ_DPC = 17,
• SLPC_STATUS_EVENT_QUEUED_NOREQ_DPC = 18,
• SLPC_STATUS_NO_EVENT_QUEUED = 19,
• SLPC_STATUS_OUT_OF_SPACE = 20,
• SLPC_STATUS_TIMEOUT = 21,
• SLPC_STATUS_NO_LOCK = 22,
• SLPC_STATUS_MAX

+};

+enum slpc_event_id {

• SLPC_EVENT_RESET = 0,
• SLPC_EVENT_SHUTDOWN = 1,
• SLPC_EVENT_PLATFORM_INFO_CHANGE = 2,
• SLPC_EVENT_DISPLAY_MODE_CHANGE = 3,
• SLPC_EVENT_FLIP_COMPLETE = 4,
• SLPC_EVENT_QUERY_TASK_STATE = 5,
• SLPC_EVENT_PARAMETER_SET = 6,
• SLPC_EVENT_PARAMETER_UNSET = 7,

+};

+enum slpc_param_id {

• SLPC_PARAM_TASK_ENABLE_GTPERF = 0,
• SLPC_PARAM_TASK_DISABLE_GTPERF = 1,
• SLPC_PARAM_TASK_ENABLE_BALANCER = 2,
• SLPC_PARAM_TASK_DISABLE_BALANCER = 3,
• SLPC_PARAM_TASK_ENABLE_DCC = 4,
• SLPC_PARAM_TASK_DISABLE_DCC = 5,
• SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ = 6,
• SLPC_PARAM_GLOBAL_MAX_GT_UNSLICE_FREQ_MHZ = 7,
• SLPC_PARAM_GLOBAL_MIN_GT_SLICE_FREQ_MHZ = 8,
• SLPC_PARAM_GLOBAL_MAX_GT_SLICE_FREQ_MHZ = 9,
• SLPC_PARAM_GTPERF_THRESHOLD_MAX_FPS = 10,
• SLPC_PARAM_GLOBAL_DISABLE_GT_FREQ_MANAGEMENT = 11,
• SLPC_PARAM_GTPERF_ENABLE_FRAMERATE_STALLING = 12,
• SLPC_PARAM_GLOBAL_DISABLE_RC6_MODE_CHANGE = 13,
• SLPC_PARAM_GLOBAL_OC_UNSLICE_FREQ_MHZ = 14,
• SLPC_PARAM_GLOBAL_OC_SLICE_FREQ_MHZ = 15,
• SLPC_PARAM_GLOBAL_ENABLE_IA_GT_BALANCING = 16,
• SLPC_PARAM_GLOBAL_ENABLE_ADAPTIVE_BURST_TURBO = 17,
• SLPC_PARAM_GLOBAL_ENABLE_EVAL_MODE = 18,
• SLPC_PARAM_GLOBAL_ENABLE_BALANCER_IN_NON_GAMING_MODE = 19,
• SLPC_PARAM_GLOBAL_RT_MODE_TURBO_FREQ_DELTA_MHZ = 20,
• SLPC_PARAM_PWRGATE_RC_MODE = 21,
• SLPC_PARAM_EDR_MODE_COMPUTE_TIMEOUT_MS = 22,
• SLPC_PARAM_EDR_QOS_FREQ_MHZ = 23,
• SLPC_PARAM_MEDIA_FF_RATIO_MODE = 24,
• SLPC_PARAM_ENABLE_IA_FREQ_LIMITING = 25,
• SLPC_PARAM_STRATEGIES = 26,
• SLPC_PARAM_POWER_PROFILE = 27,
• SLPC_IGNORE_EFFICIENT_FREQUENCY = 28,

no PARAM tag inside this enum name

• SLPC_MAX_PARAM = 32,

can we move this out of enum, maybe as standalone #define ? or remove it as doesn't seem to be useful at all

+};

+enum slpc_global_state {

• SLPC_GLOBAL_STATE_NOT_RUNNING = 0,
• SLPC_GLOBAL_STATE_INITIALIZING = 1,
• SLPC_GLOBAL_STATE_RESETTING = 2,
• SLPC_GLOBAL_STATE_RUNNING = 3,
• SLPC_GLOBAL_STATE_SHUTTING_DOWN = 4,
• SLPC_GLOBAL_STATE_ERROR = 5

+};

+enum slpc_platform_sku {

• SLPC_PLATFORM_SKU_UNDEFINED = 0,
• SLPC_PLATFORM_SKU_ULX = 1,
• SLPC_PLATFORM_SKU_ULT = 2,
• SLPC_PLATFORM_SKU_T = 3,
• SLPC_PLATFORM_SKU_MOBL = 4,
• SLPC_PLATFORM_SKU_DT = 5,
• SLPC_PLATFORM_SKU_UNKNOWN = 6,

+};

+struct slpc_platform_info {

• union {

• u32 sku;  /**< SKU info */
  

• struct {
  

• 	u32 reserved:8;
  

• 	u32 fused_slice_count:8;
  

• 	u32 reserved1:16;
  

• };
  

• };

•    union
  

• {

• u32 bitfield2;       /**< IA capability info*/
  

• struct {
  

• 	u32 max_p0_freq_bins:8;
  

• 	u32 p1_freq_bins:8;
  

• 	u32 pe_freq_bins:8;
  

• 	u32 pn_freq_bins:8;
  

• };
  

• };
• u32 reserved2[2];

+} __packed;

I'm not a big fan of using C bitfields for interface definitions

can we switch to regular #defines and use FIELD_GET|PREP ?

+struct slpc_task_state_data {

• union {

• u32 bitfield1;
  

• struct {
  

• 	u32 gtperf_task_active:1;
  

• 	u32 gtperf_stall_possible:1;
  

• 	u32 gtperf_gaming_mode:1;
  

• 	u32 gtperf_target_fps:8;
  

• 	u32 dcc_task_active:1;
  

• 	u32 in_dcc:1;
  

• 	u32 in_dct:1;
  

• 	u32 freq_switch_active:1;
  

• 	u32 ibc_enabled:1;
  

• 	u32 ibc_active:1;
  

• 	u32 pg1_enabled:1;
  

• 	u32 pg1_active:1;
  

• };
  

• };
• union {

• u32 bitfield2;
  

• struct {
  

• 	u32 max_unslice_freq:8;
  

• 	u32 min_unslice_freq:8;
  

• 	u32 max_slice_freq:8;
  

• 	u32 min_slice_freq:8;
  

• };
  

• };

+} __packed;

+struct slpc_shared_data {

• union {

• struct {
  

• 	/* Total size in bytes of this buffer. */
  

• 	u32 shared_data_size;
  

• 	u32 global_state;
  

• 	u32 display_data_addr;
  

• };
  

below all structs are named, this one not, why ?

• unsigned char reserved_header[SLPC_SHARE_DATA_SIZE_BYTE_HEADER];
  

this could be just "u8"

and I assume all these "reserved" are in fact padding, no ?

• };
• union {

• struct slpc_platform_info platform_info;
  

• unsigned char reserved_platform[SLPC_SHARE_DATA_SIZE_BYTE_PLATFORM_INFO];
  

• };

maybe we can avoid these unions by declaring padding explicitly:

struct slpc_platform_info platform_info; u8 platform_info_pad[SLPC_SHARE_DATA_SIZE_BYTE_PLATFORM_INFO - sizeof(struct slpc_platform_info)];

• union {

• struct slpc_task_state_data task_state_data;
  

• unsigned char reserved_task_state[SLPC_SHARE_DATA_SIZE_BYTE_TASK_STATE];
  

• };
• union {

• struct {
  

• u32 override_params_set_bits[SLPC_OVERRIDE_BITFIELD_SIZE];
  

• u32 override_params_values[SLPC_MAX_OVERRIDE_PARAMETERS];
  

• };
  

• unsigned char reserved_override_parameter[SLPC_SHARE_DATA_SIZE_BYTE_PARAM];
  

• };
• unsigned char reserved_other[SLPC_SHARE_DATA_SIZE_BYTE_OTHER];
• /* PAGE 2 (4096 bytes), mode based parameter will be removed soon */
• unsigned char reserved_mode_definition[4096];

+} __packed;

+enum slpc_reset_flags {

• SLPC_RESET_FLAG_TDR_OCCURRED = (1 << 0)

+};

+#define SLPC_EVENT_MAX_INPUT_ARGS 9 +#define SLPC_EVENT_MAX_OUTPUT_ARGS 1

+union slpc_event_input_header {

• u32 value;
• struct {

• u32 num_args:8;
  

• u32 event_id:8;
  

• };

+};

I guess earlier #define SLPC_EVENT is related to above can we keep related definitions together ?

+struct slpc_event_input {

• u32 h2g_action_id;
• union slpc_event_input_header header;
• u32 args[SLPC_EVENT_MAX_INPUT_ARGS];

+} __packed;

this looks like a attempt to define details of the INTEL_GUC_ACTION_SLPC_REQUEST HXG request message.

so maybe it can be moved to abi/guc_actions_slpc_abi.h ? best if you can define it in the same fashion as CTB registration one

Michal

+#endif

On 7/10/2021 9:05 AM, Michal Wajdeczko wrote:

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Allocate data structures for SLPC and functions for initializing on host side.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.c | 11 +++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 36 ++++++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 20 ++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 9d61b2d54de4..82863a9bc8e8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -336,6 +336,12 @@ int intel_guc_init(struct intel_guc *guc) goto err_ct; }

• if (intel_guc_slpc_is_used(guc)) {

• ret = intel_guc_slpc_init(&guc->slpc);
  

• if (ret)
  

• 	goto err_submission;
  

• }
• /* now that everything is perma-pinned, initialize the parameters */ guc_init_params(guc);

@@ -346,6 +352,8 @@ int intel_guc_init(struct intel_guc *guc)

return 0;

+err_submission:

• intel_guc_submission_fini(guc); err_ct: intel_guc_ct_fini(&guc->ct); err_ads:

@@ -368,6 +376,9 @@ void intel_guc_fini(struct intel_guc *guc)

i915_ggtt_disable_guc(gt->ggtt);

• if (intel_guc_slpc_is_used(guc))

• intel_guc_slpc_fini(&guc->slpc);
  

• if (intel_guc_submission_is_used(guc)) intel_guc_submission_fini(guc);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index c1f569d2300d..94e2f19951aa 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -4,11 +4,41 @@

• Copyright © 2020 Intel Corporation

*/

+#include <asm/msr-index.h>

hmm, what exactly is needed from this header ?

Was being used in a previous version for MSR reads, removed.

...

+#include "gt/intel_gt.h" +#include "gt/intel_rps.h"

+#include "i915_drv.h" #include "intel_guc_slpc.h" +#include "intel_pm.h"

+static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc) +{

• return container_of(slpc, struct intel_guc, slpc);

+}

+static int slpc_shared_data_init(struct intel_guc_slpc *slpc) +{

• struct intel_guc *guc = slpc_to_guc(slpc);
• int err;
• u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));

move err decl here

...

• err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, &slpc->vaddr);
• if (unlikely(err)) {

• DRM_ERROR("Failed to allocate slpc struct (err=%d)\n", err);
  

s/slpc/SLPC

and use drm_err instead and you may also want to print error as %pe

added.

...

• i915_vma_unpin_and_release(&slpc->vma, I915_VMA_RELEASE_MAP);
  

do you really need this ?

removed.

...

• return err;
  

• }
• return err;

+}

int intel_guc_slpc_init(struct intel_guc_slpc *slpc) {

• return 0;

• GEM_BUG_ON(slpc->vma);

• return slpc_shared_data_init(slpc); }

  /*

@@ -31,4 +61,8 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)

void intel_guc_slpc_fini(struct intel_guc_slpc *slpc) {

• if (!slpc->vma)

• return;
  

• i915_vma_unpin_and_release(&slpc->vma, I915_VMA_RELEASE_MAP); }

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index 98036459a1a3..a2643b904165 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -3,12 +3,32 @@

• Copyright © 2020 Intel Corporation

*/

should be fixed in earlier patch

...

#ifndef _INTEL_GUC_SLPC_H_ #define _INTEL_GUC_SLPC_H_

+#include <linux/mutex.h> #include "intel_guc_slpc_fwif.h"

struct intel_guc_slpc {

• /*Protects access to vma and SLPC actions */

hmm, missing mutex ;)

Removed.

...

• struct i915_vma *vma;
• void *vaddr;

no need to be void, define it as ptr to slpc_shared_data

...

• /* platform frequency limits */
• u32 min_freq;
• u32 rp0_freq;
• u32 rp1_freq;
• /* frequency softlimits */
• u32 min_freq_softlimit;
• u32 max_freq_softlimit;
• struct {

• u32 param_id;
  

• u32 param_value;
  

• u32 param_override;
  

• } debug;

can you add all these extra fields in patches which will need them?

Michal

Done.

Thanks, Vinay.

...

};

int intel_guc_slpc_init(struct intel_guc_slpc *slpc);

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Add methods for interacting with guc for enabling SLPC. Enable SLPC after guc submission has been established. GuC load will

s/guc/GuC

fail if SLPC cannot be successfully initialized. Add various helper methods to set/unset the parameters for SLPC. They can be set using h2g calls or directly setting bits in the shared

/h2g/H2G

data structure.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 221 ++++++++++++++++++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 - drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 + 3 files changed, 231 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 94e2f19951aa..e579408d1c19 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -18,6 +18,61 @@ static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc) return container_of(slpc, struct intel_guc, slpc); }

+static inline struct intel_gt *slpc_to_gt(struct intel_guc_slpc *slpc) +{

• return guc_to_gt(slpc_to_guc(slpc));

+}

+static inline struct drm_i915_private *slpc_to_i915(struct intel_guc_slpc *slpc) +{

• return (slpc_to_gt(slpc))->i915;

+}

+static void slpc_mem_set_param(struct slpc_shared_data *data,

• 		u32 id, u32 value)
  

+{

• GEM_BUG_ON(id >= SLPC_MAX_OVERRIDE_PARAMETERS);
• /* When the flag bit is set, corresponding value will be read

• * and applied by slpc.
  

fix format of multi-line comment s/slpc/SLPC

• */
  

• data->override_params_set_bits[id >> 5] |= (1 << (id % 32));

use __set_bit instead

• data->override_params_values[id] = value;

+}

+static void slpc_mem_unset_param(struct slpc_shared_data *data,

• 		 u32 id)
  

+{

• GEM_BUG_ON(id >= SLPC_MAX_OVERRIDE_PARAMETERS);
• /* When the flag bit is unset, corresponding value will not be

• * read by slpc.
  

• */
  

• data->override_params_set_bits[id >> 5] &= (~(1 << (id % 32)));

same here

• data->override_params_values[id] = 0;

+}

+static void slpc_mem_task_control(struct slpc_shared_data *data,

• 		 u64 val, u32 enable_id, u32 disable_id)
  

hmm, u64 to pass simple tri-state flag ?

+{

• /* Enabling a param involves setting the enable_id

• * to 1 and disable_id to 0. Setting it to default
  

• * will unset both enable and disable ids and let
  

• * slpc choose it's default values.
  

fix format + s/slpc/SLPC

• */
  

• if (val == SLPC_PARAM_TASK_DEFAULT) {

• /* set default */
  

• slpc_mem_unset_param(data, enable_id);
  

• slpc_mem_unset_param(data, disable_id);
  

• } else if (val == SLPC_PARAM_TASK_ENABLED) {

• /* set enable */
  

• slpc_mem_set_param(data, enable_id, 1);
  

• slpc_mem_set_param(data, disable_id, 0);
  

• } else if (val == SLPC_PARAM_TASK_DISABLED) {

• /* set disable */
  

• slpc_mem_set_param(data, disable_id, 1);
  

• slpc_mem_set_param(data, enable_id, 0);
  

• }

maybe instead of SLPC_PARAM_TASK_* flags (that btw were confusing me earlier) you can define 3x small helpers:

static void slpc_mem_set_default(data, enable_id, disable_id); static void slpc_mem_set_enabled(data, enable_id, disable_id); static void slpc_mem_set_disabled(data, enable_id, disable_id);

+}

static int slpc_shared_data_init(struct intel_guc_slpc *slpc) { struct intel_guc *guc = slpc_to_guc(slpc); @@ -34,6 +89,128 @@ static int slpc_shared_data_init(struct intel_guc_slpc *slpc) return err; }

+/*

• • Send SLPC event to guc
• */

+static int slpc_send(struct intel_guc_slpc *slpc,

• 	struct slpc_event_input *input,
  

• 	u32 in_len)
  

+{

• struct intel_guc *guc = slpc_to_guc(slpc);
• u32 *action;
• action = (u32 *)input;
• action[0] = INTEL_GUC_ACTION_SLPC_REQUEST;

why not just updating input->h2g_action_id ?

• return intel_guc_send(guc, action, in_len);

+}

+static bool slpc_running(struct intel_guc_slpc *slpc) +{

• struct slpc_shared_data *data;
• u32 slpc_global_state;
• GEM_BUG_ON(!slpc->vma);
• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));

do you really need to flush all 8K of shared data? it looks that you only need single u32

• data = slpc->vaddr;
• slpc_global_state = data->global_state;
• return (data->global_state == SLPC_GLOBAL_STATE_RUNNING);

+}

+static int host2guc_slpc_query_task_state(struct intel_guc_slpc *slpc) +{

• struct intel_guc *guc = slpc_to_guc(slpc);
• u32 shared_data_gtt_offset = intel_guc_ggtt_offset(guc, slpc->vma);
• struct slpc_event_input data = {0};
• data.header.value = SLPC_EVENT(SLPC_EVENT_QUERY_TASK_STATE, 2);

you defined header.num_args and header.event_id, don't want to use them?

• data.args[0] = shared_data_gtt_offset;
• data.args[1] = 0;
• return slpc_send(slpc, &data, 4);

magic 4

+}

+static int slpc_read_task_state(struct intel_guc_slpc *slpc) +{

• return host2guc_slpc_query_task_state(slpc);

+}

hmm, all this looks complicated more than needed, why not just:

static int guc_action_slpc_query(struct intel_guc *guc, u32 offset) { u32 request[] = { INTEL_GUC_ACTION_SLPC_REQUEST, SLPC_EVENT(SLPC_EVENT_QUERY_TASK_STATE, 2), offset, 0, }; int err;

return intel_guc_send(guc, request, ARRAY_SIZE(request)); }

static int slpc_query_task_state(struct intel_guc_slpc *slpc) { struct intel_guc *guc = slpc_to_guc(slpc); u32 offset = intel_guc_ggtt_offset(guc, slpc->vma);

return guc_action_slpc_query(guc, offset); }

btw, there is little magic in H2G data, as only event enums were defined in slpc_fwif.h (or slpc_abi.h) but it looks that len and format of args depends on the actual event used

+static const char *slpc_state_stringify(enum slpc_global_state state) +{

• const char *str = NULL;
• switch (state) {
• case SLPC_GLOBAL_STATE_NOT_RUNNING:

• str = "not running";
  

• break;
  

• case SLPC_GLOBAL_STATE_INITIALIZING:

• str = "initializing";
  

• break;
  

• case SLPC_GLOBAL_STATE_RESETTING:

• str = "resetting";
  

• break;
  

• case SLPC_GLOBAL_STATE_RUNNING:

• str = "running";
  

• break;
  

• case SLPC_GLOBAL_STATE_SHUTTING_DOWN:

• str = "shutting down";
  

• break;
  

• case SLPC_GLOBAL_STATE_ERROR:

• str = "error";
  

• break;
  

• default:

• str = "unknown";
  

• break;
  

• }
• return str;

+}

+static const char *get_slpc_state(struct intel_guc_slpc *slpc)

lot of duplicated code with slpc_running()

maybe there should be: u32 slpc_get_state(slpc); bool slpc_is_running(slpc); const char *slpc_state_string(slpc);

+{

• struct slpc_shared_data *data;
• u32 slpc_global_state;
• GEM_BUG_ON(!slpc->vma);
• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));
• data = slpc->vaddr;
• slpc_global_state = data->global_state;
• return slpc_state_stringify(slpc_global_state);

+}

+static int host2guc_slpc_reset(struct intel_guc_slpc *slpc) +{

• struct intel_guc *guc = slpc_to_guc(slpc);
• u32 shared_data_gtt_offset = intel_guc_ggtt_offset(guc, slpc->vma);
• struct slpc_event_input data = {0};
• int ret;
• data.header.value = SLPC_EVENT(SLPC_EVENT_RESET, 2);
• data.args[0] = shared_data_gtt_offset;
• data.args[1] = 0;
• /* TODO: Hardcoded 4 needs define */
• ret = slpc_send(slpc, &data, 4);
• if (!ret) {

• /* TODO: How long to Wait until SLPC is running */
  

do we know state transitions ? maybe there is no point in waiting for RUNNING if it is in ERROR or SHUTTING_DOWN ?

• if (wait_for(slpc_running(slpc), 5)) {
  

magic 5

• 	DRM_ERROR("SLPC not enabled! State = %s\n",
  

use drm_err

• 		  get_slpc_state(slpc));
  

• 	return -EIO;
  

• }
  

• }
• return ret;

+}

int intel_guc_slpc_init(struct intel_guc_slpc *slpc) { GEM_BUG_ON(slpc->vma); @@ -56,6 +233,50 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc) */ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) {

• struct drm_i915_private *i915 = slpc_to_i915(slpc);
• struct slpc_shared_data *data;
• int ret;
• GEM_BUG_ON(!slpc->vma);
• memset(slpc->vaddr, 0, sizeof(struct slpc_shared_data));
• data = slpc->vaddr;
• data->shared_data_size = sizeof(struct slpc_shared_data);
• /* Enable only GTPERF task, Disable others */
• slpc_mem_task_control(data, SLPC_PARAM_TASK_ENABLED,

• 		SLPC_PARAM_TASK_ENABLE_GTPERF,
  

• 		SLPC_PARAM_TASK_DISABLE_GTPERF);
  

• slpc_mem_task_control(data, SLPC_PARAM_TASK_DISABLED,

• 		SLPC_PARAM_TASK_ENABLE_BALANCER,
  

• 		SLPC_PARAM_TASK_DISABLE_BALANCER);
  

• slpc_mem_task_control(data, SLPC_PARAM_TASK_DISABLED,

• 		SLPC_PARAM_TASK_ENABLE_DCC,
  

• 		SLPC_PARAM_TASK_DISABLE_DCC);
  

• ret = host2guc_slpc_reset(slpc);
• if (ret) {

• drm_err(&i915->drm, "SLPC Reset event returned %d", ret);
  

you may want to print error as %pe missing \n

• return -EIO;
  

• }
• DRM_INFO("SLPC state: %s\n", get_slpc_state(slpc));

use drm_info

• if (slpc_read_task_state(slpc))

• drm_err(&i915->drm, "Unable to read task state data");
  

missing \n

• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));
• /* min and max frequency limits being used by SLPC */
• drm_info(&i915->drm, "SLPC min freq: %u Mhz, max is %u Mhz",

missing \n

• 	DIV_ROUND_CLOSEST(data->task_state_data.min_unslice_freq *
  

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER),
  

• 	DIV_ROUND_CLOSEST(data->task_state_data.max_unslice_freq *
  

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER));
  

this info/code seems to be duplicated in patch 10/16 maybe just call intel_guc_slpc_info() here once available ?

• return 0;

}

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index e2644a05f298..3e76d4d5f7bb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2321,10 +2321,6 @@ void intel_guc_submission_enable(struct intel_guc *guc)

void intel_guc_submission_disable(struct intel_guc *guc) {

• struct intel_gt *gt = guc_to_gt(guc);
• GEM_BUG_ON(gt->awake); /* GT should be parked first */

if not mistake, can you explain why it was removed ?

• /* Note: By the time we're here, GuC may have already been reset */

}

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index dca5f6d0641b..7b6c767d3eb0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -501,6 +501,14 @@ static int __uc_init_hw(struct intel_uc *uc) if (intel_uc_uses_guc_submission(uc)) intel_guc_submission_enable(guc);

• if (intel_uc_uses_guc_slpc(uc)) {

• ret = intel_guc_slpc_enable(&guc->slpc);
  

• if (ret)
  

• 	goto err_submission;
  

• drm_info(&i915->drm, "GuC SLPC %s\n",
  

• 	 enableddisabled(intel_uc_uses_guc_slpc(uc)));
  

move this drm_info after below GuC report and/or modify to have:

"GuC firmware path.bin version 1.0 loaded:yes" "GuC submission:enabled" "GuC SLPC:enabled" "HuC firmware path.bin version 1.0 authenticated:yes"

Michal

• }
• drm_info(&i915->drm, "%s firmware %s version %u.%u %s:%s\n", intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), guc->fw.path, guc->fw.major_ver_found, guc->fw.minor_ver_found,

@@ -521,6 +529,8 @@ static int __uc_init_hw(struct intel_uc *uc) /* * We've failed to load the firmware :( */ +err_submission:

• intel_guc_submission_disable(guc);

err_log_capture: __uc_capture_load_err_log(uc); err_out:

On 15.07.2021 03:58, Belgaumkar, Vinay wrote:

On 7/10/2021 10:37 AM, Michal Wajdeczko wrote:

...

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

...

...

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index e2644a05f298..3e76d4d5f7bb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2321,10 +2321,6 @@ void intel_guc_submission_enable(struct intel_guc *guc)     void intel_guc_submission_disable(struct intel_guc *guc)   { -    struct intel_gt *gt = guc_to_gt(guc);

-    GEM_BUG_ON(gt->awake); /* GT should be parked first */

if not mistake, can you explain why it was removed ?

This was part of a different commit. The BUG_ON in disable_guc_submission was added with an assumption that it will be called only during driver unload and not expected to hold any GT PM references. Since this needs to be called from an error scenario during slpc enable, remove the BUG_ON. Do we need this as a separate commit?

yes, please

Michal

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Add param set h2g helpers to set the min and max frequencies for use by SLPC.

Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 94 +++++++++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 2 + 2 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index e579408d1c19..19cb26479942 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -106,6 +106,19 @@ static int slpc_send(struct intel_guc_slpc *slpc, return intel_guc_send(guc, action, in_len); }

+static int host2guc_slpc_set_param(struct intel_guc_slpc *slpc,

• 		   u32 id, u32 value)
  

+{

• struct slpc_event_input data = {0};
• data.header.value = SLPC_EVENT(SLPC_EVENT_PARAMETER_SET, 2);
• data.args[0] = id;
• data.args[1] = value;
• return slpc_send(slpc, &data, 4);

as suggested before, use of explicit function like:

static int guc_action_slpc_param(guc, u32 id, u32 value) { u32 request[] = { INTEL_GUC_ACTION_SLPC_REQUEST, SLPC_EVENT(SLPC_EVENT_PARAMETER_SET, 2), id, value, };

return intel_guc_send(guc, request, ARRAY_SIZE(request)); }

will be simpler/cleaner

+}

static bool slpc_running(struct intel_guc_slpc *slpc) { struct slpc_shared_data *data; @@ -134,6 +147,19 @@ static int host2guc_slpc_query_task_state(struct intel_guc_slpc *slpc) return slpc_send(slpc, &data, 4); }

+static int slpc_set_param(struct intel_guc_slpc *slpc, u32 id, u32 value) +{

• struct drm_i915_private *i915 = slpc_to_i915(slpc);
• GEM_BUG_ON(id >= SLPC_MAX_PARAM);
• if (host2guc_slpc_set_param(slpc, id, value)) {

• drm_err(&i915->drm, "Unable to set param %x", id);
  

missing \n what about printing value to be set ? what about printing send error %pe ?

• return -EIO;
  

• }
• return 0;

+}

static int slpc_read_task_state(struct intel_guc_slpc *slpc) { return host2guc_slpc_query_task_state(slpc); @@ -218,6 +244,74 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc) return slpc_shared_data_init(slpc); }

+/**

• • intel_guc_slpc_max_freq_set() - Set max frequency limit for SLPC.
• • @slpc: pointer to intel_guc_slpc.
• • @val: encoded frequency

what's the encoding ?

• • This function will invoke GuC SLPC action to update the max frequency
• • limit for slice and unslice.
• • Return: 0 on success, non-zero error code on failure.
• */

+int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val) +{

• int ret;
• struct drm_i915_private *i915 = slpc_to_i915(slpc);
• intel_wakeref_t wakeref;
• wakeref = intel_runtime_pm_get(&i915->runtime_pm);

use can use with_intel_runtime_pm(rpm, wakeref)

• ret = slpc_set_param(slpc,

•        SLPC_PARAM_GLOBAL_MAX_GT_UNSLICE_FREQ_MHZ,
  

•        val);
  

• if (ret) {

• drm_err(&i915->drm,
  

• 	"Set max frequency unslice returned %d", ret);
  

missing \n print error with %pe but slpc_set_param returns only -EIO ;(

• ret = -EIO;
  

• goto done;
  

• }

+done:

• intel_runtime_pm_put(&i915->runtime_pm, wakeref);
• return ret;

+}

+/**

• • intel_guc_slpc_min_freq_set() - Set min frequency limit for SLPC.
• • @slpc: pointer to intel_guc_slpc.
• • @val: encoded frequency
• • This function will invoke GuC SLPC action to update the min frequency
• • limit.
• • Return: 0 on success, non-zero error code on failure.
• */

+int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) +{

• int ret;
• struct intel_guc *guc = slpc_to_guc(slpc);
• struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
• intel_wakeref_t wakeref;
• wakeref = intel_runtime_pm_get(&i915->runtime_pm);
• ret = slpc_set_param(slpc,

•        SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
  

•        val);
  

• if (ret) {

• drm_err(&i915->drm,
  

• 	"Set min frequency for unslice returned %d", ret);
  

as above

Michal

• ret = -EIO;
  

• goto done;
  

• }

+done:

• intel_runtime_pm_put(&i915->runtime_pm, wakeref);
• return ret;

+}

/*

• intel_guc_slpc_enable() - Start SLPC
• @slpc: pointer to intel_guc_slpc.

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index a2643b904165..a473e1ea7c10 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -34,5 +34,7 @@ struct intel_guc_slpc { int intel_guc_slpc_init(struct intel_guc_slpc *slpc); int intel_guc_slpc_enable(struct intel_guc_slpc *slpc); void intel_guc_slpc_fini(struct intel_guc_slpc *slpc); +int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val); +int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val);

#endif

On 7/10/2021 10:52 AM, Michal Wajdeczko wrote:

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Add helpers to read the min/max frequency being used by SLPC. This is done by send a h2g command which forces

s/h2g/H2G

done.

...

SLPC to update the shared data struct which can then be read.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 58 +++++++++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 2 + 2 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 19cb26479942..98a283d31734 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -278,6 +278,35 @@ int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val) return ret; }

+int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val) +{

• struct slpc_shared_data *data;
• intel_wakeref_t wakeref;
• struct drm_i915_private *i915 = guc_to_gt(slpc_to_guc(slpc))->i915;
• int ret = 0;
• wakeref = intel_runtime_pm_get(&i915->runtime_pm);
• /* Force GuC to update task data */
• if (slpc_read_task_state(slpc)) {

• DRM_ERROR("Unable to update task data");
  

use drm_err missing \n maybe this message could be moved to slpc_read_task_state ?

Done.

...

• ret = -EIO;
  

• goto done;
  

• }
• GEM_BUG_ON(!slpc->vma);
• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));

maybe this can also be part of slpc_read_task_state ?

Yup.

...

• data = slpc->vaddr;
• *val = DIV_ROUND_CLOSEST(data->task_state_data.max_unslice_freq *

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER);
  

+done:

• intel_runtime_pm_put(&i915->runtime_pm, wakeref);
• return ret;

+}

• /**
  • intel_guc_slpc_min_freq_set() - Set min frequency limit for SLPC.
  • @slpc: pointer to intel_guc_slpc.

@@ -312,6 +341,35 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val) return ret; }

+int intel_guc_slpc_get_min_freq(struct intel_guc_slpc *slpc, u32 *val)

missing kernel-doc (above intel_guc_slpc_min_freq_set has one)

done.

...

+{

• struct slpc_shared_data *data;
• intel_wakeref_t wakeref;
• struct drm_i915_private *i915 = guc_to_gt(slpc_to_guc(slpc))->i915;
• int ret = 0;
• wakeref = intel_runtime_pm_get(&i915->runtime_pm);
• /* Force GuC to update task data */
• if (slpc_read_task_state(slpc)) {

• DRM_ERROR("Unable to update task data");
  

see above

...

• ret = -EIO;
  

• goto done;
  

• }
• GEM_BUG_ON(!slpc->vma);
• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));

see above

Done.

Thanks, Vinay.

Michal

...

• data = slpc->vaddr;
• *val = DIV_ROUND_CLOSEST(data->task_state_data.min_unslice_freq *

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER);
  

+done:

• intel_runtime_pm_put(&i915->runtime_pm, wakeref);
• return ret;

+}

• /*
  • intel_guc_slpc_enable() - Start SLPC
  • @slpc: pointer to intel_guc_slpc.

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index a473e1ea7c10..2cb830cdacb5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -36,5 +36,7 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc); void intel_guc_slpc_fini(struct intel_guc_slpc *slpc); int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val); int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val); +int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val); +int intel_guc_slpc_get_min_freq(struct intel_guc_slpc *slpc, u32 *val);

#endif

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

This prints out relevant SLPC info from the SLPC shared structure.

We will send a h2g message which forces SLPC to update the shared data structure with latest information before reading it.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Sundaresan Sujaritha sujaritha.sundaresan@intel.com

---

.../gpu/drm/i915/gt/uc/intel_guc_debugfs.c | 16 ++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 53 +++++++++++++++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 3 ++ 3 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_debugfs.c index 9a03ff56e654..bef749e54601 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_debugfs.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_debugfs.c @@ -12,6 +12,7 @@ #include "gt/uc/intel_guc_ct.h" #include "gt/uc/intel_guc_ads.h" #include "gt/uc/intel_guc_submission.h" +#include "gt/uc/intel_guc_slpc.h"

static int guc_info_show(struct seq_file *m, void *data) { @@ -50,11 +51,26 @@ static int guc_registered_contexts_show(struct seq_file *m, void *data) } DEFINE_GT_DEBUGFS_ATTRIBUTE(guc_registered_contexts);

+static int guc_slpc_info_show(struct seq_file *m, void *unused) +{

• struct intel_guc *guc = m->private;
• struct intel_guc_slpc *slpc = &guc->slpc;
• struct drm_printer p = drm_seq_file_printer(m);
• if (!intel_guc_slpc_is_used(guc))

• return -ENODEV;
  

• return intel_guc_slpc_info(slpc, &p);

+}

other entries don't have empty line here

+DEFINE_GT_DEBUGFS_ATTRIBUTE(guc_slpc_info);

void intel_guc_debugfs_register(struct intel_guc *guc, struct dentry *root) { static const struct debugfs_gt_file files[] = { { "guc_info", &guc_info_fops, NULL }, { "guc_registered_contexts", &guc_registered_contexts_fops, NULL },

• { "guc_slpc_info", &guc_slpc_info_fops, NULL},
  

IIRC last field is "eval" where maybe you could add your own to check if intel_guc_slpc_is_used() to avoid exposing this info if N/A

};

if (!intel_guc_is_supported(guc)) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 98a283d31734..d179ba14ece6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -432,6 +432,59 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) return 0; }

+int intel_guc_slpc_info(struct intel_guc_slpc *slpc, struct drm_printer *p) +{

• struct drm_i915_private *i915 = guc_to_gt(slpc_to_guc(slpc))->i915;
• struct slpc_shared_data *data;
• struct slpc_platform_info *platform_info;
• struct slpc_task_state_data *task_state_data;
• intel_wakeref_t wakeref;
• int ret = 0;
• wakeref = intel_runtime_pm_get(&i915->runtime_pm);
• if (slpc_read_task_state(slpc)) {

• ret = -EIO;
  

• goto done;
  

• }
• GEM_BUG_ON(!slpc->vma);
• drm_clflush_virt_range(slpc->vaddr, sizeof(struct slpc_shared_data));

likely will go away if integrated into slpc_read_task_state

• data = slpc->vaddr;
• platform_info = &data->platform_info;

is this used ?

• task_state_data = &data->task_state_data;

as it looks that you treat these sections separately, then maybe it would be cleaner to have:

static void print_global_data(*global_data, *p) {} static void print_platform_info(*platform_info, *p) {} static void print_task_state_data(*task_state_data, *p) {}

• drm_printf(p, "SLPC state: %s\n", slpc_state_stringify(data->global_state));
• drm_printf(p, "\tgtperf task active: %d\n",

• 	task_state_data->gtperf_task_active);
  

• drm_printf(p, "\tdcc task active: %d\n",

• 		task_state_data->dcc_task_active);
  

• drm_printf(p, "\tin dcc: %d\n",

• 		task_state_data->in_dcc);
  

• drm_printf(p, "\tfreq switch active: %d\n",

• 		task_state_data->freq_switch_active);
  

• drm_printf(p, "\tibc enabled: %d\n",

• 		task_state_data->ibc_enabled);
  

• drm_printf(p, "\tibc active: %d\n",

• 		task_state_data->ibc_active);
  

• drm_printf(p, "\tpg1 enabled: %s\n",

• 		yesno(task_state_data->pg1_enabled));
  

• drm_printf(p, "\tpg1 active: %s\n",

• 		yesno(task_state_data->pg1_active));
  

• drm_printf(p, "\tmax freq: %dMHz\n",

• 		DIV_ROUND_CLOSEST(data->task_state_data.max_unslice_freq *
  

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER));
  

• drm_printf(p, "\tmin freq: %dMHz\n",

• 		DIV_ROUND_CLOSEST(data->task_state_data.min_unslice_freq *
  

• 		GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER));
  

you defined task_state_data but in above 2 you're accessing it from data

Michal

+done:

• intel_runtime_pm_put(&i915->runtime_pm, wakeref);
• return ret;

+}

void intel_guc_slpc_fini(struct intel_guc_slpc *slpc) { if (!slpc->vma) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index 2cb830cdacb5..cd12c5f19f4b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -10,6 +10,8 @@ #include <linux/mutex.h> #include "intel_guc_slpc_fwif.h"

+struct drm_printer;

struct intel_guc_slpc { /*Protects access to vma and SLPC actions */ struct i915_vma *vma; @@ -38,5 +40,6 @@ int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val); int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val); int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val); int intel_guc_slpc_get_min_freq(struct intel_guc_slpc *slpc, u32 *val); +int intel_guc_slpc_info(struct intel_guc_slpc *slpc, struct drm_printer *p);

#endif

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Cache rp0, rp1 and rpn platform limits into slpc structure for range checking while setting min/max frequencies.

Also add "soft" limits which keep track of frequency changes made from userland. These are initially set to platform min and max.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 41 +++++++++++++++++++++ 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index d32274cd1db7..6e978f27b7a6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -86,6 +86,9 @@ static int slpc_shared_data_init(struct intel_guc_slpc *slpc) return err; }

• slpc->max_freq_softlimit = 0;
• slpc->min_freq_softlimit = 0;

as mentioned earlier, now it is time to introduce these fields in .h

• return err;

}

@@ -384,6 +387,29 @@ void intel_guc_pm_intrmsk_enable(struct intel_gt *gt) GEN6_PMINTRMSK, pm_intrmsk_mbz, 0); }

+static int intel_guc_slpc_set_softlimits(struct intel_guc_slpc *slpc) +{

• int ret = 0;
• /* Softlimits are initially equivalent to platform limits

• * unless they have deviated from defaults, in which case,
  

• * we retain the values and set min/max accordingly.
  

• */
  

• if (!slpc->max_freq_softlimit)

• slpc->max_freq_softlimit = slpc->rp0_freq;
  

• else if (slpc->max_freq_softlimit != slpc->rp0_freq)

• ret = intel_guc_slpc_set_max_freq(slpc,
  

• 			slpc->max_freq_softlimit);
  

• if (!slpc->min_freq_softlimit)

• slpc->min_freq_softlimit = slpc->min_freq;
  

• else if (slpc->min_freq_softlimit != slpc->min_freq)

• ret = intel_guc_slpc_set_min_freq(slpc,
  

• 			slpc->min_freq_softlimit);
  

• return ret;

+}

/*

• intel_guc_slpc_enable() - Start SLPC
• @slpc: pointer to intel_guc_slpc.

@@ -402,6 +428,7 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) struct drm_i915_private *i915 = slpc_to_i915(slpc); struct slpc_shared_data *data; int ret;

• u32 rp_state_cap;

move up to keep "ret" last

GEM_BUG_ON(!slpc->vma);

@@ -445,6 +472,20 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) DIV_ROUND_CLOSEST(data->task_state_data.max_unslice_freq * GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER));

• rp_state_cap = intel_uncore_read(i915->gt.uncore, GEN6_RP_STATE_CAP);
• slpc->rp0_freq = ((rp_state_cap >> 0) & 0xff) * GT_FREQUENCY_MULTIPLIER;
• slpc->min_freq = ((rp_state_cap >> 16) & 0xff) * GT_FREQUENCY_MULTIPLIER;
• slpc->rp1_freq = ((rp_state_cap >> 8) & 0xff) * GT_FREQUENCY_MULTIPLIER;

we should have definitions for these bits and then we should be able to use REG_FIELD_GET

• if (intel_guc_slpc_set_softlimits(slpc))

• drm_err(&i915->drm, "Unable to set softlimits");
  

missing \n maybe we can also print error ?

• drm_info(&i915->drm,

•  "Platform fused frequency values -  min: %u Mhz, max: %u Mhz",
  

missing \n double space before 'min'

Michal

•  slpc->min_freq,
  

•  slpc->rp0_freq);
  

• return 0;

}

On 7/10/2021 11:15 AM, Michal Wajdeczko wrote:

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Cache rp0, rp1 and rpn platform limits into slpc structure for range checking while setting min/max frequencies.

Also add "soft" limits which keep track of frequency changes made from userland. These are initially set to platform min and max.

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 41 +++++++++++++++++++++ 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index d32274cd1db7..6e978f27b7a6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -86,6 +86,9 @@ static int slpc_shared_data_init(struct intel_guc_slpc *slpc) return err; }

• slpc->max_freq_softlimit = 0;
• slpc->min_freq_softlimit = 0;

as mentioned earlier, now it is time to introduce these fields in .h

done.

...

• return err; }

@@ -384,6 +387,29 @@ void intel_guc_pm_intrmsk_enable(struct intel_gt *gt) GEN6_PMINTRMSK, pm_intrmsk_mbz, 0); }

+static int intel_guc_slpc_set_softlimits(struct intel_guc_slpc *slpc) +{

• int ret = 0;
• /* Softlimits are initially equivalent to platform limits

• * unless they have deviated from defaults, in which case,
  

• * we retain the values and set min/max accordingly.
  

• */
  

• if (!slpc->max_freq_softlimit)

• slpc->max_freq_softlimit = slpc->rp0_freq;
  

• else if (slpc->max_freq_softlimit != slpc->rp0_freq)

• ret = intel_guc_slpc_set_max_freq(slpc,
  

• 			slpc->max_freq_softlimit);
  

• if (!slpc->min_freq_softlimit)

• slpc->min_freq_softlimit = slpc->min_freq;
  

• else if (slpc->min_freq_softlimit != slpc->min_freq)

• ret = intel_guc_slpc_set_min_freq(slpc,
  

• 			slpc->min_freq_softlimit);
  

• return ret;

+}

• /*
  • intel_guc_slpc_enable() - Start SLPC
  • @slpc: pointer to intel_guc_slpc.

@@ -402,6 +428,7 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) struct drm_i915_private *i915 = slpc_to_i915(slpc); struct slpc_shared_data *data; int ret;

• u32 rp_state_cap;

move up to keep "ret" last

Done.

...

GEM_BUG_ON(!slpc->vma);

@@ -445,6 +472,20 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc) DIV_ROUND_CLOSEST(data->task_state_data.max_unslice_freq * GT_FREQUENCY_MULTIPLIER, GEN9_FREQ_SCALER));

• rp_state_cap = intel_uncore_read(i915->gt.uncore, GEN6_RP_STATE_CAP);
• slpc->rp0_freq = ((rp_state_cap >> 0) & 0xff) * GT_FREQUENCY_MULTIPLIER;
• slpc->min_freq = ((rp_state_cap >> 16) & 0xff) * GT_FREQUENCY_MULTIPLIER;
• slpc->rp1_freq = ((rp_state_cap >> 8) & 0xff) * GT_FREQUENCY_MULTIPLIER;

we should have definitions for these bits and then we should be able to use REG_FIELD_GET

done.

...

• if (intel_guc_slpc_set_softlimits(slpc))

• drm_err(&i915->drm, "Unable to set softlimits");
  

missing \n maybe we can also print error ?

done.

...

• drm_info(&i915->drm,

•  "Platform fused frequency values -  min: %u Mhz, max: %u Mhz",
  

missing \n double space before 'min'

done.

Thanks, Vinay.

Michal

...

•  slpc->min_freq,
  

•  slpc->rp0_freq);
  

• return 0; }

On 7/10/2021 11:20 AM, Michal Wajdeczko wrote:

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

...

Update the get/set min/max freq hooks to work for slpc case as well. Consolidate helpers for requested/min/max frequency get/set to intel_rps where the proper action can be taken depending on whether slpc is enabled.

2x s/slpc/SLPC

done.

...

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@linux.intel.com Signed-off-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com

---

drivers/gpu/drm/i915/gt/intel_rps.c | 135 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 5 ++ drivers/gpu/drm/i915/i915_pmu.c | 2 +- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c | 71 +++------------ 5 files changed, 154 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index e858eeb2c59d..88ffc5d90730 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -37,6 +37,12 @@ static struct intel_uncore *rps_to_uncore(struct intel_rps *rps) return rps_to_gt(rps)->uncore; }

+static struct intel_guc_slpc *rps_to_slpc(struct intel_rps *rps) +{

• struct intel_gt *gt = rps_to_gt(rps);
• return &gt->uc.guc.slpc;

either add empty line between decl/code or make it one-liner

done.

...

+}

• static bool rps_uses_slpc(struct intel_rps *rps) { struct intel_gt *gt = rps_to_gt(rps);

@@ -1960,6 +1966,135 @@ u32 intel_rps_read_actual_frequency(struct intel_rps *rps) return freq; }

+u32 intel_rps_read_punit_req(struct intel_rps *rps) +{

• struct intel_uncore *uncore = rps_to_uncore(rps);

drop empty line

done.

...

• u32 pureq = intel_uncore_read(uncore, GEN6_RPNSWREQ);
• return pureq;

+}

+u32 intel_rps_get_req(struct intel_rps *rps, u32 pureq) +{

• u32 req = pureq >> GEN9_SW_REQ_UNSLICE_RATIO_SHIFT;
• return req;

+}

+u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps) +{

• u32 freq = intel_rps_get_req(rps, intel_rps_read_punit_req(rps));
• return intel_gpu_freq(rps, freq);

+}

+u32 intel_rps_get_requested_frequency(struct intel_rps *rps) +{

• if (rps_uses_slpc(rps))

• return intel_rps_read_punit_req_frequency(rps);
  

• else

• return intel_gpu_freq(rps, rps->cur_freq);
  

+}

+u32 intel_rps_get_max_frequency(struct intel_rps *rps) +{

• struct intel_guc_slpc *slpc = rps_to_slpc(rps);
• if (rps_uses_slpc(rps))

• return slpc->max_freq_softlimit;
  

• else

• return intel_gpu_freq(rps, rps->max_freq_softlimit);
  

+}

+int intel_rps_set_max_frequency(struct intel_rps *rps, u32 val) +{

• struct intel_guc_slpc *slpc = rps_to_slpc(rps);
• int ret;
• if (rps_uses_slpc(rps))

• return intel_guc_slpc_set_max_freq(slpc, val);
  

• mutex_lock(&rps->lock);
• val = intel_freq_opcode(rps, val);
• if (val < rps->min_freq ||

•    val > rps->max_freq ||
  

•    val < rps->min_freq_softlimit) {
  

• ret = -EINVAL;
  

• goto unlock;
  

• }
• if (val > rps->rp0_freq)

• DRM_DEBUG("User requested overclocking to %d\n",
  

use drm_dbg

Done.

Thanks, Vinay.

Michal

...

• 	  intel_gpu_freq(rps, val));
  

• rps->max_freq_softlimit = val;
• val = clamp_t(int, rps->cur_freq,

•       rps->min_freq_softlimit,
  

•       rps->max_freq_softlimit);
  

• /*

• * We still need *_set_rps to process the new max_delay and
  

• * update the interrupt limits and PMINTRMSK even though
  

• * frequency request may be unchanged.
  

• */
  

• intel_rps_set(rps, val);

+unlock:

• mutex_unlock(&rps->lock);
• return ret;

+}

+u32 intel_rps_get_min_frequency(struct intel_rps *rps) +{

• struct intel_guc_slpc *slpc = rps_to_slpc(rps);
• if (rps_uses_slpc(rps))

• return slpc->min_freq_softlimit;
  

• else

• return intel_gpu_freq(rps, rps->min_freq_softlimit);
  

+}

+int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val) +{

• struct intel_guc_slpc *slpc = rps_to_slpc(rps);
• int ret;
• if (rps_uses_slpc(rps))

• return intel_guc_slpc_set_min_freq(slpc, val);
  

• mutex_lock(&rps->lock);
• val = intel_freq_opcode(rps, val);
• if (val < rps->min_freq ||

•    val > rps->max_freq ||
  

•    val > rps->max_freq_softlimit) {
  

• ret = -EINVAL;
  

• goto unlock;
  

• }
• rps->min_freq_softlimit = val;
• val = clamp_t(int, rps->cur_freq,

•       rps->min_freq_softlimit,
  

•       rps->max_freq_softlimit);
  

• /*

• * We still need *_set_rps to process the new min_delay and
  

• * update the interrupt limits and PMINTRMSK even though
  

• * frequency request may be unchanged.
  

• */
  

• intel_rps_set(rps, val);

+unlock:

• mutex_unlock(&rps->lock);
• return ret;

+}

• /* External interface for intel_ips.ko */

  static struct drm_i915_private __rcu *ips_mchdev;

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 1d2cfc98b510..9a09ff5ebf64 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -31,6 +31,11 @@ int intel_gpu_freq(struct intel_rps *rps, int val); int intel_freq_opcode(struct intel_rps *rps, int val); u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1); u32 intel_rps_read_actual_frequency(struct intel_rps *rps); +u32 intel_rps_get_requested_frequency(struct intel_rps *rps); +u32 intel_rps_get_min_frequency(struct intel_rps *rps); +int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val); +u32 intel_rps_get_max_frequency(struct intel_rps *rps); +int intel_rps_set_max_frequency(struct intel_rps *rps, u32 val);

void gen5_rps_irq_handler(struct intel_rps *rps); void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir); diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 34d37d46a126..a896bec18255 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -407,7 +407,7 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)

if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) { add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_REQ],

• 		intel_gpu_freq(rps, rps->cur_freq),
  

• 		intel_rps_get_requested_frequency(rps),
  	period_ns / 1000);
  

  }

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 7d9e90aa3ec0..8ab3c2f8f8e4 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9195,6 +9195,8 @@ enum { #define GEN9_FREQUENCY(x) ((x) << 23) #define GEN6_OFFSET(x) ((x) << 19) #define GEN6_AGGRESSIVE_TURBO (0 << 15) +#define GEN9_SW_REQ_UNSLICE_RATIO_SHIFT 23

• #define GEN6_RC_VIDEO_FREQ _MMIO(0xA00C) #define GEN6_RC_CONTROL _MMIO(0xA090) #define GEN6_RC_CTL_RC6pp_ENABLE (1 << 16)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 873bf996ceb5..f2eee8491b19 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -272,7 +272,7 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev, struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); struct intel_rps *rps = &i915->gt.rps;

• return sysfs_emit(buf, "%d\n", intel_gpu_freq(rps, rps->cur_freq));

• return sysfs_emit(buf, "%d\n", intel_rps_get_requested_frequency(rps)); }

  static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)

@@ -326,9 +326,10 @@ static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev, static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);

• struct intel_rps *rps = &dev_priv->gt.rps;

• struct intel_gt *gt = &dev_priv->gt;
• struct intel_rps *rps = &gt->rps;

• return sysfs_emit(buf, "%d\n", intel_gpu_freq(rps, rps->max_freq_softlimit));

• return sysfs_emit(buf, "%d\n", intel_rps_get_max_frequency(rps)); }

  static ssize_t gt_max_freq_mhz_store(struct device *kdev,

@@ -336,7 +337,8 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, const char *buf, size_t count) { struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);

• struct intel_rps *rps = &dev_priv->gt.rps;

• struct intel_gt *gt = &dev_priv->gt;
• struct intel_rps *rps = &gt->rps; ssize_t ret; u32 val;

@@ -344,35 +346,7 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, if (ret) return ret;

• mutex_lock(&rps->lock);
• val = intel_freq_opcode(rps, val);
• if (val < rps->min_freq ||

•    val > rps->max_freq ||
  

•    val < rps->min_freq_softlimit) {
  

• ret = -EINVAL;
  

• goto unlock;
  

• }
• if (val > rps->rp0_freq)

• DRM_DEBUG("User requested overclocking to %d\n",
  

• 	  intel_gpu_freq(rps, val));
  

• rps->max_freq_softlimit = val;
• val = clamp_t(int, rps->cur_freq,

•       rps->min_freq_softlimit,
  

•       rps->max_freq_softlimit);
  

• /*

• * We still need *_set_rps to process the new max_delay and
  

• * update the interrupt limits and PMINTRMSK even though
  

• * frequency request may be unchanged.
  

• */
  

• intel_rps_set(rps, val);

-unlock:

• mutex_unlock(&rps->lock);

• ret = intel_rps_set_max_frequency(rps, val);

  return ret ?: count; }

@@ -380,9 +354,10 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);

• struct intel_rps *rps = &dev_priv->gt.rps;

• struct intel_gt *gt = &dev_priv->gt;
• struct intel_rps *rps = &gt->rps;

• return sysfs_emit(buf, "%d\n", intel_gpu_freq(rps, rps->min_freq_softlimit));

• return sysfs_emit(buf, "%d\n", intel_rps_get_min_frequency(rps)); }

  static ssize_t gt_min_freq_mhz_store(struct device *kdev,

@@ -398,31 +373,7 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, if (ret) return ret;

• mutex_lock(&rps->lock);
• val = intel_freq_opcode(rps, val);
• if (val < rps->min_freq ||

•    val > rps->max_freq ||
  

•    val > rps->max_freq_softlimit) {
  

• ret = -EINVAL;
  

• goto unlock;
  

• }
• rps->min_freq_softlimit = val;
• val = clamp_t(int, rps->cur_freq,

•       rps->min_freq_softlimit,
  

•       rps->max_freq_softlimit);
  

• /*

• * We still need *_set_rps to process the new min_delay and
  

• * update the interrupt limits and PMINTRMSK even though
  

• * frequency request may be unchanged.
  

• */
  

• intel_rps_set(rps, val);

-unlock:

• mutex_unlock(&rps->lock);

• ret = intel_rps_set_min_frequency(rps, val);

  return ret ?: count; }

On 10.07.2021 03:20, Vinay Belgaumkar wrote:

Tests that exercise the slpc get/set frequency interfaces.

Clamp_max will set max frequency to multiple levels and check that slpc requests frequency lower than or equal to it.

Clamp_min will set min frequency to different levels and check if slpc requests are higher or equal to those levels.

2x s/slpc/SLPC

Signed-off-by: Vinay Belgaumkar vinay.belgaumkar@intel.com

drivers/gpu/drm/i915/gt/intel_rps.c | 1 + drivers/gpu/drm/i915/gt/selftest_slpc.c | 333 ++++++++++++++++++ drivers/gpu/drm/i915/gt/selftest_slpc.h | 12 + .../drm/i915/selftests/i915_live_selftests.h | 1 + 4 files changed, 347 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/selftest_slpc.c create mode 100644 drivers/gpu/drm/i915/gt/selftest_slpc.h

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 88ffc5d90730..16ac2e840881 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2288,4 +2288,5 @@ EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);

#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftest_rps.c" +#include "selftest_slpc.c" #endif diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c new file mode 100644 index 000000000000..f440c1cb2afa --- /dev/null +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright © 2020 Intel Corporation

2021

• */

+#include "selftest_slpc.h" +#include "selftest_rps.h"

+#include <linux/pm_qos.h> +#include <linux/sort.h>

system headers should go first

+#include "intel_engine_heartbeat.h" +#include "intel_engine_pm.h" +#include "intel_gpu_commands.h" +#include "intel_gt_clock_utils.h" +#include "intel_gt_pm.h" +#include "intel_rc6.h" +#include "selftest_engine_heartbeat.h" +#include "intel_rps.h" +#include "selftests/igt_flush_test.h" +#include "selftests/igt_spinner.h"

wrong order ?

+#define NUM_STEPS 5 +#define H2G_DELAY 50000 +#define delay_for_h2g() usleep_range(H2G_DELAY, H2G_DELAY + 10000)

+static int set_min_freq(struct intel_guc_slpc *slpc, int freq) +{

• int ret;

add empty line

• ret = intel_guc_slpc_set_min_freq(slpc, freq);
• if (ret) {

• pr_err("Could not set min frequency to [%d]\n", freq);
  

• return ret;
  

• } else {

• /* Delay to ensure h2g completes */
  

• delay_for_h2g();
  

• }
• return ret;

+}

+static int set_max_freq(struct intel_guc_slpc *slpc, int freq) +{

• int ret;

add empty line

• ret = intel_guc_slpc_set_max_freq(slpc, freq);
• if (ret) {

• pr_err("Could not set maximum frequency [%d]\n",
  

• 	freq);
  

• return ret;
  

• } else {

• /* Delay to ensure h2g completes */
  

• delay_for_h2g();
  

• }
• return ret;

+}

+int live_slpc_clamp_min(void *arg) +{

• struct drm_i915_private *i915 = arg;
• struct intel_gt *gt = &i915->gt;
• struct intel_guc_slpc *slpc;
• struct intel_rps *rps;
• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• struct igt_spinner spin;
• int err = 0;

usually "err" is last decl

• u32 slpc_min_freq, slpc_max_freq;