From: Michal Wajdeczko <michal.wajdeczko@intel.com> To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>, Matthew Brost <matthew.brost@intel.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: john.c.harrison@intel.com Subject: Re: [PATCH 02/13] drm/i915/guc: Update MMIO based communication Date: Tue, 8 Jun 2021 10:15:06 +0200 [thread overview] Message-ID: <5dc6918e-d9ae-f435-c33f-2d6ab370224e@intel.com> (raw) In-Reply-To: <707a404a-c20b-39e6-af85-7ab93e9d3c5e@intel.com> 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@intel.com> >> >> 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@intel.com> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Piotr Piórkowski <piotr.piorkowski@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> #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); >
WARNING: multiple messages have this Message-ID (diff)
From: Michal Wajdeczko <michal.wajdeczko@intel.com> To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>, Matthew Brost <matthew.brost@intel.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 02/13] drm/i915/guc: Update MMIO based communication Date: Tue, 8 Jun 2021 10:15:06 +0200 [thread overview] Message-ID: <5dc6918e-d9ae-f435-c33f-2d6ab370224e@intel.com> (raw) In-Reply-To: <707a404a-c20b-39e6-af85-7ab93e9d3c5e@intel.com> 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@intel.com> >> >> 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@intel.com> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Piotr Piórkowski <piotr.piorkowski@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> #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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-06-08 8:15 UTC|newest] Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-07 18:03 [PATCH 00/13] Update firmware to v62.0.0 Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 22:46 ` Daniele Ceraolo Spurio 2021-06-07 22:46 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-08 7:59 ` Michal Wajdeczko 2021-06-08 7:59 ` [Intel-gfx] " Michal Wajdeczko 2021-06-07 18:03 ` [PATCH 02/13] drm/i915/guc: Update MMIO based communication Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 23:06 ` Daniele Ceraolo Spurio 2021-06-07 23:06 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-08 8:15 ` Michal Wajdeczko [this message] 2021-06-08 8:15 ` Michal Wajdeczko 2021-06-09 1:03 ` Daniele Ceraolo Spurio 2021-06-09 1:03 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-07 18:03 ` [PATCH 03/13] drm/i915/guc: Update CTB response status definition Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-08 0:05 ` Daniele Ceraolo Spurio 2021-06-08 0:05 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-08 8:23 ` Michal Wajdeczko 2021-06-08 8:23 ` [Intel-gfx] " Michal Wajdeczko 2021-06-07 18:03 ` [PATCH 04/13] drm/i915/guc: Support per context scheduling policies Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 05/13] drm/i915/guc: Add flag for mark broken CTB Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 06/13] drm/i915/guc: New definition of the CTB descriptor Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-08 0:59 ` Daniele Ceraolo Spurio 2021-06-08 0:59 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-09 18:28 ` Michal Wajdeczko 2021-06-09 18:28 ` [Intel-gfx] " Michal Wajdeczko 2021-06-07 18:03 ` [PATCH 07/13] drm/i915/guc: New definition of the CTB registration action Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-08 1:23 ` Daniele Ceraolo Spurio 2021-06-08 1:23 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-09 17:36 ` John Harrison 2021-06-09 17:36 ` [Intel-gfx] " John Harrison 2021-06-09 20:07 ` Michal Wajdeczko 2021-06-09 20:07 ` [Intel-gfx] " Michal Wajdeczko 2021-06-10 4:38 ` Matthew Brost 2021-06-10 4:38 ` [Intel-gfx] " Matthew Brost 2021-06-10 13:19 ` Michal Wajdeczko 2021-06-10 13:19 ` [Intel-gfx] " Michal Wajdeczko 2021-06-11 18:43 ` Matthew Brost 2021-06-11 18:43 ` [Intel-gfx] " Matthew Brost 2021-06-09 19:35 ` Michal Wajdeczko 2021-06-09 19:35 ` [Intel-gfx] " Michal Wajdeczko 2021-06-07 18:03 ` [PATCH 08/13] drm/i915/guc: New CTB based communication Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-08 2:20 ` Daniele Ceraolo Spurio 2021-06-08 2:20 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-10 4:01 ` Matthew Brost 2021-06-10 4:01 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 09/13] drm/i915/doc: Include GuC ABI documentation Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 17:45 ` Matthew Brost 2021-06-07 17:45 ` Matthew Brost 2021-06-07 19:38 ` Michal Wajdeczko 2021-06-07 19:38 ` Michal Wajdeczko 2021-06-07 19:35 ` Matthew Brost 2021-06-07 19:35 ` Matthew Brost 2021-06-07 18:03 ` [PATCH 10/13] drm/i915/guc: Kill guc_clients.ct_pool Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 17:57 ` Matthew Brost 2021-06-07 17:57 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 11/13] drm/i915/guc: Kill ads.client_info Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 12/13] drm/i915/guc: Unified GuC log Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:05 ` Matthew Brost 2021-06-07 18:05 ` [Intel-gfx] " Matthew Brost 2021-06-07 18:03 ` [PATCH 13/13] drm/i915/guc: Update firmware to v62.0.0 Matthew Brost 2021-06-07 18:03 ` [Intel-gfx] " Matthew Brost 2021-06-07 19:17 ` Matthew Brost 2021-06-07 19:17 ` Matthew Brost 2021-06-07 18:05 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2021-06-07 18:06 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2021-06-07 18:34 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-06-07 18:34 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning " Patchwork 2021-06-07 22:19 ` [PATCH 00/13] " Daniele Ceraolo Spurio 2021-06-07 22:19 ` [Intel-gfx] " Daniele Ceraolo Spurio 2021-06-11 18:44 ` Matthew Brost 2021-06-11 18:44 ` [Intel-gfx] " Matthew Brost 2021-06-08 2:17 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2021-06-10 4:36 [PATCH 00/13] " Matthew Brost 2021-06-10 4:36 ` [PATCH 02/13] drm/i915/guc: Update MMIO based communication Matthew Brost 2021-06-14 18:11 ` Daniele Ceraolo Spurio
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5dc6918e-d9ae-f435-c33f-2d6ab370224e@intel.com \ --to=michal.wajdeczko@intel.com \ --cc=daniele.ceraolospurio@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=john.c.harrison@intel.com \ --cc=matthew.brost@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.