Hi all,
Here's my resend of the dev->struct_mutex locking removal patches. I'd like to get them all into 4.5, so please pick them either up into your tree or ack them. I'll send a pull request for the remaining in a few weeks.
Thanks, Daniel
Daniel Vetter (20): drm/armada: Plug leak in dumb_map_offset drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl drm/armada: Drop struct_mutex from cursor paths drm/armada: Use a private mutex to protect priv->linear drm/tegra: don't take dev->struct_mutex in mmap offset ioctl drm/tegra: Use drm_gem_object_unreference_unlocked drm/gma500: Use correct unref in the gem bo create function drm/gma500: Drop dev->struct_mutex from modeset code drm/gma500: Drop dev->struct_mutex from fbdev init/teardown code drm/gma500: Drop dev->struct_mutex from mmap offset function drm/gma500: Add driver private mutex for the fault handler drm/nouveau: Drop dev->struct_mutex from fbdev init drm/exynos: Drop dev->struct_mutex from mmap offset function drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl drm/exynos: drop struct_mutex from fbdev setup drm/vgem: Simplify dum_map drm/vgem: Move get_pages to gem_create drm/vgem: Drop dev->struct_mutex drm/vma_manage: Drop has_offset
drivers/gpu/drm/armada/armada_crtc.c | 6 +----- drivers/gpu/drm/armada/armada_debugfs.c | 4 ++-- drivers/gpu/drm/armada/armada_drm.h | 3 ++- drivers/gpu/drm/armada/armada_drv.c | 1 + drivers/gpu/drm/armada/armada_gem.c | 21 ++++++++----------- drivers/gpu/drm/drm_gem.c | 17 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 ++++++++------------ drivers/gpu/drm/exynos/exynos_drm_gem.c | 15 +------------- drivers/gpu/drm/gma500/framebuffer.c | 12 ++--------- drivers/gpu/drm/gma500/gem.c | 19 ++++++----------- drivers/gpu/drm/gma500/gma_display.c | 13 +++--------- drivers/gpu/drm/gma500/gtt.c | 1 + drivers/gpu/drm/gma500/psb_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 3 --- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ----- drivers/gpu/drm/tegra/drm.c | 4 +--- drivers/gpu/drm/tegra/gem.c | 13 ++---------- drivers/gpu/drm/vgem/vgem_drv.c | 34 +++++++++---------------------- include/drm/drm_vma_manager.h | 15 +------------- 19 files changed, 69 insertions(+), 141 deletions(-)
We need to drop the gem bo reference if it's an imported one.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_gem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 60a688ef81c7..323da46c7ca8 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -285,7 +285,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, /* Don't allow imported objects to be mapped */ if (obj->obj.import_attach) { ret = -EINVAL; - goto err_unlock; + goto err_unref; }
ret = drm_gem_create_mmap_offset(&obj->obj); @@ -294,6 +294,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset); }
+ err_unref: drm_gem_object_unreference(&obj->obj); err_unlock: mutex_unlock(&dev->struct_mutex);
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
v2: Split out the leak fix in dump_map_offset into a separate patch as requested by Russell. Also align labels the same way as before to stick with local coding style.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_gem.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 323da46c7ca8..59fafa468347 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -274,12 +274,10 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, struct armada_gem_object *obj; int ret = 0;
- mutex_lock(&dev->struct_mutex); obj = armada_gem_object_lookup(dev, file, handle); if (!obj) { DRM_ERROR("failed to lookup gem object\n"); - ret = -EINVAL; - goto err_unlock; + return -EINVAL; }
/* Don't allow imported objects to be mapped */ @@ -295,9 +293,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, }
err_unref: - drm_gem_object_unreference(&obj->obj); - err_unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->obj);
return ret; }
The kms state itself is already protected by the modeset locks acquired by the drm core. The only thing left is gem bo state, and since the cursor code expects small objects which are statically mapped at create time and then invariant over the lifetime of the gem bo there's nothing to protect.
See armada_gem_dumb_create -> armada_gem_linear_back which assigns obj->addr which is the only thing used by the cursor code.
Only tricky bit is to switch to the _unlocked unreference function.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_crtc.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index cebcab560626..67fed0bbe53a 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -928,11 +928,10 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc, } }
- mutex_lock(&dev->struct_mutex); if (dcrtc->cursor_obj) { dcrtc->cursor_obj->update = NULL; dcrtc->cursor_obj->update_data = NULL; - drm_gem_object_unreference(&dcrtc->cursor_obj->obj); + drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj); } dcrtc->cursor_obj = obj; dcrtc->cursor_w = w; @@ -942,7 +941,6 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc, obj->update_data = dcrtc; obj->update = cursor_update; } - mutex_unlock(&dev->struct_mutex);
return ret; } @@ -957,11 +955,9 @@ static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) if (!dcrtc->variant->has_spu_adv_reg) return -EFAULT;
- mutex_lock(&dev->struct_mutex); dcrtc->cursor_x = x; dcrtc->cursor_y = y; ret = armada_drm_crtc_cursor_update(dcrtc, false); - mutex_unlock(&dev->struct_mutex);
return ret; }
Reusing the Big DRM Lock just leaks, and the few things left that dev->struct_mutex protected are very well contained - it's just the linear drm_mm manager.
With this armada is completely struct_mutex free!
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_debugfs.c | 4 ++-- drivers/gpu/drm/armada/armada_drm.h | 3 ++- drivers/gpu/drm/armada/armada_drv.c | 1 + drivers/gpu/drm/armada/armada_gem.c | 10 +++++----- 4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 471e45627f1e..d4f7ab0a30d4 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -21,9 +21,9 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data) struct armada_private *priv = dev->dev_private; int ret;
- mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); ret = drm_mm_dump_table(m, &priv->linear); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock);
return ret; } diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h index 4df6f2af2b21..6d2d8be38297 100644 --- a/drivers/gpu/drm/armada/armada_drm.h +++ b/drivers/gpu/drm/armada/armada_drm.h @@ -57,7 +57,8 @@ struct armada_private { DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8); struct drm_fb_helper *fbdev; struct armada_crtc *dcrtc[2]; - struct drm_mm linear; + struct drm_mm linear; /* protectec by linear_lock */ + struct mutex linear_lock; struct drm_property *csc_yuv_prop; struct drm_property *csc_rgb_prop; struct drm_property *colorkey_prop; diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index 77ab93d60125..3bd7e1cde99e 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -102,6 +102,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) dev->mode_config.preferred_depth = 24; dev->mode_config.funcs = &armada_drm_mode_config_funcs; drm_mm_init(&priv->linear, mem->start, resource_size(mem)); + mutex_init(&priv->linear_lock);
ret = component_bind_all(dev->dev, dev); if (ret) diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 59fafa468347..45e12d4c1722 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size) return roundup(size, PAGE_SIZE); }
-/* dev->struct_mutex is held here */ +/* dev_priv->linear_lock is held here */ void armada_gem_free_object(struct drm_gem_object *obj) { struct armada_gem_object *dobj = drm_to_armada_gem(obj); @@ -144,10 +144,10 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj) if (!node) return -ENOSPC;
- mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); ret = drm_mm_insert_node(&priv->linear, node, size, align, DRM_MM_SEARCH_DEFAULT); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock); if (ret) { kfree(node); return ret; @@ -158,9 +158,9 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj) /* Ensure that the memory we're returning is cleared. */ ptr = ioremap_wc(obj->linear->start, size); if (!ptr) { - mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); drm_mm_remove_node(obj->linear); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock); kfree(obj->linear); obj->linear = NULL; return -ENOMEM;
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
v2: Finally get rid of the copypasta from another commit in this commit message. And convert to _unlocked like we need to (Patrik).
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Cc: Thierry Reding thierry.reding@gmail.com Acked-by: Thierry Reding thierry.reding@gmail.com Reviewed-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/gem.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 01e16e146bfe..fb712316c522 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, struct drm_gem_object *gem; struct tegra_bo *bo;
- mutex_lock(&drm->struct_mutex); - gem = drm_gem_object_lookup(drm, file, handle); if (!gem) { dev_err(drm->dev, "failed to lookup GEM object\n"); - mutex_unlock(&drm->struct_mutex); return -EINVAL; }
@@ -421,9 +418,7 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
*offset = drm_vma_node_offset_addr(&bo->gem.vma_node);
- drm_gem_object_unreference(gem); - - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem);
return 0; }
This only grabs the mutex when really needed, but still has a might-acquire lockdep check to make sure that's always possible. With this patch tegra is officially struct_mutex free, yay!
v2: refernce_unlocked doesn't exist as kbuild spotted.
Cc: Thierry Reding thierry.reding@gmail.com Acked-by: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/drm.c | 4 +--- drivers/gpu/drm/tegra/gem.c | 6 +----- 2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index e0f827790a5e..41f4efef777b 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -277,9 +277,7 @@ host1x_bo_lookup(struct drm_device *drm, struct drm_file *file, u32 handle) if (!gem) return NULL;
- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem);
bo = to_tegra_bo(gem); return &bo->base; diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index fb712316c522..fbec29516bcb 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -30,9 +30,7 @@ static void tegra_bo_put(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;
- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(&obj->gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->gem); }
static dma_addr_t tegra_bo_pin(struct host1x_bo *bo, struct sg_table **sgt) @@ -74,9 +72,7 @@ static struct host1x_bo *tegra_bo_get(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;
- mutex_lock(&drm->struct_mutex); drm_gem_object_reference(&obj->gem); - mutex_unlock(&drm->struct_mutex);
return bo; }
This is called without dev->struct_mutex held, we need to use the _unlocked variant.
Never caught in the wild since you'd need an evil userspace which races a gem_close ioctl call with the in-progress open.
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index c707fa6fca85..e3bdc8b1c32c 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -130,7 +130,7 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, return ret; } /* We have the initial and handle reference but need only one now */ - drm_gem_object_unreference(&r->gem); + drm_gem_object_unreference_unlocked(&r->gem); *handlep = handle; return 0; }
It's either init code or already protected by other means. Note that psb_gtt_pin/unpin has it's own lock, and that's really the only piece of driver private state - all the modeset state is protected with modeset locks already.
The only important bit is to switch all gem_obj_unref calls to the _unlocked variant.
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/gma_display.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 001b450b27b3..ff17af4cfc64 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -349,8 +349,6 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* If we didn't get a handle then turn the cursor off */ if (!handle) { temp = CURSOR_MODE_DISABLE; - mutex_lock(&dev->struct_mutex); - if (gma_power_begin(dev, false)) { REG_WRITE(control, temp); REG_WRITE(base, 0); @@ -362,11 +360,9 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem); psb_gtt_unpin(gt); - drm_gem_object_unreference(gma_crtc->cursor_obj); + drm_gem_object_unreference_unlocked(gma_crtc->cursor_obj); gma_crtc->cursor_obj = NULL; } - - mutex_unlock(&dev->struct_mutex); return 0; }
@@ -376,7 +372,6 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, return -EINVAL; }
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) { ret = -ENOENT; @@ -441,17 +436,15 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, if (gma_crtc->cursor_obj) { gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem); psb_gtt_unpin(gt); - drm_gem_object_unreference(gma_crtc->cursor_obj); + drm_gem_object_unreference_unlocked(gma_crtc->cursor_obj); }
gma_crtc->cursor_obj = obj; unlock: - mutex_unlock(&dev->struct_mutex); return ret;
unref_cursor: - drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj); return ret; }
This is init/teardown code, locking is just to appease locking checks. And since gem create/free doesn't need this any more there's really no reason for grabbing dev->struct_mutex.
Again important to switch obj_unref to _unlocked variants.
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/framebuffer.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index dc0508dca1d4..ee95c03a8c54 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -406,8 +406,6 @@ static int psbfb_create(struct psb_fbdev *fbdev,
memset(dev_priv->vram_addr + backing->offset, 0, size);
- mutex_lock(&dev->struct_mutex); - info = drm_fb_helper_alloc_fbi(&fbdev->psb_fb_helper); if (IS_ERR(info)) { ret = PTR_ERR(info); @@ -463,17 +461,15 @@ static int psbfb_create(struct psb_fbdev *fbdev, dev_dbg(dev->dev, "allocated %dx%d fb\n", psbfb->base.width, psbfb->base.height);
- mutex_unlock(&dev->struct_mutex); return 0; out_unref: if (backing->stolen) psb_gtt_free_range(dev, backing); else - drm_gem_object_unreference(&backing->gem); + drm_gem_object_unreference_unlocked(&backing->gem);
drm_fb_helper_release_fbi(&fbdev->psb_fb_helper); out_err1: - mutex_unlock(&dev->struct_mutex); psb_gtt_free_range(dev, backing); return ret; } @@ -569,7 +565,7 @@ static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev *fbdev) drm_framebuffer_cleanup(&psbfb->base);
if (psbfb->gtt) - drm_gem_object_unreference(&psbfb->gtt->gem); + drm_gem_object_unreference_unlocked(&psbfb->gtt->gem); return 0; }
@@ -784,12 +780,8 @@ void psb_modeset_cleanup(struct drm_device *dev) { struct drm_psb_private *dev_priv = dev->dev_private; if (dev_priv->modeset) { - mutex_lock(&dev->struct_mutex); - drm_kms_helper_poll_fini(dev); psb_fbdev_fini(dev); drm_mode_config_cleanup(dev); - - mutex_unlock(&dev->struct_mutex); } }
Simply forgotten about this when I was doing my general cleansing of simple gem mmap offset functions. There's nothing but core functions called here, and they all have their own protection already.
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/gma500/gem.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index e3bdc8b1c32c..f0357f525f56 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -62,15 +62,10 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev, int ret = 0; struct drm_gem_object *obj;
- mutex_lock(&dev->struct_mutex); - /* GEM does all our handle to object mapping */ obj = drm_gem_object_lookup(dev, file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto unlock; - } - /* What validation is needed here ? */ + if (obj == NULL) + return -ENOENT;
/* Make it mmapable */ ret = drm_gem_create_mmap_offset(obj); @@ -78,9 +73,7 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev, goto out; *offset = drm_vma_node_offset_addr(&obj->vma_node); out: - drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj); return ret; }
There's currently two places where the gma500 fault handler relies upon dev->struct_mutex: - To protect r->mappping - To make sure vm_insert_pfn isn't called concurrently (in which case the 2nd thread would get an error code).
Everything else (specifically psb_gtt_pin) is already protected by some other locks. Hence just create a new driver-private mmap_mutex just for this function.
With this gma500 is complete dev->struct_mutex free!
Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/gma500/gem.c | 4 ++-- drivers/gpu/drm/gma500/gtt.c | 1 + drivers/gpu/drm/gma500/psb_drv.h | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index f0357f525f56..506224b3a0ad 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -182,7 +182,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
/* Make sure we don't parallel update on a fault, nor move or remove something from beneath our feet */ - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev_priv->mmap_mutex);
/* For now the mmap pins the object and it stays pinned. As things stand that will do us no harm */ @@ -208,7 +208,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address, pfn);
fail: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev_priv->mmap_mutex); switch (ret) { case 0: case -ERESTARTSYS: diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index ce015db59dc6..8f69225ce2b4 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -425,6 +425,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)
if (!resume) { mutex_init(&dev_priv->gtt_mutex); + mutex_init(&dev_priv->mmap_mutex); psb_gtt_alloc(dev); }
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h index e21726ecac32..3bd2c726dd61 100644 --- a/drivers/gpu/drm/gma500/psb_drv.h +++ b/drivers/gpu/drm/gma500/psb_drv.h @@ -465,6 +465,8 @@ struct drm_psb_private { struct mutex gtt_mutex; struct resource *gtt_mem; /* Our PCI resource */
+ struct mutex mmap_mutex; + struct psb_mmu_driver *mmu; struct psb_mmu_pd *pf_pd;
Doesn't protect anything at all.
With this patch nouveau is completely dev->struct_mutex free!
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 59f27e774acb..3bae706126bd 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -386,8 +386,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, } }
- mutex_lock(&dev->struct_mutex); - info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { ret = PTR_ERR(info); @@ -426,8 +424,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
- mutex_unlock(&dev->struct_mutex); - if (chan) nouveau_fbcon_accel_init(dev); nouveau_fbcon_zfill(dev, fbcon); @@ -441,7 +437,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, return 0;
out_unlock: - mutex_unlock(&dev->struct_mutex); if (chan) nouveau_bo_vma_del(nvbo, &fbcon->nouveau_fb.vma); nouveau_bo_unmap(nvbo);
Simply forgotten about this when I was doing my general cleansing of simple gem mmap offset functions. There's nothing but core functions called here, and they all have their own protection already.
Aside: DRM_ERROR for userspace controlled input isn't great, but that's for another patch.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 252eb301470c..91d87e7c2a2f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -448,8 +448,6 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_object *obj; int ret = 0;
- mutex_lock(&dev->struct_mutex); - /* * get offset of memory allocated for drm framebuffer. * - this callback would be called by user application @@ -459,16 +457,13 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); - ret = -EINVAL; - goto unlock; + return -EINVAL; }
*offset = drm_vma_node_offset_addr(&obj->vma_node); DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); return ret; }
The sg table isn't refcounted, there's no corresponding locking for unmapping and drm_map_sg is ok with being called concurrently.
So drop the locking since it doesn't protect anything.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 91d87e7c2a2f..a3286a1ec2b1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -378,16 +378,12 @@ int exynos_gem_map_sgt_with_dma(struct drm_device *drm_dev, { int nents;
- mutex_lock(&drm_dev->struct_mutex); - nents = dma_map_sg(drm_dev->dev, sgt->sgl, sgt->nents, dir); if (!nents) { DRM_ERROR("failed to map sgl with dma.\n"); - mutex_unlock(&drm_dev->struct_mutex); return nents; }
- mutex_unlock(&drm_dev->struct_mutex); return 0; }
The only things this protects is reading ->flags and ->size, both of which are invariant over the lifetime of an exynos gem bo. So no locking needed at all (besides that, nothing protects the writers anyway).
Aside: exynos_gem_obj->size is redundant with exynos_gem_obj->base.size and probably should be removed.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index a3286a1ec2b1..dfb3bfee1b63 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -352,12 +352,9 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, struct drm_exynos_gem_info *args = data; struct drm_gem_object *obj;
- mutex_lock(&dev->struct_mutex); - obj = drm_gem_object_lookup(dev, file_priv, args->handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); - mutex_unlock(&dev->struct_mutex); return -EINVAL; }
@@ -367,7 +364,6 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, args->size = exynos_gem->size;
drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex);
return 0; }
Hi,
On 19 November 2015 at 16:46, Daniel Vetter daniel.vetter@ffwll.ch wrote:
@@ -367,7 +364,6 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, args->size = exynos_gem->size;
drm_gem_object_unreference(obj);
mutex_unlock(&dev->struct_mutex);
drm_gem_object_unreference_unlocked, surely ...
Cheers, Daniel
On Thu, Nov 19, 2015 at 04:50:11PM +0000, Daniel Stone wrote:
Hi,
On 19 November 2015 at 16:46, Daniel Vetter daniel.vetter@ffwll.ch wrote:
@@ -367,7 +364,6 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, args->size = exynos_gem->size;
drm_gem_object_unreference(obj);
mutex_unlock(&dev->struct_mutex);drm_gem_object_unreference_unlocked, surely ...
Yeah, and then I go and do a grep for this and spot piles more offenders. -Daniel
Doesn't protect anything at all, and probably just here because a long time ago dev->struct_mutex was required to allocate gem objects.
With this patch exynos is completely struct_mutex free!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index f6118baa8e3e..e1f7abaa61df 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -138,8 +138,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
- mutex_lock(&dev->struct_mutex); - size = mode_cmd.pitches[0] * mode_cmd.height;
exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size); @@ -154,10 +152,8 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, size); }
- if (IS_ERR(exynos_gem)) { - ret = PTR_ERR(exynos_gem); - goto out; - } + if (IS_ERR(exynos_gem)) + return PTR_ERR(exynos_gem);
exynos_fbdev->exynos_gem = exynos_gem;
@@ -173,7 +169,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, if (ret < 0) goto err_destroy_framebuffer;
- mutex_unlock(&dev->struct_mutex); return ret;
err_destroy_framebuffer: @@ -181,13 +176,12 @@ err_destroy_framebuffer: err_destroy_gem: exynos_drm_gem_destroy(exynos_gem);
-/* - * if failed, all resources allocated above would be released by - * drm_mode_config_cleanup() when drm_load() had been called prior - * to any specific driver such as fimd or hdmi driver. - */ -out: - mutex_unlock(&dev->struct_mutex); + /* + * if failed, all resources allocated above would be released by + * drm_mode_config_cleanup() when drm_load() had been called prior + * to any specific driver such as fimd or hdmi driver. + */ + return ret; }
The offset manager already checks for existing offsets internally, while holding suitable locks. We can drop this check.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 02ae60c395b7..63026d4324ad 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -208,11 +208,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev, goto unlock; }
- if (!drm_vma_node_has_offset(&obj->vma_node)) { - ret = drm_gem_create_mmap_offset(obj); - if (ret) - goto unref; - } + ret = drm_gem_create_mmap_offset(obj); + if (ret) + goto unref;
BUG_ON(!obj->filp);
vgem doesn't have a shrinker or anything like that and drops backing storage only at object_free time. There's no use in trying to be clever and allocating backing storage delayed, it only causes trouble by requiring locking.
Instead grab pages when we allocate the object right away.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 63026d4324ad..1a609347236b 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -154,6 +154,10 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, if (err) goto out;
+ ret = vgem_gem_get_pages(to_vgem_bo(obj)); + if (ret) + goto out; + err = drm_gem_handle_create(file, gem_object, handle); if (err) goto handle_out; @@ -216,16 +220,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
obj->filp->private_data = obj;
- ret = vgem_gem_get_pages(to_vgem_bo(obj)); - if (ret) - goto fail_get_pages; - *offset = drm_vma_node_offset_addr(&obj->vma_node);
- goto unref; - -fail_get_pages: - drm_gem_free_mmap_offset(obj); unref: drm_gem_object_unreference(obj); unlock:
With the previous two changes it doesn't protect anything any more.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 1a609347236b..b525208375aa 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -103,12 +103,8 @@ static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) if (page_offset > num_pages) return VM_FAULT_SIGBUS;
- mutex_lock(&dev->struct_mutex); - ret = vm_insert_page(vma, (unsigned long)vmf->virtual_address, obj->pages[page_offset]); - - mutex_unlock(&dev->struct_mutex); switch (ret) { case 0: return VM_FAULT_NOPAGE; @@ -205,12 +201,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev, int ret = 0; struct drm_gem_object *obj;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (!obj) { - ret = -ENOENT; - goto unlock; - } + if (!obj) + return -ENOENT;
ret = drm_gem_create_mmap_offset(obj); if (ret) @@ -224,8 +217,7 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
unref: drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); + return ret; }
It's racy, creating mmap offsets is a slowpath, so better to remove it to avoid drivers doing broken things.
The only user is i915, and it's ok there because everything (well almost) is protected by dev->struct_mutex in i915-gem.
While at it add a note in the create_mmap_offset kerneldoc that drivers must release it again. And then I also noticed that drm_gem_object_release entirely lacks kerneldoc.
Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_gem.c | 17 +++++++++++++++++ drivers/gpu/drm/i915/i915_gem.c | 3 --- include/drm/drm_vma_manager.h | 15 +-------------- 3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e10bba4468b..5c5001171dd3 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -392,6 +392,10 @@ EXPORT_SYMBOL(drm_gem_handle_create); * @obj: obj in question * * This routine frees fake offsets allocated by drm_gem_create_mmap_offset(). + * + * Note that drm_gem_object_release() already calls this function, so drivers + * don't have to take care of releasing the mmap offset themselves when freeing + * the GEM object. */ void drm_gem_free_mmap_offset(struct drm_gem_object *obj) @@ -415,6 +419,9 @@ EXPORT_SYMBOL(drm_gem_free_mmap_offset); * This routine allocates and attaches a fake offset for @obj, in cases where * the virtual size differs from the physical size (ie. obj->size). Otherwise * just use drm_gem_create_mmap_offset(). + * + * This function is idempotent and handles an already allocated mmap offset + * transparently. Drivers do not need to check for this case. */ int drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size) @@ -436,6 +443,9 @@ EXPORT_SYMBOL(drm_gem_create_mmap_offset_size); * structures. * * This routine allocates and attaches a fake offset for @obj. + * + * Drivers can call drm_gem_free_mmap_offset() before freeing @obj to release + * the fake offset again. */ int drm_gem_create_mmap_offset(struct drm_gem_object *obj) { @@ -754,6 +764,13 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) idr_destroy(&file_private->object_idr); }
+/** + * drm_gem_object_release - release GEM buffer object resources + * @obj: GEM buffer object + * + * This releases any structures and resources used by @obj and is the invers of + * drm_gem_object_init(). + */ void drm_gem_object_release(struct drm_gem_object *obj) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 33adc8f8ab20..f10a5d57377e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1972,9 +1972,6 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) struct drm_i915_private *dev_priv = obj->base.dev->dev_private; int ret;
- if (drm_vma_node_has_offset(&obj->base.vma_node)) - return 0; - dev_priv->mm.shrinker_no_lock_stealing = true;
ret = drm_gem_create_mmap_offset(&obj->base); diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h index 2f63dd5e05eb..06ea8e077ec2 100644 --- a/include/drm/drm_vma_manager.h +++ b/include/drm/drm_vma_manager.h @@ -176,19 +176,6 @@ static inline unsigned long drm_vma_node_size(struct drm_vma_offset_node *node) }
/** - * drm_vma_node_has_offset() - Check whether node is added to offset manager - * @node: Node to be checked - * - * RETURNS: - * true iff the node was previously allocated an offset and added to - * an vma offset manager. - */ -static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node) -{ - return drm_mm_node_allocated(&node->vm_node); -} - -/** * drm_vma_node_offset_addr() - Return sanitized offset for user-space mmaps * @node: Linked offset node * @@ -220,7 +207,7 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node) static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node, struct address_space *file_mapping) { - if (drm_vma_node_has_offset(node)) + if (drm_mm_node_allocated(&node->vm_node)) unmap_mapping_range(file_mapping, drm_vma_node_offset_addr(node), drm_vma_node_size(node) << PAGE_SHIFT, 1);
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force.
v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call.
v3: Restore the accidentally removed forced-probe for echo "detect" > force.
Cc: Chris Wilson chris@chris-wilson.co.uk Reported-by: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_probe_helper.c | 46 +++++++++++++++++++------------------- drivers/gpu/drm/drm_sysfs.c | 38 +++++++++++-------------------- 2 files changed, 36 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index a214a4a93b03..b7bdf12c54a5 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -147,6 +147,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect list_for_each_entry(mode, &connector->modes, head) mode->status = MODE_UNVERIFIED;
+ old_status = connector->status; + if (connector->force) { if (connector->force == DRM_FORCE_ON || connector->force == DRM_FORCE_ON_DIGITAL) @@ -156,33 +158,31 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect if (connector->funcs->force) connector->funcs->force(connector); } else { - old_status = connector->status; - connector->status = connector->funcs->detect(connector, true); + } + + /* + * Normally either the driver's hpd code or the poll loop should + * pick up any changes and fire the hotplug event. But if + * userspace sneaks in a probe, we might miss a change. Hence + * check here, and if anything changed start the hotplug code. + */ + if (old_status != connector->status) { + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + connector->base.id, + connector->name, + old_status, connector->status);
/* - * Normally either the driver's hpd code or the poll loop should - * pick up any changes and fire the hotplug event. But if - * userspace sneaks in a probe, we might miss a change. Hence - * check here, and if anything changed start the hotplug code. + * The hotplug event code might call into the fb + * helpers, and so expects that we do not hold any + * locks. Fire up the poll struct instead, it will + * disable itself again. */ - if (old_status != connector->status) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", - connector->base.id, - connector->name, - old_status, connector->status); - - /* - * The hotplug event code might call into the fb - * helpers, and so expects that we do not hold any - * locks. Fire up the poll struct instead, it will - * disable itself again. - */ - dev->mode_config.delayed_event = true; - if (dev->mode_config.poll_enabled) - schedule_delayed_work(&dev->mode_config.output_poll_work, - 0); - } + dev->mode_config.delayed_event = true; + if (dev->mode_config.poll_enabled) + schedule_delayed_work(&dev->mode_config.output_poll_work, + 0); }
/* Re-enable polling in case the global poll config changed. */ diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev; - enum drm_connector_status old_status; + enum drm_connector_force old_force; int ret;
ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret;
- old_status = connector->status; + old_force = connector->force;
- if (sysfs_streq(buf, "detect")) { + if (sysfs_streq(buf, "detect")) connector->force = 0; - connector->status = connector->funcs->detect(connector, true); - } else if (sysfs_streq(buf, "on")) { + else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON; - } else if (sysfs_streq(buf, "on-digital")) { + else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL; - } else if (sysfs_streq(buf, "off")) { + else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF; - } else + else ret = -EINVAL;
- if (ret == 0 && connector->force) { - if (connector->force == DRM_FORCE_ON || - connector->force == DRM_FORCE_ON_DIGITAL) - connector->status = connector_status_connected; - else - connector->status = connector_status_disconnected; - if (connector->funcs->force) - connector->funcs->force(connector); - } - - if (old_status != connector->status) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + if (old_force != connector->force || !connector->force) { + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name, - old_status, connector->status); + old_force, connector->force);
- dev->mode_config.delayed_event = true; - if (dev->mode_config.poll_enabled) - schedule_delayed_work(&dev->mode_config.output_poll_work, - 0); + connector->funcs->fill_modes(connector, + dev->mode_config.max_width, + dev->mode_config.max_height); }
mutex_unlock(&dev->mode_config.mutex);
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote:
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force.
v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call.
v3: Restore the accidentally removed forced-probe for echo "detect" > force.
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
enum drm_connector_force old_force; int ret;
ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret;
- old_status = connector->status;
- old_force = connector->force;
- if (sysfs_streq(buf, "detect")) {
- if (sysfs_streq(buf, "detect")) connector->force = 0;
connector->status = connector->funcs->detect(connector, true);- } else if (sysfs_streq(buf, "on")) {
- else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON;
- } else if (sysfs_streq(buf, "on-digital")) {
- else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL;
- } else if (sysfs_streq(buf, "off")) {
- else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF;
- } else
- else ret = -EINVAL;
- if (ret == 0 && connector->force) {
if (connector->force == DRM_FORCE_ON ||connector->force == DRM_FORCE_ON_DIGITAL)connector->status = connector_status_connected;elseconnector->status = connector_status_disconnected;if (connector->funcs->force)connector->funcs->force(connector);- }
- if (old_status != connector->status) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
- if (old_force != connector->force || !connector->force) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name,
old_status, connector->status);
old_force, connector->force);
dev->mode_config.delayed_event = true;if (dev->mode_config.poll_enabled)schedule_delayed_work(&dev->mode_config.output_poll_work,0);
connector->funcs->fill_modes(connector,dev->mode_config.max_width,dev->mode_config.max_height);
This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle). -Chris
On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote:
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote:
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force.
v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call.
v3: Restore the accidentally removed forced-probe for echo "detect" > force.
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
enum drm_connector_force old_force; int ret;
ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret;
- old_status = connector->status;
- old_force = connector->force;
- if (sysfs_streq(buf, "detect")) {
- if (sysfs_streq(buf, "detect")) connector->force = 0;
connector->status = connector->funcs->detect(connector, true);- } else if (sysfs_streq(buf, "on")) {
- else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON;
- } else if (sysfs_streq(buf, "on-digital")) {
- else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL;
- } else if (sysfs_streq(buf, "off")) {
- else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF;
- } else
- else ret = -EINVAL;
- if (ret == 0 && connector->force) {
if (connector->force == DRM_FORCE_ON ||connector->force == DRM_FORCE_ON_DIGITAL)connector->status = connector_status_connected;elseconnector->status = connector_status_disconnected;if (connector->funcs->force)connector->funcs->force(connector);- }
- if (old_status != connector->status) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
- if (old_force != connector->force || !connector->force) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name,
old_status, connector->status);
old_force, connector->force);
dev->mode_config.delayed_event = true;if (dev->mode_config.poll_enabled)schedule_delayed_work(&dev->mode_config.output_poll_work,0);
connector->funcs->fill_modes(connector,dev->mode_config.max_width,dev->mode_config.max_height);This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle).
->fill_modes = drm_helper_probe_single_connector_modes which then calls ->detect. By going this way we avoid duplicating the "send uevent if anything changed" logic all over the place, and ->detect becomes purely a helper internal callback to get at the mode list for hotpluggeable outputs.
Yes this means that ->detect should become a helper function, but that's quite an invasive change. -Daniel
On Fri, Nov 20, 2015 at 09:11:00AM +0100, Daniel Vetter wrote:
On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote:
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote:
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force.
v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call.
v3: Restore the accidentally removed forced-probe for echo "detect" > force.
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
enum drm_connector_force old_force; int ret;
ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret;
- old_status = connector->status;
- old_force = connector->force;
- if (sysfs_streq(buf, "detect")) {
- if (sysfs_streq(buf, "detect")) connector->force = 0;
connector->status = connector->funcs->detect(connector, true);- } else if (sysfs_streq(buf, "on")) {
- else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON;
- } else if (sysfs_streq(buf, "on-digital")) {
- else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL;
- } else if (sysfs_streq(buf, "off")) {
- else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF;
- } else
- else ret = -EINVAL;
- if (ret == 0 && connector->force) {
if (connector->force == DRM_FORCE_ON ||connector->force == DRM_FORCE_ON_DIGITAL)connector->status = connector_status_connected;elseconnector->status = connector_status_disconnected;if (connector->funcs->force)connector->funcs->force(connector);- }
- if (old_status != connector->status) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
- if (old_force != connector->force || !connector->force) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name,
old_status, connector->status);
old_force, connector->force);
dev->mode_config.delayed_event = true;if (dev->mode_config.poll_enabled)schedule_delayed_work(&dev->mode_config.output_poll_work,0);
connector->funcs->fill_modes(connector,dev->mode_config.max_width,dev->mode_config.max_height);This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle).
->fill_modes = drm_helper_probe_single_connector_modes which then calls ->detect. By going this way we avoid duplicating the "send uevent if anything changed" logic all over the place, and ->detect becomes purely a helper internal callback to get at the mode list for hotpluggeable outputs.
Ok, that struck me as a little surprising - I was thinking of lower level get_modes apparently.
With that resolved, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Yes this means that ->detect should become a helper function, but that's quite an invasive change.
And something like .fill_modes -> .probe (after removing .detect). -Chris
On Fri, Nov 20, 2015 at 09:25:14AM +0000, Chris Wilson wrote:
On Fri, Nov 20, 2015 at 09:11:00AM +0100, Daniel Vetter wrote:
On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote:
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote:
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force.
v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call.
v3: Restore the accidentally removed forced-probe for echo "detect" > force.
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
enum drm_connector_force old_force; int ret;
ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret;
- old_status = connector->status;
- old_force = connector->force;
- if (sysfs_streq(buf, "detect")) {
- if (sysfs_streq(buf, "detect")) connector->force = 0;
connector->status = connector->funcs->detect(connector, true);- } else if (sysfs_streq(buf, "on")) {
- else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON;
- } else if (sysfs_streq(buf, "on-digital")) {
- else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL;
- } else if (sysfs_streq(buf, "off")) {
- else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF;
- } else
- else ret = -EINVAL;
- if (ret == 0 && connector->force) {
if (connector->force == DRM_FORCE_ON ||connector->force == DRM_FORCE_ON_DIGITAL)connector->status = connector_status_connected;elseconnector->status = connector_status_disconnected;if (connector->funcs->force)connector->funcs->force(connector);- }
- if (old_status != connector->status) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
- if (old_force != connector->force || !connector->force) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name,
old_status, connector->status);
old_force, connector->force);
dev->mode_config.delayed_event = true;if (dev->mode_config.poll_enabled)schedule_delayed_work(&dev->mode_config.output_poll_work,0);
connector->funcs->fill_modes(connector,dev->mode_config.max_width,dev->mode_config.max_height);This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle).
->fill_modes = drm_helper_probe_single_connector_modes which then calls ->detect. By going this way we avoid duplicating the "send uevent if anything changed" logic all over the place, and ->detect becomes purely a helper internal callback to get at the mode list for hotpluggeable outputs.
Ok, that struck me as a little surprising - I was thinking of lower level get_modes apparently.
With that resolved, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Applied to drm-misc, thanks for the review.
Yes this means that ->detect should become a helper function, but that's quite an invasive change.
And something like .fill_modes -> .probe (after removing .detect).
Hm, not sure. For panels we never really probe anything, the important bit is (at least for the caller in drm_crtc.c) that it fills in the connectore->modes list. Given that I think the current name is okish. -Daniel
On Tue, Nov 24, 2015 at 11:51:09AM +0100, Daniel Vetter wrote:
On Fri, Nov 20, 2015 at 09:25:14AM +0000, Chris Wilson wrote:
And something like .fill_modes -> .probe (after removing .detect).
Hm, not sure. For panels we never really probe anything, the important bit is (at least for the caller in drm_crtc.c) that it fills in the connectore->modes list. Given that I think the current name is okish.
imo .fill_modes() does not imply that it communicates with the output, unlike say .detect(), .probe(), or .get_modes(). -Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Stone -
Daniel Vetter -
Daniel Vetter