[Intel-gfx] [PATCH v12 05/17] drm/i915/guc/slpc: Add SLPC communication interfaces
Sagar Arun Kamble
sagar.a.kamble at intel.com
Fri Mar 30 15:57:44 UTC 2018
On 3/30/2018 7:07 PM, Michal Wajdeczko wrote:
> On Fri, 30 Mar 2018 10:31:50 +0200, Sagar Arun Kamble
> <sagar.a.kamble at intel.com> wrote:
<snip>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_slpc.h
>> b/drivers/gpu/drm/i915/intel_guc_slpc.h
>> index 66c76fe..81250c0 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_slpc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_slpc.h
>> @@ -6,6 +6,8 @@
>> #ifndef _INTEL_GUC_SLPC_H_
>> #define _INTEL_GUC_SLPC_H_
>> +#include <intel_guc_slpc_fwif.h>
>
> Please use "" instead of <>
Yes. will hopefully not forget this next time :)
>
>> +
>> struct intel_guc_slpc {
>> };
>> diff --git a/drivers/gpu/drm/i915/intel_guc_slpc_fwif.h
>> b/drivers/gpu/drm/i915/intel_guc_slpc_fwif.h
>> new file mode 100644
>> index 0000000..9400af4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_guc_slpc_fwif.h
>> @@ -0,0 +1,211 @@
>> +/*
>> + * SPDX-License-Identifier: MIT
>> + *
>> + * Copyright © 2015-2018 Intel Corporation
>> + */
>> +#ifndef _INTEL_GUC_SLPC_FWIF_H_
>> +#define _INTEL_GUC_SLPC_FWIF_H_
>> +
>> +#include <linux/types.h>
>> +
>> +enum slpc_status {
>
> s/slpc_status/intel_guc_slpc_status
>
I have done this because of following reasons:
1. GuC SLPC interface file shared by firmware team names enums/structs
as SLPM_* or SLPM_KMD_*. i understand that this isn't strict requirement. :)
2. INTEL_GUC_STATUS|EVENT|PARAM_* becomes more than 80 chars for some values
3. I wanted intel_guc_slpc_fwif.h to be included only in
intel_guc_slpc.c from where we can export functions as intel_guc_slpc_*
static functions in intel_guc_slpc.c are named as slpc_*.
In my series, point 3 above is not done but that was intent. There
is access to SLPC enums from debugfs.c. Would consolidate all in
intel_guc_slpc.c and export.
Does this plan look good?
>> + 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
>
> s/SLPC_STATUS/INTEL_GUC_SLPC_STATUS
>
>> +};
>> +
>> +enum slpc_event_id {
>
> s/slpc_event_id/intel_guc_slpc_event
>
>> + 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,
>
> s/SLPC_EVENT/INTEL_GUC_SLPC_EVENT
>
>> +};
>> +
>> +enum slpc_param_id {
>
> s/slpc_param_id/intel_guc_slpc_param
>
>> + 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,
>
> s/SLPC_PARAM/INTEL_GUC_SLPC_PARAM
>
>> + SLPC_MAX_PARAM,
>> + SLPC_KMD_MAX_PARAM = 32,
>
> hmm, do we really need these two ? please drop
>
> or maybe these are related to SLPC_MAX_OVERRIDE_PARAMETERS ?
Ok. Will drop SLPC_KMD_MAX_PARAM.
There are total 192 params (MAX_OVERRIDE_PARAMS) out of which 32
(KMD_MAX_PARAM) params can be exposed
to KMD, but currently only params till MAX_PARAM < 32 are exported.
>
>> +};
>> +
>> +enum slpc_global_state {
>
> s/slpc_global_state/intel_guc_slpc_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
>
> s/SLPC_GLOBAL_STATE/INTEL_GUC_SLPC_STATE
>
>> +};
>> +
>> +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,
>
> hmm, this should be already known to GuC/SLPC.
> and if still needed, can it be made non-SLPC specific?
>
In current firmwares this is not possible but certainly can be changed
in newer firmwares.
Will suggest this to Bob, Daisy.
Will keep it SLPC specific for now as only SLPC uses it.
>> +};
>> +
>> +enum slpc_power_source {
>
> s/slpc_power_source/intel_power_source
>
>> + SLPC_POWER_SOURCE_UNDEFINED = 0,
>> + SLPC_POWER_SOURCE_AC = 1,
>> + SLPC_POWER_SOURCE_DC = 2,
>> + SLPC_POWER_SOURCE_UNKNOWN = 3,
>
> s/SLPC_POWER_SOURCE/INTEL_POWER_SOURCE
>
>> +};
>> +
>> +enum slpc_power_plan {
>> + SLPC_POWER_PLAN_UNDEFINED = 0,
>> + SLPC_POWER_PLAN_BATTERY_SAVER = 1,
>> + SLPC_POWER_PLAN_BALANCED = 2,
>> + SLPC_POWER_PLAN_PERFORMANCE = 3,
>> + SLPC_POWER_PLAN_UNKNOWN = 4,
>> +};
>> +
>> +struct slpc_platform_info {
>
> s/slpc_platform_info/intel_guc_slpc_platform
>
>> + u8 sku;
>> + u8 slice_count;
>> + u8 reserved;
>> + u8 power_plan_source;
>
> from macros below, it looks that this is bitfield and
> in other struct bitfields are defined explicitly...
>
> what about defining related MASK/SHIFT and MAKE/TO macros here?
>
Yes. Can add those.
>> + u8 p0_freq;
>> + u8 p1_freq;
>> + u8 pe_freq;
>> + u8 pn_freq;
>> + u32 reserved1;
>> + u32 reserved2;
>> +} __packed;
>> +
>> +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;
>> + u32 reserved:13;
>
> All register bits and most of shared data are defined as
> macros, maybe we can do the same for SLPC ?
>
> #define INTEL_GUC_SLPC_TASK_GTPERF_ACTIVE (1 << 0)
> #define INTEL_GUC_SLPC_TASK_GTPERF_STALL (1 << 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;
>> +
>> +#define SLPC_MAX_OVERRIDE_PARAMETERS 192
>> +#define SLPC_OVERRIDE_BITFIELD_SIZE \
>> + ((SLPC_MAX_OVERRIDE_PARAMETERS + 31) / 32)
>
> are there "NON-OVERRIDE" parameters?
>
Yes. Non-accessible ones (>MAX_PARAM)
>> +
>> +struct slpc_shared_data {
>
> s/slpc_shared_data/intel_guc_slpc_shared_data
> or
> s/slpc_shared_data/intel_guc_slpc_data
>
>> + u32 reserved;
>
> hmm, starting with reserved is odd ;)
Yes.
>
>> + u32 shared_data_size;
>
> s/shared_data_size/size
>
Will do this.
>> + u32 global_state;
>> + struct slpc_platform_info platform_info;
>> + struct slpc_task_state_data task_state_data;
>> + u32 override_params_set_bits[SLPC_OVERRIDE_BITFIELD_SIZE];
>> + u32 override_params_values[SLPC_MAX_OVERRIDE_PARAMETERS];
>
> above two can be promoted to separate struct:
>
> struct intel_guc_slpc_params {
> u32 valid[INTEL_GUC_SLPC_PARAMS_BITFIELD_SIZE];
> u32 values[INTEL_GUC_SLPC_MAX_PARAMS];
> };
>
Sure.
>> +} __packed;
>> +
>> +enum slpc_reset_flags {
>
> s/slpc_reset_flags/intel_guc_slpc_reset_flags
>
>> + SLPC_RESET_FLAG_TDR_OCCURRED = (1 << 0)
>
> s/SLPC_RESET_FLAG/INTEL_GUC_SLPC_RESET_FLAG
> and to minimize later diffs add trailing ","
>
>> +};
>> +
>> +#define SLPC_EVENT_MAX_INPUT_ARGS 7
>> +#define SLPC_EVENT_MAX_OUTPUT_ARGS 1
>> +
>> +union slpc_event_input_header {
>> + u32 value;
>> + struct {
>> + u32 num_args:8;
>> + u32 event_id:8;
>> + };
>
> I'm not sure that this struct deserves separate definition
>
>> +};
>> +
>> +struct slpc_event_input {
>> + u32 h2g_action_id;
>
> what h2g action is this ?
Seems to be not used by even by GuC SLPC. another change for interface
update.
> if this event is sent over CTB then probably num_args is
> redundant as total CTB message len is already stored in
> CT header...
>
Agree.
>> + union slpc_event_input_header header;
>> + u32 args[SLPC_EVENT_MAX_INPUT_ARGS];
>> +} __packed;
>> +
>> +union slpc_event_output_header {
>> + u32 value;
>> + struct {
>> + u32 num_args:8;
>> + u32 event_id:8;
>> + u32 status:16;
>> + };
>> +};
>> +
>> +struct slpc_event_output {
>> + u32 reserved;
>
> is this needed ?
>
Can skip. What I have done is take entire SLPC interface file from
firmware source and just rename things.
Agree that such things should not be exported if they are not going to
be used. Will share with Bob/Daisy.
>> + union slpc_event_output_header header;
>
> hmm, "header" should be first ;)
>
>> + u32 args[SLPC_EVENT_MAX_OUTPUT_ARGS];
>> +} __packed;
>> +
>> +#define SLPC_EVENT(id, argc) ((u32)(id) << 8 | (argc))
>> +#define SLPC_POWER_PLAN_SOURCE(plan, source) ((plan) | ((source) << 6))
>> +#define SLPC_POWER_PLAN(plan_source) ((plan_source) & 0x3F)
>> +#define SLPC_POWER_SOURCE(plan_source) ((plan_source) >> 6)
>
> maybe:
>
> INTEL_GUC_SLPC_MAKE_EVENT
> INTEL_GUC_SLPC_MAKE_POWER_INFO
> INTEL_GUC_SLPC_POWER_INFO_TO_PLAN
> INTEL_GUC_SLPC_POWER_INFO_TO_SOURCE
>
> and use SHIFT/MASK macros
>
Yes but need your thoughts on INTEL_GUC prefix.
>> +
>> +#define SLPC_PARAM_TASK_DEFAULT 0
>> +#define SLPC_PARAM_TASK_ENABLED 1
>> +#define SLPC_PARAM_TASK_DISABLED 2
>> +#define SLPC_PARAM_TASK_UNKNOWN 3
>
> in other places you are using enums, why #defines here?
>
Will unify.
Thanks for the review
>> +
>> +#endif
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list