[PATCH v2] drm/cma-helper: Set VM_DONTEXPAND for mmap

[PATCH v2] drm/cma-helper: Set VM_DONTEXPAND for mmap

[Alyssa Rosenzweig]

From: Robin Murphy <robin.murphy@arm.com>

drm_gem_cma_mmap() cannot assume every implementation of dma_mmap_wc()
will end up calling remap_pfn_range() (which happens to set the relevant
vma flag, among others), so in order to make sure expectations around
VM_DONTEXPAND are met, let it explicitly set the flag like most other
GEM mmap implementations do.

This avoids repeated warnings on a small minority of systems where the
display is behind an IOMMU, and has a simple driver which does not
override drm_gem_cma_default_funcs. Arm hdlcd is an in-tree affected
driver. Out-of-tree, the Apple DCP driver is affected; this fix is
required for DCP to be mainlined.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Reviewed-and-tested-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index d53388199f34..63e48d98263d 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -510,6 +510,7 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	 */
 	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 	vma->vm_flags &= ~VM_PFNMAP;
+	vma->vm_flags |= VM_DONTEXPAND;
 
 	cma_obj = to_drm_gem_cma_obj(obj);
 
-- 
2.30.2

Re: [PATCH v2] drm/cma-helper: Set VM_DONTEXPAND for mmap

[Daniel Vetter]

On Wed, Oct 13, 2021 at 10:36:54AM -0400, Alyssa Rosenzweig wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> 
> drm_gem_cma_mmap() cannot assume every implementation of dma_mmap_wc()
> will end up calling remap_pfn_range() (which happens to set the relevant
> vma flag, among others), so in order to make sure expectations around
> VM_DONTEXPAND are met, let it explicitly set the flag like most other
> GEM mmap implementations do.
> 
> This avoids repeated warnings on a small minority of systems where the
> display is behind an IOMMU, and has a simple driver which does not
> override drm_gem_cma_default_funcs. Arm hdlcd is an in-tree affected
> driver. Out-of-tree, the Apple DCP driver is affected; this fix is
> required for DCP to be mainlined.

How/where does this warn? Also there should be a lot more drivers than
just these two which have an iommu for the display block, so this not
working is definitely a more wide-spread issue.
-Daniel

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Reviewed-and-tested-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index d53388199f34..63e48d98263d 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -510,6 +510,7 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  	 */
>  	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  	vma->vm_flags &= ~VM_PFNMAP;
> +	vma->vm_flags |= VM_DONTEXPAND;
>  
>  	cma_obj = to_drm_gem_cma_obj(obj);
>  
> -- 
> 2.30.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] drm/cma-helper: Set VM_DONTEXPAND for mmap

[Alyssa Rosenzweig]

> > From: Robin Murphy <robin.murphy@arm.com>
> > 
> > drm_gem_cma_mmap() cannot assume every implementation of dma_mmap_wc()
> > will end up calling remap_pfn_range() (which happens to set the relevant
> > vma flag, among others), so in order to make sure expectations around
> > VM_DONTEXPAND are met, let it explicitly set the flag like most other
> > GEM mmap implementations do.
> > 
> > This avoids repeated warnings on a small minority of systems where the
> > display is behind an IOMMU, and has a simple driver which does not
> > override drm_gem_cma_default_funcs. Arm hdlcd is an in-tree affected
> > driver. Out-of-tree, the Apple DCP driver is affected; this fix is
> > required for DCP to be mainlined.
> 
> How/where does this warn? Also there should be a lot more drivers than
> just these two which have an iommu for the display block, so this not
> working is definitely a more wide-spread issue.

To summarize our discussion on IRC:

This fails `WARN_ON(!(vma->vm_flags & VM_DONTEXPAND))` in
drm_gem_mmap_obj. This warning was introduced in Oct 2019.

For a driver to hit this code path, it must use the CMA helpers without
overriding dem_gem_cma_default_funcs, but use CMA backed by a hardware
IOMMU instead of a physical carveout. This means popular drivers don't
hit this warning: normal drivers that use CMA do so with a carveout
instead of an IOMMU, and normal drivers with an IOMMU do not use the
default CMA helpers. hdlcd is one of the few drivers hitting this, but
hdlcd gets very little testing. Seeing as the last significant change to
hdlcd was in May 2019, it's believable that nobody noticed until Robin
hit this WARN and typed out this patch, especially as the driver still
works despite the WARN.

Re: [PATCH v2] drm/cma-helper: Set VM_DONTEXPAND for mmap

[Robin Murphy]

Hi Daniel,

On 2021-10-13 18:08, Daniel Vetter wrote:
> On Wed, Oct 13, 2021 at 10:36:54AM -0400, Alyssa Rosenzweig wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> drm_gem_cma_mmap() cannot assume every implementation of dma_mmap_wc()
>> will end up calling remap_pfn_range() (which happens to set the relevant
>> vma flag, among others), so in order to make sure expectations around
>> VM_DONTEXPAND are met, let it explicitly set the flag like most other
>> GEM mmap implementations do.
>>
>> This avoids repeated warnings on a small minority of systems where the
>> display is behind an IOMMU, and has a simple driver which does not
>> override drm_gem_cma_default_funcs. Arm hdlcd is an in-tree affected
>> driver. Out-of-tree, the Apple DCP driver is affected; this fix is
>> required for DCP to be mainlined.
> 
> How/where does this warn?

In drm_gem_mmap_obj().

> Also there should be a lot more drivers than
> just these two which have an iommu for the display block, so this not
> working is definitely a more wide-spread issue.

As the commit message implies, all those other drivers appear to end up 
using different mmap() implementations one way or another. Once I'd 
eventually figured it out, it didn't surprise me that the combination of 
a trivially-dumb display with an IOMMU is an oddball corner-case.

> -Daniel
> 
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> Reviewed-and-tested-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Alyssa - thanks for reviving this BTW, I'd forgotten all about it! - for 
future reference the clunky olde-worlde version of reassigning an MR to 
yourself is to add your sign-off to the end of the block (in addition to 
any review tags you may have previously given or choose to add) to note 
that you've chosen to take on responsibility for the patch[1]. FWIW I'm 
also partial to the practice of adding a little note in between if 
you've made any tweaks, e.g. "[alyssa: clarify affected drivers]", but 
that's often more of a personal choice.

Cheers,
Robin.

[1] 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

>> ---
>>   drivers/gpu/drm/drm_gem_cma_helper.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
>> index d53388199f34..63e48d98263d 100644
>> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> @@ -510,6 +510,7 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>   	 */
>>   	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>>   	vma->vm_flags &= ~VM_PFNMAP;
>> +	vma->vm_flags |= VM_DONTEXPAND;
>>   
>>   	cma_obj = to_drm_gem_cma_obj(obj);
>>   
>> -- 
>> 2.30.2
>>
>