[PATCH 4/9] drm: Begin an API for in-kernel clients
thomas at shipmail.org
Thu May 24 09:25:04 UTC 2018
On 05/24/2018 10:32 AM, Daniel Vetter wrote:
> On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote:
>> Hi, Noralf.
>> A couple of issues below:
>> On 05/23/2018 04:34 PM, Noralf Trønnes wrote:
>>> This the beginning of an API for in-kernel clients.
>>> First out is a way to get a framebuffer backed by a dumb buffer.
>>> Only GEM drivers are supported.
>>> The original idea of using an exported dma-buf was dropped because it
>>> also creates an anonomous file descriptor which doesn't work when the
>>> buffer is created from a kernel thread. The easy way out is to use
>>> drm_driver.gem_prime_vmap to get the virtual address, which requires a
>>> GEM object. This excludes the vmwgfx driver which is the only non-GEM
>>> driver apart from the legacy ones. A solution for vmwgfx will have to be
>>> worked out later if it wants to support the client API which it probably
>>> will when we have a bootsplash client.
>> Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel
>> use rather than using GEM directly?
>> But the main issue is pinning. It looks like the buffers are going to be
>> vmapped() for a long time, which requires pinning, and that doesn't work for
>> some drivers when they bind the framebuffer to a plane, since that might
>> require pinning in another memory region and the vmap would have to be torn
>> down. Besides, buffer pinning should really be avoided if possible:
>> Since we can't page-fault vmaps, and setting up / tearing down vmaps is
>> potentially an expensive operation, could we perhaps have a mapping api that
>> allows the driver to cache vmaps?
>> vmap() // Indicates that we want to map a bo
>> begin_access() // Returns a virtual address which may vary between calls.
>> Allows access. A fast operation. Behind the lines pins / reserves the bo and
>> returns a cached vmap if the bo didn't move since last begin_access(), which
>> is the typical case.
>> end_access() // Disable access. Unpins / unreserves the bo.
>> vunmap_cached() //Indicates that the map is no longer needed. The driver can
>> release the cached map.
>> The idea is that the API client would wrap all bo map accesses with
>> begin_access() end_access(), allowing for the bo to be moved in between.
> So originally my ideas for the cpu side dma-buf interfaces where all meant
> to handle this. But then the first implementations bothered with none of
> this, but instead expected that stuff is pinned, and vmap Just Works.
> Which yeah doesn't work for vmwgfx and is a pain in a few other cases.
> I agree it'd be nice to fix all this, but it's also not a problem that
> this patch set here started. And since it's all optional (and vmwgfx isn't
> even using the current fb helper code) I think it's reasonable to address
> this post-merge (if someone gets around to it ever). What we'd need is is
> a fallback for when vmap doesn't exist (for fbdev that probably means a
> vmalloc'ed buffer + manual uploads, because fbdev), plus making sure
> dma-buf implementations actually implement it.
My argument here is that, If I understand Noralf, this is intended to be
an API exported outside of drm. In that case we shouldn't replicate the
assumed behaviour of incomplete dma-buf implementations in a new API.
Also the fact that vmwgfx currently isn't using the fbdev helpers isn't
a good argument to design an API so that vmwgfx can _never_ use the
fbdev helpers. The reason we aren't using them is that the kms
implementation was so old that we didn't implement the necessary helper
Also, I might be misunderstanding the code a bit, but I doubt that
vmwgfx is the only hardware with pinning restrictions on the
framebuffer? I was under the assumption that most discrete hardware
required the framebuffer to be pinned in VRAM?
So the important question is, Is this a set of helpers for shared-memory
GEM drivers to implement fbdev? Then I wouldn't bother, If it's intended
to become an API for clients outside of DRM, then I would have to insist
on the API being changed to reflect that.
More information about the dri-devel