| Message ID | 1424601516-19267-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 22 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > In > > daniel@phenom:~/linux/src$ git show ccfc08655 copy-paste fail? J. > commit ccfc08655d5fd5076828f45fb09194c070f2f63a > Author: Rob Clark <robdclark@gmail.com> > Date: Thu Dec 18 16:01:48 2014 -0500 > > drm: tweak getconnector locking > > We need to extend the locking to cover connector->state reading for > atomic drivers, but the above commit was a bit too eager and also > included the fill_modes callback. Which on i915 on old platforms using > load detection needs to acquire modeset locks, resulting in a deadlock > on output probing. > > Reported-by: Marc Finet <m.dreadlock@gmail.com> > Cc: Marc Finet <m.dreadlock@gmail.com> > Cc: robdclark@gmail.com > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b15d720eda4c..ce5f1193ecd6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2180,7 +2180,6 @@ 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(&dev->mode_config.connection_mutex, NULL); > > connector = drm_connector_find(dev, out_resp->connector_id); > if (!connector) { > @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->mm_height = connector->display_info.height_mm; > out_resp->subpixel = connector->display_info.subpixel_order; > out_resp->connection = connector->status; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > encoder = drm_connector_get_encoder(connector); > if (encoder) > out_resp->encoder_id = encoder->base.id; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Feb 22, 2015 at 5:38 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > In > > daniel@phenom:~/linux/src$ git show ccfc08655 > commit ccfc08655d5fd5076828f45fb09194c070f2f63a > Author: Rob Clark <robdclark@gmail.com> > Date: Thu Dec 18 16:01:48 2014 -0500 > > drm: tweak getconnector locking > > We need to extend the locking to cover connector->state reading for > atomic drivers, but the above commit was a bit too eager and also > included the fill_modes callback. Which on i915 on old platforms using > load detection needs to acquire modeset locks, resulting in a deadlock > on output probing. > > Reported-by: Marc Finet <m.dreadlock@gmail.com> > Cc: Marc Finet <m.dreadlock@gmail.com> > Cc: robdclark@gmail.com > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b15d720eda4c..ce5f1193ecd6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2180,7 +2180,6 @@ 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(&dev->mode_config.connection_mutex, NULL); > > connector = drm_connector_find(dev, out_resp->connector_id); > if (!connector) { > @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->mm_height = connector->display_info.height_mm; > out_resp->subpixel = connector->display_info.subpixel_order; > out_resp->connection = connector->status; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > encoder = drm_connector_get_encoder(connector); > if (encoder) > out_resp->encoder_id = encoder->base.id; > -- > 2.1.4 >
On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote: > In > > daniel@phenom:~/linux/src$ git show ccfc08655 > commit ccfc08655d5fd5076828f45fb09194c070f2f63a > Author: Rob Clark <robdclark@gmail.com> > Date: Thu Dec 18 16:01:48 2014 -0500 > > drm: tweak getconnector locking > > We need to extend the locking to cover connector->state reading for > atomic drivers, but the above commit was a bit too eager and also > included the fill_modes callback. Which on i915 on old platforms using > load detection needs to acquire modeset locks, resulting in a deadlock > on output probing. > > Reported-by: Marc Finet <m.dreadlock@gmail.com> > Cc: Marc Finet <m.dreadlock@gmail.com> > Cc: robdclark@gmail.com > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Tested-by: Marc Finet <m.dreadlock@gmail.com> Thanks. > --- > drivers/gpu/drm/drm_crtc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b15d720eda4c..ce5f1193ecd6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2180,7 +2180,6 @@ 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(&dev->mode_config.connection_mutex, NULL); > > connector = drm_connector_find(dev, out_resp->connector_id); > if (!connector) { > @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->mm_height = connector->display_info.height_mm; > out_resp->subpixel = connector->display_info.subpixel_order; > out_resp->connection = connector->status; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > encoder = drm_connector_get_encoder(connector); > if (encoder) > out_resp->encoder_id = encoder->base.id; > -- > 2.1.4 >
2015-02-24 1:55 GMT+02:00 Marc Finet <m.dreadlock@gmail.com>: > On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote: >> In >> >> daniel@phenom:~/linux/src$ git show ccfc08655 >> commit ccfc08655d5fd5076828f45fb09194c070f2f63a >> Author: Rob Clark <robdclark@gmail.com> >> Date: Thu Dec 18 16:01:48 2014 -0500 >> >> drm: tweak getconnector locking >> >> We need to extend the locking to cover connector->state reading for >> atomic drivers, but the above commit was a bit too eager and also >> included the fill_modes callback. Which on i915 on old platforms using >> load detection needs to acquire modeset locks, resulting in a deadlock >> on output probing. >> >> Reported-by: Marc Finet <m.dreadlock@gmail.com> >> Cc: Marc Finet <m.dreadlock@gmail.com> >> Cc: robdclark@gmail.com >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Tested-by: Marc Finet <m.dreadlock@gmail.com> > > Thanks. >> --- >> drivers/gpu/drm/drm_crtc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index b15d720eda4c..ce5f1193ecd6 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -2180,7 +2180,6 @@ 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(&dev->mode_config.connection_mutex, NULL); >> >> connector = drm_connector_find(dev, out_resp->connector_id); >> if (!connector) { >> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> out_resp->mm_height = connector->display_info.height_mm; >> out_resp->subpixel = connector->display_info.subpixel_order; >> out_resp->connection = connector->status; >> + >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> encoder = drm_connector_get_encoder(connector); >> if (encoder) >> out_resp->encoder_id = encoder->base.id; >> -- >> 2.1.4 >> Hi, I think this patch introduced the following. If the drm_connector_find() call fails, we jump to the 'out' label and call drm_modeset_unlock(), before the drm_modeset_lock() call. [ 59.076011] ===================================== [ 59.076011] [ BUG: bad unlock balance detected! ] [ 59.076011] 4.0.0-rc1+ #60 Tainted: G W [ 59.076011] ------------------------------------- [ 59.076011] trinity-c14/2418 is trying to release lock (crtc_ww_class_mutex) at: [ 59.076011] [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0 [ 59.076011] but there are no more locks to release! [ 59.076011] [ 59.076011] other info that might help us debug this: [ 59.076011] 1 lock held by trinity-c14/2418: [ 59.076011] #0: (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffff815ef253>] drm_mode_getconnector+0x83/0x400 [ 59.076011] [ 59.076011] stack backtrace: [ 59.076011] CPU: 0 PID: 2418 Comm: trinity-c14 Tainted: G W 4.0.0-rc1+ #60 [ 59.076011] Hardware name: Hewlett-Packard HP Compaq dc5750 Small Form Factor/0A64h, BIOS 786E3 v02.10 01/25/2007 [ 59.076011] ffffffff81dc2872 ffff88005b777b48 ffffffff81db6706 0000000000000000 [ 59.076011] ffff88005b719440 ffff88005b777b78 ffffffff810f8074 00000000001d51c0 [ 59.076011] ffff880069d3a5d0 ffff88005b719440 ffffffff81dc2872 ffff88005b777bf8 [ 59.076011] Call Trace: [ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0 [ 59.076011] [<ffffffff81db6706>] dump_stack+0x4c/0x65 [ 59.076011] [<ffffffff810f8074>] print_unlock_imbalance_bug+0xf4/0x100 [ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0 [ 59.076011] [<ffffffff810fc76e>] lock_release_non_nested+0x24e/0x350 [ 59.076011] [<ffffffff810f8eb5>] ? mark_held_locks+0x75/0xa0 [ 59.076011] [<ffffffff81dc275d>] ? __mutex_unlock_slowpath+0xad/0x170 [ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0 [ 59.076011] [<ffffffff810fcd7c>] lock_release+0xbc/0x380 [ 59.076011] [<ffffffff81dc2721>] __mutex_unlock_slowpath+0x71/0x170 [ 59.076011] [<ffffffff815ef253>] ? drm_mode_getconnector+0x83/0x400 [ 59.076011] [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0 [ 59.076011] [<ffffffff815fc21f>] drm_modeset_unlock+0x2f/0x40 [ 59.076011] [<ffffffff815ef450>] drm_mode_getconnector+0x280/0x400 [ 59.076011] [<ffffffff811cfb07>] ? might_fault+0x57/0xb0 [ 59.076011] [<ffffffff815e1f7c>] drm_ioctl+0x19c/0x640 [ 59.076011] [<ffffffff810f90ad>] ? trace_hardirqs_on+0xd/0x10 [ 59.076011] [<ffffffff8160c066>] radeon_drm_ioctl+0x46/0x80 [ 59.076011] [<ffffffff81211868>] do_vfs_ioctl+0x318/0x570 [ 59.076011] [<ffffffff81462ef6>] ? selinux_file_ioctl+0x56/0x110 [ 59.076011] [<ffffffff81211b41>] SyS_ioctl+0x81/0xa0 [ 59.076011] [<ffffffff81dc6492>] system_call_fastpath+0x12/0x17 [ 59.316190] [drm:radeon_gem_pread_ioctl] *ERROR* unimplemented radeon_gem_pread_ioctl [ 59.579952] [drm:radeon_info_ioctl] *ERROR* copy_to_user radeon_info_ioctl:555 [watchdog] kernel became tainted! (512/0) Last seed was 1541132817 Tommi
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b15d720eda4c..ce5f1193ecd6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2180,7 +2180,6 @@ 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(&dev->mode_config.connection_mutex, NULL); connector = drm_connector_find(dev, out_resp->connector_id); if (!connector) { @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->mm_height = connector->display_info.height_mm; out_resp->subpixel = connector->display_info.subpixel_order; out_resp->connection = connector->status; + + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector); if (encoder) out_resp->encoder_id = encoder->base.id;
In daniel@phenom:~/linux/src$ git show ccfc08655 commit ccfc08655d5fd5076828f45fb09194c070f2f63a Author: Rob Clark <robdclark@gmail.com> Date: Thu Dec 18 16:01:48 2014 -0500 drm: tweak getconnector locking We need to extend the locking to cover connector->state reading for atomic drivers, but the above commit was a bit too eager and also included the fill_modes callback. Which on i915 on old platforms using load detection needs to acquire modeset locks, resulting in a deadlock on output probing. Reported-by: Marc Finet <m.dreadlock@gmail.com> Cc: Marc Finet <m.dreadlock@gmail.com> Cc: robdclark@gmail.com Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_crtc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)