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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 25.10.2019 14.20, skrev Thomas Zimmermann:
> (cc: Gerd)
> 
> Hi
> 
> Am 25.10.19 um 13:44 schrieb 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@xxxxxxx> 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@xxxxxxx>
>>>>>>> ---
>>>>>>>  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
> 
> If I understand that discussion correctly, the concern was that write
> combining and shared memory would not work well together. So you went
> with always-cached?
> 
> Just recently, Gerd added unconditional write combining in rev 0be8958936.
> 

I don't remember, Rob picked up the patch since I couldn't use shmem
after all for tinydrm.

I see in the commit message that he made this change:

v7:
- Use write-combine for mmap instead. This is the more common
  case. (robher)

Then we have this:
drm/gem_shmem: Use a writecombine mapping for ->vaddr (be7d9f05c53e)

So it looks like my initial work has been fixed up piece by piece to use
writecombine.

Noralf.

> Best regards
> Thomas
> 
>>
>> In tinydrm I have sped up uncached writes on arm by copying one pixel
>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux