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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 3/20/2024 4:20 AM, Lucas De Marchi wrote:
> 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.

I always observed NPD to be func name + offset,  not the line number.


> 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

I will not defend this patch much :) now that I think I have a better one

https://patchwork.freedesktop.org/series/131371/


Regards,

Nirmoy

>
> 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@intel.com>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@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;