| Message ID | 1397080053-22171-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > If I unplug the eDP monitor, the BIOS of my machine will enable the > VDD bit, then when the driver loads it will think VDD is enabled. It > will detect that the eDP is not enabled and return false from > intel_edp_init_connector. This will trigger a call to > edp_panel_vdd_off_sync(), which trigger a WARN saying that the > refcount of the power domain is less than zero. > > The problem happens because the driver gets a refcount whenever it > enables the VDD bit, and puts the refcount whenever it disables the > VDD bit. But on this case, the BIOS enabled VDD, so all we do is to > call put() without calling get() first, so the code added is there to > make sure we always have the get() in case the BIOS enabled the bit. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Is this the bug reported by Chris on irc yesterday? Chris? -Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e48d47c..a432904 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > { > struct drm_connector *connector = &intel_connector->base; > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > bool has_dpcd; > @@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > if (!is_edp(intel_dp)) > return true; > > + /* The VDD bit needs a power domain reference, so if the bit is already > + * enabled when we boot, grab this reference. */ > + if (edp_have_panel_vdd(intel_dp)) { > + enum intel_display_power_domain power_domain; > + power_domain = intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > + } > + > /* Cache DPCD and EDID for edp. */ > intel_edp_panel_vdd_on(intel_dp); > has_dpcd = intel_dp_get_dpcd(intel_dp); > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 10, 2014 at 09:21:50AM +0200, Daniel Vetter wrote: > On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > If I unplug the eDP monitor, the BIOS of my machine will enable the > > VDD bit, then when the driver loads it will think VDD is enabled. It > > will detect that the eDP is not enabled and return false from > > intel_edp_init_connector. This will trigger a call to > > edp_panel_vdd_off_sync(), which trigger a WARN saying that the > > refcount of the power domain is less than zero. > > > > The problem happens because the driver gets a refcount whenever it > > enables the VDD bit, and puts the refcount whenever it disables the > > VDD bit. But on this case, the BIOS enabled VDD, so all we do is to > > call put() without calling get() first, so the code added is there to > > make sure we always have the get() in case the BIOS enabled the bit. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Is this the bug reported by Chris on irc yesterday? Chris? Looks likely... Tested-by: Chris Wilson <chris@chris-wilson.co.uk> Bikeshed requests: That chunk is repeated a few times, please refactor it into a small function. The caller of intel_edp_init_connector() has a redundant if(is_edp) in its error handling path. -Chris
On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > If I unplug the eDP monitor, the BIOS of my machine will enable the > VDD bit, then when the driver loads it will think VDD is enabled. It > will detect that the eDP is not enabled and return false from > intel_edp_init_connector. This will trigger a call to > edp_panel_vdd_off_sync(), which trigger a WARN saying that the > refcount of the power domain is less than zero. > > The problem happens because the driver gets a refcount whenever it > enables the VDD bit, and puts the refcount whenever it disables the > VDD bit. But on this case, the BIOS enabled VDD, so all we do is to > call put() without calling get() first, so the code added is there to > make sure we always have the get() in case the BIOS enabled the bit. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Probably also Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987 Paulo can you please polish the patch as requested by Chris and resubmit? Please also repoke the bug. Thanks, Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e48d47c..a432904 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > { > struct drm_connector *connector = &intel_connector->base; > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > bool has_dpcd; > @@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > if (!is_edp(intel_dp)) > return true; > > + /* The VDD bit needs a power domain reference, so if the bit is already > + * enabled when we boot, grab this reference. */ > + if (edp_have_panel_vdd(intel_dp)) { > + enum intel_display_power_domain power_domain; > + power_domain = intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > + } > + > /* Cache DPCD and EDID for edp. */ > intel_edp_panel_vdd_on(intel_dp); > has_dpcd = intel_dp_get_dpcd(intel_dp); > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e48d47c..a432904 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, { struct drm_connector *connector = &intel_connector->base; struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - struct drm_device *dev = intel_dig_port->base.base.dev; + struct intel_encoder *intel_encoder = &intel_dig_port->base; + struct drm_device *dev = intel_encoder->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_display_mode *fixed_mode = NULL; bool has_dpcd; @@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!is_edp(intel_dp)) return true; + /* The VDD bit needs a power domain reference, so if the bit is already + * enabled when we boot, grab this reference. */ + if (edp_have_panel_vdd(intel_dp)) { + enum intel_display_power_domain power_domain; + power_domain = intel_display_port_power_domain(intel_encoder); + intel_display_power_get(dev_priv, power_domain); + } + /* Cache DPCD and EDID for edp. */ intel_edp_panel_vdd_on(intel_dp); has_dpcd = intel_dp_get_dpcd(intel_dp);