diff mbox

drm/fb_cma_helper: Implement fb_mmap callback

Message ID 8fd95ac1440e0f01daad6d4380be3a4c8fa61055.1465301219.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Robin Murphy June 7, 2016, 12:18 p.m. UTC 
In the absence of an fb_mmap callback, the fbdev code falls back to a
naive implementation which relies upon the DMA address being the same
as the physical address, and the buffer being physically contiguous
from there. Whilst this often holds for standard CMA allocations via
the platform's regular DMA ops, if the allocation is provided by an
IOMMU then such assumptions can fall apart spectacularly.

To resolve this, reroute the fb_mmap call to the appropriate DMA API
implementation, as per the other cma_helper calls.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
some concerns about the general way fb_cma_helper uses fb_info[1], but
AFAICS that's a longstanding separate problem orthogonal to this patch.

Robin.

[1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288

 drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Daniel Vetter June 7, 2016, 2:43 p.m. UTC | #1 
On Tue, Jun 07, 2016 at 01:18:09PM +0100, Robin Murphy wrote:
> In the absence of an fb_mmap callback, the fbdev code falls back to a
> naive implementation which relies upon the DMA address being the same
> as the physical address, and the buffer being physically contiguous
> from there. Whilst this often holds for standard CMA allocations via
> the platform's regular DMA ops, if the allocation is provided by an
> IOMMU then such assumptions can fall apart spectacularly.
> 
> To resolve this, reroute the fb_mmap call to the appropriate DMA API
> implementation, as per the other cma_helper calls.
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
> some concerns about the general way fb_cma_helper uses fb_info[1], but
> AFAICS that's a longstanding separate problem orthogonal to this patch.

Do you want me to pull this in through drm-misc, or will this land through
some driver tree? Note that I'll do -misc pulls about every week, so if
you don't send your pull this week I'd like to pull it in to avoid
hilarity^W needless conflicts.
-Daniel

> 
> Robin.
> 
> [1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288
> 
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 172cafe11c71..a25afc068d3f 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  
>  #define DEFAULT_FBDEFIO_DELAY_MS 50
> @@ -297,6 +298,12 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
>  EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
>  #endif
>  
> +static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +{
> +	return dma_mmap_writecombine(info->device, vma, info->screen_base,
> +				     info->fix.smem_start, info->fix.smem_len);
> +}
> +
>  static struct fb_ops drm_fbdev_cma_ops = {
>  	.owner		= THIS_MODULE,
>  	.fb_fillrect	= drm_fb_helper_sys_fillrect,
> @@ -307,6 +314,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
>  	.fb_blank	= drm_fb_helper_blank,
>  	.fb_pan_display	= drm_fb_helper_pan_display,
>  	.fb_setcmap	= drm_fb_helper_setcmap,
> +	.fb_mmap	= drm_fb_cma_mmap,
>  };
>  
>  static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> -- 
> 2.8.1.dirty
>
Liviu Dudau June 7, 2016, 2:51 p.m. UTC | #2 
On Tue, Jun 07, 2016 at 04:43:05PM +0200, Daniel Vetter wrote:
> On Tue, Jun 07, 2016 at 01:18:09PM +0100, Robin Murphy wrote:
> > In the absence of an fb_mmap callback, the fbdev code falls back to a
> > naive implementation which relies upon the DMA address being the same
> > as the physical address, and the buffer being physically contiguous
> > from there. Whilst this often holds for standard CMA allocations via
> > the platform's regular DMA ops, if the allocation is provided by an
> > IOMMU then such assumptions can fall apart spectacularly.
> > 
> > To resolve this, reroute the fb_mmap call to the appropriate DMA API
> > implementation, as per the other cma_helper calls.
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 
> > Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
> > some concerns about the general way fb_cma_helper uses fb_info[1], but
> > AFAICS that's a longstanding separate problem orthogonal to this patch.
> 
> Do you want me to pull this in through drm-misc, or will this land through
> some driver tree? Note that I'll do -misc pulls about every week, so if
> you don't send your pull this week I'd like to pull it in to avoid
> hilarity^W needless conflicts.
> -Daniel

Hi Daniel,

This patch is for drm core, so no driver can carry it (?).

Please pull it into your drm-misc.

Best regards,
Liviu

> 
> > 
> > Robin.
> > 
> > [1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288
> > 
> >  drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 172cafe11c71..a25afc068d3f 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -23,6 +23,7 @@
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_gem_cma_helper.h>
> >  #include <drm/drm_fb_cma_helper.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/module.h>
> >  
> >  #define DEFAULT_FBDEFIO_DELAY_MS 50
> > @@ -297,6 +298,12 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
> >  EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
> >  #endif
> >  
> > +static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > +{
> > +	return dma_mmap_writecombine(info->device, vma, info->screen_base,
> > +				     info->fix.smem_start, info->fix.smem_len);
> > +}
> > +
> >  static struct fb_ops drm_fbdev_cma_ops = {
> >  	.owner		= THIS_MODULE,
> >  	.fb_fillrect	= drm_fb_helper_sys_fillrect,
> > @@ -307,6 +314,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
> >  	.fb_blank	= drm_fb_helper_blank,
> >  	.fb_pan_display	= drm_fb_helper_pan_display,
> >  	.fb_setcmap	= drm_fb_helper_setcmap,
> > +	.fb_mmap	= drm_fb_cma_mmap,
> >  };
> >  
> >  static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> > -- 
> > 2.8.1.dirty
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
Robin Murphy June 7, 2016, 3:03 p.m. UTC | #3 
On 07/06/16 15:43, Daniel Vetter wrote:
> On Tue, Jun 07, 2016 at 01:18:09PM +0100, Robin Murphy wrote:
>> In the absence of an fb_mmap callback, the fbdev code falls back to a
>> naive implementation which relies upon the DMA address being the same
>> as the physical address, and the buffer being physically contiguous
>> from there. Whilst this often holds for standard CMA allocations via
>> the platform's regular DMA ops, if the allocation is provided by an
>> IOMMU then such assumptions can fall apart spectacularly.
>>
>> To resolve this, reroute the fb_mmap call to the appropriate DMA API
>> implementation, as per the other cma_helper calls.
>>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
>> some concerns about the general way fb_cma_helper uses fb_info[1], but
>> AFAICS that's a longstanding separate problem orthogonal to this patch.
>
> Do you want me to pull this in through drm-misc, or will this land through
> some driver tree? Note that I'll do -misc pulls about every week, so if
> you don't send your pull this week I'd like to pull it in to avoid
> hilarity^W needless conflicts.

Yeah, drm-misc sounds appropriate so if you're happy to pick it up, 
please feel free :)

Robin.

> -Daniel
>
>>
>> Robin.
>>
>> [1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288
>>
>>   drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 172cafe11c71..a25afc068d3f 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -23,6 +23,7 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_gem_cma_helper.h>
>>   #include <drm/drm_fb_cma_helper.h>
>> +#include <linux/dma-mapping.h>
>>   #include <linux/module.h>
>>
>>   #define DEFAULT_FBDEFIO_DELAY_MS 50
>> @@ -297,6 +298,12 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
>>   EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
>>   #endif
>>
>> +static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> +{
>> +	return dma_mmap_writecombine(info->device, vma, info->screen_base,
>> +				     info->fix.smem_start, info->fix.smem_len);
>> +}
>> +
>>   static struct fb_ops drm_fbdev_cma_ops = {
>>   	.owner		= THIS_MODULE,
>>   	.fb_fillrect	= drm_fb_helper_sys_fillrect,
>> @@ -307,6 +314,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
>>   	.fb_blank	= drm_fb_helper_blank,
>>   	.fb_pan_display	= drm_fb_helper_pan_display,
>>   	.fb_setcmap	= drm_fb_helper_setcmap,
>> +	.fb_mmap	= drm_fb_cma_mmap,
>>   };
>>
>>   static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
>> --
>> 2.8.1.dirty
>>
>
Daniel Vetter June 7, 2016, 7:43 p.m. UTC | #4 
On Tue, Jun 07, 2016 at 04:03:40PM +0100, Robin Murphy wrote:
> On 07/06/16 15:43, Daniel Vetter wrote:
> > On Tue, Jun 07, 2016 at 01:18:09PM +0100, Robin Murphy wrote:
> > > In the absence of an fb_mmap callback, the fbdev code falls back to a
> > > naive implementation which relies upon the DMA address being the same
> > > as the physical address, and the buffer being physically contiguous
> > > from there. Whilst this often holds for standard CMA allocations via
> > > the platform's regular DMA ops, if the allocation is provided by an
> > > IOMMU then such assumptions can fall apart spectacularly.
> > > 
> > > To resolve this, reroute the fb_mmap call to the appropriate DMA API
> > > implementation, as per the other cma_helper calls.
> > > 
> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > > 
> > > Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
> > > some concerns about the general way fb_cma_helper uses fb_info[1], but
> > > AFAICS that's a longstanding separate problem orthogonal to this patch.
> > 
> > Do you want me to pull this in through drm-misc, or will this land through
> > some driver tree? Note that I'll do -misc pulls about every week, so if
> > you don't send your pull this week I'd like to pull it in to avoid
> > hilarity^W needless conflicts.
> 
> Yeah, drm-misc sounds appropriate so if you're happy to pick it up, please
> feel free :)

Applied, thanks.
-Daniel

> 
> Robin.
> 
> > -Daniel
> > 
> > > 
> > > Robin.
> > > 
> > > [1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288
> > > 
> > >   drivers/gpu/drm/drm_fb_cma_helper.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index 172cafe11c71..a25afc068d3f 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -23,6 +23,7 @@
> > >   #include <drm/drm_crtc_helper.h>
> > >   #include <drm/drm_gem_cma_helper.h>
> > >   #include <drm/drm_fb_cma_helper.h>
> > > +#include <linux/dma-mapping.h>
> > >   #include <linux/module.h>
> > > 
> > >   #define DEFAULT_FBDEFIO_DELAY_MS 50
> > > @@ -297,6 +298,12 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
> > >   EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
> > >   #endif
> > > 
> > > +static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > > +{
> > > +	return dma_mmap_writecombine(info->device, vma, info->screen_base,
> > > +				     info->fix.smem_start, info->fix.smem_len);
> > > +}
> > > +
> > >   static struct fb_ops drm_fbdev_cma_ops = {
> > >   	.owner		= THIS_MODULE,
> > >   	.fb_fillrect	= drm_fb_helper_sys_fillrect,
> > > @@ -307,6 +314,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
> > >   	.fb_blank	= drm_fb_helper_blank,
> > >   	.fb_pan_display	= drm_fb_helper_pan_display,
> > >   	.fb_setcmap	= drm_fb_helper_setcmap,
> > > +	.fb_mmap	= drm_fb_cma_mmap,
> > >   };
> > > 
> > >   static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> > > --
> > > 2.8.1.dirty
> > > 
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 172cafe11c71..a25afc068d3f 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -23,6 +23,7 @@ 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 
 #define DEFAULT_FBDEFIO_DELAY_MS 50
@@ -297,6 +298,12 @@  int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
 EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
 #endif
 
+static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	return dma_mmap_writecombine(info->device, vma, info->screen_base,
+				     info->fix.smem_start, info->fix.smem_len);
+}
+
 static struct fb_ops drm_fbdev_cma_ops = {
 	.owner		= THIS_MODULE,
 	.fb_fillrect	= drm_fb_helper_sys_fillrect,
@@ -307,6 +314,7 @@  static struct fb_ops drm_fbdev_cma_ops = {
 	.fb_blank	= drm_fb_helper_blank,
 	.fb_pan_display	= drm_fb_helper_pan_display,
 	.fb_setcmap	= drm_fb_helper_setcmap,
+	.fb_mmap	= drm_fb_cma_mmap,
 };
 
 static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,