diff mbox series

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

Message ID 20230909223848.427849-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

Alan Previn Sept. 9, 2023, 10:38 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>
---
 .../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    | 11 ++++++----
 3 files changed, 31 insertions(+), 6 deletions(-)

Comments

Balasubrawmanian, Vivaik Sept. 14, 2023, 10:25 p.m. UTC | #1 
On 9/9/2023 3:38 PM, Alan Previn wrote:
> 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>
> ---
>   .../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    | 11 ++++++----
>   3 files changed, 31 insertions(+), 6 deletions(-)
>
> 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..fe6a2f78cea0 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(500)) < 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..5ae5c5d9608b 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 350
> +/*
> + * Max FW response time is 350ms, 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..a4f17b3ea286 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> @@ -8,16 +8,19 @@
>   
>   #include <linux/types.h>
>   
> +#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
> +
>   struct intel_pxp;
>   
> -#define GSC_REPLY_LATENCY_MS 210
> +#define GSC_REPLY_LATENCY_MS GSC_HECI_REPLY_LATENCY_MS
>   /*
> - * 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.
> + * Max FW response time is 350ms, but this should be counted from the time the
> + * command has hit the GSC-CS hardware, not the preceding handoff to GuC CTB.
>    */
>   #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_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);

Reviewed-by: Balasubrawmanian, Vivaik 
<vivaik.balasubrawmanian@intel.com> 
<mailto:vivaik.balasubrawmanian@intel.com>
Alan Previn Sept. 15, 2023, 5:30 p.m. UTC | #2 
On Sat, 2023-09-09 at 15:38 -0700, Teres Alexis, Alan Previn wrote:
> 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>
> ---
>  .../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    | 11 ++++++----
>  3 files changed, 31 insertions(+), 6 deletions(-)
alan: snip


> index 09d3fbdad05a..5ae5c5d9608b 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 350
> +/*
> + * Max FW response time is 350ms, but this should be counted from the time the
> + * command has hit the GSC-CS hardware, not the preceding handoff to GuC CTB.
> + */
alan: continue to face timeout issues - so increasing this to ~500 to absorb other hw/sw system latencies.
this also matches what the gsc-proxy code was doing - so i could use the same macro for that other code path.
Alan Previn Sept. 15, 2023, 5:58 p.m. UTC | #3 
On Sat, 2023-09-09 at 15:38 -0700, Teres Alexis, Alan Previn wrote:
> 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,
alan:snip
Vivaik replied with RB on dri-devel: https://lists.freedesktop.org/archives/dri-devel/2023-September/422861.html
we connected offline and agreed that his RB can remain standing on condition i fix the PAGE_ALIGNED -> PAGE_ALIGN fix.
Thanks Vivaik for reviewing.
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..fe6a2f78cea0 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(500)) < 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..5ae5c5d9608b 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 350
+/*
+ * Max FW response time is 350ms, 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..a4f17b3ea286 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -8,16 +8,19 @@ 
 
 #include <linux/types.h>
 
+#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
+
 struct intel_pxp;
 
-#define GSC_REPLY_LATENCY_MS 210
+#define GSC_REPLY_LATENCY_MS GSC_HECI_REPLY_LATENCY_MS
 /*
- * 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.
+ * Max FW response time is 350ms, but this should be counted from the time the
+ * command has hit the GSC-CS hardware, not the preceding handoff to GuC CTB.
  */
 #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_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);