Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"

From: "Christian König" <christian.koenig@amd.com>
To: Qiang Yu <yuq825@gmail.com>
Cc: Simon Shields <simon@lineageos.org>,
	devicetree@vger.kernel.org, Connor Abbott <cwabbott0@gmail.com>,
	Marek Vasut <marex@denx.de>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Andrei Paulau <7134956@gmail.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Vasily Khoruzhick <anarsoul@gmail.com>,
	Erico Nunes <nunes.erico@gmail.com>
Subject: Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"
Date: Thu, 24 May 2018 08:46:15 +0200	[thread overview]
Message-ID: <75f682e9-5b31-ad82-441f-b2c250edc0a0@amd.com> (raw)
In-Reply-To: <CAKGbVbv10zGLe34J1BXdKHg1Kg0=7A3jUy_8pnjVjNs1T=r8mw@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2696 bytes --]

Am 24.05.2018 um 03:38 schrieb Qiang Yu:
> [SNIP]
>>> Adding fence is done already, and I did wait it before unmap. But then
>>> I see when
>>> the buffer is shared between processes, the "perfect wait" is just
>>> wait the fence
>>> from this process's task, so it's better to also distinguish fences.
>>> If so, I just think
>>> why we don't just wait tasks from this process in the preclose before
>>> unmap/free
>>> buffer in the drm_release()?
>>
>> Well it depends on your VM management. When userspace expects that the VM
>> space the BO used is reusable immediately than the TTM callback won't work.
>>
>> On the other hand you can just grab the list of fences on a BO and filter
>> out the ones from your current process and wait for those. See
>> amdgpu_sync_resv() as an example how to do that.
> In current lima implementation, user space driver is responsible not unmap/free
> buffer before task is complete. And VM map/unmap is not differed.

Well it's up to you how to design userspace, but in the past doing it 
like that turned out to be a rather bad design decision.

Keep in mind that the kernel driver must guarantee that a shaders can 
never access freed up memory.

Otherwise taking over the system from an unprivileged processes becomes 
just a typing exercise when you manage to access freed memory which is 
now used for a page table.

Because of this we have a separate tracking in amdgpu so that we not 
only know who is using which BO, who is using which VM.

> This works simple and fine except the case that user press Ctrl+C to terminate
> the application which will force to close drm fd.

I'm not sure if that actually works as fine as you think.

For an example of what we had to add to prevent security breaches, take 
a look at amdgpu_gem_object_close().

> I'd more prefer to wait buffer fence before vm unmap and filter like
> amdgpu_sync_resv() compared to implement refcount in kernel task.
> But these two ways are all not as simple as preclose.

Well, I would rather say you should either delay VM unmap operations 
until all users of the VM are done with their work using the 
ttm_bo_destroy callback.

Or you block in the gem_close_object callback until all tasks using the 
BO are done with it.

> So I still don't understand why you don't want to get preclose back even
> have to introduce other complicated mechanism to cover the case free/unmap
> buffer before this process's task is done?

We intentionally removed the preclose callback to prevent certain use 
cases, bringing it back to allow your use case looks rather fishy to me.

BTW: What exactly is the issue with using the postclose callback?

Regards,
Christian.

>
> Regards,
> Qiang
>

[-- Attachment #1.2: Type: text/html, Size: 4056 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel