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

[Daniel Vetter <daniel@ffwll.ch>]

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.

And yes the pinning aspect is really ugly, but that's by far not the only
annoying aspect of trying to emulate fbdev on top of kms. defio handling
is another really problematic thing. I think we won't ever reach a 100%
solution on this. That's why I asked Noralf to rework his series to tie
into the existing ->fb_probe callback, since converting everyone over to
this isn't likely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx