[40/59] drm/vram-helper: Drop drm_gem_prime_export/import
diff mbox series

Message ID 20190614203615.12639-41-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • prime doc polish and ... a few cleanups
Related show

Commit Message

Daniel Vetter June 14, 2019, 8:35 p.m. UTC
They're the default.

Aside: Would be really nice to switch the others over to
drm_gem_object_funcs.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_vram_helper.h | 2 --
 1 file changed, 2 deletions(-)

Comments

Gerd Hoffmann June 17, 2019, 6:03 a.m. UTC | #1
On Fri, Jun 14, 2019 at 10:35:56PM +0200, Daniel Vetter wrote:
> They're the default.
> 
> Aside: Would be really nice to switch the others over to
> drm_gem_object_funcs.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Gerd Hoffmann June 17, 2019, 8:24 a.m. UTC | #2
Hi,

> Aside: Would be really nice to switch the others over to
> drm_gem_object_funcs.

While most callbacks are pretty straight forward (just hook the same
callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
bit more obscure.

First, there seem to be two ways to mmap a gem buffer:

  (1) drm_driver->fops->mmap, and
  (2) drm_driver->gem_prime_mmap.

drm_gem_object_funcs has just a single vm_ops ...

Also it is not obvious how one would convert something which basically
calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.

thanks,
  Gerd
Daniel Vetter June 17, 2019, 1:59 p.m. UTC | #3
On Mon, Jun 17, 2019 at 10:24:38AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Aside: Would be really nice to switch the others over to
> > drm_gem_object_funcs.
> 
> While most callbacks are pretty straight forward (just hook the same
> callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
> bit more obscure.
> 
> First, there seem to be two ways to mmap a gem buffer:
> 
>   (1) drm_driver->fops->mmap, and
>   (2) drm_driver->gem_prime_mmap.
> 
> drm_gem_object_funcs has just a single vm_ops ...
> 
> Also it is not obvious how one would convert something which basically
> calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.

Yeah the mmap side is still a mess, but my series here was getting a bit
too long already. There's a bunch of problems here:

drm_driver->gem_prime_mmap could be nuked and instead we use
drm_gem_prime_mmap everywhere. Especially the various versions in helpers
really don't add much.

The trouble is that removing the hook outright isn't quite right, because
it also signals "is mmap allowed on this dma-buf". I'm kinda tempted to
just make that a hard requirement, and force people who can't support mmap
on the dma-buf (or who need begin/end_cpu_access hooks) to supply their
own set of dma_buf_ops.

Second one is drm_driver->fops->mmap. That one we need to keep, but this
isn't mmap on a buffer, but mmap on the entire drm_device. The one which
should be replaced by drm_gem_object_funcs.vm_ops is
drm_driver->gem_vm_ops.

Hope that explains a bit better what's going on here. Step one here for
mmap is definitely to roll out drm_gem_prime_mmap as far as possible, so
it's easier to understand where the exceptions are.

Cheers, Daniel
Gerd Hoffmann June 18, 2019, 4:49 a.m. UTC | #4
Hi,

> > While most callbacks are pretty straight forward (just hook the same
> > callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
> > bit more obscure.
> > 
> > First, there seem to be two ways to mmap a gem buffer:
> > 
> >   (1) drm_driver->fops->mmap, and
> >   (2) drm_driver->gem_prime_mmap.
> > 
> > drm_gem_object_funcs has just a single vm_ops ...
> > 
> > Also it is not obvious how one would convert something which basically
> > calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.
> 
> Yeah the mmap side is still a mess, but my series here was getting a bit
> too long already. There's a bunch of problems here:
> 
> drm_driver->gem_prime_mmap could be nuked and instead we use
> drm_gem_prime_mmap everywhere. Especially the various versions in helpers
> really don't add much.

Well, everything using ttm has the problem that we have another
duplication here: both gem and ttm have a vma_node ...

So (for example) drm_gem_vram_driver_gem_prime_mmap() is a thin wrapper
which does (a) copy vm_node.start from ttm to gem vma_node and (b) calls
drm_gem_prime_mmap().

> Second one is drm_driver->fops->mmap. That one we need to keep, but this
> isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> should be replaced by drm_gem_object_funcs.vm_ops is
> drm_driver->gem_vm_ops.

Hmm, seems ttm hasn't something I can hook into drm_driver->gem_vm_ops ...

cheers,
  Gerd
Daniel Vetter June 18, 2019, 7:59 a.m. UTC | #5
On Tue, Jun 18, 2019 at 6:49 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > While most callbacks are pretty straight forward (just hook the same
> > > callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
> > > bit more obscure.
> > >
> > > First, there seem to be two ways to mmap a gem buffer:
> > >
> > >   (1) drm_driver->fops->mmap, and
> > >   (2) drm_driver->gem_prime_mmap.
> > >
> > > drm_gem_object_funcs has just a single vm_ops ...
> > >
> > > Also it is not obvious how one would convert something which basically
> > > calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.
> >
> > Yeah the mmap side is still a mess, but my series here was getting a bit
> > too long already. There's a bunch of problems here:
> >
> > drm_driver->gem_prime_mmap could be nuked and instead we use
> > drm_gem_prime_mmap everywhere. Especially the various versions in helpers
> > really don't add much.
>
> Well, everything using ttm has the problem that we have another
> duplication here: both gem and ttm have a vma_node ...
>
> So (for example) drm_gem_vram_driver_gem_prime_mmap() is a thin wrapper
> which does (a) copy vm_node.start from ttm to gem vma_node and (b) calls
> drm_gem_prime_mmap().

Hm ... maybe we should ditch the ttm vma offset stuff and fold that
over entirely to the gem way of doing things. The only thing you're
going to loose is the ->verify_access callback, which again is just to
get back to gem I think. You would need a slightly differrent vm_ops
structure though, since the ttm vm ops expect a ttm_buffer_object,
whereas gem gives you a drm_gem_buffer_object. So either need to
overwrite all those, or maybe it's inded time to just make ttm_bo a
subclass of gem_bo.

> > Second one is drm_driver->fops->mmap. That one we need to keep, but this
> > isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> > should be replaced by drm_gem_object_funcs.vm_ops is
> > drm_driver->gem_vm_ops.
>
> Hmm, seems ttm hasn't something I can hook into drm_driver->gem_vm_ops ...

ttm_bo_vm_ops seems to be the thing you want.

Cheers, Daniel
Gerd Hoffmann June 19, 2019, 11:21 a.m. UTC | #6
Hi,

> > > Second one is drm_driver->fops->mmap. That one we need to keep, but this
> > > isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> > > should be replaced by drm_gem_object_funcs.vm_ops is
> > > drm_driver->gem_vm_ops.
> >
> > Hmm, seems ttm hasn't something I can hook into drm_driver->gem_vm_ops ...
> 
> ttm_bo_vm_ops seems to be the thing you want.

Wouldn't work as-is, but when ttm bo are a subclass of gem bos should
be possible to create something usable based on it.

Related question: why there is no drm_gem_object_funcs.mmap() callback?
I think it would make sense to have a callback where the bo-specific
setup can be done, i.e. what ttm_bo_mmap() or drm_gem_shmem_mmap() are
doing, and have some generic function which basically does the lookup,
then dispatches.

cheers,
  Gerd
Daniel Vetter June 19, 2019, 11:31 a.m. UTC | #7
On Wed, Jun 19, 2019 at 1:21 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > Second one is drm_driver->fops->mmap. That one we need to keep, but this
> > > > isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> > > > should be replaced by drm_gem_object_funcs.vm_ops is
> > > > drm_driver->gem_vm_ops.
> > >
> > > Hmm, seems ttm hasn't something I can hook into drm_driver->gem_vm_ops ...
> >
> > ttm_bo_vm_ops seems to be the thing you want.
>
> Wouldn't work as-is, but when ttm bo are a subclass of gem bos should
> be possible to create something usable based on it.

You'd need to create driver-specific wrappers, but that's somewhat
defeating the point.

> Related question: why there is no drm_gem_object_funcs.mmap() callback?
> I think it would make sense to have a callback where the bo-specific
> setup can be done, i.e. what ttm_bo_mmap() or drm_gem_shmem_mmap() are
> doing, and have some generic function which basically does the lookup,
> then dispatches.

Maybe. Atm all we have around mmap is a sprawling number of different
solutions. Atm I'm not really sure which direction we really should
head into ...
-Daniel
Thomas Zimmermann June 27, 2019, 8:27 a.m. UTC | #8
Hi

Am 17.06.19 um 15:59 schrieb Daniel Vetter:
> On Mon, Jun 17, 2019 at 10:24:38AM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> Aside: Would be really nice to switch the others over to
>>> drm_gem_object_funcs.
>>
>> While most callbacks are pretty straight forward (just hook the same
>> callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
>> bit more obscure.
>>
>> First, there seem to be two ways to mmap a gem buffer:
>>
>>   (1) drm_driver->fops->mmap, and
>>   (2) drm_driver->gem_prime_mmap.
>>
>> drm_gem_object_funcs has just a single vm_ops ...
>>
>> Also it is not obvious how one would convert something which basically
>> calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.
> 
> Yeah the mmap side is still a mess, but my series here was getting a bit
> too long already. There's a bunch of problems here:
> 
> drm_driver->gem_prime_mmap could be nuked and instead we use
> drm_gem_prime_mmap everywhere. Especially the various versions in helpers
> really don't add much.
> 
> The trouble is that removing the hook outright isn't quite right, because
> it also signals "is mmap allowed on this dma-buf". I'm kinda tempted to
> just make that a hard requirement, and force people who can't support mmap
> on the dma-buf (or who need begin/end_cpu_access hooks) to supply their
> own set of dma_buf_ops.
> 
> Second one is drm_driver->fops->mmap. That one we need to keep, but this
> isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> should be replaced by drm_gem_object_funcs.vm_ops is
> drm_driver->gem_vm_ops.

From what I've seen in fbdev drivers, it's an mmap of the framebuffer
memory, which typically is the same as the device's memory but doesn't
have to. And it's only valid for/while the current display mode (e.g.,
mgafb doesn't set fixes.smem_length until you configure a mode).

I guess it should be legal to just mmap the shadow FB from the fbcon
emulation.

Best regards
Thomas

> Hope that explains a bit better what's going on here. Step one here for
> mmap is definitely to roll out drm_gem_prime_mmap as far as possible, so
> it's easier to understand where the exceptions are.
> 
> Cheers, Daniel
>
Daniel Vetter June 27, 2019, 9:59 a.m. UTC | #9
On Thu, Jun 27, 2019 at 10:27 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 17.06.19 um 15:59 schrieb Daniel Vetter:
> > On Mon, Jun 17, 2019 at 10:24:38AM +0200, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> Aside: Would be really nice to switch the others over to
> >>> drm_gem_object_funcs.
> >>
> >> While most callbacks are pretty straight forward (just hook the same
> >> callbacks into the drm_gem_object_funcs. struct) the mmap bits are a
> >> bit more obscure.
> >>
> >> First, there seem to be two ways to mmap a gem buffer:
> >>
> >>   (1) drm_driver->fops->mmap, and
> >>   (2) drm_driver->gem_prime_mmap.
> >>
> >> drm_gem_object_funcs has just a single vm_ops ...
> >>
> >> Also it is not obvious how one would convert something which basically
> >> calls ttm_bo_mmap() in drm_driver->fops->mmap to the new interface.
> >
> > Yeah the mmap side is still a mess, but my series here was getting a bit
> > too long already. There's a bunch of problems here:
> >
> > drm_driver->gem_prime_mmap could be nuked and instead we use
> > drm_gem_prime_mmap everywhere. Especially the various versions in helpers
> > really don't add much.
> >
> > The trouble is that removing the hook outright isn't quite right, because
> > it also signals "is mmap allowed on this dma-buf". I'm kinda tempted to
> > just make that a hard requirement, and force people who can't support mmap
> > on the dma-buf (or who need begin/end_cpu_access hooks) to supply their
> > own set of dma_buf_ops.
> >
> > Second one is drm_driver->fops->mmap. That one we need to keep, but this
> > isn't mmap on a buffer, but mmap on the entire drm_device. The one which
> > should be replaced by drm_gem_object_funcs.vm_ops is
> > drm_driver->gem_vm_ops.
>
> From what I've seen in fbdev drivers, it's an mmap of the framebuffer
> memory, which typically is the same as the device's memory but doesn't
> have to. And it's only valid for/while the current display mode (e.g.,
> mgafb doesn't set fixes.smem_length until you configure a mode).
>
> I guess it should be legal to just mmap the shadow FB from the fbcon
> emulation.

fbdev mmap is an entirely different beast ... but yeah maybe we should
rework that one too. Problem there is that it intereacts badly with
defio, so for at least some drivers you can't do the straightforward
thing at all.
-Daniel

>
> Best regards
> Thomas
>
> > Hope that explains a bit better what's going on here. Step one here for
> > mmap is definitely to roll out drm_gem_prime_mmap as far as possible, so
> > it's easier to understand where the exceptions are.
> >
> > Cheers, Daniel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imend├Ârffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG N├╝rnberg)
>

Patch
diff mbox series

diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 9581ea0a4f7e..1a0ea18e7a74 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -142,8 +142,6 @@  int drm_gem_vram_driver_gem_prime_mmap(struct drm_gem_object *obj,
 				       struct vm_area_struct *vma);
 
 #define DRM_GEM_VRAM_DRIVER_PRIME \
-	.gem_prime_export = drm_gem_prime_export, \
-	.gem_prime_import = drm_gem_prime_import, \
 	.gem_prime_pin	  = drm_gem_vram_driver_gem_prime_pin, \
 	.gem_prime_unpin  = drm_gem_vram_driver_gem_prime_unpin, \
 	.gem_prime_vmap	  = drm_gem_vram_driver_gem_prime_vmap, \