- config_cleanup was confused: It claimed that callers need to hold the modeset lock, but the connector|encoder_cleanup helpers grabbed that themselves (note that crtc_cleanup did _not_ grab the modeset lock). Which resulted in all drivers _not_ hodling the lock. Since this is for single-threaded cleanup code, drop the requirement from docs and also drop the lock_grabbing from all _cleanup functions.

- Kill the LOCKING section in the doctype, since clearly we're not good enough to keep them up-to-date. And misleading locking documentation is worse than useless (see e.g. the comment in the vmgfx driver about the cleanup mess). And since for most functions the very first line either grabs the lock or has a WARN_ON(!locked) the documentation doesn't really add anything.

- Instead put in some effort into explaining the only two special cases a bit better: config_init and config_cleanup are both called from single-threaded setup/teardown code, so don't do any locking. It's the driver's job though to enforce this.

- Where lacking, add a WARN_ON(!is_locked). Not many places though, since locking around fbdev setup/teardown is through-roughly screwed up, and so will break almost every single WARN annotation I've tried to add.

- Add a drm_modeset_is_locked helper - the Grate Modset Locking Rework will use the compiler to assist in the big reorg by renaming the mode lock, so start encapsulating things. Unfortunately this ended up in the "wrong" header file since it needs the definition of struct drm_device.

v2: Drop most WARNS again - we hit them all over the place, mostly in the setup and teardown sequences. And trying to fix it up leads to nice deadlocks, since the locking in the setup code is really inconsistent.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 105 ++++++-------------------------------------- include/drm/drmP.h | 5 +++ 2 files changed, 18 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f2d667b..b970e41 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -208,8 +208,6 @@ char *drm_get_connector_status_name(enum drm_connector_status status) * @ptr: object pointer, used to generate unique ID * @type: object type * - * LOCKING: - * * Create a unique identifier based on @ptr in @dev's identifier space. Used * for tracking modes, CRTCs and connectors. * @@ -247,9 +245,6 @@ again: * @dev: DRM device * @id: ID to free * - * LOCKING: - * Caller must hold DRM mode_config lock. - * * Free @id from @dev's unique identifier pool. */ static void drm_mode_object_put(struct drm_device *dev, @@ -279,9 +274,6 @@ EXPORT_SYMBOL(drm_mode_object_find); * drm_framebuffer_init - initialize a framebuffer * @dev: DRM device * - * LOCKING: - * Caller must hold mode config lock. - * * Allocates an ID for the framebuffer's parent mode object, sets its mode * functions & device file and adds it to the master fd list. * @@ -317,15 +309,12 @@ static void drm_framebuffer_free(struct kref *kref)

/** * drm_framebuffer_unreference - unref a framebuffer - * - * LOCKING: - * Caller must hold mode config lock. */ void drm_framebuffer_unreference(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev; DRM_DEBUG("FB ID: %d\n", fb->base.id); - WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); + WARN_ON(!drm_modeset_is_locked(dev)); kref_put(&fb->refcount, drm_framebuffer_free); } EXPORT_SYMBOL(drm_framebuffer_unreference); @@ -344,15 +333,13 @@ EXPORT_SYMBOL(drm_framebuffer_reference); * drm_framebuffer_cleanup - remove a framebuffer object * @fb: framebuffer to remove * - * LOCKING: - * Caller must hold mode config lock. - * * Scans all the CRTCs in @dev's mode_config. If they're using @fb, removes * it, setting it to NULL. */ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev; + /* * This could be moved to drm_framebuffer_remove(), but for * debugging is nice to keep around the list of fb's that are @@ -370,9 +357,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); * drm_framebuffer_remove - remove and unreference a framebuffer object * @fb: framebuffer to remove * - * LOCKING: - * Caller must hold mode config lock. - * * Scans all the CRTCs and planes in @dev's mode_config. If they're * using @fb, removes it, setting it to NULL. */ @@ -384,6 +368,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) struct drm_mode_set set; int ret;

+ WARN_ON(!drm_modeset_is_locked(dev)); + /* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (crtc->fb == fb) { @@ -421,9 +407,6 @@ EXPORT_SYMBOL(drm_framebuffer_remove); * @crtc: CRTC object to init * @funcs: callbacks for the new CRTC * - * LOCKING: - * Takes mode_config lock. - * * Inits a new object created as base part of an driver crtc object. * * RETURNS: @@ -460,9 +443,6 @@ EXPORT_SYMBOL(drm_crtc_init); * drm_crtc_cleanup - Cleans up the core crtc usage. * @crtc: CRTC to cleanup * - * LOCKING: - * Caller must hold mode config lock. - * * Cleanup @crtc. Removes from drm modesetting space * does NOT free object, caller does that. */ @@ -484,9 +464,6 @@ EXPORT_SYMBOL(drm_crtc_cleanup); * @connector: connector the new mode * @mode: mode data * - * LOCKING: - * Caller must hold mode config lock. - * * Add @mode to @connector's mode list for later use. */ void drm_mode_probed_add(struct drm_connector *connector, @@ -501,9 +478,6 @@ EXPORT_SYMBOL(drm_mode_probed_add); * @connector: connector list to modify * @mode: mode to remove * - * LOCKING: - * Caller must hold mode config lock. - * * Remove @mode from @connector's mode list, then free it. */ void drm_mode_remove(struct drm_connector *connector, @@ -521,9 +495,6 @@ EXPORT_SYMBOL(drm_mode_remove); * @funcs: callbacks for this connector * @name: user visible name of the connector * - * LOCKING: - * Takes mode config lock. - * * Initialises a preallocated connector. Connectors should be * subclassed as part of driver connector objects. * @@ -577,9 +548,6 @@ EXPORT_SYMBOL(drm_connector_init); * drm_connector_cleanup - cleans up an initialised connector * @connector: connector to cleanup * - * LOCKING: - * Takes mode config lock. - * * Cleans up the connector but doesn't free the object. */ void drm_connector_cleanup(struct drm_connector *connector) @@ -596,11 +564,9 @@ void drm_connector_cleanup(struct drm_connector *connector) list_for_each_entry_safe(mode, t, &connector->user_modes, head) drm_mode_remove(connector, mode);

- mutex_lock(&dev->mode_config.mutex); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; - mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_connector_cleanup);

@@ -721,9 +687,6 @@ EXPORT_SYMBOL(drm_plane_cleanup); * drm_mode_create - create a new display mode * @dev: DRM device * - * LOCKING: - * Caller must hold DRM mode_config lock. - * * Create a new drm_display_mode, give it an ID, and return it. * * RETURNS: @@ -751,9 +714,6 @@ EXPORT_SYMBOL(drm_mode_create); * @dev: DRM device * @mode: mode to remove * - * LOCKING: - * Caller must hold mode config lock. - * * Free @mode's unique identifier, then free it. */ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode) @@ -978,11 +938,13 @@ EXPORT_SYMBOL(drm_mode_create_dirty_info_property); * drm_mode_config_init - initialize DRM mode_configuration structure * @dev: DRM device * - * LOCKING: - * None, should happen single threaded at init time. - * * Initialize @dev's mode_config structure, used for tracking the graphics * configuration of @dev. + * + * Since this initializes the modeset locks, no locking is possible. Which is no + * problem, since this should happen single threaded at init time. It is the + * driver's problem to ensure this guarantee. + * */ void drm_mode_config_init(struct drm_device *dev) { @@ -1057,12 +1019,13 @@ EXPORT_SYMBOL(drm_mode_group_init_legacy_group); * drm_mode_config_cleanup - free up DRM mode_config info * @dev: DRM device * - * LOCKING: - * Caller must hold mode config lock. - * * Free up all the connectors and CRTCs associated with this DRM device, then * free up the framebuffers and associated buffer objects. * + * Note that since this /should/ happen single-threaded at driver/device + * teardown time, no locking is required. It's the driver's job to ensure that + * this guarantee actually holds true. + * * FIXME: cleanup any dangling user buffer objects too */ void drm_mode_config_cleanup(struct drm_device *dev) @@ -1112,9 +1075,6 @@ EXPORT_SYMBOL(drm_mode_config_cleanup); * @out: drm_mode_modeinfo struct to return to the user * @in: drm_display_mode to use * - * LOCKING: - * None. - * * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to * the user. */ @@ -1151,9 +1111,6 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, * @out: drm_display_mode to return to the user * @in: drm_mode_modeinfo to use * - * LOCKING: - * None. - * * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to * the caller. * @@ -1193,9 +1150,6 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out, * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Construct a set of configuration description structures and return * them to the user, including CRTC, connector and framebuffer configuration. * @@ -1381,9 +1335,6 @@ out: * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Construct a CRTC configuration structure to return to the user. * * Called by the user via ioctl. @@ -1441,9 +1392,6 @@ out: * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Construct a connector configuration structure to return to the user. * * Called by the user via ioctl. @@ -1618,9 +1566,6 @@ out: * @data: ioctl data * @file_priv: DRM file info * - * LOCKING: - * Takes mode config lock. - * * Return an plane count and set of IDs. */ int drm_mode_getplane_res(struct drm_device *dev, void *data, @@ -1667,9 +1612,6 @@ out: * @data: ioctl data * @file_priv: DRM file info * - * LOCKING: - * Takes mode config lock. - * * Return plane info, including formats supported, gamma size, any * current fb, etc. */ @@ -1735,9 +1677,6 @@ out: * @data: ioctl data* * @file_prive: DRM file info * - * LOCKING: - * Takes mode config lock. - * * Set plane info, including placement, fb, scaling, and other factors. * Or pass a NULL fb to disable. */ @@ -1867,9 +1806,6 @@ out: * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Build a new CRTC configuration based on user request. * * Called by the user via ioctl. @@ -2125,9 +2061,6 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format); * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Add a new FB to the specified CRTC, given a user request. * * Called by the user via ioctl. @@ -2309,9 +2242,6 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Add a new FB to the specified CRTC, given a user request with format. * * Called by the user via ioctl. @@ -2375,9 +2305,6 @@ out: * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Remove the FB specified by the user. * * Called by the user via ioctl. @@ -2430,9 +2357,6 @@ out: * @cmd: cmd from ioctl * @arg: arg from ioctl * - * LOCKING: - * Takes mode config lock. - * * Lookup the FB given its ID and return info about it. * * Called by the user via ioctl. @@ -2549,9 +2473,6 @@ out_err1: * drm_fb_release - remove and free the FBs on this file * @filp: file * from the ioctl * - * LOCKING: - * Takes mode config lock. - * * Destroy all the FBs associated with @filp. * * Called by the user via ioctl. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index fad21c9..3c609ab 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1276,6 +1276,11 @@ static inline int drm_device_is_unplugged(struct drm_device *dev) return ret; }

+static inline bool drm_modeset_is_locked(struct drm_device *dev) +{ + return mutex_is_locked(&dev->mode_config.mutex); +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/

With more fine-grained locking we can no longer rely on the big mode_config lock to prevent concurrent access to mode resources like framebuffers. Instead a framebuffer becomes accessible to other threads as soon as it is added to the relevant lookup structures. Hence it needs to be fully set up by the time drivers call drm_framebuffer_init.

This patch here is the drivers part of that reorg. Nothing really fancy going on safe for three special cases.

- exynos needs to be careful to properly unref all handles. - nouveau gets a resource leak fixed for free: one of the error cases didn't cleanup the framebuffer, which is now moot since the framebuffer is only registered once it is fully set up. - vmwgfx requires a slight reordering of operations, I'm hoping I didn't break anything (but it's refcount management only, so should be safe).

v2: Split out exynos, since it's a bit more hairy than expected.

v3: Drop bogus cirrus hunk noticed by Richard Wilbur.

v4: Split out vmwgfx since there's a small change in return values.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_main.c | 4 ++-- drivers/gpu/drm/cirrus/cirrus_main.c | 4 ++-- drivers/gpu/drm/drm_fb_cma_helper.c | 10 +++++----- drivers/gpu/drm/gma500/framebuffer.c | 4 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +++-- drivers/gpu/drm/mgag200/mgag200_main.c | 8 +++++--- drivers/gpu/drm/nouveau/nouveau_display.c | 10 +++++----- drivers/gpu/drm/radeon/radeon_display.c | 2 +- drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/staging/omapdrm/omap_fb.c | 16 ++++++++-------- 10 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index f668e6c..d5ba709 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -266,13 +266,13 @@ int ast_framebuffer_init(struct drm_device *dev, { int ret;

+ drm_helper_mode_fill_fb_struct(&ast_fb->base, mode_cmd); + ast_fb->obj = obj; ret = drm_framebuffer_init(dev, &ast_fb->base, &ast_fb_funcs); if (ret) { DRM_ERROR("framebuffer init failed %d\n", ret); return ret; } - drm_helper_mode_fill_fb_struct(&ast_fb->base, mode_cmd); - ast_fb->obj = obj; return 0; }

diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index 6a9b12e..364474c 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -42,13 +42,13 @@ int cirrus_framebuffer_init(struct drm_device *dev, { int ret;

+ drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd); + gfb->obj = obj; ret = drm_framebuffer_init(dev, &gfb->base, &cirrus_fb_funcs); if (ret) { DRM_ERROR("drm_framebuffer_init failed: %d\n", ret); return ret; } - drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd); - gfb->obj = obj; return 0; }

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index fd9d0af..e1e0cb0 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -85,6 +85,11 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev, if (!fb_cma) return ERR_PTR(-ENOMEM);

+ drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd); + + for (i = 0; i < num_planes; i++) + fb_cma->obj[i] = obj[i]; + ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs); if (ret) { dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret); @@ -92,11 +97,6 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev, return ERR_PTR(ret); }

- drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd); - - for (i = 0; i < num_planes; i++) - fb_cma->obj[i] = obj[i]; - return fb_cma; }

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index afded54..38e7e75 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -260,13 +260,13 @@ static int psb_framebuffer_init(struct drm_device *dev, default: return -EINVAL; } + drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd); + fb->gtt = gt; ret = drm_framebuffer_init(dev, &fb->base, &psb_fb_funcs); if (ret) { dev_err(dev->dev, "framebuffer init failed: %d\n", ret); return ret; } - drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd); - fb->gtt = gt; return 0; }

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cf0c713..b2af790 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8387,14 +8387,15 @@ int intel_framebuffer_init(struct drm_device *dev, if (mode_cmd->offsets[0] != 0) return -EINVAL;

+ drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd); + intel_fb->obj = obj; + ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs); if (ret) { DRM_ERROR("framebuffer init failed %d\n", ret); return ret; }

- drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd); - intel_fb->obj = obj; return 0; }

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 70dd3c5..266438a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -40,13 +40,15 @@ int mgag200_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj) { - int ret = drm_framebuffer_init(dev, &gfb->base, &mga_fb_funcs); + int ret; + + drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd); + gfb->obj = obj; + ret = drm_framebuffer_init(dev, &gfb->base, &mga_fb_funcs); if (ret) { DRM_ERROR("drm_framebuffer_init failed: %d\n", ret); return ret; } - drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd); - gfb->obj = obj; return 0; }

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index e4188f2..2591ff2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -78,11 +78,6 @@ nouveau_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb = &nv_fb->base; int ret;

- ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs); - if (ret) { - return ret; - } - drm_helper_mode_fill_fb_struct(fb, mode_cmd); nv_fb->nvbo = nvbo;

@@ -125,6 +120,11 @@ nouveau_framebuffer_init(struct drm_device *dev, } }

+ ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs); + if (ret) { + return ret; + } + return 0; }

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 1da2386..12ec3ef 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1089,12 +1089,12 @@ radeon_framebuffer_init(struct drm_device *dev, { int ret; rfb->obj = obj; + drm_helper_mode_fill_fb_struct(&rfb->base, mode_cmd); ret = drm_framebuffer_init(dev, &rfb->base, &radeon_fb_funcs); if (ret) { rfb->obj = NULL; return ret; } - drm_helper_mode_fill_fb_struct(&rfb->base, mode_cmd); return 0; }

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index d4ab3be..829c4a7 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -435,8 +435,8 @@ udl_framebuffer_init(struct drm_device *dev, int ret;

ufb->obj = obj; - ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs); drm_helper_mode_fill_fb_struct(&ufb->base, mode_cmd); + ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs); return ret; }

diff --git a/drivers/staging/omapdrm/omap_fb.c b/drivers/staging/omapdrm/omap_fb.c index 09028e9..bf6421f 100644 --- a/drivers/staging/omapdrm/omap_fb.c +++ b/drivers/staging/omapdrm/omap_fb.c @@ -424,14 +424,6 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, }

fb = &omap_fb->base; - ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs); - if (ret) { - dev_err(dev->dev, "framebuffer init failed: %d\n", ret); - goto fail; - } - - DBG("create: FB ID: %d (%p)", fb->base.id, fb); - omap_fb->format = format;

for (i = 0; i < n; i++) { @@ -462,6 +454,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,

drm_helper_mode_fill_fb_struct(fb, mode_cmd);

+ ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs); + if (ret) { + dev_err(dev->dev, "framebuffer init failed: %d\n", ret); + goto fail; + } + + DBG("create: FB ID: %d (%p)", fb->base.id, fb); + return fb;

fail:

vmwgfx has an oddity, when failing to reference the surface it'll return 0, since that's what the successfull drm_framebuffer_init will leave behind in ret. Fix this up by returning -EINVAL.

Split out from all the other driver updates due to the above tiny semantic change. Shouldn't matter though since the reference grabbing seemingly can't fail.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 5474394..edc9792 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -681,14 +681,10 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, goto out_err1; }

- ret = drm_framebuffer_init(dev, &vfbs->base.base, - &vmw_framebuffer_surface_funcs); - if (ret) - goto out_err2; - if (!vmw_surface_reference(surface)) { DRM_ERROR("failed to reference surface %p\n", surface); - goto out_err3; + ret = -EINVAL; + goto out_err2; }

/* XXX get the first 3 from the surface info */ @@ -707,10 +703,15 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,

*out = &vfbs->base;

+ ret = drm_framebuffer_init(dev, &vfbs->base.base, + &vmw_framebuffer_surface_funcs); + if (ret) + goto out_err3; + return 0;

out_err3: - drm_framebuffer_cleanup(&vfbs->base.base); + vmw_surface_unreference(&surface); out_err2: kfree(vfbs); out_err1: @@ -1053,14 +1054,10 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, goto out_err1; }

- ret = drm_framebuffer_init(dev, &vfbd->base.base, - &vmw_framebuffer_dmabuf_funcs); - if (ret) - goto out_err2; - if (!vmw_dmabuf_reference(dmabuf)) { DRM_ERROR("failed to reference dmabuf %p\n", dmabuf); - goto out_err3; + ret = -EINVAL; + goto out_err2; }

vfbd->base.base.bits_per_pixel = mode_cmd->bpp; @@ -1077,10 +1074,15 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, vfbd->base.user_handle = mode_cmd->handle; *out = &vfbd->base;

+ ret = drm_framebuffer_init(dev, &vfbd->base.base, + &vmw_framebuffer_dmabuf_funcs); + if (ret) + goto out_err3; + return 0;

out_err3: - drm_framebuffer_cleanup(&vfbd->base.base); + vmw_dmabuf_unreference(&dmabuf); out_err2: kfree(vfbd); out_err1:

With per-crtc locks modeset operations can run in parallel, and the cursor code uses the device-global evo master channel for hw frobbing. But the pageflip code can also sync with the master under some circumstances. Hence just wrap things up in a mutex to ensure that pushbuf access doesn't intermingle.

The approach here is a bit overkill since the per-crtc channels used to schedule the pageflips could probably be used without this pushbuf locking, but I'm not familiar enough with the nouveau codebase to be sure of that.

v2: Add missing mutex_init to avoid angering lockdep.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/nouveau/nv50_display.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 3587408..d4cbea1 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -128,6 +128,11 @@ struct nv50_dmac { struct nv50_chan base; dma_addr_t handle; u32 *ptr; + + /* Protects against concurrent pushbuf access to this channel, lock is + * grabbed by evo_wait (if the pushbuf reservation is successful) and + * dropped again by evo_kick. */ + struct mutex lock; };

static void @@ -271,6 +276,8 @@ nv50_dmac_create(struct nouveau_object *core, u32 bclass, u8 head, u32 pushbuf = *(u32 *)data; int ret;

+ mutex_init(&dmac->lock); + dmac->ptr = pci_alloc_consistent(nv_device(core)->pdev, PAGE_SIZE, &dmac->handle); if (!dmac->ptr) @@ -395,11 +402,13 @@ evo_wait(void *evoc, int nr) struct nv50_dmac *dmac = evoc; u32 put = nv_ro32(dmac->base.user, 0x0000) / 4;

+ mutex_lock(&dmac->lock); if (put + nr >= (PAGE_SIZE / 4) - 8) { dmac->ptr[put] = 0x20000000;

nv_wo32(dmac->base.user, 0x0000, 0x00000000); if (!nv_wait(dmac->base.user, 0x0004, ~0, 0x00000000)) { + mutex_unlock(&dmac->lock); NV_ERROR(dmac->base.user, "channel stalled\n"); return NULL; } @@ -415,6 +424,7 @@ evo_kick(u32 *push, void *evoc) { struct nv50_dmac *dmac = evoc; nv_wo32(dmac->base.user, 0x0000, (push - dmac->ptr) << 2); + mutex_unlock(&dmac->lock); }

#define evo_mthd(p,m,s) *((p)++) = (((s) << 18) | (m))

Some drivers don't have real ->create_handle callbacks.

- cirrus/ast/mga200: Returns either 0 or -EINVAL.

- udl: Didn't even bother with a callback, leading to a nice userspace-triggerable OOPS.

- vmwgfx: This driver bothered with an implementation to return 0 as the handle (which is the canonical no-obj gem handle).

All have in common that ->create_handle doesn't really make too much sense for them - that ioctl is used only for seamless fb takeover in the radeon/nouveau/i915 ddx drivers. So allow drivers to not implement this and return a consistent -ENODEV.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_main.c | 8 -------- drivers/gpu/drm/cirrus/cirrus_main.c | 8 -------- drivers/gpu/drm/drm_crtc.c | 5 ++++- drivers/gpu/drm/mgag200/mgag200_main.c | 8 -------- drivers/gpu/drm/udl/udl_fb.c | 1 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ------------ 6 files changed, 4 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index d5ba709..f60fd7b 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -246,16 +246,8 @@ static void ast_user_framebuffer_destroy(struct drm_framebuffer *fb) kfree(fb); }

-static int ast_user_framebuffer_create_handle(struct drm_framebuffer *fb, - struct drm_file *file, - unsigned int *handle) -{ - return -EINVAL; -} - static const struct drm_framebuffer_funcs ast_fb_funcs = { .destroy = ast_user_framebuffer_destroy, - .create_handle = ast_user_framebuffer_create_handle, };

diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index 364474c..35cbae8 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -23,16 +23,8 @@ static void cirrus_user_framebuffer_destroy(struct drm_framebuffer *fb) kfree(fb); }

-static int cirrus_user_framebuffer_create_handle(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int *handle) -{ - return 0; -} - static const struct drm_framebuffer_funcs cirrus_fb_funcs = { .destroy = cirrus_user_framebuffer_destroy, - .create_handle = cirrus_user_framebuffer_create_handle, };

int cirrus_framebuffer_init(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8d665fa..a9abf49 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2384,7 +2384,10 @@ int drm_mode_getfb(struct drm_device *dev, r->depth = fb->depth; r->bpp = fb->bits_per_pixel; r->pitch = fb->pitches[0]; - fb->funcs->create_handle(fb, file_priv, &r->handle); + if (fb->funcs->create_handle) + ret = fb->funcs->create_handle(fb, file_priv, &r->handle); + else + ret = -ENODEV;

out: mutex_unlock(&dev->mode_config.mutex); diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 266438a..64297c7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -23,16 +23,8 @@ static void mga_user_framebuffer_destroy(struct drm_framebuffer *fb) kfree(fb); }

-static int mga_user_framebuffer_create_handle(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int *handle) -{ - return 0; -} - static const struct drm_framebuffer_funcs mga_fb_funcs = { .destroy = mga_user_framebuffer_destroy, - .create_handle = mga_user_framebuffer_create_handle, };

int mgag200_framebuffer_init(struct drm_device *dev, diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 829c4a7..f8904b4 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -422,7 +422,6 @@ static void udl_user_framebuffer_destroy(struct drm_framebuffer *fb) static const struct drm_framebuffer_funcs udlfb_funcs = { .destroy = udl_user_framebuffer_destroy, .dirty = udl_user_framebuffer_dirty, - .create_handle = NULL, };

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index edc9792..9c0876b 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -373,16 +373,6 @@ void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv) * Generic framebuffer code */

-int vmw_framebuffer_create_handle(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int *handle) -{ - if (handle) - *handle = 0; - - return 0; -} - /* * Surface framebuffer code */ @@ -610,7 +600,6 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, static struct drm_framebuffer_funcs vmw_framebuffer_surface_funcs = { .destroy = vmw_framebuffer_surface_destroy, .dirty = vmw_framebuffer_surface_dirty, - .create_handle = vmw_framebuffer_create_handle, };

static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, @@ -961,7 +950,6 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, static struct drm_framebuffer_funcs vmw_framebuffer_dmabuf_funcs = { .destroy = vmw_framebuffer_dmabuf_destroy, .dirty = vmw_framebuffer_dmabuf_dirty, - .create_handle = vmw_framebuffer_create_handle, };

/**

This is the first step towards introducing the new modeset locking scheme. The plan is to put helper functions into place at all the right places step-by-step, so that the final patch to switch on the new locking scheme doesn't need to touch every single driver.

This helper here will serve as the shotgun solutions for all places where a more fine-grained locking isn't (yet) implemented.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 147 +++++++++++++++++++++---------------- drivers/gpu/drm/drm_crtc_helper.c | 8 +- drivers/gpu/drm/drm_fb_helper.c | 20 ++--- include/drm/drm_crtc.h | 3 + 4 files changed, 102 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7ca2f28..7c8c5cd 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -37,6 +37,29 @@ #include <drm/drm_edid.h> #include <drm/drm_fourcc.h>

+/** + * drm_modeset_lock_all - take all modeset locks + * @dev: drm device + * + * This function takes all modeset locks, suitable where a more fine-grained + * scheme isn't (yet) implemented. + */ +void drm_modeset_lock_all(struct drm_device *dev) +{ + mutex_lock(&dev->mode_config.mutex); +} +EXPORT_SYMBOL(drm_modeset_lock_all); + +/** + * drm_modeset_unlock_all - drop all modeset locks + * @drm: device + */ +void drm_modeset_unlock_all(struct drm_device *dev) +{ + mutex_unlock(&dev->mode_config.mutex); +} +EXPORT_SYMBOL(drm_modeset_unlock_all); + /* Avoid boilerplate. I'm tired of typing. */ #define DRM_ENUM_NAME_FN(fnname, list) \ char *fnname(int val) \ @@ -425,7 +448,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, crtc->funcs = funcs; crtc->invert_dimensions = false;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) @@ -437,7 +460,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, dev->mode_config.num_crtc++;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return ret; } @@ -512,7 +535,7 @@ int drm_connector_init(struct drm_device *dev, { int ret;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR); if (ret) @@ -542,7 +565,7 @@ int drm_connector_init(struct drm_device *dev, dev->mode_config.dpms_property, 0);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return ret; } @@ -592,7 +615,7 @@ int drm_encoder_init(struct drm_device *dev, { int ret;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER); if (ret) @@ -606,7 +629,7 @@ int drm_encoder_init(struct drm_device *dev, dev->mode_config.num_encoder++;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return ret; } @@ -615,11 +638,11 @@ EXPORT_SYMBOL(drm_encoder_init); void drm_encoder_cleanup(struct drm_encoder *encoder) { struct drm_device *dev = encoder->dev; - mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); drm_mode_object_put(dev, &encoder->base); list_del(&encoder->head); dev->mode_config.num_encoder--; - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); } EXPORT_SYMBOL(drm_encoder_cleanup);

@@ -631,7 +654,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, { int ret;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); if (ret) @@ -665,7 +688,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, }

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return ret; } @@ -675,7 +698,7 @@ void drm_plane_cleanup(struct drm_plane *plane) { struct drm_device *dev = plane->dev;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); kfree(plane->format_types); drm_mode_object_put(dev, &plane->base); /* if not added to a list, it must be a private plane */ @@ -683,7 +706,7 @@ void drm_plane_cleanup(struct drm_plane *plane) list_del(&plane->head); dev->mode_config.num_plane--; } - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); } EXPORT_SYMBOL(drm_plane_cleanup);

@@ -963,9 +986,9 @@ void drm_mode_config_init(struct drm_device *dev) INIT_LIST_HEAD(&dev->mode_config.plane_list); idr_init(&dev->mode_config.crtc_idr);

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); drm_mode_create_standard_connector_properties(dev); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

/* Just to be sure */ dev->mode_config.num_fb = 0; @@ -1185,7 +1208,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

/* * For the non-control nodes we need to limit the list of resources @@ -1327,7 +1350,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, card_res->count_connectors, card_res->count_encoders);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1355,7 +1378,7 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, crtc_resp->crtc_id, DRM_MODE_OBJECT_CRTC); @@ -1383,7 +1406,7 @@ int drm_mode_getcrtc(struct drm_device *dev, }

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1426,7 +1449,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,

DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, out_resp->connector_id, DRM_MODE_OBJECT_CONNECTOR); @@ -1523,7 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->count_encoders = encoders_count;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1538,7 +1561,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, enc_resp->encoder_id, DRM_MODE_OBJECT_ENCODER); if (!obj) { @@ -1557,7 +1580,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data, enc_resp->possible_clones = encoder->possible_clones;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1581,7 +1604,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); config = &dev->mode_config;

/* @@ -1603,7 +1626,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, plane_resp->count_planes = config->num_plane;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1628,7 +1651,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, plane_resp->plane_id, DRM_MODE_OBJECT_PLANE); if (!obj) { @@ -1668,7 +1691,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data, plane_resp->count_format_types = plane->format_count;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -1696,7 +1719,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

/* * First, find the plane, crtc, and fb objects. If not available, @@ -1795,7 +1818,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, }

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return ret; } @@ -1850,7 +1873,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, if (crtc_req->x > INT_MAX || crtc_req->y > INT_MAX) return -ERANGE;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, crtc_req->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) { @@ -1983,7 +2006,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, out: kfree(connector_set); drm_mode_destroy(dev, mode); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2001,7 +2024,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, if (!req->flags || (~DRM_MODE_CURSOR_FLAGS & req->flags)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, req->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) { DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id); @@ -2029,7 +2052,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, } } out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2108,7 +2131,7 @@ int drm_mode_addfb(struct drm_device *dev, if ((config->min_height > r.height) || (r.height > config->max_height)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

/* TODO check buffer is sufficiently large */ /* TODO setup destructor callback */ @@ -2125,7 +2148,7 @@ int drm_mode_addfb(struct drm_device *dev, DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2293,7 +2316,7 @@ int drm_mode_addfb2(struct drm_device *dev, if (ret) return ret;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

fb = dev->mode_config.funcs->fb_create(dev, file_priv, r); if (IS_ERR(fb)) { @@ -2307,7 +2330,7 @@ int drm_mode_addfb2(struct drm_device *dev, DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2337,7 +2360,7 @@ int drm_mode_rmfb(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, *id, DRM_MODE_OBJECT_FB); /* TODO check that we really get a framebuffer back. */ if (!obj) { @@ -2358,7 +2381,7 @@ int drm_mode_rmfb(struct drm_device *dev, drm_framebuffer_remove(fb);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2386,7 +2409,7 @@ int drm_mode_getfb(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB); if (!obj) { ret = -EINVAL; @@ -2405,7 +2428,7 @@ int drm_mode_getfb(struct drm_device *dev, ret = -ENODEV;

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2424,7 +2447,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB); if (!obj) { ret = -EINVAL; @@ -2478,7 +2501,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, out_err2: kfree(clips); out_err1: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2499,11 +2522,11 @@ void drm_fb_release(struct drm_file *priv) struct drm_device *dev = priv->minor->dev; struct drm_framebuffer *fb, *tfb;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) { drm_framebuffer_remove(fb); } - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); }

/** @@ -2618,7 +2641,7 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, mode_cmd->connector_id, DRM_MODE_OBJECT_CONNECTOR); if (!obj) { @@ -2642,7 +2665,7 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev,

drm_mode_attachmode(dev, connector, mode); out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2671,7 +2694,7 @@ int drm_mode_detachmode_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, mode_cmd->connector_id, DRM_MODE_OBJECT_CONNECTOR); if (!obj) { @@ -2688,7 +2711,7 @@ int drm_mode_detachmode_ioctl(struct drm_device *dev,

ret = drm_mode_detachmode(dev, connector, &mode); out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -2934,7 +2957,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, out_resp->prop_id, DRM_MODE_OBJECT_PROPERTY); if (!obj) { ret = -EINVAL; @@ -3012,7 +3035,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, out_resp->count_enum_blobs = blob_count; } done: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -3063,7 +3086,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, out_resp->blob_id, DRM_MODE_OBJECT_BLOB); if (!obj) { ret = -EINVAL; @@ -3081,7 +3104,7 @@ int drm_mode_getblob_ioctl(struct drm_device *dev, out_resp->length = blob->length;

done: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -3223,7 +3246,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type); if (!obj) { @@ -3260,7 +3283,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, } arg->count_props = props_count; out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -3277,7 +3300,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type); if (!arg_obj) @@ -3315,7 +3338,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, }

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -3377,7 +3400,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, crtc_lut->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) { ret = -EINVAL; @@ -3418,7 +3441,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev, crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret;

} @@ -3436,7 +3459,7 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, crtc_lut->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) { ret = -EINVAL; @@ -3469,7 +3492,7 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, goto out; } out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

@@ -3489,7 +3512,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, page_flip->reserved != 0) return -EINVAL;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, page_flip->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) goto out; @@ -3567,7 +3590,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, }

out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; }

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 7b2d378..400ef86 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -980,7 +980,7 @@ static void output_poll_execute(struct work_struct *work) if (!drm_kms_helper_poll) return;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Ignore forced connectors. */ @@ -1010,7 +1010,7 @@ static void output_poll_execute(struct work_struct *work) changed = true; }

- mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

if (changed) drm_kms_helper_hotplug_event(dev); @@ -1070,7 +1070,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) if (!dev->mode_config.poll_enabled) return;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Only handle HPD capable connectors. */ @@ -1088,7 +1088,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) changed = true; }

- mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

if (changed) drm_kms_helper_hotplug_event(dev); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 82c3a9f..be0f2d6 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -337,7 +337,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) /* * For each CRTC in this fb, turn the connectors on/off. */ - mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc;

@@ -352,7 +352,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) dev->mode_config.dpms_property, dpms_mode); } } - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); }

int drm_fb_helper_blank(int blank, struct fb_info *info) @@ -672,16 +672,16 @@ int drm_fb_helper_set_par(struct fb_info *info) return -EINVAL; }

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; ret = drm_mode_set_config_internal(&fb_helper->crtc_info[i].mode_set); if (ret) { - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; } } - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

if (fb_helper->delayed_hotplug) { fb_helper->delayed_hotplug = false; @@ -701,7 +701,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, int ret = 0; int i;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc;

@@ -718,7 +718,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, } } } - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret; } EXPORT_SYMBOL(drm_fb_helper_pan_display); @@ -1375,7 +1375,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) if (!fb_helper->fb) return 0;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (crtc->fb) crtcs_bound++; @@ -1385,7 +1385,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)

if (bound < crtcs_bound) { fb_helper->delayed_hotplug = true; - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return 0; } DRM_DEBUG_KMS("\n"); @@ -1397,7 +1397,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) count = drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height); drm_setup_crtcs(fb_helper); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 665289a..9f0524d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -842,6 +842,9 @@ struct drm_prop_enum_list { char *name; };

+extern void drm_modeset_lock_all(struct drm_device *dev); +extern void drm_modeset_unlock_all(struct drm_device *dev); + extern int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs);

Only two places: - suspend/resume - Some really strange mode validation tool with too much funny-lucking hand-rolled conversion code. - The recently-added lastclose fbdev restore code.

Better safe than sorry, so convert both places to keep the locking semantics as much as possible.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/psb_device.c | 8 ++++---- drivers/gpu/drm/gma500/psb_drv.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c index b58c470..f6f534b 100644 --- a/drivers/gpu/drm/gma500/psb_device.c +++ b/drivers/gpu/drm/gma500/psb_device.c @@ -194,7 +194,7 @@ static int psb_save_display_registers(struct drm_device *dev) regs->saveCHICKENBIT = PSB_RVDC32(DSPCHICKENBIT);

/* Save crtc and output state */ - mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (drm_helper_crtc_in_use(crtc)) crtc->funcs->save(crtc); @@ -204,7 +204,7 @@ static int psb_save_display_registers(struct drm_device *dev) if (connector->funcs->save) connector->funcs->save(connector);

- mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return 0; }

@@ -234,7 +234,7 @@ static int psb_restore_display_registers(struct drm_device *dev) /*make sure VGA plane is off. it initializes to on after reset!*/ PSB_WVDC32(0x80000000, VGACNTRL);

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) if (drm_helper_crtc_in_use(crtc)) crtc->funcs->restore(crtc); @@ -243,7 +243,7 @@ static int psb_restore_display_registers(struct drm_device *dev) if (connector->funcs->restore) connector->funcs->restore(connector);

- mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return 0; }

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index dbcefe9..111e3df 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -153,11 +153,11 @@ static void psb_lastclose(struct drm_device *dev) struct drm_psb_private *dev_priv = dev->dev_private; struct psb_fbdev *fbdev = dev_priv->fbdev;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev); ret = drm_fb_helper_restore_fbdev_mode(&fbdev->psb_fb_helper); if (ret) DRM_DEBUG("failed to restore crtc mode\n"); - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev);

return; } @@ -486,7 +486,7 @@ static int psb_mode_operation_ioctl(struct drm_device *dev, void *data, case PSB_MODE_OPERATION_MODE_VALID: umode = &arg->mode;

- mutex_lock(&dev->mode_config.mutex); + drm_modeset_lock_all(dev);

obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_CONNECTOR); @@ -535,7 +535,7 @@ static int psb_mode_operation_ioctl(struct drm_device *dev, void *data, if (mode) drm_mode_destroy(dev, mode); mode_op_out: - mutex_unlock(&dev->mode_config.mutex); + drm_modeset_unlock_all(dev); return ret;

default:

Only a resume method to account for.

v2: Fixup compilation.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c index 1c350fc..d701219 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c @@ -313,9 +313,9 @@ static int shmob_drm_pm_resume(struct device *dev) { struct shmob_drm_device *sdev = dev_get_drvdata(dev);

- mutex_lock(&sdev->ddev->mode_config.mutex); + drm_modeset_lock_all(sdev->ddev); shmob_drm_crtc_resume(&sdev->crtc); - mutex_unlock(&sdev->ddev->mode_config.mutex); + drm_modeset_unlock_all(sdev->ddev);

drm_kms_helper_poll_enable(sdev->ddev); return 0;

*drumroll*

The basic idea is to protect per-crtc state which can change without touching the output configuration with separate mutexes, i.e. all the input side state to a crtc like framebuffers, cursor settings or plane configuration. Holding such a crtc lock gives a read-lock on all the other crtc state which can be changed by e.g. a modeset.

All non-crtc state is still protected by the mode_config mutex. Callers that need to change modeset state of a crtc (e.g. dpms or set_mode) need to grab both the mode_config lock and nested within any crtc locks.

Note that since there can only ever be one holder of the mode_config lock we can grab the subordinate crtc locks in any order (if we need to grab more than one of them). Lockdep can handle such nesting with the mutex_lock_nest_lock call correctly.

With this functions that only touch connectors/encoders but not crtcs only need to take the mode_config lock. The biggest such case is the output probing, which means that we can now pageflip and move cursors while the output probe code is reading an edid.

Most cases neatly fall into the three buckets: - Only touches connectors and similar output state and so only needs the mode_config lock. - Touches the global configuration and so needs all locks. - Only touches the crtc input side and so only needs the crtc lock.

But a few cases that need special consideration:

- Load detection which requires a crtc. The mode_config lock already prevents a modeset change, so we can use any unused crtc as we like to do load detection. The only thing to consider is that such temporary state changes don't leak out to userspace through ioctls that only take the crtc look (like a pageflip). Hence the load detect code needs to grab the crtc of any output pipes it touches (but only if it touches state used by the pageflip or cursor ioctls).

- Atomic pageflip when moving planes. The first case is sane hw, where planes have a fixed association with crtcs - nothing needs to be done there. More insane^Wflexible hw needs to have plane->crtc mapping which is separately protect with a lock that nests within the crtc lock. If the plane is unused we can just assign it to the current crtc and continue. But if a plane is already in use by another crtc we can't just reassign it.

Two solution present themselves: Either go back to a slow-path which takes all modeset locks, potentially incure quite a hefty delay. Or simply disallowing such changes in one atomic pageflip - in general the vblanks of two crtcs are not synced, so there's no sane way to atomically flip such plane changes accross more than one crtc. I'd heavily favour the later approach, going as far as mandating it as part of the ABI of such a new a nuclear pageflip.

And if we _really_ want such semantics, we can always get them by introducing another pageflip mutex between the mode_config.mutex and the individual crtc locks. Pageflips crossing more than one crtc would then need to take that lock first, to lock out concurrent multi-crtc pageflips.

- Optimized global modeset operations: We could just take the mode_config lock and then lazily lock all crtc which are affected by a modeset operation. This has the advantage that pageflip could continue unhampered on unaffected crtc. But if e.g. global resources like plls need to be reassigned and so affect unrelated crtcs we can still do that - nested locking works in any order.

This patch just adds the locks and takes them in drm_modeset_lock_all, no real locking changes yet.

v2: Need to initialize the new lock in crtc_init and lock it righ away, for otherwise the modeset_unlock_all below will try to unlock a not-locked mutex.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 12 ++++++++++++ include/drm/drm_crtc.h | 9 +++++++++ 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7c8c5cd..0494ff4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -46,7 +46,12 @@ */ void drm_modeset_lock_all(struct drm_device *dev) { + struct drm_crtc *crtc; + mutex_lock(&dev->mode_config.mutex); + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_lock_all);

@@ -56,6 +61,11 @@ EXPORT_SYMBOL(drm_modeset_lock_all); */ void drm_modeset_unlock_all(struct drm_device *dev) { + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + mutex_unlock(&crtc->mutex); + mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_unlock_all); @@ -449,6 +459,8 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, crtc->invert_dimensions = false;

drm_modeset_lock_all(dev); + mutex_init(&crtc->mutex); + mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);

ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 9f0524d..c89b116 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -390,6 +390,15 @@ struct drm_crtc { struct drm_device *dev; struct list_head head;

+ /** + * crtc mutex + * + * This provides a read lock for the overall crtc state (mode, dpms + * state, ...) and a write lock for everything which can be update + * without a full modeset (fb, cursor data, ...) + */ + struct mutex mutex; + struct drm_mode_object base;

/* framebuffer the connector is currently bound to */

->cursor_move uses mostly the same facilities in drivers as ->cursor_set, so pretty much nothing to fix up:

- ast/gma500/i915: They all use per-crtc registers to update the cursor position. ast again touches the global cursor cache, but that's ok since there's only one crtc.

- nouveau: nv50+ is again special, updates happen through the per-crtc channel (without pushbufs), so it's not protected by the new evo lock introduced earlier. But since this channel is per-crtc, we should be fine anyway.

- radeon: A bit a mess: avivo asics need a workaround when both output pipes are enabled, which means it'll access the crtc list. Just reading that flag is ok though as long as radeon _always_ grabs all locks when changing the crtc configuration. Which means with the current scheme it cannot do an optimized modeset which only locks the relevant crtcs. This can be fixed though by introducing a bit of global state with separate locks and ensure in the modeset code that the cursor will be updated appropriately when enabling the 2nd pipe (on affected asics).

- vmwgfx: I still don't understand what it's doing exactly, so apply the same trick for now.

v2: Fixup unlocking for the error cases, spotted by Richard Wilbur.

v3: Another error-case fixup.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 10 ++++------ drivers/gpu/drm/radeon/radeon_cursor.c | 8 +++++++- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 13 +++++++++++++ 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e62a12f..457e4faf 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2039,34 +2039,32 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, obj = drm_mode_object_find(dev, req->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) { DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id); - ret = -EINVAL; - goto out; + return -EINVAL; } crtc = obj_to_crtc(obj);

+ mutex_lock(&crtc->mutex); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set) { ret = -ENXIO; goto out; } /* Turns off the cursor if handle is 0 */ - mutex_lock(&crtc->mutex); ret = crtc->funcs->cursor_set(crtc, file_priv, req->handle, req->width, req->height); - mutex_unlock(&crtc->mutex); }

if (req->flags & DRM_MODE_CURSOR_MOVE) { if (crtc->funcs->cursor_move) { - drm_modeset_lock_all(dev); ret = crtc->funcs->cursor_move(crtc, req->x, req->y); - drm_modeset_unlock_all(dev); } else { ret = -EFAULT; goto out; } } out: + mutex_unlock(&crtc->mutex); + return ret; }

diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c index ad6df62..c1680e6 100644 --- a/drivers/gpu/drm/radeon/radeon_cursor.c +++ b/drivers/gpu/drm/radeon/radeon_cursor.c @@ -245,8 +245,14 @@ int radeon_crtc_cursor_move(struct drm_crtc *crtc, int i = 0; struct drm_crtc *crtc_p;

- /* avivo cursor image can't end on 128 pixel boundary or + /* + * avivo cursor image can't end on 128 pixel boundary or * go past the end of the frame if both crtcs are enabled + * + * NOTE: It is safe to access crtc->enabled of other crtcs + * without holding either the mode_config lock or the other + * crtc's lock as long as write access to this flag _always_ + * grabs all locks. */ list_for_each_entry(crtc_p, &crtc->dev->mode_config.crtc_list, head) { if (crtc_p->enabled) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 8d82e63..3e3c7ab 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -264,10 +264,23 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + crtc->x; du->cursor_y = y + crtc->y;

+ /* + * FIXME: Unclear whether there's any global state touched by the + * cursor_set function, especially vmw_cursor_update_position looks + * suspicious. For now take the easy route and reacquire all locks. We + * can do this since the caller in the drm core doesn't check anything + * which is protected by any looks. + */ + mutex_unlock(&crtc->mutex); + drm_modeset_lock_all(dev_priv->dev); + vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x, du->cursor_y + du->hotspot_y);

+ drm_modeset_unlock_all(dev_priv->dev); + mutex_lock(&crtc->mutex); + return 0; }

And replace all fb lookups with it. Also add a WARN to drm_mode_object_find since that is now no longer the blessed interface to look up an fb. And add kerneldoc to both functions.

This only updates all callsites, but immediately drops the acquired refence again. Hence all callers still rely on the fact that a mode fb can't disappear while they're holding the struct mutex. Subsequent patches will instate proper use of refcounts, and then rework the rmfb and unref code to no longer serialize fb destruction with the mode_config lock. We don't want that since otherwise a compositor might end up stalling for a few frames in rmfb.

v2: Don't use kref_get_unless_zero - Greg KH doesn't like that kind of interface.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 109 ++++++++++++++++++++++----------- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 24 ++++---- include/drm/drm_crtc.h | 2 + 3 files changed, 86 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d387ddb..1c13984 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -294,11 +294,24 @@ static void drm_mode_object_put(struct drm_device *dev, mutex_unlock(&dev->mode_config.idr_mutex); }

+/** + * drm_mode_object_find - look up a drm object with static lifetime + * @dev: drm device + * @id: id of the mode object + * @type: type of the mode object + * + * Note that framebuffers cannot be looked up with this functions - since those + * are reference counted, they need special treatment. + */ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, uint32_t id, uint32_t type) { struct drm_mode_object *obj = NULL;

+ /* Framebuffers are reference counted and need their own lookup + * function.*/ + WARN_ON(type == DRM_MODE_OBJECT_FB); + mutex_lock(&dev->mode_config.idr_mutex); obj = idr_find(&dev->mode_config.crtc_idr, id); if (!obj || (obj->type != type) || (obj->id != id)) @@ -359,6 +372,40 @@ static void drm_framebuffer_free(struct kref *kref) }

/** + * drm_framebuffer_lookup - look up a drm framebuffer and grab a reference + * @dev: drm device + * @id: id of the fb object + * + * If successful, this grabs an additional reference to the framebuffer - + * callers need to make sure to eventually unreference the returned framebuffer + * again. + */ +struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev, + uint32_t id) +{ + struct drm_mode_object *obj = NULL; + struct drm_framebuffer *fb; + + mutex_lock(&dev->mode_config.fb_lock); + + mutex_lock(&dev->mode_config.idr_mutex); + obj = idr_find(&dev->mode_config.crtc_idr, id); + if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id)) + fb = NULL; + else + fb = obj_to_fb(obj); + mutex_unlock(&dev->mode_config.idr_mutex); + + if (fb) + kref_get(&fb->refcount); + + mutex_unlock(&dev->mode_config.fb_lock); + + return fb; +} +EXPORT_SYMBOL(drm_framebuffer_lookup); + +/** * drm_framebuffer_unreference - unref a framebuffer * @fb: framebuffer to unref * @@ -1788,17 +1835,15 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } crtc = obj_to_crtc(obj);

- mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, plane_req->fb_id, - DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, plane_req->fb_id); + if (!fb) { DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", plane_req->fb_id); ret = -ENOENT; goto out; } - fb = obj_to_fb(obj); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

/* Check whether this plane supports the fb pixel format. */ for (i = 0; i < plane->format_count; i++) @@ -1933,17 +1978,16 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, } fb = crtc->fb; } else { - mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, crtc_req->fb_id, - DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, crtc_req->fb_id); + if (!fb) { DRM_DEBUG_KMS("Unknown FB ID%d\n", crtc_req->fb_id); ret = -EINVAL; goto out; } - fb = obj_to_fb(obj); + /* fb is protect by the mode_config lock, so drop the + * ref immediately */ + drm_framebuffer_unreference(fb); }

mode = drm_mode_create(dev); @@ -2392,7 +2436,6 @@ int drm_mode_addfb2(struct drm_device *dev, int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv) { - struct drm_mode_object *obj; struct drm_framebuffer *fb = NULL; struct drm_framebuffer *fbl = NULL; uint32_t *id = data; @@ -2403,16 +2446,13 @@ int drm_mode_rmfb(struct drm_device *dev, return -EINVAL;

drm_modeset_lock_all(dev); - mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, *id, DRM_MODE_OBJECT_FB); - /* TODO check that we really get a framebuffer back. */ - if (!obj) { - mutex_unlock(&dev->mode_config.fb_lock); + fb = drm_framebuffer_lookup(dev, *id); + if (!fb) { ret = -EINVAL; goto out; } - fb = obj_to_fb(obj); - mutex_unlock(&dev->mode_config.fb_lock); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

mutex_lock(&file_priv->fbs_lock); list_for_each_entry(fbl, &file_priv->fbs, filp_head) @@ -2451,7 +2491,6 @@ int drm_mode_getfb(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_fb_cmd *r = data; - struct drm_mode_object *obj; struct drm_framebuffer *fb; int ret = 0;

@@ -2459,14 +2498,13 @@ int drm_mode_getfb(struct drm_device *dev, return -EINVAL;

drm_modeset_lock_all(dev); - mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, r->fb_id); + if (!fb) { ret = -EINVAL; goto out; } - fb = obj_to_fb(obj); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

r->height = fb->height; r->width = fb->width; @@ -2489,7 +2527,6 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, struct drm_clip_rect __user *clips_ptr; struct drm_clip_rect *clips = NULL; struct drm_mode_fb_dirty_cmd *r = data; - struct drm_mode_object *obj; struct drm_framebuffer *fb; unsigned flags; int num_clips; @@ -2499,14 +2536,13 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, return -EINVAL;

drm_modeset_lock_all(dev); - mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, r->fb_id, DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, r->fb_id); + if (!fb) { ret = -EINVAL; goto out_err1; } - fb = obj_to_fb(obj); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

num_clips = r->num_clips; clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r->clips_ptr; @@ -3586,12 +3622,11 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (crtc->funcs->page_flip == NULL) goto out;

- mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, page_flip->fb_id, DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) + fb = drm_framebuffer_lookup(dev, page_flip->fb_id); + if (!fb) goto out; - fb = obj_to_fb(obj); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

hdisplay = crtc->mode.hdisplay; vdisplay = crtc->mode.vdisplay; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c index 0d6a161..1b8f428 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c @@ -131,7 +131,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, struct vmw_master *vmaster = vmw_master(file_priv->master); struct drm_vmw_rect __user *clips_ptr; struct drm_vmw_rect *clips = NULL; - struct drm_mode_object *obj; + struct drm_framebuffer *fb; struct vmw_framebuffer *vfb; struct vmw_resource *res; uint32_t num_clips; @@ -165,15 +165,15 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,

drm_modeset_lock_all(dev);

- mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, arg->fb_id); + if (!fb) { DRM_ERROR("Invalid framebuffer id.\n"); ret = -EINVAL; goto out_no_fb; } - vfb = vmw_framebuffer_to_vfb(obj_to_fb(obj)); + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb); + vfb = vmw_framebuffer_to_vfb(fb);

ret = ttm_read_lock(&vmaster->lock, true); if (unlikely(ret != 0)) @@ -217,7 +217,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, struct vmw_master *vmaster = vmw_master(file_priv->master); struct drm_vmw_rect __user *clips_ptr; struct drm_vmw_rect *clips = NULL; - struct drm_mode_object *obj; + struct drm_framebuffer *fb; struct vmw_framebuffer *vfb; uint32_t num_clips; int ret; @@ -250,16 +250,16 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data,

drm_modeset_lock_all(dev);

- mutex_lock(&dev->mode_config.fb_lock); - obj = drm_mode_object_find(dev, arg->fb_id, DRM_MODE_OBJECT_FB); - mutex_unlock(&dev->mode_config.fb_lock); - if (!obj) { + fb = drm_framebuffer_lookup(dev, arg->fb_id); + if (!fb) { DRM_ERROR("Invalid framebuffer id.\n"); ret = -EINVAL; goto out_no_fb; } + /* fb is protect by the mode_config lock, so drop the ref immediately */ + drm_framebuffer_unreference(fb);

- vfb = vmw_framebuffer_to_vfb(obj_to_fb(obj)); + vfb = vmw_framebuffer_to_vfb(fb); if (!vfb->dmabuf) { DRM_ERROR("Framebuffer not dmabuf backed.\n"); ret = -EINVAL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c35a807..7dc1b31 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -958,6 +958,8 @@ extern void drm_framebuffer_set_object(struct drm_device *dev, extern int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, const struct drm_framebuffer_funcs *funcs); +extern struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev, + uint32_t id); extern void drm_framebuffer_unreference(struct drm_framebuffer *fb); extern void drm_framebuffer_reference(struct drm_framebuffer *fb); extern void drm_framebuffer_remove(struct drm_framebuffer *fb);

Since otherwise looking and reference-counting around drm_framebuffer_lookup will be an unmanageable mess. With this change, an object can either be found in the idr and will stay around once we incremented the reference counter. Or it will be gone for good and can't be looked up using its id any more.

Atomicity is guaranteed by the dev->mode_config.fb_lock. The newly-introduce fpriv->fbs_lock looks a bit redundant, but the next patch will shuffle the locking order between these two locks and all the modeset locks taken in modeset_lock_all, so we'll need it.

Also, since userspace could do really funky stuff and race e.g. a getresources with an rmfb, we need to make sure that the kernel doesn't fall over trying to look-up an inexistent fb, or causing confusion by having two fbs around with the same id. Simply reset the framebuffer id to 0, which marks it as reaped. Any lookups of that id will fail, so the object is really gone for good from userspace's pov.

Note that we still need to protect the "remove framebuffer from all use-cases" and the final unreference with the modeset-lock, since most framebuffer use-sites don't implement proper reference counting yet. We can only lift this once _all_ users are converted.

With this change, two references are held on alife, but unused framebuffers: - The reference for the idr lookup, created in this patch. - For user-created framebuffers the fpriv->fbs reference, for driver-private fbs the driver is supposed to hold it's own last reference.

Note that the dev->mode_config.fb_list itself does _not_ hold a reference onto the framebuffers (this list is essentially only used for debugfs files). Hence if there's anything left there when the driver has cleaned up all it's modeset resources, this is a ref-leak. WARN about it.

Now we only need to fix up all other places to properly reference count framebuffers.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 118 ++++++++++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4e7c362..b6eecd2 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -356,6 +356,9 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, if (ret) goto out;

+ /* Grab the idr reference. */ + drm_framebuffer_reference(fb); + dev->mode_config.num_fb++; list_add(&fb->head, &dev->mode_config.fb_list); out: @@ -372,6 +375,23 @@ static void drm_framebuffer_free(struct kref *kref) fb->funcs->destroy(fb); }

+static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev, + uint32_t id) +{ + struct drm_mode_object *obj = NULL; + struct drm_framebuffer *fb; + + mutex_lock(&dev->mode_config.idr_mutex); + obj = idr_find(&dev->mode_config.crtc_idr, id); + if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id)) + fb = NULL; + else + fb = obj_to_fb(obj); + mutex_unlock(&dev->mode_config.idr_mutex); + + return fb; +} + /** * drm_framebuffer_lookup - look up a drm framebuffer and grab a reference * @dev: drm device @@ -384,22 +404,12 @@ static void drm_framebuffer_free(struct kref *kref) struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev, uint32_t id) { - struct drm_mode_object *obj = NULL; struct drm_framebuffer *fb;

mutex_lock(&dev->mode_config.fb_lock); - - mutex_lock(&dev->mode_config.idr_mutex); - obj = idr_find(&dev->mode_config.crtc_idr, id); - if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id)) - fb = NULL; - else - fb = obj_to_fb(obj); - mutex_unlock(&dev->mode_config.idr_mutex); - + fb = __drm_framebuffer_lookup(dev, id); if (fb) kref_get(&fb->refcount); - mutex_unlock(&dev->mode_config.fb_lock);

return fb; @@ -430,6 +440,24 @@ void drm_framebuffer_reference(struct drm_framebuffer *fb) } EXPORT_SYMBOL(drm_framebuffer_reference);

+static void drm_framebuffer_free_bug(struct kref *kref) +{ + BUG(); +} + +/* dev->mode_config.fb_lock must be held! */ +static void __drm_framebuffer_unregister(struct drm_device *dev, + struct drm_framebuffer *fb) +{ + mutex_lock(&dev->mode_config.idr_mutex); + idr_remove(&dev->mode_config.crtc_idr, fb->base.id); + mutex_unlock(&dev->mode_config.idr_mutex); + + fb->base.id = 0; + + kref_put(&fb->refcount, drm_framebuffer_free_bug); +} + /** * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr * @fb: fb to unregister @@ -441,6 +469,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference); */ void drm_framebuffer_unregister_private(struct drm_framebuffer *fb) { + struct drm_device *dev = fb->dev; + + mutex_lock(&dev->mode_config.fb_lock); + /* Mark fb as reaped and drop idr ref. */ + __drm_framebuffer_unregister(dev, fb); + mutex_unlock(&dev->mode_config.fb_lock); } EXPORT_SYMBOL(drm_framebuffer_unregister_private);

@@ -464,14 +498,6 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev;

- /* - * This could be moved to drm_framebuffer_remove(), but for - * debugging is nice to keep around the list of fb's that are - * no longer associated w/ a drm_file but are not unreferenced - * yet. (i915 and omapdrm have debugfs files which will show - * this.) - */ - drm_mode_object_put(dev, &fb->base); mutex_lock(&dev->mode_config.fb_lock); list_del(&fb->head); dev->mode_config.num_fb--; @@ -1181,9 +1207,15 @@ void drm_mode_config_cleanup(struct drm_device *dev) drm_property_destroy(dev, property); }

- /* Single-threaded teardown context, so it's not requied to grab the + /* + * Single-threaded teardown context, so it's not requied to grab the * fb_lock to protect against concurrent fb_list access. Contrary, it - * would actually deadlock with the drm_framebuffer_cleanup function. */ + * would actually deadlock with the drm_framebuffer_cleanup function. + * + * Also, if there are any framebuffers left, that's a driver leak now, + * so politely WARN about this. + */ + WARN_ON(!list_empty(&dev->mode_config.fb_list)); list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) { drm_framebuffer_remove(fb); } @@ -2464,39 +2496,41 @@ int drm_mode_rmfb(struct drm_device *dev, struct drm_framebuffer *fb = NULL; struct drm_framebuffer *fbl = NULL; uint32_t *id = data; - int ret = 0; int found = 0;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- drm_modeset_lock_all(dev); - fb = drm_framebuffer_lookup(dev, *id); - if (!fb) { - ret = -EINVAL; - goto out; - } - /* fb is protect by the mode_config lock, so drop the ref immediately */ - drm_framebuffer_unreference(fb); - mutex_lock(&file_priv->fbs_lock); + mutex_lock(&dev->mode_config.fb_lock); + fb = __drm_framebuffer_lookup(dev, *id); + if (!fb) + goto fail_lookup; + list_for_each_entry(fbl, &file_priv->fbs, filp_head) if (fb == fbl) found = 1; - if (!found) { - ret = -EINVAL; - mutex_unlock(&file_priv->fbs_lock); - goto out; - } + if (!found) + goto fail_lookup; + + /* Mark fb as reaped, we still have a ref from fpriv->fbs. */ + __drm_framebuffer_unregister(dev, fb);

list_del_init(&fb->filp_head); + mutex_unlock(&dev->mode_config.fb_lock); mutex_unlock(&file_priv->fbs_lock);

+ drm_modeset_lock_all(dev); drm_framebuffer_remove(fb); -out: drm_modeset_unlock_all(dev);

- return ret; + return 0; + +fail_lookup: + mutex_unlock(&dev->mode_config.fb_lock); + mutex_unlock(&file_priv->fbs_lock); + + return -EINVAL; }

/** @@ -2639,7 +2673,15 @@ void drm_fb_release(struct drm_file *priv) drm_modeset_lock_all(dev); mutex_lock(&priv->fbs_lock); list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) { + + mutex_lock(&dev->mode_config.fb_lock); + /* Mark fb as reaped, we still have a ref from fpriv->fbs. */ + __drm_framebuffer_unregister(dev, fb); + mutex_unlock(&dev->mode_config.fb_lock); + list_del_init(&fb->filp_head); + + /* This will also drop the fpriv->fbs reference. */ drm_framebuffer_remove(fb); } mutex_unlock(&priv->fbs_lock);

And drop it where it's not needed. Most driver just lookup the gem object, allocate an fb struct, fill in all the useful fields and then register it with drm_framebuffer_init.

All of these operations are already separately locked, and since we only put the fb into the fpriv->fbs list _after_ having called ->fb_create, we can't also race with rmfb. We can otoh race with other ioctls that put the framebuffer to use, but all drivers have been reorganized already to call drm_framebuffer_init last in the fb creation sequence.

So essentially, we can completely remove any modeset locks from the addfb ioctl paths. Yeah!

Also, reference-counting is solid - we get a reference from fb_create which we transfer to the fpriv->fbs list. And after unlocking the fpriv->fbs_lock we don't touch the framebuffer any longer. Furthermore drm_framebuffer_init has added a 2nd reference for the idr lookup, and any access through that table will do it's own refcounting.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 9 --------- 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b2a845b..60d6d87 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2271,18 +2271,12 @@ int drm_mode_addfb(struct drm_device *dev, if ((config->min_height > r.height) || (r.height > config->max_height)) return -EINVAL;

- drm_modeset_lock_all(dev); - - /* TODO check buffer is sufficiently large */ - /* TODO setup destructor callback */ - fb = dev->mode_config.funcs->fb_create(dev, file_priv, &r); if (IS_ERR(fb)) { DRM_DEBUG_KMS("could not create framebuffer\n"); drm_modeset_unlock_all(dev); return PTR_ERR(fb); } - drm_modeset_unlock_all(dev);

mutex_lock(&file_priv->fbs_lock); or->fb_id = fb->base.id; @@ -2457,15 +2451,12 @@ int drm_mode_addfb2(struct drm_device *dev, if (ret) return ret;

- drm_modeset_lock_all(dev); - fb = dev->mode_config.funcs->fb_create(dev, file_priv, r); if (IS_ERR(fb)) { DRM_DEBUG_KMS("could not create framebuffer\n"); drm_modeset_unlock_all(dev); return PTR_ERR(fb); } - drm_modeset_unlock_all(dev);

mutex_lock(&file_priv->fbs_lock); r->fb_id = fb->base.id;

We only need to ensure that the fb stays around for long enough. While at it, only grab the modeset locks when we need them (since most drivers don't implement the dirty callback, this should help jitter and stalls when using the generic modeset driver).

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8003063..ed0108e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2580,14 +2580,9 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;

- drm_modeset_lock_all(dev); fb = drm_framebuffer_lookup(dev, r->fb_id); - if (!fb) { - ret = -EINVAL; - goto out_err1; - } - /* fb is protect by the mode_config lock, so drop the ref immediately */ - drm_framebuffer_unreference(fb); + if (!fb) + return -EINVAL;

num_clips = r->num_clips; clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r->clips_ptr; @@ -2625,17 +2620,19 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, }

if (fb->funcs->dirty) { + drm_modeset_lock_all(dev); ret = fb->funcs->dirty(fb, file_priv, flags, r->color, clips, num_clips); + drm_modeset_unlock_all(dev); } else { ret = -ENOSYS; - goto out_err2; }

out_err2: kfree(clips); out_err1: - drm_modeset_unlock_all(dev); + drm_framebuffer_unreference(fb); + return ret; }

With the prep patch to encapsulate ->set_crtc calls, this is now rather easy. Hooray for inconsistent semantics between ->set_crtc and ->page_flip, where the driver callback is supposed to update the fb pointer, and ->update_plane, where the drm core does the same.

Also, since the drm core functions check crtc->fb before calling into driver callbacks, we can't really reduce the critical sections protected by the mode_config locks.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 57ddf2f..61adee6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1984,8 +1984,21 @@ out: int drm_mode_set_config_internal(struct drm_mode_set *set) { struct drm_crtc *crtc = set->crtc; + struct drm_framebuffer *fb, *old_fb; + int ret; + + old_fb = crtc->fb; + fb = set->fb;

- return crtc->funcs->set_config(set); + ret = crtc->funcs->set_config(set); + if (ret == 0) { + if (old_fb) + drm_framebuffer_unreference(old_fb); + if (fb) + drm_framebuffer_reference(fb); + } + + return ret; } EXPORT_SYMBOL(drm_mode_set_config_internal);

@@ -2046,6 +2059,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; } fb = crtc->fb; + /* Make refcounting symmetric with the lookup path. */ + drm_framebuffer_reference(fb); } else { fb = drm_framebuffer_lookup(dev, crtc_req->fb_id); if (!fb) { @@ -2054,9 +2069,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, ret = -EINVAL; goto out; } - /* fb is protect by the mode_config lock, so drop the - * ref immediately */ - drm_framebuffer_unreference(fb); }

mode = drm_mode_create(dev); @@ -2156,6 +2168,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, ret = drm_mode_set_config_internal(&set);

out: + if (fb) + drm_framebuffer_unreference(fb); + kfree(connector_set); drm_mode_destroy(dev, mode); drm_modeset_unlock_all(dev); @@ -3656,7 +3671,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_mode_crtc_page_flip *page_flip = data; struct drm_mode_object *obj; struct drm_crtc *crtc; - struct drm_framebuffer *fb; + struct drm_framebuffer *fb = NULL, *old_fb = NULL; struct drm_pending_vblank_event *e = NULL; unsigned long flags; int hdisplay, vdisplay; @@ -3687,8 +3702,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, fb = drm_framebuffer_lookup(dev, page_flip->fb_id); if (!fb) goto out; - /* fb is protect by the mode_config lock, so drop the ref immediately */ - drm_framebuffer_unreference(fb);

hdisplay = crtc->mode.hdisplay; vdisplay = crtc->mode.vdisplay; @@ -3734,6 +3747,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, (void (*) (struct drm_pending_event *)) kfree; }

+ old_fb = crtc->fb; ret = crtc->funcs->page_flip(crtc, fb, e); if (ret) { if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { @@ -3742,9 +3756,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); } + /* Keep the old fb, don't unref it. */ + old_fb = NULL; + } else { + /* Unref only the old framebuffer. */ + fb = NULL; }

out: + if (fb) + drm_framebuffer_unreference(fb); + if (old_fb) + drm_framebuffer_unreference(old_fb); drm_modeset_unlock_all(dev); return ret; }

Now that all framebuffer usage is properly refcounted, we are no longer required to hold the modeset locks while dropping the last reference. Hence implemented a fastpath which avoids the potential stalls associated with grabbing mode_config.lock for the case where there's no other reference around.

Explain in a big comment why it is safe. Also update kerneldocs with the new locking rules around drm_framebuffer_remove.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 72 +++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 61adee6..1e238c1 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -517,7 +517,11 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); * * Scans all the CRTCs and planes in @dev's mode_config. If they're * using @fb, removes it, setting it to NULL. Then drops the reference to the - * passed-in framebuffer. + * passed-in framebuffer. Might take the modeset locks. + * + * Note that this function optimizes the cleanup away if the caller holds the + * last reference to the framebuffer. It is also guaranteed to not take the + * modeset locks in this case. */ void drm_framebuffer_remove(struct drm_framebuffer *fb) { @@ -527,33 +531,51 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) struct drm_mode_set set; int ret;

- WARN_ON(!drm_modeset_is_locked(dev)); WARN_ON(!list_empty(&fb->filp_head));

- /* remove from any CRTC */ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb == fb) { - /* should turn off the crtc */ - memset(&set, 0, sizeof(struct drm_mode_set)); - set.crtc = crtc; - set.fb = NULL; - ret = drm_mode_set_config_internal(&set); - if (ret) - DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); + /* + * drm ABI mandates that we remove any deleted framebuffers from active + * useage. But since most sane clients only remove framebuffers they no + * longer need, try to optimize this away. + * + * Since we're holding a reference ourselves, observing a refcount of 1 + * means that we're the last holder and can skip it. Also, the refcount + * can never increase from 1 again, so we don't need any barriers or + * locks. + * + * Note that userspace could try to race with use and instate a new + * usage _after_ we've cleared all current ones. End result will be an + * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot + * in this manner. + */ + if (atomic_read(&fb->refcount.refcount) > 1) { + drm_modeset_lock_all(dev); + /* remove from any CRTC */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + if (crtc->fb == fb) { + /* should turn off the crtc */ + memset(&set, 0, sizeof(struct drm_mode_set)); + set.crtc = crtc; + set.fb = NULL; + ret = drm_mode_set_config_internal(&set); + if (ret) + DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); + } } - }

- list_for_each_entry(plane, &dev->mode_config.plane_list, head) { - if (plane->fb == fb) { - /* should turn off the crtc */ - ret = plane->funcs->disable_plane(plane); - if (ret) - DRM_ERROR("failed to disable plane with busy fb\n"); - /* disconnect the plane from the fb and crtc: */ - __drm_framebuffer_unreference(plane->fb); - plane->fb = NULL; - plane->crtc = NULL; + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + if (plane->fb == fb) { + /* should turn off the crtc */ + ret = plane->funcs->disable_plane(plane); + if (ret) + DRM_ERROR("failed to disable plane with busy fb\n"); + /* disconnect the plane from the fb and crtc: */ + __drm_framebuffer_unreference(plane->fb); + plane->fb = NULL; + plane->crtc = NULL; + } } + drm_modeset_unlock_all(dev); }

drm_framebuffer_unreference(fb); @@ -2538,9 +2560,7 @@ int drm_mode_rmfb(struct drm_device *dev, mutex_unlock(&dev->mode_config.fb_lock); mutex_unlock(&file_priv->fbs_lock);

- drm_modeset_lock_all(dev); drm_framebuffer_remove(fb); - drm_modeset_unlock_all(dev);

return 0;

@@ -2691,9 +2711,7 @@ void drm_fb_release(struct drm_file *priv) list_del_init(&fb->filp_head);

/* This will also drop the fpriv->fbs reference. */ - drm_modeset_lock_all(dev); drm_framebuffer_remove(fb); - drm_modeset_unlock_all(dev); } mutex_unlock(&priv->fbs_lock); }

The coup de grace of the entire journey. No more dropped frames every 10s on my testbox!

I've tried to audit all ->detect and ->get_modes callbacks, but things became a bit fuzzy after trying to piece together the umpteenth implemenation. Afaict most drivers just have bog-standard output register frobbing with a notch of i2c edid reading, nothing which could potentially race with the newly concurrent pageflip/set_cursor code. The big exception is load-detection code which requires a running pipe, but radeon/nouveau seem to to this without touching any state which can be observed from page_flip (e.g. disabled crtcs temporarily getting enabled and so a pageflip succeeding).

The only special case I could find is the i915 load detect code. That uses the normal modeset interface to enable the load-detect crtc, and so userspace could try to squeeze in a pageflip on the load-detect pipe. So we need to grab the relevant crtc mutex in there, to avoid the temporary crtc enabling to sneak out and be visible to userspace.

Note that the sysfs files already stopped grabbing the per-crtc locks, since I didn't want to bother with doing a interruptible modeset_lock_all. But since there's very little in-between breakage (essentially just the ability for userspace to pageflip on load-detect crtcs when it shouldn't on the i915 driver) I figured I don't need to bother.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 5 +++-- drivers/gpu/drm/drm_crtc_helper.c | 8 ++++---- drivers/gpu/drm/i915/intel_display.c | 10 ++++++++-- 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 16ee864..8e4c574 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1617,7 +1617,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,

DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);

- drm_modeset_lock_all(dev); + mutex_lock(&dev->mode_config.mutex);

obj = drm_mode_object_find(dev, out_resp->connector_id, DRM_MODE_OBJECT_CONNECTOR); @@ -1714,7 +1714,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->count_encoders = encoders_count;

out: - drm_modeset_unlock_all(dev); + mutex_unlock(&dev->mode_config.mutex); + return ret; }

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 400ef86..7b2d378 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -980,7 +980,7 @@ static void output_poll_execute(struct work_struct *work) if (!drm_kms_helper_poll) return;

- drm_modeset_lock_all(dev); + mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Ignore forced connectors. */ @@ -1010,7 +1010,7 @@ static void output_poll_execute(struct work_struct *work) changed = true; }

- drm_modeset_unlock_all(dev); + mutex_unlock(&dev->mode_config.mutex);

if (changed) drm_kms_helper_hotplug_event(dev); @@ -1070,7 +1070,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) if (!dev->mode_config.poll_enabled) return;

- drm_modeset_lock_all(dev); + mutex_lock(&dev->mode_config.mutex); list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

/* Only handle HPD capable connectors. */ @@ -1088,7 +1088,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) changed = true; }

- drm_modeset_unlock_all(dev); + mutex_unlock(&dev->mode_config.mutex);

if (changed) drm_kms_helper_hotplug_event(dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 06fbf3e..9095d41 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6416,6 +6416,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, if (encoder->crtc) { crtc = encoder->crtc;

+ mutex_lock(&crtc->mutex); + old->dpms_mode = connector->dpms; old->load_detect_temp = false;

@@ -6445,6 +6447,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, return false; }

+ mutex_lock(&crtc->mutex); intel_encoder->new_crtc = to_intel_crtc(crtc); to_intel_connector(connector)->new_encoder = intel_encoder;

@@ -6472,6 +6475,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n"); if (IS_ERR(fb)) { DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n"); + mutex_unlock(&crtc->mutex); return false; }

@@ -6479,6 +6483,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); if (old->release_fb) old->release_fb->funcs->destroy(old->release_fb); + mutex_unlock(&crtc->mutex); return false; }

@@ -6493,14 +6498,13 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, struct intel_encoder *intel_encoder = intel_attached_encoder(connector); struct drm_encoder *encoder = &intel_encoder->base; + struct drm_crtc *crtc = encoder->crtc;

DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", connector->base.id, drm_get_connector_name(connector), encoder->base.id, drm_get_encoder_name(encoder));

if (old->load_detect_temp) { - struct drm_crtc *crtc = encoder->crtc; - to_intel_connector(connector)->new_encoder = NULL; intel_encoder->new_crtc = NULL; intel_set_mode(crtc, NULL, 0, 0, NULL); @@ -6516,6 +6520,8 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, /* Switch crtc and encoder back off if necessary */ if (old->dpms_mode != DRM_MODE_DPMS_ON) connector->funcs->dpms(connector, old->dpms_mode); + + mutex_unlock(&crtc->mutex); }

/* Returns the clock of the currently programmed mode of the given pipe. */

We need to make sure that the fbcon is still bound when touching the hw, since otherwise we might corrupt the modeset state of kms clients. X mostly works around that with VT switching and setting the VT into raw mode, which disables most fbcon events.

Raw kms test programs though don't do that dance, and in the future we might want to aim to abolish CONFIG_VT anyway. So improve preventive measures a bit. To do so, extract the existing logic for handling hotplug events (which X can't block with the current set of tricks) and reuse it for the fbdev blanking helper.

Long-term we really need to either scrap this all and only have a OOPS console, or come up with a saner model for device ownership sharing between fbdev/fbcon and kms userspace.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index be0f2d6..0c6e25e 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -305,6 +305,24 @@ void drm_fb_helper_restore(void) } EXPORT_SYMBOL(drm_fb_helper_restore);

+static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper) +{ + struct drm_device *dev = fb_helper->dev; + struct drm_crtc *crtc; + int bound = 0, crtcs_bound = 0; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + if (crtc->fb) + crtcs_bound++; + if (crtc->fb == fb_helper->fb) + bound++; + } + + if (bound < crtcs_bound) + return false; + return true; +} + #ifdef CONFIG_MAGIC_SYSRQ static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) { @@ -338,6 +356,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) * For each CRTC in this fb, turn the connectors on/off. */ drm_modeset_lock_all(dev); + if (!drm_fb_helper_is_bound(fb_helper)) { + drm_modeset_unlock_all(dev); + return; + } + for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc;

@@ -702,6 +725,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, int i;

drm_modeset_lock_all(dev); + if (!drm_fb_helper_is_bound(fb_helper)) { + drm_modeset_unlock_all(dev); + return -EBUSY; + } + for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc;

@@ -1369,21 +1397,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) struct drm_device *dev = fb_helper->dev; int count = 0; u32 max_width, max_height, bpp_sel; - int bound = 0, crtcs_bound = 0; - struct drm_crtc *crtc;

if (!fb_helper->fb) return 0;

drm_modeset_lock_all(dev); - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb) - crtcs_bound++; - if (crtc->fb == fb_helper->fb) - bound++; - } - - if (bound < crtcs_bound) { + if (!drm_fb_helper_is_bound(fb_helper)) { fb_helper->delayed_hotplug = true; drm_modeset_unlock_all(dev); return 0;