On 08.06.2021 01:06, Daniele Ceraolo Spurio wrote:
>
>
> On 6/7/2021 11:03 AM, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>>
>> The MMIO based Host-to-GuC communication protocol has been
>> updated to use unified HXG messages.
>>
>> Update our intel_guc_send_mmio() function by correctly handle
>> BUSY, RETRY and FAILURE replies. Also update our documentation.
>>
>> GuC: 55.0.0
>> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>> Cc: Piotr Piórkowski <piotr.piorkowski@xxxxxxxxx>
>> Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx> #v3
>> ---
>> .../gt/uc/abi/guc_communication_mmio_abi.h | 63 ++++++-------
>> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 92 ++++++++++++++-----
>> 2 files changed, 97 insertions(+), 58 deletions(-)
>>
>> diff --git
>> a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
>> b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
>> index be066a62e9e0..3f9039e3ef9d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
>> @@ -7,46 +7,43 @@
>> #define _ABI_GUC_COMMUNICATION_MMIO_ABI_H
>> /**
>> - * DOC: MMIO based communication
>> + * DOC: GuC 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.
>> + * 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).
>> *
>> - * 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**
>> + * 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.
>> *
>> - * The following message **types** are supported:
>> + * 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.
>
> Is this statement still true? I believe no MMIO H2G is over 4 DWs (given
> the limitation of the new gen11+ scratch regs), while CTB messages can
> be longer than 8 DWs.
oops, it was just copy/past, you're correct, with new unified firmware,
all MMIO H2G are up to 4 DWs
>
>> *
>> - * - **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.
>> + * For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
>> + * are, regardless on lower count, preffered over legacy ones.
>
> typo: preffered -> preferred
>
>> *
>> - * - **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.
>> + * The MMIO based communication is mainly used during driver
>> initialization
>> + * phase to setup the `CTB based communication`_ that will be used
>> afterwards.
>> */
>> #define GUC_MAX_MMIO_MSG_LEN 8
>
> See comment above. Reduce this to 4?
yes, must be reduced
>
>> +/**
>> + * DOC: MMIO HXG Message
>> + *
>> + * Format of the MMIO messages follows definitions of `HXG Message`_.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31:0 |
>> +--------------------------------------------------------+ |
>> + * +---+-------+
>> | | |
>> + * |...| | | Embedded `HXG
>> Message`_ | |
>> + * +---+-------+
>> | | |
>> + * | n | 31:0 |
>> +--------------------------------------------------------+ |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> #endif /* _ABI_GUC_COMMUNICATION_MMIO_ABI_H */
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index f147cb389a20..b773567cb080 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -376,29 +376,27 @@ void intel_guc_fini(struct intel_guc *guc)
>> /*
>> * 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 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);
>> -
>> - /* 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);
>> + GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, request[0]) !=
>> GUC_HXG_ORIGIN_HOST);
>> + GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_TYPE, request[0]) !=
>> GUC_HXG_TYPE_REQUEST);
>> mutex_lock(&guc->send_mutex);
>> intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);
>> +retry:
>> 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, i), request[i]);
>> intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));
>> @@ -410,30 +408,74 @@ 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_HXG_MSG_0_ORIGIN,
>> + FIELD_PREP(GUC_HXG_MSG_0_ORIGIN,
>> + GUC_HXG_ORIGIN_GUC),
>> + 10, 10, &header);
>> + if (unlikely(ret)) {
>> +timeout:
>> + drm_err(&i915->drm, "mmio request %#x: no reply %x\n",
>> + request[0], header);
>> + goto out;
>> + }
>> - if (ret) {
>> - DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
>> - action[0], ret, status);
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) ==
>> GUC_HXG_TYPE_NO_RESPONSE_BUSY) {
>> +#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc,
>> 0)); \
>> + FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) != GUC_HXG_ORIGIN_GUC
>> || \
>> + FIELD_GET(GUC_HXG_MSG_0_TYPE, header) !=
>> GUC_HXG_TYPE_NO_RESPONSE_BUSY; })
>> +
>> + ret = wait_for(done, 1000);
>> + if (unlikely(ret))
>> + goto timeout;
>> + if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) !=
>> + GUC_HXG_ORIGIN_GUC))
>> + goto proto;
>> +#undef done
>> + }
>> +
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) ==
>> GUC_HXG_TYPE_NO_RESPONSE_RETRY) {
>> + u32 reason = FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, header);
>> +
>> + drm_dbg(&i915->drm, "mmio request %#x: retrying, reason %u\n",
>> + request[0], reason);
>> + goto retry;
>> + }
>> +
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) ==
>> GUC_HXG_TYPE_RESPONSE_FAILURE) {
>> + u32 hint = FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, header);
>> + u32 error = FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, header);
>> +
>> + drm_err(&i915->drm, "mmio request %#x: failure %x/%u\n",
>> + request[0], error, hint);
>> + ret = -ENXIO;
>> + goto out;
>> + }
>> +
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) !=
>> GUC_HXG_TYPE_RESPONSE_SUCCESS) {
>> +proto:
>> + drm_err(&i915->drm, "mmio request %#x: unexpected reply %#x\n",
>> + request[0], header);
>> + ret = -EPROTO;
>> goto out;
>> }
>> if (response_buf) {
>> - int count = min(response_buf_size, guc->send_regs.count - 1);
>> + int count = min(response_buf_size, guc->send_regs.count);
>> - for (i = 0; i < count; i++)
>> + GEM_BUG_ON(!count);
>> +
>> + response_buf[0] = header;
>> +
>> + for (i = 1; i < count; i++)
>> response_buf[i] = intel_uncore_read(uncore,
>> - guc_send_reg(guc, i + 1));
>> - }
>> + guc_send_reg(guc, i));
>
> This could use a note in the commit message to remark that we have no
> users for the returned data yet and therefore nothing will break if we
> change what we return through it.
I hope this will do the work:
"Since some of the new MMIO actions may use DATA0 from MMIO HXG
response, we must update intel_guc_send_mmio() to copy full response,
including HXG header. There will be no impact to existing users as all
of them are only relying just on return code."
>
> Apart from the nits, the logic looks good to me.
> Daniele
>
>> - /* Use data from the GuC response as our return value */
>> - ret = INTEL_GUC_MSG_TO_DATA(status);
>> + /* Use number of copied dwords as our return value */
>> + ret = count;
>> + } else {
>> + /* Use data from the GuC response as our return value */
>> + ret = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, header);
>> + }
>> out:
>> intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
