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

[Jani Nikula <jani.nikula@linux.intel.com>]

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