Re: [PATCH v12 07/17] drm/i915/guc/slpc: Send RESET event to restart/enable SLPC tasks

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Fri, 30 Mar 2018 10:31:52 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Host to GuC actions for SLPC receive additional data as output through
> scratch registers currently. intel_guc_send_and_receive handles this.
> We need to define SLPC specific Host to GuC send action (slpc_send) as
> wrapper on top of it to process the SLPC status that is received in
> SOFT_SCRATCH(1).
>
> Send host2guc SLPC reset event to GuC post GuC load for enabling SLPC.
> Post this, i915 can ascertain if SLPC has started running successfully
> through shared data. This check is done by waiting for maximum 5ms.
> SLPC reset event also needs to be sent when parameters in shared data
> are updated.
>
> v1: Extract host2guc_slpc to handle slpc status code and style changes.
>     (Paulo). Removed WARN_ON for checking msb of gtt address of shared
>      gem obj. (Chris). host2guc_action to i915_guc_action change.(Sagar)
>     Updating SLPC enabled status. (Sagar)
>
> v2: Commit message update. (David)
>
> v3: Rebase.
>
> v4: Added DRM_INFO message when SLPC is enabled.
>
> v5: Updated patch as host2guc_slpc is moved to earlier patch. SLPC
>     activation status message put after checking the state from shared
>     data during intel_init_gt_powersave.
>
> v6: Added definition of host2guc_slpc and clflush the shared data only
>     for required size. Setting state to NOT_RUNNING before sending RESET
>     event. Output data for SLPC actions is to be retrieved during
>     intel_guc_send with lock protection so created wrapper
>     __intel_guc_send that outputs GuC output data if needed. Clearing
>     pm_rps_events on confirming SLPC RUNNING status so that even if
>     host touches any of the PM registers by mistake it should not have
>     any effect. (Sagar)
>
> v7: Added save/restore_default_rps as Uncore sanitize will clear the
>     RP_CONTROL setup by BIOS. s/i915_ggtt_offset/guc_ggtt_offset.
>
> v8: Added support for handling TDR based SLPC reset. Added functions
>     host2guc_slpc_tdr_reset, intel_slpc_reset_prepare and
>     intel_slpc_tdr_reset to handle TDR based SLPC reset.
>
> v9: Moved TDR support to later patch. Removed intel_slpc_get_status
>     and waiting for maximum of 5ms for SLPC state to turn RUNNING instead
>     of hiding the latency across uc_init_hw and init_gt_powersave.
>     s/if..else/switch..case in intel_guc_slpc_get_state_str. Removed SLPC
>     sanitization from init_gt_powersave. (Michal Wajdeczko)
>
> v10: Rebase.
>
> v11: Rebase. Created slpc_send func as wrapper on guc_send_and_receive.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_slpc.c | 239  
> ++++++++++++++++++++++++++++++++++
>  1 file changed, 239 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_slpc.c  
> b/drivers/gpu/drm/i915/intel_guc_slpc.c
> index 974a3c0..bc2c717 100644
> --- a/drivers/gpu/drm/i915/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/intel_guc_slpc.c
> @@ -163,6 +163,211 @@ static void slpc_shared_data_init(struct  
> intel_guc_slpc *slpc)
>  	kunmap_atomic(data);
>  }
> +static const char *slpc_status_stringify(int status)

I guess this should be defined as:

static const char *slpc_status_stringify(enum slpc_status status)
or
static const char *stringify_slpc_status(enum slpc_status status)

but I'm wondering if we really need to be that user friendly,
as all these errors are unlikely and will be interpreted by the
professionals that can match error value with the enum name in code;)

> +{
> +	const char *str = NULL;
> +
> +	switch(status) {
> +	case SLPC_STATUS_OK:
> +		str = "Ok";
> +		break;
> +	case SLPC_STATUS_ERROR:
> +		str = "Error";
> +		break;
> +	case SLPC_STATUS_ILLEGAL_COMMAND:
> +		str = "Illegal command";
> +		break;
> +	case SLPC_STATUS_INVALID_ARGS:
> +		str = "Invalid args";
> +		break;
> +	case SLPC_STATUS_INVALID_PARAMS:
> +		str = "Invalid params";
> +		break;
> +	case SLPC_STATUS_INVALID_DATA:
> +		str = "Invalid data";
> +		break;
> +	case SLPC_STATUS_OUT_OF_RANGE:
> +		str = "Out of range";
> +		break;
> +	case SLPC_STATUS_NOT_SUPPORTED:
> +		str = "Not supported";
> +		break;
> +	case SLPC_STATUS_NOT_IMPLEMENTED:
> +		str = "Not implemented";
> +		break;
> +	case SLPC_STATUS_NO_DATA:
> +		str = "No data";
> +		break;
> +	case SLPC_STATUS_EVENT_NOT_REGISTERED:
> +		str = "Event not registered";
> +		break;
> +	case SLPC_STATUS_REGISTER_LOCKED:
> +		str = "Register locked";
> +		break;
> +	case SLPC_STATUS_TEMPORARILY_UNAVAILABLE:
> +		str = "Temporarily unavailable";
> +		break;
> +	case SLPC_STATUS_VALUE_ALREADY_SET:
> +		str = "Value already set";
> +		break;
> +	case SLPC_STATUS_VALUE_ALREADY_UNSET:
> +		str = "Value already unset";
> +		break;
> +	case SLPC_STATUS_VALUE_NOT_CHANGED:
> +		str = "Value not changed";
> +		break;
> +	case SLPC_STATUS_MEMIO_ERROR:
> +		str = "MMIO error";
> +		break;
> +	case SLPC_STATUS_EVENT_QUEUED_REQ_DPC:
> +		str = "Event queued, DPC requested";
> +		break;
> +	case SLPC_STATUS_EVENT_QUEUED_NOREQ_DPC:
> +		str = "Event queued, DPC not requested";
> +		break;
> +	case SLPC_STATUS_NO_EVENT_QUEUED:
> +		str = "Event not queued";
> +		break;
> +	case SLPC_STATUS_OUT_OF_SPACE:
> +		str = "Out of space";
> +		break;
> +	case SLPC_STATUS_TIMEOUT:
> +		str = "Timeout";
> +		break;
> +	case SLPC_STATUS_NO_LOCK:
> +		str = "No lock";
> +		break;
> +	}
> +
> +	return str;
> +}
> +
> +static void slpc_send(struct intel_guc_slpc *slpc,
> +		      struct slpc_event_input *input, u32 in_len)
> +{
> +	struct intel_guc *guc = slpc_to_guc(slpc);
> +	struct slpc_event_output output;
> +	u32 out_len = 1;
> +	u32 *action;
> +	int ret;
> +
> +	action = (u32 *)input;

maybe better to be explicit and use:

	action = input->h2g_action_id;

> +	action[0] = INTEL_GUC_ACTION_SLPC_REQUEST;
> +
> +	ret = intel_guc_send_and_receive(guc, action, in_len,
> +					 (u32 *)&output.header, out_len);
> +
> +	/*
> +	 * Currently output data from Host to GuC SLPC actions is populated
> +	 * in scratch registers SOFT_SCRATCH(1) to SOFT_SCRATCH(14) based

hmm, this comment will be wrong if we use CT as communication method

> +	 * on event. Currently only SLPC action status in GuC is meaningful
> +	 * as Host can query only overridden parameters and that are fetched
> +	 * from Host-GuC SLPC shared data. Other parameters can be read through
> +	 * output data but that is available only for debug GuC firmwares.
> +	 */
> +	if (!ret) {
> +		ret = output.header.status;
> +		if (ret) {
> +			GEM_BUG_ON(ret >= SLPC_STATUS_MAX);
> +			DRM_ERROR("event 0x%x status %s\n",
> +				  ((output.header.value & 0xFF00) >> 8),
> +				  slpc_status_stringify(ret));
> +		}
> +	}
> +}
> +
> +static void 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};
> +
> +	data.header.value = SLPC_EVENT(SLPC_EVENT_RESET, 2);

maybe s/SLPC_EVENT/MAKE_SLPC_EVENT_HEADER ?

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

you don't need to pass 4 explicitly, instead let slpc_send() use:

	input->num_args

> +}
> +
> +static void 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);
> +	data.args[0] = shared_data_gtt_offset;

no need for local var, use:

	data.args[0] = intel_guc_ggtt_offset(guc, slpc->vma);

> +	data.args[1] = 0;
> +
> +	slpc_send(slpc, &data, 4);
> +}
> +
> +static void slpc_read_shared_data(struct intel_guc_slpc *slpc,
> +				  struct slpc_shared_data *data)
> +{
> +	struct page *page;
> +	void *pv = NULL;
> +
> +	host2guc_slpc_query_task_state(slpc);
> +
> +	page = i915_vma_first_page(slpc->vma);
> +	pv = kmap_atomic(page);

maybe it would be better to kmap once and keep pv as part of slpc?

> +
> +	drm_clflush_virt_range(pv, sizeof(struct slpc_shared_data));
> +	memcpy(data, pv, sizeof(struct slpc_shared_data));
> +
> +	kunmap_atomic(pv);
> +}
> +
> +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 *slpc_get_state(struct intel_guc_slpc *slpc)

to be more flexible, I would rather define this helper as:

static enum slpc_global_state slpc_get_state(struct intel_guc_slpc *slpc)

to let the caller decide later if he want value or string

> +{
> +	struct slpc_shared_data data;
> +
> +	slpc_read_shared_data(slpc, &data);
> +
> +	return slpc_state_stringify(data.global_state);
> +}
> +
> +static bool slpc_running(struct intel_guc_slpc *slpc)
> +{
> +	struct slpc_shared_data data;
> +
> +	slpc_read_shared_data(slpc, &data);
> +
> +	return (data.global_state == SLPC_GLOBAL_STATE_RUNNING);
> +}
> +
>  /**
>   * intel_guc_slpc_init() - Initialize the SLPC shared data structure.
>   * @slpc: pointer to intel_guc_slpc.
> @@ -204,8 +409,42 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>  	return 0;
>  }
> +/**
> + * intel_guc_slpc_enable() - Start SLPC.
> + * @slpc: pointer to intel_guc_slpc.
> + *
> + * This function will start GuC SLPC by sending Host to GuC action.
> + * SLPC shared data has to be initialized prior to this by
> + * intel_guc_slpc_init().
> + *
> + * Return: 0 on success, non-zero error code on failure.
> + */
>  int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>  {
> +	struct slpc_shared_data *data;
> +	struct page *page;
> +
> +	mutex_lock(&slpc->lock);
> +
> +	page = i915_vma_first_page(slpc->vma);
> +	data = kmap_atomic(page);
> +	data->global_state = SLPC_GLOBAL_STATE_NOT_RUNNING;
> +	kunmap_atomic(data);
> +
> +	host2guc_slpc_reset(slpc);

please add error check

> +
> +	/* Check whether SLPC is running */
> +	if (wait_for(slpc_running(slpc), 5)) {

hmm, it looks that reading shared data will be done many times
in this function ... why we can't add H2G SLPC command that will
return with status that will match 'global' status, not just ack?

> +		DRM_ERROR("SLPC not enabled! State = %s\n",
> +			  slpc_get_state(slpc));
> +		mutex_unlock(&slpc->lock);
> +		return -EIO;
> +	}
> +
> +	DRM_INFO("SLPC state: %s\n", slpc_get_state(slpc));
> +
> +	mutex_unlock(&slpc->lock);
> +
>  	return 0;
>  }
>

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