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

[Thomas Zimmermann]

Hi

Am 25.10.19 um 11:28 schrieb 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.

Thanks for clarifying. Actually, it does have these calls in its dirty
handler. [1]

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

Is there a reason why shmem doesn't implement the wc-vs.cached logic?
Could this be added?

(I'm just trying to understand the available options here before
attempting to do a conversion.)

Best regards
Thomas

> 
> 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
> 

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191025/5201c86a/attachment-0001.sig>