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

Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> · Tue, 8 Jun 2021 18:03:18 -0700

<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);







