Re: [PATCH 02/13] drm/i915/guc: Update MMIO based communication

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux