diff mbox series

[v6,1/3] drm/i915/pxp/mtl: Update pxp-firmware response timeout

Message ID 20230915183051.1232026-2-alan.previn.teres.alexis@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/pxp/mtl: Update gsc-heci cmd submission to align with fw/hw spec | expand

Commit Message

Teres Alexis, Alan Previn Sept. 15, 2023, 6:30 p.m. UTC 
Update the max GSC-fw response time to match updated internal
fw specs. Because this response time is an SLA on the firmware,
not inclusive of i915->GuC->HW handoff latency, when submitting
requests to the GSC fw via intel_gsc_uc_heci_cmd_submit helpers,
start the count after the request hits the GSC command streamer.
Also, move GSC_REPLY_LATENCY_MS definition from pxp header to
intel_gsc_uc_heci_cmd_submit.h since its for any GSC HECI packet.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Vivaik Balasubrawmanian <vivaik.balasubrawmanian@intel.com>
---
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 20 +++++++++++++++++--
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |  6 ++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    | 10 ++++------
 3 files changed, 28 insertions(+), 8 deletions(-)

Comments

kernel test robot Sept. 16, 2023, 2:25 a.m. UTC | #1 
Hi Alan,

kernel test robot noticed the following build errors:

[auto build test ERROR on cf1e91e884bb1113c653e654e9de1754fc1d4488]

url:    https://github.com/intel-lab-lkp/linux/commits/Alan-Previn/drm-i915-pxp-mtl-Update-pxp-firmware-response-timeout/20230916-023150
base:   cf1e91e884bb1113c653e654e9de1754fc1d4488
patch link:    https://lore.kernel.org/r/20230915183051.1232026-2-alan.previn.teres.alexis%40intel.com
patch subject: [PATCH v6 1/3] drm/i915/pxp/mtl: Update pxp-firmware response timeout
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230916/202309161030.TcQkW66y-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230916/202309161030.TcQkW66y-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309161030.TcQkW66y-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c: In function 'gsccs_send_message':
>> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:115:52: error: 'GSC_REPLY_LATENCY_MS' undeclared (first use in this function); did you mean 'GSC_HECI_REPLY_LATENCY_MS'?
     115 |                                                    GSC_REPLY_LATENCY_MS);
         |                                                    ^~~~~~~~~~~~~~~~~~~~
         |                                                    GSC_HECI_REPLY_LATENCY_MS
   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:115:52: note: each undeclared identifier is reported only once for each function it appears in


vim +115 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c

99afb7cc8c4457 Alan Previn       2023-05-11   51  
dc9ac125d81faf Alan Previn       2023-05-11   52  static int
dc9ac125d81faf Alan Previn       2023-05-11   53  gsccs_send_message(struct intel_pxp *pxp,
dc9ac125d81faf Alan Previn       2023-05-11   54  		   void *msg_in, size_t msg_in_size,
dc9ac125d81faf Alan Previn       2023-05-11   55  		   void *msg_out, size_t msg_out_size_max,
dc9ac125d81faf Alan Previn       2023-05-11   56  		   size_t *msg_out_len,
dc9ac125d81faf Alan Previn       2023-05-11   57  		   u64 *gsc_msg_handle_retry)
dc9ac125d81faf Alan Previn       2023-05-11   58  {
dc9ac125d81faf Alan Previn       2023-05-11   59  	struct intel_gt *gt = pxp->ctrl_gt;
dc9ac125d81faf Alan Previn       2023-05-11   60  	struct drm_i915_private *i915 = gt->i915;
dc9ac125d81faf Alan Previn       2023-05-11   61  	struct gsccs_session_resources *exec_res =  &pxp->gsccs_res;
dc9ac125d81faf Alan Previn       2023-05-11   62  	struct intel_gsc_mtl_header *header = exec_res->pkt_vaddr;
dc9ac125d81faf Alan Previn       2023-05-11   63  	struct intel_gsc_heci_non_priv_pkt pkt;
dc9ac125d81faf Alan Previn       2023-05-11   64  	size_t max_msg_size;
dc9ac125d81faf Alan Previn       2023-05-11   65  	u32 reply_size;
dc9ac125d81faf Alan Previn       2023-05-11   66  	int ret;
dc9ac125d81faf Alan Previn       2023-05-11   67  
dc9ac125d81faf Alan Previn       2023-05-11   68  	if (!exec_res->ce)
dc9ac125d81faf Alan Previn       2023-05-11   69  		return -ENODEV;
dc9ac125d81faf Alan Previn       2023-05-11   70  
dc9ac125d81faf Alan Previn       2023-05-11   71  	max_msg_size = PXP43_MAX_HECI_INOUT_SIZE - sizeof(*header);
dc9ac125d81faf Alan Previn       2023-05-11   72  
dc9ac125d81faf Alan Previn       2023-05-11   73  	if (msg_in_size > max_msg_size || msg_out_size_max > max_msg_size)
dc9ac125d81faf Alan Previn       2023-05-11   74  		return -ENOSPC;
dc9ac125d81faf Alan Previn       2023-05-11   75  
dc9ac125d81faf Alan Previn       2023-05-11   76  	if (!exec_res->pkt_vma || !exec_res->bb_vma)
dc9ac125d81faf Alan Previn       2023-05-11   77  		return -ENOENT;
dc9ac125d81faf Alan Previn       2023-05-11   78  
dc9ac125d81faf Alan Previn       2023-05-11   79  	GEM_BUG_ON(exec_res->pkt_vma->size < (2 * PXP43_MAX_HECI_INOUT_SIZE));
dc9ac125d81faf Alan Previn       2023-05-11   80  
dc9ac125d81faf Alan Previn       2023-05-11   81  	mutex_lock(&pxp->tee_mutex);
dc9ac125d81faf Alan Previn       2023-05-11   82  
dc9ac125d81faf Alan Previn       2023-05-11   83  	memset(header, 0, sizeof(*header));
dc9ac125d81faf Alan Previn       2023-05-11   84  	intel_gsc_uc_heci_cmd_emit_mtl_header(header, HECI_MEADDRESS_PXP,
dc9ac125d81faf Alan Previn       2023-05-11   85  					      msg_in_size + sizeof(*header),
dc9ac125d81faf Alan Previn       2023-05-11   86  					      exec_res->host_session_handle);
dc9ac125d81faf Alan Previn       2023-05-11   87  
dc9ac125d81faf Alan Previn       2023-05-11   88  	/* check if this is a host-session-handle cleanup call (empty packet) */
dc9ac125d81faf Alan Previn       2023-05-11   89  	if (!msg_in && !msg_out)
dc9ac125d81faf Alan Previn       2023-05-11   90  		header->flags |= GSC_INFLAG_MSG_CLEANUP;
dc9ac125d81faf Alan Previn       2023-05-11   91  
dc9ac125d81faf Alan Previn       2023-05-11   92  	/* copy caller provided gsc message handle if this is polling for a prior msg completion */
dc9ac125d81faf Alan Previn       2023-05-11   93  	header->gsc_message_handle = *gsc_msg_handle_retry;
dc9ac125d81faf Alan Previn       2023-05-11   94  
dc9ac125d81faf Alan Previn       2023-05-11   95  	/* NOTE: zero size packets are used for session-cleanups */
dc9ac125d81faf Alan Previn       2023-05-11   96  	if (msg_in && msg_in_size)
dc9ac125d81faf Alan Previn       2023-05-11   97  		memcpy(exec_res->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
dc9ac125d81faf Alan Previn       2023-05-11   98  
dc9ac125d81faf Alan Previn       2023-05-11   99  	pkt.addr_in = i915_vma_offset(exec_res->pkt_vma);
dc9ac125d81faf Alan Previn       2023-05-11  100  	pkt.size_in = header->message_size;
dc9ac125d81faf Alan Previn       2023-05-11  101  	pkt.addr_out = pkt.addr_in + PXP43_MAX_HECI_INOUT_SIZE;
dc9ac125d81faf Alan Previn       2023-05-11  102  	pkt.size_out = msg_out_size_max + sizeof(*header);
dc9ac125d81faf Alan Previn       2023-05-11  103  	pkt.heci_pkt_vma = exec_res->pkt_vma;
dc9ac125d81faf Alan Previn       2023-05-11  104  	pkt.bb_vma = exec_res->bb_vma;
dc9ac125d81faf Alan Previn       2023-05-11  105  
dc9ac125d81faf Alan Previn       2023-05-11  106  	/*
dc9ac125d81faf Alan Previn       2023-05-11  107  	 * Before submitting, let's clear-out the validity marker on the reply offset.
dc9ac125d81faf Alan Previn       2023-05-11  108  	 * We use offset PXP43_MAX_HECI_INOUT_SIZE for reply location so point header there.
dc9ac125d81faf Alan Previn       2023-05-11  109  	 */
dc9ac125d81faf Alan Previn       2023-05-11  110  	header = exec_res->pkt_vaddr + PXP43_MAX_HECI_INOUT_SIZE;
dc9ac125d81faf Alan Previn       2023-05-11  111  	header->validity_marker = 0;
dc9ac125d81faf Alan Previn       2023-05-11  112  
dc9ac125d81faf Alan Previn       2023-05-11  113  	ret = intel_gsc_uc_heci_cmd_submit_nonpriv(&gt->uc.gsc,
dc9ac125d81faf Alan Previn       2023-05-11  114  						   exec_res->ce, &pkt, exec_res->bb_vaddr,
dc9ac125d81faf Alan Previn       2023-05-11 @115  						   GSC_REPLY_LATENCY_MS);
dc9ac125d81faf Alan Previn       2023-05-11  116  	if (ret) {
dc9ac125d81faf Alan Previn       2023-05-11  117  		drm_err(&i915->drm, "failed to send gsc PXP msg (%d)\n", ret);
dc9ac125d81faf Alan Previn       2023-05-11  118  		goto unlock;
dc9ac125d81faf Alan Previn       2023-05-11  119  	}
dc9ac125d81faf Alan Previn       2023-05-11  120  
dc9ac125d81faf Alan Previn       2023-05-11  121  	/* Response validity marker, status and busyness */
dc9ac125d81faf Alan Previn       2023-05-11  122  	if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) {
dc9ac125d81faf Alan Previn       2023-05-11  123  		drm_err(&i915->drm, "gsc PXP reply with invalid validity marker\n");
dc9ac125d81faf Alan Previn       2023-05-11  124  		ret = -EINVAL;
dc9ac125d81faf Alan Previn       2023-05-11  125  		goto unlock;
dc9ac125d81faf Alan Previn       2023-05-11  126  	}
dc9ac125d81faf Alan Previn       2023-05-11  127  	if (header->status != 0) {
dc9ac125d81faf Alan Previn       2023-05-11  128  		drm_dbg(&i915->drm, "gsc PXP reply status has error = 0x%08x\n",
dc9ac125d81faf Alan Previn       2023-05-11  129  			header->status);
dc9ac125d81faf Alan Previn       2023-05-11  130  		ret = -EINVAL;
dc9ac125d81faf Alan Previn       2023-05-11  131  		goto unlock;
dc9ac125d81faf Alan Previn       2023-05-11  132  	}
dc9ac125d81faf Alan Previn       2023-05-11  133  	if (header->flags & GSC_OUTFLAG_MSG_PENDING) {
dc9ac125d81faf Alan Previn       2023-05-11  134  		drm_dbg(&i915->drm, "gsc PXP reply is busy\n");
dc9ac125d81faf Alan Previn       2023-05-11  135  		/*
dc9ac125d81faf Alan Previn       2023-05-11  136  		 * When the GSC firmware replies with pending bit, it means that the requested
dc9ac125d81faf Alan Previn       2023-05-11  137  		 * operation has begun but the completion is pending and the caller needs
dc9ac125d81faf Alan Previn       2023-05-11  138  		 * to re-request with the gsc_message_handle that was returned by the firmware.
dc9ac125d81faf Alan Previn       2023-05-11  139  		 * until the pending bit is turned off.
dc9ac125d81faf Alan Previn       2023-05-11  140  		 */
dc9ac125d81faf Alan Previn       2023-05-11  141  		*gsc_msg_handle_retry = header->gsc_message_handle;
dc9ac125d81faf Alan Previn       2023-05-11  142  		ret = -EAGAIN;
dc9ac125d81faf Alan Previn       2023-05-11  143  		goto unlock;
dc9ac125d81faf Alan Previn       2023-05-11  144  	}
dc9ac125d81faf Alan Previn       2023-05-11  145  
dc9ac125d81faf Alan Previn       2023-05-11  146  	reply_size = header->message_size - sizeof(*header);
dc9ac125d81faf Alan Previn       2023-05-11  147  	if (reply_size > msg_out_size_max) {
5c315434fdb6ab Nathan Chancellor 2023-05-30  148  		drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%zu)\n",
dc9ac125d81faf Alan Previn       2023-05-11  149  			 reply_size, msg_out_size_max);
dc9ac125d81faf Alan Previn       2023-05-11  150  		reply_size = msg_out_size_max;
dc9ac125d81faf Alan Previn       2023-05-11  151  	}
dc9ac125d81faf Alan Previn       2023-05-11  152  
dc9ac125d81faf Alan Previn       2023-05-11  153  	if (msg_out)
dc9ac125d81faf Alan Previn       2023-05-11  154  		memcpy(msg_out, exec_res->pkt_vaddr + PXP43_MAX_HECI_INOUT_SIZE + sizeof(*header),
dc9ac125d81faf Alan Previn       2023-05-11  155  		       reply_size);
dc9ac125d81faf Alan Previn       2023-05-11  156  	if (msg_out_len)
dc9ac125d81faf Alan Previn       2023-05-11  157  		*msg_out_len = reply_size;
dc9ac125d81faf Alan Previn       2023-05-11  158  
dc9ac125d81faf Alan Previn       2023-05-11  159  unlock:
dc9ac125d81faf Alan Previn       2023-05-11  160  	mutex_unlock(&pxp->tee_mutex);
dc9ac125d81faf Alan Previn       2023-05-11  161  	return ret;
dc9ac125d81faf Alan Previn       2023-05-11  162  }
dc9ac125d81faf Alan Previn       2023-05-11  163
Teres Alexis, Alan Previn Sept. 17, 2023, 8:53 p.m. UTC | #2 
On Sat, 2023-09-16 at 10:25 +0800, lkp wrote:
> Hi Alan,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on cf1e91e884bb1113c653e654e9de1754fc1d4488]
> 
> aAll errors (new ones prefixed by >>):
> 
> 
alan:snip
alan: missed building with PXP config after that last change below - will re-rev this.

>    drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c: In function 'gsccs_send_message':
> > > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:115:52: error: 'GSC_REPLY_LATENCY_MS' undeclared (first use in this function); did you mean 'GSC_HECI_REPLY_LATENCY_MS'?
>      115 |                                                    GSC_REPLY_LATENCY_MS);
>          |                                                    ^~~~~~~~~~~~~~~~~~~~
>          |                                                    GSC_HECI_REPLY_LATENCY_MS
>    drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:115:52: note: each undeclared identifier is reported only once for each function it appears in
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
index 89ed5ee9cded..2fde5c360cff 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
@@ -81,8 +81,17 @@  int intel_gsc_uc_heci_cmd_submit_packet(struct intel_gsc_uc *gsc, u64 addr_in,
 
 	i915_request_add(rq);
 
-	if (!err && i915_request_wait(rq, 0, msecs_to_jiffies(500)) < 0)
-		err = -ETIME;
+	if (!err) {
+		/*
+		 * Start timeout for i915_request_wait only after considering one possible
+		 * pending GSC-HECI submission cycle on the other (non-privileged) path.
+		 */
+		if (wait_for(i915_request_started(rq), GSC_HECI_REPLY_LATENCY_MS))
+			drm_dbg(&gsc_uc_to_gt(gsc)->i915->drm,
+				"Delay in gsc-heci-priv submission to gsccs-hw");
+		if (i915_request_wait(rq, 0, msecs_to_jiffies(GSC_HECI_REPLY_LATENCY_MS)) < 0)
+			err = -ETIME;
+	}
 
 	i915_request_put(rq);
 
@@ -186,6 +195,13 @@  intel_gsc_uc_heci_cmd_submit_nonpriv(struct intel_gsc_uc *gsc,
 	i915_request_add(rq);
 
 	if (!err) {
+		/*
+		 * Start timeout for i915_request_wait only after considering one possible
+		 * pending GSC-HECI submission cycle on the other (privileged) path.
+		 */
+		if (wait_for(i915_request_started(rq), GSC_HECI_REPLY_LATENCY_MS))
+			drm_dbg(&gsc_uc_to_gt(gsc)->i915->drm,
+				"Delay in gsc-heci-non-priv submission to gsccs-hw");
 		if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
 				      msecs_to_jiffies(timeout_ms)) < 0)
 			err = -ETIME;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
index 09d3fbdad05a..c4308291c003 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -12,6 +12,12 @@  struct i915_vma;
 struct intel_context;
 struct intel_gsc_uc;
 
+#define GSC_HECI_REPLY_LATENCY_MS 500
+/*
+ * Max FW response time is 500ms, but this should be counted from the time the
+ * command has hit the GSC-CS hardware, not the preceding handoff to GuC CTB.
+ */
+
 struct intel_gsc_mtl_header {
 	u32 validity_marker;
 #define GSC_HECI_VALIDITY_MARKER 0xA578875A
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
index 298ad38e6c7d..9aae779c4da3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -8,16 +8,14 @@ 
 
 #include <linux/types.h>
 
+#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
+
 struct intel_pxp;
 
-#define GSC_REPLY_LATENCY_MS 210
-/*
- * Max FW response time is 200ms, to which we add 10ms to account for overhead
- * such as request preparation, GuC submission to hw and pipeline completion times.
- */
 #define GSC_PENDING_RETRY_MAXCOUNT 40
 #define GSC_PENDING_RETRY_PAUSE_MS 50
-#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS)
+#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_HECI_REPLY_LATENCY_MS + \
+					 (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS))
 
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_gsccs_fini(struct intel_pxp *pxp);