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

[Rob Herring]

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

Rob