Crank up the warning a notch and point at the right set of locking functions for atomic drivers.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index aac9122f1da2..b2d20eb6c807 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p, * to dmesg in case of error irq's. (Hint, you probably want to * ratelimit this!) * - * The caller must drm_modeset_lock_all(), or if this is called - * from error irq handler, it should not be enabled by default. - * (Ie. if you are debugging errors you might not care that this - * is racey. But calling this without all modeset locks held is - * not inherently safe.) + * The caller must wrap this drm_modeset_lock_all_ctx() and + * drm_modeset_drop_locks(). If this is called from error irq handler, it should + * not be enabled by default - if you are debugging errors you might + * not care that this is racey, but calling this without all modeset locks held + * is inherently unsafe. */ void drm_state_dump(struct drm_device *dev, struct drm_printer *p) {
Hi,
On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
Crank up the warning a notch and point at the right set of locking functions for atomic drivers.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index aac9122f1da2..b2d20eb6c807 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
- to dmesg in case of error irq's. (Hint, you probably want to
- ratelimit this!)
- The caller must drm_modeset_lock_all(), or if this is called
- from error irq handler, it should not be enabled by default.
- (Ie. if you are debugging errors you might not care that this
- is racey. But calling this without all modeset locks held is
- not inherently safe.)
- The caller must wrap this drm_modeset_lock_all_ctx() and
- drm_modeset_drop_locks(). If this is called from error irq handler, it should
- not be enabled by default - if you are debugging errors you might
- not care that this is racey, but calling this without all modeset locks held
*/
- is inherently unsafe.
void drm_state_dump(struct drm_device *dev, struct drm_printer *p) {
For the comment itself: Acked-by: Maxime Ripard mripard@kernel.org
But maybe we should add some lockdep assertion to make sure we can catch someone actually doing this?
On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard maxime@cerno.tech wrote:
Hi,
On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
Crank up the warning a notch and point at the right set of locking functions for atomic drivers.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index aac9122f1da2..b2d20eb6c807 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
- to dmesg in case of error irq's. (Hint, you probably want to
- ratelimit this!)
- The caller must drm_modeset_lock_all(), or if this is called
- from error irq handler, it should not be enabled by default.
- (Ie. if you are debugging errors you might not care that this
- is racey. But calling this without all modeset locks held is
- not inherently safe.)
- The caller must wrap this drm_modeset_lock_all_ctx() and
- drm_modeset_drop_locks(). If this is called from error irq handler, it should
- not be enabled by default - if you are debugging errors you might
- not care that this is racey, but calling this without all modeset locks held
*/
- is inherently unsafe.
void drm_state_dump(struct drm_device *dev, struct drm_printer *p) {
For the comment itself: Acked-by: Maxime Ripard mripard@kernel.org
Thanks for taking a look, will merge.
But maybe we should add some lockdep assertion to make sure we can catch someone actually doing this?
I think it has some use for ad-hoc debugging in random places, or maybe as a an opt-in "tains your kernel" debug option. And then you really don't want your useful debug output burried in a few pages of lockdep splat first :-) -Daniel
On Fri, Oct 02, 2020 at 02:37:25PM +0200, Daniel Vetter wrote:
On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard maxime@cerno.tech wrote:
On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
Crank up the warning a notch and point at the right set of locking functions for atomic drivers.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index aac9122f1da2..b2d20eb6c807 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
- to dmesg in case of error irq's. (Hint, you probably want to
- ratelimit this!)
- The caller must drm_modeset_lock_all(), or if this is called
- from error irq handler, it should not be enabled by default.
- (Ie. if you are debugging errors you might not care that this
- is racey. But calling this without all modeset locks held is
- not inherently safe.)
- The caller must wrap this drm_modeset_lock_all_ctx() and
- drm_modeset_drop_locks(). If this is called from error irq handler, it should
- not be enabled by default - if you are debugging errors you might
- not care that this is racey, but calling this without all modeset locks held
*/
- is inherently unsafe.
void drm_state_dump(struct drm_device *dev, struct drm_printer *p) {
For the comment itself: Acked-by: Maxime Ripard mripard@kernel.org
Thanks for taking a look, will merge.
But maybe we should add some lockdep assertion to make sure we can catch someone actually doing this?
I think it has some use for ad-hoc debugging in random places, or maybe as a an opt-in "tains your kernel" debug option. And then you really don't want your useful debug output burried in a few pages of lockdep splat first :-)
Yeah, but at the same time reducing the discoverability of this locking expectation for the common case to favor some one-off debugging by someone that has more chance to know better seems like not the best trade-off.
Or maybe make the assertion conditional on drm.debug not being set or something?
Maxime
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maxime Ripard