Re: [PATCH i-g-t 3/4] eudebug: Add eudebug pagefault event declarations

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Andrzej,

On 18.11.2024 17:52, Hajda, Andrzej wrote:
> W dniu 15.11.2024 o 15:11, Gwan-gyeong Mun pisze:
>> From: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>
>> Update the xe_eudebug drm to include pagefault events.
>>
>> Co-developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>> ---
>>   include/drm-uapi-experimental/xe_drm_eudebug.h | 13 +++++++++++++
> 
> I am lost between prelim/eudebug/experimental header placements. So just 
> out of curiosity what is the role of drm-uapi-experimental comparing to 
> prelim or just drm-uapi?

The experimental placement is there to ensure that the eudebug api won't 
get removed by a uapi update procedure documented by igt docs. It was 
reported by and requested by:

	Ville Syrjälä <ville.syrjala@linux.intel.com>

Apart from the placement, this is also a single file (compared to two 
files in the public kernel eudebug branch IIRC) which was requested 
during upstream review. But the api itself should be identical to the 
one found in the public eudebug-dev kernel branch.

Thanks,
Christoph

> 
> 
>>   lib/xe/xe_eudebug.c                            | 11 +++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/include/drm-uapi-experimental/xe_drm_eudebug.h 
>> b/include/drm-uapi-experimental/xe_drm_eudebug.h
>> index f50051e9a..ed2e728c2 100644
>> --- a/include/drm-uapi-experimental/xe_drm_eudebug.h
>> +++ b/include/drm-uapi-experimental/xe_drm_eudebug.h
>> @@ -154,6 +154,7 @@ struct drm_xe_eudebug_event {
>>   #define DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE    9
>>   #define DRM_XE_EUDEBUG_EVENT_METADATA        10
>>   #define DRM_XE_EUDEBUG_EVENT_VM_BIND_OP_METADATA 11
>> +#define DRM_XE_EUDEBUG_EVENT_PAGEFAULT        12
>>       __u16 flags;
>>   #define DRM_XE_EUDEBUG_EVENT_CREATE        (1 << 0)
>> @@ -352,6 +353,18 @@ struct drm_xe_eudebug_event_vm_bind_op_metadata {
>>       __u64 metadata_cookie;
>>   };
>> +struct drm_xe_eudebug_event_pagefault {
>> +    struct drm_xe_eudebug_event base;
>> +
>> +    __u64 client_handle;
>> +    __u64 exec_queue_handle;
>> +    __u64 lrc_handle;
>> +    __u32 flags;
>> +    __u32 bitmask_size;
>> +    __u64 pagefault_address;
>> +    __u8 bitmask[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
>> index c5814421b..de01e6b49 100644
>> --- a/lib/xe/xe_eudebug.c
>> +++ b/lib/xe/xe_eudebug.c
>> @@ -91,6 +91,8 @@ static const char *type_to_str(unsigned int type)
>>           return "metadata";
>>       case DRM_XE_EUDEBUG_EVENT_VM_BIND_OP_METADATA:
>>           return "vm_bind_op_metadata";
>> +    case DRM_XE_EUDEBUG_EVENT_PAGEFAULT:
>> +        return "pagefault";
>>       }
>>       return "UNKNOWN";
>> @@ -222,6 +224,15 @@ static const char *event_members_to_str(struct 
>> drm_xe_eudebug_event *e, char *bu
>>               op->vm_bind_op_ref_seqno, op->metadata_handle, 
>> op->metadata_cookie);
>>           break;
>>       }
>> +    case DRM_XE_EUDEBUG_EVENT_PAGEFAULT: {
>> +        struct drm_xe_eudebug_event_pagefault *pf = (void *)e;
> 
> maybe better: pf = container_of(e, pf, base);
> 
> Beside this:
> 
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> 
> Regards
> Andrzej
> 
>> +
>> +        sprintf(buf, "client_handle=%llu, exec_queue_handle=%llu, "
>> +            "lrc_handle=%llu, bitmask_size=%d, 
>> pagefault_address=0x%llx",
>> +            pf->client_handle, pf->exec_queue_handle, pf->lrc_handle,
>> +            pf->bitmask_size, pf->pagefault_address);
>> +        break;
>> +    }
>>       default:
>>           strcpy(buf, "<...>");
>>       }
>