[PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Robin Murphy]

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(+)

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

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Daniel Vetter]

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
> 

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

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[liviu.dudau]

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
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Robin Murphy]

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

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Daniel Vetter]

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

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

[PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Robin Murphy]

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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Hi dri-devel,

This is an empirical fix for something I tickled via the newly-added
ARM HDLCD driver on a Juno platform - I have no idea whatsoever about
how "proper" it is in terms of the DRM infrastructure, so feel free to
treat this as a bug report rather than an actual patch if appropriate ;)

 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 bb88e3d..117d7ea 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>
 
 struct drm_fb_cma {
@@ -221,6 +222,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,
@@ -231,6 +238,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_create(struct drm_fb_helper *helper,
-- 
2.7.3.dirty

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Daniel Vetter]

On Wed, Mar 16, 2016 at 02:57:49PM +0000, 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Hi dri-devel,
> 
> This is an empirical fix for something I tickled via the newly-added
> ARM HDLCD driver on a Juno platform - I have no idea whatsoever about
> how "proper" it is in terms of the DRM infrastructure, so feel free to
> treat this as a bug report rather than an actual patch if appropriate ;)

I think the best case would be if we could have a generic fbdev helper
that remaps to dumb mmap support. But that's a bit tricky to pull of:
1. from fb_info we can get at the fbdev drm_framebuffer.
2. from a drm_framebuffer we can get at the underlying backing storage
object using fb->funcs->get_handle.
3. With that handle we could go into the dumb mmap support (using als the
vma) and create the mmap.

Except that ->get_handle needs a file_priv, and that just exist for the
fbdev emulation kms client. I guess we could fix that by creating a
minimal fake drm file_priv for the fbdev emulation (and treat it more like
any other kms client), but I think that's way too much work when this
simple patch here gets the job done.

Long story short, assuming this doesn't break other cma-helper using
drivers:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
>  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 bb88e3d..117d7ea 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>
>  
>  struct drm_fb_cma {
> @@ -221,6 +222,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,
> @@ -231,6 +238,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_create(struct drm_fb_helper *helper,
> -- 
> 2.7.3.dirty
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Russell King - ARM Linux]

On Wed, Mar 16, 2016 at 04:28:25PM +0100, Daniel Vetter wrote:
> On Wed, Mar 16, 2016 at 02:57:49PM +0000, 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.
> > 
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 
> > Hi dri-devel,
> > 
> > This is an empirical fix for something I tickled via the newly-added
> > ARM HDLCD driver on a Juno platform - I have no idea whatsoever about
> > how "proper" it is in terms of the DRM infrastructure, so feel free to
> > treat this as a bug report rather than an actual patch if appropriate ;)
> 
> I think the best case would be if we could have a generic fbdev helper
> that remaps to dumb mmap support. But that's a bit tricky to pull of:
> 1. from fb_info we can get at the fbdev drm_framebuffer.
> 2. from a drm_framebuffer we can get at the underlying backing storage
> object using fb->funcs->get_handle.
> 3. With that handle we could go into the dumb mmap support (using als the
> vma) and create the mmap.
> 
> Except that ->get_handle needs a file_priv, and that just exist for the
> fbdev emulation kms client. I guess we could fix that by creating a
> minimal fake drm file_priv for the fbdev emulation (and treat it more like
> any other kms client), but I think that's way too much work when this
> simple patch here gets the job done.

I think first, a different question needs to be answered:

include/uapi/linux/fb.h:

struct fb_fix_screeninfo {
        char id[16];                    /* identification string eg "TT Builtin" */
        unsigned long smem_start;       /* Start of frame buffer mem */
                                        /* (physical address) */

Should a DMA address be exposed through smem_start, rather than a
physical address as the long-standing documentation quoted above
has stated?

Is it, in fact, a driver bug to store something that isn't a physical
address there?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm/fb_cma_helper: Implement fb_mmap callback

[Robin Murphy]

On 16/03/16 19:14, Russell King - ARM Linux wrote:
> On Wed, Mar 16, 2016 at 04:28:25PM +0100, Daniel Vetter wrote:
>> On Wed, Mar 16, 2016 at 02:57:49PM +0000, 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.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>
>>> Hi dri-devel,
>>>
>>> This is an empirical fix for something I tickled via the newly-added
>>> ARM HDLCD driver on a Juno platform - I have no idea whatsoever about
>>> how "proper" it is in terms of the DRM infrastructure, so feel free to
>>> treat this as a bug report rather than an actual patch if appropriate ;)
>>
>> I think the best case would be if we could have a generic fbdev helper
>> that remaps to dumb mmap support. But that's a bit tricky to pull of:
>> 1. from fb_info we can get at the fbdev drm_framebuffer.
>> 2. from a drm_framebuffer we can get at the underlying backing storage
>> object using fb->funcs->get_handle.
>> 3. With that handle we could go into the dumb mmap support (using als the
>> vma) and create the mmap.
>>
>> Except that ->get_handle needs a file_priv, and that just exist for the
>> fbdev emulation kms client. I guess we could fix that by creating a
>> minimal fake drm file_priv for the fbdev emulation (and treat it more like
>> any other kms client), but I think that's way too much work when this
>> simple patch here gets the job done.
>
> I think first, a different question needs to be answered:
>
> include/uapi/linux/fb.h:
>
> struct fb_fix_screeninfo {
>          char id[16];                    /* identification string eg "TT Builtin" */
>          unsigned long smem_start;       /* Start of frame buffer mem */
>                                          /* (physical address) */
>
> Should a DMA address be exposed through smem_start, rather than a
> physical address as the long-standing documentation quoted above
> has stated?
>
> Is it, in fact, a driver bug to store something that isn't a physical
> address there?

We could also go into whether it's right to store even a physical 
address in something which isn't necessarily big enough...

After the time I spent in the bowels of the fbdev code figuring out this 
crash, I fear that if we dig too deep we may awaken something in the 
darkness ;)

Robin.