Re: [PATCH 4/8] drm/ttm: factor out ttm_bo_mmap_vma_setup

[Gerd Hoffmann]

On Fri, Sep 13, 2019 at 02:56:09PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 13.09.19 um 14:29 schrieb Gerd Hoffmann:
> > Factor out ttm vma setup to a new function.  Reduces
> > code duplication a bit and allows to implement
> > &drm_gem_object_funcs.mmap in gem ttm helpers.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > ---
> >  include/drm/ttm/ttm_bo_api.h    |  8 ++++++
> >  drivers/gpu/drm/ttm/ttm_bo_vm.c | 47 ++++++++++++++++++---------------
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> > index 43c4929a2171..88c652f49602 100644
> > --- a/include/drm/ttm/ttm_bo_api.h
> > +++ b/include/drm/ttm/ttm_bo_api.h
> > @@ -734,6 +734,14 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo);
> >  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  		struct ttm_bo_device *bdev);
> >  
> > +/**
> > + * ttm_bo_mmap_vma_setup - initialize vma for ttm bo mmap
> > + *
> > + * @bo: The buffer object.
> > + * @vma: vma as input from the mmap method.
> > + */
> > +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma);
> > +
> >  void *ttm_kmap_atomic_prot(struct page *page, pgprot_t prot);
> >  
> >  void ttm_kunmap_atomic_prot(void *addr, pgprot_t prot);
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > index 4aa007edffb0..7c0e85c10e0e 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > @@ -426,6 +426,29 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
> >  	return bo;
> >  }
> >  
> > +void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
> > +{
> > +	vma->vm_ops = &ttm_bo_vm_ops;
> > +
> > +	/*
> > +	 * Note: We're transferring the bo reference to
> > +	 * vma->vm_private_data here.
> > +	 */
> > +
> > +	vma->vm_private_data = bo;
> > +
> > +	/*
> > +	 * We'd like to use VM_PFNMAP on shared mappings, where
> > +	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> > +	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> > +	 * bad for performance. Until that has been sorted out, use
> > +	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> > +	 */
> > +	vma->vm_flags |= VM_MIXEDMAP;
> > +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_mmap_vma_setup);
> > +
> >  int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  		struct ttm_bo_device *bdev)
> >  {
> > @@ -449,24 +472,7 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> >  	if (unlikely(ret != 0))
> >  		goto out_unref;
> >  
> > -	vma->vm_ops = &ttm_bo_vm_ops;
> > -
> > -	/*
> > -	 * Note: We're transferring the bo reference to
> > -	 * vma->vm_private_data here.
> > -	 */
> > -
> > -	vma->vm_private_data = bo;
> > -
> > -	/*
> > -	 * We'd like to use VM_PFNMAP on shared mappings, where
> > -	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
> > -	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
> > -	 * bad for performance. Until that has been sorted out, use
> > -	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
> > -	 */
> > -	vma->vm_flags |= VM_MIXEDMAP;
> > -	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> > +	ttm_bo_mmap_vma_setup(bo, vma);
> >  	return 0;
> >  out_unref:
> >  	ttm_bo_put(bo);
> > @@ -481,10 +487,7 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma, struct ttm_buffer_object *bo)
> >  
> >  	ttm_bo_get(bo);
> >  
> > -	vma->vm_ops = &ttm_bo_vm_ops;
> > -	vma->vm_private_data = bo;
> > -	vma->vm_flags |= VM_MIXEDMAP;
> > -	vma->vm_flags |= VM_IO | VM_DONTEXPAND;
> > +	ttm_bo_mmap_vma_setup(bo, vma);
> Just double-checking:  ttm_bo_mmap_vma_setup() will set VM_DONTDUMP in
> vm_flags. Is that OK?

Should be ok, according to daniel vetter this most likely dates back to
the days where drivers exposed hardware registers mmap()able object.

But doing this as separate patch is probably a good idea ...

cheers,
  Gerd