diff mbox series

[v1] drm/xe/guc: Fix h2g_write usage of GUC_CTB_MSG_MAX_LEN

Message ID 20230602185213.2635921-1-alan.previn.teres.alexis@intel.com (mailing list archive)
State New, archived
Headers show
Series [v1] drm/xe/guc: Fix h2g_write usage of GUC_CTB_MSG_MAX_LEN | expand

Commit Message

Alan Previn June 2, 2023, 6:52 p.m. UTC 
In the ABI header, GUC_CTB_MSG_MIN_LEN is '1' because
GUC_CTB_HDR_LEN is 1. This aligns with H2G/G2H CTB specification
where all command formats are defined in units of dwords so that '1'
is a dword. Accordingly, GUC_CTB_MSG_MAX_LEN is 256-1 (i.e. 255
dwords). However, h2g_write was incorrectly assuming that
GUC_CTB_MSG_MAX_LEN was in bytes. Fix this.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_ct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 9f49c413b187c00f223eaf1e8dbe2c1a97c9954b

Comments

Matthew Brost June 5, 2023, 4:47 p.m. UTC | #1 
On Fri, Jun 02, 2023 at 11:52:13AM -0700, Alan Previn wrote:
> In the ABI header, GUC_CTB_MSG_MIN_LEN is '1' because
> GUC_CTB_HDR_LEN is 1. This aligns with H2G/G2H CTB specification
> where all command formats are defined in units of dwords so that '1'
> is a dword. Accordingly, GUC_CTB_MSG_MAX_LEN is 256-1 (i.e. 255
> dwords). However, h2g_write was incorrectly assuming that
> GUC_CTB_MSG_MAX_LEN was in bytes. Fix this.
> 

The patch itself make sense but concerned about the value of
GUC_CTB_MSG_MAX_LEN. Is the max size of CTB really 256 DWs? That seems
way to large. Also I think some tools are going to complain with the
stack being this large too.

Matt

> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_ct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 137c184df487..8e9df6052b02 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -401,7 +401,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  {
>  	struct xe_device *xe = ct_to_xe(ct);
>  	struct guc_ctb *h2g = &ct->ctbs.h2g;
> -	u32 cmd[GUC_CTB_MSG_MAX_LEN / sizeof(u32)];
> +	u32 cmd[GUC_CTB_MSG_MAX_LEN];
>  	u32 cmd_len = len + GUC_CTB_HDR_LEN;
>  	u32 cmd_idx = 0, i;
>  	u32 tail = h2g->info.tail;
> @@ -409,7 +409,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  							 tail * sizeof(u32));
>  
>  	lockdep_assert_held(&ct->lock);
> -	XE_BUG_ON(len * sizeof(u32) > GUC_CTB_MSG_MAX_LEN);
> +	XE_BUG_ON(len > GUC_CTB_MSG_MAX_LEN - 1);
>  	XE_BUG_ON(tail > h2g->info.size);
>  
>  	/* Command will wrap, zero fill (NOPs), return and check credits again */
> 
> base-commit: 9f49c413b187c00f223eaf1e8dbe2c1a97c9954b
> -- 
> 2.39.0
>
Alan Previn June 8, 2023, 12:12 a.m. UTC | #2 
On Fri, 2023-06-02 at 11:52 -0700, Alan Previn wrote:
> In the ABI header, GUC_CTB_MSG_MIN_LEN is '1' because
> GUC_CTB_HDR_LEN is 1. This aligns with H2G/G2H CTB specification
> where all command formats are defined in units of dwords so that '1'
> is a dword. Accordingly, GUC_CTB_MSG_MAX_LEN is 256-1 (i.e. 255
> dwords). However, h2g_write was incorrectly assuming that
> GUC_CTB_MSG_MAX_LEN was in bytes. Fix this.
> 
> 
alan:snip

> The patch itself make sense but concerned about the value of
> GUC_CTB_MSG_MAX_LEN. Is the max size of CTB really 256 DWs? That seems
> way to large. Also I think some tools are going to complain with the
> stack being this large too.
> 
> Matt

alan: good point - i will go back and find if we have this value internally spec'd before we continue.
Alan Previn June 8, 2023, 10:13 p.m. UTC | #3 
On Thu, 2023-06-08 at 00:12 +0000, Teres Alexis, Alan Previn wrote:
> On Fri, 2023-06-02 at 11:52 -0700, Alan Previn wrote:
> 
> alan: good point - i will go back and find if we have this value internally spec'd before we continue.
alan: actually, even if the spec allowed very large cmds (as its internally spec'd to allow that), we shouldnt consumer the stack like that - will fix this that way.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
index 137c184df487..8e9df6052b02 100644
--- a/drivers/gpu/drm/xe/xe_guc_ct.c
+++ b/drivers/gpu/drm/xe/xe_guc_ct.c
@@ -401,7 +401,7 @@  static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
 {
 	struct xe_device *xe = ct_to_xe(ct);
 	struct guc_ctb *h2g = &ct->ctbs.h2g;
-	u32 cmd[GUC_CTB_MSG_MAX_LEN / sizeof(u32)];
+	u32 cmd[GUC_CTB_MSG_MAX_LEN];
 	u32 cmd_len = len + GUC_CTB_HDR_LEN;
 	u32 cmd_idx = 0, i;
 	u32 tail = h2g->info.tail;
@@ -409,7 +409,7 @@  static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
 							 tail * sizeof(u32));
 
 	lockdep_assert_held(&ct->lock);
-	XE_BUG_ON(len * sizeof(u32) > GUC_CTB_MSG_MAX_LEN);
+	XE_BUG_ON(len > GUC_CTB_MSG_MAX_LEN - 1);
 	XE_BUG_ON(tail > h2g->info.size);
 
 	/* Command will wrap, zero fill (NOPs), return and check credits again */