On la, 2015-12-19 at 09:58 +0000, Chris Wilson wrote:
> Once all the preparations are complete, we are ready to write the
> modesetting to the hardware. During this phase, we will be making
> lots
> of HW register access, so take a top level wakeref to prevent an
> unwarranted rpm suspend cycle mid-commit. Lower level functions
> should
> be waking the individual power wells as required.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93439
I would separate here two things:
The device level power flip-flopping you mention and the fix for the
above bug. For the flip-flopping we could use what you suggest, perhaps
by also avoiding waking up the device if nothing will change and
everything will stay disabled.
As for the fix I would go with what Ville suggested. By ensuring we
keep an RPM reference we still allow for a display power domain
reference to come and go in the middle of the HW readout. I went ahead
and tried the following which got rid of the problem too, if people are
ok with it I could convert the rest of the HW readout places
accordingly and send out the patch. We can also
get pm_runtime_get_if_in_use() into use once it's added, but it's
not crucial for the fix.
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1f9a368..907377dc 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1910,13 +1910,16 @@ bool intel_ddi_connector_get_hw_state(struct
intel_connector *intel_connector)
enum transcoder cpu_transcoder;
enum intel_display_power_domain power_domain;
uint32_t tmp;
+ bool ret;
power_domain = intel_display_port_power_domain(intel_encoder);
- if (!intel_display_power_is_enabled(dev_priv, power_domain))
+ if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
return false;
- if (!intel_encoder->get_hw_state(intel_encoder, &pipe))
- return false;
+ if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
+ ret = false;
+ goto out;
+ }
if (port == PORT_A)
cpu_transcoder = TRANSCODER_EDP;
@@ -1928,23 +1931,30 @@ bool intel_ddi_connector_get_hw_state(struct
intel_connector *intel_connector)
switch (tmp & TRANS_DDI_MODE_SELECT_MASK) {
case TRANS_DDI_MODE_SELECT_HDMI:
case TRANS_DDI_MODE_SELECT_DVI:
- return (type == DRM_MODE_CONNECTOR_HDMIA);
+ ret = type == DRM_MODE_CONNECTOR_HDMIA;
+ goto out;
case TRANS_DDI_MODE_SELECT_DP_SST:
- if (type == DRM_MODE_CONNECTOR_eDP)
- return true;
- return (type == DRM_MODE_CONNECTOR_DisplayPort);
+ ret = type == DRM_MODE_CONNECTOR_eDP ||
+ type == DRM_MODE_CONNECTOR_DisplayPort;
+ goto out;
case TRANS_DDI_MODE_SELECT_DP_MST:
/* if the transcoder is in MST state then
* connector isn't connected */
- return false;
+ ret = false;
+ goto out;
case TRANS_DDI_MODE_SELECT_FDI:
- return (type == DRM_MODE_CONNECTOR_VGA);
+ ret = type == DRM_MODE_CONNECTOR_VGA;
+ goto out;
default:
- return false;
+ ret = false;
}
+out:
+ intel_display_power_put(dev_priv, power_domain);
+
+ return ret;
}
bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 059b46e..3c84159 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1456,6 +1456,8 @@ bool __intel_display_power_is_enabled(struct
drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
void intel_display_power_get(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+ enum intel_display_power_domain domain);
void intel_display_power_put(struct drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bbca527..6c4f170 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1470,6 +1470,17 @@ void intel_display_power_get(struct drm_i915_private
*dev_priv,
mutex_unlock(&power_domains->lock);
}
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+ enum intel_display_power_domain domain)
+{
+ if (!intel_display_power_is_enabled(dev_priv, domain))
+ return false;
+
+ intel_display_power_get(dev_priv, domain);
+
+ return true;
+}
+
/**
* intel_display_power_put - release a power domain reference
* @dev_priv: i915 device instance
--Imre
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Imre Deak <imre.d...@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index abd2d2944022..60451c3932db 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13470,6 +13470,13 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> drm_atomic_helper_swap_state(dev, state);
> dev_priv->wm.config = to_intel_atomic_state(state)-
> >wm_config;
>
> + /* Take a rpm wakeref for the duration of the commit. Lower
> level
> + * functions should be acquiring the power wells for their
> own use,
> + * we take this toplevel reference to prevent rpm suspend
> cycles
> + * mid-commit.
> + */
> + intel_runtime_pm_get(dev_priv);
> +
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> @@ -13558,6 +13565,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> if (any_ms)
> intel_modeset_check_state(dev, state);
>
> + intel_runtime_pm_put(dev_priv);
> +
> drm_atomic_state_free(state);
>
> return 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
