Re: [Intel-xe] [RFC PATCH] drm/xe: Add uapi for dumpable bos

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Den 2023-10-10 kl. 23:04, skrev Souza, Jose:
> + Lionel
>
> Hi Lionel
>
> Can you please take over this thread while I'm on vacation?
>
> On Fri, 2023-10-06 at 22:12 +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> On 2023-10-06 20:29, Souza, Jose wrote:
>>> On Fri, 2023-10-06 at 19:18 +0200, maarten.lankhorst@linux.intel.com wrote:
>>>> From: Maarten Lankhorst <dev@lankhorst.se>
>>>>
>>>> Add XE_GEM_CREATE_FLAG_DUMPABLE for use with VM_BIND.
>>>> For BO's with vram regions, XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM
>>>> also needs to be specified.
>>>>
>>>> After this, we can use the flag XE_VM_BIND_FLAG_DUMPABLE to notify
>>>> devcoredump that this mapping should be dumped.
>>>>
>>>> This is not hooked up, but the uapi should be ready before merging.
>>>> If needed, we can relax the restrictions later.
>>>>
>>>> It's also not yet certain whether the BO contents will be preserved,
>>>> or a reference to the BO. The latter allows us to skip a whole lot of
>>>> tricky locking. This requires userspace to destroy all bo's that were
>>>> marked dumpable though, but I feel this is a cleaner solution from
>>>> a kernel perspective.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_bo.c |  9 +++++++++
>>>>    drivers/gpu/drm/xe/xe_bo.h |  1 +
>>>>    drivers/gpu/drm/xe/xe_vm.c | 17 ++++++++++++++---
>>>>    include/uapi/drm/xe_drm.h  | 25 +++++++++++++++++++++++++
>>>>    4 files changed, 49 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>>>> index 61789c0e88fb..0de0bd5eb825 100644
>>>> --- a/drivers/gpu/drm/xe/xe_bo.c
>>>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>>>> @@ -1776,6 +1776,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>    			 ~(XE_GEM_CREATE_FLAG_DEFER_BACKING |
>>>>    			   XE_GEM_CREATE_FLAG_SCANOUT |
>>>>    			   XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM |
>>>> +			   XE_GEM_CREATE_FLAG_DUMPABLE |
>>>>    			   xe->info.mem_region_mask)))
>>>>    		return -EINVAL;
>>>>    
>>>> @@ -1810,6 +1811,14 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>    		bo_flags |= XE_BO_NEEDS_CPU_ACCESS;
>>>>    	}
>>>>    
>>>> +	if (args->flags & XE_GEM_CREATE_FLAG_DUMPABLE) {
>>>> +		if (XE_IOCTL_DBG(xe, (bo_flags & XE_BO_CREATE_VRAM_MASK &&
>>>> +				 !(bo_flags & XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM))))
>>>> +			return -EINVAL;
>>>> +
>>>> +		bo_flags |= XE_BO_NEEDS_CPU_ACCESS | XE_BO_DUMPABLE;
>>> It is not viable to just ignore VMAs that can't be mapped in CPU during dump?
>>> Because it will not be viable to dump real applications in small PCIe bar systems, it would run out of CPU mappable memory.
>>>
>>> And if UMD really wants a dump of a small test that fits into the CPU mappable memory it can set XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM and get the
>>> whole dump.
>>>
>>> So we would be able to drop the need of XE_GEM_CREATE_FLAG_DUMPABLE.
>> It would be doable, we could evict the BO's to sysmem if we only take a
>> reference and dump each bo.
>>
>> Taking a reference during crash would require the userspace app to
>> recreate all dumpable bo's after a hang, or it will risk overwriting
>> the state at the time of the dump.
> But what/who would overwrite the bos during dump?
>
> I don't think it is possible to recreate all BOs that are necessary in dump.
> For example vertex information is provided my applications, would not be viable to UMD keep copies of that.

What I mean here is that the bo's are still accessible by the app, but 
it's the apps responsibility of not overwriting it before a crash dump 
is taken. So in this case if you want to keep the vertex data, you can 
copy the contents from the old bo, or hope it's simply not altered 
before the crash dump is taken.

Cheers,

~Maarten