[PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Chris Wilson]

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
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@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;
-- 
2.6.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 7cdc548e77f503593b83a1c5d58f4dcc862c17e2 drm-intel-nightly: 2015y-12m-18d-19h-26m-21s UTC integration manifest


Results at /archive/results/CI_IGT_test/Patchwork_730/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 7cdc548e77f503593b83a1c5d58f4dcc862c17e2 drm-intel-nightly: 2015y-12m-18d-19h-26m-21s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (bdw-ultra)
                dmesg-warn -> PASS       (skl-i7k-2)
                dmesg-warn -> PASS       (ivb-t430s)
                dmesg-warn -> PASS       (snb-x220t)
                dmesg-warn -> PASS       (snb-dellxps)
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (bdw-nuci7)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup read-crc-pipe-c:
                dmesg-warn -> PASS       (skl-i7k-2)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (bdw-ultra)

bdw-nuci7        total:135  pass:125  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:125  dwarn:1   dfail:0   fail:0   skip:6  
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:99   dwarn:1   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:128  dwarn:1   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:122  dwarn:1   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:122  dwarn:1   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_730/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Daniel Vetter]

On Sat, Dec 19, 2015 at 09:58:43AM +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
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>

The original idea here was that doing this will paper over bugs in our rpm
refcounting. There's also the problem that for modeset stuff we have all
the power wells still to take care of.

For the referenced bug we should add a power domain check in the get hw
state function instead, which is what we've been doing with all the other
similar hw state readout functions too.
-Daniel

> ---
>  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;
> -- 
> 2.6.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Chris Wilson]

On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> On Sat, Dec 19, 2015 at 09:58:43AM +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
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@intel.com>
> 
> The original idea here was that doing this will paper over bugs in our rpm
> refcounting. There's also the problem that for modeset stuff we have all
> the power wells still to take care of.
> 
> For the referenced bug we should add a power domain check in the get hw
> state function instead, which is what we've been doing with all the other
> similar hw state readout functions too.

Agreed that there is another bug, but in the long term, we do want a
"prolonged" wakeref here. In the next evolution of the wakeref assertions,
we should be able to differentiate between the two (i.e. when we have
fine grained wakerefs around the hw access, we need to assert we hold one
of that type in the mmio accessor, rather than the prolonged version).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Daniel Vetter]

On Mon, Dec 21, 2015 at 04:14:53PM +0000, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> > On Sat, Dec 19, 2015 at 09:58:43AM +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
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > 
> > The original idea here was that doing this will paper over bugs in our rpm
> > refcounting. There's also the problem that for modeset stuff we have all
> > the power wells still to take care of.
> > 
> > For the referenced bug we should add a power domain check in the get hw
> > state function instead, which is what we've been doing with all the other
> > similar hw state readout functions too.
> 
> Agreed that there is another bug, but in the long term, we do want a
> "prolonged" wakeref here. In the next evolution of the wakeref assertions,
> we should be able to differentiate between the two (i.e. when we have
> fine grained wakerefs around the hw access, we need to assert we hold one
> of that type in the mmio accessor, rather than the prolonged version).

Why? If we enforce that I fear we lose implicit coverage. Currently if you
touch any piece of modeset hw and don't have the corresponding long-time
rpm/power well ref there's a good chance something will spot this. If we
have a short-term rpm reference for everything we won't noticed these
problems around the long-term rpm references any more.

Imo the only thing short-term references are useful for is lockdep
annotations to detect deadlocks, since lockdep requires that we drop a
lock in the same process again. Long-term ones would simply do a
might_lock in the get function to annotate the deadlock with rpm resume
functions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Chris Wilson]

On Mon, Dec 21, 2015 at 05:28:16PM +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2015 at 04:14:53PM +0000, Chris Wilson wrote:
> > On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> > > On Sat, Dec 19, 2015 at 09:58:43AM +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
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > 
> > > The original idea here was that doing this will paper over bugs in our rpm
> > > refcounting. There's also the problem that for modeset stuff we have all
> > > the power wells still to take care of.
> > > 
> > > For the referenced bug we should add a power domain check in the get hw
> > > state function instead, which is what we've been doing with all the other
> > > similar hw state readout functions too.
> > 
> > Agreed that there is another bug, but in the long term, we do want a
> > "prolonged" wakeref here. In the next evolution of the wakeref assertions,
> > we should be able to differentiate between the two (i.e. when we have
> > fine grained wakerefs around the hw access, we need to assert we hold one
> > of that type in the mmio accessor, rather than the prolonged version).
> 
> Why? If we enforce that I fear we lose implicit coverage. Currently if you
> touch any piece of modeset hw and don't have the corresponding long-time
> rpm/power well ref there's a good chance something will spot this. If we
> have a short-term rpm reference for everything we won't noticed these
> problems around the long-term rpm references any more.

The theory being that when we get autosuspend on the order of say a
hundred microseconds, we start to run into the real possibility of an
rpm cycle mid update.
 
> Imo the only thing short-term references are useful for is lockdep
> annotations to detect deadlocks, since lockdep requires that we drop a
> lock in the same process again. Long-term ones would simply do a
> might_lock in the get function to annotate the deadlock with rpm resume
> functions.

I'm thinking of a world where suspend-resume time are on the order of
microseconds and the rpm suspend interval not much greater.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Daniel Vetter]

On Mon, Dec 21, 2015 at 04:37:41PM +0000, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 05:28:16PM +0100, Daniel Vetter wrote:
> > On Mon, Dec 21, 2015 at 04:14:53PM +0000, Chris Wilson wrote:
> > > On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> > > > On Sat, Dec 19, 2015 at 09:58:43AM +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
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > The original idea here was that doing this will paper over bugs in our rpm
> > > > refcounting. There's also the problem that for modeset stuff we have all
> > > > the power wells still to take care of.
> > > > 
> > > > For the referenced bug we should add a power domain check in the get hw
> > > > state function instead, which is what we've been doing with all the other
> > > > similar hw state readout functions too.
> > > 
> > > Agreed that there is another bug, but in the long term, we do want a
> > > "prolonged" wakeref here. In the next evolution of the wakeref assertions,
> > > we should be able to differentiate between the two (i.e. when we have
> > > fine grained wakerefs around the hw access, we need to assert we hold one
> > > of that type in the mmio accessor, rather than the prolonged version).
> > 
> > Why? If we enforce that I fear we lose implicit coverage. Currently if you
> > touch any piece of modeset hw and don't have the corresponding long-time
> > rpm/power well ref there's a good chance something will spot this. If we
> > have a short-term rpm reference for everything we won't noticed these
> > problems around the long-term rpm references any more.
> 
> The theory being that when we get autosuspend on the order of say a
> hundred microseconds, we start to run into the real possibility of an
> rpm cycle mid update.
>  
> > Imo the only thing short-term references are useful for is lockdep
> > annotations to detect deadlocks, since lockdep requires that we drop a
> > lock in the same process again. Long-term ones would simply do a
> > might_lock in the get function to annotate the deadlock with rpm resume
> > functions.
> 
> I'm thinking of a world where suspend-resume time are on the order of
> microseconds and the rpm suspend interval not much greater.

That's why we need a bit of hystersis to avoid that. Statistics rule of
thumb is to only suspend once you've spent about as much time idle as it
would take you to suspend/resume. And since we do a few global updates we
actually acquire new power wells before old ones, so as long as you don't
do 2 ioctls to change configurations it's impossible to accidentally
suspend in between. The rest is just appropriately tuning defaults (and
still setting it to 0 in igt for maximum nastiness).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Chris Wilson]

On Mon, Dec 21, 2015 at 07:21:33PM +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2015 at 04:37:41PM +0000, Chris Wilson wrote:
> > On Mon, Dec 21, 2015 at 05:28:16PM +0100, Daniel Vetter wrote:
> > > On Mon, Dec 21, 2015 at 04:14:53PM +0000, Chris Wilson wrote:
> > > > On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> > > > > On Sat, Dec 19, 2015 at 09:58:43AM +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
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > 
> > > > > The original idea here was that doing this will paper over bugs in our rpm
> > > > > refcounting. There's also the problem that for modeset stuff we have all
> > > > > the power wells still to take care of.
> > > > > 
> > > > > For the referenced bug we should add a power domain check in the get hw
> > > > > state function instead, which is what we've been doing with all the other
> > > > > similar hw state readout functions too.
> > > > 
> > > > Agreed that there is another bug, but in the long term, we do want a
> > > > "prolonged" wakeref here. In the next evolution of the wakeref assertions,
> > > > we should be able to differentiate between the two (i.e. when we have
> > > > fine grained wakerefs around the hw access, we need to assert we hold one
> > > > of that type in the mmio accessor, rather than the prolonged version).
> > > 
> > > Why? If we enforce that I fear we lose implicit coverage. Currently if you
> > > touch any piece of modeset hw and don't have the corresponding long-time
> > > rpm/power well ref there's a good chance something will spot this. If we
> > > have a short-term rpm reference for everything we won't noticed these
> > > problems around the long-term rpm references any more.
> > 
> > The theory being that when we get autosuspend on the order of say a
> > hundred microseconds, we start to run into the real possibility of an
> > rpm cycle mid update.
> >  
> > > Imo the only thing short-term references are useful for is lockdep
> > > annotations to detect deadlocks, since lockdep requires that we drop a
> > > lock in the same process again. Long-term ones would simply do a
> > > might_lock in the get function to annotate the deadlock with rpm resume
> > > functions.
> > 
> > I'm thinking of a world where suspend-resume time are on the order of
> > microseconds and the rpm suspend interval not much greater.
> 
> That's why we need a bit of hystersis to avoid that. Statistics rule of
> thumb is to only suspend once you've spent about as much time idle as it
> would take you to suspend/resume. And since we do a few global updates we
> actually acquire new power wells before old ones, so as long as you don't
> do 2 ioctls to change configurations it's impossible to accidentally
> suspend in between. The rest is just appropriately tuning defaults (and
> still setting it to 0 in igt for maximum nastiness).

I still feel we are talking about complementary techiniques for
achieving the same thing - and that there will always be places in the
code where we want to say "do not thing about suspending until the end of
this big function".
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Imre Deak]

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 <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@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

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Joonas Lahtinen]

On to, 2016-01-14 at 18:50 +0200, Imre Deak wrote:
> 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.
> 

Below patch looks fine. Just that I'd use s/if_enabled/if_in_use/ to
match the PM API better. We're already doing too much of a good job to
having many names for same thing.

Regards, Joonas

> 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 <chris@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@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

Re: [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit

[Imre Deak]

On Thu, 2016-01-21 at 13:52 +0200, Joonas Lahtinen wrote:
> On to, 2016-01-14 at 18:50 +0200, Imre Deak wrote:
> [...]
> Below patch looks fine. Just that I'd use s/if_enabled/if_in_use/ to
> match the PM API better. We're already doing too much of a good job
> to
> having many names for same thing.

We do have two separate states that we handle separately in both
frameworks:
- enabled/active: the HW is powered-on at the moment
- in_use: the HW is powered-on _and_ we hold a reference

So imo it makes sense to make this distinction in the function names
too.

--Imre

> Regards, Joonas
> 
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx