[PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
Noralf Trønnes
noralf at tronnes.org
Fri Oct 25 11:47:32 UTC 2019
Den 25.10.2019 13.44, skrev Noralf Trønnes:
>
>
> Den 25.10.2019 12.12, skrev 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 had a flag to set this in the initial version of the shmem helper
> modeled after udl, but Thomas Hellstrom brought up a question and it was
> dropped. The issue was beyond my understanding:
>
> [PATCH v3 0/2] drm: Add shmem GEM library
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
>
> In tinydrm I have sped up uncached writes on arm by copying one pixel
s/writes/reads/
> line at a time into a temporary buffer before accessing the individual
> bytes. See drm_fb_swab16().
>
> Noralf.
>
>> (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
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list