[Intel-gfx] [PATCH 4/9] drm: Begin an API for in-kernel clients

[Thomas Hellstrom]

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 
callbacks...

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.

/Thomas