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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
