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

[Thomas Hellstrom]

On 05/24/2018 12:14 PM, Daniel Vetter wrote:
> On Thu, May 24, 2018 at 11:25:04AM +0200, Thomas Hellstrom wrote:
>> 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.
> This is definitely not an api for anything outside of drm. Just an attempt
> to consolidate kernel-internal drm access like fbdev, a simple bootsplash
> or an emergency console would need to do. Having some limitations in the
> initial versions, as long as we have some idea how to handle them, seems
> perfectly fine to me. This isn't meant to be a mandatory replacement for
> anything - no intentions of exporting this to userspace.
>

OK, yeah my concern is really for generic code and that there in the end 
would be too much code to change if we wanted to support this, but at 
least the generic code would be somewhat contained.

But it seems like we're at least in agreement on the problematic areas, 
and as long as they are open for change I guess we can live with that.

/Thomas