[PATCH v2 2/2] drm/xe: Add a WARN_ON for NULL job in xe_sync_entry_signal

[Lucas De Marchi]

On Mon, Mar 18, 2024 at 07:41:56PM +0100, Nirmoy Das wrote:
>
>On 3/18/2024 6:41 PM, Michal Wajdeczko wrote:
>>
>>On 18.03.2024 17:43, Nirmoy Das wrote:
>>>Add a warn for NULL job when sync->type is
>>>DRM_XE_SYNC_TYPE_USER_FENCE. This should be a programming
>>>error and should never happen so warn and let the kernel crash
>>>if that ever happens.
>>IMO adding WARN and then let kernel crash is pointless as you will have
>>almost exactly the same report as from NPD
>
>WARN should give us the line number which is not the case for NDP I think.

and a NPD will put the line with the NPD straight in the RIP line. So,
to second what Michal said, what's the point?

it would make sense if you were adding the warning earlier and make the
driver still behave. The place it was added, not so much

Lucas De Marchi

>
>
>Regards,
>
>Nirmoy
>
>>
>>for programming errors we should xe_assert() that will provide necessary
>>hint during code refactor but will be compiled on production builds
>>
>>only if you feel that that job could be still (but unlikely) NULL then
>>you should use drm_WARN/xe_gt_WARN and provide necessary fallback
>>
>>>Cc: Matthew Auld <matthew.auld at intel.com>
>>>Cc: Matthew Brost <matthew.brost at intel.com>
>>>Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>>>---
>>>  drivers/gpu/drm/xe/xe_sync.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
>>>index 02c9577fe418..247505c3478d 100644
>>>--- a/drivers/gpu/drm/xe/xe_sync.c
>>>+++ b/drivers/gpu/drm/xe/xe_sync.c
>>>@@ -255,6 +255,7 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct xe_sched_job *job,
>>>  			dma_fence_put(fence);
>>>  		}
>>>  	} else if (sync->type == DRM_XE_SYNC_TYPE_USER_FENCE) {
>>>+		XE_WARN_ON(!job);
>>>  		job->user_fence.used = true;
>>>  		job->user_fence.addr = sync->addr;
>>>  		job->user_fence.value = sync->timeline_value;