Re: [PATCH v2] drm: Reverse lock order in DRM display-panning helpers()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 11, 2019 at 04:31:14PM +0200, Thomas Zimmermann wrote:
> During display panning, acquiring drm_client_dev.modeset_mutex after the
> locks in drm_fb_helper.dev creates a deadlock with drm_setup_crtcs() as
> shown below:
> 
>   [    4.959319] fbcon: radeondrmfb (fb0) is primary device
>   [    4.993952] Console: switching to colour frame buffer device 240x67
>   [    4.994040]
>   [    4.994041] ======================================================
>   [    4.994041] WARNING: possible circular locking dependency detected
>   [    4.994042] 5.2.0-rc4-1-default+ #39 Tainted: G            E
>   [    4.994043] ------------------------------------------------------
>   [    4.994043] systemd-udevd/369 is trying to acquire lock:
>   [    4.994044] 00000000fb622acb (&client->modeset_mutex){+.+.}, at: drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>   [    4.994055]
>   [    4.994055] but task is already holding lock:
>   [    4.994055] 0000000028767ae4 (crtc_ww_class_mutex){+.+.}, at: drm_modeset_lock+0x42/0xf0 [drm]
>   [    4.994072]
>   [    4.994072] which lock already depends on the new lock.
>   [    4.994072]
>   [    4.994072]
>   [    4.994072] the existing dependency chain (in reverse order) is:
>   [    4.994073]
>   [    4.994073] -> #3 (crtc_ww_class_mutex){+.+.}:
>   [    4.994076]        lock_acquire+0x9e/0x170
>   [    4.994079]        __ww_mutex_lock.constprop.18+0x97/0xf40
>   [    4.994080]        ww_mutex_lock+0x30/0x90
>   [    4.994091]        drm_modeset_lock+0x42/0xf0 [drm]
>   [    4.994102]        drm_modeset_lock_all_ctx+0x1f/0xe0 [drm]
>   [    4.994113]        drm_modeset_lock_all+0x5e/0x1a0 [drm]
>   [    4.994163]        intel_modeset_init+0x60b/0xda0 [i915]
>   ..
>   [    4.994253]
>   [    4.994253] -> #2 (crtc_ww_class_acquire){+.+.}:
>   [    4.994255]        lock_acquire+0x9e/0x170
>   [    4.994270]        drm_modeset_acquire_init+0xcc/0x100 [drm]
>   [    4.994280]        drm_modeset_lock_all+0x44/0x1a0 [drm]
>   [    4.994320]        intel_modeset_init+0x60b/0xda0 [i915]
>   ..
>   [    4.994403]
>   [    4.994403] -> #1 (&dev->mode_config.mutex){+.+.}:
>   [    4.994405]        lock_acquire+0x9e/0x170
>   [    4.994408]        __mutex_lock+0x62/0x8c0
>   [    4.994413]        drm_setup_crtcs+0x17c/0xc50 [drm_kms_helper]
>   [    4.994418]        __drm_fb_helper_initial_config_and_unlock+0x34/0x530 [drm_kms_helper]
>   [    4.994450]        radeon_fbdev_init+0x110/0x130 [radeon]
>   ..
>   [    4.994535]
>   [    4.994535] -> #0 (&client->modeset_mutex){+.+.}:
>   [    4.994537]        __lock_acquire+0xa85/0xe90
>   [    4.994538]        lock_acquire+0x9e/0x170
>   [    4.994540]        __mutex_lock+0x62/0x8c0
>   [    4.994545]        drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>   [    4.994547]        fb_pan_display+0x92/0x120
>   [    4.994549]        bit_update_start+0x1a/0x40
>   [    4.994550]        fbcon_switch+0x392/0x580
>   [    4.994552]        redraw_screen+0x12c/0x220
>   [    4.994553]        do_bind_con_driver.cold.30+0xe1/0x10d
>   [    4.994554]        do_take_over_console+0x113/0x190
>   [    4.994555]        do_fbcon_takeover+0x58/0xb0
>   [    4.994557]        notifier_call_chain+0x47/0x70
>   [    4.994558]        blocking_notifier_call_chain+0x44/0x60
>   [    4.994559]        register_framebuffer+0x231/0x310
>   [    4.994564]        __drm_fb_helper_initial_config_and_unlock+0x2fd/0x530 [drm_kms_helper]
>   [    4.994590]        radeon_fbdev_init+0x110/0x130 [radeon]
>   ..
> 
> This problem was introduced in
> 
>   d81294afe	drm/fb-helper: Remove drm_fb_helper_crtc
> 
> Reversing the lock ordering in the display-panning code fixes the issue. In
> addition to the fix, the DRM client now implements the panning. The call to
> drm_client_modeset_pan_display() acquires drm_client_dev.modeset_mutex once
> for the whole operation.
> 
> The change was suggested by Daniel Vetter and Noralf Tronnes.
> 
> v2:
> 	* move display panning to DRM client
> 	* acquire modeset_mutex once in drm_client_modeset_pan_display()
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Fixes: d81294afeecdacc8d84804ba0bcb3d39e64d0f27

This won't pass dim's checks. See

$ dim fixes <sha1>

for a tool for this.

Wrt the bugfix, I'd say lets go with your simpler fix first, since I think
this one here needs a bit more work to provide a nice interface.

> ---
>  drivers/gpu/drm/drm_client_modeset.c | 110 +++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_fb_helper.c      |  79 ++-----------------
>  include/drm/drm_client.h             |   4 +
>  3 files changed, 117 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index b1984f901a6d..20c8fe94249e 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -288,6 +288,19 @@ static int drm_client_modeset_commit_legacy(struct drm_client_dev *client)
>  	return ret;
>  }
>  
> +static int __drm_client_modeset_commit_force(struct drm_client_dev *client)
> +{
> +	struct drm_device *dev = client->dev;
> +	int ret;
> +
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		ret = drm_client_modeset_commit_atomic(client, true);
> +	else
> +		ret = drm_client_modeset_commit_legacy(client);
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_client_modeset_commit_force() - Force commit CRTC configuration
>   * @client: DRM client
> @@ -299,14 +312,10 @@ static int drm_client_modeset_commit_legacy(struct drm_client_dev *client)
>   */
>  int drm_client_modeset_commit_force(struct drm_client_dev *client)
>  {
> -	struct drm_device *dev = client->dev;
>  	int ret;
>  
>  	mutex_lock(&client->modeset_mutex);
> -	if (drm_drv_uses_atomic_modeset(dev))
> -		ret = drm_client_modeset_commit_atomic(client, true);
> -	else
> -		ret = drm_client_modeset_commit_legacy(client);
> +	ret = __drm_client_modeset_commit_force(client);
>  	mutex_unlock(&client->modeset_mutex);
>  
>  	return ret;
> @@ -390,3 +399,94 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_client_modeset_dpms);
> +
> +static void pan_set(struct drm_client_dev *client, unsigned int x,
> +		    unsigned int y)
> +{
> +	struct drm_mode_set *mode_set;
> +
> +	drm_client_for_each_modeset(mode_set, client) {
> +		mode_set->x = x;
> +		mode_set->y = y;
> +	}
> +}
> +
> +static int drm_client_modeset_pan_display_atomic(struct drm_client_dev *client,
> +						 unsigned int x,
> +						 unsigned int y,
> +						 unsigned int default_x,
> +						 unsigned int default_y)
> +{
> +	int ret;
> +
> +	pan_set(client, x, y);
> +
> +	ret = __drm_client_modeset_commit_force(client);
> +	if (ret)
> +		pan_set(client, default_x, default_y);
> +
> +	return ret;
> +}
> +
> +static int drm_client_modeset_pan_display_legacy(struct drm_client_dev *client,
> +						 unsigned int x,
> +						 unsigned int y)
> +{
> +	struct drm_device *dev = client->dev;
> +	struct drm_mode_set *modeset;
> +	int ret = 0;
> +
> +	drm_modeset_lock_all(dev);
> +	drm_client_for_each_modeset(modeset, client) {
> +		modeset->x = x;
> +		modeset->y = y;
> +
> +		if (modeset->num_connectors)
> +			ret = drm_mode_set_config_internal(modeset);
> +	}
> +	drm_modeset_unlock_all(dev);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_client_modeset_dpms() - Set display panning
> + * @client: DRM client
> + * @x: Horizontal coordinate
> + * @y: Vertical coordinate
> + * @default_x: Fallback horizontal coordinate
> + * @default_y: Fallback vertical coordinate

Why do we need these? Can't we make a local copy of modeset->x/y and
restore them if it all goes boom? Would be a much nicer interface imo.
-Daniel

> + *
> + * This function sets the display panning for screens with a virtual
> + * resolution. The x and y coordinates refer to the display's upper
> + * left pixel. If updating fails, the fallback coordinates provide a
> + * safe default value, such as (0,0).
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_pan_display(struct drm_client_dev *client,
> +				   unsigned int x, unsigned int y,
> +				   unsigned int default_x,
> +				   unsigned int default_y)
> +{
> +	struct drm_device *dev = client->dev;
> +	int ret;
> +
> +	if (!drm_master_internal_acquire(dev))
> +		return -EBUSY;
> +
> +	mutex_lock(&client->modeset_mutex);
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		ret = drm_client_modeset_pan_display_atomic(client, x, y,
> +							    default_x,
> +							    default_y);
> +	else
> +		ret = drm_client_modeset_pan_display_legacy(client, x, y);
> +	mutex_unlock(&client->modeset_mutex);
> +
> +	drm_master_internal_release(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_pan_display);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7b388674a456..482e92c182c8 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1548,64 +1548,6 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_set_par);
>  
> -static void pan_set(struct drm_fb_helper *fb_helper, int x, int y)
> -{
> -	struct drm_mode_set *mode_set;
> -
> -	mutex_lock(&fb_helper->client.modeset_mutex);
> -	drm_client_for_each_modeset(mode_set, &fb_helper->client) {
> -		mode_set->x = x;
> -		mode_set->y = y;
> -	}
> -	mutex_unlock(&fb_helper->client.modeset_mutex);
> -}
> -
> -static int pan_display_atomic(struct fb_var_screeninfo *var,
> -			      struct fb_info *info)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -	int ret;
> -
> -	pan_set(fb_helper, var->xoffset, var->yoffset);
> -
> -	ret = drm_client_modeset_commit_force(&fb_helper->client);
> -	if (!ret) {
> -		info->var.xoffset = var->xoffset;
> -		info->var.yoffset = var->yoffset;
> -	} else
> -		pan_set(fb_helper, info->var.xoffset, info->var.yoffset);
> -
> -	return ret;
> -}
> -
> -static int pan_display_legacy(struct fb_var_screeninfo *var,
> -			      struct fb_info *info)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_client_dev *client = &fb_helper->client;
> -	struct drm_mode_set *modeset;
> -	int ret = 0;
> -
> -	drm_modeset_lock_all(fb_helper->dev);
> -	mutex_lock(&client->modeset_mutex);
> -	drm_client_for_each_modeset(modeset, client) {
> -		modeset->x = var->xoffset;
> -		modeset->y = var->yoffset;
> -
> -		if (modeset->num_connectors) {
> -			ret = drm_mode_set_config_internal(modeset);
> -			if (!ret) {
> -				info->var.xoffset = var->xoffset;
> -				info->var.yoffset = var->yoffset;
> -			}
> -		}
> -	}
> -	mutex_unlock(&client->modeset_mutex);
> -	drm_modeset_unlock_all(fb_helper->dev);
> -
> -	return ret;
> -}
> -
>  /**
>   * drm_fb_helper_pan_display - implementation for &fb_ops.fb_pan_display
>   * @var: updated screen information
> @@ -1615,27 +1557,22 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  			      struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_device *dev = fb_helper->dev;
>  	int ret;
>  
>  	if (oops_in_progress)
>  		return -EBUSY;
>  
>  	mutex_lock(&fb_helper->lock);
> -	if (!drm_master_internal_acquire(dev)) {
> -		ret = -EBUSY;
> -		goto unlock;
> -	}
> -
> -	if (drm_drv_uses_atomic_modeset(dev))
> -		ret = pan_display_atomic(var, info);
> -	else
> -		ret = pan_display_legacy(var, info);
> -
> -	drm_master_internal_release(dev);
> -unlock:
> +	ret = drm_client_modeset_pan_display(&fb_helper->client, var->xoffset,
> +					     var->yoffset, info->var.xoffset,
> +					     info->var.yoffset);
>  	mutex_unlock(&fb_helper->lock);
>  
> +	if (!ret) {
> +		info->var.xoffset = var->xoffset;
> +		info->var.yoffset = var->yoffset;
> +	}
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_pan_display);
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 6cf48419f77f..37bf55634227 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -159,6 +159,10 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>  int drm_client_modeset_commit_force(struct drm_client_dev *client);
>  int drm_client_modeset_commit(struct drm_client_dev *client);
>  int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
> +int drm_client_modeset_pan_display(struct drm_client_dev *client,
> +				   unsigned int x, unsigned int y,
> +				   unsigned int default_x,
> +				   unsigned int default_y);
>  
>  /**
>   * drm_client_for_each_modeset() - Iterate over client modesets
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux