[RFC] mesa: Export BOs in RW mode

Submitted by Steven Price on July 3, 2019, 2:13 p.m.

Details

Message ID 12e0bb1f-9a71-a120-7012-397fa0a16fd9@arm.com
State New
Headers show
Series "mesa: Export BOs in RW mode" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Steven Price July 3, 2019, 2:13 p.m.
On 03/07/2019 14:56, Boris Brezillon wrote:
> On Wed, 3 Jul 2019 07:45:32 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
> 
>> On Wed, Jul 3, 2019 at 7:34 AM Boris Brezillon
>> <boris.brezillon@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

 	shmem = to_drm_gem_shmem_obj(vma->vm_private_data);

Patch hide | download patch | download mbox

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);
+		return ret?:1;
+	}
+
 	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
 			       vma);

@@ -1196,6 +1201,16 @@  int drm_gem_mmap(struct file *filp, struct
vm_area_struct *vma)

 	return ret;
 }
+
+int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	int ret;
+
+	ret = drm_gem_mmap_helper(filp, vma);
+	if (ret == 1)
+		return 0;
+	return ret;
+}
 EXPORT_SYMBOL(drm_gem_mmap);

 void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 472ea5d81f82..b85d84e4d4a8 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -466,8 +466,10 @@  int drm_gem_shmem_mmap(struct file *filp, struct
vm_area_struct *vma)
 	struct drm_gem_shmem_object *shmem;
 	int ret;

-	ret = drm_gem_mmap(filp, vma);
-	if (ret)
+	ret = drm_gem_mmap_helper(filp, vma);
+	if (ret == 1)
+		return 0; /* Exporter handles the mapping */
+	else if (ret)
 		return ret;


Comments

On Wed, Jul 3, 2019 at 8:13 AM Steven Price <steven.price@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@kernel.org> wrote:
> >
> >> On Wed, Jul 3, 2019 at 7:34 AM Boris Brezillon
> >> <boris.brezillon@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 -> ...

Rob
On 03/07/2019 16:59, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 8:13 AM Steven Price <steven.price@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@kernel.org> wrote:
>>>
>>>> On Wed, Jul 3, 2019 at 7:34 AM Boris Brezillon
>>>> <boris.brezillon@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