[Mesa-dev] [RFC PATCH] mesa: Export BOs in RW mode

[Steven Price]

On 03/07/2019 16:59, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 8:13 AM Steven Price <steven.price at arm.com> wrote:
>>
>> On 03/07/2019 14:56, Boris Brezillon wrote:
>>> On Wed, 3 Jul 2019 07:45:32 -0600
>>> Rob Herring <robh+dt at kernel.org> wrote:
>>>
>>>> On Wed, Jul 3, 2019 at 7:34 AM Boris Brezillon
>>>> <boris.brezillon at collabora.com> wrote:
>>>>>
>>>>> Exported BOs might be imported back, then mmap()-ed to be written
>>>>> too. Most drivers handle that by mmap()-ing the GEM handle after it's
>>>>> been imported, but, according to [1], this is illegal.
>>>>
>>>> It's not illegal, but is supposed to go thru the dmabuf mmap
>>>> functions.
>>>
>>> That's basically what I'm proposing here, just didn't post the patch
>>> skipping the GET_OFFSET step and doing the mmap() on the dmabuf FD
>>> instead of the DRM-node one, but I have it working for panfrost.
>>
>> If we want to we could make the Panfrost kernel driver internally call
>> dma_buf_mmap() so that mapping using the DRM-node "just works". This is
>> indeed what the kbase driver does.
>>
>>>> However, none of the driver I've looked at (etnaviv, msm,
>>>> v3d, vgem) do that. It probably works because it's the same driver
>>>> doing the import and export or both drivers have essentially the same
>>>> implementations.
>>>
>>> Yes, but maybe that's something we should start fixing if mmap()-ing
>>> the dmabuf is the recommended solution.
>>
>> I'm open to options here. User space calling mmap() on the dma_buf file
>> descriptor should always be safe (the exporter can do whatever is
>> necessary to make it work). If that happens then the patches I posted
>> close off the DRM node version which could be broken if the exporter
>> needs to do anything to prepare the buffer for CPU access (i.e. cache
>> maintenance).
>>
>> Alternatively if user space wants/needs to use the DMA node then we can
>> take a look at what needs to change in the kernel. From a quick look at
>> the code it seems we'd need to split drm_gem_mmap() into a helper so
>> that it can return whether the exporter is handling everything or if the
>> caller needs to do some more work (e.g. drm_gem_shmem_mmap() needs to
>> allocate backing pages). But because drm_gem_mmap() is used as the
>> direct callback for some drivers we'd need to preserve the interface.
>>
>> The below (completely untested) patch demonstrates the idea.
>>
>> Steve
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index a8c4468f03d9..df661e24cadf 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1140,7 +1140,7 @@ EXPORT_SYMBOL(drm_gem_mmap_obj);
>>   * If the caller is not granted access to the buffer object, the mmap
>> will fail
>>   * with EACCES. Please see the vma manager for more information.
>>   */
>> -int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> +int drm_gem_mmap_helper(struct file *filp, struct vm_area_struct *vma)
>>  {
>>         struct drm_file *priv = filp->private_data;
>>         struct drm_device *dev = priv->minor->dev;
>> @@ -1189,6 +1189,11 @@ int drm_gem_mmap(struct file *filp, struct
>> vm_area_struct *vma)
>>                 vma->vm_flags &= ~VM_MAYWRITE;
>>         }
>>
>> +       if (obj->import_attach) {
>> +               ret = dma_buf_mmap(obj->dma_buf, vma, 0);
> 
> I don't see how calling dma_buf_mmap() would work. Looking at etnaviv
> which is the only GPU driver calling it, it looks to me like there a
> recursive loop:
> 
> driver->fops.mmap -> obj->mmap -> dma_buf_mmap -> .gem_prime_mmap ->
> etnaviv_gem_prime_mmap -> obj->mmap -> dma_buf_mmap -> ...

I did say it was untested :)

The call to dma_buf_mmap should only be in the drivers->fops.mmap
callback, not in obj->mmap. I have to admit I didn't actually bother to
look where drm_gem_mmap() was being used. So drm_gem_shmem_mmap() needs
to call a function which does do the dma_buf_mmap(), but the callback in
obj->mmap shouldn't.

There's a reason I initially went for simply preventing user space
mmap()ing imported objects (through the DRM node) rather than trying to
make it work ;)

Steve