Hello,
The purpose of this RFC is to understand what is needed to allow to write drm/kms drivers for devices without MMU.
There are some MCU platforms, like stm32f4, which don't have MMU but have hardware display IP where is it possible to implement a drm/kms driver.
Obviously that start by removing MMU configuration flag from Kconfig. But while coding the driver for stm32f4 (on discovery board to have enough memory) we have already identify few other pieces of code that need to be change. We have been inspired by what already exist in v4l2 where using mmuless devices is possible.
Since we have only use cma helpers we only have partial view of what could be needed for other part of drm/kms framework.
Benjamin Gaignard (1): drm: allow to use mmuless SoC
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_vm.c | 4 +++ drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org --- drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_vm.c | 4 +++ drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8d9cf73..36c102b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -6,7 +6,7 @@ # menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" - depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && MMU && HAS_DMA + depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA select HDMI select FB_CMDLINE select I2C @@ -98,7 +98,7 @@ config DRM_LOAD_EDID_FIRMWARE
config DRM_TTM tristate - depends on DRM + depends on DRM && MMU help GPU memory management subsystem for devices with multiple GPU memory types. Will be enabled automatically if a device driver diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558..a3cef79 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -343,10 +343,30 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { +#ifdef CONFIG_MMU return dma_mmap_writecombine(info->device, vma, info->screen_base, info->fix.smem_start, info->fix.smem_len); +#else + return vm_iomap_memory(vma, vma->vm_start, info->fix.smem_len); +#endif }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +unsigned long get_fb_unmapped_area(struct file *filp, + unsigned long addr, unsigned long len, + unsigned long pgoff, unsigned long flags) +{ + struct fb_info * const info = filp->private_data; + unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len); + + if (pgoff > fb_size || len > fb_size - pgoff) + return -EINVAL; + + return (unsigned long)info->screen_base + pgoff; +} +EXPORT_SYMBOL_GPL(get_fb_unmapped_area); +#endif + static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS, diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 1d6c335..de13535 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -318,11 +318,16 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, vma->vm_flags &= ~VM_PFNMAP; vma->vm_pgoff = 0;
+#ifdef CONFIG_MMU ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr, cma_obj->paddr, vma->vm_end - vma->vm_start); if (ret) drm_gem_vm_close(vma);
+#else + ret = vm_iomap_memory(vma, vma->vm_start, + (vma->vm_end - vma->vm_start)); +#endif return ret; }
@@ -358,6 +363,63 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
+#ifdef CONFIG_MMU +#define drm_gem_cma_get_unmapped_area NULL +#else +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp, + unsigned long addr, + unsigned long len, + unsigned long pgoff, + unsigned long flags) +{ + struct drm_gem_cma_object *cma_obj; + struct drm_gem_object *obj = NULL; + struct drm_file *priv = filp->private_data; + struct drm_device *dev = priv->minor->dev; + struct drm_vma_offset_node *node; + + if (drm_device_is_unplugged(dev)) + return -ENODEV; + + drm_vma_offset_lock_lookup(dev->vma_offset_manager); + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, + pgoff, + len >> PAGE_SHIFT); + if (likely(node)) { + obj = container_of(node, struct drm_gem_object, vma_node); + /* + * When the object is being freed, after it hits 0-refcnt it + * proceeds to tear down the object. In the process it will + * attempt to remove the VMA offset and so acquire this + * mgr->vm_lock. Therefore if we find an object with a 0-refcnt + * that matches our range, we know it is in the process of being + * destroyed and will be freed as soon as we release the lock - + * so we have to check for the 0-refcnted object and treat it as + * invalid. + */ + if (!kref_get_unless_zero(&obj->refcount)) + obj = NULL; + } + + drm_vma_offset_unlock_lookup(dev->vma_offset_manager); + + if (!obj) + return -EINVAL; + + if (!drm_vma_node_is_allowed(node, priv)) { + drm_gem_object_unreference_unlocked(obj); + return -EACCES; + } + + cma_obj = to_drm_gem_cma_obj(obj); + + drm_gem_object_unreference_unlocked(obj); + + return cma_obj->vaddr ? (unsigned long)cma_obj->vaddr : -EINVAL; +} +#endif +EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area); + #ifdef CONFIG_DEBUG_FS /** * drm_gem_cma_describe - describe a CMA GEM object for debugfs diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index caa4e4c..974e02b 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -489,6 +489,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -497,6 +498,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif }
vma->vm_ops = &drm_vm_dma_ops; @@ -573,6 +575,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -581,6 +584,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif }
switch (map->type) { diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..d6a83bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1492,6 +1492,14 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, return 0; }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +extern unsigned long get_fb_unmapped_area(struct file *filp, + unsigned long addr, + unsigned long len, + unsigned long pgoff, + unsigned long flags); +#endif + static const struct file_operations fb_fops = { .owner = THIS_MODULE, .read = fb_read, diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index acd6af8..79e9dbd 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -51,6 +51,12 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size);
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp, + unsigned long addr, + unsigned long len, + unsigned long pgoff, + unsigned long flags); + extern const struct vm_operations_struct drm_gem_cma_vm_ops;
#ifdef CONFIG_DEBUG_FS
Hi Benjamin,
Thank you for the patch.
On Wednesday 30 Nov 2016 12:21:24 Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_vm.c | 4 +++
I'll let someone more knowledgeable than me comment on these two files.
drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558..a3cef79 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -343,10 +343,30 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { +#ifdef CONFIG_MMU return dma_mmap_writecombine(info->device, vma, info->screen_base, info->fix.smem_start, info- fix.smem_len); +#else
- return vm_iomap_memory(vma, vma->vm_start, info->fix.smem_len);
+#endif }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
Shouldn't this be #ifndef ? When the symbol is defined the implementation is provided by the architecture.
By the way, given that you provide a generic implementation here, what's the reason some architecture need to implement the function in a special way ? Couldn't we come up with a generic implementation across all architectures ?
+unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr, unsigned long len,unsigned long pgoff, unsigned long flags)+{
- struct fb_info * const info = filp->private_data;
- unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
- if (pgoff > fb_size || len > fb_size - pgoff)
return -EINVAL;- return (unsigned long)info->screen_base + pgoff;
+} +EXPORT_SYMBOL_GPL(get_fb_unmapped_area);
Shouldn't this be defined somewhere in drivers/video/ instead, as the code is fbdev-specific and only used in drivers/video/fbdev/core/fbmem.c ?
+#endif
static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS,
[snip]
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..d6a83bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1492,6 +1492,14 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, return 0; }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +extern unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);+#endif
This should come from a header file.
static const struct file_operations fb_fops = { .owner = THIS_MODULE, .read = fb_read, diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index acd6af8..79e9dbd 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -51,6 +51,12 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size);
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);extern const struct vm_operations_struct drm_gem_cma_vm_ops;
#ifdef CONFIG_DEBUG_FS
2016-11-30 12:35 GMT+01:00 Laurent Pinchart laurent.pinchart@ideasonboard.com:
Hi Benjamin,
Thank you for the patch.
On Wednesday 30 Nov 2016 12:21:24 Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_vm.c | 4 +++
I'll let someone more knowledgeable than me comment on these two files.
drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558..a3cef79 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -343,10 +343,30 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { +#ifdef CONFIG_MMU return dma_mmap_writecombine(info->device, vma, info->screen_base, info->fix.smem_start, info- fix.smem_len); +#else
return vm_iomap_memory(vma, vma->vm_start, info->fix.smem_len);+#endif }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
Shouldn't this be #ifndef ? When the symbol is defined the implementation is provided by the architecture.
By the way, given that you provide a generic implementation here, what's the reason some architecture need to implement the function in a special way ? Couldn't we come up with a generic implementation across all architectures ?
+unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr, unsigned long len,unsigned long pgoff, unsigned long flags)+{
struct fb_info * const info = filp->private_data;unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);if (pgoff > fb_size || len > fb_size - pgoff)return -EINVAL;return (unsigned long)info->screen_base + pgoff;+} +EXPORT_SYMBOL_GPL(get_fb_unmapped_area);
Shouldn't this be defined somewhere in drivers/video/ instead, as the code is fbdev-specific and only used in drivers/video/fbdev/core/fbmem.c ?
maybe I could migrate this code in drivers/video/fbdev/core/fbmem.c and play with HAVE_ARCH_FB_UNMAPPED_AREA and get_fb_unmapped_area to know if the symbol already exist or not. I think that would also fix your previous and next remark because I this case I will not have declare the prototype.
+#endif
static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS,
[snip]
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..d6a83bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1492,6 +1492,14 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, return 0; }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +extern unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);+#endif
This should come from a header file.
static const struct file_operations fb_fops = { .owner = THIS_MODULE, .read = fb_read, diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index acd6af8..79e9dbd 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -51,6 +51,12 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size);
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);extern const struct vm_operations_struct drm_gem_cma_vm_ops;
#ifdef CONFIG_DEBUG_FS
-- Regards,
Laurent Pinchart
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++++++++++++++++++++++++
Hacking this into cma helpers feels very wrong to me. mmu-less looks like it works substantially differently enough that it warrants a new set of mmu-less gem helpers. And maybe that will showcase more code-refactoring opportunities between cma helpers, mmu-less helpers and shmem-based gem objects. And if there's none, then I think copypasting is better than ending up in a mess.
drivers/gpu/drm/drm_vm.c | 4 +++ drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8d9cf73..36c102b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -6,7 +6,7 @@ # menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
- depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && MMU && HAS_DMA
- depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA select HDMI select FB_CMDLINE select I2C
@@ -98,7 +98,7 @@ config DRM_LOAD_EDID_FIRMWARE config DRM_TTM tristate
- depends on DRM
- depends on DRM && MMU help GPU memory management subsystem for devices with multiple GPU memory types. Will be enabled automatically if a device driver
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558..a3cef79 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -343,10 +343,30 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg) static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { +#ifdef CONFIG_MMU return dma_mmap_writecombine(info->device, vma, info->screen_base, info->fix.smem_start, info->fix.smem_len); +#else
- return vm_iomap_memory(vma, vma->vm_start, info->fix.smem_len);
+#endif } +#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr, unsigned long len,unsigned long pgoff, unsigned long flags)+{
- struct fb_info * const info = filp->private_data;
- unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
- if (pgoff > fb_size || len > fb_size - pgoff)
return -EINVAL;- return (unsigned long)info->screen_base + pgoff;
+} +EXPORT_SYMBOL_GPL(get_fb_unmapped_area); +#endif
static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS, diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 1d6c335..de13535 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -318,11 +318,16 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, vma->vm_flags &= ~VM_PFNMAP; vma->vm_pgoff = 0; +#ifdef CONFIG_MMU ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr, cma_obj->paddr, vma->vm_end - vma->vm_start); if (ret) drm_gem_vm_close(vma); +#else
- ret = vm_iomap_memory(vma, vma->vm_start,
(vma->vm_end - vma->vm_start));+#endif return ret; } @@ -358,6 +363,63 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); +#ifdef CONFIG_MMU +#define drm_gem_cma_get_unmapped_area NULL +#else +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags)+{
- struct drm_gem_cma_object *cma_obj;
- struct drm_gem_object *obj = NULL;
- struct drm_file *priv = filp->private_data;
- struct drm_device *dev = priv->minor->dev;
- struct drm_vma_offset_node *node;
- if (drm_device_is_unplugged(dev))
return -ENODEV;- drm_vma_offset_lock_lookup(dev->vma_offset_manager);
- node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
pgoff,len >> PAGE_SHIFT);- if (likely(node)) {
obj = container_of(node, struct drm_gem_object, vma_node);/** When the object is being freed, after it hits 0-refcnt it* proceeds to tear down the object. In the process it will* attempt to remove the VMA offset and so acquire this* mgr->vm_lock. Therefore if we find an object with a 0-refcnt* that matches our range, we know it is in the process of being* destroyed and will be freed as soon as we release the lock -* so we have to check for the 0-refcnted object and treat it as* invalid.*/if (!kref_get_unless_zero(&obj->refcount))obj = NULL;- }
- drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
- if (!obj)
return -EINVAL;- if (!drm_vma_node_is_allowed(node, priv)) {
drm_gem_object_unreference_unlocked(obj);return -EACCES;- }
- cma_obj = to_drm_gem_cma_obj(obj);
- drm_gem_object_unreference_unlocked(obj);
- return cma_obj->vaddr ? (unsigned long)cma_obj->vaddr : -EINVAL;
+} +#endif +EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
#ifdef CONFIG_DEBUG_FS /**
- drm_gem_cma_describe - describe a CMA GEM object for debugfs
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index caa4e4c..974e02b 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -489,6 +489,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -497,6 +498,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif } vma->vm_ops = &drm_vm_dma_ops; @@ -573,6 +575,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -581,6 +584,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif } switch (map->type) {
drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Cheers, Daniel
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..d6a83bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1492,6 +1492,14 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, return 0; } +#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +extern unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);+#endif
static const struct file_operations fb_fops = { .owner = THIS_MODULE, .read = fb_read, diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index acd6af8..79e9dbd 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -51,6 +51,12 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size); +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);extern const struct vm_operations_struct drm_gem_cma_vm_ops;
#ifdef CONFIG_DEBUG_FS
1.9.1
2016-11-30 14:52 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++++++++++++++++++++++++
Hacking this into cma helpers feels very wrong to me. mmu-less looks like it works substantially differently enough that it warrants a new set of mmu-less gem helpers. And maybe that will showcase more code-refactoring opportunities between cma helpers, mmu-less helpers and shmem-based gem objects. And if there's none, then I think copypasting is better than ending up in a mess.
Only the 2 calls to dma_mmap_wc() are problematic within ARM nommu architecture. After Laurent remarks I have move get_fb_unmapped_area() to fbmem.c it really limits the modifications in cma helpers...
drivers/gpu/drm/drm_vm.c | 4 +++ drivers/video/fbdev/core/fbmem.c | 8 +++++ include/drm/drm_gem_cma_helper.h | 6 ++++ 6 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8d9cf73..36c102b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -6,7 +6,7 @@ # menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && MMU && HAS_DMA
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA select HDMI select FB_CMDLINE select I2C@@ -98,7 +98,7 @@ config DRM_LOAD_EDID_FIRMWARE
config DRM_TTM tristate
depends on DRM
depends on DRM && MMU help GPU memory management subsystem for devices with multiple GPU memory types. Will be enabled automatically if a device driverdiff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558..a3cef79 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -343,10 +343,30 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { +#ifdef CONFIG_MMU return dma_mmap_writecombine(info->device, vma, info->screen_base, info->fix.smem_start, info->fix.smem_len); +#else
return vm_iomap_memory(vma, vma->vm_start, info->fix.smem_len);+#endif }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr, unsigned long len,unsigned long pgoff, unsigned long flags)+{
struct fb_info * const info = filp->private_data;unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);if (pgoff > fb_size || len > fb_size - pgoff)return -EINVAL;return (unsigned long)info->screen_base + pgoff;+} +EXPORT_SYMBOL_GPL(get_fb_unmapped_area); +#endif
static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS, diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 1d6c335..de13535 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -318,11 +318,16 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, vma->vm_flags &= ~VM_PFNMAP; vma->vm_pgoff = 0;
+#ifdef CONFIG_MMU ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr, cma_obj->paddr, vma->vm_end - vma->vm_start); if (ret) drm_gem_vm_close(vma);
+#else
ret = vm_iomap_memory(vma, vma->vm_start,(vma->vm_end - vma->vm_start));+#endif return ret; }
@@ -358,6 +363,63 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
+#ifdef CONFIG_MMU +#define drm_gem_cma_get_unmapped_area NULL +#else +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags)+{
struct drm_gem_cma_object *cma_obj;struct drm_gem_object *obj = NULL;struct drm_file *priv = filp->private_data;struct drm_device *dev = priv->minor->dev;struct drm_vma_offset_node *node;if (drm_device_is_unplugged(dev))return -ENODEV;drm_vma_offset_lock_lookup(dev->vma_offset_manager);node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,pgoff,len >> PAGE_SHIFT);if (likely(node)) {obj = container_of(node, struct drm_gem_object, vma_node);/** When the object is being freed, after it hits 0-refcnt it* proceeds to tear down the object. In the process it will* attempt to remove the VMA offset and so acquire this* mgr->vm_lock. Therefore if we find an object with a 0-refcnt* that matches our range, we know it is in the process of being* destroyed and will be freed as soon as we release the lock -* so we have to check for the 0-refcnted object and treat it as* invalid.*/if (!kref_get_unless_zero(&obj->refcount))obj = NULL;}drm_vma_offset_unlock_lookup(dev->vma_offset_manager);if (!obj)return -EINVAL;if (!drm_vma_node_is_allowed(node, priv)) {drm_gem_object_unreference_unlocked(obj);return -EACCES;}cma_obj = to_drm_gem_cma_obj(obj);drm_gem_object_unreference_unlocked(obj);return cma_obj->vaddr ? (unsigned long)cma_obj->vaddr : -EINVAL;+} +#endif +EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
#ifdef CONFIG_DEBUG_FS /**
- drm_gem_cma_describe - describe a CMA GEM object for debugfs
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index caa4e4c..974e02b 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -489,6 +489,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -497,6 +498,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif }
vma->vm_ops = &drm_vm_dma_ops;@@ -573,6 +575,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) #if defined(__i386__) || defined(__x86_64__) pgprot_val(vma->vm_page_prot) &= ~_PAGE_RW; #else +#ifdef CONFIG_MMU /* Ye gads this is ugly. With more thought we could move this up higher and use `protection_map' instead. */ @@ -581,6 +584,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) (pte_wrprotect (__pte(pgprot_val(vma->vm_page_prot))))); #endif +#endif }
switch (map->type) {drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Following your advice I have removed drm_vm.c from the build if CONFIG_MMU is not set I only had to stub drm_legacy_vma_flush() to compile. I will include that in v3
Cheers, Daniel
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 76c1ad9..d6a83bd 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1492,6 +1492,14 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, return 0; }
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA +extern unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);+#endif
static const struct file_operations fb_fops = { .owner = THIS_MODULE, .read = fb_read, diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index acd6af8..79e9dbd 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -51,6 +51,12 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, size_t size);
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags);extern const struct vm_operations_struct drm_gem_cma_vm_ops;
#ifdef CONFIG_DEBUG_FS
1.9.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi Benjamin,
On Wednesday 30 Nov 2016 16:08:23 Benjamin Gaignard wrote:
2016-11-30 14:52 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++++
[snip]
drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Following your advice I have removed drm_vm.c from the build if CONFIG_MMU is not set
How about only including it if !DRM_LEGACY && !DRM_NOUVEAU ?
I only had to stub drm_legacy_vma_flush() to compile. I will include that in v3
2016-11-30 16:19 GMT+01:00 Laurent Pinchart laurent.pinchart@ideasonboard.com:
Hi Benjamin,
On Wednesday 30 Nov 2016 16:08:23 Benjamin Gaignard wrote:
2016-11-30 14:52 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++++
[snip]
drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Following your advice I have removed drm_vm.c from the build if CONFIG_MMU is not set
How about only including it if !DRM_LEGACY && !DRM_NOUVEAU ?
I don't understand the link between !DRM_LEGACY && !DRM_NOUVEAU and MMU...
By chance would you mean including only if DRM_LEGACY && DRM_NOUVEAU ?
I only had to stub drm_legacy_vma_flush() to compile. I will include that in v3
-- Regards,
Laurent Pinchart
Hi Benjamin,
On Wednesday 30 Nov 2016 16:34:37 Benjamin Gaignard wrote:
2016-11-30 16:19 GMT+01:00 Laurent Pinchart:
On Wednesday 30 Nov 2016 16:08:23 Benjamin Gaignard wrote:
2016-11-30 14:52 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++
[snip]
drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Following your advice I have removed drm_vm.c from the build if CONFIG_MMU is not set
How about only including it if !DRM_LEGACY && !DRM_NOUVEAU ?
I don't understand the link between !DRM_LEGACY && !DRM_NOUVEAU and MMU...
By chance would you mean including only if DRM_LEGACY && DRM_NOUVEAU ?
I meant excluding it if !DRM_LEGACY && !DRM_NOUVEAU, sorry, so including it if DRM_LEGACY || DRM_NOUVEAU.
(Probably out of scope for this patch series, but why do we need it in nouveau ?)
I only had to stub drm_legacy_vma_flush() to compile. I will include that in v3
On Wed, Nov 30, 2016 at 05:36:46PM +0200, Laurent Pinchart wrote:
Hi Benjamin,
On Wednesday 30 Nov 2016 16:34:37 Benjamin Gaignard wrote:
2016-11-30 16:19 GMT+01:00 Laurent Pinchart:
On Wednesday 30 Nov 2016 16:08:23 Benjamin Gaignard wrote:
2016-11-30 14:52 GMT+01:00 Daniel Vetter daniel@ffwll.ch:
On Wed, Nov 30, 2016 at 12:21:24PM +0100, Benjamin Gaignard wrote:
Some platforms without MMU have display drivers where a drm/kms driver could be implemented. Before doing such kind of thing drm/kms must allow to use mmuless devices. This patch proposes to remove MMU configuration flag and add some cma helpers functions to help implementing mmuless display driver
Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org
drivers/gpu/drm/Kconfig | 4 +-- drivers/gpu/drm/drm_fb_cma_helper.c | 20 ++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 62 +++++++++++++++++++++++++++
[snip]
drm_vm.c is a legacy horror show. Instead of hacking even more garbage into this, can't we just not compile this for MMU-less platforms? A bunch of stubs in drm_internal.h is all that should be needed for this, since on MMU-less you should never be able to enable one of the legacy drivers which need the exported symbols from this file.
Following your advice I have removed drm_vm.c from the build if CONFIG_MMU is not set
How about only including it if !DRM_LEGACY && !DRM_NOUVEAU ?
I don't understand the link between !DRM_LEGACY && !DRM_NOUVEAU and MMU...
By chance would you mean including only if DRM_LEGACY && DRM_NOUVEAU ?
I meant excluding it if !DRM_LEGACY && !DRM_NOUVEAU, sorry, so including it if DRM_LEGACY || DRM_NOUVEAU.
(Probably out of scope for this patch series, but why do we need it in nouveau ?)
Don't ask, but you did: The very first version of the nouveau kms ddx was a partial conversion from ums to kms, and still create some of the old garabge. Among them a ctx and a few other things. Of course it was never used, but the init code was overlooked, and it checks for errors. Which means we can't just fail.
The other problem is that rhel shipped that version of nouveau, which means we need to wait 10 years or so until we can nuke it all :( -Daniel
linaro-kernel@lists.linaro.org
-
Benjamin Gaignard -
Daniel Vetter -
Laurent Pinchart