[1/2] drm/i915/guc: Update to GuC v45.0.0

Commit Message

John Harrison Aug. 7, 2020, 5:46 p.m. UTC

From: John Harrison <[e-mail]>

Update to the latest GuC firmware. This includes some significant
changes to the interface.

Signed-off-by: John Harrison <[e-mail]>
Author: Daniele Ceraolo Spurio <[e-mail]>
Author: John Harrison <[e-mail]>
Author: Matthew Brost <[e-mail]>
Author: Michal Wajdeczko <[e-mail]>
Author: Michel Thierry <[e-mail]>
Author: Oscar Mateo <[e-mail]>
Author: Rodrigo Vivi <[e-mail]>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   3 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.c       | 125 +++++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h   | 215 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c   | 116 ++++++++--
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c    |  21 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h  | 110 ++++------
 drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h   |   5 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  27 ++-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   2 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h |   6 +-
 10 files changed, 485 insertions(+), 145 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h

Comments

Daniele Ceraolo Spurio Aug. 11, 2020, 1:04 a.m. UTC | #1

On 8/7/2020 10:46 AM, [e-mail] wrote:
> From: John Harrison <[e-mail]>
>
> Update to the latest GuC firmware. This includes some significant
> changes to the interface.

A high level overview of the changes would be nice for extra clarity. A 
couple of example:

- GuC now requires a private memory area
- you're dropping the programming of the reg_state struct, but that's 
actually still defined in the structure, so IMO it's worth mentioning 
that it will come back with GuC submission.

>
> Signed-off-by: John Harrison <[e-mail]>
> Author: Daniele Ceraolo Spurio <[e-mail]>
> Author: John Harrison <[e-mail]>
> Author: Matthew Brost <[e-mail]>
> Author: Michal Wajdeczko <[e-mail]>
> Author: Michel Thierry <[e-mail]>
> Author: Oscar Mateo <[e-mail]>
> Author: Rodrigo Vivi <[e-mail]>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   3 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc.c       | 125 +++++++----
>   drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h   | 215 +++++++++++++++++++
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c   | 116 ++++++++--
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c    |  21 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h  | 110 ++++------
>   drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h   |   5 +
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  27 ++-
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   2 +
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h |   6 +-
>   10 files changed, 485 insertions(+), 145 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index ea4ba2afe9f9..9cd62d68abc6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -305,8 +305,9 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>   	engine->i915 = i915;
>   	engine->gt = gt;
>   	engine->uncore = gt->uncore;
> -	engine->hw_id = engine->guc_id = info->hw_id;
>   	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> +	engine->hw_id = info->hw_id;
> +	engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
>   
>   	engine->class = info->class;
>   	engine->instance = info->instance;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 861657897c0f..8d30e1d7d8a6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -7,6 +7,7 @@
>   #include "gt/intel_gt_irq.h"
>   #include "gt/intel_gt_pm_irq.h"
>   #include "intel_guc.h"
> +#include "intel_guc_abi.h"
>   #include "intel_guc_ads.h"
>   #include "intel_guc_submission.h"
>   #include "i915_drv.h"
> @@ -213,23 +214,6 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
>   	return flags;
>   }
>   
> -static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
> -{
> -	u32 flags = 0;
> -
> -	if (intel_guc_submission_is_used(guc)) {
> -		u32 ctxnum, base;
> -
> -		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
> -		ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
> -
> -		base >>= PAGE_SHIFT;
> -		flags |= (base << GUC_CTL_BASE_ADDR_SHIFT) |
> -			(ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
> -	}
> -	return flags;
> -}
> -
>   static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
>   {
>   	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
> @@ -291,7 +275,6 @@ static void guc_init_params(struct intel_guc *guc)
>   
>   	BUILD_BUG_ON(sizeof(guc->params) != GUC_CTL_MAX_DWORDS * sizeof(u32));
>   
> -	params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
>   	params[GUC_CTL_LOG_PARAMS] = guc_ctl_log_params_flags(guc);
>   	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
>   	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
> @@ -400,32 +383,65 @@ void intel_guc_fini(struct intel_guc *guc)
>   	intel_uc_fw_fini(&guc->fw);
>   }
>   
> +static bool is_valid_mmio_action(u32 action)
> +{
> +	return action == GUC_ACTION_REGISTER_CTB ||
> +	       action == GUC_ACTION_DEREGISTER_CTB;
> +}
> +
> +static int guc_status_to_errno(u32 status)
> +{
> +	switch (status) {
> +	case GUC_STATUS_UNKNOWN_ACTION:
> +		return -EOPNOTSUPP;
> +	case GUC_STATUS_INVALID_PARAMS:
> +		return -EINVAL;
> +	case GUC_STATUS_INVALID_ADDR:
> +		return -EFAULT;
> +	case GUC_STATUS_CTX_NOT_REGISTERED:
> +		return -ESRCH;
> +	case GUC_STATUS_CTB_FULL:
> +		return -ENOSPC;
> +	case GUC_STATUS_UNAUTHORIZED_REQUEST:
> +		return -EACCES;
> +	case GUC_STATUS_GENERIC_FAIL:
> +	default:
> +		return -ENXIO;
> +	}
> +}
> +
>   /*
>    * This function implements the MMIO based host to GuC interface.
>    */
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request, u32 len,
>   			u32 *response_buf, u32 response_buf_size)
>   {
> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>   	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
> -	u32 status;
> +	u32 action = FIELD_GET(GUC_MMIO_REQUEST_ACTION, *request);
> +	u32 subcode = FIELD_GET(GUC_MMIO_REQUEST_SUBCODE, *request);
> +	u32 magic = GUC_MMIO_MSG_MAGIC_DEFAULT;
> +	u32 header;
>   	int i;
>   	int ret;
>   
>   	GEM_BUG_ON(!len);
>   	GEM_BUG_ON(len > guc->send_regs.count);
>   
> -	/* We expect only action code */
> -	GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
> +	/* We expect to use MMIO only for special actions */
> +	GEM_BUG_ON(!is_valid_mmio_action(action));
>   
> -	/* If CT is available, we expect to use MMIO only during init/fini */
> -	GEM_BUG_ON(*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> -		   *action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> +	header = FIELD_PREP(GUC_MMIO_MSG_TYPE, GUC_MMIO_MSG_TYPE_REQUEST) |
> +		FIELD_PREP(GUC_MMIO_MSG_MAGIC, magic) |
> +		FIELD_PREP(GUC_MMIO_REQUEST_SUBCODE, subcode) |
> +		FIELD_PREP(GUC_MMIO_REQUEST_ACTION, action);
>   
>   	mutex_lock(&guc->send_mutex);
>   	intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);
>   
> -	for (i = 0; i < len; i++)
> -		intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]);
> +	intel_uncore_write(uncore, guc_send_reg(guc, 0), header);
> +	for (i = 1; i < len; i++)
> +		intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);
>   
>   	intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));
>   
> @@ -437,17 +453,45 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>   	 */
>   	ret = __intel_wait_for_register_fw(uncore,
>   					   guc_send_reg(guc, 0),
> -					   INTEL_GUC_MSG_TYPE_MASK,
> -					   INTEL_GUC_MSG_TYPE_RESPONSE <<
> -					   INTEL_GUC_MSG_TYPE_SHIFT,
> -					   10, 10, &status);
> -	/* If GuC explicitly returned an error, convert it to -EIO */
> -	if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
> -		ret = -EIO;
> +					   GUC_MMIO_MSG_ORIGIN,
> +					   FIELD_PREP(GUC_MMIO_MSG_ORIGIN,
> +						      GUC_MMIO_MSG_ORIGIN_GUC),
> +					   10, 10, &header);
> +	if (unlikely(ret))
> +		goto out;
>   
> -	if (ret) {
> -		DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
> -			  action[0], ret, status);
> +	if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
> +		ret = -ESTALE;
> +		goto out;
> +	}

I think we should move this after the check for busyness. IIRC the 
protocol says we need to wait for the GuC to stop being busy before 
sending more messages. That way you can also unify this with the other 
similar check below.

> +
> +	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) == GUC_MMIO_MSG_TYPE_BUSY) {
> +#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc, 0)); \
> +		FIELD_GET(GUC_MMIO_MSG_TYPE, header) != GUC_MMIO_MSG_TYPE_BUSY; })
> +		ret = wait_for(done, 1000);
> +		if (unlikely(ret))
> +			goto out;
> +		if (unlikely(FIELD_GET(GUC_MMIO_MSG_ORIGIN, header) !=
> +				       GUC_MMIO_MSG_ORIGIN_GUC)) {

This check on GUC_MMIO_MSG_ORIGIN is redundant. We already waited for it 
to be GUC_MMIO_MSG_ORIGIN_GUC during the wait above and we're not 
sending other message from i915 while we're waiting for this one, so 
that field can't change.

> +			ret = -EPROTO;
> +			goto out;
> +		}
> +		if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
> +			ret = -ESTALE;
> +			goto out;
> +		}
> +#undef done
> +	}
> +
> +	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) == GUC_MMIO_MSG_TYPE_ERROR) {
> +		u32 status = FIELD_GET(GUC_MMIO_ERROR_STATUS, header);
> +
> +		ret = guc_status_to_errno(status);
> +		goto out;
> +	}
> +
> +	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) != GUC_MMIO_MSG_TYPE_RESPONSE) {
> +		ret = -EPROTO;
>   		goto out;
>   	}
>   
> @@ -460,12 +504,17 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>   	}
>   
>   	/* Use data from the GuC response as our return value */
> -	ret = INTEL_GUC_MSG_TO_DATA(status);

INTEL_GUC_MSG_TO_DATA is still used in intel_guc_ct.c. Either stick to 
that or update the other use-case to the new define.

> +	ret = FIELD_GET(GUC_MMIO_RESPONSE_DATA0, header);
>   
>   out:
>   	intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
>   	mutex_unlock(&guc->send_mutex);
>   
> +	if (unlikely(ret < 0))
> +		drm_err(&i915->drm,
> +			"GuC MMIO action %#06x failed with error %d %#x\n",
> +			*request, ret, header);
> +
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
> new file mode 100644
> index 000000000000..95043788e0ad
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
> @@ -0,0 +1,215 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#ifndef __INTEL_GUC_ABI_H__
> +#define __INTEL_GUC_ABI_H__
> +
> +/**
> + * DOC: GuC MMIO based communication
> + *
> + * The MMIO based communication between Host and GuC relies on special
> + * hardware registers which format could be defined by the software
> + * (so called scratch registers).
> + *
> + * Each MMIO based message, both Host to GuC (H2G) and GuC to Host (G2H)
> + * messages, which maximum length depends on number of available scratch
> + * registers, is directly written into those scratch registers.
> + *
> + * For Gen9+, there are 16 software scratch registers 0xC180-0xC1B8,
> + * but no H2G command takes more than 8 parameters and the GuC firmware
> + * itself uses an 8-element array to store the H2G message.
> + *
> + * For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
> + * are, regardless on lower count, preffered over legacy ones.
> + *
> + * The MMIO based communication is mainly used during driver initialization
> + * phase to setup the CTB based communication that will be used afterwards.
> + *
> + * Details of the messages formats depend on GuC firmware version being used
> + * by the host driver. Documented here messages are for GuC 45.0 and newer.
> + */
> +
> +#define GUC_MAX_MMIO_MSG_LEN			8

Are we ok with not having an INTEL_ prefix to all the defines? I'm not 
complaining, we usually stick with that prefix but I'm not sure if it is 
an iron rule or not.

> +
> +/**
> + * DOC: GuC MMIO message format
> + *
> + * Bits of the first scratch register are treated as a message header,
> + * and other registers values are used to hold message payload (data)::
> + *
> + *      +=======================================================+
> + *      |  SCRATCH  |                                           |
> + *      +-----------+-------------------------------------------+
> + *      | 0 | 31:28 | message ORIGIN/TYPE                       |
> + *      |   | 27:24 | message MAGIC                             |
> + *      |   |  23:0 | message DATA0 (depends on TYPE)           |
> + *      +---+-------+-------------------------------------------+
> + *      | 1 |  31:0 | message DATA1 (depends on TYPE)           |
> + *      |...|       | message DATA2 (depends on TYPE)           |
> + *      | n |       | message DATA3 (depends on TYPE)           |
> + *      +=======================================================+
> + *
> + * Where:
> + *  - **TYPE** is a 4b message type identifier.
> + *  - **MAGIC** is a 4b message sequence number.
> + *    The same sequence number must be included in all related messages.
> + *    This field is used for tracking and debug purposes.

I don't think "magic" is a clear enough name for the role of the field. 
"Request id" or something like that would be more appropriate IMO.

> + *  - **DATA0..3** bits represents message payload.
> + *    Definitions of these bits depends on message **TYPE**.
> + *
> + * The MSB of the header and **TYPE** indicates origin of the message:
> + *  - 0 - message from the Host
> + *  - 1 - message from the GuC
> + *
> + * Currently supported message types are:
> + *  - 0x0 - **REQUEST** - represents Host request message to the GuC
> + *  - 0xF - **SUCCESS RESPONSE** - GuC reply for the earlier request
> + *  - 0xE - **ERROR RESPONSE** - GuC failure reply for the earlier request
> + *  - 0xB - **BUSY** - GuC will be processing request for the longer time
> + */
> +
> +#define GUC_MMIO_MSG_ORIGIN			(0x1 << 31)
> +#define   GUC_MMIO_MSG_ORIGIN_HOST		0u
> +#define   GUC_MMIO_MSG_ORIGIN_GUC		1u
> +
> +#define GUC_MMIO_MSG_TYPE			(0xF << 28)
> +#define   GUC_MMIO_MSG_TYPE_REQUEST		0x0
> +#define   GUC_MMIO_MSG_TYPE_RESPONSE		0xF
> +#define   GUC_MMIO_MSG_TYPE_ERROR		0xE
> +#define   GUC_MMIO_MSG_TYPE_BUSY		0xB
> +
> +#define GUC_MMIO_MSG_MAGIC			(0xF << 24)
> +#define   GUC_MMIO_MSG_MAGIC_DEFAULT		0u
> +
> +/**
> + * DOC: GuC MMIO H2G REQUEST
> + *
> + * The MMIO H2G REQUEST message is used by the Host to request some action
> + * to be performed by the GuC::
> + *
> + *      +=======================================================+
> + *      |  SCRATCH  |                                           |
> + *      +=======================================================+
> + *      | 0 | 31:28 | message TYPE = 0x0 = REQUEST              |
> + *      |   | 27:24 | message MAGIC                             |
> + *      |   | 23:16 | request SUBCODE (depends on ACTION)       |
> + *      |   |  15:0 | request ACTION code                       |
> + *      +-------------------------------------------------------+
> + *      | 1 |  31:0 | request DATA1 (depends on ACTION/SUBCODE) |
> + *      |...|       | request DATA2 (depends on ACTION/SUBCODE) |
> + *      | n |  31:0 | request DATA3 (depends on ACTION/SUBCODE) |
> + *      +=======================================================+
> + *
> + * Where:
> + *  - **TYPE** must be set to value 0x0.
> + *  - **MAGIC** message sequence number is generated by the host.
> + *  - **ACTION** represents 16b request action code that defines both
> + *    purpose of the request and format of the passed payload data.
> + *    List of supported codes depends on GuC version and plaform.
> + *    Action code can't be zero.
> + *  - **SUBCODE** is optional 8b data related to the **ACTION**.
> + *    MBZ if not explicitly defined by given **ACTION**.
> + *  - **DATA1..3** are optional 32b payload dwords related to **ACTION**.
> + *    Format of each dword is defined by specific **ACTION**.
> + */
> +
> +#define GUC_MMIO_REQUEST_SUBCODE		(0xFF << 16)
> +#define GUC_MMIO_REQUEST_ACTION			(0xFFFF << 0)
> +#define   GUC_ACTION_INVALID			0x0000
> +#define   GUC_ACTION_REGISTER_CTB		0x4505
> +#define   GUC_ACTION_DEREGISTER_CTB		0x4506

These are now defined twice (once here and once in the enum). We should 
pick one for consistency.

> +
> +/**
> + * DOC: GuC MMIO G2H SUCCESS RESPONSE
> + *
> + * The MMIO G2H SUCCESS RESPONSE message is used by the GuC to reply to
> + * the earlier H2G request message. This message is used if no errors
> + * were encountered by the GuC during processing of the request::
> + *
> + *      +=======================================================+
> + *      |  SCRATCH  |                                           |
> + *      +=======================================================+
> + *      | 0 | 31:28 | message TYPE = 0xF = SUCCESS RESPONSE     |
> + *      |   | 27:24 | message MAGIC                             |
> + *      |   |  23:0 | response DATA0 (depends on ACTION/SUBCODE)|
> + *      +-------------------------------------------------------+
> + *      | 1 |  31:0 | response DATA1 (depends on ACTION/SUBCODE)|
> + *      |...|       | response DATA2 (depends on ACTION/SUBCODE)|
> + *      | n |  31:0 | response DATA3 (depends on ACTION/SUBCODE)|
> + *      +=======================================================+
> + *
> + * Where:
> + *  - **TYPE** must be set to value 0xF.
> + *  - **MAGIC** must match value used by the host in **REQUEST** message.
> + *  - **DATA** is optional payload data related to **ACTION**.
> + *    Format is defined by each **ACTION** separately.
> + */
> +
> +#define GUC_MMIO_RESPONSE_DATA0			(0xFFFFFF << 0)
> +
> +/**
> + * DOC: GuC MMIO G2H ERROR RESPONSE
> + *
> + * The MMIO G2H ERROR RESPONSE message is used by the GuC to reply to
> + * the earlier H2G request message. This message is used if some errors
> + * were encountered by the GuC during processing of the request::
> + *
> + *      +=======================================================+
> + *      |  SCRATCH  |                                           |
> + *      +=======================================================+
> + *      | 0 | 31:28 | message TYPE = 0xE = ERROR RESPONSE       |
> + *      |   | 27:24 | message MAGIC                             |
> + *      |   | 23:16 | reserved MBZ                              |
> + *      |   |  15:0 | response STATUS failure code              |
> + *      +-------------------------------------------------------+
> + *      | 1 |  31:0 | reserved MBZ                              |
> + *      |...|       | reserved MBZ                              |
> + *      | n |  31:0 | reserved MBZ                              |
> + *      +=======================================================+
> + *
> + * Where:
> + *  - **TYPE** must be set to value 0xE.
> + *  - **MAGIC** must match value used by the host in **REQUEST** message.
> + *  - **STATUS** represents non-zero failure code.
> + */
> +
> +#define GUC_MMIO_ERROR_STATUS			(0xFFFF << 0)
> +#define   GUC_STATUS_UNKNOWN_ACTION		0x30
> +#define   GUC_STATUS_INVALID_PARAMS		0x60
> +#define   GUC_STATUS_INVALID_ADDR		0x80
> +#define   GUC_STATUS_CTX_NOT_REGISTERED		0x100
> +#define   GUC_STATUS_NO_LOCAL_MEMORY		0x200
> +#define   GUC_STATUS_NO_VIRTUALIZATION		0x300
> +#define   GUC_STATUS_CTB_FULL			0x301
> +#define   GUC_STATUS_UNAUTHORIZED_REQUEST	0x302
> +#define   GUC_STATUS_GENERIC_FAIL		0xF000
> +
> +/**
> + * DOC: MMIO G2H BUSY RESPONSE
> + *
> + * The MMIO G2H BUSY RESPONSE message is used by the GuC to reply to
> + * the earlier H2G request message. This message is used if processing
> + * of the request will take longer time and final SUCCESS/ERROR response
> + * message will be delivered later::
> + *
> + *      +=======================================================+
> + *      |  SCRATCH  |                                           |
> + *      +=======================================================+
> + *      | 0 | 31:28 | message TYPE = 0xB = BUSY RESPONSE        |
> + *      |   | 27:24 | message MAGIC                             |
> + *      |   |  23:0 | reserved MBZ                              |
> + *      +-------------------------------------------------------+
> + *      | 1 |  31:0 | reserved MBZ                              |
> + *      |...|       | reserved MBZ                              |
> + *      | n |  31:0 | reserved MBZ                              |
> + *      +=======================================================+
> + *
> + * Where:
> + *  - **TYPE** must be set to value 0xB.
> + *  - **MAGIC** must match value used by the host in **REQUEST** message.
> + *  - all other bits are reserved as MBZ.
> + */
> +
> +#endif /* __INTEL_GUC_ABI_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index d44061033f23..e66cbf1be320 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -10,11 +10,52 @@
>   
>   /*
>    * The Additional Data Struct (ADS) has pointers for different buffers used by
> - * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
> - * scheduling policies (guc_policies), a structure describing a collection of
> - * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
> - * its internal state for sleep.
> + * the GuC. One single gem object contains the ADS struct itself (guc_ads) and
> + * all the extra buffers indirectly linked via the ADS struct's entires.
> + *
> + * Layout of the ADS blob allocated for the GuC:
> + *
> + *      +---------------------------------------+ <== base
> + *      | guc_ads                               |
> + *      +---------------------------------------+
> + *      | guc_policies                          |
> + *      +---------------------------------------+
> + *      | guc_gt_system_info                    |
> + *      +---------------------------------------+
> + *      | guc_clients_info                      |
> + *      +---------------------------------------+
> + *      | guc_ct_pool_entry[size]               |
> + *      +---------------------------------------+
> + *      | padding                               |
> + *      +---------------------------------------+ <== 4K aligned
> + *      | private data                          |
> + *      +---------------------------------------+
> + *      | padding                               |
> + *      +---------------------------------------+ <== 4K aligned
>    */

I think it we could expand the comment (or add a new one) to make clear 
that the private data is not included in the __guc_ads_blob to keep 
things simple since it is variable in size, but it is still part of the 
ads_blobpasses to GuC.

> +struct __guc_ads_blob {
> +	struct guc_ads ads;
> +	struct guc_policies policies;
> +	struct guc_gt_system_info system_info;
> +	struct guc_clients_info clients_info;
> +	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
> +} __packed;
> +
> +static u32 guc_ads_private_data_size(struct intel_guc *guc)
> +{
> +	return PAGE_ALIGN(guc->fw.private_data_size);
> +}
> +
> +static u32 guc_ads_private_data_offset(struct intel_guc *guc)
> +{
> +	return PAGE_ALIGN(sizeof(struct __guc_ads_blob));
> +}
> +
> +static u32 guc_ads_blob_size(struct intel_guc *guc)
> +{
> +	return guc_ads_private_data_offset(guc) +
> +	       guc_ads_private_data_size(guc);
> +}
>   
>   static void guc_policy_init(struct guc_policy *policy)
>   {
> @@ -48,23 +89,33 @@ static void guc_ct_pool_entries_init(struct guc_ct_pool_entry *pool, u32 num)
>   	memset(pool, 0, num * sizeof(*pool));
>   }
>   
> +static void guc_mapping_table_init(struct intel_gt *gt,
> +				   struct guc_gt_system_info *system_info)
> +{
> +	unsigned int i, j;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	/* Table must be set to invalid values for entries not used */
> +	for (i = 0; i < GUC_MAX_ENGINE_CLASSES; ++i)
> +		for (j = 0; j < GUC_MAX_INSTANCES_PER_CLASS; ++j)
> +			system_info->mapping_table[i][j] =
> +				GUC_MAX_INSTANCES_PER_CLASS;
> +
> +	for_each_engine(engine, gt, id) {
> +		u8 guc_class = engine->class;
> +
> +		system_info->mapping_table[guc_class][engine->instance]
> +			= engine->instance;
> +	}
> +}
> +
>   /*
>    * The first 80 dwords of the register state context, containing the
>    * execlists and ppgtt registers.
>    */
>   #define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
>   
> -/* The ads obj includes the struct itself and buffers passed to GuC */
> -struct __guc_ads_blob {
> -	struct guc_ads ads;
> -	struct guc_policies policies;
> -	struct guc_mmio_reg_state reg_state;
> -	struct guc_gt_system_info system_info;
> -	struct guc_clients_info clients_info;
> -	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
> -	u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> -} __packed;
> -
>   static void __guc_ads_init(struct intel_guc *guc)
>   {
>   	struct intel_gt *gt = guc_to_gt(guc);
> @@ -107,6 +158,16 @@ static void __guc_ads_init(struct intel_guc *guc)
>   	blob->system_info.vebox_enable_mask = VEBOX_MASK(gt);
>   	blob->system_info.vdbox_sfc_support_mask = gt->info.vdbox_sfc_access;
>   
> +	if (INTEL_GEN(gt->i915) >= 12 && !IS_DGFX(gt->i915)) {
> +		u32 distdbreg = intel_uncore_read(gt->uncore,
> +						  GEN12_DIST_DBS_POPULATED);
> +		blob->system_info.num_of_doorbells_per_sqidi =
> +			((distdbreg >> GEN12_DOORBELLS_PER_SQIDI_SHIFT) &
> +			 GEN12_DOORBELLS_PER_SQIDI) + 1;
> +	}
> +
> +	guc_mapping_table_init(guc_to_gt(guc), &blob->system_info);
> +
>   	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
>   
>   	/* Clients info  */
> @@ -118,11 +179,12 @@ static void __guc_ads_init(struct intel_guc *guc)
>   
>   	/* ADS */
>   	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
> -	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
> -	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
>   	blob->ads.gt_system_info = base + ptr_offset(blob, system_info);
>   	blob->ads.clients_info = base + ptr_offset(blob, clients_info);
>   
> +	/* Private Data */
> +	blob->ads.private_data = base + guc_ads_private_data_offset(guc);
> +
>   	i915_gem_object_flush_map(guc->ads_vma->obj);
>   }
>   
> @@ -135,14 +197,15 @@ static void __guc_ads_init(struct intel_guc *guc)
>    */
>   int intel_guc_ads_create(struct intel_guc *guc)
>   {
> -	const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
> +	u32 size;
>   	int ret;
>   
>   	GEM_BUG_ON(guc->ads_vma);
>   
> +	size = guc_ads_blob_size(guc);
> +
>   	ret = intel_guc_allocate_and_map_vma(guc, size, &guc->ads_vma,
>   					     (void **)&guc->ads_blob);
> -
>   	if (ret)
>   		return ret;
>   
> @@ -156,6 +219,18 @@ void intel_guc_ads_destroy(struct intel_guc *guc)
>   	i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP);
>   }
>   
> +void guc_ads_private_data_reset(struct intel_guc *guc)
> +{
> +	u32 size;
> +
> +	size = guc_ads_private_data_size(guc);
> +	if (!size)
> +		return;
> +
> +	memset((void *)guc->ads_blob + guc_ads_private_data_offset(guc), 0,
> +	       size);
> +}
> +
>   /**
>    * intel_guc_ads_reset() - prepares GuC Additional Data Struct for reuse
>    * @guc: intel_guc struct
> @@ -168,5 +243,8 @@ void intel_guc_ads_reset(struct intel_guc *guc)
>   {
>   	if (!guc->ads_vma)
>   		return;
> +
>   	__guc_ads_init(guc);
> +
> +	guc_ads_private_data_reset(guc);
>   }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> index d4a87f4c9421..212de0a939bf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -74,8 +74,9 @@ static inline bool guc_ready(struct intel_uncore *uncore, u32 *status)
>   		((val & GS_MIA_CORE_STATE) && (uk_val == GS_UKERNEL_LAPIC_DONE));
>   }
>   
> -static int guc_wait_ucode(struct intel_uncore *uncore)
> +static int guc_wait_ucode(struct intel_gt *gt)
>   {
> +	struct intel_uncore *uncore = gt->uncore;

The gt seems to be used only for gt->i915, why not just use uncore->i915?

>   	u32 status;
>   	int ret;
>   
> @@ -93,12 +94,24 @@ static int guc_wait_ucode(struct intel_uncore *uncore)
>   	if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>   		DRM_ERROR("GuC firmware signature verification failed\n");
>   		ret = -ENOEXEC;
> -	}
> -
> +	} else
>   	if ((status & GS_UKERNEL_MASK) == GS_UKERNEL_EXCEPTION) {
>   		DRM_ERROR("GuC firmware exception. EIP: %#x\n",
>   			  intel_uncore_read(uncore, SOFT_SCRATCH(13)));
>   		ret = -ENXIO;
> +	} else
> +	if (ret) {
> +		i915_probe_error(gt->i915, "GuC load failed: status = 0x%08X\n",
> +				 status);
> +		i915_probe_error(gt->i915, "GuC status: Reset = %d, "
> +				 "BootROM = 0x%02X, UKernel = 0x%02X, "
> +				 "MIA = 0x%02X, Auth = 0x%02X\n",
> +				 (status >> GS_RESET_SHIFT) & 1,
> +				 (status & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT,
> +				 (status & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT,
> +				 (status & GS_MIA_MASK) >> GS_MIA_SHIFT,
> +				 (status & GS_AUTH_STATUS_MASK) >>
> +							  GS_AUTH_STATUS_SHIFT);

nit: I think using the FIELD_GET macro here as well could make things 
cleaner

>   	}
>   
>   	return ret;
> @@ -139,7 +152,7 @@ int intel_guc_fw_upload(struct intel_guc *guc)
>   	if (ret)
>   		goto out;
>   
> -	ret = guc_wait_ucode(uncore);
> +	ret = guc_wait_ucode(gt);
>   	if (ret)
>   		goto out;
>   
> 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 a6b733c146c9..77e49d53bb5c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -27,7 +27,7 @@
>   #define GUC_MAX_ENGINES_NUM		(GUC_VIDEO_ENGINE2 + 1)
>   
>   #define GUC_MAX_ENGINE_CLASSES		5
> -#define GUC_MAX_INSTANCES_PER_CLASS	16
> +#define GUC_MAX_INSTANCES_PER_CLASS	32
>   
>   #define GUC_DOORBELL_INVALID		256
>   
> @@ -62,12 +62,7 @@
>   #define GUC_STAGE_DESC_ATTR_PCH		BIT(6)
>   #define GUC_STAGE_DESC_ATTR_TERMINATED	BIT(7)
>   
> -/* New GuC control data */
> -#define GUC_CTL_CTXINFO			0
> -#define   GUC_CTL_CTXNUM_IN16_SHIFT	0
> -#define   GUC_CTL_BASE_ADDR_SHIFT	12
> -
> -#define GUC_CTL_LOG_PARAMS		1
> +#define GUC_CTL_LOG_PARAMS		0
>   #define   GUC_LOG_VALID			(1 << 0)
>   #define   GUC_LOG_NOTIFY_ON_HALF_FULL	(1 << 1)
>   #define   GUC_LOG_ALLOC_IN_MEGABYTE	(1 << 3)
> @@ -79,11 +74,11 @@
>   #define   GUC_LOG_ISR_MASK	        (0x7 << GUC_LOG_ISR_SHIFT)
>   #define   GUC_LOG_BUF_ADDR_SHIFT	12
>   
> -#define GUC_CTL_WA			2
> -#define GUC_CTL_FEATURE			3
> +#define GUC_CTL_WA			1
> +#define GUC_CTL_FEATURE			2
>   #define   GUC_CTL_DISABLE_SCHEDULER	(1 << 14)
>   
> -#define GUC_CTL_DEBUG			4
> +#define GUC_CTL_DEBUG			3
>   #define   GUC_LOG_VERBOSITY_SHIFT	0
>   #define   GUC_LOG_VERBOSITY_LOW		(0 << GUC_LOG_VERBOSITY_SHIFT)
>   #define   GUC_LOG_VERBOSITY_MED		(1 << GUC_LOG_VERBOSITY_SHIFT)
> @@ -97,12 +92,31 @@
>   #define   GUC_LOG_DISABLED		(1 << 6)
>   #define   GUC_PROFILE_ENABLED		(1 << 7)
>   
> -#define GUC_CTL_ADS			5
> +#define GUC_CTL_ADS			4
>   #define   GUC_ADS_ADDR_SHIFT		1
>   #define   GUC_ADS_ADDR_MASK		(0xFFFFF << GUC_ADS_ADDR_SHIFT)
>   
>   #define GUC_CTL_MAX_DWORDS		(SOFT_SCRATCH_COUNT - 2) /* [1..14] */
>   
> +/*
> + * The class goes in bits [0..2] of the GuC ID, the instance in bits [3..6].
> + * Bit 7 can be used for operations that apply to all engine classes&instances.
> + */
> +#define GUC_ENGINE_CLASS_SHIFT		0
> +#define GUC_ENGINE_CLASS_MASK		(0x7 << GUC_ENGINE_CLASS_SHIFT)
> +#define GUC_ENGINE_INSTANCE_SHIFT	3
> +#define GUC_ENGINE_INSTANCE_MASK	(0xf << GUC_ENGINE_INSTANCE_SHIFT)
> +#define GUC_ENGINE_ALL_INSTANCES	(1 << 7)
> +
> +#define MAKE_GUC_ID(class, instance) \
> +	(((class) << GUC_ENGINE_CLASS_SHIFT) | \
> +	 ((instance) << GUC_ENGINE_INSTANCE_SHIFT))
> +
> +#define GUC_ID_TO_ENGINE_CLASS(guc_id) \
> +	(((guc_id) & GUC_ENGINE_CLASS_MASK) >> GUC_ENGINE_CLASS_SHIFT)
> +#define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
> +	(((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
> +
>   /* Work item for submitting workloads into work queue of GuC. */
>   struct guc_wq_item {
>   	u32 header;
> @@ -338,7 +352,6 @@ struct guc_policies {
>   /* GuC MMIO reg state struct */
>   
>   
> -#define GUC_REGSET_MAX_REGISTERS	64
>   #define GUC_S3_SAVE_SPACE_PAGES		10
>   
>   struct guc_mmio_reg {
> @@ -348,16 +361,17 @@ struct guc_mmio_reg {
>   #define GUC_REGSET_MASKED		(1 << 0)
>   } __packed;
>   
> -struct guc_mmio_regset {
> -	struct guc_mmio_reg registers[GUC_REGSET_MAX_REGISTERS];
> -	u32 values_valid;
> -	u32 number_of_registers;
> -} __packed;
> -
>   /* GuC register sets */
> -struct guc_mmio_reg_state {
> -	struct guc_mmio_regset engine_reg[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
> -	u32 reserved[98];
> +struct guc_mmio_reg_set {
> +	u32 address;
> +	union {
> +		struct {
> +			u32 count:16;
> +			u32 reserved1:12;
> +			u32 reserved2:4;
> +		};
> +		u32 count_u32;
> +	};
>   } __packed;
>   
>   /* HW info */
> @@ -369,7 +383,9 @@ struct guc_gt_system_info {
>   	u32 vdbox_enable_mask;
>   	u32 vdbox_sfc_support_mask;
>   	u32 vebox_enable_mask;
> -	u32 reserved[9];
> +	u32 num_of_doorbells_per_sqidi;
> +	u8 mapping_table[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
> +	u32 reserved2[8];
>   } __packed;
>   
>   /* Clients info */
> @@ -390,15 +406,16 @@ struct guc_clients_info {
>   
>   /* GuC Additional Data Struct */
>   struct guc_ads {
> -	u32 reg_state_addr;
> -	u32 reg_state_buffer;
> +	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
> +	u32 reserved0;
>   	u32 scheduler_policies;
>   	u32 gt_system_info;
>   	u32 clients_info;
>   	u32 control_data;
>   	u32 golden_context_lrca[GUC_MAX_ENGINE_CLASSES];
>   	u32 eng_state_size[GUC_MAX_ENGINE_CLASSES];
> -	u32 reserved[16];
> +	u32 private_data;
> +	u32 reserved[15];
>   } __packed;
>   
>   /* GuC logging structures */
> @@ -474,49 +491,6 @@ struct guc_shared_ctx_data {
>   	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>   } __packed;
>   
> -/**
> - * DOC: MMIO based communication
> - *
> - * The MMIO based communication between Host and GuC uses software scratch
> - * registers, where first register holds data treated as message header,
> - * and other registers are used to hold message payload.
> - *
> - * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
> - * but no H2G command takes more than 8 parameters and the GuC FW
> - * itself uses an 8-element array to store the H2G message.
> - *
> - *      +-----------+---------+---------+---------+
> - *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
> - *      +-----------+---------+---------+---------+
> - *      | header    |      optional payload       |
> - *      +======+====+=========+=========+=========+
> - *      | 31:28|type|         |         |         |
> - *      +------+----+         |         |         |
> - *      | 27:16|data|         |         |         |
> - *      +------+----+         |         |         |
> - *      |  15:0|code|         |         |         |
> - *      +------+----+---------+---------+---------+
> - *
> - * The message header consists of:
> - *
> - * - **type**, indicates message type
> - * - **code**, indicates message code, is specific for **type**
> - * - **data**, indicates message data, optional, depends on **code**
> - *
> - * The following message **types** are supported:
> - *
> - * - **REQUEST**, indicates Host-to-GuC request, requested GuC action code
> - *   must be priovided in **code** field. Optional action specific parameters
> - *   can be provided in remaining payload registers or **data** field.
> - *
> - * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC request,
> - *   action response status will be provided in **code** field. Optional
> - *   response data can be returned in remaining payload registers or **data**
> - *   field.
> - */
> -
> -#define GUC_MAX_MMIO_MSG_LEN		8
> -
>   #define INTEL_GUC_MSG_TYPE_SHIFT	28
>   #define INTEL_GUC_MSG_TYPE_MASK		(0xF << INTEL_GUC_MSG_TYPE_SHIFT)
>   #define INTEL_GUC_MSG_DATA_SHIFT	16

These GUC_MSG defines are also duplicated

Daniele

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> index 1949346e714e..b37fc2ffaef2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
> @@ -118,6 +118,11 @@ struct guc_doorbell_info {
>   #define   GEN8_DRB_VALID		  (1<<0)
>   #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
>   
> +#define GEN12_DIST_DBS_POPULATED		_MMIO(0xd08)
> +#define   GEN12_DOORBELLS_PER_SQIDI_SHIFT	16
> +#define   GEN12_DOORBELLS_PER_SQIDI		(0xff)
> +#define   GEN12_SQIDIS_DOORBELL_EXIST		(0xffff)
> +
>   #define DE_GUCRMR			_MMIO(0x44054)
>   
>   #define GUC_BCS_RCS_IER			_MMIO(0xC550)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 59b27aba15c6..f7cb0b1f1ba5 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -44,23 +44,19 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>    * List of required GuC and HuC binaries per-platform.
>    * Must be ordered based on platform + revid, from newer to older.
>    *
> - * TGL 35.2 is interface-compatible with 33.0 for previous Gens. The deltas
> - * between 33.0 and 35.2 are only related to new additions to support new Gen12
> - * features.
> - *
>    * Note that RKL uses the same firmware as TGL.
>    */
>   #define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
> -	fw_def(ROCKETLAKE,  0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7, 0, 12)) \
> -	fw_def(TIGERLAKE,   0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7, 0, 12)) \
> -	fw_def(ELKHARTLAKE, 0, guc_def(ehl, 33, 0, 4), huc_def(ehl,  9, 0, 0)) \
> -	fw_def(ICELAKE,     0, guc_def(icl, 33, 0, 0), huc_def(icl,  9, 0, 0)) \
> -	fw_def(COMETLAKE,   5, guc_def(cml, 33, 0, 0), huc_def(cml,  4, 0, 0)) \
> -	fw_def(COFFEELAKE,  0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4, 0, 0)) \
> -	fw_def(GEMINILAKE,  0, guc_def(glk, 33, 0, 0), huc_def(glk,  4, 0, 0)) \
> -	fw_def(KABYLAKE,    0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4, 0, 0)) \
> -	fw_def(BROXTON,     0, guc_def(bxt, 33, 0, 0), huc_def(bxt,  2, 0, 0)) \
> -	fw_def(SKYLAKE,     0, guc_def(skl, 33, 0, 0), huc_def(skl,  2, 0, 0))
> +	fw_def(ROCKETLAKE,  0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7, 0, 12)) \
> +	fw_def(TIGERLAKE,   0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7, 0, 12)) \
> +	fw_def(ELKHARTLAKE, 0, guc_def(ehl, 45, 0, 0), huc_def(ehl,  9, 0, 0)) \
> +	fw_def(ICELAKE,     0, guc_def(icl, 45, 0, 0), huc_def(icl,  9, 0, 0)) \
> +	fw_def(COMETLAKE,   5, guc_def(cml, 45, 0, 0), huc_def(cml,  4, 0, 0)) \
> +	fw_def(COFFEELAKE,  0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4, 0, 0)) \
> +	fw_def(GEMINILAKE,  0, guc_def(glk, 45, 0, 0), huc_def(glk,  4, 0, 0)) \
> +	fw_def(KABYLAKE,    0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4, 0, 0)) \
> +	fw_def(BROXTON,     0, guc_def(bxt, 45, 0, 0), huc_def(bxt,  2, 0, 0)) \
> +	fw_def(SKYLAKE,     0, guc_def(skl, 45, 0, 0), huc_def(skl,  2, 0, 0))
>   
>   #define __MAKE_UC_FW_PATH(prefix_, name_, major_, minor_, patch_) \
>   	"i915/" \
> @@ -371,6 +367,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   		}
>   	}
>   
> +	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
> +		uc_fw->private_data_size = css->private_data_size;
> +
>   	obj = i915_gem_object_create_shmem_from_data(i915, fw->data, fw->size);
>   	if (IS_ERR(obj)) {
>   		err = PTR_ERR(obj);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 23d3a423ac0f..99bb1fe1af66 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -88,6 +88,8 @@ struct intel_uc_fw {
>   
>   	u32 rsa_size;
>   	u32 ucode_size;
> +
> +	u32 private_data_size;
>   };
>   
>   #ifdef CONFIG_DRM_I915_DEBUG_GUC
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> index 029214cdedd5..e41ffc7a7fbc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
> @@ -69,7 +69,11 @@ struct uc_css_header {
>   #define CSS_SW_VERSION_UC_MAJOR		(0xFF << 16)
>   #define CSS_SW_VERSION_UC_MINOR		(0xFF << 8)
>   #define CSS_SW_VERSION_UC_PATCH		(0xFF << 0)
> -	u32 reserved[14];
> +	u32 reserved0[13];
> +	union {
> +		u32 private_data_size; /* only applies to GuC */
> +		u32 reserved1;
> +	};
>   	u32 header_info;
>   } __packed;
>   static_assert(sizeof(struct uc_css_header) == 128);

Michal Wajdeczko Aug. 19, 2020, 9:14 p.m. UTC | #2

On 11.08.2020 03:04, Daniele Ceraolo Spurio wrote:
> 
> 
> On 8/7/2020 10:46 AM, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> Update to the latest GuC firmware. This includes some significant
>> changes to the interface.
> 
> A high level overview of the changes would be nice for extra clarity. A
> couple of example:
> 
> - GuC now requires a private memory area
> - you're dropping the programming of the reg_state struct, but that's
> actually still defined in the structure, so IMO it's worth mentioning
> that it will come back with GuC submission.
> 
>>
>> Signed-off-by: John Harrison <[e-mail]>
>> Author: Daniele Ceraolo Spurio <[e-mail]>
>> Author: John Harrison <[e-mail]>
>> Author: Matthew Brost <[e-mail]>
>> Author: Michal Wajdeczko <[e-mail]>
>> Author: Michel Thierry <[e-mail]>
>> Author: Oscar Mateo <[e-mail]>
>> Author: Rodrigo Vivi <[e-mail]>

For the review process it would be better to have individual patches
from each author. We can squash them just before merge.

>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   3 +-
>>   drivers/gpu/drm/i915/gt/uc/intel_guc.c       | 125 +++++++----
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h   | 215 +++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c   | 116 ++++++++--
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c    |  21 +-
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h  | 110 ++++------
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h   |   5 +
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  27 ++-
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   2 +
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h |   6 +-
>>   10 files changed, 485 insertions(+), 145 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index ea4ba2afe9f9..9cd62d68abc6 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -305,8 +305,9 @@ static int intel_engine_setup(struct intel_gt *gt,
>> enum intel_engine_id id)
>>       engine->i915 = i915;
>>       engine->gt = gt;
>>       engine->uncore = gt->uncore;
>> -    engine->hw_id = engine->guc_id = info->hw_id;
>>       engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
>> +    engine->hw_id = info->hw_id;
>> +    engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
>>         engine->class = info->class;
>>       engine->instance = info->instance;
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index 861657897c0f..8d30e1d7d8a6 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -7,6 +7,7 @@
>>   #include "gt/intel_gt_irq.h"
>>   #include "gt/intel_gt_pm_irq.h"
>>   #include "intel_guc.h"
>> +#include "intel_guc_abi.h"
>>   #include "intel_guc_ads.h"
>>   #include "intel_guc_submission.h"
>>   #include "i915_drv.h"
>> @@ -213,23 +214,6 @@ static u32 guc_ctl_feature_flags(struct intel_guc
>> *guc)
>>       return flags;
>>   }
>>   -static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
>> -{
>> -    u32 flags = 0;
>> -
>> -    if (intel_guc_submission_is_used(guc)) {
>> -        u32 ctxnum, base;
>> -
>> -        base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
>> -        ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
>> -
>> -        base >>= PAGE_SHIFT;
>> -        flags |= (base << GUC_CTL_BASE_ADDR_SHIFT) |
>> -            (ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
>> -    }
>> -    return flags;
>> -}
>> -
>>   static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
>>   {
>>       u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >>
>> PAGE_SHIFT;
>> @@ -291,7 +275,6 @@ static void guc_init_params(struct intel_guc *guc)
>>         BUILD_BUG_ON(sizeof(guc->params) != GUC_CTL_MAX_DWORDS *
>> sizeof(u32));
>>   -    params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
>>       params[GUC_CTL_LOG_PARAMS] = guc_ctl_log_params_flags(guc);
>>       params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
>>       params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
>> @@ -400,32 +383,65 @@ void intel_guc_fini(struct intel_guc *guc)
>>       intel_uc_fw_fini(&guc->fw);
>>   }
>>   +static bool is_valid_mmio_action(u32 action)
>> +{
>> +    return action == GUC_ACTION_REGISTER_CTB ||
>> +           action == GUC_ACTION_DEREGISTER_CTB;
>> +}
>> +
>> +static int guc_status_to_errno(u32 status)
>> +{
>> +    switch (status) {
>> +    case GUC_STATUS_UNKNOWN_ACTION:
>> +        return -EOPNOTSUPP;
>> +    case GUC_STATUS_INVALID_PARAMS:
>> +        return -EINVAL;
>> +    case GUC_STATUS_INVALID_ADDR:
>> +        return -EFAULT;
>> +    case GUC_STATUS_CTX_NOT_REGISTERED:
>> +        return -ESRCH;
>> +    case GUC_STATUS_CTB_FULL:
>> +        return -ENOSPC;
>> +    case GUC_STATUS_UNAUTHORIZED_REQUEST:
>> +        return -EACCES;
>> +    case GUC_STATUS_GENERIC_FAIL:
>> +    default:
>> +        return -ENXIO;
>> +    }
>> +}
>> +
>>   /*
>>    * This function implements the MMIO based host to GuC interface.
>>    */
>> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
>> len,
>> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request,
>> u32 len,
>>               u32 *response_buf, u32 response_buf_size)
>>   {
>> +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>>       struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
>> -    u32 status;
>> +    u32 action = FIELD_GET(GUC_MMIO_REQUEST_ACTION, *request);
>> +    u32 subcode = FIELD_GET(GUC_MMIO_REQUEST_SUBCODE, *request);
>> +    u32 magic = GUC_MMIO_MSG_MAGIC_DEFAULT;
>> +    u32 header;
>>       int i;
>>       int ret;
>>         GEM_BUG_ON(!len);
>>       GEM_BUG_ON(len > guc->send_regs.count);
>>   -    /* We expect only action code */
>> -    GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
>> +    /* We expect to use MMIO only for special actions */
>> +    GEM_BUG_ON(!is_valid_mmio_action(action));
>>   -    /* If CT is available, we expect to use MMIO only during
>> init/fini */
>> -    GEM_BUG_ON(*action !=
>> INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
>> -           *action !=
>> INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
>> +    header = FIELD_PREP(GUC_MMIO_MSG_TYPE, GUC_MMIO_MSG_TYPE_REQUEST) |
>> +        FIELD_PREP(GUC_MMIO_MSG_MAGIC, magic) |
>> +        FIELD_PREP(GUC_MMIO_REQUEST_SUBCODE, subcode) |
>> +        FIELD_PREP(GUC_MMIO_REQUEST_ACTION, action);
>>         mutex_lock(&guc->send_mutex);
>>       intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);
>>   -    for (i = 0; i < len; i++)
>> -        intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]);
>> +    intel_uncore_write(uncore, guc_send_reg(guc, 0), header);
>> +    for (i = 1; i < len; i++)
>> +        intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);
>>         intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));
>>   @@ -437,17 +453,45 @@ int intel_guc_send_mmio(struct intel_guc *guc,
>> const u32 *action, u32 len,
>>        */
>>       ret = __intel_wait_for_register_fw(uncore,
>>                          guc_send_reg(guc, 0),
>> -                       INTEL_GUC_MSG_TYPE_MASK,
>> -                       INTEL_GUC_MSG_TYPE_RESPONSE <<
>> -                       INTEL_GUC_MSG_TYPE_SHIFT,
>> -                       10, 10, &status);
>> -    /* If GuC explicitly returned an error, convert it to -EIO */
>> -    if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
>> -        ret = -EIO;
>> +                       GUC_MMIO_MSG_ORIGIN,
>> +                       FIELD_PREP(GUC_MMIO_MSG_ORIGIN,
>> +                              GUC_MMIO_MSG_ORIGIN_GUC),
>> +                       10, 10, &header);
>> +    if (unlikely(ret))
>> +        goto out;
>>   -    if (ret) {
>> -        DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
>> -              action[0], ret, status);
>> +    if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
>> +        ret = -ESTALE;
>> +        goto out;
>> +    }
> 
> I think we should move this after the check for busyness. IIRC the
> protocol says we need to wait for the GuC to stop being busy before
> sending more messages. That way you can also unify this with the other
> similar check below.

but each MMIO message should be handled separately and part of that
handling is a check of the 'magic' field, so we can't skip it here

> 
>> +
>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) ==
>> GUC_MMIO_MSG_TYPE_BUSY) {
>> +#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc,
>> 0)); \
>> +        FIELD_GET(GUC_MMIO_MSG_TYPE, header) !=
>> GUC_MMIO_MSG_TYPE_BUSY; })
>> +        ret = wait_for(done, 1000);
>> +        if (unlikely(ret))
>> +            goto out;
>> +        if (unlikely(FIELD_GET(GUC_MMIO_MSG_ORIGIN, header) !=
>> +                       GUC_MMIO_MSG_ORIGIN_GUC)) {
> 
> This check on GUC_MMIO_MSG_ORIGIN is redundant. We already waited for it
> to be GUC_MMIO_MSG_ORIGIN_GUC during the wait above and we're not
> sending other message from i915 while we're waiting for this one, so
> that field can't change.

in the wait above we are waiting for message different than BUSY and we
expect that this other message will be from the GuC (as this is part of
the protocol) but we should still check that GuC is compliant

> 
>> +            ret = -EPROTO;
>> +            goto out;
>> +        }
>> +        if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
>> +            ret = -ESTALE;
>> +            goto out;
>> +        }
>> +#undef done
>> +    }
>> +
>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) ==
>> GUC_MMIO_MSG_TYPE_ERROR) {
>> +        u32 status = FIELD_GET(GUC_MMIO_ERROR_STATUS, header);
>> +
>> +        ret = guc_status_to_errno(status);
>> +        goto out;
>> +    }
>> +
>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) !=
>> GUC_MMIO_MSG_TYPE_RESPONSE) {
>> +        ret = -EPROTO;
>>           goto out;
>>       }
>>   @@ -460,12 +504,17 @@ int intel_guc_send_mmio(struct intel_guc *guc,
>> const u32 *action, u32 len,
>>       }
>>         /* Use data from the GuC response as our return value */
>> -    ret = INTEL_GUC_MSG_TO_DATA(status);
> 
> INTEL_GUC_MSG_TO_DATA is still used in intel_guc_ct.c. Either stick to
> that or update the other use-case to the new define.

this new define is for MMIO messages, changes to CTB messages will
follow soon

> 
>> +    ret = FIELD_GET(GUC_MMIO_RESPONSE_DATA0, header);
>>     out:
>>       intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
>>       mutex_unlock(&guc->send_mutex);
>>   +    if (unlikely(ret < 0))
>> +        drm_err(&i915->drm,
>> +            "GuC MMIO action %#06x failed with error %d %#x\n",
>> +            *request, ret, header);
>> +
>>       return ret;
>>   }
>>   diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>> new file mode 100644
>> index 000000000000..95043788e0ad
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>> @@ -0,0 +1,215 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2020 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_GUC_ABI_H__
>> +#define __INTEL_GUC_ABI_H__
>> +
>> +/**
>> + * DOC: GuC MMIO based communication
>> + *
>> + * The MMIO based communication between Host and GuC relies on special
>> + * hardware registers which format could be defined by the software
>> + * (so called scratch registers).
>> + *
>> + * Each MMIO based message, both Host to GuC (H2G) and GuC to Host (G2H)
>> + * messages, which maximum length depends on number of available scratch
>> + * registers, is directly written into those scratch registers.
>> + *
>> + * For Gen9+, there are 16 software scratch registers 0xC180-0xC1B8,
>> + * but no H2G command takes more than 8 parameters and the GuC firmware
>> + * itself uses an 8-element array to store the H2G message.
>> + *
>> + * For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
>> + * are, regardless on lower count, preffered over legacy ones.
>> + *
>> + * The MMIO based communication is mainly used during driver
>> initialization
>> + * phase to setup the CTB based communication that will be used
>> afterwards.
>> + *
>> + * Details of the messages formats depend on GuC firmware version
>> being used
>> + * by the host driver. Documented here messages are for GuC 45.0 and
>> newer.
>> + */
>> +
>> +#define GUC_MAX_MMIO_MSG_LEN            8
> 
> Are we ok with not having an INTEL_ prefix to all the defines? I'm not
> complaining, we usually stick with that prefix but I'm not sure if it is
> an iron rule or not.

existing defines in intel_guc_fwif.h have "GUC" and "INTEL_GUC" (or
none) prefixes and at the same time almost all structs have "guc"
prefix, so the best option IMHO is to stick with just guc/GUC prefix.

> 
>> +
>> +/**
>> + * DOC: GuC MMIO message format
>> + *
>> + * Bits of the first scratch register are treated as a message header,
>> + * and other registers values are used to hold message payload (data)::
>> + *
>> + *      +=======================================================+
>> + *      |  SCRATCH  |                                           |
>> + *      +-----------+-------------------------------------------+
>> + *      | 0 | 31:28 | message ORIGIN/TYPE                       |
>> + *      |   | 27:24 | message MAGIC                             |
>> + *      |   |  23:0 | message DATA0 (depends on TYPE)           |
>> + *      +---+-------+-------------------------------------------+
>> + *      | 1 |  31:0 | message DATA1 (depends on TYPE)           |
>> + *      |...|       | message DATA2 (depends on TYPE)           |
>> + *      | n |       | message DATA3 (depends on TYPE)           |
>> + *      +=======================================================+
>> + *
>> + * Where:
>> + *  - **TYPE** is a 4b message type identifier.
>> + *  - **MAGIC** is a 4b message sequence number.
>> + *    The same sequence number must be included in all related messages.
>> + *    This field is used for tracking and debug purposes.
> 
> I don't think "magic" is a clear enough name for the role of the field.
> "Request id" or something like that would be more appropriate IMO.

we are following naming from GuC spec

> 
>> + *  - **DATA0..3** bits represents message payload.
>> + *    Definitions of these bits depends on message **TYPE**.
>> + *
>> + * The MSB of the header and **TYPE** indicates origin of the message:
>> + *  - 0 - message from the Host
>> + *  - 1 - message from the GuC
>> + *
>> + * Currently supported message types are:
>> + *  - 0x0 - **REQUEST** - represents Host request message to the GuC
>> + *  - 0xF - **SUCCESS RESPONSE** - GuC reply for the earlier request
>> + *  - 0xE - **ERROR RESPONSE** - GuC failure reply for the earlier
>> request
>> + *  - 0xB - **BUSY** - GuC will be processing request for the longer
>> time
>> + */
>> +
>> +#define GUC_MMIO_MSG_ORIGIN            (0x1 << 31)
>> +#define   GUC_MMIO_MSG_ORIGIN_HOST        0u
>> +#define   GUC_MMIO_MSG_ORIGIN_GUC        1u
>> +
>> +#define GUC_MMIO_MSG_TYPE            (0xF << 28)
>> +#define   GUC_MMIO_MSG_TYPE_REQUEST        0x0
>> +#define   GUC_MMIO_MSG_TYPE_RESPONSE        0xF
>> +#define   GUC_MMIO_MSG_TYPE_ERROR        0xE
>> +#define   GUC_MMIO_MSG_TYPE_BUSY        0xB
>> +
>> +#define GUC_MMIO_MSG_MAGIC            (0xF << 24)
>> +#define   GUC_MMIO_MSG_MAGIC_DEFAULT        0u
>> +
>> +/**
>> + * DOC: GuC MMIO H2G REQUEST
>> + *
>> + * The MMIO H2G REQUEST message is used by the Host to request some
>> action
>> + * to be performed by the GuC::
>> + *
>> + *      +=======================================================+
>> + *      |  SCRATCH  |                                           |
>> + *      +=======================================================+
>> + *      | 0 | 31:28 | message TYPE = 0x0 = REQUEST              |
>> + *      |   | 27:24 | message MAGIC                             |
>> + *      |   | 23:16 | request SUBCODE (depends on ACTION)       |
>> + *      |   |  15:0 | request ACTION code                       |
>> + *      +-------------------------------------------------------+
>> + *      | 1 |  31:0 | request DATA1 (depends on ACTION/SUBCODE) |
>> + *      |...|       | request DATA2 (depends on ACTION/SUBCODE) |
>> + *      | n |  31:0 | request DATA3 (depends on ACTION/SUBCODE) |
>> + *      +=======================================================+
>> + *
>> + * Where:
>> + *  - **TYPE** must be set to value 0x0.
>> + *  - **MAGIC** message sequence number is generated by the host.
>> + *  - **ACTION** represents 16b request action code that defines both
>> + *    purpose of the request and format of the passed payload data.
>> + *    List of supported codes depends on GuC version and plaform.
>> + *    Action code can't be zero.
>> + *  - **SUBCODE** is optional 8b data related to the **ACTION**.
>> + *    MBZ if not explicitly defined by given **ACTION**.
>> + *  - **DATA1..3** are optional 32b payload dwords related to
>> **ACTION**.
>> + *    Format of each dword is defined by specific **ACTION**.
>> + */
>> +
>> +#define GUC_MMIO_REQUEST_SUBCODE        (0xFF << 16)
>> +#define GUC_MMIO_REQUEST_ACTION            (0xFFFF << 0)
>> +#define   GUC_ACTION_INVALID            0x0000
>> +#define   GUC_ACTION_REGISTER_CTB        0x4505
>> +#define   GUC_ACTION_DEREGISTER_CTB        0x4506
> 
> These are now defined twice (once here and once in the enum). We should
> pick one for consistency.

we can drop old ones from intel_guc_fwif.h

> 
>> +
>> +/**
>> + * DOC: GuC MMIO G2H SUCCESS RESPONSE
>> + *
>> + * The MMIO G2H SUCCESS RESPONSE message is used by the GuC to reply to
>> + * the earlier H2G request message. This message is used if no errors
>> + * were encountered by the GuC during processing of the request::
>> + *
>> + *      +=======================================================+
>> + *      |  SCRATCH  |                                           |
>> + *      +=======================================================+
>> + *      | 0 | 31:28 | message TYPE = 0xF = SUCCESS RESPONSE     |
>> + *      |   | 27:24 | message MAGIC                             |
>> + *      |   |  23:0 | response DATA0 (depends on ACTION/SUBCODE)|
>> + *      +-------------------------------------------------------+
>> + *      | 1 |  31:0 | response DATA1 (depends on ACTION/SUBCODE)|
>> + *      |...|       | response DATA2 (depends on ACTION/SUBCODE)|
>> + *      | n |  31:0 | response DATA3 (depends on ACTION/SUBCODE)|
>> + *      +=======================================================+
>> + *
>> + * Where:
>> + *  - **TYPE** must be set to value 0xF.
>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>> message.
>> + *  - **DATA** is optional payload data related to **ACTION**.
>> + *    Format is defined by each **ACTION** separately.
>> + */
>> +
>> +#define GUC_MMIO_RESPONSE_DATA0            (0xFFFFFF << 0)
>> +
>> +/**
>> + * DOC: GuC MMIO G2H ERROR RESPONSE
>> + *
>> + * The MMIO G2H ERROR RESPONSE message is used by the GuC to reply to
>> + * the earlier H2G request message. This message is used if some errors
>> + * were encountered by the GuC during processing of the request::
>> + *
>> + *      +=======================================================+
>> + *      |  SCRATCH  |                                           |
>> + *      +=======================================================+
>> + *      | 0 | 31:28 | message TYPE = 0xE = ERROR RESPONSE       |
>> + *      |   | 27:24 | message MAGIC                             |
>> + *      |   | 23:16 | reserved MBZ                              |
>> + *      |   |  15:0 | response STATUS failure code              |
>> + *      +-------------------------------------------------------+
>> + *      | 1 |  31:0 | reserved MBZ                              |
>> + *      |...|       | reserved MBZ                              |
>> + *      | n |  31:0 | reserved MBZ                              |
>> + *      +=======================================================+
>> + *
>> + * Where:
>> + *  - **TYPE** must be set to value 0xE.
>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>> message.
>> + *  - **STATUS** represents non-zero failure code.
>> + */
>> +
>> +#define GUC_MMIO_ERROR_STATUS            (0xFFFF << 0)
>> +#define   GUC_STATUS_UNKNOWN_ACTION        0x30
>> +#define   GUC_STATUS_INVALID_PARAMS        0x60
>> +#define   GUC_STATUS_INVALID_ADDR        0x80
>> +#define   GUC_STATUS_CTX_NOT_REGISTERED        0x100
>> +#define   GUC_STATUS_NO_LOCAL_MEMORY        0x200
>> +#define   GUC_STATUS_NO_VIRTUALIZATION        0x300
>> +#define   GUC_STATUS_CTB_FULL            0x301
>> +#define   GUC_STATUS_UNAUTHORIZED_REQUEST    0x302
>> +#define   GUC_STATUS_GENERIC_FAIL        0xF000
>> +
>> +/**
>> + * DOC: MMIO G2H BUSY RESPONSE
>> + *
>> + * The MMIO G2H BUSY RESPONSE message is used by the GuC to reply to
>> + * the earlier H2G request message. This message is used if processing
>> + * of the request will take longer time and final SUCCESS/ERROR response
>> + * message will be delivered later::
>> + *
>> + *      +=======================================================+
>> + *      |  SCRATCH  |                                           |
>> + *      +=======================================================+
>> + *      | 0 | 31:28 | message TYPE = 0xB = BUSY RESPONSE        |
>> + *      |   | 27:24 | message MAGIC                             |
>> + *      |   |  23:0 | reserved MBZ                              |
>> + *      +-------------------------------------------------------+
>> + *      | 1 |  31:0 | reserved MBZ                              |
>> + *      |...|       | reserved MBZ                              |
>> + *      | n |  31:0 | reserved MBZ                              |
>> + *      +=======================================================+
>> + *
>> + * Where:
>> + *  - **TYPE** must be set to value 0xB.
>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>> message.
>> + *  - all other bits are reserved as MBZ.
>> + */
>> +
>> +#endif /* __INTEL_GUC_ABI_H__ */
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>> index d44061033f23..e66cbf1be320 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>> @@ -10,11 +10,52 @@
>>     /*
>>    * The Additional Data Struct (ADS) has pointers for different
>> buffers used by
>> - * the GuC. One single gem object contains the ADS struct itself
>> (guc_ads), the
>> - * scheduling policies (guc_policies), a structure describing a
>> collection of
>> - * register sets (guc_mmio_reg_state) and some extra pages for the
>> GuC to save
>> - * its internal state for sleep.
>> + * the GuC. One single gem object contains the ADS struct itself
>> (guc_ads) and
>> + * all the extra buffers indirectly linked via the ADS struct's entires.
>> + *
>> + * Layout of the ADS blob allocated for the GuC:
>> + *
>> + *      +---------------------------------------+ <== base
>> + *      | guc_ads                               |
>> + *      +---------------------------------------+
>> + *      | guc_policies                          |
>> + *      +---------------------------------------+
>> + *      | guc_gt_system_info                    |
>> + *      +---------------------------------------+
>> + *      | guc_clients_info                      |
>> + *      +---------------------------------------+
>> + *      | guc_ct_pool_entry[size]               |
>> + *      +---------------------------------------+
>> + *      | padding                               |
>> + *      +---------------------------------------+ <== 4K aligned
>> + *      | private data                          |
>> + *      +---------------------------------------+
>> + *      | padding                               |
>> + *      +---------------------------------------+ <== 4K aligned
>>    */
> 
> I think it we could expand the comment (or add a new one) to make clear
> that the private data is not included in the __guc_ads_blob to keep
> things simple since it is variable in size, but it is still part of the
> ads_blobpasses to GuC.
> 
>> +struct __guc_ads_blob {
>> +    struct guc_ads ads;
>> +    struct guc_policies policies;
>> +    struct guc_gt_system_info system_info;
>> +    struct guc_clients_info clients_info;
>> +    struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
>> +} __packed;
>> +
>> +static u32 guc_ads_private_data_size(struct intel_guc *guc)
>> +{
>> +    return PAGE_ALIGN(guc->fw.private_data_size);
>> +}
>> +
>> +static u32 guc_ads_private_data_offset(struct intel_guc *guc)
>> +{
>> +    return PAGE_ALIGN(sizeof(struct __guc_ads_blob));
>> +}
>> +
>> +static u32 guc_ads_blob_size(struct intel_guc *guc)
>> +{
>> +    return guc_ads_private_data_offset(guc) +
>> +           guc_ads_private_data_size(guc);
>> +}
>>     static void guc_policy_init(struct guc_policy *policy)
>>   {
>> @@ -48,23 +89,33 @@ static void guc_ct_pool_entries_init(struct
>> guc_ct_pool_entry *pool, u32 num)
>>       memset(pool, 0, num * sizeof(*pool));
>>   }
>>   +static void guc_mapping_table_init(struct intel_gt *gt,
>> +                   struct guc_gt_system_info *system_info)
>> +{
>> +    unsigned int i, j;
>> +    struct intel_engine_cs *engine;
>> +    enum intel_engine_id id;
>> +
>> +    /* Table must be set to invalid values for entries not used */
>> +    for (i = 0; i < GUC_MAX_ENGINE_CLASSES; ++i)
>> +        for (j = 0; j < GUC_MAX_INSTANCES_PER_CLASS; ++j)
>> +            system_info->mapping_table[i][j] =
>> +                GUC_MAX_INSTANCES_PER_CLASS;
>> +
>> +    for_each_engine(engine, gt, id) {
>> +        u8 guc_class = engine->class;
>> +
>> +        system_info->mapping_table[guc_class][engine->instance]
>> +            = engine->instance;
>> +    }
>> +}
>> +
>>   /*
>>    * The first 80 dwords of the register state context, containing the
>>    * execlists and ppgtt registers.
>>    */
>>   #define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>>   -/* The ads obj includes the struct itself and buffers passed to GuC */
>> -struct __guc_ads_blob {
>> -    struct guc_ads ads;
>> -    struct guc_policies policies;
>> -    struct guc_mmio_reg_state reg_state;
>> -    struct guc_gt_system_info system_info;
>> -    struct guc_clients_info clients_info;
>> -    struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
>> -    u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>> -} __packed;
>> -
>>   static void __guc_ads_init(struct intel_guc *guc)
>>   {
>>       struct intel_gt *gt = guc_to_gt(guc);
>> @@ -107,6 +158,16 @@ static void __guc_ads_init(struct intel_guc *guc)
>>       blob->system_info.vebox_enable_mask = VEBOX_MASK(gt);
>>       blob->system_info.vdbox_sfc_support_mask =
>> gt->info.vdbox_sfc_access;
>>   +    if (INTEL_GEN(gt->i915) >= 12 && !IS_DGFX(gt->i915)) {
>> +        u32 distdbreg = intel_uncore_read(gt->uncore,
>> +                          GEN12_DIST_DBS_POPULATED);
>> +        blob->system_info.num_of_doorbells_per_sqidi =
>> +            ((distdbreg >> GEN12_DOORBELLS_PER_SQIDI_SHIFT) &
>> +             GEN12_DOORBELLS_PER_SQIDI) + 1;
>> +    }
>> +
>> +    guc_mapping_table_init(guc_to_gt(guc), &blob->system_info);
>> +
>>       base = intel_guc_ggtt_offset(guc, guc->ads_vma);
>>         /* Clients info  */
>> @@ -118,11 +179,12 @@ static void __guc_ads_init(struct intel_guc *guc)
>>         /* ADS */
>>       blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
>> -    blob->ads.reg_state_buffer = base + ptr_offset(blob,
>> reg_state_buffer);
>> -    blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
>>       blob->ads.gt_system_info = base + ptr_offset(blob, system_info);
>>       blob->ads.clients_info = base + ptr_offset(blob, clients_info);
>>   +    /* Private Data */
>> +    blob->ads.private_data = base + guc_ads_private_data_offset(guc);
>> +
>>       i915_gem_object_flush_map(guc->ads_vma->obj);
>>   }
>>   @@ -135,14 +197,15 @@ static void __guc_ads_init(struct intel_guc *guc)
>>    */
>>   int intel_guc_ads_create(struct intel_guc *guc)
>>   {
>> -    const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
>> +    u32 size;
>>       int ret;
>>         GEM_BUG_ON(guc->ads_vma);
>>   +    size = guc_ads_blob_size(guc);
>> +
>>       ret = intel_guc_allocate_and_map_vma(guc, size, &guc->ads_vma,
>>                            (void **)&guc->ads_blob);
>> -
>>       if (ret)
>>           return ret;
>>   @@ -156,6 +219,18 @@ void intel_guc_ads_destroy(struct intel_guc *guc)
>>       i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP);
>>   }
>>   +void guc_ads_private_data_reset(struct intel_guc *guc)
>> +{
>> +    u32 size;
>> +
>> +    size = guc_ads_private_data_size(guc);
>> +    if (!size)
>> +        return;
>> +
>> +    memset((void *)guc->ads_blob + guc_ads_private_data_offset(guc), 0,
>> +           size);
>> +}
>> +
>>   /**
>>    * intel_guc_ads_reset() - prepares GuC Additional Data Struct for
>> reuse
>>    * @guc: intel_guc struct
>> @@ -168,5 +243,8 @@ void intel_guc_ads_reset(struct intel_guc *guc)
>>   {
>>       if (!guc->ads_vma)
>>           return;
>> +
>>       __guc_ads_init(guc);
>> +
>> +    guc_ads_private_data_reset(guc);
>>   }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> index d4a87f4c9421..212de0a939bf 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> @@ -74,8 +74,9 @@ static inline bool guc_ready(struct intel_uncore
>> *uncore, u32 *status)
>>           ((val & GS_MIA_CORE_STATE) && (uk_val ==
>> GS_UKERNEL_LAPIC_DONE));
>>   }
>>   -static int guc_wait_ucode(struct intel_uncore *uncore)
>> +static int guc_wait_ucode(struct intel_gt *gt)
>>   {
>> +    struct intel_uncore *uncore = gt->uncore;
> 
> The gt seems to be used only for gt->i915, why not just use uncore->i915?
> 
>>       u32 status;
>>       int ret;
>>   @@ -93,12 +94,24 @@ static int guc_wait_ucode(struct intel_uncore
>> *uncore)
>>       if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>>           DRM_ERROR("GuC firmware signature verification failed\n");
>>           ret = -ENOEXEC;
>> -    }
>> -
>> +    } else
>>       if ((status & GS_UKERNEL_MASK) == GS_UKERNEL_EXCEPTION) {
>>           DRM_ERROR("GuC firmware exception. EIP: %#x\n",
>>                 intel_uncore_read(uncore, SOFT_SCRATCH(13)));
>>           ret = -ENXIO;
>> +    } else
>> +    if (ret) {
>> +        i915_probe_error(gt->i915, "GuC load failed: status = 0x%08X\n",
>> +                 status);
>> +        i915_probe_error(gt->i915, "GuC status: Reset = %d, "
>> +                 "BootROM = 0x%02X, UKernel = 0x%02X, "
>> +                 "MIA = 0x%02X, Auth = 0x%02X\n",
>> +                 (status >> GS_RESET_SHIFT) & 1,
>> +                 (status & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT,
>> +                 (status & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT,
>> +                 (status & GS_MIA_MASK) >> GS_MIA_SHIFT,
>> +                 (status & GS_AUTH_STATUS_MASK) >>
>> +                              GS_AUTH_STATUS_SHIFT);
> 
> nit: I think using the FIELD_GET macro here as well could make things
> cleaner
> 
>>       }
>>         return ret;
>> @@ -139,7 +152,7 @@ int intel_guc_fw_upload(struct intel_guc *guc)
>>       if (ret)
>>           goto out;
>>   -    ret = guc_wait_ucode(uncore);
>> +    ret = guc_wait_ucode(gt);
>>       if (ret)
>>           goto out;
>>   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 a6b733c146c9..77e49d53bb5c 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> @@ -27,7 +27,7 @@
>>   #define GUC_MAX_ENGINES_NUM        (GUC_VIDEO_ENGINE2 + 1)
>>     #define GUC_MAX_ENGINE_CLASSES        5
>> -#define GUC_MAX_INSTANCES_PER_CLASS    16
>> +#define GUC_MAX_INSTANCES_PER_CLASS    32
>>     #define GUC_DOORBELL_INVALID        256
>>   @@ -62,12 +62,7 @@
>>   #define GUC_STAGE_DESC_ATTR_PCH        BIT(6)
>>   #define GUC_STAGE_DESC_ATTR_TERMINATED    BIT(7)
>>   -/* New GuC control data */
>> -#define GUC_CTL_CTXINFO            0
>> -#define   GUC_CTL_CTXNUM_IN16_SHIFT    0
>> -#define   GUC_CTL_BASE_ADDR_SHIFT    12
>> -
>> -#define GUC_CTL_LOG_PARAMS        1
>> +#define GUC_CTL_LOG_PARAMS        0
>>   #define   GUC_LOG_VALID            (1 << 0)
>>   #define   GUC_LOG_NOTIFY_ON_HALF_FULL    (1 << 1)
>>   #define   GUC_LOG_ALLOC_IN_MEGABYTE    (1 << 3)
>> @@ -79,11 +74,11 @@
>>   #define   GUC_LOG_ISR_MASK            (0x7 << GUC_LOG_ISR_SHIFT)
>>   #define   GUC_LOG_BUF_ADDR_SHIFT    12
>>   -#define GUC_CTL_WA            2
>> -#define GUC_CTL_FEATURE            3
>> +#define GUC_CTL_WA            1
>> +#define GUC_CTL_FEATURE            2
>>   #define   GUC_CTL_DISABLE_SCHEDULER    (1 << 14)
>>   -#define GUC_CTL_DEBUG            4
>> +#define GUC_CTL_DEBUG            3
>>   #define   GUC_LOG_VERBOSITY_SHIFT    0
>>   #define   GUC_LOG_VERBOSITY_LOW        (0 << GUC_LOG_VERBOSITY_SHIFT)
>>   #define   GUC_LOG_VERBOSITY_MED        (1 << GUC_LOG_VERBOSITY_SHIFT)
>> @@ -97,12 +92,31 @@
>>   #define   GUC_LOG_DISABLED        (1 << 6)
>>   #define   GUC_PROFILE_ENABLED        (1 << 7)
>>   -#define GUC_CTL_ADS            5
>> +#define GUC_CTL_ADS            4
>>   #define   GUC_ADS_ADDR_SHIFT        1
>>   #define   GUC_ADS_ADDR_MASK        (0xFFFFF << GUC_ADS_ADDR_SHIFT)
>>     #define GUC_CTL_MAX_DWORDS        (SOFT_SCRATCH_COUNT - 2) /*
>> [1..14] */
>>   +/*
>> + * The class goes in bits [0..2] of the GuC ID, the instance in bits
>> [3..6].
>> + * Bit 7 can be used for operations that apply to all engine
>> classes&instances.
>> + */
>> +#define GUC_ENGINE_CLASS_SHIFT        0
>> +#define GUC_ENGINE_CLASS_MASK        (0x7 << GUC_ENGINE_CLASS_SHIFT)
>> +#define GUC_ENGINE_INSTANCE_SHIFT    3
>> +#define GUC_ENGINE_INSTANCE_MASK    (0xf << GUC_ENGINE_INSTANCE_SHIFT)
>> +#define GUC_ENGINE_ALL_INSTANCES    (1 << 7)
>> +
>> +#define MAKE_GUC_ID(class, instance) \
>> +    (((class) << GUC_ENGINE_CLASS_SHIFT) | \
>> +     ((instance) << GUC_ENGINE_INSTANCE_SHIFT))
>> +
>> +#define GUC_ID_TO_ENGINE_CLASS(guc_id) \
>> +    (((guc_id) & GUC_ENGINE_CLASS_MASK) >> GUC_ENGINE_CLASS_SHIFT)
>> +#define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
>> +    (((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
>> +
>>   /* Work item for submitting workloads into work queue of GuC. */
>>   struct guc_wq_item {
>>       u32 header;
>> @@ -338,7 +352,6 @@ struct guc_policies {
>>   /* GuC MMIO reg state struct */
>>     -#define GUC_REGSET_MAX_REGISTERS    64
>>   #define GUC_S3_SAVE_SPACE_PAGES        10
>>     struct guc_mmio_reg {
>> @@ -348,16 +361,17 @@ struct guc_mmio_reg {
>>   #define GUC_REGSET_MASKED        (1 << 0)
>>   } __packed;
>>   -struct guc_mmio_regset {
>> -    struct guc_mmio_reg registers[GUC_REGSET_MAX_REGISTERS];
>> -    u32 values_valid;
>> -    u32 number_of_registers;
>> -} __packed;
>> -
>>   /* GuC register sets */
>> -struct guc_mmio_reg_state {
>> -    struct guc_mmio_regset
>> engine_reg[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>> -    u32 reserved[98];
>> +struct guc_mmio_reg_set {
>> +    u32 address;
>> +    union {
>> +        struct {
>> +            u32 count:16;
>> +            u32 reserved1:12;
>> +            u32 reserved2:4;
>> +        };
>> +        u32 count_u32;
>> +    };
>>   } __packed;
>>     /* HW info */
>> @@ -369,7 +383,9 @@ struct guc_gt_system_info {
>>       u32 vdbox_enable_mask;
>>       u32 vdbox_sfc_support_mask;
>>       u32 vebox_enable_mask;
>> -    u32 reserved[9];
>> +    u32 num_of_doorbells_per_sqidi;
>> +    u8
>> mapping_table[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>> +    u32 reserved2[8];
>>   } __packed;
>>     /* Clients info */
>> @@ -390,15 +406,16 @@ struct guc_clients_info {
>>     /* GuC Additional Data Struct */
>>   struct guc_ads {
>> -    u32 reg_state_addr;
>> -    u32 reg_state_buffer;
>> +    struct guc_mmio_reg_set
>> reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>> +    u32 reserved0;
>>       u32 scheduler_policies;
>>       u32 gt_system_info;
>>       u32 clients_info;
>>       u32 control_data;
>>       u32 golden_context_lrca[GUC_MAX_ENGINE_CLASSES];
>>       u32 eng_state_size[GUC_MAX_ENGINE_CLASSES];
>> -    u32 reserved[16];
>> +    u32 private_data;
>> +    u32 reserved[15];
>>   } __packed;
>>     /* GuC logging structures */
>> @@ -474,49 +491,6 @@ struct guc_shared_ctx_data {
>>       struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>>   } __packed;
>>   -/**
>> - * DOC: MMIO based communication
>> - *
>> - * The MMIO based communication between Host and GuC uses software
>> scratch
>> - * registers, where first register holds data treated as message header,
>> - * and other registers are used to hold message payload.
>> - *
>> - * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
>> - * but no H2G command takes more than 8 parameters and the GuC FW
>> - * itself uses an 8-element array to store the H2G message.
>> - *
>> - *      +-----------+---------+---------+---------+
>> - *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
>> - *      +-----------+---------+---------+---------+
>> - *      | header    |      optional payload       |
>> - *      +======+====+=========+=========+=========+
>> - *      | 31:28|type|         |         |         |
>> - *      +------+----+         |         |         |
>> - *      | 27:16|data|         |         |         |
>> - *      +------+----+         |         |         |
>> - *      |  15:0|code|         |         |         |
>> - *      +------+----+---------+---------+---------+
>> - *
>> - * The message header consists of:
>> - *
>> - * - **type**, indicates message type
>> - * - **code**, indicates message code, is specific for **type**
>> - * - **data**, indicates message data, optional, depends on **code**
>> - *
>> - * The following message **types** are supported:
>> - *
>> - * - **REQUEST**, indicates Host-to-GuC request, requested GuC action
>> code
>> - *   must be priovided in **code** field. Optional action specific
>> parameters
>> - *   can be provided in remaining payload registers or **data** field.
>> - *
>> - * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC
>> request,
>> - *   action response status will be provided in **code** field. Optional
>> - *   response data can be returned in remaining payload registers or
>> **data**
>> - *   field.
>> - */
>> -
>> -#define GUC_MAX_MMIO_MSG_LEN        8
>> -
>>   #define INTEL_GUC_MSG_TYPE_SHIFT    28
>>   #define INTEL_GUC_MSG_TYPE_MASK        (0xF <<
>> INTEL_GUC_MSG_TYPE_SHIFT)
>>   #define INTEL_GUC_MSG_DATA_SHIFT    16
> 
> These GUC_MSG defines are also duplicated

These are still used by the CTB and we want to keep CTB definitions
separate as we will do some refactoring there too.

Michal

> 
> Daniele
> 
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>> index 1949346e714e..b37fc2ffaef2 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>> @@ -118,6 +118,11 @@ struct guc_doorbell_info {
>>   #define   GEN8_DRB_VALID          (1<<0)
>>   #define GEN8_DRBREGU(x)            _MMIO(0x1000 + (x) * 8 + 4)
>>   +#define GEN12_DIST_DBS_POPULATED        _MMIO(0xd08)
>> +#define   GEN12_DOORBELLS_PER_SQIDI_SHIFT    16
>> +#define   GEN12_DOORBELLS_PER_SQIDI        (0xff)
>> +#define   GEN12_SQIDIS_DOORBELL_EXIST        (0xffff)
>> +
>>   #define DE_GUCRMR            _MMIO(0x44054)
>>     #define GUC_BCS_RCS_IER            _MMIO(0xC550)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index 59b27aba15c6..f7cb0b1f1ba5 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -44,23 +44,19 @@ void intel_uc_fw_change_status(struct intel_uc_fw
>> *uc_fw,
>>    * List of required GuC and HuC binaries per-platform.
>>    * Must be ordered based on platform + revid, from newer to older.
>>    *
>> - * TGL 35.2 is interface-compatible with 33.0 for previous Gens. The
>> deltas
>> - * between 33.0 and 35.2 are only related to new additions to support
>> new Gen12
>> - * features.
>> - *
>>    * Note that RKL uses the same firmware as TGL.
>>    */
>>   #define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
>> -    fw_def(ROCKETLAKE,  0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7,
>> 0, 12)) \
>> -    fw_def(TIGERLAKE,   0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7,
>> 0, 12)) \
>> -    fw_def(ELKHARTLAKE, 0, guc_def(ehl, 33, 0, 4), huc_def(ehl,  9,
>> 0, 0)) \
>> -    fw_def(ICELAKE,     0, guc_def(icl, 33, 0, 0), huc_def(icl,  9,
>> 0, 0)) \
>> -    fw_def(COMETLAKE,   5, guc_def(cml, 33, 0, 0), huc_def(cml,  4,
>> 0, 0)) \
>> -    fw_def(COFFEELAKE,  0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4,
>> 0, 0)) \
>> -    fw_def(GEMINILAKE,  0, guc_def(glk, 33, 0, 0), huc_def(glk,  4,
>> 0, 0)) \
>> -    fw_def(KABYLAKE,    0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4,
>> 0, 0)) \
>> -    fw_def(BROXTON,     0, guc_def(bxt, 33, 0, 0), huc_def(bxt,  2,
>> 0, 0)) \
>> -    fw_def(SKYLAKE,     0, guc_def(skl, 33, 0, 0), huc_def(skl,  2,
>> 0, 0))
>> +    fw_def(ROCKETLAKE,  0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7,
>> 0, 12)) \
>> +    fw_def(TIGERLAKE,   0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7,
>> 0, 12)) \
>> +    fw_def(ELKHARTLAKE, 0, guc_def(ehl, 45, 0, 0), huc_def(ehl,  9,
>> 0, 0)) \
>> +    fw_def(ICELAKE,     0, guc_def(icl, 45, 0, 0), huc_def(icl,  9,
>> 0, 0)) \
>> +    fw_def(COMETLAKE,   5, guc_def(cml, 45, 0, 0), huc_def(cml,  4,
>> 0, 0)) \
>> +    fw_def(COFFEELAKE,  0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4,
>> 0, 0)) \
>> +    fw_def(GEMINILAKE,  0, guc_def(glk, 45, 0, 0), huc_def(glk,  4,
>> 0, 0)) \
>> +    fw_def(KABYLAKE,    0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4,
>> 0, 0)) \
>> +    fw_def(BROXTON,     0, guc_def(bxt, 45, 0, 0), huc_def(bxt,  2,
>> 0, 0)) \
>> +    fw_def(SKYLAKE,     0, guc_def(skl, 45, 0, 0), huc_def(skl,  2,
>> 0, 0))
>>     #define __MAKE_UC_FW_PATH(prefix_, name_, major_, minor_, patch_) \
>>       "i915/" \
>> @@ -371,6 +367,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>>           }
>>       }
>>   +    if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
>> +        uc_fw->private_data_size = css->private_data_size;
>> +
>>       obj = i915_gem_object_create_shmem_from_data(i915, fw->data,
>> fw->size);
>>       if (IS_ERR(obj)) {
>>           err = PTR_ERR(obj);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> index 23d3a423ac0f..99bb1fe1af66 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> @@ -88,6 +88,8 @@ struct intel_uc_fw {
>>         u32 rsa_size;
>>       u32 ucode_size;
>> +
>> +    u32 private_data_size;
>>   };
>>     #ifdef CONFIG_DRM_I915_DEBUG_GUC
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> index 029214cdedd5..e41ffc7a7fbc 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>> @@ -69,7 +69,11 @@ struct uc_css_header {
>>   #define CSS_SW_VERSION_UC_MAJOR        (0xFF << 16)
>>   #define CSS_SW_VERSION_UC_MINOR        (0xFF << 8)
>>   #define CSS_SW_VERSION_UC_PATCH        (0xFF << 0)
>> -    u32 reserved[14];
>> +    u32 reserved0[13];
>> +    union {
>> +        u32 private_data_size; /* only applies to GuC */
>> +        u32 reserved1;
>> +    };
>>       u32 header_info;
>>   } __packed;
>>   static_assert(sizeof(struct uc_css_header) == 128);
>

Daniele Ceraolo Spurio Aug. 20, 2020, 9:29 p.m. UTC | #3

On 8/19/2020 2:14 PM, Michal Wajdeczko wrote:
>
> On 11.08.2020 03:04, Daniele Ceraolo Spurio wrote:
>>
>> On 8/7/2020 10:46 AM, [e-mail] wrote:
>>> From: John Harrison <[e-mail]>
>>>
>>> Update to the latest GuC firmware. This includes some significant
>>> changes to the interface.
>> A high level overview of the changes would be nice for extra clarity. A
>> couple of example:
>>
>> - GuC now requires a private memory area
>> - you're dropping the programming of the reg_state struct, but that's
>> actually still defined in the structure, so IMO it's worth mentioning
>> that it will come back with GuC submission.
>>
>>> Signed-off-by: John Harrison <[e-mail]>
>>> Author: Daniele Ceraolo Spurio <[e-mail]>
>>> Author: John Harrison <[e-mail]>
>>> Author: Matthew Brost <[e-mail]>
>>> Author: Michal Wajdeczko <[e-mail]>
>>> Author: Michel Thierry <[e-mail]>
>>> Author: Oscar Mateo <[e-mail]>
>>> Author: Rodrigo Vivi <[e-mail]>
> For the review process it would be better to have individual patches
> from each author. We can squash them just before merge.
>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_engine_cs.c    |   3 +-
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc.c       | 125 +++++++----
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h   | 215 +++++++++++++++++++
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c   | 116 ++++++++--
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c    |  21 +-
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h  | 110 ++++------
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h   |   5 +
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  27 ++-
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   2 +
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h |   6 +-
>>>    10 files changed, 485 insertions(+), 145 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> index ea4ba2afe9f9..9cd62d68abc6 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> @@ -305,8 +305,9 @@ static int intel_engine_setup(struct intel_gt *gt,
>>> enum intel_engine_id id)
>>>        engine->i915 = i915;
>>>        engine->gt = gt;
>>>        engine->uncore = gt->uncore;
>>> -    engine->hw_id = engine->guc_id = info->hw_id;
>>>        engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
>>> +    engine->hw_id = info->hw_id;
>>> +    engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
>>>          engine->class = info->class;
>>>        engine->instance = info->instance;
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> index 861657897c0f..8d30e1d7d8a6 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> @@ -7,6 +7,7 @@
>>>    #include "gt/intel_gt_irq.h"
>>>    #include "gt/intel_gt_pm_irq.h"
>>>    #include "intel_guc.h"
>>> +#include "intel_guc_abi.h"
>>>    #include "intel_guc_ads.h"
>>>    #include "intel_guc_submission.h"
>>>    #include "i915_drv.h"
>>> @@ -213,23 +214,6 @@ static u32 guc_ctl_feature_flags(struct intel_guc
>>> *guc)
>>>        return flags;
>>>    }
>>>    -static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
>>> -{
>>> -    u32 flags = 0;
>>> -
>>> -    if (intel_guc_submission_is_used(guc)) {
>>> -        u32 ctxnum, base;
>>> -
>>> -        base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
>>> -        ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
>>> -
>>> -        base >>= PAGE_SHIFT;
>>> -        flags |= (base << GUC_CTL_BASE_ADDR_SHIFT) |
>>> -            (ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
>>> -    }
>>> -    return flags;
>>> -}
>>> -
>>>    static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
>>>    {
>>>        u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >>
>>> PAGE_SHIFT;
>>> @@ -291,7 +275,6 @@ static void guc_init_params(struct intel_guc *guc)
>>>          BUILD_BUG_ON(sizeof(guc->params) != GUC_CTL_MAX_DWORDS *
>>> sizeof(u32));
>>>    -    params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
>>>        params[GUC_CTL_LOG_PARAMS] = guc_ctl_log_params_flags(guc);
>>>        params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
>>>        params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
>>> @@ -400,32 +383,65 @@ void intel_guc_fini(struct intel_guc *guc)
>>>        intel_uc_fw_fini(&guc->fw);
>>>    }
>>>    +static bool is_valid_mmio_action(u32 action)
>>> +{
>>> +    return action == GUC_ACTION_REGISTER_CTB ||
>>> +           action == GUC_ACTION_DEREGISTER_CTB;
>>> +}
>>> +
>>> +static int guc_status_to_errno(u32 status)
>>> +{
>>> +    switch (status) {
>>> +    case GUC_STATUS_UNKNOWN_ACTION:
>>> +        return -EOPNOTSUPP;
>>> +    case GUC_STATUS_INVALID_PARAMS:
>>> +        return -EINVAL;
>>> +    case GUC_STATUS_INVALID_ADDR:
>>> +        return -EFAULT;
>>> +    case GUC_STATUS_CTX_NOT_REGISTERED:
>>> +        return -ESRCH;
>>> +    case GUC_STATUS_CTB_FULL:
>>> +        return -ENOSPC;
>>> +    case GUC_STATUS_UNAUTHORIZED_REQUEST:
>>> +        return -EACCES;
>>> +    case GUC_STATUS_GENERIC_FAIL:
>>> +    default:
>>> +        return -ENXIO;
>>> +    }
>>> +}
>>> +
>>>    /*
>>>     * This function implements the MMIO based host to GuC interface.
>>>     */
>>> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
>>> len,
>>> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request,
>>> u32 len,
>>>                u32 *response_buf, u32 response_buf_size)
>>>    {
>>> +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>>>        struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
>>> -    u32 status;
>>> +    u32 action = FIELD_GET(GUC_MMIO_REQUEST_ACTION, *request);
>>> +    u32 subcode = FIELD_GET(GUC_MMIO_REQUEST_SUBCODE, *request);
>>> +    u32 magic = GUC_MMIO_MSG_MAGIC_DEFAULT;
>>> +    u32 header;
>>>        int i;
>>>        int ret;
>>>          GEM_BUG_ON(!len);
>>>        GEM_BUG_ON(len > guc->send_regs.count);
>>>    -    /* We expect only action code */
>>> -    GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
>>> +    /* We expect to use MMIO only for special actions */
>>> +    GEM_BUG_ON(!is_valid_mmio_action(action));
>>>    -    /* If CT is available, we expect to use MMIO only during
>>> init/fini */
>>> -    GEM_BUG_ON(*action !=
>>> INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
>>> -           *action !=
>>> INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
>>> +    header = FIELD_PREP(GUC_MMIO_MSG_TYPE, GUC_MMIO_MSG_TYPE_REQUEST) |
>>> +        FIELD_PREP(GUC_MMIO_MSG_MAGIC, magic) |
>>> +        FIELD_PREP(GUC_MMIO_REQUEST_SUBCODE, subcode) |
>>> +        FIELD_PREP(GUC_MMIO_REQUEST_ACTION, action);
>>>          mutex_lock(&guc->send_mutex);
>>>        intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);
>>>    -    for (i = 0; i < len; i++)
>>> -        intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]);
>>> +    intel_uncore_write(uncore, guc_send_reg(guc, 0), header);
>>> +    for (i = 1; i < len; i++)
>>> +        intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);
>>>          intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));
>>>    @@ -437,17 +453,45 @@ int intel_guc_send_mmio(struct intel_guc *guc,
>>> const u32 *action, u32 len,
>>>         */
>>>        ret = __intel_wait_for_register_fw(uncore,
>>>                           guc_send_reg(guc, 0),
>>> -                       INTEL_GUC_MSG_TYPE_MASK,
>>> -                       INTEL_GUC_MSG_TYPE_RESPONSE <<
>>> -                       INTEL_GUC_MSG_TYPE_SHIFT,
>>> -                       10, 10, &status);
>>> -    /* If GuC explicitly returned an error, convert it to -EIO */
>>> -    if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
>>> -        ret = -EIO;
>>> +                       GUC_MMIO_MSG_ORIGIN,
>>> +                       FIELD_PREP(GUC_MMIO_MSG_ORIGIN,
>>> +                              GUC_MMIO_MSG_ORIGIN_GUC),
>>> +                       10, 10, &header);
>>> +    if (unlikely(ret))
>>> +        goto out;
>>>    -    if (ret) {
>>> -        DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
>>> -              action[0], ret, status);
>>> +    if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
>>> +        ret = -ESTALE;
>>> +        goto out;
>>> +    }
>> I think we should move this after the check for busyness. IIRC the
>> protocol says we need to wait for the GuC to stop being busy before
>> sending more messages. That way you can also unify this with the other
>> similar check below.
> but each MMIO message should be handled separately and part of that
> handling is a check of the 'magic' field, so we can't skip it here

I wasn't suggesting to skip the magic check, just to do it after the 
busyness check. If we exit this function while there is a busy message 
there is a risk we could send another message while the GuC is still 
busy with this message, which would likely lead to undefined behavior. 
My suggestion is to do as follow:

     wait_for(message is response);
     if (MSG_TYPE == BUSY)
         wait_for (MSG_TYPE != BUSY);

     if (MSG_MAGIC != expected magic)
         return error;

>>> +
>>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) ==
>>> GUC_MMIO_MSG_TYPE_BUSY) {
>>> +#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc,
>>> 0)); \
>>> +        FIELD_GET(GUC_MMIO_MSG_TYPE, header) !=
>>> GUC_MMIO_MSG_TYPE_BUSY; })
>>> +        ret = wait_for(done, 1000);
>>> +        if (unlikely(ret))
>>> +            goto out;
>>> +        if (unlikely(FIELD_GET(GUC_MMIO_MSG_ORIGIN, header) !=
>>> +                       GUC_MMIO_MSG_ORIGIN_GUC)) {
>> This check on GUC_MMIO_MSG_ORIGIN is redundant. We already waited for it
>> to be GUC_MMIO_MSG_ORIGIN_GUC during the wait above and we're not
>> sending other message from i915 while we're waiting for this one, so
>> that field can't change.
> in the wait above we are waiting for message different than BUSY and we
> expect that this other message will be from the GuC (as this is part of
> the protocol) but we should still check that GuC is compliant

I think this is overly paranoid. Even in the very unlikely case that the 
GuC marked its own messages as coming from the host by mistake, why 
would we care? We are already sure this message is coming from the GuC, 
no matter what the bit says. IMO if you want this kind of compliance 
coverage it should be in a test, preferably on the GuC side of things.

>>> +            ret = -EPROTO;
>>> +            goto out;
>>> +        }
>>> +        if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
>>> +            ret = -ESTALE;
>>> +            goto out;
>>> +        }
>>> +#undef done
>>> +    }
>>> +
>>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) ==
>>> GUC_MMIO_MSG_TYPE_ERROR) {
>>> +        u32 status = FIELD_GET(GUC_MMIO_ERROR_STATUS, header);
>>> +
>>> +        ret = guc_status_to_errno(status);
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) !=
>>> GUC_MMIO_MSG_TYPE_RESPONSE) {
>>> +        ret = -EPROTO;
>>>            goto out;
>>>        }
>>>    @@ -460,12 +504,17 @@ int intel_guc_send_mmio(struct intel_guc *guc,
>>> const u32 *action, u32 len,
>>>        }
>>>          /* Use data from the GuC response as our return value */
>>> -    ret = INTEL_GUC_MSG_TO_DATA(status);
>> INTEL_GUC_MSG_TO_DATA is still used in intel_guc_ct.c. Either stick to
>> that or update the other use-case to the new define.
> this new define is for MMIO messages, changes to CTB messages will
> follow soon
>
>>> +    ret = FIELD_GET(GUC_MMIO_RESPONSE_DATA0, header);
>>>      out:
>>>        intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
>>>        mutex_unlock(&guc->send_mutex);
>>>    +    if (unlikely(ret < 0))
>>> +        drm_err(&i915->drm,
>>> +            "GuC MMIO action %#06x failed with error %d %#x\n",
>>> +            *request, ret, header);
>>> +
>>>        return ret;
>>>    }
>>>    diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>>> new file mode 100644
>>> index 000000000000..95043788e0ad
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
>>> @@ -0,0 +1,215 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2020 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_GUC_ABI_H__
>>> +#define __INTEL_GUC_ABI_H__
>>> +
>>> +/**
>>> + * DOC: GuC MMIO based communication
>>> + *
>>> + * The MMIO based communication between Host and GuC relies on special
>>> + * hardware registers which format could be defined by the software
>>> + * (so called scratch registers).
>>> + *
>>> + * Each MMIO based message, both Host to GuC (H2G) and GuC to Host (G2H)
>>> + * messages, which maximum length depends on number of available scratch
>>> + * registers, is directly written into those scratch registers.
>>> + *
>>> + * For Gen9+, there are 16 software scratch registers 0xC180-0xC1B8,
>>> + * but no H2G command takes more than 8 parameters and the GuC firmware
>>> + * itself uses an 8-element array to store the H2G message.
>>> + *
>>> + * For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
>>> + * are, regardless on lower count, preffered over legacy ones.
>>> + *
>>> + * The MMIO based communication is mainly used during driver
>>> initialization
>>> + * phase to setup the CTB based communication that will be used
>>> afterwards.
>>> + *
>>> + * Details of the messages formats depend on GuC firmware version
>>> being used
>>> + * by the host driver. Documented here messages are for GuC 45.0 and
>>> newer.
>>> + */
>>> +
>>> +#define GUC_MAX_MMIO_MSG_LEN            8
>> Are we ok with not having an INTEL_ prefix to all the defines? I'm not
>> complaining, we usually stick with that prefix but I'm not sure if it is
>> an iron rule or not.
> existing defines in intel_guc_fwif.h have "GUC" and "INTEL_GUC" (or
> none) prefixes and at the same time almost all structs have "guc"
> prefix, so the best option IMHO is to stick with just guc/GUC prefix.

Ok for me, but we'll need to migrate the other defines as well so we're 
consistent.

>>> +
>>> +/**
>>> + * DOC: GuC MMIO message format
>>> + *
>>> + * Bits of the first scratch register are treated as a message header,
>>> + * and other registers values are used to hold message payload (data)::
>>> + *
>>> + *      +=======================================================+
>>> + *      |  SCRATCH  |                                           |
>>> + *      +-----------+-------------------------------------------+
>>> + *      | 0 | 31:28 | message ORIGIN/TYPE                       |
>>> + *      |   | 27:24 | message MAGIC                             |
>>> + *      |   |  23:0 | message DATA0 (depends on TYPE)           |
>>> + *      +---+-------+-------------------------------------------+
>>> + *      | 1 |  31:0 | message DATA1 (depends on TYPE)           |
>>> + *      |...|       | message DATA2 (depends on TYPE)           |
>>> + *      | n |       | message DATA3 (depends on TYPE)           |
>>> + *      +=======================================================+
>>> + *
>>> + * Where:
>>> + *  - **TYPE** is a 4b message type identifier.
>>> + *  - **MAGIC** is a 4b message sequence number.
>>> + *    The same sequence number must be included in all related messages.
>>> + *    This field is used for tracking and debug purposes.
>> I don't think "magic" is a clear enough name for the role of the field.
>> "Request id" or something like that would be more appropriate IMO.
> we are following naming from GuC spec
>
>>> + *  - **DATA0..3** bits represents message payload.
>>> + *    Definitions of these bits depends on message **TYPE**.
>>> + *
>>> + * The MSB of the header and **TYPE** indicates origin of the message:
>>> + *  - 0 - message from the Host
>>> + *  - 1 - message from the GuC
>>> + *
>>> + * Currently supported message types are:
>>> + *  - 0x0 - **REQUEST** - represents Host request message to the GuC
>>> + *  - 0xF - **SUCCESS RESPONSE** - GuC reply for the earlier request
>>> + *  - 0xE - **ERROR RESPONSE** - GuC failure reply for the earlier
>>> request
>>> + *  - 0xB - **BUSY** - GuC will be processing request for the longer
>>> time
>>> + */
>>> +
>>> +#define GUC_MMIO_MSG_ORIGIN            (0x1 << 31)
>>> +#define   GUC_MMIO_MSG_ORIGIN_HOST        0u
>>> +#define   GUC_MMIO_MSG_ORIGIN_GUC        1u
>>> +
>>> +#define GUC_MMIO_MSG_TYPE            (0xF << 28)
>>> +#define   GUC_MMIO_MSG_TYPE_REQUEST        0x0
>>> +#define   GUC_MMIO_MSG_TYPE_RESPONSE        0xF
>>> +#define   GUC_MMIO_MSG_TYPE_ERROR        0xE
>>> +#define   GUC_MMIO_MSG_TYPE_BUSY        0xB
>>> +
>>> +#define GUC_MMIO_MSG_MAGIC            (0xF << 24)
>>> +#define   GUC_MMIO_MSG_MAGIC_DEFAULT        0u
>>> +
>>> +/**
>>> + * DOC: GuC MMIO H2G REQUEST
>>> + *
>>> + * The MMIO H2G REQUEST message is used by the Host to request some
>>> action
>>> + * to be performed by the GuC::
>>> + *
>>> + *      +=======================================================+
>>> + *      |  SCRATCH  |                                           |
>>> + *      +=======================================================+
>>> + *      | 0 | 31:28 | message TYPE = 0x0 = REQUEST              |
>>> + *      |   | 27:24 | message MAGIC                             |
>>> + *      |   | 23:16 | request SUBCODE (depends on ACTION)       |
>>> + *      |   |  15:0 | request ACTION code                       |
>>> + *      +-------------------------------------------------------+
>>> + *      | 1 |  31:0 | request DATA1 (depends on ACTION/SUBCODE) |
>>> + *      |...|       | request DATA2 (depends on ACTION/SUBCODE) |
>>> + *      | n |  31:0 | request DATA3 (depends on ACTION/SUBCODE) |
>>> + *      +=======================================================+
>>> + *
>>> + * Where:
>>> + *  - **TYPE** must be set to value 0x0.
>>> + *  - **MAGIC** message sequence number is generated by the host.
>>> + *  - **ACTION** represents 16b request action code that defines both
>>> + *    purpose of the request and format of the passed payload data.
>>> + *    List of supported codes depends on GuC version and plaform.
>>> + *    Action code can't be zero.
>>> + *  - **SUBCODE** is optional 8b data related to the **ACTION**.
>>> + *    MBZ if not explicitly defined by given **ACTION**.
>>> + *  - **DATA1..3** are optional 32b payload dwords related to
>>> **ACTION**.
>>> + *    Format of each dword is defined by specific **ACTION**.
>>> + */
>>> +
>>> +#define GUC_MMIO_REQUEST_SUBCODE        (0xFF << 16)
>>> +#define GUC_MMIO_REQUEST_ACTION            (0xFFFF << 0)
>>> +#define   GUC_ACTION_INVALID            0x0000
>>> +#define   GUC_ACTION_REGISTER_CTB        0x4505
>>> +#define   GUC_ACTION_DEREGISTER_CTB        0x4506
>> These are now defined twice (once here and once in the enum). We should
>> pick one for consistency.
> we can drop old ones from intel_guc_fwif.h
>
>>> +
>>> +/**
>>> + * DOC: GuC MMIO G2H SUCCESS RESPONSE
>>> + *
>>> + * The MMIO G2H SUCCESS RESPONSE message is used by the GuC to reply to
>>> + * the earlier H2G request message. This message is used if no errors
>>> + * were encountered by the GuC during processing of the request::
>>> + *
>>> + *      +=======================================================+
>>> + *      |  SCRATCH  |                                           |
>>> + *      +=======================================================+
>>> + *      | 0 | 31:28 | message TYPE = 0xF = SUCCESS RESPONSE     |
>>> + *      |   | 27:24 | message MAGIC                             |
>>> + *      |   |  23:0 | response DATA0 (depends on ACTION/SUBCODE)|
>>> + *      +-------------------------------------------------------+
>>> + *      | 1 |  31:0 | response DATA1 (depends on ACTION/SUBCODE)|
>>> + *      |...|       | response DATA2 (depends on ACTION/SUBCODE)|
>>> + *      | n |  31:0 | response DATA3 (depends on ACTION/SUBCODE)|
>>> + *      +=======================================================+
>>> + *
>>> + * Where:
>>> + *  - **TYPE** must be set to value 0xF.
>>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>>> message.
>>> + *  - **DATA** is optional payload data related to **ACTION**.
>>> + *    Format is defined by each **ACTION** separately.
>>> + */
>>> +
>>> +#define GUC_MMIO_RESPONSE_DATA0            (0xFFFFFF << 0)
>>> +
>>> +/**
>>> + * DOC: GuC MMIO G2H ERROR RESPONSE
>>> + *
>>> + * The MMIO G2H ERROR RESPONSE message is used by the GuC to reply to
>>> + * the earlier H2G request message. This message is used if some errors
>>> + * were encountered by the GuC during processing of the request::
>>> + *
>>> + *      +=======================================================+
>>> + *      |  SCRATCH  |                                           |
>>> + *      +=======================================================+
>>> + *      | 0 | 31:28 | message TYPE = 0xE = ERROR RESPONSE       |
>>> + *      |   | 27:24 | message MAGIC                             |
>>> + *      |   | 23:16 | reserved MBZ                              |
>>> + *      |   |  15:0 | response STATUS failure code              |
>>> + *      +-------------------------------------------------------+
>>> + *      | 1 |  31:0 | reserved MBZ                              |
>>> + *      |...|       | reserved MBZ                              |
>>> + *      | n |  31:0 | reserved MBZ                              |
>>> + *      +=======================================================+
>>> + *
>>> + * Where:
>>> + *  - **TYPE** must be set to value 0xE.
>>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>>> message.
>>> + *  - **STATUS** represents non-zero failure code.
>>> + */
>>> +
>>> +#define GUC_MMIO_ERROR_STATUS            (0xFFFF << 0)
>>> +#define   GUC_STATUS_UNKNOWN_ACTION        0x30
>>> +#define   GUC_STATUS_INVALID_PARAMS        0x60
>>> +#define   GUC_STATUS_INVALID_ADDR        0x80
>>> +#define   GUC_STATUS_CTX_NOT_REGISTERED        0x100
>>> +#define   GUC_STATUS_NO_LOCAL_MEMORY        0x200
>>> +#define   GUC_STATUS_NO_VIRTUALIZATION        0x300
>>> +#define   GUC_STATUS_CTB_FULL            0x301
>>> +#define   GUC_STATUS_UNAUTHORIZED_REQUEST    0x302
>>> +#define   GUC_STATUS_GENERIC_FAIL        0xF000
>>> +
>>> +/**
>>> + * DOC: MMIO G2H BUSY RESPONSE
>>> + *
>>> + * The MMIO G2H BUSY RESPONSE message is used by the GuC to reply to
>>> + * the earlier H2G request message. This message is used if processing
>>> + * of the request will take longer time and final SUCCESS/ERROR response
>>> + * message will be delivered later::
>>> + *
>>> + *      +=======================================================+
>>> + *      |  SCRATCH  |                                           |
>>> + *      +=======================================================+
>>> + *      | 0 | 31:28 | message TYPE = 0xB = BUSY RESPONSE        |
>>> + *      |   | 27:24 | message MAGIC                             |
>>> + *      |   |  23:0 | reserved MBZ                              |
>>> + *      +-------------------------------------------------------+
>>> + *      | 1 |  31:0 | reserved MBZ                              |
>>> + *      |...|       | reserved MBZ                              |
>>> + *      | n |  31:0 | reserved MBZ                              |
>>> + *      +=======================================================+
>>> + *
>>> + * Where:
>>> + *  - **TYPE** must be set to value 0xB.
>>> + *  - **MAGIC** must match value used by the host in **REQUEST**
>>> message.
>>> + *  - all other bits are reserved as MBZ.
>>> + */
>>> +
>>> +#endif /* __INTEL_GUC_ABI_H__ */
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>>> index d44061033f23..e66cbf1be320 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>>> @@ -10,11 +10,52 @@
>>>      /*
>>>     * The Additional Data Struct (ADS) has pointers for different
>>> buffers used by
>>> - * the GuC. One single gem object contains the ADS struct itself
>>> (guc_ads), the
>>> - * scheduling policies (guc_policies), a structure describing a
>>> collection of
>>> - * register sets (guc_mmio_reg_state) and some extra pages for the
>>> GuC to save
>>> - * its internal state for sleep.
>>> + * the GuC. One single gem object contains the ADS struct itself
>>> (guc_ads) and
>>> + * all the extra buffers indirectly linked via the ADS struct's entires.
>>> + *
>>> + * Layout of the ADS blob allocated for the GuC:
>>> + *
>>> + *      +---------------------------------------+ <== base
>>> + *      | guc_ads                               |
>>> + *      +---------------------------------------+
>>> + *      | guc_policies                          |
>>> + *      +---------------------------------------+
>>> + *      | guc_gt_system_info                    |
>>> + *      +---------------------------------------+
>>> + *      | guc_clients_info                      |
>>> + *      +---------------------------------------+
>>> + *      | guc_ct_pool_entry[size]               |
>>> + *      +---------------------------------------+
>>> + *      | padding                               |
>>> + *      +---------------------------------------+ <== 4K aligned
>>> + *      | private data                          |
>>> + *      +---------------------------------------+
>>> + *      | padding                               |
>>> + *      +---------------------------------------+ <== 4K aligned
>>>     */
>> I think it we could expand the comment (or add a new one) to make clear
>> that the private data is not included in the __guc_ads_blob to keep
>> things simple since it is variable in size, but it is still part of the
>> ads_blobpasses to GuC.
>>
>>> +struct __guc_ads_blob {
>>> +    struct guc_ads ads;
>>> +    struct guc_policies policies;
>>> +    struct guc_gt_system_info system_info;
>>> +    struct guc_clients_info clients_info;
>>> +    struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
>>> +} __packed;
>>> +
>>> +static u32 guc_ads_private_data_size(struct intel_guc *guc)
>>> +{
>>> +    return PAGE_ALIGN(guc->fw.private_data_size);
>>> +}
>>> +
>>> +static u32 guc_ads_private_data_offset(struct intel_guc *guc)
>>> +{
>>> +    return PAGE_ALIGN(sizeof(struct __guc_ads_blob));
>>> +}
>>> +
>>> +static u32 guc_ads_blob_size(struct intel_guc *guc)
>>> +{
>>> +    return guc_ads_private_data_offset(guc) +
>>> +           guc_ads_private_data_size(guc);
>>> +}
>>>      static void guc_policy_init(struct guc_policy *policy)
>>>    {
>>> @@ -48,23 +89,33 @@ static void guc_ct_pool_entries_init(struct
>>> guc_ct_pool_entry *pool, u32 num)
>>>        memset(pool, 0, num * sizeof(*pool));
>>>    }
>>>    +static void guc_mapping_table_init(struct intel_gt *gt,
>>> +                   struct guc_gt_system_info *system_info)
>>> +{
>>> +    unsigned int i, j;
>>> +    struct intel_engine_cs *engine;
>>> +    enum intel_engine_id id;
>>> +
>>> +    /* Table must be set to invalid values for entries not used */
>>> +    for (i = 0; i < GUC_MAX_ENGINE_CLASSES; ++i)
>>> +        for (j = 0; j < GUC_MAX_INSTANCES_PER_CLASS; ++j)
>>> +            system_info->mapping_table[i][j] =
>>> +                GUC_MAX_INSTANCES_PER_CLASS;
>>> +
>>> +    for_each_engine(engine, gt, id) {
>>> +        u8 guc_class = engine->class;
>>> +
>>> +        system_info->mapping_table[guc_class][engine->instance]
>>> +            = engine->instance;
>>> +    }
>>> +}
>>> +
>>>    /*
>>>     * The first 80 dwords of the register state context, containing the
>>>     * execlists and ppgtt registers.
>>>     */
>>>    #define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>>>    -/* The ads obj includes the struct itself and buffers passed to GuC */
>>> -struct __guc_ads_blob {
>>> -    struct guc_ads ads;
>>> -    struct guc_policies policies;
>>> -    struct guc_mmio_reg_state reg_state;
>>> -    struct guc_gt_system_info system_info;
>>> -    struct guc_clients_info clients_info;
>>> -    struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
>>> -    u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>>> -} __packed;
>>> -
>>>    static void __guc_ads_init(struct intel_guc *guc)
>>>    {
>>>        struct intel_gt *gt = guc_to_gt(guc);
>>> @@ -107,6 +158,16 @@ static void __guc_ads_init(struct intel_guc *guc)
>>>        blob->system_info.vebox_enable_mask = VEBOX_MASK(gt);
>>>        blob->system_info.vdbox_sfc_support_mask =
>>> gt->info.vdbox_sfc_access;
>>>    +    if (INTEL_GEN(gt->i915) >= 12 && !IS_DGFX(gt->i915)) {
>>> +        u32 distdbreg = intel_uncore_read(gt->uncore,
>>> +                          GEN12_DIST_DBS_POPULATED);
>>> +        blob->system_info.num_of_doorbells_per_sqidi =
>>> +            ((distdbreg >> GEN12_DOORBELLS_PER_SQIDI_SHIFT) &
>>> +             GEN12_DOORBELLS_PER_SQIDI) + 1;
>>> +    }
>>> +
>>> +    guc_mapping_table_init(guc_to_gt(guc), &blob->system_info);
>>> +
>>>        base = intel_guc_ggtt_offset(guc, guc->ads_vma);
>>>          /* Clients info  */
>>> @@ -118,11 +179,12 @@ static void __guc_ads_init(struct intel_guc *guc)
>>>          /* ADS */
>>>        blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
>>> -    blob->ads.reg_state_buffer = base + ptr_offset(blob,
>>> reg_state_buffer);
>>> -    blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
>>>        blob->ads.gt_system_info = base + ptr_offset(blob, system_info);
>>>        blob->ads.clients_info = base + ptr_offset(blob, clients_info);
>>>    +    /* Private Data */
>>> +    blob->ads.private_data = base + guc_ads_private_data_offset(guc);
>>> +
>>>        i915_gem_object_flush_map(guc->ads_vma->obj);
>>>    }
>>>    @@ -135,14 +197,15 @@ static void __guc_ads_init(struct intel_guc *guc)
>>>     */
>>>    int intel_guc_ads_create(struct intel_guc *guc)
>>>    {
>>> -    const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
>>> +    u32 size;
>>>        int ret;
>>>          GEM_BUG_ON(guc->ads_vma);
>>>    +    size = guc_ads_blob_size(guc);
>>> +
>>>        ret = intel_guc_allocate_and_map_vma(guc, size, &guc->ads_vma,
>>>                             (void **)&guc->ads_blob);
>>> -
>>>        if (ret)
>>>            return ret;
>>>    @@ -156,6 +219,18 @@ void intel_guc_ads_destroy(struct intel_guc *guc)
>>>        i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP);
>>>    }
>>>    +void guc_ads_private_data_reset(struct intel_guc *guc)
>>> +{
>>> +    u32 size;
>>> +
>>> +    size = guc_ads_private_data_size(guc);
>>> +    if (!size)
>>> +        return;
>>> +
>>> +    memset((void *)guc->ads_blob + guc_ads_private_data_offset(guc), 0,
>>> +           size);
>>> +}
>>> +
>>>    /**
>>>     * intel_guc_ads_reset() - prepares GuC Additional Data Struct for
>>> reuse
>>>     * @guc: intel_guc struct
>>> @@ -168,5 +243,8 @@ void intel_guc_ads_reset(struct intel_guc *guc)
>>>    {
>>>        if (!guc->ads_vma)
>>>            return;
>>> +
>>>        __guc_ads_init(guc);
>>> +
>>> +    guc_ads_private_data_reset(guc);
>>>    }
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>>> index d4a87f4c9421..212de0a939bf 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>>> @@ -74,8 +74,9 @@ static inline bool guc_ready(struct intel_uncore
>>> *uncore, u32 *status)
>>>            ((val & GS_MIA_CORE_STATE) && (uk_val ==
>>> GS_UKERNEL_LAPIC_DONE));
>>>    }
>>>    -static int guc_wait_ucode(struct intel_uncore *uncore)
>>> +static int guc_wait_ucode(struct intel_gt *gt)
>>>    {
>>> +    struct intel_uncore *uncore = gt->uncore;
>> The gt seems to be used only for gt->i915, why not just use uncore->i915?
>>
>>>        u32 status;
>>>        int ret;
>>>    @@ -93,12 +94,24 @@ static int guc_wait_ucode(struct intel_uncore
>>> *uncore)
>>>        if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>>>            DRM_ERROR("GuC firmware signature verification failed\n");
>>>            ret = -ENOEXEC;
>>> -    }
>>> -
>>> +    } else
>>>        if ((status & GS_UKERNEL_MASK) == GS_UKERNEL_EXCEPTION) {
>>>            DRM_ERROR("GuC firmware exception. EIP: %#x\n",
>>>                  intel_uncore_read(uncore, SOFT_SCRATCH(13)));
>>>            ret = -ENXIO;
>>> +    } else
>>> +    if (ret) {
>>> +        i915_probe_error(gt->i915, "GuC load failed: status = 0x%08X\n",
>>> +                 status);
>>> +        i915_probe_error(gt->i915, "GuC status: Reset = %d, "
>>> +                 "BootROM = 0x%02X, UKernel = 0x%02X, "
>>> +                 "MIA = 0x%02X, Auth = 0x%02X\n",
>>> +                 (status >> GS_RESET_SHIFT) & 1,
>>> +                 (status & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT,
>>> +                 (status & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT,
>>> +                 (status & GS_MIA_MASK) >> GS_MIA_SHIFT,
>>> +                 (status & GS_AUTH_STATUS_MASK) >>
>>> +                              GS_AUTH_STATUS_SHIFT);
>> nit: I think using the FIELD_GET macro here as well could make things
>> cleaner
>>
>>>        }
>>>          return ret;
>>> @@ -139,7 +152,7 @@ int intel_guc_fw_upload(struct intel_guc *guc)
>>>        if (ret)
>>>            goto out;
>>>    -    ret = guc_wait_ucode(uncore);
>>> +    ret = guc_wait_ucode(gt);
>>>        if (ret)
>>>            goto out;
>>>    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 a6b733c146c9..77e49d53bb5c 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>> @@ -27,7 +27,7 @@
>>>    #define GUC_MAX_ENGINES_NUM        (GUC_VIDEO_ENGINE2 + 1)
>>>      #define GUC_MAX_ENGINE_CLASSES        5
>>> -#define GUC_MAX_INSTANCES_PER_CLASS    16
>>> +#define GUC_MAX_INSTANCES_PER_CLASS    32
>>>      #define GUC_DOORBELL_INVALID        256
>>>    @@ -62,12 +62,7 @@
>>>    #define GUC_STAGE_DESC_ATTR_PCH        BIT(6)
>>>    #define GUC_STAGE_DESC_ATTR_TERMINATED    BIT(7)
>>>    -/* New GuC control data */
>>> -#define GUC_CTL_CTXINFO            0
>>> -#define   GUC_CTL_CTXNUM_IN16_SHIFT    0
>>> -#define   GUC_CTL_BASE_ADDR_SHIFT    12
>>> -
>>> -#define GUC_CTL_LOG_PARAMS        1
>>> +#define GUC_CTL_LOG_PARAMS        0
>>>    #define   GUC_LOG_VALID            (1 << 0)
>>>    #define   GUC_LOG_NOTIFY_ON_HALF_FULL    (1 << 1)
>>>    #define   GUC_LOG_ALLOC_IN_MEGABYTE    (1 << 3)
>>> @@ -79,11 +74,11 @@
>>>    #define   GUC_LOG_ISR_MASK            (0x7 << GUC_LOG_ISR_SHIFT)
>>>    #define   GUC_LOG_BUF_ADDR_SHIFT    12
>>>    -#define GUC_CTL_WA            2
>>> -#define GUC_CTL_FEATURE            3
>>> +#define GUC_CTL_WA            1
>>> +#define GUC_CTL_FEATURE            2
>>>    #define   GUC_CTL_DISABLE_SCHEDULER    (1 << 14)
>>>    -#define GUC_CTL_DEBUG            4
>>> +#define GUC_CTL_DEBUG            3
>>>    #define   GUC_LOG_VERBOSITY_SHIFT    0
>>>    #define   GUC_LOG_VERBOSITY_LOW        (0 << GUC_LOG_VERBOSITY_SHIFT)
>>>    #define   GUC_LOG_VERBOSITY_MED        (1 << GUC_LOG_VERBOSITY_SHIFT)
>>> @@ -97,12 +92,31 @@
>>>    #define   GUC_LOG_DISABLED        (1 << 6)
>>>    #define   GUC_PROFILE_ENABLED        (1 << 7)
>>>    -#define GUC_CTL_ADS            5
>>> +#define GUC_CTL_ADS            4
>>>    #define   GUC_ADS_ADDR_SHIFT        1
>>>    #define   GUC_ADS_ADDR_MASK        (0xFFFFF << GUC_ADS_ADDR_SHIFT)
>>>      #define GUC_CTL_MAX_DWORDS        (SOFT_SCRATCH_COUNT - 2) /*
>>> [1..14] */
>>>    +/*
>>> + * The class goes in bits [0..2] of the GuC ID, the instance in bits
>>> [3..6].
>>> + * Bit 7 can be used for operations that apply to all engine
>>> classes&instances.
>>> + */
>>> +#define GUC_ENGINE_CLASS_SHIFT        0
>>> +#define GUC_ENGINE_CLASS_MASK        (0x7 << GUC_ENGINE_CLASS_SHIFT)
>>> +#define GUC_ENGINE_INSTANCE_SHIFT    3
>>> +#define GUC_ENGINE_INSTANCE_MASK    (0xf << GUC_ENGINE_INSTANCE_SHIFT)
>>> +#define GUC_ENGINE_ALL_INSTANCES    (1 << 7)
>>> +
>>> +#define MAKE_GUC_ID(class, instance) \
>>> +    (((class) << GUC_ENGINE_CLASS_SHIFT) | \
>>> +     ((instance) << GUC_ENGINE_INSTANCE_SHIFT))
>>> +
>>> +#define GUC_ID_TO_ENGINE_CLASS(guc_id) \
>>> +    (((guc_id) & GUC_ENGINE_CLASS_MASK) >> GUC_ENGINE_CLASS_SHIFT)
>>> +#define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
>>> +    (((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
>>> +
>>>    /* Work item for submitting workloads into work queue of GuC. */
>>>    struct guc_wq_item {
>>>        u32 header;
>>> @@ -338,7 +352,6 @@ struct guc_policies {
>>>    /* GuC MMIO reg state struct */
>>>      -#define GUC_REGSET_MAX_REGISTERS    64
>>>    #define GUC_S3_SAVE_SPACE_PAGES        10
>>>      struct guc_mmio_reg {
>>> @@ -348,16 +361,17 @@ struct guc_mmio_reg {
>>>    #define GUC_REGSET_MASKED        (1 << 0)
>>>    } __packed;
>>>    -struct guc_mmio_regset {
>>> -    struct guc_mmio_reg registers[GUC_REGSET_MAX_REGISTERS];
>>> -    u32 values_valid;
>>> -    u32 number_of_registers;
>>> -} __packed;
>>> -
>>>    /* GuC register sets */
>>> -struct guc_mmio_reg_state {
>>> -    struct guc_mmio_regset
>>> engine_reg[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>>> -    u32 reserved[98];
>>> +struct guc_mmio_reg_set {
>>> +    u32 address;
>>> +    union {
>>> +        struct {
>>> +            u32 count:16;
>>> +            u32 reserved1:12;
>>> +            u32 reserved2:4;
>>> +        };
>>> +        u32 count_u32;
>>> +    };
>>>    } __packed;
>>>      /* HW info */
>>> @@ -369,7 +383,9 @@ struct guc_gt_system_info {
>>>        u32 vdbox_enable_mask;
>>>        u32 vdbox_sfc_support_mask;
>>>        u32 vebox_enable_mask;
>>> -    u32 reserved[9];
>>> +    u32 num_of_doorbells_per_sqidi;
>>> +    u8
>>> mapping_table[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>>> +    u32 reserved2[8];
>>>    } __packed;
>>>      /* Clients info */
>>> @@ -390,15 +406,16 @@ struct guc_clients_info {
>>>      /* GuC Additional Data Struct */
>>>    struct guc_ads {
>>> -    u32 reg_state_addr;
>>> -    u32 reg_state_buffer;
>>> +    struct guc_mmio_reg_set
>>> reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>>> +    u32 reserved0;
>>>        u32 scheduler_policies;
>>>        u32 gt_system_info;
>>>        u32 clients_info;
>>>        u32 control_data;
>>>        u32 golden_context_lrca[GUC_MAX_ENGINE_CLASSES];
>>>        u32 eng_state_size[GUC_MAX_ENGINE_CLASSES];
>>> -    u32 reserved[16];
>>> +    u32 private_data;
>>> +    u32 reserved[15];
>>>    } __packed;
>>>      /* GuC logging structures */
>>> @@ -474,49 +491,6 @@ struct guc_shared_ctx_data {
>>>        struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>>>    } __packed;
>>>    -/**
>>> - * DOC: MMIO based communication
>>> - *
>>> - * The MMIO based communication between Host and GuC uses software
>>> scratch
>>> - * registers, where first register holds data treated as message header,
>>> - * and other registers are used to hold message payload.
>>> - *
>>> - * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
>>> - * but no H2G command takes more than 8 parameters and the GuC FW
>>> - * itself uses an 8-element array to store the H2G message.
>>> - *
>>> - *      +-----------+---------+---------+---------+
>>> - *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
>>> - *      +-----------+---------+---------+---------+
>>> - *      | header    |      optional payload       |
>>> - *      +======+====+=========+=========+=========+
>>> - *      | 31:28|type|         |         |         |
>>> - *      +------+----+         |         |         |
>>> - *      | 27:16|data|         |         |         |
>>> - *      +------+----+         |         |         |
>>> - *      |  15:0|code|         |         |         |
>>> - *      +------+----+---------+---------+---------+
>>> - *
>>> - * The message header consists of:
>>> - *
>>> - * - **type**, indicates message type
>>> - * - **code**, indicates message code, is specific for **type**
>>> - * - **data**, indicates message data, optional, depends on **code**
>>> - *
>>> - * The following message **types** are supported:
>>> - *
>>> - * - **REQUEST**, indicates Host-to-GuC request, requested GuC action
>>> code
>>> - *   must be priovided in **code** field. Optional action specific
>>> parameters
>>> - *   can be provided in remaining payload registers or **data** field.
>>> - *
>>> - * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC
>>> request,
>>> - *   action response status will be provided in **code** field. Optional
>>> - *   response data can be returned in remaining payload registers or
>>> **data**
>>> - *   field.
>>> - */
>>> -
>>> -#define GUC_MAX_MMIO_MSG_LEN        8
>>> -
>>>    #define INTEL_GUC_MSG_TYPE_SHIFT    28
>>>    #define INTEL_GUC_MSG_TYPE_MASK        (0xF <<
>>> INTEL_GUC_MSG_TYPE_SHIFT)
>>>    #define INTEL_GUC_MSG_DATA_SHIFT    16
>> These GUC_MSG defines are also duplicated
> These are still used by the CTB and we want to keep CTB definitions
> separate as we will do some refactoring there too.

Is the GuC changing those defines for the CTB in a future release, or is 
the refactoring purely on the i915 side? in the latter case I'd prefer 
for all the duplication to be removed now until the CTB refactoring comes.

Daniele

> Michal
>
>> Daniele
>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>>> index 1949346e714e..b37fc2ffaef2 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
>>> @@ -118,6 +118,11 @@ struct guc_doorbell_info {
>>>    #define   GEN8_DRB_VALID          (1<<0)
>>>    #define GEN8_DRBREGU(x)            _MMIO(0x1000 + (x) * 8 + 4)
>>>    +#define GEN12_DIST_DBS_POPULATED        _MMIO(0xd08)
>>> +#define   GEN12_DOORBELLS_PER_SQIDI_SHIFT    16
>>> +#define   GEN12_DOORBELLS_PER_SQIDI        (0xff)
>>> +#define   GEN12_SQIDIS_DOORBELL_EXIST        (0xffff)
>>> +
>>>    #define DE_GUCRMR            _MMIO(0x44054)
>>>      #define GUC_BCS_RCS_IER            _MMIO(0xC550)
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>> index 59b27aba15c6..f7cb0b1f1ba5 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>> @@ -44,23 +44,19 @@ void intel_uc_fw_change_status(struct intel_uc_fw
>>> *uc_fw,
>>>     * List of required GuC and HuC binaries per-platform.
>>>     * Must be ordered based on platform + revid, from newer to older.
>>>     *
>>> - * TGL 35.2 is interface-compatible with 33.0 for previous Gens. The
>>> deltas
>>> - * between 33.0 and 35.2 are only related to new additions to support
>>> new Gen12
>>> - * features.
>>> - *
>>>     * Note that RKL uses the same firmware as TGL.
>>>     */
>>>    #define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
>>> -    fw_def(ROCKETLAKE,  0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7,
>>> 0, 12)) \
>>> -    fw_def(TIGERLAKE,   0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7,
>>> 0, 12)) \
>>> -    fw_def(ELKHARTLAKE, 0, guc_def(ehl, 33, 0, 4), huc_def(ehl,  9,
>>> 0, 0)) \
>>> -    fw_def(ICELAKE,     0, guc_def(icl, 33, 0, 0), huc_def(icl,  9,
>>> 0, 0)) \
>>> -    fw_def(COMETLAKE,   5, guc_def(cml, 33, 0, 0), huc_def(cml,  4,
>>> 0, 0)) \
>>> -    fw_def(COFFEELAKE,  0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4,
>>> 0, 0)) \
>>> -    fw_def(GEMINILAKE,  0, guc_def(glk, 33, 0, 0), huc_def(glk,  4,
>>> 0, 0)) \
>>> -    fw_def(KABYLAKE,    0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4,
>>> 0, 0)) \
>>> -    fw_def(BROXTON,     0, guc_def(bxt, 33, 0, 0), huc_def(bxt,  2,
>>> 0, 0)) \
>>> -    fw_def(SKYLAKE,     0, guc_def(skl, 33, 0, 0), huc_def(skl,  2,
>>> 0, 0))
>>> +    fw_def(ROCKETLAKE,  0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7,
>>> 0, 12)) \
>>> +    fw_def(TIGERLAKE,   0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7,
>>> 0, 12)) \
>>> +    fw_def(ELKHARTLAKE, 0, guc_def(ehl, 45, 0, 0), huc_def(ehl,  9,
>>> 0, 0)) \
>>> +    fw_def(ICELAKE,     0, guc_def(icl, 45, 0, 0), huc_def(icl,  9,
>>> 0, 0)) \
>>> +    fw_def(COMETLAKE,   5, guc_def(cml, 45, 0, 0), huc_def(cml,  4,
>>> 0, 0)) \
>>> +    fw_def(COFFEELAKE,  0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4,
>>> 0, 0)) \
>>> +    fw_def(GEMINILAKE,  0, guc_def(glk, 45, 0, 0), huc_def(glk,  4,
>>> 0, 0)) \
>>> +    fw_def(KABYLAKE,    0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4,
>>> 0, 0)) \
>>> +    fw_def(BROXTON,     0, guc_def(bxt, 45, 0, 0), huc_def(bxt,  2,
>>> 0, 0)) \
>>> +    fw_def(SKYLAKE,     0, guc_def(skl, 45, 0, 0), huc_def(skl,  2,
>>> 0, 0))
>>>      #define __MAKE_UC_FW_PATH(prefix_, name_, major_, minor_, patch_) \
>>>        "i915/" \
>>> @@ -371,6 +367,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>>>            }
>>>        }
>>>    +    if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
>>> +        uc_fw->private_data_size = css->private_data_size;
>>> +
>>>        obj = i915_gem_object_create_shmem_from_data(i915, fw->data,
>>> fw->size);
>>>        if (IS_ERR(obj)) {
>>>            err = PTR_ERR(obj);
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>> index 23d3a423ac0f..99bb1fe1af66 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>> @@ -88,6 +88,8 @@ struct intel_uc_fw {
>>>          u32 rsa_size;
>>>        u32 ucode_size;
>>> +
>>> +    u32 private_data_size;
>>>    };
>>>      #ifdef CONFIG_DRM_I915_DEBUG_GUC
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>>> index 029214cdedd5..e41ffc7a7fbc 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
>>> @@ -69,7 +69,11 @@ struct uc_css_header {
>>>    #define CSS_SW_VERSION_UC_MAJOR        (0xFF << 16)
>>>    #define CSS_SW_VERSION_UC_MINOR        (0xFF << 8)
>>>    #define CSS_SW_VERSION_UC_PATCH        (0xFF << 0)
>>> -    u32 reserved[14];
>>> +    u32 reserved0[13];
>>> +    union {
>>> +        u32 private_data_size; /* only applies to GuC */
>>> +        u32 reserved1;
>>> +    };
>>>        u32 header_info;
>>>    } __packed;
>>>    static_assert(sizeof(struct uc_css_header) == 128);

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index ea4ba2afe9f9..9cd62d68abc6 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -305,8 +305,9 @@  static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
 	engine->i915 = i915;
 	engine->gt = gt;
 	engine->uncore = gt->uncore;
-	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
+	engine->hw_id = info->hw_id;
+	engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
 
 	engine->class = info->class;
 	engine->instance = info->instance;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 861657897c0f..8d30e1d7d8a6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -7,6 +7,7 @@ 
 #include "gt/intel_gt_irq.h"
 #include "gt/intel_gt_pm_irq.h"
 #include "intel_guc.h"
+#include "intel_guc_abi.h"
 #include "intel_guc_ads.h"
 #include "intel_guc_submission.h"
 #include "i915_drv.h"
@@ -213,23 +214,6 @@  static u32 guc_ctl_feature_flags(struct intel_guc *guc)
 	return flags;
 }
 
-static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
-{
-	u32 flags = 0;
-
-	if (intel_guc_submission_is_used(guc)) {
-		u32 ctxnum, base;
-
-		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
-		ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;
-
-		base >>= PAGE_SHIFT;
-		flags |= (base << GUC_CTL_BASE_ADDR_SHIFT) |
-			(ctxnum << GUC_CTL_CTXNUM_IN16_SHIFT);
-	}
-	return flags;
-}
-
 static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 {
 	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
@@ -291,7 +275,6 @@  static void guc_init_params(struct intel_guc *guc)
 
 	BUILD_BUG_ON(sizeof(guc->params) != GUC_CTL_MAX_DWORDS * sizeof(u32));
 
-	params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
 	params[GUC_CTL_LOG_PARAMS] = guc_ctl_log_params_flags(guc);
 	params[GUC_CTL_FEATURE] = guc_ctl_feature_flags(guc);
 	params[GUC_CTL_DEBUG] = guc_ctl_debug_flags(guc);
@@ -400,32 +383,65 @@  void intel_guc_fini(struct intel_guc *guc)
 	intel_uc_fw_fini(&guc->fw);
 }
 
+static bool is_valid_mmio_action(u32 action)
+{
+	return action == GUC_ACTION_REGISTER_CTB ||
+	       action == GUC_ACTION_DEREGISTER_CTB;
+}
+
+static int guc_status_to_errno(u32 status)
+{
+	switch (status) {
+	case GUC_STATUS_UNKNOWN_ACTION:
+		return -EOPNOTSUPP;
+	case GUC_STATUS_INVALID_PARAMS:
+		return -EINVAL;
+	case GUC_STATUS_INVALID_ADDR:
+		return -EFAULT;
+	case GUC_STATUS_CTX_NOT_REGISTERED:
+		return -ESRCH;
+	case GUC_STATUS_CTB_FULL:
+		return -ENOSPC;
+	case GUC_STATUS_UNAUTHORIZED_REQUEST:
+		return -EACCES;
+	case GUC_STATUS_GENERIC_FAIL:
+	default:
+		return -ENXIO;
+	}
+}
+
 /*
  * This function implements the MMIO based host to GuC interface.
  */
-int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request, u32 len,
 			u32 *response_buf, u32 response_buf_size)
 {
+	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
 	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
-	u32 status;
+	u32 action = FIELD_GET(GUC_MMIO_REQUEST_ACTION, *request);
+	u32 subcode = FIELD_GET(GUC_MMIO_REQUEST_SUBCODE, *request);
+	u32 magic = GUC_MMIO_MSG_MAGIC_DEFAULT;
+	u32 header;
 	int i;
 	int ret;
 
 	GEM_BUG_ON(!len);
 	GEM_BUG_ON(len > guc->send_regs.count);
 
-	/* We expect only action code */
-	GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
+	/* We expect to use MMIO only for special actions */
+	GEM_BUG_ON(!is_valid_mmio_action(action));
 
-	/* If CT is available, we expect to use MMIO only during init/fini */
-	GEM_BUG_ON(*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
-		   *action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
+	header = FIELD_PREP(GUC_MMIO_MSG_TYPE, GUC_MMIO_MSG_TYPE_REQUEST) |
+		FIELD_PREP(GUC_MMIO_MSG_MAGIC, magic) |
+		FIELD_PREP(GUC_MMIO_REQUEST_SUBCODE, subcode) |
+		FIELD_PREP(GUC_MMIO_REQUEST_ACTION, action);
 
 	mutex_lock(&guc->send_mutex);
 	intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);
 
-	for (i = 0; i < len; i++)
-		intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]);
+	intel_uncore_write(uncore, guc_send_reg(guc, 0), header);
+	for (i = 1; i < len; i++)
+		intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);
 
 	intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));
 
@@ -437,17 +453,45 @@  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 	 */
 	ret = __intel_wait_for_register_fw(uncore,
 					   guc_send_reg(guc, 0),
-					   INTEL_GUC_MSG_TYPE_MASK,
-					   INTEL_GUC_MSG_TYPE_RESPONSE <<
-					   INTEL_GUC_MSG_TYPE_SHIFT,
-					   10, 10, &status);
-	/* If GuC explicitly returned an error, convert it to -EIO */
-	if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
-		ret = -EIO;
+					   GUC_MMIO_MSG_ORIGIN,
+					   FIELD_PREP(GUC_MMIO_MSG_ORIGIN,
+						      GUC_MMIO_MSG_ORIGIN_GUC),
+					   10, 10, &header);
+	if (unlikely(ret))
+		goto out;
 
-	if (ret) {
-		DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
-			  action[0], ret, status);
+	if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
+		ret = -ESTALE;
+		goto out;
+	}
+
+	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) == GUC_MMIO_MSG_TYPE_BUSY) {
+#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc, 0)); \
+		FIELD_GET(GUC_MMIO_MSG_TYPE, header) != GUC_MMIO_MSG_TYPE_BUSY; })
+		ret = wait_for(done, 1000);
+		if (unlikely(ret))
+			goto out;
+		if (unlikely(FIELD_GET(GUC_MMIO_MSG_ORIGIN, header) !=
+				       GUC_MMIO_MSG_ORIGIN_GUC)) {
+			ret = -EPROTO;
+			goto out;
+		}
+		if (unlikely(FIELD_GET(GUC_MMIO_MSG_MAGIC, header) != magic)) {
+			ret = -ESTALE;
+			goto out;
+		}
+#undef done
+	}
+
+	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) == GUC_MMIO_MSG_TYPE_ERROR) {
+		u32 status = FIELD_GET(GUC_MMIO_ERROR_STATUS, header);
+
+		ret = guc_status_to_errno(status);
+		goto out;
+	}
+
+	if (FIELD_GET(GUC_MMIO_MSG_TYPE, header) != GUC_MMIO_MSG_TYPE_RESPONSE) {
+		ret = -EPROTO;
 		goto out;
 	}
 
@@ -460,12 +504,17 @@  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 	}
 
 	/* Use data from the GuC response as our return value */
-	ret = INTEL_GUC_MSG_TO_DATA(status);
+	ret = FIELD_GET(GUC_MMIO_RESPONSE_DATA0, header);
 
 out:
 	intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
+	if (unlikely(ret < 0))
+		drm_err(&i915->drm,
+			"GuC MMIO action %#06x failed with error %d %#x\n",
+			*request, ret, header);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
new file mode 100644
index 000000000000..95043788e0ad
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_abi.h
@@ -0,0 +1,215 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#ifndef __INTEL_GUC_ABI_H__
+#define __INTEL_GUC_ABI_H__
+
+/**
+ * DOC: GuC MMIO based communication
+ *
+ * The MMIO based communication between Host and GuC relies on special
+ * hardware registers which format could be defined by the software
+ * (so called scratch registers).
+ *
+ * Each MMIO based message, both Host to GuC (H2G) and GuC to Host (G2H)
+ * messages, which maximum length depends on number of available scratch
+ * registers, is directly written into those scratch registers.
+ *
+ * For Gen9+, there are 16 software scratch registers 0xC180-0xC1B8,
+ * but no H2G command takes more than 8 parameters and the GuC firmware
+ * itself uses an 8-element array to store the H2G message.
+ *
+ * For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
+ * are, regardless on lower count, preffered over legacy ones.
+ *
+ * The MMIO based communication is mainly used during driver initialization
+ * phase to setup the CTB based communication that will be used afterwards.
+ *
+ * Details of the messages formats depend on GuC firmware version being used
+ * by the host driver. Documented here messages are for GuC 45.0 and newer.
+ */
+
+#define GUC_MAX_MMIO_MSG_LEN			8
+
+/**
+ * DOC: GuC MMIO message format
+ *
+ * Bits of the first scratch register are treated as a message header,
+ * and other registers values are used to hold message payload (data)::
+ *
+ *      +=======================================================+
+ *      |  SCRATCH  |                                           |
+ *      +-----------+-------------------------------------------+
+ *      | 0 | 31:28 | message ORIGIN/TYPE                       |
+ *      |   | 27:24 | message MAGIC                             |
+ *      |   |  23:0 | message DATA0 (depends on TYPE)           |
+ *      +---+-------+-------------------------------------------+
+ *      | 1 |  31:0 | message DATA1 (depends on TYPE)           |
+ *      |...|       | message DATA2 (depends on TYPE)           |
+ *      | n |       | message DATA3 (depends on TYPE)           |
+ *      +=======================================================+
+ *
+ * Where:
+ *  - **TYPE** is a 4b message type identifier.
+ *  - **MAGIC** is a 4b message sequence number.
+ *    The same sequence number must be included in all related messages.
+ *    This field is used for tracking and debug purposes.
+ *  - **DATA0..3** bits represents message payload.
+ *    Definitions of these bits depends on message **TYPE**.
+ *
+ * The MSB of the header and **TYPE** indicates origin of the message:
+ *  - 0 - message from the Host
+ *  - 1 - message from the GuC
+ *
+ * Currently supported message types are:
+ *  - 0x0 - **REQUEST** - represents Host request message to the GuC
+ *  - 0xF - **SUCCESS RESPONSE** - GuC reply for the earlier request
+ *  - 0xE - **ERROR RESPONSE** - GuC failure reply for the earlier request
+ *  - 0xB - **BUSY** - GuC will be processing request for the longer time
+ */
+
+#define GUC_MMIO_MSG_ORIGIN			(0x1 << 31)
+#define   GUC_MMIO_MSG_ORIGIN_HOST		0u
+#define   GUC_MMIO_MSG_ORIGIN_GUC		1u
+
+#define GUC_MMIO_MSG_TYPE			(0xF << 28)
+#define   GUC_MMIO_MSG_TYPE_REQUEST		0x0
+#define   GUC_MMIO_MSG_TYPE_RESPONSE		0xF
+#define   GUC_MMIO_MSG_TYPE_ERROR		0xE
+#define   GUC_MMIO_MSG_TYPE_BUSY		0xB
+
+#define GUC_MMIO_MSG_MAGIC			(0xF << 24)
+#define   GUC_MMIO_MSG_MAGIC_DEFAULT		0u
+
+/**
+ * DOC: GuC MMIO H2G REQUEST
+ *
+ * The MMIO H2G REQUEST message is used by the Host to request some action
+ * to be performed by the GuC::
+ *
+ *      +=======================================================+
+ *      |  SCRATCH  |                                           |
+ *      +=======================================================+
+ *      | 0 | 31:28 | message TYPE = 0x0 = REQUEST              |
+ *      |   | 27:24 | message MAGIC                             |
+ *      |   | 23:16 | request SUBCODE (depends on ACTION)       |
+ *      |   |  15:0 | request ACTION code                       |
+ *      +-------------------------------------------------------+
+ *      | 1 |  31:0 | request DATA1 (depends on ACTION/SUBCODE) |
+ *      |...|       | request DATA2 (depends on ACTION/SUBCODE) |
+ *      | n |  31:0 | request DATA3 (depends on ACTION/SUBCODE) |
+ *      +=======================================================+
+ *
+ * Where:
+ *  - **TYPE** must be set to value 0x0.
+ *  - **MAGIC** message sequence number is generated by the host.
+ *  - **ACTION** represents 16b request action code that defines both
+ *    purpose of the request and format of the passed payload data.
+ *    List of supported codes depends on GuC version and plaform.
+ *    Action code can't be zero.
+ *  - **SUBCODE** is optional 8b data related to the **ACTION**.
+ *    MBZ if not explicitly defined by given **ACTION**.
+ *  - **DATA1..3** are optional 32b payload dwords related to **ACTION**.
+ *    Format of each dword is defined by specific **ACTION**.
+ */
+
+#define GUC_MMIO_REQUEST_SUBCODE		(0xFF << 16)
+#define GUC_MMIO_REQUEST_ACTION			(0xFFFF << 0)
+#define   GUC_ACTION_INVALID			0x0000
+#define   GUC_ACTION_REGISTER_CTB		0x4505
+#define   GUC_ACTION_DEREGISTER_CTB		0x4506
+
+/**
+ * DOC: GuC MMIO G2H SUCCESS RESPONSE
+ *
+ * The MMIO G2H SUCCESS RESPONSE message is used by the GuC to reply to
+ * the earlier H2G request message. This message is used if no errors
+ * were encountered by the GuC during processing of the request::
+ *
+ *      +=======================================================+
+ *      |  SCRATCH  |                                           |
+ *      +=======================================================+
+ *      | 0 | 31:28 | message TYPE = 0xF = SUCCESS RESPONSE     |
+ *      |   | 27:24 | message MAGIC                             |
+ *      |   |  23:0 | response DATA0 (depends on ACTION/SUBCODE)|
+ *      +-------------------------------------------------------+
+ *      | 1 |  31:0 | response DATA1 (depends on ACTION/SUBCODE)|
+ *      |...|       | response DATA2 (depends on ACTION/SUBCODE)|
+ *      | n |  31:0 | response DATA3 (depends on ACTION/SUBCODE)|
+ *      +=======================================================+
+ *
+ * Where:
+ *  - **TYPE** must be set to value 0xF.
+ *  - **MAGIC** must match value used by the host in **REQUEST** message.
+ *  - **DATA** is optional payload data related to **ACTION**.
+ *    Format is defined by each **ACTION** separately.
+ */
+
+#define GUC_MMIO_RESPONSE_DATA0			(0xFFFFFF << 0)
+
+/**
+ * DOC: GuC MMIO G2H ERROR RESPONSE
+ *
+ * The MMIO G2H ERROR RESPONSE message is used by the GuC to reply to
+ * the earlier H2G request message. This message is used if some errors
+ * were encountered by the GuC during processing of the request::
+ *
+ *      +=======================================================+
+ *      |  SCRATCH  |                                           |
+ *      +=======================================================+
+ *      | 0 | 31:28 | message TYPE = 0xE = ERROR RESPONSE       |
+ *      |   | 27:24 | message MAGIC                             |
+ *      |   | 23:16 | reserved MBZ                              |
+ *      |   |  15:0 | response STATUS failure code              |
+ *      +-------------------------------------------------------+
+ *      | 1 |  31:0 | reserved MBZ                              |
+ *      |...|       | reserved MBZ                              |
+ *      | n |  31:0 | reserved MBZ                              |
+ *      +=======================================================+
+ *
+ * Where:
+ *  - **TYPE** must be set to value 0xE.
+ *  - **MAGIC** must match value used by the host in **REQUEST** message.
+ *  - **STATUS** represents non-zero failure code.
+ */
+
+#define GUC_MMIO_ERROR_STATUS			(0xFFFF << 0)
+#define   GUC_STATUS_UNKNOWN_ACTION		0x30
+#define   GUC_STATUS_INVALID_PARAMS		0x60
+#define   GUC_STATUS_INVALID_ADDR		0x80
+#define   GUC_STATUS_CTX_NOT_REGISTERED		0x100
+#define   GUC_STATUS_NO_LOCAL_MEMORY		0x200
+#define   GUC_STATUS_NO_VIRTUALIZATION		0x300
+#define   GUC_STATUS_CTB_FULL			0x301
+#define   GUC_STATUS_UNAUTHORIZED_REQUEST	0x302
+#define   GUC_STATUS_GENERIC_FAIL		0xF000
+
+/**
+ * DOC: MMIO G2H BUSY RESPONSE
+ *
+ * The MMIO G2H BUSY RESPONSE message is used by the GuC to reply to
+ * the earlier H2G request message. This message is used if processing
+ * of the request will take longer time and final SUCCESS/ERROR response
+ * message will be delivered later::
+ *
+ *      +=======================================================+
+ *      |  SCRATCH  |                                           |
+ *      +=======================================================+
+ *      | 0 | 31:28 | message TYPE = 0xB = BUSY RESPONSE        |
+ *      |   | 27:24 | message MAGIC                             |
+ *      |   |  23:0 | reserved MBZ                              |
+ *      +-------------------------------------------------------+
+ *      | 1 |  31:0 | reserved MBZ                              |
+ *      |...|       | reserved MBZ                              |
+ *      | n |  31:0 | reserved MBZ                              |
+ *      +=======================================================+
+ *
+ * Where:
+ *  - **TYPE** must be set to value 0xB.
+ *  - **MAGIC** must match value used by the host in **REQUEST** message.
+ *  - all other bits are reserved as MBZ.
+ */
+
+#endif /* __INTEL_GUC_ABI_H__ */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index d44061033f23..e66cbf1be320 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -10,11 +10,52 @@ 
 
 /*
  * The Additional Data Struct (ADS) has pointers for different buffers used by
- * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
- * scheduling policies (guc_policies), a structure describing a collection of
- * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
- * its internal state for sleep.
+ * the GuC. One single gem object contains the ADS struct itself (guc_ads) and
+ * all the extra buffers indirectly linked via the ADS struct's entires.
+ *
+ * Layout of the ADS blob allocated for the GuC:
+ *
+ *      +---------------------------------------+ <== base
+ *      | guc_ads                               |
+ *      +---------------------------------------+
+ *      | guc_policies                          |
+ *      +---------------------------------------+
+ *      | guc_gt_system_info                    |
+ *      +---------------------------------------+
+ *      | guc_clients_info                      |
+ *      +---------------------------------------+
+ *      | guc_ct_pool_entry[size]               |
+ *      +---------------------------------------+
+ *      | padding                               |
+ *      +---------------------------------------+ <== 4K aligned
+ *      | private data                          |
+ *      +---------------------------------------+
+ *      | padding                               |
+ *      +---------------------------------------+ <== 4K aligned
  */
+struct __guc_ads_blob {
+	struct guc_ads ads;
+	struct guc_policies policies;
+	struct guc_gt_system_info system_info;
+	struct guc_clients_info clients_info;
+	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
+} __packed;
+
+static u32 guc_ads_private_data_size(struct intel_guc *guc)
+{
+	return PAGE_ALIGN(guc->fw.private_data_size);
+}
+
+static u32 guc_ads_private_data_offset(struct intel_guc *guc)
+{
+	return PAGE_ALIGN(sizeof(struct __guc_ads_blob));
+}
+
+static u32 guc_ads_blob_size(struct intel_guc *guc)
+{
+	return guc_ads_private_data_offset(guc) +
+	       guc_ads_private_data_size(guc);
+}
 
 static void guc_policy_init(struct guc_policy *policy)
 {
@@ -48,23 +89,33 @@  static void guc_ct_pool_entries_init(struct guc_ct_pool_entry *pool, u32 num)
 	memset(pool, 0, num * sizeof(*pool));
 }
 
+static void guc_mapping_table_init(struct intel_gt *gt,
+				   struct guc_gt_system_info *system_info)
+{
+	unsigned int i, j;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	/* Table must be set to invalid values for entries not used */
+	for (i = 0; i < GUC_MAX_ENGINE_CLASSES; ++i)
+		for (j = 0; j < GUC_MAX_INSTANCES_PER_CLASS; ++j)
+			system_info->mapping_table[i][j] =
+				GUC_MAX_INSTANCES_PER_CLASS;
+
+	for_each_engine(engine, gt, id) {
+		u8 guc_class = engine->class;
+
+		system_info->mapping_table[guc_class][engine->instance]
+			= engine->instance;
+	}
+}
+
 /*
  * The first 80 dwords of the register state context, containing the
  * execlists and ppgtt registers.
  */
 #define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
 
-/* The ads obj includes the struct itself and buffers passed to GuC */
-struct __guc_ads_blob {
-	struct guc_ads ads;
-	struct guc_policies policies;
-	struct guc_mmio_reg_state reg_state;
-	struct guc_gt_system_info system_info;
-	struct guc_clients_info clients_info;
-	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
-	u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
-} __packed;
-
 static void __guc_ads_init(struct intel_guc *guc)
 {
 	struct intel_gt *gt = guc_to_gt(guc);
@@ -107,6 +158,16 @@  static void __guc_ads_init(struct intel_guc *guc)
 	blob->system_info.vebox_enable_mask = VEBOX_MASK(gt);
 	blob->system_info.vdbox_sfc_support_mask = gt->info.vdbox_sfc_access;
 
+	if (INTEL_GEN(gt->i915) >= 12 && !IS_DGFX(gt->i915)) {
+		u32 distdbreg = intel_uncore_read(gt->uncore,
+						  GEN12_DIST_DBS_POPULATED);
+		blob->system_info.num_of_doorbells_per_sqidi =
+			((distdbreg >> GEN12_DOORBELLS_PER_SQIDI_SHIFT) &
+			 GEN12_DOORBELLS_PER_SQIDI) + 1;
+	}
+
+	guc_mapping_table_init(guc_to_gt(guc), &blob->system_info);
+
 	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
 
 	/* Clients info  */
@@ -118,11 +179,12 @@  static void __guc_ads_init(struct intel_guc *guc)
 
 	/* ADS */
 	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
-	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
-	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
 	blob->ads.gt_system_info = base + ptr_offset(blob, system_info);
 	blob->ads.clients_info = base + ptr_offset(blob, clients_info);
 
+	/* Private Data */
+	blob->ads.private_data = base + guc_ads_private_data_offset(guc);
+
 	i915_gem_object_flush_map(guc->ads_vma->obj);
 }
 
@@ -135,14 +197,15 @@  static void __guc_ads_init(struct intel_guc *guc)
  */
 int intel_guc_ads_create(struct intel_guc *guc)
 {
-	const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
+	u32 size;
 	int ret;
 
 	GEM_BUG_ON(guc->ads_vma);
 
+	size = guc_ads_blob_size(guc);
+
 	ret = intel_guc_allocate_and_map_vma(guc, size, &guc->ads_vma,
 					     (void **)&guc->ads_blob);
-
 	if (ret)
 		return ret;
 
@@ -156,6 +219,18 @@  void intel_guc_ads_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->ads_vma, I915_VMA_RELEASE_MAP);
 }
 
+void guc_ads_private_data_reset(struct intel_guc *guc)
+{
+	u32 size;
+
+	size = guc_ads_private_data_size(guc);
+	if (!size)
+		return;
+
+	memset((void *)guc->ads_blob + guc_ads_private_data_offset(guc), 0,
+	       size);
+}
+
 /**
  * intel_guc_ads_reset() - prepares GuC Additional Data Struct for reuse
  * @guc: intel_guc struct
@@ -168,5 +243,8 @@  void intel_guc_ads_reset(struct intel_guc *guc)
 {
 	if (!guc->ads_vma)
 		return;
+
 	__guc_ads_init(guc);
+
+	guc_ads_private_data_reset(guc);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index d4a87f4c9421..212de0a939bf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -74,8 +74,9 @@  static inline bool guc_ready(struct intel_uncore *uncore, u32 *status)
 		((val & GS_MIA_CORE_STATE) && (uk_val == GS_UKERNEL_LAPIC_DONE));
 }
 
-static int guc_wait_ucode(struct intel_uncore *uncore)
+static int guc_wait_ucode(struct intel_gt *gt)
 {
+	struct intel_uncore *uncore = gt->uncore;
 	u32 status;
 	int ret;
 
@@ -93,12 +94,24 @@  static int guc_wait_ucode(struct intel_uncore *uncore)
 	if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
 		DRM_ERROR("GuC firmware signature verification failed\n");
 		ret = -ENOEXEC;
-	}
-
+	} else
 	if ((status & GS_UKERNEL_MASK) == GS_UKERNEL_EXCEPTION) {
 		DRM_ERROR("GuC firmware exception. EIP: %#x\n",
 			  intel_uncore_read(uncore, SOFT_SCRATCH(13)));
 		ret = -ENXIO;
+	} else
+	if (ret) {
+		i915_probe_error(gt->i915, "GuC load failed: status = 0x%08X\n",
+				 status);
+		i915_probe_error(gt->i915, "GuC status: Reset = %d, "
+				 "BootROM = 0x%02X, UKernel = 0x%02X, "
+				 "MIA = 0x%02X, Auth = 0x%02X\n",
+				 (status >> GS_RESET_SHIFT) & 1,
+				 (status & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT,
+				 (status & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT,
+				 (status & GS_MIA_MASK) >> GS_MIA_SHIFT,
+				 (status & GS_AUTH_STATUS_MASK) >>
+							  GS_AUTH_STATUS_SHIFT);
 	}
 
 	return ret;
@@ -139,7 +152,7 @@  int intel_guc_fw_upload(struct intel_guc *guc)
 	if (ret)
 		goto out;
 
-	ret = guc_wait_ucode(uncore);
+	ret = guc_wait_ucode(gt);
 	if (ret)
 		goto out;
 
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 a6b733c146c9..77e49d53bb5c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -27,7 +27,7 @@ 
 #define GUC_MAX_ENGINES_NUM		(GUC_VIDEO_ENGINE2 + 1)
 
 #define GUC_MAX_ENGINE_CLASSES		5
-#define GUC_MAX_INSTANCES_PER_CLASS	16
+#define GUC_MAX_INSTANCES_PER_CLASS	32
 
 #define GUC_DOORBELL_INVALID		256
 
@@ -62,12 +62,7 @@ 
 #define GUC_STAGE_DESC_ATTR_PCH		BIT(6)
 #define GUC_STAGE_DESC_ATTR_TERMINATED	BIT(7)
 
-/* New GuC control data */
-#define GUC_CTL_CTXINFO			0
-#define   GUC_CTL_CTXNUM_IN16_SHIFT	0
-#define   GUC_CTL_BASE_ADDR_SHIFT	12
-
-#define GUC_CTL_LOG_PARAMS		1
+#define GUC_CTL_LOG_PARAMS		0
 #define   GUC_LOG_VALID			(1 << 0)
 #define   GUC_LOG_NOTIFY_ON_HALF_FULL	(1 << 1)
 #define   GUC_LOG_ALLOC_IN_MEGABYTE	(1 << 3)
@@ -79,11 +74,11 @@ 
 #define   GUC_LOG_ISR_MASK	        (0x7 << GUC_LOG_ISR_SHIFT)
 #define   GUC_LOG_BUF_ADDR_SHIFT	12
 
-#define GUC_CTL_WA			2
-#define GUC_CTL_FEATURE			3
+#define GUC_CTL_WA			1
+#define GUC_CTL_FEATURE			2
 #define   GUC_CTL_DISABLE_SCHEDULER	(1 << 14)
 
-#define GUC_CTL_DEBUG			4
+#define GUC_CTL_DEBUG			3
 #define   GUC_LOG_VERBOSITY_SHIFT	0
 #define   GUC_LOG_VERBOSITY_LOW		(0 << GUC_LOG_VERBOSITY_SHIFT)
 #define   GUC_LOG_VERBOSITY_MED		(1 << GUC_LOG_VERBOSITY_SHIFT)
@@ -97,12 +92,31 @@ 
 #define   GUC_LOG_DISABLED		(1 << 6)
 #define   GUC_PROFILE_ENABLED		(1 << 7)
 
-#define GUC_CTL_ADS			5
+#define GUC_CTL_ADS			4
 #define   GUC_ADS_ADDR_SHIFT		1
 #define   GUC_ADS_ADDR_MASK		(0xFFFFF << GUC_ADS_ADDR_SHIFT)
 
 #define GUC_CTL_MAX_DWORDS		(SOFT_SCRATCH_COUNT - 2) /* [1..14] */
 
+/*
+ * The class goes in bits [0..2] of the GuC ID, the instance in bits [3..6].
+ * Bit 7 can be used for operations that apply to all engine classes&instances.
+ */
+#define GUC_ENGINE_CLASS_SHIFT		0
+#define GUC_ENGINE_CLASS_MASK		(0x7 << GUC_ENGINE_CLASS_SHIFT)
+#define GUC_ENGINE_INSTANCE_SHIFT	3
+#define GUC_ENGINE_INSTANCE_MASK	(0xf << GUC_ENGINE_INSTANCE_SHIFT)
+#define GUC_ENGINE_ALL_INSTANCES	(1 << 7)
+
+#define MAKE_GUC_ID(class, instance) \
+	(((class) << GUC_ENGINE_CLASS_SHIFT) | \
+	 ((instance) << GUC_ENGINE_INSTANCE_SHIFT))
+
+#define GUC_ID_TO_ENGINE_CLASS(guc_id) \
+	(((guc_id) & GUC_ENGINE_CLASS_MASK) >> GUC_ENGINE_CLASS_SHIFT)
+#define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
+	(((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
+
 /* Work item for submitting workloads into work queue of GuC. */
 struct guc_wq_item {
 	u32 header;
@@ -338,7 +352,6 @@  struct guc_policies {
 /* GuC MMIO reg state struct */
 
 
-#define GUC_REGSET_MAX_REGISTERS	64
 #define GUC_S3_SAVE_SPACE_PAGES		10
 
 struct guc_mmio_reg {
@@ -348,16 +361,17 @@  struct guc_mmio_reg {
 #define GUC_REGSET_MASKED		(1 << 0)
 } __packed;
 
-struct guc_mmio_regset {
-	struct guc_mmio_reg registers[GUC_REGSET_MAX_REGISTERS];
-	u32 values_valid;
-	u32 number_of_registers;
-} __packed;
-
 /* GuC register sets */
-struct guc_mmio_reg_state {
-	struct guc_mmio_regset engine_reg[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
-	u32 reserved[98];
+struct guc_mmio_reg_set {
+	u32 address;
+	union {
+		struct {
+			u32 count:16;
+			u32 reserved1:12;
+			u32 reserved2:4;
+		};
+		u32 count_u32;
+	};
 } __packed;
 
 /* HW info */
@@ -369,7 +383,9 @@  struct guc_gt_system_info {
 	u32 vdbox_enable_mask;
 	u32 vdbox_sfc_support_mask;
 	u32 vebox_enable_mask;
-	u32 reserved[9];
+	u32 num_of_doorbells_per_sqidi;
+	u8 mapping_table[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
+	u32 reserved2[8];
 } __packed;
 
 /* Clients info */
@@ -390,15 +406,16 @@  struct guc_clients_info {
 
 /* GuC Additional Data Struct */
 struct guc_ads {
-	u32 reg_state_addr;
-	u32 reg_state_buffer;
+	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
+	u32 reserved0;
 	u32 scheduler_policies;
 	u32 gt_system_info;
 	u32 clients_info;
 	u32 control_data;
 	u32 golden_context_lrca[GUC_MAX_ENGINE_CLASSES];
 	u32 eng_state_size[GUC_MAX_ENGINE_CLASSES];
-	u32 reserved[16];
+	u32 private_data;
+	u32 reserved[15];
 } __packed;
 
 /* GuC logging structures */
@@ -474,49 +491,6 @@  struct guc_shared_ctx_data {
 	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
 } __packed;
 
-/**
- * DOC: MMIO based communication
- *
- * The MMIO based communication between Host and GuC uses software scratch
- * registers, where first register holds data treated as message header,
- * and other registers are used to hold message payload.
- *
- * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
- * but no H2G command takes more than 8 parameters and the GuC FW
- * itself uses an 8-element array to store the H2G message.
- *
- *      +-----------+---------+---------+---------+
- *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
- *      +-----------+---------+---------+---------+
- *      | header    |      optional payload       |
- *      +======+====+=========+=========+=========+
- *      | 31:28|type|         |         |         |
- *      +------+----+         |         |         |
- *      | 27:16|data|         |         |         |
- *      +------+----+         |         |         |
- *      |  15:0|code|         |         |         |
- *      +------+----+---------+---------+---------+
- *
- * The message header consists of:
- *
- * - **type**, indicates message type
- * - **code**, indicates message code, is specific for **type**
- * - **data**, indicates message data, optional, depends on **code**
- *
- * The following message **types** are supported:
- *
- * - **REQUEST**, indicates Host-to-GuC request, requested GuC action code
- *   must be priovided in **code** field. Optional action specific parameters
- *   can be provided in remaining payload registers or **data** field.
- *
- * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC request,
- *   action response status will be provided in **code** field. Optional
- *   response data can be returned in remaining payload registers or **data**
- *   field.
- */
-
-#define GUC_MAX_MMIO_MSG_LEN		8
-
 #define INTEL_GUC_MSG_TYPE_SHIFT	28
 #define INTEL_GUC_MSG_TYPE_MASK		(0xF << INTEL_GUC_MSG_TYPE_SHIFT)
 #define INTEL_GUC_MSG_DATA_SHIFT	16
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
index 1949346e714e..b37fc2ffaef2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h
@@ -118,6 +118,11 @@  struct guc_doorbell_info {
 #define   GEN8_DRB_VALID		  (1<<0)
 #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
 
+#define GEN12_DIST_DBS_POPULATED		_MMIO(0xd08)
+#define   GEN12_DOORBELLS_PER_SQIDI_SHIFT	16
+#define   GEN12_DOORBELLS_PER_SQIDI		(0xff)
+#define   GEN12_SQIDIS_DOORBELL_EXIST		(0xffff)
+
 #define DE_GUCRMR			_MMIO(0x44054)
 
 #define GUC_BCS_RCS_IER			_MMIO(0xC550)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 59b27aba15c6..f7cb0b1f1ba5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -44,23 +44,19 @@  void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
  * List of required GuC and HuC binaries per-platform.
  * Must be ordered based on platform + revid, from newer to older.
  *
- * TGL 35.2 is interface-compatible with 33.0 for previous Gens. The deltas
- * between 33.0 and 35.2 are only related to new additions to support new Gen12
- * features.
- *
  * Note that RKL uses the same firmware as TGL.
  */
 #define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
-	fw_def(ROCKETLAKE,  0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7, 0, 12)) \
-	fw_def(TIGERLAKE,   0, guc_def(tgl, 35, 2, 0), huc_def(tgl,  7, 0, 12)) \
-	fw_def(ELKHARTLAKE, 0, guc_def(ehl, 33, 0, 4), huc_def(ehl,  9, 0, 0)) \
-	fw_def(ICELAKE,     0, guc_def(icl, 33, 0, 0), huc_def(icl,  9, 0, 0)) \
-	fw_def(COMETLAKE,   5, guc_def(cml, 33, 0, 0), huc_def(cml,  4, 0, 0)) \
-	fw_def(COFFEELAKE,  0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4, 0, 0)) \
-	fw_def(GEMINILAKE,  0, guc_def(glk, 33, 0, 0), huc_def(glk,  4, 0, 0)) \
-	fw_def(KABYLAKE,    0, guc_def(kbl, 33, 0, 0), huc_def(kbl,  4, 0, 0)) \
-	fw_def(BROXTON,     0, guc_def(bxt, 33, 0, 0), huc_def(bxt,  2, 0, 0)) \
-	fw_def(SKYLAKE,     0, guc_def(skl, 33, 0, 0), huc_def(skl,  2, 0, 0))
+	fw_def(ROCKETLAKE,  0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7, 0, 12)) \
+	fw_def(TIGERLAKE,   0, guc_def(tgl, 45, 0, 0), huc_def(tgl,  7, 0, 12)) \
+	fw_def(ELKHARTLAKE, 0, guc_def(ehl, 45, 0, 0), huc_def(ehl,  9, 0, 0)) \
+	fw_def(ICELAKE,     0, guc_def(icl, 45, 0, 0), huc_def(icl,  9, 0, 0)) \
+	fw_def(COMETLAKE,   5, guc_def(cml, 45, 0, 0), huc_def(cml,  4, 0, 0)) \
+	fw_def(COFFEELAKE,  0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4, 0, 0)) \
+	fw_def(GEMINILAKE,  0, guc_def(glk, 45, 0, 0), huc_def(glk,  4, 0, 0)) \
+	fw_def(KABYLAKE,    0, guc_def(kbl, 45, 0, 0), huc_def(kbl,  4, 0, 0)) \
+	fw_def(BROXTON,     0, guc_def(bxt, 45, 0, 0), huc_def(bxt,  2, 0, 0)) \
+	fw_def(SKYLAKE,     0, guc_def(skl, 45, 0, 0), huc_def(skl,  2, 0, 0))
 
 #define __MAKE_UC_FW_PATH(prefix_, name_, major_, minor_, patch_) \
 	"i915/" \
@@ -371,6 +367,9 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 		}
 	}
 
+	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
+		uc_fw->private_data_size = css->private_data_size;
+
 	obj = i915_gem_object_create_shmem_from_data(i915, fw->data, fw->size);
 	if (IS_ERR(obj)) {
 		err = PTR_ERR(obj);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index 23d3a423ac0f..99bb1fe1af66 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -88,6 +88,8 @@  struct intel_uc_fw {
 
 	u32 rsa_size;
 	u32 ucode_size;
+
+	u32 private_data_size;
 };
 
 #ifdef CONFIG_DRM_I915_DEBUG_GUC
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
index 029214cdedd5..e41ffc7a7fbc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h
@@ -69,7 +69,11 @@  struct uc_css_header {
 #define CSS_SW_VERSION_UC_MAJOR		(0xFF << 16)
 #define CSS_SW_VERSION_UC_MINOR		(0xFF << 8)
 #define CSS_SW_VERSION_UC_PATCH		(0xFF << 0)
-	u32 reserved[14];
+	u32 reserved0[13];
+	union {
+		u32 private_data_size; /* only applies to GuC */
+		u32 reserved1;
+	};
 	u32 header_info;
 } __packed;
 static_assert(sizeof(struct uc_css_header) == 128);