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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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;
>