There was a gap in handling MMIO result from CTB (de)registration and while fixing it improve some other error reports.
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
Michal Wajdeczko (4): drm/i915/guc: Verify result from CTB (de)register action drm/i915/guc: Print error name on CTB (de)registration failure drm/i915/guc: Print error name on CTB send failure drm/i915/guc: Move and improve error message for missed CTB reply
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 30 ++++++++++++++--------- 1 file changed, 18 insertions(+), 12 deletions(-)
In commit b839a869dfc9 ("drm/i915/guc: Add support for data reporting in GuC responses") we missed the hypothetical case that GuC might return positive non-zero value as success data.
While that would be lucky treated as error case, and at the end will result in reporting valid -EIO, in the meantime this value will be passed to ERR_PTR that could be misleading.
v2: rebased
Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: Dan Carpenter dan.carpenter@oracle.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 43409044528e..a26bb55c0898 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -148,12 +148,15 @@ static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type, FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr), FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr), }; + int ret;
GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST); GEM_BUG_ON(size % SZ_4K);
/* CT registration must go over MMIO */ - return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); + ret = intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); + + return ret > 0 ? -EPROTO : ret; }
static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, @@ -177,11 +180,14 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type) FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB), FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type), }; + int ret;
GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);
/* CT deregistration must go over MMIO */ - return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); + ret = intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); + + return ret > 0 ? -EPROTO : ret; }
static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)
On Thu, Jul 01, 2021 at 05:55:10PM +0200, Michal Wajdeczko wrote:
In commit b839a869dfc9 ("drm/i915/guc: Add support for data reporting in GuC responses") we missed the hypothetical case that GuC might return positive non-zero value as success data.
While that would be lucky treated as error case, and at the end will result in reporting valid -EIO, in the meantime this value will be passed to ERR_PTR that could be misleading.
v2: rebased
Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: Dan Carpenter dan.carpenter@oracle.com
Return value where all integers are possible is always a bit fragile, especially here where the meaning additionally depends upon whether you supply a reply buffer or not.
Would be good to document this with some kerneldoc, but maybe the CTB interface is a bit too unclear here and that's not worth it (there's at least a ton of functions/variants that just arent used).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 43409044528e..a26bb55c0898 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -148,12 +148,15 @@ static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type, FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr), FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr), };
int ret;
GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST); GEM_BUG_ON(size % SZ_4K);
/* CT registration must go over MMIO */
- return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);
- ret = intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);
- return ret > 0 ? -EPROTO : ret;
}
static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, @@ -177,11 +180,14 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type) FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB), FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type), };
int ret;
GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);
/* CT deregistration must go over MMIO */
- return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);
- ret = intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);
- return ret > 0 ? -EPROTO : ret;
}
static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)
2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err)) - CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n", - guc_ct_buffer_type_to_str(type), err); + CT_ERROR(ct, "Failed to register %s buffer (%pe)\n", + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); return err; }
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err)) - CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n", - guc_ct_buffer_type_to_str(type), err); + CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n", + guc_ct_buffer_type_to_str(type), ERR_PTR(err)); return err; }
On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err))
CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));
errname() is what you want here, not this convoluted jumping through hoops to fake an error pointer.
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return err; }
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err))
CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));}
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 18.08.2021 16:20, Daniel Vetter wrote:
On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err))
CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));errname() is what you want here, not this convoluted jumping through hoops to fake an error pointer.
nope, I was already trying that shortcut, but errname() is not exported so we fail with
ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined!
so unless we add that export we must follow initial approach [1]
-Michal
[1] https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a712...
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return err; }
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err))
CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));}
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 18, 2021 at 5:11 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 18.08.2021 16:20, Daniel Vetter wrote:
On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err))
CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));errname() is what you want here, not this convoluted jumping through hoops to fake an error pointer.
nope, I was already trying that shortcut, but errname() is not exported so we fail with
ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined!
so unless we add that export we must follow initial approach [1]
Then we export that function. This is all open source, we can actually fix things up, there should _never_ be a need to hack around things like this.
And yes I'm keenly aware that there's a pile of i915-gem code which outright flaunts this principle, but that doesn't mean we should continue with that. scripts/get_maintainers.pl can help with finding all the people you need to cc on that export patch. -Daniel
-Michal
[1] https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a712...
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return err;}
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err))
CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));}
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 18.08.2021 18:35, Daniel Vetter wrote:
On Wed, Aug 18, 2021 at 5:11 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 18.08.2021 16:20, Daniel Vetter wrote:
On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err))
CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));errname() is what you want here, not this convoluted jumping through hoops to fake an error pointer.
nope, I was already trying that shortcut, but errname() is not exported so we fail with
ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined!
so unless we add that export we must follow initial approach [1]
Then we export that function. This is all open source, we can actually fix things up, there should _never_ be a need to hack around things like this.
simple export might be not sufficient, as this function returns NULL for unrecognized error codes, and it might be hard to print that code in plain format, as it %pe does it for us for free.
is that big problem to use ERR_PTR? I'm not the only/first one
see drivers/net/can/usb/etas_es58x/es58x_core.c drivers/net/ethernet/freescale/enetc/enetc_pf.c drivers/net/phy/phylink.c ...
And yes I'm keenly aware that there's a pile of i915-gem code which outright flaunts this principle, but that doesn't mean we should continue with that. scripts/get_maintainers.pl can help with finding all the people you need to cc on that export patch.
I'm perfectly fine with updating/fixing shared code (did that before, have few more ideas on my todo-list) but in this case I'm not so sure
-Michal
-Daniel
-Michal
[1] https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a712...
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return err;}
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err))
CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));}
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 18, 2021 at 09:12:32PM +0200, Michal Wajdeczko wrote:
On 18.08.2021 18:35, Daniel Vetter wrote:
On Wed, Aug 18, 2021 at 5:11 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 18.08.2021 16:20, Daniel Vetter wrote:
On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index a26bb55c0898..18d52c39f0c2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, err = guc_action_register_ct_buffer(ct_to_guc(ct), type, desc_addr, buff_addr, size); if (unlikely(err))
CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));errname() is what you want here, not this convoluted jumping through hoops to fake an error pointer.
nope, I was already trying that shortcut, but errname() is not exported so we fail with
ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined!
so unless we add that export we must follow initial approach [1]
Then we export that function. This is all open source, we can actually fix things up, there should _never_ be a need to hack around things like this.
simple export might be not sufficient, as this function returns NULL for unrecognized error codes, and it might be hard to print that code in plain format, as it %pe does it for us for free.
"(%s:%i)", errname(ret), ret
Would work, but maybe not so pretty. Otoh %pe for unrecognized integers is also not very pretty.
is that big problem to use ERR_PTR? I'm not the only/first one
see drivers/net/can/usb/etas_es58x/es58x_core.c drivers/net/ethernet/freescale/enetc/enetc_pf.c drivers/net/phy/phylink.c ...
Ah yeah, looking through grep more than half the users do this pattern. Which still feels extremely silly, because it's not a pointer we're printing, and when it's not an errno we should probably print it as an integer or something. But also meh. On both patches, as-is:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
And yes I'm keenly aware that there's a pile of i915-gem code which outright flaunts this principle, but that doesn't mean we should continue with that. scripts/get_maintainers.pl can help with finding all the people you need to cc on that export patch.
I'm perfectly fine with updating/fixing shared code (did that before, have few more ideas on my todo-list) but in this case I'm not so sure
I think an %ie extension or something like that for printk would make some sense. There's absolute enormous amounts of this kind of casting going on, and it just doesn't make sense to me.
It would be pretty easy way to get like 100 patches into the kernel to clean it all up :-) -Daniel
-Michal
-Daniel
-Michal
[1] https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a712...
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return err;}
@@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
if (unlikely(err))
CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",guc_ct_buffer_type_to_str(type), err);
CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",guc_ct_buffer_type_to_str(type), ERR_PTR(err));}
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 18d52c39f0c2..8110586ce1fd 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -580,8 +580,8 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { - CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n", - action[0], ret, status); + CT_ERROR(ct, "Sending action %#x failed (%pe) status=%#X\n", + action[0], ERR_PTR(ret), status); } else if (unlikely(ret)) { CT_DEBUG(ct, "send action %#x returned %d (%#x)\n", action[0], ret, ret);
On Thu, Jul 01, 2021 at 05:55:12PM +0200, Michal Wajdeczko wrote:
Instead of plain error value (%d) print more user friendly error name (%pe).
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 18d52c39f0c2..8110586ce1fd 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -580,8 +580,8 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) {
CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",action[0], ret, status);
CT_ERROR(ct, "Sending action %#x failed (%pe) status=%#X\n",action[0], ERR_PTR(ret), status);
errname(), not this, with that:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
} else if (unlikely(ret)) { CT_DEBUG(ct, "send action %#x returned %d (%#x)\n", action[0], ret, ret); -- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
If we timeout waiting for a CT reply we print very simple error message. Improve that and by moving error reporting to the caller we can use CT_ERROR instead of DRM_ERROR and report just fence as error code will be reported later anyway.
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 8110586ce1fd..f488a51e1ebe 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -490,9 +490,6 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) err = wait_for(done, 10); #undef done
- if (unlikely(err)) - DRM_ERROR("CT: fence %u err %d\n", req->fence, err); - *status = req->status; return err; } @@ -536,8 +533,11 @@ static int ct_send(struct intel_guc_ct *ct, intel_guc_notify(ct_to_guc(ct));
err = wait_for_ct_request_update(&request, status); - if (unlikely(err)) + if (unlikely(err)) { + CT_ERROR(ct, "No response for request %#x (fence %u)\n", + action[0], request.fence); goto unlink; + }
if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != GUC_HXG_TYPE_RESPONSE_SUCCESS) { err = -EIO;
On Thu, Jul 01, 2021 at 05:55:13PM +0200, Michal Wajdeczko wrote:
If we timeout waiting for a CT reply we print very simple error message. Improve that and by moving error reporting to the caller we can use CT_ERROR instead of DRM_ERROR and report just fence as error code will be reported later anyway.
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com
Look reasonable.
Btw for within the driver we generally never document static inline functions with full kerneldoc. That's overkill and they get stale real fast. What would be useful to document is the interface with the driver at large (i.e. non-static functions), especially for something that's used all over like CTB will be. But then we're back to responsibilities and especialy aroung gpu reset, so not sure whether documenting the current code before that's sorted is the best idea.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 8110586ce1fd..f488a51e1ebe 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -490,9 +490,6 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) err = wait_for(done, 10); #undef done
- if (unlikely(err))
DRM_ERROR("CT: fence %u err %d\n", req->fence, err);- *status = req->status; return err;
} @@ -536,8 +533,11 @@ static int ct_send(struct intel_guc_ct *ct, intel_guc_notify(ct_to_guc(ct));
err = wait_for_ct_request_update(&request, status);
- if (unlikely(err))
if (unlikely(err)) {
CT_ERROR(ct, "No response for request %#x (fence %u)\n",action[0], request.fence);goto unlink;
}
if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != GUC_HXG_TYPE_RESPONSE_SUCCESS) { err = -EIO;
-- 2.25.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Michal Wajdeczko