| Message ID | 1364230570-27468-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 25, 2013 at 5:56 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Thanks to apple gpu mux fail we detect an eDP output, but can't read > anything over dp aux. In the resulting failure path we then hit a > paranoid WARN about potential locking. > > Since the WARN is pretty useful for normal operation just paper over > it in the failure case by grabbing the demanded (but for init/teardown > not really required) lock. > > I've checked our driver unload code and we already don't hold the kms > lock when calling drm_mode_config_cleanup. So this won't lead to a new > deadlock when reloading i915.ko. > > v2: Make it compile. > > Reported-by: Dave Airlie <airlied@gmail.com> > Cc: Dave Airlie <airlied@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Now also quickly tested on my edp to check whether I really didn't break module reload. Seems to still work. Dave, since I don't have anything else pending for fixes, can you please merge this for drm-fixes directly if it works for you? Thanks, Daniel
On Mon, 25 Mar 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Thanks to apple gpu mux fail we detect an eDP output, but can't read > anything over dp aux. In the resulting failure path we then hit a > paranoid WARN about potential locking. > > Since the WARN is pretty useful for normal operation just paper over > it in the failure case by grabbing the demanded (but for init/teardown > not really required) lock. > > I've checked our driver unload code and we already don't hold the kms > lock when calling drm_mode_config_cleanup. So this won't lead to a new > deadlock when reloading i915.ko. Also, drm_encoder_cleanup() grabs mode_config mutex internally, so we would have deadlocks already if intel_dp_encoder_destroy() were called while holding the lock. Reviewed-by: Jani Nikula <jani.nikula@intel.com> An observation outside of this patch, I find it a bit ugly that it depends on the ironlake_edp_panel_vdd_off() sync parameter whether the caller needs to hold the mode_config mutex or not. Perhaps separate functions for sync/async would be neater, but don't hold your breath waiting for patches from me to that effect. ;) > > v2: Make it compile. > > Reported-by: Dave Airlie <airlied@gmail.com> > Cc: Dave Airlie <airlied@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index d7d4afe..8fc93f9 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2559,12 +2559,15 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > { > struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); > struct intel_dp *intel_dp = &intel_dig_port->dp; > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > > i2c_del_adapter(&intel_dp->adapter); > drm_encoder_cleanup(encoder); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > + mutex_lock(&dev->mode_config.mutex); > ironlake_panel_vdd_off_sync(intel_dp); > + mutex_unlock(&dev->mode_config.mutex); > } > kfree(intel_dig_port); > } > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 26, 2013 at 09:45:47AM +0200, Jani Nikula wrote: > On Mon, 25 Mar 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Thanks to apple gpu mux fail we detect an eDP output, but can't read > > anything over dp aux. In the resulting failure path we then hit a > > paranoid WARN about potential locking. > > > > Since the WARN is pretty useful for normal operation just paper over > > it in the failure case by grabbing the demanded (but for init/teardown > > not really required) lock. > > > > I've checked our driver unload code and we already don't hold the kms > > lock when calling drm_mode_config_cleanup. So this won't lead to a new > > deadlock when reloading i915.ko. > > Also, drm_encoder_cleanup() grabs mode_config mutex internally, so we > would have deadlocks already if intel_dp_encoder_destroy() were called > while holding the lock. > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> Thanks for the review, smashed on top of -fixes. > An observation outside of this patch, I find it a bit ugly that it > depends on the ironlake_edp_panel_vdd_off() sync parameter whether the > caller needs to hold the mode_config mutex or not. Perhaps separate > functions for sync/async would be neater, but don't hold your breath > waiting for patches from me to that effect. ;) Yeah, it's no the greatest interface, ever ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d7d4afe..8fc93f9 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2559,12 +2559,15 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) { struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); struct intel_dp *intel_dp = &intel_dig_port->dp; + struct drm_device *dev = intel_dp_to_dev(intel_dp); i2c_del_adapter(&intel_dp->adapter); drm_encoder_cleanup(encoder); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work); + mutex_lock(&dev->mode_config.mutex); ironlake_panel_vdd_off_sync(intel_dp); + mutex_unlock(&dev->mode_config.mutex); } kfree(intel_dig_port); }
Thanks to apple gpu mux fail we detect an eDP output, but can't read anything over dp aux. In the resulting failure path we then hit a paranoid WARN about potential locking. Since the WARN is pretty useful for normal operation just paper over it in the failure case by grabbing the demanded (but for init/teardown not really required) lock. I've checked our driver unload code and we already don't hold the kms lock when calling drm_mode_config_cleanup. So this won't lead to a new deadlock when reloading i915.ko. v2: Make it compile. Reported-by: Dave Airlie <airlied@gmail.com> Cc: Dave Airlie <airlied@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_dp.c | 3 +++ 1 file changed, 3 insertions(+)