[PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()

[Daniel Vetter]

On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann at suse.de> wrote
>
> Hi
>
> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
> > On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
> >> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
> >> required by generic fbdev emulation. Supporting free() is easy as
> >> well. More udl-specific interfaces can probably be replaced by GEM
> >> object functions in later patches.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> >> ---
> >>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> >> index 3ea0cd9ae2d6..6ca097c270d6 100644
> >> --- a/drivers/gpu/drm/udl/udl_gem.c
> >> +++ b/drivers/gpu/drm/udl/udl_gem.c
> >> @@ -11,6 +11,8 @@
> >>
> >>  #include "udl_drv.h"
> >>
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
> >> +
> >>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>                                          size_t size)
> >>  {
> >> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>              return NULL;
> >>      }
> >>
> >> +    obj->base.funcs = &udl_gem_object_funcs;
> >>      obj->flags = UDL_BO_CACHEABLE;
> >>      return obj;
> >>  }
> >> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
> >>      mutex_unlock(&udl->gem_lock);
> >>      return ret;
> >>  }
> >> +
> >> +/*
> >> + * Helpers for struct drm_gem_object_funcs
> >> + */
> >> +
> >> +static void udl_gem_object_free(struct drm_gem_object *obj)
> >> +{
> >> +    udl_gem_free_object(obj);
> >> +}
> >> +
> >> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
> >> +{
> >> +    struct udl_gem_object *uobj = to_udl_bo(obj);
> >> +    int ret;
> >> +
> >> +    ret = udl_gem_vmap(uobj);
> >> +    if (ret)
> >> +            return ERR_PTR(ret);
> >> +    return uobj->vmapping;
> >> +}
> >> +
> >> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
> >> +{
> >> +    return udl_gem_vunmap(to_udl_bo(obj));
> >> +}
> >> +
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
> >> +    .free   = udl_gem_object_free,
> >> +    .vmap   = udl_gem_object_vmap,
> >> +    .vunmap = udl_gem_object_vunmap,
> >
> > Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>
> I see. Should have thought of that...
>
> > say simpler to first cut over to shmem helpers.
>
> Right. I was already attempting the conversion and the udl gem is more
> or less the same as shmem, except for the flags field. [1] The flag
> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
> set to writecombining, and for local BOs it is set to cached. Shmem
> always maps with writecombining.
>
> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
> kind of optimization. Do you have an idea?
>
> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57

udl doesn't set prefer_shadow = 1, which means compositors will render
directly into the buffer. And for that you want caching. For imported
dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
udl would need to get the dma_buf_begin/end_cpu_access calls added
(and that would probably be faster than going with wc for everything).
So we do want cachable, it's going to suck otherwise.

But that should be fairly easy to do by overwriting the obj->mmap
callback and wrapping it around the shmem one.

What surprises me more is that udl supports mmap on imported dma-buf,
that's some real quirky stuff. Not sure there's really other drivers
doing that. Might be easier to rip that out :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch