Re: drm/xe/devcoredump: Drop a FIXME in devcoredump

[Shekhar Chauhan <shekhar.chauhan@intel.com>]

On 5/12/2026 12:47, Vivekanandan, Balasubramani wrote:
> On 12.05.2026 11:25, Shekhar Chauhan wrote:
>> The FIXME says that xe_engine_snapshot_print.. is accessing persistent
>> driver data, unlike what the FIXME says that it does. Drop the FIXME
>> since the current code is not going to access the hardware while
>> dumping.
> Is there any typo in the above commit message? Did you intent to say
>
> The xe_engine_snapshot_print.. is accessing persistent
> driver data, unlike what the FIXME says that it does.
>
> After making that correction,
>
> Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Okay, that can be done (although my initial intent was to keep it as 
"that" to signify what the FIXME said). But, making it as "the" instead 
of "that" also makes sense.

Since it is a minor change, do you think I should send a new revision 
for it or is this something the maintainer(s) can just take care of 
while merging?

-shekhar

>
> Regards,
> Bala
>
>
>> More details about this patch:
>> https://patchwork.freedesktop.org/patch/703884/?series=161407&rev=1
>> The starting two feedbacks make sense and the original patch is wrong
>> in adding those changes, but the last feedback is the one which
>> highlights the point.
>>
>> Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_capture.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
>> index bc49e40165a3..21f7caf9ea08 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
>> @@ -1841,12 +1841,6 @@ void xe_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm
>>   		   str_yes_no(snapshot->kernel_reserved));
>>   
>>   	for (type = GUC_STATE_CAPTURE_TYPE_GLOBAL; type < GUC_STATE_CAPTURE_TYPE_MAX; type++) {
>> -		/*
>> -		 * FIXME: During devcoredump print we should avoid accessing the
>> -		 * driver pointers for gt or engine. Printing should be done only
>> -		 * using the snapshot captured. Here we are accessing the gt
>> -		 * pointer. It should be fixed.
>> -		 */
>>   		list = xe_guc_capture_get_reg_desc_list(gt, GUC_CAPTURE_LIST_INDEX_PF, type,
>>   							capture_class, false);
>>   		snapshot_print_by_list_order(snapshot, p, type, list);

-- 
Shekhar Chauhan
Linux Graphics Software Engineer
Intel Corporation