From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@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 18:03:18 -0700 [thread overview]
Message-ID: <6c4cf5e3-5fe7-1c2d-7b7a-422aa203ee27@intel.com> (raw)
In-Reply-To: <5dc6918e-d9ae-f435-c33f-2d6ab370224e@intel.com>
<snip>
>>> #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."
Yes it does.
Daniele
>
>> 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);
next prev parent reply other threads:[~2021-06-09 1:03 UTC|newest]
Thread overview: 42+ 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 ` [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages Matthew Brost
2021-06-07 22:46 ` Daniele Ceraolo Spurio
2021-06-08 7:59 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 02/13] drm/i915/guc: Update MMIO based communication Matthew Brost
2021-06-07 23:06 ` Daniele Ceraolo Spurio
2021-06-08 8:15 ` Michal Wajdeczko
2021-06-09 1:03 ` Daniele Ceraolo Spurio [this message]
2021-06-07 18:03 ` [PATCH 03/13] drm/i915/guc: Update CTB response status definition Matthew Brost
2021-06-08 0:05 ` Daniele Ceraolo Spurio
2021-06-08 8:23 ` 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 ` [PATCH 05/13] drm/i915/guc: Add flag for mark broken CTB Matthew Brost
2021-06-07 18:03 ` [PATCH 06/13] drm/i915/guc: New definition of the CTB descriptor Matthew Brost
2021-06-08 0:59 ` Daniele Ceraolo Spurio
2021-06-09 18:28 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 07/13] drm/i915/guc: New definition of the CTB registration action Matthew Brost
2021-06-08 1:23 ` Daniele Ceraolo Spurio
2021-06-09 17:36 ` John Harrison
2021-06-09 20:07 ` Michal Wajdeczko
2021-06-10 4:38 ` Matthew Brost
2021-06-10 13:19 ` Michal Wajdeczko
2021-06-11 18:43 ` Matthew Brost
2021-06-09 19:35 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 08/13] drm/i915/guc: New CTB based communication Matthew Brost
2021-06-08 2:20 ` Daniele Ceraolo Spurio
2021-06-10 4:01 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 09/13] drm/i915/doc: Include GuC ABI documentation Matthew Brost
2021-06-07 17:45 ` [Intel-gfx] " Matthew Brost
2021-06-07 19:38 ` Michal Wajdeczko
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 17:57 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 11/13] drm/i915/guc: Kill ads.client_info Matthew Brost
2021-06-07 18:03 ` [PATCH 12/13] drm/i915/guc: Unified GuC log Matthew Brost
2021-06-07 18:05 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 13/13] drm/i915/guc: Update firmware to v62.0.0 Matthew Brost
2021-06-07 19:17 ` [Intel-gfx] " Matthew Brost
2021-06-07 22:19 ` [PATCH 00/13] " Daniele Ceraolo Spurio
2021-06-11 18:44 ` Matthew Brost
-- strict thread matches above, loose matches on Subject: below --
2021-06-10 4:36 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=6c4cf5e3-5fe7-1c2d-7b7a-422aa203ee27@intel.com \
--to=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 \
--cc=michal.wajdeczko@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).