[PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[Daniel Vetter]

We have two calling contexts for thise function:

- In the crtc helper code itself as part of the ->set_config
  implementation. In this calling context all modeset locks are
  already held, as they should.

- In drivers not implementing fastboot before the fbdev/fbcon setup
  and initialization. This has been added for all drivers in

  commit 76a39dbfb2d1bc45219839e5a95d4ceaf6ca114f
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Sun Jan 20 23:12:54 2013 +0100

      drm/fb-helper: don't disable everything in initial_config

  In this calling context we do not hold any modeset locks since the
  immediately following call to initialize the fbev emulation grabs
  all these locks themselves.

- There are two exceptions to the above rule: shmob doesn't have fbdev
  emulation support. I've manually checked the callchain up to the
  driver load function and no kms locks are held.

The right fix therefore is to split this helper into an internal and
external version and add the required locking to the function exported
to drivers.

This remedies locking inconsistencies exposed by me adding locking
WARNs as part of the recent kerneldoc abi polishing done in

commit 62ff94a5492175759546f8bc61383189d6b49122
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 22:18:47 2014 +0100

    drm/crtc-helper: remove LOCKING from kerneldoc

and

commit 63951385052f7974155fa38f962f0f4e9847f90a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 15:14:15 2014 +0100

    drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index a85517854073..9d8fb32357c0 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -278,17 +278,7 @@ drm_encoder_disable(struct drm_encoder *encoder)
 		encoder->bridge->funcs->post_disable(encoder->bridge);
 }
 
-/**
- * drm_helper_disable_unused_functions - disable unused objects
- * @dev: DRM device
- *
- * This function walks through the entire mode setting configuration of @dev. It
- * will remove any crtc links of unused encoders and encoder links of
- * disconnected connectors. Then it will disable all unused encoders and crtcs
- * either by calling their disable callback if available or by calling their
- * dpms callback with DRM_MODE_DPMS_OFF.
- */
-void drm_helper_disable_unused_functions(struct drm_device *dev)
+static void __drm_helper_disable_unused_functions(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
@@ -323,6 +313,23 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
 		}
 	}
 }
+
+/**
+ * drm_helper_disable_unused_functions - disable unused objects
+ * @dev: DRM device
+ *
+ * This function walks through the entire mode setting configuration of @dev. It
+ * will remove any crtc links of unused encoders and encoder links of
+ * disconnected connectors. Then it will disable all unused encoders and crtcs
+ * either by calling their disable callback if available or by calling their
+ * dpms callback with DRM_MODE_DPMS_OFF.
+ */
+void drm_helper_disable_unused_functions(struct drm_device *dev)
+{
+	drm_modeset_lock_all(dev);
+	__drm_helper_disable_unused_functions(dev);
+	drm_modeset_unlock_all(dev);
+}
 EXPORT_SYMBOL(drm_helper_disable_unused_functions);
 
 /*
-- 
1.8.4.rc3

[PATCH 2/2] drm/fb-helper: improve drm_fb_helper_initial_config locking

[Daniel Vetter]

The locking in drm_fb_helper_initial_config is a bit troublesome for a
few reasons:

- We can't just wrap the entire function up into modeset locks since
  the fbdev registration might call down into fbcon code, which then
  through our ->set_par implementation needs to be able to grab all
  modeset locks. So we'd have a neat deadlock.

- This implies though that all current callers don't hold any modeset
  locks by necessity, so we have free reign to grab any modeset locks
  we need to grab.

- The private state of the fbdev helper doesn't need any protection
  through locks, since once we have the fbdev registered it is mostly
  invariant or protected through the modeset locking in ->set_par and
  other callbacks. We can fully rely on driver having non-racy setup
  sequences here. For the initial config computation we actually may
  not grab locks since drivers which provide their own magic sauce
  (like i915) might need to grab locks themselves.

- We should grab locks though when we probe outputs. Currently there's
  not much risk, but already now userspace could start poking at sysfs
  files and so probe concurrently. I expect that in the future driver
  init will be much more async, and since probing is really
  time-consuming this is a prime candidate.

- We must not hold any crtc->mutex locks while calling probe functions
  since those might need to lock a crtc for e.g. load detection. i915
  is such a driver.

Also it's the probing calls which hit upon piles of new locking
asserts I've recently added in

commit 62ff94a5492175759546f8bc61383189d6b49122
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 22:18:47 2014 +0100

    drm/crtc-helper: remove LOCKING from kerneldoc

and

commit 63951385052f7974155fa38f962f0f4e9847f90a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 15:14:15 2014 +0100

    drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c

Hence the right fix is to grab the mode_config mutex, but only that
and only right around the probe calls.

It seems to be sufficient to shut up all the locking WARNINGs I see on
i915 and nouveau in drm_fb_helper_initial_config.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 87876198801d..16f271e21b9c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1536,9 +1536,11 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 
 	drm_fb_helper_parse_command_line(fb_helper);
 
+	mutex_lock(&dev->mode_config.mutex);
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
 						    dev->mode_config.max_width,
 						    dev->mode_config.max_height);
+	mutex_unlock(&dev->mode_config.mutex);
 	/*
 	 * we shouldn't end up with no modes here.
 	 */
-- 
1.8.4.rc3

Re: [PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[Chris Wilson]

On Thu, Mar 20, 2014 at 02:01:21PM +0100, Daniel Vetter wrote:
> We have two calling contexts for thise function:
> 
> - In the crtc helper code itself as part of the ->set_config
>   implementation. In this calling context all modeset locks are
>   already held, as they should.
> 
> - In drivers not implementing fastboot before the fbdev/fbcon setup
>   and initialization. This has been added for all drivers in
> 
>   commit 76a39dbfb2d1bc45219839e5a95d4ceaf6ca114f
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Sun Jan 20 23:12:54 2013 +0100
> 
>       drm/fb-helper: don't disable everything in initial_config
> 
>   In this calling context we do not hold any modeset locks since the
>   immediately following call to initialize the fbev emulation grabs
>   all these locks themselves.
> 
> - There are two exceptions to the above rule: shmob doesn't have fbdev
>   emulation support. I've manually checked the callchain up to the
>   driver load function and no kms locks are held.
> 
> The right fix therefore is to split this helper into an internal and
> external version and add the required locking to the function exported
> to drivers.
> 
> This remedies locking inconsistencies exposed by me adding locking
> WARNs as part of the recent kerneldoc abi polishing done in
> 
> commit 62ff94a5492175759546f8bc61383189d6b49122
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 22:18:47 2014 +0100
> 
>     drm/crtc-helper: remove LOCKING from kerneldoc
> 
> and
> 
> commit 63951385052f7974155fa38f962f0f4e9847f90a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 15:14:15 2014 +0100
> 
>     drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index a85517854073..9d8fb32357c0 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -278,17 +278,7 @@ drm_encoder_disable(struct drm_encoder *encoder)
>  		encoder->bridge->funcs->post_disable(encoder->bridge);
>  }
>  
> -/**
> - * drm_helper_disable_unused_functions - disable unused objects
> - * @dev: DRM device
> - *
> - * This function walks through the entire mode setting configuration of @dev. It
> - * will remove any crtc links of unused encoders and encoder links of
> - * disconnected connectors. Then it will disable all unused encoders and crtcs
> - * either by calling their disable callback if available or by calling their
> - * dpms callback with DRM_MODE_DPMS_OFF.
> - */
> -void drm_helper_disable_unused_functions(struct drm_device *dev)
> +static void __drm_helper_disable_unused_functions(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
> @@ -323,6 +313,23 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
>  		}
>  	}
>  }
> +
> +/**
> + * drm_helper_disable_unused_functions - disable unused objects
> + * @dev: DRM device
> + *
> + * This function walks through the entire mode setting configuration of @dev. It
> + * will remove any crtc links of unused encoders and encoder links of
> + * disconnected connectors. Then it will disable all unused encoders and crtcs
> + * either by calling their disable callback if available or by calling their
> + * dpms callback with DRM_MODE_DPMS_OFF.
> + */
> +void drm_helper_disable_unused_functions(struct drm_device *dev)
> +{
> +	drm_modeset_lock_all(dev);
> +	__drm_helper_disable_unused_functions(dev);
> +	drm_modeset_unlock_all(dev);
> +}
>  EXPORT_SYMBOL(drm_helper_disable_unused_functions);

drm_helper_disable_unused_functions() is called by
drm_crtc_helper_disable() which is expected to be holding the lock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[Daniel Vetter]

We have two calling contexts for thise function:

- In the crtc helper code itself as part of the ->set_config
  implementation. In this calling context all modeset locks are
  already held, as they should.

- In drivers not implementing fastboot before the fbdev/fbcon setup
  and initialization. This has been added for all drivers in

  commit 76a39dbfb2d1bc45219839e5a95d4ceaf6ca114f
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Sun Jan 20 23:12:54 2013 +0100

      drm/fb-helper: don't disable everything in initial_config

  In this calling context we do not hold any modeset locks since the
  immediately following call to initialize the fbev emulation grabs
  all these locks themselves.

- There are two exceptions to the above rule: shmob doesn't have fbdev
  emulation support. I've manually checked the callchain up to the
  driver load function and no kms locks are held.

The right fix therefore is to split this helper into an internal and
external version and add the required locking to the function exported
to drivers.

This remedies locking inconsistencies exposed by me adding locking
WARNs as part of the recent kerneldoc abi polishing done in

commit 62ff94a5492175759546f8bc61383189d6b49122
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 22:18:47 2014 +0100

    drm/crtc-helper: remove LOCKING from kerneldoc

and

commit 63951385052f7974155fa38f962f0f4e9847f90a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 15:14:15 2014 +0100

    drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c

v2: It helps when I actually git add the entire thing.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index a85517854073..5d2b7a5716e6 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -278,17 +278,7 @@ drm_encoder_disable(struct drm_encoder *encoder)
 		encoder->bridge->funcs->post_disable(encoder->bridge);
 }
 
-/**
- * drm_helper_disable_unused_functions - disable unused objects
- * @dev: DRM device
- *
- * This function walks through the entire mode setting configuration of @dev. It
- * will remove any crtc links of unused encoders and encoder links of
- * disconnected connectors. Then it will disable all unused encoders and crtcs
- * either by calling their disable callback if available or by calling their
- * dpms callback with DRM_MODE_DPMS_OFF.
- */
-void drm_helper_disable_unused_functions(struct drm_device *dev)
+static void __drm_helper_disable_unused_functions(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
@@ -323,6 +313,23 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
 		}
 	}
 }
+
+/**
+ * drm_helper_disable_unused_functions - disable unused objects
+ * @dev: DRM device
+ *
+ * This function walks through the entire mode setting configuration of @dev. It
+ * will remove any crtc links of unused encoders and encoder links of
+ * disconnected connectors. Then it will disable all unused encoders and crtcs
+ * either by calling their disable callback if available or by calling their
+ * dpms callback with DRM_MODE_DPMS_OFF.
+ */
+void drm_helper_disable_unused_functions(struct drm_device *dev)
+{
+	drm_modeset_lock_all(dev);
+	__drm_helper_disable_unused_functions(dev);
+	drm_modeset_unlock_all(dev);
+}
 EXPORT_SYMBOL(drm_helper_disable_unused_functions);
 
 /*
@@ -552,7 +559,7 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
 		}
 	}
 
-	drm_helper_disable_unused_functions(dev);
+	__drm_helper_disable_unused_functions(dev);
 	return 0;
 }
 
@@ -776,7 +783,7 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 				set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON);
 			}
 		}
-		drm_helper_disable_unused_functions(dev);
+		__drm_helper_disable_unused_functions(dev);
 	} else if (fb_changed) {
 		set->crtc->x = set->x;
 		set->crtc->y = set->y;
@@ -1009,7 +1016,7 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
 	}
 
 	/* disable the unused connectors while restoring the modesetting */
-	drm_helper_disable_unused_functions(dev);
+	__drm_helper_disable_unused_functions(dev);
 }
 EXPORT_SYMBOL(drm_helper_resume_force_mode);
 
-- 
1.8.4.rc3

[PATCH 2/2] drm/fb-helper: improve drm_fb_helper_initial_config locking

[Daniel Vetter]

The locking in drm_fb_helper_initial_config is a bit troublesome for a
few reasons:

- We can't just wrap the entire function up into modeset locks since
  the fbdev registration might call down into fbcon code, which then
  through our ->set_par implementation needs to be able to grab all
  modeset locks. So we'd have a neat deadlock.

- This implies though that all current callers don't hold any modeset
  locks by necessity, so we have free reign to grab any modeset locks
  we need to grab.

- The private state of the fbdev helper doesn't need any protection
  through locks, since once we have the fbdev registered it is mostly
  invariant or protected through the modeset locking in ->set_par and
  other callbacks. We can fully rely on driver having non-racy setup
  sequences here. For the initial config computation we actually may
  not grab locks since drivers which provide their own magic sauce
  (like i915) might need to grab locks themselves.

- We should grab locks though when we probe outputs. Currently there's
  not much risk, but already now userspace could start poking at sysfs
  files and so probe concurrently. I expect that in the future driver
  init will be much more async, and since probing is really
  time-consuming this is a prime candidate.

- We must not hold any crtc->mutex locks while calling probe functions
  since those might need to lock a crtc for e.g. load detection. i915
  is such a driver.

Also it's the probing calls which hit upon piles of new locking
asserts I've recently added in

commit 62ff94a5492175759546f8bc61383189d6b49122
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 22:18:47 2014 +0100

    drm/crtc-helper: remove LOCKING from kerneldoc

and

commit 63951385052f7974155fa38f962f0f4e9847f90a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jan 23 15:14:15 2014 +0100

    drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c

Hence the right fix is to grab the mode_config mutex, but only that
and only right around the probe calls.

It seems to be sufficient to shut up all the locking WARNINGs I see on
i915 and nouveau in drm_fb_helper_initial_config.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 87876198801d..16f271e21b9c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1536,9 +1536,11 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 
 	drm_fb_helper_parse_command_line(fb_helper);
 
+	mutex_lock(&dev->mode_config.mutex);
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
 						    dev->mode_config.max_width,
 						    dev->mode_config.max_height);
+	mutex_unlock(&dev->mode_config.mutex);
 	/*
 	 * we shouldn't end up with no modes here.
 	 */
-- 
1.8.4.rc3

Re: [PATCH 2/2] drm/fb-helper: improve drm_fb_helper_initial_config locking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 450 bytes --]

On Thu, Mar 20, 2014 at 02:26:35PM +0100, Daniel Vetter wrote:
[...]
> Hence the right fix is to grab the mode_config mutex, but only that
> and only right around the probe calls.
> 
> It seems to be sufficient to shut up all the locking WARNINGs I see on
> i915 and nouveau in drm_fb_helper_initial_config.

Fixes any of the WARNINGs remaining after patch 1/2 for me on Tegra as
well, so:

Tested-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[Chris Wilson]

On Thu, Mar 20, 2014 at 02:26:34PM +0100, Daniel Vetter wrote:
> We have two calling contexts for thise function:
> 
> - In the crtc helper code itself as part of the ->set_config
>   implementation. In this calling context all modeset locks are
>   already held, as they should.
> 
> - In drivers not implementing fastboot before the fbdev/fbcon setup
>   and initialization. This has been added for all drivers in
> 
>   commit 76a39dbfb2d1bc45219839e5a95d4ceaf6ca114f
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Sun Jan 20 23:12:54 2013 +0100
> 
>       drm/fb-helper: don't disable everything in initial_config
> 
>   In this calling context we do not hold any modeset locks since the
>   immediately following call to initialize the fbev emulation grabs
>   all these locks themselves.
> 
> - There are two exceptions to the above rule: shmob doesn't have fbdev
>   emulation support. I've manually checked the callchain up to the
>   driver load function and no kms locks are held.
> 
> The right fix therefore is to split this helper into an internal and
> external version and add the required locking to the function exported
> to drivers.
> 
> This remedies locking inconsistencies exposed by me adding locking
> WARNs as part of the recent kerneldoc abi polishing done in
> 
> commit 62ff94a5492175759546f8bc61383189d6b49122
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 22:18:47 2014 +0100
> 
>     drm/crtc-helper: remove LOCKING from kerneldoc
> 
> and
> 
> commit 63951385052f7974155fa38f962f0f4e9847f90a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 15:14:15 2014 +0100
> 
>     drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c
> 
> v2: It helps when I actually git add the entire thing.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 419 bytes --]

On Thu, Mar 20, 2014 at 02:26:34PM +0100, Daniel Vetter wrote:
[...]
> The right fix therefore is to split this helper into an internal and
> external version and add the required locking to the function exported
> to drivers.
> 
> This remedies locking inconsistencies exposed by me adding locking
> WARNs as part of the recent kerneldoc abi polishing done in

Tested-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel