Re: [PATCH] drm/xe/gsc: Add status check during gsc header readout

Re: [PATCH] drm/xe/gsc: Add status check during gsc header readout

[Daniele Ceraolo Spurio]

On 1/21/2024 10:35 PM, Suraj Kandpal wrote:
> Before checking if data is present in the message reply check the
> status in header and see if it indicates any error.
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_gsc_submit.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c
> index 9ecc1ead6844..0ccc6d52dd3f 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_submit.c
> @@ -125,11 +125,18 @@ int xe_gsc_read_out_header(struct xe_device *xe,
>   {
>   	u32 marker = mtl_gsc_header_rd(xe, map, offset, validity_marker);
>   	u32 size = mtl_gsc_header_rd(xe, map, offset, message_size);
> +	u32 status = mtl_gsc_header_rd(xe, map, offset, status);
>   	u32 payload_size = size - GSC_HDR_SIZE;
>   
>   	if (marker != GSC_HECI_VALIDITY_MARKER)
>   		return -EPROTO;
>   
> +	if (status != 0) {
> +		drm_dbg_kms(xe, "GSC header readout indicates error: %d\n",
> +			    status);

This should be an error message (drm_err). With that:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> +		return -EINVAL;
> +	}
> +
>   	if (size < GSC_HDR_SIZE || payload_size < min_payload_size)
>   		return -ENODATA;
>   

Re: [PATCH] drm/xe/gsc: Add status check during gsc header readout

[Daniele Ceraolo Spurio]

On 1/22/2024 10:28 PM, Suraj Kandpal wrote:
> Before checking if data is present in the message reply check the
> status in header and see if it indicates any error.
>
> --v2
> - Use drm_err() instead of drm_dbg_kms() [Daniele]
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_gsc_submit.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c
> index 9ecc1ead6844..3c402fe36026 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_submit.c
> @@ -125,11 +125,18 @@ int xe_gsc_read_out_header(struct xe_device *xe,
>   {
>   	u32 marker = mtl_gsc_header_rd(xe, map, offset, validity_marker);
>   	u32 size = mtl_gsc_header_rd(xe, map, offset, message_size);
> +	u32 status = mtl_gsc_header_rd(xe, map, offset, status);
>   	u32 payload_size = size - GSC_HDR_SIZE;
>   
>   	if (marker != GSC_HECI_VALIDITY_MARKER)
>   		return -EPROTO;
>   
> +	if (status != 0) {
> +		drm_err(xe, "GSC header readout indicates error: %d\n",

Sorry I didn't notice it in the first rev, but this should be &xe->drm 
and not just xe. Note that since drm is the first thing inside the xe 
structure, a pointer to the xe structure also points to xe->drm, so it 
still works like this, but it isn't clean.

Daniele

> +			status);
> +		return -EINVAL;
> +	}
> +
>   	if (size < GSC_HDR_SIZE || payload_size < min_payload_size)
>   		return -ENODATA;
>   

Re: [PATCH] drm/xe/gsc: Add status check during gsc header readout

[Jani Nikula]

On Tue, 23 Jan 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
> On 1/22/2024 10:28 PM, Suraj Kandpal wrote:
>> Before checking if data is present in the message reply check the
>> status in header and see if it indicates any error.
>>
>> --v2
>> - Use drm_err() instead of drm_dbg_kms() [Daniele]
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gsc_submit.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c
>> index 9ecc1ead6844..3c402fe36026 100644
>> --- a/drivers/gpu/drm/xe/xe_gsc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_gsc_submit.c
>> @@ -125,11 +125,18 @@ int xe_gsc_read_out_header(struct xe_device *xe,
>>   {
>>   	u32 marker = mtl_gsc_header_rd(xe, map, offset, validity_marker);
>>   	u32 size = mtl_gsc_header_rd(xe, map, offset, message_size);
>> +	u32 status = mtl_gsc_header_rd(xe, map, offset, status);
>>   	u32 payload_size = size - GSC_HDR_SIZE;
>>   
>>   	if (marker != GSC_HECI_VALIDITY_MARKER)
>>   		return -EPROTO;
>>   
>> +	if (status != 0) {
>> +		drm_err(xe, "GSC header readout indicates error: %d\n",
>
> Sorry I didn't notice it in the first rev, but this should be &xe->drm 
> and not just xe. Note that since drm is the first thing inside the xe 
> structure, a pointer to the xe structure also points to xe->drm, so it 
> still works like this, but it isn't clean.

Does it actually build? There's no xe->dev member. See the definitions
of drm_err and __drm_printk.

Btw Suraj's messages didn't get through to the list, and thus CI wasn't
triggered.

BR,
Jani.



>
> Daniele
>
>> +			status);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (size < GSC_HDR_SIZE || payload_size < min_payload_size)
>>   		return -ENODATA;
>>   
>

-- 
Jani Nikula, Intel

[PATCH] drm/xe/gsc: Add status check during gsc header readout

[Suraj Kandpal]

Before checking if data is present in the message reply check the
status in header and see if it indicates any error.

--v2
- Use drm_err() instead of drm_dbg_kms() [Daniele]

--v3
- Use &xe->drm in drm_err to make it more cleaner [Daniele]

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/xe/xe_gsc_submit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c
index 9ecc1ead6844..348994b271be 100644
--- a/drivers/gpu/drm/xe/xe_gsc_submit.c
+++ b/drivers/gpu/drm/xe/xe_gsc_submit.c
@@ -125,11 +125,18 @@ int xe_gsc_read_out_header(struct xe_device *xe,
 {
 	u32 marker = mtl_gsc_header_rd(xe, map, offset, validity_marker);
 	u32 size = mtl_gsc_header_rd(xe, map, offset, message_size);
+	u32 status = mtl_gsc_header_rd(xe, map, offset, status);
 	u32 payload_size = size - GSC_HDR_SIZE;
 
 	if (marker != GSC_HECI_VALIDITY_MARKER)
 		return -EPROTO;
 
+	if (status != 0) {
+		drm_err(&xe->drm, "GSC header readout indicates error: %d\n",
+			status);
+		return -EINVAL;
+	}
+
 	if (size < GSC_HDR_SIZE || payload_size < min_payload_size)
 		return -ENODATA;
 
-- 
2.25.1