[1/2] drm/i915: Get power refs in encoder->get_power_domain()

Submitted by Imre Deak on April 5, 2019, 3:36 p.m.

Details

Message ID 20190405153657.20921-1-imre.deak@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak April 5, 2019, 3:36 p.m.
Push getting the reference for the encoders' power domains into the
encoder get_power_domain() hook instead of doing this from the caller.
This way the encoder can store away the corresponding wakerefs.

This fixes the DSI encoder disabling, which didn't release these
power references it acquired during HW state readout.

Fixes: 0e6e0be4c9523 ("drm/i915: Markup paired operations on display power domains")
Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/icl_dsi.c       | 17 ++++++++---------
 drivers/gpu/drm/i915/intel_ddi.c     | 17 ++++++++---------
 drivers/gpu/drm/i915/intel_display.c |  6 +-----
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++----
 4 files changed, 23 insertions(+), 27 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
index b67ffaa283dc..90744032da85 100644
--- a/drivers/gpu/drm/i915/icl_dsi.c
+++ b/drivers/gpu/drm/i915/icl_dsi.c
@@ -1219,20 +1219,19 @@  static int gen11_dsi_compute_config(struct intel_encoder *encoder,
 	return 0;
 }
 
-static u64 gen11_dsi_get_power_domains(struct intel_encoder *encoder,
-				       struct intel_crtc_state *crtc_state)
+static void gen11_dsi_get_power_domains(struct intel_encoder *encoder,
+					struct intel_crtc_state *crtc_state)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	u64 domains = 0;
 	enum port port;
 
 	for_each_dsi_port(port, intel_dsi->ports)
-		if (port == PORT_A)
-			domains |= BIT_ULL(POWER_DOMAIN_PORT_DDI_A_IO);
-		else
-			domains |= BIT_ULL(POWER_DOMAIN_PORT_DDI_B_IO);
-
-	return domains;
+		intel_dsi->io_wakeref[port] =
+			intel_display_power_get(dev_priv,
+						port == PORT_A ?
+						POWER_DOMAIN_PORT_DDI_A_IO :
+						POWER_DOMAIN_PORT_DDI_B_IO);
 }
 
 static bool gen11_dsi_get_hw_state(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3f1e491bd0c0..58aa9c07c97c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2043,12 +2043,11 @@  intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port)
 					      intel_aux_power_domain(dig_port);
 }
 
-static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
-				       struct intel_crtc_state *crtc_state)
+static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
+					struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port;
-	u64 domains;
 
 	/*
 	 * TODO: Add support for MST encoders. Atm, the following should never
@@ -2056,10 +2055,10 @@  static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
 	 * hook.
 	 */
 	if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)))
-		return 0;
+		return;
 
 	dig_port = enc_to_dig_port(&encoder->base);
-	domains = BIT_ULL(dig_port->ddi_io_power_domain);
+	intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
 
 	/*
 	 * AUX power is only needed for (e)DP mode, and for HDMI mode on TC
@@ -2067,15 +2066,15 @@  static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
 	 */
 	if (intel_crtc_has_dp_encoder(crtc_state) ||
 	    intel_port_is_tc(dev_priv, encoder->port))
-		domains |= BIT_ULL(intel_ddi_main_link_aux_domain(dig_port));
+		intel_display_power_get(dev_priv,
+					intel_ddi_main_link_aux_domain(dig_port));
 
 	/*
 	 * VDSC power is needed when DSC is enabled
 	 */
 	if (crtc_state->dsc_params.compression_enable)
-		domains |= BIT_ULL(intel_dsc_power_domain(crtc_state));
-
-	return domains;
+		intel_display_power_get(dev_priv,
+					intel_dsc_power_domain(crtc_state));
 }
 
 void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7ecfb7d98839..fb10fe57a61f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16255,8 +16255,6 @@  get_encoder_power_domains(struct drm_i915_private *dev_priv)
 	struct intel_encoder *encoder;
 
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
-		u64 get_domains;
-		enum intel_display_power_domain domain;
 		struct intel_crtc_state *crtc_state;
 
 		if (!encoder->get_power_domains)
@@ -16270,9 +16268,7 @@  get_encoder_power_domains(struct drm_i915_private *dev_priv)
 			continue;
 
 		crtc_state = to_intel_crtc_state(encoder->base.crtc->state);
-		get_domains = encoder->get_power_domains(encoder, crtc_state);
-		for_each_power_domain(domain, get_domains)
-			intel_display_power_get(dev_priv, domain);
+		encoder->get_power_domains(encoder, crtc_state);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 64544613920b..3df048ec7b5e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -271,10 +271,12 @@  struct intel_encoder {
 	 * be set correctly before calling this function. */
 	void (*get_config)(struct intel_encoder *,
 			   struct intel_crtc_state *pipe_config);
-	/* Returns a mask of power domains that need to be referenced as part
-	 * of the hardware state readout code. */
-	u64 (*get_power_domains)(struct intel_encoder *encoder,
-				 struct intel_crtc_state *crtc_state);
+	/*
+	 * Acquires the power domains needed for an active encoder during
+	 * hardware state readout.
+	 */
+	void (*get_power_domains)(struct intel_encoder *encoder,
+				  struct intel_crtc_state *crtc_state);
 	/*
 	 * Called during system suspend after all pending requests for the
 	 * encoder are flushed (for example for DP AUX transactions) and

Comments

Quoting Imre Deak (2019-04-05 16:36:56)
> Push getting the reference for the encoders' power domains into the
> encoder get_power_domain() hook instead of doing this from the caller.
> This way the encoder can store away the corresponding wakerefs.
> 
> This fixes the DSI encoder disabling, which didn't release these
> power references it acquired during HW state readout.

The io_wakeref is for the paired io_enable/io_disable.

get_encoder_power_domains() is the owner of these wakerefs, and they
then belong to the atomic state from preparation through use to final
release.
-Chris
On Sat, Apr 06, 2019 at 05:05:53PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2019-04-05 16:36:56)
> > Push getting the reference for the encoders' power domains into the
> > encoder get_power_domain() hook instead of doing this from the caller.
> > This way the encoder can store away the corresponding wakerefs.
> > 
> > This fixes the DSI encoder disabling, which didn't release these
> > power references it acquired during HW state readout.
> 
> The io_wakeref is for the paired io_enable/io_disable.
> 
> get_encoder_power_domains() is the owner of these wakerefs, and they
> then belong to the atomic state from preparation through use to final
> release.

Yes, we have two cases:

1. HW readout

encoder->get_power_domains() -> io_enable() -> takes wakerefs
encoder->disable() -> io_disable() -> releases wakerefs

2. modeset

encoder->enable() -> io_enable() -> takes wakerefs
encoder->disable() -> io_disble() -> releases wakerefs

--Imre
On Sat, Apr 06, 2019 at 03:53:36PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915: Get power refs in encoder->get_power_domain()
> URL   : https://patchwork.freedesktop.org/series/59071/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_5881_full -> Patchwork_12702_full
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_12702_full absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_12702_full, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_12702_full:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@i915_pm_rpm@cursor-dpms:
>     - shard-glk:          PASS -> DMESG-WARN

"Use count on domain AUDIO is already zero"

The change doesn't affect how the audio domain refs are get/put, so the
it's an unrelated issue. It looks like the following bug:
https://bugs.freedesktop.org/show_bug.cgi?id=109513

> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_12702_full that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_eio@reset-stress:
>     - shard-snb:          PASS -> FAIL [fdo#109661]
> 
>   * igt@gem_exec_schedule@preempt-other-chain-bsd2:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +3
> 
>   * igt@gem_exec_store@cachelines-bsd1:
>     - shard-snb:          NOTRUN -> SKIP [fdo#109271] +80
> 
>   * igt@gem_exec_suspend@basic-s3:
>     - shard-apl:          PASS -> DMESG-WARN [fdo#108566]
> 
>   * igt@gem_ppgtt@blt-vs-render-ctxn:
>     - shard-iclb:         PASS -> INCOMPLETE [fdo#109801]
> 
>   * igt@i915_pm_rpm@debugfs-forcewake-user:
>     - shard-skl:          NOTRUN -> INCOMPLETE [fdo#107807]
> 
>   * igt@i915_pm_rpm@i2c:
>     - shard-iclb:         PASS -> DMESG-WARN [fdo#109982]
> 
>   * igt@kms_atomic_transition@4x-modeset-transitions-nonblocking-fencing:
>     - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +13
> 
>   * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
>     - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#110222]
> 
>   * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
>     - shard-apl:          NOTRUN -> DMESG-WARN [fdo#110222]
>     - shard-skl:          NOTRUN -> DMESG-WARN [fdo#110222] +2
> 
>   * igt@kms_busy@extended-modeset-hang-oldfb-render-f:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109278] +1
> 
>   * igt@kms_busy@extended-pageflip-hang-newfb-render-c:
>     - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +6
> 
>   * igt@kms_busy@extended-pageflip-hang-oldfb-render-f:
>     - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
> 
>   * igt@kms_ccs@pipe-c-missing-ccs-buffer:
>     - shard-apl:          NOTRUN -> SKIP [fdo#109271] +36
> 
>   * igt@kms_chamelium@dp-crc-single:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109284] +1
> 
>   * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109274]
> 
>   * igt@kms_cursor_legacy@cursor-vs-flip-legacy:
>     - shard-iclb:         PASS -> FAIL [fdo#103355]
> 
>   * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
>     - shard-glk:          PASS -> FAIL [fdo#105363]
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
>     - shard-iclb:         PASS -> FAIL [fdo#103167] +5
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-2p-shrfb-fliptrack:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +3
> 
>   * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-gtt:
>     - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +22
> 
>   * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-onoff:
>     - shard-iclb:         PASS -> FAIL [fdo#109247] +5
> 
>   * igt@kms_lease@atomic_implicit_crtc:
>     - shard-skl:          NOTRUN -> FAIL [fdo#110279]
>     - shard-apl:          NOTRUN -> FAIL [fdo#110279]
> 
>   * igt@kms_lease@setcrtc_implicit_plane:
>     - shard-skl:          NOTRUN -> FAIL [fdo#110281]
> 
>   * igt@kms_pipe_crc_basic@read-crc-pipe-e:
>     - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2
> 
>   * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
>     - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
>     - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145] +3
> 
>   * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
>     - shard-skl:          NOTRUN -> FAIL [fdo#108145]
> 
>   * igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
>     - shard-iclb:         PASS -> INCOMPLETE [fdo#110041]
> 
>   * igt@kms_psr@primary_render:
>     - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +1
> 
>   * igt@kms_psr@psr2_cursor_mmap_cpu:
>     - shard-iclb:         PASS -> SKIP [fdo#109441]
> 
>   * igt@kms_psr@psr2_sprite_plane_onoff:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109441]
> 
>   * igt@kms_setmode@basic:
>     - shard-skl:          NOTRUN -> FAIL [fdo#99912]
> 
>   * igt@kms_vblank@pipe-b-ts-continuation-suspend:
>     - shard-apl:          PASS -> FAIL [fdo#104894]
> 
>   * igt@kms_vblank@pipe-c-ts-continuation-suspend:
>     - shard-iclb:         PASS -> FAIL [fdo#104894]
> 
>   * igt@perf_pmu@semaphore-wait-vcs1:
>     - shard-skl:          NOTRUN -> SKIP [fdo#109271] +124
> 
>   
> #### Possible fixes ####
> 
>   * igt@gem_ppgtt@blt-vs-render-ctx0:
>     - shard-iclb:         INCOMPLETE [fdo#109801] -> PASS
> 
>   * igt@i915_pm_rpm@reg-read-ioctl:
>     - shard-skl:          INCOMPLETE [fdo#107807] -> PASS
> 
>   * igt@i915_pm_rpm@universal-planes-dpms:
>     - shard-iclb:         INCOMPLETE [fdo#107713] / [fdo#108840] -> PASS
> 
>   * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
>     - shard-glk:          FAIL [fdo#106509] / [fdo#107409] -> PASS
> 
>   * igt@kms_fbcon_fbt@fbc:
>     - shard-iclb:         DMESG-WARN [fdo#109593] -> PASS
> 
>   * igt@kms_flip@flip-vs-suspend:
>     - shard-kbl:          DMESG-WARN [fdo#108566] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbc-suspend:
>     - shard-iclb:         FAIL [fdo#103375] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
>     - shard-iclb:         FAIL [fdo#103167] -> PASS +4
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite:
>     - shard-iclb:         FAIL [fdo#109247] -> PASS +15
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-pwrite:
>     - shard-iclb:         FAIL [fdo#105682] / [fdo#109247] -> PASS
> 
>   * igt@kms_plane@pixel-format-pipe-c-planes:
>     - shard-glk:          SKIP [fdo#109271] -> PASS +1
> 
>   * igt@kms_plane_scaling@pipe-a-scaler-with-rotation:
>     - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> PASS +2
> 
>   * igt@kms_psr@psr2_sprite_mmap_cpu:
>     - shard-iclb:         SKIP [fdo#109441] -> PASS
> 
>   * igt@kms_psr@sprite_plane_move:
>     - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS
> 
>   * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
>     - shard-kbl:          FAIL [fdo#109016] -> PASS
> 
>   * igt@kms_setmode@basic:
>     - shard-apl:          FAIL [fdo#99912] -> PASS
>     - shard-kbl:          FAIL [fdo#99912] -> PASS
> 
>   * igt@kms_vblank@pipe-a-ts-continuation-modeset-rpm:
>     - shard-apl:          FAIL [fdo#104894] -> PASS +1
> 
>   * igt@kms_vblank@pipe-a-ts-continuation-suspend:
>     - shard-iclb:         FAIL [fdo#104894] -> PASS
> 
>   * igt@perf@oa-exponents:
>     - shard-glk:          FAIL [fdo#105483] -> PASS
> 
>   
> #### Warnings ####
> 
>   * igt@i915_pm_rpm@dpms-non-lpsp:
>     - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]
> 
>   * igt@i915_selftest@live_contexts:
>     - shard-iclb:         DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569]
> 
>   * igt@runner@aborted:
>     - shard-glk:          FAIL [fdo#109373] / [k.org#202321] -> ( 2 FAIL ) [fdo#109373] / [k.org#202321]
> 
>   
>   [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
>   [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
>   [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
>   [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
>   [fdo#105483]: https://bugs.freedesktop.org/show_bug.cgi?id=105483
>   [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
>   [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
>   [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
>   [fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409
>   [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
>   [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
>   [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
>   [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
>   [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
>   [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
>   [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
>   [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
>   [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
>   [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
>   [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
>   [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
>   [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
>   [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
>   [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
>   [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
>   [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
>   [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
>   [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
>   [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
>   [fdo#109801]: https://bugs.freedesktop.org/show_bug.cgi?id=109801
>   [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
>   [fdo#110041]: https://bugs.freedesktop.org/show_bug.cgi?id=110041
>   [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
>   [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
>   [fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
>   [fdo#110281]: https://bugs.freedesktop.org/show_bug.cgi?id=110281
>   [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
>   [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
> 
> 
> Participating hosts (10 -> 9)
> ------------------------------
> 
>   Missing    (1): shard-hsw 
> 
> 
> Build changes
> -------------
> 
>     * Linux: CI_DRM_5881 -> Patchwork_12702
> 
>   CI_DRM_5881: b070175c76da1440a747fd023ee6253e573055f8 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4931: 019f892e5d1a0a9643cb726c47ce2d99c14b444f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_12702: d6ea568a1475dbb348d88ebb268c397c5967a284 @ git://anongit.freedesktop.org/gfx-ci/linux
>   piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12702/
Quoting Imre Deak (2019-04-07 11:25:37)
> On Sat, Apr 06, 2019 at 05:05:53PM +0100, Chris Wilson wrote:
> > Quoting Imre Deak (2019-04-05 16:36:56)
> > > Push getting the reference for the encoders' power domains into the
> > > encoder get_power_domain() hook instead of doing this from the caller.
> > > This way the encoder can store away the corresponding wakerefs.
> > > 
> > > This fixes the DSI encoder disabling, which didn't release these
> > > power references it acquired during HW state readout.
> > 
> > The io_wakeref is for the paired io_enable/io_disable.
> > 
> > get_encoder_power_domains() is the owner of these wakerefs, and they
> > then belong to the atomic state from preparation through use to final
> > release.
> 
> Yes, we have two cases:
> 
> 1. HW readout
> 
> encoder->get_power_domains() -> io_enable() -> takes wakerefs
> encoder->disable() -> io_disable() -> releases wakerefs

However, get_power_domains is used for the atomic state wakerefs (as
well?) and they have different much longer lifetimes.

Clear io_wakerefs, and put a WARN_ON(io_wakeref[]) prior to filling it
to convince me that it is being used to track a temporary wakeref.

get_power_domains() and atomic needs to take responsibility for the
overlapping set of powerwells imo.
-Chris
On Sun, Apr 07, 2019 at 12:41:08PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2019-04-07 11:25:37)
> > On Sat, Apr 06, 2019 at 05:05:53PM +0100, Chris Wilson wrote:
> > > Quoting Imre Deak (2019-04-05 16:36:56)
> > > > Push getting the reference for the encoders' power domains into the
> > > > encoder get_power_domain() hook instead of doing this from the caller.
> > > > This way the encoder can store away the corresponding wakerefs.
> > > > 
> > > > This fixes the DSI encoder disabling, which didn't release these
> > > > power references it acquired during HW state readout.
> > > 
> > > The io_wakeref is for the paired io_enable/io_disable.
> > > 
> > > get_encoder_power_domains() is the owner of these wakerefs, and they
> > > then belong to the atomic state from preparation through use to final
> > > release.
> > 
> > Yes, we have two cases:
> > 
> > 1. HW readout
> > 
> > encoder->get_power_domains() -> io_enable() -> takes wakerefs
> > encoder->disable() -> io_disable() -> releases wakerefs
> 
> However, get_power_domains is used for the atomic state wakerefs (as
> well?) and they have different much longer lifetimes.

It's only used during HW readout, if the encoder is active. Then we need
to take the wakerefs the same way encoder->enable() would take them,
since ->enable() won't be called. Then the refs taken by
->get_power_domains() will be held for long, until ->disable() is
called, so they have the same lifetime as in the ->enable()/->disable()
case.

> Clear io_wakerefs, and put a WARN_ON(io_wakeref[]) prior to filling it
> to convince me that it is being used to track a temporary wakeref.

Ok, I thought about it too just after you wrote. Yes, at that point
there should be no wakerefs stored there, which we should check for.
Will send v2.

> get_power_domains() and atomic needs to take responsibility for the
> overlapping set of powerwells imo.

->get_power_domains() is essentially the powerref acquisition part of the
encoder->get_hw_state(). We have a separate hook for it since the latter
one is also called during verification, where we don't want to take the
long time refs (they must have been acquired already by the modeset we
are verifying).

> -Chris
On Sun, Apr 07, 2019 at 02:55:14PM +0300, Imre Deak wrote:
> On Sun, Apr 07, 2019 at 12:41:08PM +0100, Chris Wilson wrote:
> > Quoting Imre Deak (2019-04-07 11:25:37)
> > > On Sat, Apr 06, 2019 at 05:05:53PM +0100, Chris Wilson wrote:
> > > > Quoting Imre Deak (2019-04-05 16:36:56)
> > > > > Push getting the reference for the encoders' power domains into the
> > > > > encoder get_power_domain() hook instead of doing this from the caller.
> > > > > This way the encoder can store away the corresponding wakerefs.
> > > > > 
> > > > > This fixes the DSI encoder disabling, which didn't release these
> > > > > power references it acquired during HW state readout.
> > > > 
> > > > The io_wakeref is for the paired io_enable/io_disable.
> > > > 
> > > > get_encoder_power_domains() is the owner of these wakerefs, and they
> > > > then belong to the atomic state from preparation through use to final
> > > > release.
> > > 
> > > Yes, we have two cases:
> > > 
> > > 1. HW readout
> > > 
> > > encoder->get_power_domains() -> io_enable() -> takes wakerefs
> > > encoder->disable() -> io_disable() -> releases wakerefs
> > 
> > However, get_power_domains is used for the atomic state wakerefs (as
> > well?) and they have different much longer lifetimes.
> 
> It's only used during HW readout, if the encoder is active. Then we need
> to take the wakerefs the same way encoder->enable() would take them,
> since ->enable() won't be called. Then the refs taken by
> ->get_power_domains() will be held for long, until ->disable() is
> called, so they have the same lifetime as in the ->enable()/->disable()
> case.
> 
> > Clear io_wakerefs, and put a WARN_ON(io_wakeref[]) prior to filling it
> > to convince me that it is being used to track a temporary wakeref.
> 
> Ok, I thought about it too just after you wrote. Yes, at that point
> there should be no wakerefs stored there, which we should check for.
> Will send v2.
> 
> > get_power_domains() and atomic needs to take responsibility for the
> > overlapping set of powerwells imo.
> 
> ->get_power_domains() is essentially the powerref acquisition part of the
> encoder->get_hw_state(). We have a separate hook for it since the latter
> one is also called during verification, where we don't want to take the
> long time refs (they must have been acquired already by the modeset we
> are verifying).

Note the similar logic for crtc powerrefs in
intel_modeset_setup_hw_state()/modeset_get_crtc_power_domains().

> 
> > -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Imre Deak (2019-04-07 13:06:32)
> On Sun, Apr 07, 2019 at 02:55:14PM +0300, Imre Deak wrote:
> > > get_power_domains() and atomic needs to take responsibility for the
> > > overlapping set of powerwells imo.
> > 
> > ->get_power_domains() is essentially the powerref acquisition part of the
> > encoder->get_hw_state(). We have a separate hook for it since the latter
> > one is also called during verification, where we don't want to take the
> > long time refs (they must have been acquired already by the modeset we
> > are verifying).
> 
> Note the similar logic for crtc powerrefs in
> intel_modeset_setup_hw_state()/modeset_get_crtc_power_domains().

Hmm. That's what I thought this was. I probably was confusing the paths.

Also while you are here, care to look over
https://patchwork.freedesktop.org/patch/296632/?series=59089&rev=2
-Chris
On Sun, Apr 07, 2019 at 02:40:18PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v2,1/2] drm/i915: Get power refs in encoder->get_power_domains() (rev2)
> URL   : https://patchwork.freedesktop.org/series/59071/
> State : failure

Thanks for the review, pushed to -dinq. Below the explanation for the
failure.

> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_5883_full -> Patchwork_12719_full
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_12719_full absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_12719_full, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_12719_full:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@gem_exec_parallel@vebox-fds:
>     - shard-iclb:         NOTRUN -> SKIP +1

Pre-existing issue (on a machine without DSI), looks like
https://bugs.freedesktop.org/show_bug.cgi?id=110351

> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_12719_full that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_pread@pagefault-pread:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109277]
> 
>   * igt@gem_tiled_fence_blits@normal:
>     - shard-iclb:         PASS -> TIMEOUT [fdo#109673]
> 
>   * igt@gem_tiled_swapping@non-threaded:
>     - shard-iclb:         PASS -> FAIL [fdo#108686]
>     - shard-snb:          PASS -> INCOMPLETE [fdo#105411]
> 
>   * igt@i915_hangman@error-state-capture-bsd2:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +3
> 
>   * igt@i915_pm_rpm@debugfs-forcewake-user:
>     - shard-skl:          PASS -> INCOMPLETE [fdo#107807]
> 
>   * igt@i915_pm_rpm@modeset-non-lpsp:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109308]
> 
>   * igt@i915_suspend@debugfs-reader:
>     - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]
> 
>   * igt@kms_atomic_transition@3x-modeset-transitions-nonblocking:
>     - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3
> 
>   * igt@kms_atomic_transition@5x-modeset-transitions-nonblocking:
>     - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +13
> 
>   * igt@kms_atomic_transition@plane-all-modeset-transition-fencing:
>     - shard-apl:          PASS -> INCOMPLETE [fdo#103927]
> 
>   * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
>     - shard-skl:          NOTRUN -> DMESG-WARN [fdo#110222] +1
> 
>   * igt@kms_busy@extended-pageflip-hang-oldfb-render-d:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109278] +6
> 
>   * igt@kms_chamelium@vga-hpd-after-suspend:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109284] +1
> 
>   * igt@kms_cursor_crc@cursor-256x256-suspend:
>     - shard-skl:          NOTRUN -> INCOMPLETE [fdo#104108]
> 
>   * igt@kms_cursor_legacy@cursorb-vs-flipb-atomic-transitions-varying-size:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +3
> 
>   * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-ytiled:
>     - shard-snb:          NOTRUN -> SKIP [fdo#109271] +37
> 
>   * igt@kms_fbcon_fbt@fbc:
>     - shard-iclb:         PASS -> DMESG-WARN [fdo#109593]
> 
>   * igt@kms_flip@plain-flip-fb-recreate:
>     - shard-skl:          NOTRUN -> FAIL [fdo#100368]
> 
>   * igt@kms_force_connector_basic@prune-stale-modes:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109285]
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
>     - shard-iclb:         PASS -> FAIL [fdo#103167] +3
> 
>   * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-blt:
>     - shard-skl:          NOTRUN -> SKIP [fdo#109271] +140
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-blt:
>     - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +19
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-2p-shrfb-fliptrack:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +11
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-indfb-scaledprimary:
>     - shard-iclb:         NOTRUN -> FAIL [fdo#109247]
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-pwrite:
>     - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#109247] +1
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-shrfb-scaledprimary:
>     - shard-apl:          NOTRUN -> SKIP [fdo#109271] +12
> 
>   * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-pwrite:
>     - shard-iclb:         PASS -> FAIL [fdo#109247] +34
> 
>   * igt@kms_plane@pixel-format-pipe-b-planes:
>     - shard-glk:          PASS -> SKIP [fdo#109271]
> 
>   * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
>     - shard-skl:          NOTRUN -> FAIL [fdo#108145] +1
> 
>   * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
>     - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
> 
>   * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
>     - shard-skl:          PASS -> FAIL [fdo#107815]
> 
>   * igt@kms_psr@psr2_cursor_render:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109441]
> 
>   * igt@kms_psr@psr2_primary_page_flip:
>     - shard-iclb:         PASS -> SKIP [fdo#109441] +1
> 
>   * igt@kms_psr@sprite_mmap_cpu:
>     - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +6
> 
>   * igt@kms_setmode@basic:
>     - shard-skl:          NOTRUN -> FAIL [fdo#99912]
> 
>   * igt@kms_sysfs_edid_timing:
>     - shard-skl:          NOTRUN -> FAIL [fdo#100047]
> 
>   * igt@kms_universal_plane@disable-primary-vs-flip-pipe-d:
>     - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
> 
>   * igt@kms_vblank@pipe-a-ts-continuation-dpms-rpm:
>     - shard-apl:          PASS -> FAIL [fdo#104894]
> 
>   * igt@prime_nv_api@nv_i915_import_twice_check_flink_name:
>     - shard-iclb:         NOTRUN -> SKIP [fdo#109291] +1
> 
>   * igt@runner@aborted:
>     - shard-iclb:         NOTRUN -> FAIL [fdo#109593]
> 
>   
> #### Possible fixes ####
> 
>   * igt@gem_ppgtt@blt-vs-render-ctx0:
>     - shard-iclb:         INCOMPLETE [fdo#109801] -> PASS
> 
>   * igt@i915_pm_rpm@gem-execbuf:
>     - shard-skl:          INCOMPLETE [fdo#107803] / [fdo#107807] -> PASS
> 
>   * igt@i915_pm_rpm@i2c:
>     - shard-iclb:         DMESG-WARN [fdo#109982] -> PASS
> 
>   * igt@i915_suspend@sysfs-reader:
>     - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS
> 
>   * igt@kms_frontbuffer_tracking@fbc-stridechange:
>     - shard-iclb:         FAIL [fdo#103167] -> PASS +4
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt:
>     - shard-iclb:         FAIL [fdo#109247] -> PASS +16
> 
>   * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
>     - shard-skl:          INCOMPLETE [fdo#104108] / [fdo#107773] -> PASS
> 
>   * igt@kms_psr@sprite_render:
>     - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +4
> 
>   * igt@kms_setmode@basic:
>     - shard-kbl:          FAIL [fdo#99912] -> PASS
> 
>   
> #### Warnings ####
> 
>   * igt@kms_psr@psr2_cursor_plane_onoff:
>     - shard-iclb:         SKIP [fdo#109441] -> FAIL [fdo#107383] / [fdo#110215] +3
> 
>   
>   [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>   [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
>   [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
>   [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
>   [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
>   [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
>   [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
>   [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
>   [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
>   [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
>   [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
>   [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
>   [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
>   [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
>   [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
>   [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
>   [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
>   [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
>   [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
>   [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
>   [fdo#109277]: https://bugs.freedesktop.org/show_bug.cgi?id=109277
>   [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
>   [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
>   [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
>   [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
>   [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
>   [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
>   [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
>   [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
>   [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
>   [fdo#109801]: https://bugs.freedesktop.org/show_bug.cgi?id=109801
>   [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
>   [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
>   [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
>   [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
> 
> 
> Participating hosts (10 -> 9)
> ------------------------------
> 
>   Missing    (1): shard-hsw 
> 
> 
> Build changes
> -------------
> 
>     * Linux: CI_DRM_5883 -> Patchwork_12719
> 
>   CI_DRM_5883: 95420638cafc51663c74ccfe1340c309fdcdd8ab @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4932: 08cf63a8fac11e3594b57580331fb319241a0d69 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_12719: 693e07a98a8c02cf3d4ff7faaa8a485a50cd7ba4 @ git://anongit.freedesktop.org/gfx-ci/linux
>   piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12719/