[Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[José Roberto de Souza]

Most of the kms_frontbuffer_tracking tests disables the feature being
tested, draw, get the CRC then enable the feature, draw again, get the
CRC and check if it matches.
Some times it is able to do that with a fastset, so
intel_pre_plane_update() is executed but intel_fbc_can_flip_nuke() was
not checking if FBC is now enabled in this CRTC leaving FBC active and
causing the warning bellow in __intel_fbc_disable()

[IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-indfb-multidraw
Setting dangerous option enable_fbc - tainting kernel
i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR debug to f
i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid debug mask f
i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR debug to 1
i915 0000:00:02.0: [drm:intel_atomic_check [i915]] [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of EDID bpp 24, requested bpp 36, max platform bpp 36
[drm:intel_dp_compute_config [i915]] DP link computation with max lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
[drm:intel_dp_compute_config [i915]] Force DSC en = 0
[drm:intel_dp_compute_config [i915]] DP lane count 2 clock 270000 bpp 24
[drm:intel_dp_compute_config [i915]] DP link rate required 414360 available 540000
i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp: 24, pipe bpp: 24, dithering: 0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] [CRTC:91:pipe A] enable: yes [fastset]
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active: yes, output_types: EDP (0x100), output format: RGB
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n: lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202, link_n: 524288, tu: 64
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0, infoframes enabled: 0x0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] requested mode:
[drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted mode:
[drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
[drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920 1968 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port clock: 270000, pipe src size: 1920x1080, pixel rate 138120
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] linetime: 119, ips linetime: 0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] num_scalers: 2, scaler_users: 0x0, scaler_id: -1
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x00000000, size: 0x00000000, disabled, force thru: no
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0
[drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0: 0x1c001a5, cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1: 0x0, mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0, mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0, mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] csc_mode: 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST master transcoder: <invalid>
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24 little-endian (0x34325258), visible: yes
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotation: 0x1, scaler: -1
i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src: 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]] Disabling PSR1
i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel doesn't support DRRS
------------[ cut here ]------------
i915 0000:00:02.0: drm_WARN_ON(fbc->active)
WARNING: CPU: 4 PID: 1175 at drivers/gpu/drm/i915/display/intel_fbc.c:973 __intel_fbc_disable+0xa5/0x130 [i915]
Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal coretemp crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core cdc_ether e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt intel_lpss_pci prime_numbers
CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted: G     U            5.5.0-CI-Trybot_5651+ #1
Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750 06/14/2019
RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7 c1 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce e0 <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33 30 e1
RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff8884799d0000 RCX: 0000000000000006
RDX: 0000000000001905 RSI: ffff888495dac970 RDI: ffffffff823731a1
RBP: ffff88847c05d000 R08: ffff888495dac970 R09: 0000000000000000
R10: ffffc90000613b88 R11: 0000000000000000 R12: ffff88849bba7e40
R13: ffff8884799d0000 R14: ffff888498564000 R15: 0000000000000000
FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4: 0000000000760ee0
PKRU: 55555554
Call Trace:
 intel_fbc_disable+0x4a/0x50 [i915]
 intel_update_crtc+0x12c/0x1d0 [i915]
 skl_commit_modeset_enables+0x14d/0x600 [i915]
 intel_atomic_commit_tail+0x30d/0x1480 [i915]
 ? queue_work_on+0x31/0x70
 ? intel_atomic_commit_ready+0x3f/0x48 [i915]
 ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
 intel_atomic_commit+0x312/0x390 [i915]
 intel_psr_fastset_force+0x119/0x150 [i915]
 i915_edp_psr_debug_set+0x53/0x70 [i915]
 simple_attr_write+0xb0/0xd0
 full_proxy_write+0x51/0x80
 vfs_write+0xb9/0x1d0
 ksys_write+0x9f/0xe0
 do_syscall_64+0x4f/0x220
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f8157240281
Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f 84 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89 d4 53
RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f8157240281
RDX: 0000000000000003 RSI: 00007f8157901152 RDI: 0000000000000008
RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8157901152
R13: 0000000000000008 R14: 00005589d298dce0 R15: 0000000000000000
irq event stamp: 55208
hardirqs last  enabled at (55207): [<ffffffff8112f3fc>] vprintk_emit+0xcc/0x330
hardirqs last disabled at (55208): [<ffffffff81001ca0>] trace_hardirqs_off_thunk+0x1a/0x1c
softirqs last  enabled at (54926): [<ffffffff81e00385>] __do_softirq+0x385/0x47f
softirqs last disabled at (54915): [<ffffffff810ba15a>] irq_exit+0xba/0xc0
---[ end trace afa50c52e5a512bb ]---
[drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
i915 0000:00:02.0: [drm:verify_connector_state [i915]] [CONNECTOR:215:eDP-1]
i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]] [CRTC:91:pipe A]
[drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec status: 0
i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150 [i915]] DPLL 0

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index ddf8d3bb7a7d..a6038fe74f63 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const struct intel_crtc_state *crtc_state)
 	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
 		return false;
 
+	if (!crtc_state->enable_fbc)
+		return false;
+
 	if (!params->plane_visible)
 		return false;
 
-- 
2.25.1

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

[Intel-gfx] [PATCH v3 2/3] drm/i915/display: Do not write in removed FBC fence registers

[José Roberto de Souza]

From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>

Platforms without fences don't have FBC host tracking and those
registers are marked as reserved in those platforms.

v2: checking num_fences to write to FBC fence registers (Ville)

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index a6038fe74f63..1d76e3646a25 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -320,7 +320,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 			       SNB_CPU_FENCE_ENABLE | params->fence_id);
 		intel_de_write(dev_priv, DPFC_CPU_FENCE_OFFSET,
 			       params->crtc.fence_y_offset);
-	} else {
+	} else if (dev_priv->ggtt.num_fences) {
 		intel_de_write(dev_priv, SNB_DPFC_CTL_SA, 0);
 		intel_de_write(dev_priv, DPFC_CPU_FENCE_OFFSET, 0);
 	}
-- 
2.25.1

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

[Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[José Roberto de Souza]

dGFX have local memory so it do not have aperture and do not support
CPU fences but even for iGFX it have a small number of fences.

As replacement for fences to track frontbuffer modifications by CPU
we have a software tracking that is already in used by FBC and PSR.
PSR don't support fences so it shows that this tracking is reliable.

So lets make fences a nice-to-have to activate FBC for GEN9+, this
will allow us to enable FBC for dGFXs and iGFXs even when there is no
available fence.

We do not set fences to rotated planes but FBC only have restrictions
against 16bpp, so adding it here.

Also adding a new check for the tiling format, fences are only set
to X and Y tiled planes but again FBC don't have any restrictions
against tiling so adding linear as supported as well, other formats
should be added after tested but IGT only supports drawing in thse
3 formats.

intel_fbc_hw_tracking_covers_screen() maybe can also have the same
treatment as fences but BSpec is not clear if the size limitation is
for hardware tracking or general use of FBC and I don't have a 5K
display to test it, so keeping as is for safety.

v2:
- Added tiling and pixel format rotation checks
- Changed the GEN version not requiring fences to 11 from 9, DDX
needs some changes but it don't have support for GEN11+

v3:
- Changed back to GEN9+
- Moved GEN test to inside of tiling_is_valid()

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 45 ++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 1d76e3646a25..a0d1d661a006 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -585,7 +585,7 @@ static bool stride_is_valid(struct drm_i915_private *dev_priv,
 }
 
 static bool pixel_format_is_valid(struct drm_i915_private *dev_priv,
-				  u32 pixel_format)
+				  u32 pixel_format, unsigned int rotation)
 {
 	switch (pixel_format) {
 	case DRM_FORMAT_XRGB8888:
@@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct drm_i915_private *dev_priv,
 		/* WaFbcOnly1to1Ratio:ctg */
 		if (IS_G4X(dev_priv))
 			return false;
+		if ((rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) &&
+		    INTEL_GEN(dev_priv) >= 9)
+			return false;
 		return true;
 	default:
 		return false;
@@ -639,6 +642,22 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
 	return effective_w <= max_w && effective_h <= max_h;
 }
 
+static bool tiling_is_valid(struct drm_i915_private *dev_priv,
+			    uint64_t modifier)
+{
+	switch (modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+		if (INTEL_GEN(dev_priv) >= 9)
+			return true;
+		return false;
+	case I915_FORMAT_MOD_X_TILED:
+	case I915_FORMAT_MOD_Y_TILED:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 					 const struct intel_crtc_state *crtc_state,
 					 const struct intel_plane_state *plane_state)
@@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
 
 	cache->fb.format = fb->format;
 	cache->fb.stride = fb->pitches[0];
+	cache->fb.modifier = fb->modifier;
 
 	drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE &&
 		    !plane_state->vma->fence);
@@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
-	/* The use of a CPU fence is mandatory in order to detect writes
-	 * by the CPU to the scanout and trigger updates to the FBC.
+	/* The use of a CPU fence is one of two ways to detect writes by the
+	 * CPU to the scanout and trigger updates to the FBC.
+	 *
+	 * The other method is by software tracking(see
+	 * intel_fbc_invalidate/flush()), it will manually notify FBC and nuke
+	 * the current compressed buffer and recompress it.
 	 *
 	 * Note that is possible for a tiled surface to be unmappable (and
-	 * so have no fence associated with it) due to aperture constaints
+	 * so have no fence associated with it) due to aperture constraints
 	 * at the time of pinning.
 	 *
 	 * FIXME with 90/270 degree rotation we should use the fence on
 	 * the normal GTT view (the rotated view doesn't even have a
 	 * fence). Would need changes to the FBC fence Y offset as well.
-	 * For now this will effecively disable FBC with 90/270 degree
+	 * For now this will effectively disable FBC with 90/270 degree
 	 * rotation.
 	 */
-	if (cache->fence_id < 0) {
+	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
 		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
 		return false;
 	}
+
+	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
+		fbc->no_fbc_reason = "tiling unsupported";
+		return false;
+	}
+
 	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
 	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
 		fbc->no_fbc_reason = "rotation unsupported";
@@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
-	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
+	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
+				   cache->plane.rotation)) {
 		fbc->no_fbc_reason = "pixel format is invalid";
 		return false;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3330b538d379..bf88663d8217 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -413,6 +413,7 @@ struct intel_fbc {
 		struct {
 			const struct drm_format_info *format;
 			unsigned int stride;
+			u64 modifier;
 		} fb;
 		u16 gen9_wa_cfb_stride;
 		s8 fence_id;
-- 
2.25.1

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter
URL   : https://patchwork.freedesktop.org/series/73618/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
a2421f8f2be7 drm/i915/display: Deactive FBC in fastsets when disabled by parameter
-:20: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#20: 
i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR debug to f

total: 0 errors, 1 warnings, 0 checks, 9 lines checked
9eaa91cf030e drm/i915/display: Do not write in removed FBC fence registers
37c66998b30e drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter
URL   : https://patchwork.freedesktop.org/series/73618/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7963 -> Patchwork_16615
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16615 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16615, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_16615:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_evict:
    - fi-bwr-2160:        [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-bwr-2160/igt@i915_selftest@live_evict.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-bwr-2160/igt@i915_selftest@live_evict.html

  
Known issues
------------

  Here are the changes found in Patchwork_16615 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_gtt:
    - fi-bdw-5557u:       [PASS][3] -> [TIMEOUT][4] ([fdo#112271])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-bdw-5557u/igt@i915_selftest@live_gtt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-bdw-5557u/igt@i915_selftest@live_gtt.html

  * igt@i915_selftest@live_sanitycheck:
    - fi-icl-u3:          [PASS][5] -> [DMESG-WARN][6] ([i915#585]) +39 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-hsw-peppy:       [INCOMPLETE][7] ([i915#694] / [i915#816]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-hsw-peppy/igt@gem_close_race@basic-threads.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-hsw-peppy/igt@gem_close_race@basic-threads.html

  * igt@i915_selftest@live_active:
    - fi-icl-dsi:         [DMESG-FAIL][9] ([i915#765]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-icl-dsi/igt@i915_selftest@live_active.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-icl-dsi/igt@i915_selftest@live_active.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-cfl-8700k:       [INCOMPLETE][11] ([i915#424]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html
    - fi-cml-s:           [DMESG-FAIL][13] ([i915#877]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-cml-s/igt@i915_selftest@live_gem_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-cml-s/igt@i915_selftest@live_gem_contexts.html

  * igt@i915_selftest@live_gt_heartbeat:
    - fi-kbl-7500u:       [DMESG-FAIL][15] ([fdo#112406]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-kbl-7500u/igt@i915_selftest@live_gt_heartbeat.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-kbl-7500u/igt@i915_selftest@live_gt_heartbeat.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - fi-bsw-n3050:       [FAIL][17] ([i915#34]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html

  
#### Warnings ####

  * igt@i915_pm_rpm@basic-rte:
    - fi-kbl-guc:         [FAIL][19] ([i915#579]) -> [SKIP][20] ([fdo#109271])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-icl-u3:          [SKIP][21] ([fdo#109284] / [fdo#111827]) -> [SKIP][22] ([fdo#109284] / [fdo#111827] / [i915#585])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-icl-u3/igt@kms_chamelium@dp-hpd-fast.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-icl-u3/igt@kms_chamelium@dp-hpd-fast.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u3:          [SKIP][23] ([fdo#109285]) -> [SKIP][24] ([fdo#109285] / [i915#585])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7963/fi-icl-u3/igt@kms_force_connector_basic@force-load-detect.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/fi-icl-u3/igt@kms_force_connector_basic@force-load-detect.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [fdo#112406]: https://bugs.freedesktop.org/show_bug.cgi?id=112406
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
  [i915#585]: https://gitlab.freedesktop.org/drm/intel/issues/585
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#765]: https://gitlab.freedesktop.org/drm/intel/issues/765
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#877]: https://gitlab.freedesktop.org/drm/intel/issues/877


Participating hosts (50 -> 41)
------------------------------

  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-glk-dsi fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-gdg-551 fi-bdw-samus fi-skl-6700k2 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7963 -> Patchwork_16615

  CI-20190529: 20190529
  CI_DRM_7963: e0d737598eb749378a5dc4ed3dfafc6f79d512cb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5448: 116020b1f83c1b3994c76882df7f77b6731d78ba @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16615: 37c66998b30eddbf673992930130fc0ddf9198b9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

37c66998b30e drm/i915/display/fbc: Make fences a nice-to-have for GEN9+
9eaa91cf030e drm/i915/display: Do not write in removed FBC fence registers
a2421f8f2be7 drm/i915/display: Deactive FBC in fastsets when disabled by parameter

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16615/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Ville Syrjälä]

On Tue, Feb 18, 2020 at 05:42:28PM -0800, José Roberto de Souza wrote:
> Most of the kms_frontbuffer_tracking tests disables the feature being
> tested, draw, get the CRC then enable the feature, draw again, get the
> CRC and check if it matches.
> Some times it is able to do that with a fastset, so
> intel_pre_plane_update() is executed but intel_fbc_can_flip_nuke() was
> not checking if FBC is now enabled in this CRTC leaving FBC active and
> causing the warning bellow in __intel_fbc_disable()
> 
> [IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-indfb-multidraw
> Setting dangerous option enable_fbc - tainting kernel
> i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR debug to f
> i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid debug mask f
> i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR debug to 1
> i915 0000:00:02.0: [drm:intel_atomic_check [i915]] [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of EDID bpp 24, requested bpp 36, max platform bpp 36
> [drm:intel_dp_compute_config [i915]] DP link computation with max lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
> [drm:intel_dp_compute_config [i915]] Force DSC en = 0
> [drm:intel_dp_compute_config [i915]] DP lane count 2 clock 270000 bpp 24
> [drm:intel_dp_compute_config [i915]] DP link rate required 414360 available 540000
> i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp: 24, pipe bpp: 24, dithering: 0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] [CRTC:91:pipe A] enable: yes [fastset]
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active: yes, output_types: EDP (0x100), output format: RGB
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n: lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202, link_n: 524288, tu: 64
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0, infoframes enabled: 0x0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] requested mode:
> [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted mode:
> [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> [drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920 1968 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port clock: 270000, pipe src size: 1920x1080, pixel rate 138120
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] linetime: 119, ips linetime: 0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] num_scalers: 2, scaler_users: 0x0, scaler_id: -1
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x00000000, size: 0x00000000, disabled, force thru: no
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0
> [drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0: 0x1c001a5, cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1: 0x0, mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0, mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0, mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] csc_mode: 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST master transcoder: <invalid>
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24 little-endian (0x34325258), visible: yes
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotation: 0x1, scaler: -1
> i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src: 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
> i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]] Disabling PSR1
> i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel doesn't support DRRS
> ------------[ cut here ]------------
> i915 0000:00:02.0: drm_WARN_ON(fbc->active)
> WARNING: CPU: 4 PID: 1175 at drivers/gpu/drm/i915/display/intel_fbc.c:973 __intel_fbc_disable+0xa5/0x130 [i915]
> Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal coretemp crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core cdc_ether e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt intel_lpss_pci prime_numbers
> CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted: G     U            5.5.0-CI-Trybot_5651+ #1
> Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750 06/14/2019
> RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
> Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7 c1 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce e0 <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33 30 e1
> RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff8884799d0000 RCX: 0000000000000006
> RDX: 0000000000001905 RSI: ffff888495dac970 RDI: ffffffff823731a1
> RBP: ffff88847c05d000 R08: ffff888495dac970 R09: 0000000000000000
> R10: ffffc90000613b88 R11: 0000000000000000 R12: ffff88849bba7e40
> R13: ffff8884799d0000 R14: ffff888498564000 R15: 0000000000000000
> FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4: 0000000000760ee0
> PKRU: 55555554
> Call Trace:
>  intel_fbc_disable+0x4a/0x50 [i915]
>  intel_update_crtc+0x12c/0x1d0 [i915]
>  skl_commit_modeset_enables+0x14d/0x600 [i915]
>  intel_atomic_commit_tail+0x30d/0x1480 [i915]
>  ? queue_work_on+0x31/0x70
>  ? intel_atomic_commit_ready+0x3f/0x48 [i915]
>  ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
>  intel_atomic_commit+0x312/0x390 [i915]
>  intel_psr_fastset_force+0x119/0x150 [i915]
>  i915_edp_psr_debug_set+0x53/0x70 [i915]
>  simple_attr_write+0xb0/0xd0
>  full_proxy_write+0x51/0x80
>  vfs_write+0xb9/0x1d0
>  ksys_write+0x9f/0xe0
>  do_syscall_64+0x4f/0x220
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f8157240281
> Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f 84 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89 d4 53
> RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f8157240281
> RDX: 0000000000000003 RSI: 00007f8157901152 RDI: 0000000000000008
> RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8157901152
> R13: 0000000000000008 R14: 00005589d298dce0 R15: 0000000000000000
> irq event stamp: 55208
> hardirqs last  enabled at (55207): [<ffffffff8112f3fc>] vprintk_emit+0xcc/0x330
> hardirqs last disabled at (55208): [<ffffffff81001ca0>] trace_hardirqs_off_thunk+0x1a/0x1c
> softirqs last  enabled at (54926): [<ffffffff81e00385>] __do_softirq+0x385/0x47f
> softirqs last disabled at (54915): [<ffffffff810ba15a>] irq_exit+0xba/0xc0
> ---[ end trace afa50c52e5a512bb ]---
> [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
> i915 0000:00:02.0: [drm:verify_connector_state [i915]] [CONNECTOR:215:eDP-1]
> i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]] [CRTC:91:pipe A]
> [drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec status: 0
> i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150 [i915]] DPLL 0
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index ddf8d3bb7a7d..a6038fe74f63 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const struct intel_crtc_state *crtc_state)
>  	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
>  		return false;
>  
> +	if (!crtc_state->enable_fbc)
> +		return false;

The code is still quite the horror show, but from the looks of things
we'd want this check in intel_fbc_can_activate() instead.

With the check relocated this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

PS. Oh dear, we still have that crtc_state->enable_fbc mess in there.
I should have patches to eliminate that tucked away in a branch
somewhere...

> +
>  	if (!params->plane_visible)
>  		return false;
>  
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Souza, Jose]

On Wed, 2020-02-19 at 15:37 +0200, Ville Syrjälä wrote:
> On Tue, Feb 18, 2020 at 05:42:28PM -0800, José Roberto de Souza
> wrote:
> > Most of the kms_frontbuffer_tracking tests disables the feature
> > being
> > tested, draw, get the CRC then enable the feature, draw again, get
> > the
> > CRC and check if it matches.
> > Some times it is able to do that with a fastset, so
> > intel_pre_plane_update() is executed but intel_fbc_can_flip_nuke()
> > was
> > not checking if FBC is now enabled in this CRTC leaving FBC active
> > and
> > causing the warning bellow in __intel_fbc_disable()
> > 
> > [IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-indfb-
> > multidraw
> > Setting dangerous option enable_fbc - tainting kernel
> > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR
> > debug to f
> > i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid debug
> > mask f
> > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR
> > debug to 1
> > i915 0000:00:02.0: [drm:intel_atomic_check [i915]]
> > [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of EDID
> > bpp 24, requested bpp 36, max platform bpp 36
> > [drm:intel_dp_compute_config [i915]] DP link computation with max
> > lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
> > [drm:intel_dp_compute_config [i915]] Force DSC en = 0
> > [drm:intel_dp_compute_config [i915]] DP lane count 2 clock 270000
> > bpp 24
> > [drm:intel_dp_compute_config [i915]] DP link rate required 414360
> > available 540000
> > i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp: 24,
> > pipe bpp: 24, dithering: 0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > [CRTC:91:pipe A] enable: yes [fastset]
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active: yes,
> > output_types: EDP (0x100), output format: RGB
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n:
> > lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202, link_n:
> > 524288, tu: 64
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio: 0,
> > infoframes: 0, infoframes enabled: 0x0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] requested
> > mode:
> > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120
> > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted
> > mode:
> > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120
> > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > [drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920 1968
> > 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port clock:
> > 270000, pipe src size: 1920x1080, pixel rate 138120
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] linetime:
> > 119, ips linetime: 0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] num_scalers:
> > 2, scaler_users: 0x0, scaler_id: -1
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch pfit:
> > pos: 0x00000000, size: 0x00000000, disabled, force thru: no
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0,
> > double wide: 0
> > [drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0: 0x1c001a5,
> > cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1: 0x0,
> > mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0,
> > mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0,
> > mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] csc_mode:
> > 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST master
> > transcoder: <invalid>
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24 little-
> > endian (0x34325258), visible: yes
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotatio
> > n: 0x1, scaler: -1
> > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src:
> > 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
> > i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]] Disabling
> > PSR1
> > i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel doesn't
> > support DRRS
> > ------------[ cut here ]------------
> > i915 0000:00:02.0: drm_WARN_ON(fbc->active)
> > WARNING: CPU: 4 PID: 1175 at
> > drivers/gpu/drm/i915/display/intel_fbc.c:973
> > __intel_fbc_disable+0xa5/0x130 [i915]
> > Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek
> > snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal coretemp
> > crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg
> > snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core cdc_ether
> > e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt
> > intel_lpss_pci prime_numbers
> > CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted:
> > G     U            5.5.0-CI-Trybot_5651+ #1
> > Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U
> > DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750
> > 06/14/2019
> > RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
> > Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7 c1
> > 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce e0
> > <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33 30 e1
> > RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
> > RAX: 0000000000000000 RBX: ffff8884799d0000 RCX: 0000000000000006
> > RDX: 0000000000001905 RSI: ffff888495dac970 RDI: ffffffff823731a1
> > RBP: ffff88847c05d000 R08: ffff888495dac970 R09: 0000000000000000
> > R10: ffffc90000613b88 R11: 0000000000000000 R12: ffff88849bba7e40
> > R13: ffff8884799d0000 R14: ffff888498564000 R15: 0000000000000000
> > FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000)
> > knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4: 0000000000760ee0
> > PKRU: 55555554
> > Call Trace:
> >  intel_fbc_disable+0x4a/0x50 [i915]
> >  intel_update_crtc+0x12c/0x1d0 [i915]
> >  skl_commit_modeset_enables+0x14d/0x600 [i915]
> >  intel_atomic_commit_tail+0x30d/0x1480 [i915]
> >  ? queue_work_on+0x31/0x70
> >  ? intel_atomic_commit_ready+0x3f/0x48 [i915]
> >  ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
> >  intel_atomic_commit+0x312/0x390 [i915]
> >  intel_psr_fastset_force+0x119/0x150 [i915]
> >  i915_edp_psr_debug_set+0x53/0x70 [i915]
> >  simple_attr_write+0xb0/0xd0
> >  full_proxy_write+0x51/0x80
> >  vfs_write+0xb9/0x1d0
> >  ksys_write+0x9f/0xe0
> >  do_syscall_64+0x4f/0x220
> >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x7f8157240281
> > Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f 84
> > 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f 05
> > <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89 d4 53
> > RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX:
> > 0000000000000001
> > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f8157240281
> > RDX: 0000000000000003 RSI: 00007f8157901152 RDI: 0000000000000008
> > RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8157901152
> > R13: 0000000000000008 R14: 00005589d298dce0 R15: 0000000000000000
> > irq event stamp: 55208
> > hardirqs last  enabled at (55207): [<ffffffff8112f3fc>]
> > vprintk_emit+0xcc/0x330
> > hardirqs last disabled at (55208): [<ffffffff81001ca0>]
> > trace_hardirqs_off_thunk+0x1a/0x1c
> > softirqs last  enabled at (54926): [<ffffffff81e00385>]
> > __do_softirq+0x385/0x47f
> > softirqs last disabled at (54915): [<ffffffff810ba15a>]
> > irq_exit+0xba/0xc0
> > ---[ end trace afa50c52e5a512bb ]---
> > [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
> > i915 0000:00:02.0: [drm:verify_connector_state [i915]]
> > [CONNECTOR:215:eDP-1]
> > i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]]
> > [CRTC:91:pipe A]
> > [drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec status: 0
> > i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150 [i915]]
> > DPLL 0
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index ddf8d3bb7a7d..a6038fe74f63 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const
> > struct intel_crtc_state *crtc_state)
> >  	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> >  		return false;
> >  
> > +	if (!crtc_state->enable_fbc)
> > +		return false;
> 
> The code is still quite the horror show, but from the looks of things
> we'd want this check in intel_fbc_can_activate() instead.

From intel_fbc_flush() we don't have the intel_crtc_state, so move it
to intel_fbc_can_activate() and cache the enable_fbc in
intel_fbc_update_state_cache() too?

> 
> With the check relocated this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> PS. Oh dear, we still have that crtc_state->enable_fbc mess in there.
> I should have patches to eliminate that tucked away in a branch
> somewhere...
> 
> > +
> >  	if (!params->plane_visible)
> >  		return false;
> >  
> > -- 
> > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Ville Syrjälä]

On Wed, Feb 19, 2020 at 06:37:27PM +0000, Souza, Jose wrote:
> On Wed, 2020-02-19 at 15:37 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 18, 2020 at 05:42:28PM -0800, José Roberto de Souza
> > wrote:
> > > Most of the kms_frontbuffer_tracking tests disables the feature
> > > being
> > > tested, draw, get the CRC then enable the feature, draw again, get
> > > the
> > > CRC and check if it matches.
> > > Some times it is able to do that with a fastset, so
> > > intel_pre_plane_update() is executed but intel_fbc_can_flip_nuke()
> > > was
> > > not checking if FBC is now enabled in this CRTC leaving FBC active
> > > and
> > > causing the warning bellow in __intel_fbc_disable()
> > > 
> > > [IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-indfb-
> > > multidraw
> > > Setting dangerous option enable_fbc - tainting kernel
> > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR
> > > debug to f
> > > i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid debug
> > > mask f
> > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting PSR
> > > debug to 1
> > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]]
> > > [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of EDID
> > > bpp 24, requested bpp 36, max platform bpp 36
> > > [drm:intel_dp_compute_config [i915]] DP link computation with max
> > > lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
> > > [drm:intel_dp_compute_config [i915]] Force DSC en = 0
> > > [drm:intel_dp_compute_config [i915]] DP lane count 2 clock 270000
> > > bpp 24
> > > [drm:intel_dp_compute_config [i915]] DP link rate required 414360
> > > available 540000
> > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp: 24,
> > > pipe bpp: 24, dithering: 0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > [CRTC:91:pipe A] enable: yes [fastset]
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active: yes,
> > > output_types: EDP (0x100), output format: RGB
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n:
> > > lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202, link_n:
> > > 524288, tu: 64
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio: 0,
> > > infoframes: 0, infoframes enabled: 0x0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] requested
> > > mode:
> > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120
> > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted
> > > mode:
> > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60 138120
> > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > [drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920 1968
> > > 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port clock:
> > > 270000, pipe src size: 1920x1080, pixel rate 138120
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] linetime:
> > > 119, ips linetime: 0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] num_scalers:
> > > 2, scaler_users: 0x0, scaler_id: -1
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch pfit:
> > > pos: 0x00000000, size: 0x00000000, disabled, force thru: no
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0,
> > > double wide: 0
> > > [drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0: 0x1c001a5,
> > > cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1: 0x0,
> > > mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0,
> > > mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0,
> > > mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] csc_mode:
> > > 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST master
> > > transcoder: <invalid>
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24 little-
> > > endian (0x34325258), visible: yes
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotatio
> > > n: 0x1, scaler: -1
> > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src:
> > > 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
> > > i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]] Disabling
> > > PSR1
> > > i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel doesn't
> > > support DRRS
> > > ------------[ cut here ]------------
> > > i915 0000:00:02.0: drm_WARN_ON(fbc->active)
> > > WARNING: CPU: 4 PID: 1175 at
> > > drivers/gpu/drm/i915/display/intel_fbc.c:973
> > > __intel_fbc_disable+0xa5/0x130 [i915]
> > > Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek
> > > snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal coretemp
> > > crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg
> > > snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core cdc_ether
> > > e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt
> > > intel_lpss_pci prime_numbers
> > > CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted:
> > > G     U            5.5.0-CI-Trybot_5651+ #1
> > > Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U
> > > DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750
> > > 06/14/2019
> > > RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
> > > Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7 c1
> > > 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce e0
> > > <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33 30 e1
> > > RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
> > > RAX: 0000000000000000 RBX: ffff8884799d0000 RCX: 0000000000000006
> > > RDX: 0000000000001905 RSI: ffff888495dac970 RDI: ffffffff823731a1
> > > RBP: ffff88847c05d000 R08: ffff888495dac970 R09: 0000000000000000
> > > R10: ffffc90000613b88 R11: 0000000000000000 R12: ffff88849bba7e40
> > > R13: ffff8884799d0000 R14: ffff888498564000 R15: 0000000000000000
> > > FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000)
> > > knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4: 0000000000760ee0
> > > PKRU: 55555554
> > > Call Trace:
> > >  intel_fbc_disable+0x4a/0x50 [i915]
> > >  intel_update_crtc+0x12c/0x1d0 [i915]
> > >  skl_commit_modeset_enables+0x14d/0x600 [i915]
> > >  intel_atomic_commit_tail+0x30d/0x1480 [i915]
> > >  ? queue_work_on+0x31/0x70
> > >  ? intel_atomic_commit_ready+0x3f/0x48 [i915]
> > >  ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
> > >  intel_atomic_commit+0x312/0x390 [i915]
> > >  intel_psr_fastset_force+0x119/0x150 [i915]
> > >  i915_edp_psr_debug_set+0x53/0x70 [i915]
> > >  simple_attr_write+0xb0/0xd0
> > >  full_proxy_write+0x51/0x80
> > >  vfs_write+0xb9/0x1d0
> > >  ksys_write+0x9f/0xe0
> > >  do_syscall_64+0x4f/0x220
> > >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > RIP: 0033:0x7f8157240281
> > > Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f 84
> > > 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f 05
> > > <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89 d4 53
> > > RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX:
> > > 0000000000000001
> > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f8157240281
> > > RDX: 0000000000000003 RSI: 00007f8157901152 RDI: 0000000000000008
> > > RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8157901152
> > > R13: 0000000000000008 R14: 00005589d298dce0 R15: 0000000000000000
> > > irq event stamp: 55208
> > > hardirqs last  enabled at (55207): [<ffffffff8112f3fc>]
> > > vprintk_emit+0xcc/0x330
> > > hardirqs last disabled at (55208): [<ffffffff81001ca0>]
> > > trace_hardirqs_off_thunk+0x1a/0x1c
> > > softirqs last  enabled at (54926): [<ffffffff81e00385>]
> > > __do_softirq+0x385/0x47f
> > > softirqs last disabled at (54915): [<ffffffff810ba15a>]
> > > irq_exit+0xba/0xc0
> > > ---[ end trace afa50c52e5a512bb ]---
> > > [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
> > > i915 0000:00:02.0: [drm:verify_connector_state [i915]]
> > > [CONNECTOR:215:eDP-1]
> > > i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]]
> > > [CRTC:91:pipe A]
> > > [drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec status: 0
> > > i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150 [i915]]
> > > DPLL 0
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index ddf8d3bb7a7d..a6038fe74f63 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const
> > > struct intel_crtc_state *crtc_state)
> > >  	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> > >  		return false;
> > >  
> > > +	if (!crtc_state->enable_fbc)
> > > +		return false;
> > 
> > The code is still quite the horror show, but from the looks of things
> > we'd want this check in intel_fbc_can_activate() instead.
> 
> From intel_fbc_flush() we don't have the intel_crtc_state, so move it
> to intel_fbc_can_activate() and cache the enable_fbc in
> intel_fbc_update_state_cache() too?

Oh, I failed to notice you're looking at the crtc state here. Thought
you just check the modparam directly. I'd just call intel_fbc_can_enable()
in intel_fbc_can_activate() directly. That crtc_state->enable_fbc thing
is just hopelessly broken atm.

> 
> > 
> > With the check relocated this is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > PS. Oh dear, we still have that crtc_state->enable_fbc mess in there.
> > I should have patches to eliminate that tucked away in a branch
> > somewhere...
> > 
> > > +
> > >  	if (!params->plane_visible)
> > >  		return false;
> > >  
> > > -- 
> > > 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Ville Syrjälä]

On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza wrote:
> dGFX have local memory so it do not have aperture and do not support
> CPU fences but even for iGFX it have a small number of fences.
> 
> As replacement for fences to track frontbuffer modifications by CPU
> we have a software tracking that is already in used by FBC and PSR.
> PSR don't support fences so it shows that this tracking is reliable.
> 
> So lets make fences a nice-to-have to activate FBC for GEN9+, this
> will allow us to enable FBC for dGFXs and iGFXs even when there is no
> available fence.
> 
> We do not set fences to rotated planes but FBC only have restrictions
> against 16bpp, so adding it here.
> 
> Also adding a new check for the tiling format, fences are only set
> to X and Y tiled planes but again FBC don't have any restrictions
> against tiling so adding linear as supported as well, other formats
> should be added after tested but IGT only supports drawing in thse
> 3 formats.
> 
> intel_fbc_hw_tracking_covers_screen() maybe can also have the same
> treatment as fences but BSpec is not clear if the size limitation is
> for hardware tracking or general use of FBC and I don't have a 5K
> display to test it, so keeping as is for safety.
> 
> v2:
> - Added tiling and pixel format rotation checks
> - Changed the GEN version not requiring fences to 11 from 9, DDX
> needs some changes but it don't have support for GEN11+
> 
> v3:
> - Changed back to GEN9+
> - Moved GEN test to inside of tiling_is_valid()
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 45 ++++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>  2 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1d76e3646a25..a0d1d661a006 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -585,7 +585,7 @@ static bool stride_is_valid(struct drm_i915_private *dev_priv,
>  }
>  
>  static bool pixel_format_is_valid(struct drm_i915_private *dev_priv,
> -				  u32 pixel_format)
> +				  u32 pixel_format, unsigned int rotation)
>  {
>  	switch (pixel_format) {
>  	case DRM_FORMAT_XRGB8888:
> @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct drm_i915_private *dev_priv,
>  		/* WaFbcOnly1to1Ratio:ctg */
>  		if (IS_G4X(dev_priv))
>  			return false;
> +		if ((rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270)) &&
> +		    INTEL_GEN(dev_priv) >= 9)
> +			return false;

Would still would prefer a rotations_is_valid() or some such thing.

>  		return true;
>  	default:
>  		return false;
> @@ -639,6 +642,22 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  	return effective_w <= max_w && effective_h <= max_h;
>  }
>  
> +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> +			    uint64_t modifier)
> +{
> +	switch (modifier) {
> +	case DRM_FORMAT_MOD_LINEAR:
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			return true;

Have we checked that eg. fbcon cursor still blinks correctly
with FBC active and all?

> +		return false;
> +	case I915_FORMAT_MOD_X_TILED:
> +	case I915_FORMAT_MOD_Y_TILED:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  					 const struct intel_crtc_state *crtc_state,
>  					 const struct intel_plane_state *plane_state)
> @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  
>  	cache->fb.format = fb->format;
>  	cache->fb.stride = fb->pitches[0];
> +	cache->fb.modifier = fb->modifier;
>  
>  	drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE &&
>  		    !plane_state->vma->fence);
> @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	/* The use of a CPU fence is mandatory in order to detect writes
> -	 * by the CPU to the scanout and trigger updates to the FBC.
> +	/* The use of a CPU fence is one of two ways to detect writes by the
> +	 * CPU to the scanout and trigger updates to the FBC.
> +	 *
> +	 * The other method is by software tracking(see
> +	 * intel_fbc_invalidate/flush()), it will manually notify FBC and nuke
> +	 * the current compressed buffer and recompress it.
>  	 *
>  	 * Note that is possible for a tiled surface to be unmappable (and
> -	 * so have no fence associated with it) due to aperture constaints
> +	 * so have no fence associated with it) due to aperture constraints
>  	 * at the time of pinning.
>  	 *
>  	 * FIXME with 90/270 degree rotation we should use the fence on
>  	 * the normal GTT view (the rotated view doesn't even have a
>  	 * fence). Would need changes to the FBC fence Y offset as well.
> -	 * For now this will effecively disable FBC with 90/270 degree
> +	 * For now this will effectively disable FBC with 90/270 degree
>  	 * rotation.
>  	 */
> -	if (cache->fence_id < 0) {
> +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
>  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
>  		return false;
>  	}
> +
> +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> +		fbc->no_fbc_reason = "tiling unsupported";
> +		return false;
> +	}
> +
>  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
>  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
>  		fbc->no_fbc_reason = "rotation unsupported";
> @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> +				   cache->plane.rotation)) {
>  		fbc->no_fbc_reason = "pixel format is invalid";
>  		return false;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3330b538d379..bf88663d8217 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -413,6 +413,7 @@ struct intel_fbc {
>  		struct {
>  			const struct drm_format_info *format;
>  			unsigned int stride;
> +			u64 modifier;
>  		} fb;
>  		u16 gen9_wa_cfb_stride;
>  		s8 fence_id;
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Souza, Jose]

On Wed, 2020-02-19 at 20:52 +0200, Ville Syrjälä wrote:
> On Wed, Feb 19, 2020 at 06:37:27PM +0000, Souza, Jose wrote:
> > On Wed, 2020-02-19 at 15:37 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 18, 2020 at 05:42:28PM -0800, José Roberto de Souza
> > > wrote:
> > > > Most of the kms_frontbuffer_tracking tests disables the feature
> > > > being
> > > > tested, draw, get the CRC then enable the feature, draw again,
> > > > get
> > > > the
> > > > CRC and check if it matches.
> > > > Some times it is able to do that with a fastset, so
> > > > intel_pre_plane_update() is executed but
> > > > intel_fbc_can_flip_nuke()
> > > > was
> > > > not checking if FBC is now enabled in this CRTC leaving FBC
> > > > active
> > > > and
> > > > causing the warning bellow in __intel_fbc_disable()
> > > > 
> > > > [IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-
> > > > indfb-
> > > > multidraw
> > > > Setting dangerous option enable_fbc - tainting kernel
> > > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting
> > > > PSR
> > > > debug to f
> > > > i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid
> > > > debug
> > > > mask f
> > > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting
> > > > PSR
> > > > debug to 1
> > > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]]
> > > > [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of
> > > > EDID
> > > > bpp 24, requested bpp 36, max platform bpp 36
> > > > [drm:intel_dp_compute_config [i915]] DP link computation with
> > > > max
> > > > lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
> > > > [drm:intel_dp_compute_config [i915]] Force DSC en = 0
> > > > [drm:intel_dp_compute_config [i915]] DP lane count 2 clock
> > > > 270000
> > > > bpp 24
> > > > [drm:intel_dp_compute_config [i915]] DP link rate required
> > > > 414360
> > > > available 540000
> > > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp:
> > > > 24,
> > > > pipe bpp: 24, dithering: 0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > [CRTC:91:pipe A] enable: yes [fastset]
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active:
> > > > yes,
> > > > output_types: EDP (0x100), output format: RGB
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n:
> > > > lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202,
> > > > link_n:
> > > > 524288, tu: 64
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio:
> > > > 0,
> > > > infoframes: 0, infoframes enabled: 0x0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > requested
> > > > mode:
> > > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60
> > > > 138120
> > > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted
> > > > mode:
> > > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60
> > > > 138120
> > > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > > [drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920
> > > > 1968
> > > > 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port
> > > > clock:
> > > > 270000, pipe src size: 1920x1080, pixel rate 138120
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > linetime:
> > > > 119, ips linetime: 0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > num_scalers:
> > > > 2, scaler_users: 0x0, scaler_id: -1
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch
> > > > pfit:
> > > > pos: 0x00000000, size: 0x00000000, disabled, force thru: no
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0,
> > > > double wide: 0
> > > > [drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0:
> > > > 0x1c001a5,
> > > > cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1:
> > > > 0x0,
> > > > mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0,
> > > > mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0,
> > > > mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > csc_mode:
> > > > 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST
> > > > master
> > > > transcoder: <invalid>
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24
> > > > little-
> > > > endian (0x34325258), visible: yes
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotatio
> > > > n: 0x1, scaler: -1
> > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src:
> > > > 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
> > > > i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]]
> > > > Disabling
> > > > PSR1
> > > > i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel
> > > > doesn't
> > > > support DRRS
> > > > ------------[ cut here ]------------
> > > > i915 0000:00:02.0: drm_WARN_ON(fbc->active)
> > > > WARNING: CPU: 4 PID: 1175 at
> > > > drivers/gpu/drm/i915/display/intel_fbc.c:973
> > > > __intel_fbc_disable+0xa5/0x130 [i915]
> > > > Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek
> > > > snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal
> > > > coretemp
> > > > crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg
> > > > snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core
> > > > cdc_ether
> > > > e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt
> > > > intel_lpss_pci prime_numbers
> > > > CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted:
> > > > G     U            5.5.0-CI-Trybot_5651+ #1
> > > > Hardware name: Intel Corporation Ice Lake Client
> > > > Platform/IceLake U
> > > > DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750
> > > > 06/14/2019
> > > > RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
> > > > Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7
> > > > c1
> > > > 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce
> > > > e0
> > > > <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33
> > > > 30 e1
> > > > RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
> > > > RAX: 0000000000000000 RBX: ffff8884799d0000 RCX:
> > > > 0000000000000006
> > > > RDX: 0000000000001905 RSI: ffff888495dac970 RDI:
> > > > ffffffff823731a1
> > > > RBP: ffff88847c05d000 R08: ffff888495dac970 R09:
> > > > 0000000000000000
> > > > R10: ffffc90000613b88 R11: 0000000000000000 R12:
> > > > ffff88849bba7e40
> > > > R13: ffff8884799d0000 R14: ffff888498564000 R15:
> > > > 0000000000000000
> > > > FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000)
> > > > knlGS:0000000000000000
> > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4:
> > > > 0000000000760ee0
> > > > PKRU: 55555554
> > > > Call Trace:
> > > >  intel_fbc_disable+0x4a/0x50 [i915]
> > > >  intel_update_crtc+0x12c/0x1d0 [i915]
> > > >  skl_commit_modeset_enables+0x14d/0x600 [i915]
> > > >  intel_atomic_commit_tail+0x30d/0x1480 [i915]
> > > >  ? queue_work_on+0x31/0x70
> > > >  ? intel_atomic_commit_ready+0x3f/0x48 [i915]
> > > >  ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
> > > >  intel_atomic_commit+0x312/0x390 [i915]
> > > >  intel_psr_fastset_force+0x119/0x150 [i915]
> > > >  i915_edp_psr_debug_set+0x53/0x70 [i915]
> > > >  simple_attr_write+0xb0/0xd0
> > > >  full_proxy_write+0x51/0x80
> > > >  vfs_write+0xb9/0x1d0
> > > >  ksys_write+0x9f/0xe0
> > > >  do_syscall_64+0x4f/0x220
> > > >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > > RIP: 0033:0x7f8157240281
> > > > Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f
> > > > 84
> > > > 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f
> > > > 05
> > > > <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89
> > > > d4 53
> > > > RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX:
> > > > 0000000000000001
> > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
> > > > 00007f8157240281
> > > > RDX: 0000000000000003 RSI: 00007f8157901152 RDI:
> > > > 0000000000000008
> > > > RBP: 0000000000000003 R08: 0000000000000000 R09:
> > > > 0000000000000000
> > > > R10: 0000000000000000 R11: 0000000000000246 R12:
> > > > 00007f8157901152
> > > > R13: 0000000000000008 R14: 00005589d298dce0 R15:
> > > > 0000000000000000
> > > > irq event stamp: 55208
> > > > hardirqs last  enabled at (55207): [<ffffffff8112f3fc>]
> > > > vprintk_emit+0xcc/0x330
> > > > hardirqs last disabled at (55208): [<ffffffff81001ca0>]
> > > > trace_hardirqs_off_thunk+0x1a/0x1c
> > > > softirqs last  enabled at (54926): [<ffffffff81e00385>]
> > > > __do_softirq+0x385/0x47f
> > > > softirqs last disabled at (54915): [<ffffffff810ba15a>]
> > > > irq_exit+0xba/0xc0
> > > > ---[ end trace afa50c52e5a512bb ]---
> > > > [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
> > > > i915 0000:00:02.0: [drm:verify_connector_state [i915]]
> > > > [CONNECTOR:215:eDP-1]
> > > > i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]]
> > > > [CRTC:91:pipe A]
> > > > [drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec
> > > > status: 0
> > > > i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150
> > > > [i915]]
> > > > DPLL 0
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index ddf8d3bb7a7d..a6038fe74f63 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const
> > > > struct intel_crtc_state *crtc_state)
> > > >  	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> > > >  		return false;
> > > >  
> > > > +	if (!crtc_state->enable_fbc)
> > > > +		return false;
> > > 
> > > The code is still quite the horror show, but from the looks of
> > > things
> > > we'd want this check in intel_fbc_can_activate() instead.
> > 
> > From intel_fbc_flush() we don't have the intel_crtc_state, so move
> > it
> > to intel_fbc_can_activate() and cache the enable_fbc in
> > intel_fbc_update_state_cache() too?
> 
> Oh, I failed to notice you're looking at the crtc state here. Thought
> you just check the modparam directly. I'd just call
> intel_fbc_can_enable()
> in intel_fbc_can_activate() directly. That crtc_state->enable_fbc
> thing
> is just hopelessly broken atm.

crtc_state->enable_fbc=true is backed by intel_fbc_can_enable() and
later we have:
intel_update_crtc()
	...
	if (new_crtc_state->update_pipe && !new_crtc_state->enable_fbc)
		intel_fbc_disable(crtc);

My felling it that would be more consistent keep using it but let me
know what you think is best.

> 
> > > With the check relocated this is
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > PS. Oh dear, we still have that crtc_state->enable_fbc mess in
> > > there.
> > > I should have patches to eliminate that tucked away in a branch
> > > somewhere...
> > > 
> > > > +
> > > >  	if (!params->plane_visible)
> > > >  		return false;
> > > >  
> > > > -- 
> > > > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Souza, Jose]

On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> wrote:
> > dGFX have local memory so it do not have aperture and do not
> > support
> > CPU fences but even for iGFX it have a small number of fences.
> > 
> > As replacement for fences to track frontbuffer modifications by CPU
> > we have a software tracking that is already in used by FBC and PSR.
> > PSR don't support fences so it shows that this tracking is
> > reliable.
> > 
> > So lets make fences a nice-to-have to activate FBC for GEN9+, this
> > will allow us to enable FBC for dGFXs and iGFXs even when there is
> > no
> > available fence.
> > 
> > We do not set fences to rotated planes but FBC only have
> > restrictions
> > against 16bpp, so adding it here.
> > 
> > Also adding a new check for the tiling format, fences are only set
> > to X and Y tiled planes but again FBC don't have any restrictions
> > against tiling so adding linear as supported as well, other formats
> > should be added after tested but IGT only supports drawing in thse
> > 3 formats.
> > 
> > intel_fbc_hw_tracking_covers_screen() maybe can also have the same
> > treatment as fences but BSpec is not clear if the size limitation
> > is
> > for hardware tracking or general use of FBC and I don't have a 5K
> > display to test it, so keeping as is for safety.
> > 
> > v2:
> > - Added tiling and pixel format rotation checks
> > - Changed the GEN version not requiring fences to 11 from 9, DDX
> > needs some changes but it don't have support for GEN11+
> > 
> > v3:
> > - Changed back to GEN9+
> > - Moved GEN test to inside of tiling_is_valid()
> > 
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > ++++++++++++++++++++----
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  2 files changed, 39 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 1d76e3646a25..a0d1d661a006 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > drm_i915_private *dev_priv,
> >  }
> >  
> >  static bool pixel_format_is_valid(struct drm_i915_private
> > *dev_priv,
> > -				  u32 pixel_format)
> > +				  u32 pixel_format, unsigned int
> > rotation)
> >  {
> >  	switch (pixel_format) {
> >  	case DRM_FORMAT_XRGB8888:
> > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > drm_i915_private *dev_priv,
> >  		/* WaFbcOnly1to1Ratio:ctg */
> >  		if (IS_G4X(dev_priv))
> >  			return false;
> > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > DRM_MODE_ROTATE_270)) &&
> > +		    INTEL_GEN(dev_priv) >= 9)
> > +			return false;
> 
> Would still would prefer a rotations_is_valid() or some such thing.

Like this?

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
b/drivers/gpu/drm/i915/display/intel_fbc.c
index 5e35c894bdf9..692edd45b769 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
drm_i915_private *dev_priv,
                /* WaFbcOnly1to1Ratio:ctg */
                if (IS_G4X(dev_priv))
                        return false;
-               if ((rotation & (DRM_MODE_ROTATE_90 |
DRM_MODE_ROTATE_270)) &&
-                   INTEL_GEN(dev_priv) >= 9)
-                       return false;
                return true;
        default:
                return false;
        }
 }

+static bool rotations_is_valid(struct drm_i915_private *dev_priv,
+                              u32 pixel_format, unsigned int rotation)
+{
+       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
DRM_FORMAT_RGB565 &&
+           rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270))
+               return false;
+       return true;
+}
+
 /*
  * For some reason, the hardware tracking starts looking at whatever
we
  * programmed as the display plane base address register. It does not
look at
@@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
intel_crtc *crtc)
                return false;
        }

+       if (!rotations_is_valid(dev_priv, cache->fb.format->format,
+                               cache->plane.rotation)) {
+               fbc->no_fbc_reason = "plane rotation is invalid";
+               return false;
+       }
+
        if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE
&&
            cache->fb.format->has_alpha) {
                fbc->no_fbc_reason = "per-pixel alpha blending is
incompatible with FBC";


> 
> >  		return true;
> >  	default:
> >  		return false;
> > @@ -639,6 +642,22 @@ static bool
> > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> >  	return effective_w <= max_w && effective_h <= max_h;
> >  }
> >  
> > +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> > +			    uint64_t modifier)
> > +{
> > +	switch (modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +		if (INTEL_GEN(dev_priv) >= 9)
> > +			return true;
> 
> Have we checked that eg. fbcon cursor still blinks correctly
> with FBC active and all?

Hum on fbcon FBC is enabled but it never compress, IGT tests with
fences complete disables are working fine, screen is updated when tests
asks to with FBC enabled and compressing.

I will debug fbcon a little more to understand why it is never
compressing. 

> 
> > +		return false;
> > +	case I915_FORMAT_MOD_X_TILED:
> > +	case I915_FORMAT_MOD_Y_TILED:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> >  					 const struct intel_crtc_state
> > *crtc_state,
> >  					 const struct intel_plane_state
> > *plane_state)
> > @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct
> > intel_crtc *crtc,
> >  
> >  	cache->fb.format = fb->format;
> >  	cache->fb.stride = fb->pitches[0];
> > +	cache->fb.modifier = fb->modifier;
> >  
> >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > PLANE_HAS_FENCE &&
> >  		    !plane_state->vma->fence);
> > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	/* The use of a CPU fence is mandatory in order to detect
> > writes
> > -	 * by the CPU to the scanout and trigger updates to the FBC.
> > +	/* The use of a CPU fence is one of two ways to detect writes
> > by the
> > +	 * CPU to the scanout and trigger updates to the FBC.
> > +	 *
> > +	 * The other method is by software tracking(see
> > +	 * intel_fbc_invalidate/flush()), it will manually notify FBC
> > and nuke
> > +	 * the current compressed buffer and recompress it.
> >  	 *
> >  	 * Note that is possible for a tiled surface to be unmappable
> > (and
> > -	 * so have no fence associated with it) due to aperture
> > constaints
> > +	 * so have no fence associated with it) due to aperture
> > constraints
> >  	 * at the time of pinning.
> >  	 *
> >  	 * FIXME with 90/270 degree rotation we should use the fence on
> >  	 * the normal GTT view (the rotated view doesn't even have a
> >  	 * fence). Would need changes to the FBC fence Y offset as
> > well.
> > -	 * For now this will effecively disable FBC with 90/270 degree
> > +	 * For now this will effectively disable FBC with 90/270 degree
> >  	 * rotation.
> >  	 */
> > -	if (cache->fence_id < 0) {
> > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> >  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> >  		return false;
> >  	}
> > +
> > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > +		fbc->no_fbc_reason = "tiling unsupported";
> > +		return false;
> > +	}
> > +
> >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> >  		fbc->no_fbc_reason = "rotation unsupported";
> > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) 
> > {
> > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> > +				   cache->plane.rotation)) {
> >  		fbc->no_fbc_reason = "pixel format is invalid";
> >  		return false;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3330b538d379..bf88663d8217 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -413,6 +413,7 @@ struct intel_fbc {
> >  		struct {
> >  			const struct drm_format_info *format;
> >  			unsigned int stride;
> > +			u64 modifier;
> >  		} fb;
> >  		u16 gen9_wa_cfb_stride;
> >  		s8 fence_id;
> > -- 
> > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter (rev2)
URL   : https://patchwork.freedesktop.org/series/73618/
State : failure

== Summary ==

Applying: drm/i915/display: Deactive FBC in fastsets when disabled by parameter
Applying: drm/i915/display: Do not write in removed FBC fence registers
Applying: drm/i915/display/fbc: Make fences a nice-to-have for GEN9+
error: git diff header lacks filename information when removing 1 leading pathname component (line 2)
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0003 drm/i915/display/fbc: Make fences a nice-to-have for GEN9+
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Deactive FBC in fastsets when disabled by parameter

[Ville Syrjälä]

On Fri, Mar 06, 2020 at 12:22:09AM +0000, Souza, Jose wrote:
> On Wed, 2020-02-19 at 20:52 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 19, 2020 at 06:37:27PM +0000, Souza, Jose wrote:
> > > On Wed, 2020-02-19 at 15:37 +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 18, 2020 at 05:42:28PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > Most of the kms_frontbuffer_tracking tests disables the feature
> > > > > being
> > > > > tested, draw, get the CRC then enable the feature, draw again,
> > > > > get
> > > > > the
> > > > > CRC and check if it matches.
> > > > > Some times it is able to do that with a fastset, so
> > > > > intel_pre_plane_update() is executed but
> > > > > intel_fbc_can_flip_nuke()
> > > > > was
> > > > > not checking if FBC is now enabled in this CRTC leaving FBC
> > > > > active
> > > > > and
> > > > > causing the warning bellow in __intel_fbc_disable()
> > > > > 
> > > > > [IGT] kms_frontbuffer_tracking: starting subtest fbc-1p-pri-
> > > > > indfb-
> > > > > multidraw
> > > > > Setting dangerous option enable_fbc - tainting kernel
> > > > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting
> > > > > PSR
> > > > > debug to f
> > > > > i915 0000:00:02.0: [drm:intel_psr_debug_set [i915]] Invalid
> > > > > debug
> > > > > mask f
> > > > > i915 0000:00:02.0: [drm:i915_edp_psr_debug_set [i915]] Setting
> > > > > PSR
> > > > > debug to 1
> > > > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]]
> > > > > [CONNECTOR:215:eDP-1] Limiting display bpp to 24 instead of
> > > > > EDID
> > > > > bpp 24, requested bpp 36, max platform bpp 36
> > > > > [drm:intel_dp_compute_config [i915]] DP link computation with
> > > > > max
> > > > > lane count 2 max rate 270000 max bpp 24 pixel clock 138120KHz
> > > > > [drm:intel_dp_compute_config [i915]] Force DSC en = 0
> > > > > [drm:intel_dp_compute_config [i915]] DP lane count 2 clock
> > > > > 270000
> > > > > bpp 24
> > > > > [drm:intel_dp_compute_config [i915]] DP link rate required
> > > > > 414360
> > > > > available 540000
> > > > > i915 0000:00:02.0: [drm:intel_atomic_check [i915]] hw max bpp:
> > > > > 24,
> > > > > pipe bpp: 24, dithering: 0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > [CRTC:91:pipe A] enable: yes [fastset]
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] active:
> > > > > yes,
> > > > > output_types: EDP (0x100), output format: RGB
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > cpu_transcoder: EDP, pipe bpp: 24, dithering: 0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] dp m_n:
> > > > > lanes: 2; gmch_m: 6436858, gmch_n: 8388608, link_m: 268202,
> > > > > link_n:
> > > > > 524288, tu: 64
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] audio:
> > > > > 0,
> > > > > infoframes: 0, infoframes enabled: 0x0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > requested
> > > > > mode:
> > > > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60
> > > > > 138120
> > > > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] adjusted
> > > > > mode:
> > > > > [drm:drm_mode_debug_printmodeline] Modeline "1920x1080": 60
> > > > > 138120
> > > > > 1920 1968 2018 2052 1080 1084 1086 1122 0x48 0xa
> > > > > [drm:intel_dump_pipe_config [i915]] crtc timings: 138120 1920
> > > > > 1968
> > > > > 2018 2052 1080 1084 1086 1122, type: 0x48 flags: 0xa
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] port
> > > > > clock:
> > > > > 270000, pipe src size: 1920x1080, pixel rate 138120
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > linetime:
> > > > > 119, ips linetime: 0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > num_scalers:
> > > > > 2, scaler_users: 0x0, scaler_id: -1
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] pch
> > > > > pfit:
> > > > > pos: 0x00000000, size: 0x00000000, disabled, force thru: no
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] ips: 0,
> > > > > double wide: 0
> > > > > [drm:icl_dump_hw_state [i915]] dpll_hw_state: cfgcr0:
> > > > > 0x1c001a5,
> > > > > cfgcr1: 0x8b, mg_refclkin_ctl: 0x0, hg_clktop2_coreclkctl1:
> > > > > 0x0,
> > > > > mg_clktop2_hsclkctl: 0x0, mg_pll_div0: 0x0, mg_pll_div2: 0x0,
> > > > > mg_pll_lf: 0x0, mg_pll_frac_lock: 0x0, mg_pll_ssc: 0x0,
> > > > > mg_pll_bias: 0x0, mg_pll_tdc_coldst_bias: 0x0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > csc_mode:
> > > > > 0x0 gamma_mode: 0x0 gamma_enable: 0 csc_enable: 0
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] MST
> > > > > master
> > > > > transcoder: <invalid>
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]]
> > > > > [PLANE:31:plane 1A] fb: [FB:262] 1920x1080 format = XR24
> > > > > little-
> > > > > endian (0x34325258), visible: yes
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	rotatio
> > > > > n: 0x1, scaler: -1
> > > > > i915 0000:00:02.0: [drm:intel_dump_pipe_config [i915]] 	src:
> > > > > 1920.000000x1080.000000+0.000000+0.000000 dst: 1920x1080+0+0
> > > > > i915 0000:00:02.0: [drm:intel_psr_disable_locked [i915]]
> > > > > Disabling
> > > > > PSR1
> > > > > i915 0000:00:02.0: [drm:intel_ddi_update_pipe [i915]] Panel
> > > > > doesn't
> > > > > support DRRS
> > > > > ------------[ cut here ]------------
> > > > > i915 0000:00:02.0: drm_WARN_ON(fbc->active)
> > > > > WARNING: CPU: 4 PID: 1175 at
> > > > > drivers/gpu/drm/i915/display/intel_fbc.c:973
> > > > > __intel_fbc_disable+0xa5/0x130 [i915]
> > > > > Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek
> > > > > snd_hda_codec_generic i915 mei_hdcp x86_pkg_temp_thermal
> > > > > coretemp
> > > > > crct10dif_pclmul snd_hda_intel crc32_pclmul snd_intel_dspcfg
> > > > > snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core
> > > > > cdc_ether
> > > > > e1000e usbnet mii snd_pcm ptp mei_me pps_core mei thunderbolt
> > > > > intel_lpss_pci prime_numbers
> > > > > CPU: 4 PID: 1175 Comm: kms_frontbuffer Tainted:
> > > > > G     U            5.5.0-CI-Trybot_5651+ #1
> > > > > Hardware name: Intel Corporation Ice Lake Client
> > > > > Platform/IceLake U
> > > > > DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750
> > > > > 06/14/2019
> > > > > RIP: 0010:__intel_fbc_disable+0xa5/0x130 [i915]
> > > > > Code: 8b 67 50 4d 85 e4 0f 84 8f 00 00 00 e8 44 33 30 e1 48 c7
> > > > > c1
> > > > > 72 f6 4c a0 4c 89 e2 48 89 c6 48 c7 c7 42 f6 4c a0 e8 0b 9d ce
> > > > > e0
> > > > > <0f> 0b eb 90 48 8b 7b 18 4c 8b 67 50 4d 85 e4 74 6d e8 15 33
> > > > > 30 e1
> > > > > RSP: 0018:ffffc90000613b68 EFLAGS: 00010282
> > > > > RAX: 0000000000000000 RBX: ffff8884799d0000 RCX:
> > > > > 0000000000000006
> > > > > RDX: 0000000000001905 RSI: ffff888495dac970 RDI:
> > > > > ffffffff823731a1
> > > > > RBP: ffff88847c05d000 R08: ffff888495dac970 R09:
> > > > > 0000000000000000
> > > > > R10: ffffc90000613b88 R11: 0000000000000000 R12:
> > > > > ffff88849bba7e40
> > > > > R13: ffff8884799d0000 R14: ffff888498564000 R15:
> > > > > 0000000000000000
> > > > > FS:  00007f8157f08300(0000) GS:ffff8884a0000000(0000)
> > > > > knlGS:0000000000000000
> > > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: 00007ffdbfea2eb8 CR3: 000000049d1cc001 CR4:
> > > > > 0000000000760ee0
> > > > > PKRU: 55555554
> > > > > Call Trace:
> > > > >  intel_fbc_disable+0x4a/0x50 [i915]
> > > > >  intel_update_crtc+0x12c/0x1d0 [i915]
> > > > >  skl_commit_modeset_enables+0x14d/0x600 [i915]
> > > > >  intel_atomic_commit_tail+0x30d/0x1480 [i915]
> > > > >  ? queue_work_on+0x31/0x70
> > > > >  ? intel_atomic_commit_ready+0x3f/0x48 [i915]
> > > > >  ? __i915_sw_fence_complete+0x1a0/0x250 [i915]
> > > > >  intel_atomic_commit+0x312/0x390 [i915]
> > > > >  intel_psr_fastset_force+0x119/0x150 [i915]
> > > > >  i915_edp_psr_debug_set+0x53/0x70 [i915]
> > > > >  simple_attr_write+0xb0/0xd0
> > > > >  full_proxy_write+0x51/0x80
> > > > >  vfs_write+0xb9/0x1d0
> > > > >  ksys_write+0x9f/0xe0
> > > > >  do_syscall_64+0x4f/0x220
> > > > >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > > > RIP: 0033:0x7f8157240281
> > > > > Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f
> > > > > 84
> > > > > 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f
> > > > > 05
> > > > > <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89
> > > > > d4 53
> > > > > RSP: 002b:00007ffdbfea59d8 EFLAGS: 00000246 ORIG_RAX:
> > > > > 0000000000000001
> > > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
> > > > > 00007f8157240281
> > > > > RDX: 0000000000000003 RSI: 00007f8157901152 RDI:
> > > > > 0000000000000008
> > > > > RBP: 0000000000000003 R08: 0000000000000000 R09:
> > > > > 0000000000000000
> > > > > R10: 0000000000000000 R11: 0000000000000246 R12:
> > > > > 00007f8157901152
> > > > > R13: 0000000000000008 R14: 00005589d298dce0 R15:
> > > > > 0000000000000000
> > > > > irq event stamp: 55208
> > > > > hardirqs last  enabled at (55207): [<ffffffff8112f3fc>]
> > > > > vprintk_emit+0xcc/0x330
> > > > > hardirqs last disabled at (55208): [<ffffffff81001ca0>]
> > > > > trace_hardirqs_off_thunk+0x1a/0x1c
> > > > > softirqs last  enabled at (54926): [<ffffffff81e00385>]
> > > > > __do_softirq+0x385/0x47f
> > > > > softirqs last disabled at (54915): [<ffffffff810ba15a>]
> > > > > irq_exit+0xba/0xc0
> > > > > ---[ end trace afa50c52e5a512bb ]---
> > > > > [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A
> > > > > i915 0000:00:02.0: [drm:verify_connector_state [i915]]
> > > > > [CONNECTOR:215:eDP-1]
> > > > > i915 0000:00:02.0: [drm:intel_atomic_commit_tail [i915]]
> > > > > [CRTC:91:pipe A]
> > > > > [drm:intel_ddi_get_config [i915]] [ENCODER:214:DDI A] Fec
> > > > > status: 0
> > > > > i915 0000:00:02.0: [drm:verify_single_dpll_state.isra.150
> > > > > [i915]]
> > > > > DPLL 0
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > index ddf8d3bb7a7d..a6038fe74f63 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > @@ -856,6 +856,9 @@ static bool intel_fbc_can_flip_nuke(const
> > > > > struct intel_crtc_state *crtc_state)
> > > > >  	if (drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> > > > >  		return false;
> > > > >  
> > > > > +	if (!crtc_state->enable_fbc)
> > > > > +		return false;
> > > > 
> > > > The code is still quite the horror show, but from the looks of
> > > > things
> > > > we'd want this check in intel_fbc_can_activate() instead.
> > > 
> > > From intel_fbc_flush() we don't have the intel_crtc_state, so move
> > > it
> > > to intel_fbc_can_activate() and cache the enable_fbc in
> > > intel_fbc_update_state_cache() too?
> > 
> > Oh, I failed to notice you're looking at the crtc state here. Thought
> > you just check the modparam directly. I'd just call
> > intel_fbc_can_enable()
> > in intel_fbc_can_activate() directly. That crtc_state->enable_fbc
> > thing
> > is just hopelessly broken atm.
> 
> crtc_state->enable_fbc=true is backed by intel_fbc_can_enable() and
> later we have:
> intel_update_crtc()
> 	...
> 	if (new_crtc_state->update_pipe && !new_crtc_state->enable_fbc)
> 		intel_fbc_disable(crtc);
> 
> My felling it that would be more consistent keep using it but let me
> know what you think is best.

crtc_state->enable_fbc is just broken. It can more or less randombly get
set for any FBC capable crtc and after that it remains set whether or
not the crtc can keep doing FBC. The whole FBC state handling needs
a bit of a redesign.

> 
> > 
> > > > With the check relocated this is
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > PS. Oh dear, we still have that crtc_state->enable_fbc mess in
> > > > there.
> > > > I should have patches to eliminate that tucked away in a branch
> > > > somewhere...
> > > > 
> > > > > +
> > > > >  	if (!params->plane_visible)
> > > > >  		return false;
> > > > >  
> > > > > -- 
> > > > > 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Souza, Jose]

On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote:
> On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> > wrote:
> > > dGFX have local memory so it do not have aperture and do not
> > > support
> > > CPU fences but even for iGFX it have a small number of fences.
> > > 
> > > As replacement for fences to track frontbuffer modifications by
> > > CPU
> > > we have a software tracking that is already in used by FBC and
> > > PSR.
> > > PSR don't support fences so it shows that this tracking is
> > > reliable.
> > > 
> > > So lets make fences a nice-to-have to activate FBC for GEN9+,
> > > this
> > > will allow us to enable FBC for dGFXs and iGFXs even when there
> > > is
> > > no
> > > available fence.
> > > 
> > > We do not set fences to rotated planes but FBC only have
> > > restrictions
> > > against 16bpp, so adding it here.
> > > 
> > > Also adding a new check for the tiling format, fences are only
> > > set
> > > to X and Y tiled planes but again FBC don't have any restrictions
> > > against tiling so adding linear as supported as well, other
> > > formats
> > > should be added after tested but IGT only supports drawing in
> > > thse
> > > 3 formats.
> > > 
> > > intel_fbc_hw_tracking_covers_screen() maybe can also have the
> > > same
> > > treatment as fences but BSpec is not clear if the size limitation
> > > is
> > > for hardware tracking or general use of FBC and I don't have a 5K
> > > display to test it, so keeping as is for safety.
> > > 
> > > v2:
> > > - Added tiling and pixel format rotation checks
> > > - Changed the GEN version not requiring fences to 11 from 9, DDX
> > > needs some changes but it don't have support for GEN11+
> > > 
> > > v3:
> > > - Changed back to GEN9+
> > > - Moved GEN test to inside of tiling_is_valid()
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > > ++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > >  2 files changed, 39 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 1d76e3646a25..a0d1d661a006 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > > drm_i915_private *dev_priv,
> > >  }
> > >  
> > >  static bool pixel_format_is_valid(struct drm_i915_private
> > > *dev_priv,
> > > -				  u32 pixel_format)
> > > +				  u32 pixel_format, unsigned int
> > > rotation)
> > >  {
> > >  	switch (pixel_format) {
> > >  	case DRM_FORMAT_XRGB8888:
> > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > > drm_i915_private *dev_priv,
> > >  		/* WaFbcOnly1to1Ratio:ctg */
> > >  		if (IS_G4X(dev_priv))
> > >  			return false;
> > > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > > DRM_MODE_ROTATE_270)) &&
> > > +		    INTEL_GEN(dev_priv) >= 9)
> > > +			return false;
> > 
> > Would still would prefer a rotations_is_valid() or some such thing.
> 
> Like this?
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 5e35c894bdf9..692edd45b769 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
> drm_i915_private *dev_priv,
>                 /* WaFbcOnly1to1Ratio:ctg */
>                 if (IS_G4X(dev_priv))
>                         return false;
> -               if ((rotation & (DRM_MODE_ROTATE_90 |
> DRM_MODE_ROTATE_270)) &&
> -                   INTEL_GEN(dev_priv) >= 9)
> -                       return false;
>                 return true;
>         default:
>                 return false;
>         }
>  }
> 
> +static bool rotations_is_valid(struct drm_i915_private *dev_priv,
> +                              u32 pixel_format, unsigned int
> rotation)
> +{
> +       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
> DRM_FORMAT_RGB565 &&
> +           rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270))
> +               return false;
> +       return true;
> +}
> +
>  /*
>   * For some reason, the hardware tracking starts looking at whatever
> we
>   * programmed as the display plane base address register. It does
> not
> look at
> @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
> intel_crtc *crtc)
>                 return false;
>         }
> 
> +       if (!rotations_is_valid(dev_priv, cache->fb.format->format,
> +                               cache->plane.rotation)) {
> +               fbc->no_fbc_reason = "plane rotation is invalid";
> +               return false;
> +       }
> +
>         if (cache->plane.pixel_blend_mode !=
> DRM_MODE_BLEND_PIXEL_NONE
> &&
>             cache->fb.format->has_alpha) {
>                 fbc->no_fbc_reason = "per-pixel alpha blending is
> incompatible with FBC";
> 
> 
> > >  		return true;
> > >  	default:
> > >  		return false;
> > > @@ -639,6 +642,22 @@ static bool
> > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > >  	return effective_w <= max_w && effective_h <= max_h;
> > >  }
> > >  
> > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> > > +			    uint64_t modifier)
> > > +{
> > > +	switch (modifier) {
> > > +	case DRM_FORMAT_MOD_LINEAR:
> > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > +			return true;
> > 
> > Have we checked that eg. fbcon cursor still blinks correctly
> > with FBC active and all?
> 
> Hum on fbcon FBC is enabled but it never compress, IGT tests with
> fences complete disables are working fine, screen is updated when
> tests
> asks to with FBC enabled and compressing.
> 
> I will debug fbcon a little more to understand why it is never
> compressing. 

Actually is is compressing but is very rare, I even reduced the blink
rate to 1hz but it did not helped.

I have compared the plane registers
between fbcon and IGT both in tiling linear but the only difference is
the plane surface address:

# fbcon
[drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
[drm:skl_update_plane [i915]] 	plane_stride=78
[drm:skl_update_plane [i915]] 	plane_pos=0
[drm:skl_update_plane [i915]] 	plane_size=437077f
[drm:skl_update_plane [i915]] 	plane_aux_dist=0
[drm:skl_update_plane [i915]] 	plane_color_ctl=2000
[drm:skl_update_plane [i915]] 	plane_keyval=0
[drm:skl_update_plane [i915]] 	plane_keymsk=0
[drm:skl_update_plane [i915]] 	plane_keymax=ff000000
[drm:skl_update_plane [i915]] 	plane_offset=0
[drm:skl_update_plane [i915]] 	plane_ctl=84000000
[drm:skl_update_plane [i915]] 	plane_surf=40000

# IGT test with tiling linear framebuffer
[drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
[drm:skl_update_plane [i915]] 	plane_stride=78
[drm:skl_update_plane [i915]] 	plane_pos=0
[drm:skl_update_plane [i915]] 	plane_size=437077f
[drm:skl_update_plane [i915]] 	plane_aux_dist=0
[drm:skl_update_plane [i915]] 	plane_color_ctl=2000
[drm:skl_update_plane [i915]] 	plane_keyval=0
[drm:skl_update_plane [i915]] 	plane_keymsk=0
[drm:skl_update_plane [i915]] 	plane_keymax=ff000000
[drm:skl_update_plane [i915]] 	plane_offset=0
[drm:skl_update_plane [i915]] 	plane_ctl=84000000
[drm:skl_update_plane [i915]] 	plane_surf=1040000

Do you have any tips of what could be causing this?

> 
> > > +		return false;
> > > +	case I915_FORMAT_MOD_X_TILED:
> > > +	case I915_FORMAT_MOD_Y_TILED:
> > > +		return true;
> > > +	default:
> > > +		return false;
> > > +	}
> > > +}
> > > +
> > >  static void intel_fbc_update_state_cache(struct intel_crtc
> > > *crtc,
> > >  					 const struct intel_crtc_state
> > > *crtc_state,
> > >  					 const struct intel_plane_state
> > > *plane_state)
> > > @@ -672,6 +691,7 @@ static void
> > > intel_fbc_update_state_cache(struct
> > > intel_crtc *crtc,
> > >  
> > >  	cache->fb.format = fb->format;
> > >  	cache->fb.stride = fb->pitches[0];
> > > +	cache->fb.modifier = fb->modifier;
> > >  
> > >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > > PLANE_HAS_FENCE &&
> > >  		    !plane_state->vma->fence);
> > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct
> > > intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > -	/* The use of a CPU fence is mandatory in order to detect
> > > writes
> > > -	 * by the CPU to the scanout and trigger updates to the FBC.
> > > +	/* The use of a CPU fence is one of two ways to detect writes
> > > by the
> > > +	 * CPU to the scanout and trigger updates to the FBC.
> > > +	 *
> > > +	 * The other method is by software tracking(see
> > > +	 * intel_fbc_invalidate/flush()), it will manually notify FBC
> > > and nuke
> > > +	 * the current compressed buffer and recompress it.
> > >  	 *
> > >  	 * Note that is possible for a tiled surface to be unmappable
> > > (and
> > > -	 * so have no fence associated with it) due to aperture
> > > constaints
> > > +	 * so have no fence associated with it) due to aperture
> > > constraints
> > >  	 * at the time of pinning.
> > >  	 *
> > >  	 * FIXME with 90/270 degree rotation we should use the fence on
> > >  	 * the normal GTT view (the rotated view doesn't even have a
> > >  	 * fence). Would need changes to the FBC fence Y offset as
> > > well.
> > > -	 * For now this will effecively disable FBC with 90/270 degree
> > > +	 * For now this will effectively disable FBC with 90/270 degree
> > >  	 * rotation.
> > >  	 */
> > > -	if (cache->fence_id < 0) {
> > > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> > >  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> > >  		return false;
> > >  	}
> > > +
> > > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > > +		fbc->no_fbc_reason = "tiling unsupported";
> > > +		return false;
> > > +	}
> > > +
> > >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> > >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > > intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > >format)) 
> > > {
> > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> > > +				   cache->plane.rotation)) {
> > >  		fbc->no_fbc_reason = "pixel format is invalid";
> > >  		return false;
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 3330b538d379..bf88663d8217 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -413,6 +413,7 @@ struct intel_fbc {
> > >  		struct {
> > >  			const struct drm_format_info *format;
> > >  			unsigned int stride;
> > > +			u64 modifier;
> > >  		} fb;
> > >  		u16 gen9_wa_cfb_stride;
> > >  		s8 fence_id;
> > > -- 
> > > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Ville Syrjälä]

On Thu, Mar 12, 2020 at 12:35:56AM +0000, Souza, Jose wrote:
> On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote:
> > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> > > wrote:
> > > > dGFX have local memory so it do not have aperture and do not
> > > > support
> > > > CPU fences but even for iGFX it have a small number of fences.
> > > > 
> > > > As replacement for fences to track frontbuffer modifications by
> > > > CPU
> > > > we have a software tracking that is already in used by FBC and
> > > > PSR.
> > > > PSR don't support fences so it shows that this tracking is
> > > > reliable.
> > > > 
> > > > So lets make fences a nice-to-have to activate FBC for GEN9+,
> > > > this
> > > > will allow us to enable FBC for dGFXs and iGFXs even when there
> > > > is
> > > > no
> > > > available fence.
> > > > 
> > > > We do not set fences to rotated planes but FBC only have
> > > > restrictions
> > > > against 16bpp, so adding it here.
> > > > 
> > > > Also adding a new check for the tiling format, fences are only
> > > > set
> > > > to X and Y tiled planes but again FBC don't have any restrictions
> > > > against tiling so adding linear as supported as well, other
> > > > formats
> > > > should be added after tested but IGT only supports drawing in
> > > > thse
> > > > 3 formats.
> > > > 
> > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the
> > > > same
> > > > treatment as fences but BSpec is not clear if the size limitation
> > > > is
> > > > for hardware tracking or general use of FBC and I don't have a 5K
> > > > display to test it, so keeping as is for safety.
> > > > 
> > > > v2:
> > > > - Added tiling and pixel format rotation checks
> > > > - Changed the GEN version not requiring fences to 11 from 9, DDX
> > > > needs some changes but it don't have support for GEN11+
> > > > 
> > > > v3:
> > > > - Changed back to GEN9+
> > > > - Moved GEN test to inside of tiling_is_valid()
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > > > ++++++++++++++++++++----
> > > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > > >  2 files changed, 39 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 1d76e3646a25..a0d1d661a006 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > > > drm_i915_private *dev_priv,
> > > >  }
> > > >  
> > > >  static bool pixel_format_is_valid(struct drm_i915_private
> > > > *dev_priv,
> > > > -				  u32 pixel_format)
> > > > +				  u32 pixel_format, unsigned int
> > > > rotation)
> > > >  {
> > > >  	switch (pixel_format) {
> > > >  	case DRM_FORMAT_XRGB8888:
> > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > > > drm_i915_private *dev_priv,
> > > >  		/* WaFbcOnly1to1Ratio:ctg */
> > > >  		if (IS_G4X(dev_priv))
> > > >  			return false;
> > > > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > > > DRM_MODE_ROTATE_270)) &&
> > > > +		    INTEL_GEN(dev_priv) >= 9)
> > > > +			return false;
> > > 
> > > Would still would prefer a rotations_is_valid() or some such thing.
> > 
> > Like this?
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 5e35c894bdf9..692edd45b769 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
> > drm_i915_private *dev_priv,
> >                 /* WaFbcOnly1to1Ratio:ctg */
> >                 if (IS_G4X(dev_priv))
> >                         return false;
> > -               if ((rotation & (DRM_MODE_ROTATE_90 |
> > DRM_MODE_ROTATE_270)) &&
> > -                   INTEL_GEN(dev_priv) >= 9)
> > -                       return false;
> >                 return true;
> >         default:
> >                 return false;
> >         }
> >  }
> > 
> > +static bool rotations_is_valid(struct drm_i915_private *dev_priv,
> > +                              u32 pixel_format, unsigned int
> > rotation)
> > +{
> > +       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
> > DRM_FORMAT_RGB565 &&
> > +           rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270))
> > +               return false;
> > +       return true;
> > +}
> > +
> >  /*
> >   * For some reason, the hardware tracking starts looking at whatever
> > we
> >   * programmed as the display plane base address register. It does
> > not
> > look at
> > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >                 return false;
> >         }
> > 
> > +       if (!rotations_is_valid(dev_priv, cache->fb.format->format,
> > +                               cache->plane.rotation)) {
> > +               fbc->no_fbc_reason = "plane rotation is invalid";
> > +               return false;
> > +       }
> > +
> >         if (cache->plane.pixel_blend_mode !=
> > DRM_MODE_BLEND_PIXEL_NONE
> > &&
> >             cache->fb.format->has_alpha) {
> >                 fbc->no_fbc_reason = "per-pixel alpha blending is
> > incompatible with FBC";
> > 
> > 
> > > >  		return true;
> > > >  	default:
> > > >  		return false;
> > > > @@ -639,6 +642,22 @@ static bool
> > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > > >  	return effective_w <= max_w && effective_h <= max_h;
> > > >  }
> > > >  
> > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> > > > +			    uint64_t modifier)
> > > > +{
> > > > +	switch (modifier) {
> > > > +	case DRM_FORMAT_MOD_LINEAR:
> > > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > > +			return true;
> > > 
> > > Have we checked that eg. fbcon cursor still blinks correctly
> > > with FBC active and all?
> > 
> > Hum on fbcon FBC is enabled but it never compress, IGT tests with
> > fences complete disables are working fine, screen is updated when
> > tests
> > asks to with FBC enabled and compressing.
> > 
> > I will debug fbcon a little more to understand why it is never
> > compressing. 
> 
> Actually is is compressing but is very rare, I even reduced the blink
> rate to 1hz but it did not helped.

Is FBC actually enabled all the time though? By "compressing" I take
you mean the debugfs thing we have? I don't actually remember what
that looks at. Number of compressed segments? Or something else?
You're polling it fast enough to not simply suffer from poor sampling?

> 
> I have compared the plane registers
> between fbcon and IGT both in tiling linear but the only difference is
> the plane surface address:
> 
> # fbcon
> [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
> [drm:skl_update_plane [i915]] 	plane_stride=78
> [drm:skl_update_plane [i915]] 	plane_pos=0
> [drm:skl_update_plane [i915]] 	plane_size=437077f
> [drm:skl_update_plane [i915]] 	plane_aux_dist=0
> [drm:skl_update_plane [i915]] 	plane_color_ctl=2000
> [drm:skl_update_plane [i915]] 	plane_keyval=0
> [drm:skl_update_plane [i915]] 	plane_keymsk=0
> [drm:skl_update_plane [i915]] 	plane_keymax=ff000000
> [drm:skl_update_plane [i915]] 	plane_offset=0
> [drm:skl_update_plane [i915]] 	plane_ctl=84000000
> [drm:skl_update_plane [i915]] 	plane_surf=40000
> 
> # IGT test with tiling linear framebuffer
> [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
> [drm:skl_update_plane [i915]] 	plane_stride=78
> [drm:skl_update_plane [i915]] 	plane_pos=0
> [drm:skl_update_plane [i915]] 	plane_size=437077f
> [drm:skl_update_plane [i915]] 	plane_aux_dist=0
> [drm:skl_update_plane [i915]] 	plane_color_ctl=2000
> [drm:skl_update_plane [i915]] 	plane_keyval=0
> [drm:skl_update_plane [i915]] 	plane_keymsk=0
> [drm:skl_update_plane [i915]] 	plane_keymax=ff000000
> [drm:skl_update_plane [i915]] 	plane_offset=0
> [drm:skl_update_plane [i915]] 	plane_ctl=84000000
> [drm:skl_update_plane [i915]] 	plane_surf=1040000
> 
> Do you have any tips of what could be causing this?
> 
> > 
> > > > +		return false;
> > > > +	case I915_FORMAT_MOD_X_TILED:
> > > > +	case I915_FORMAT_MOD_Y_TILED:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > > +
> > > >  static void intel_fbc_update_state_cache(struct intel_crtc
> > > > *crtc,
> > > >  					 const struct intel_crtc_state
> > > > *crtc_state,
> > > >  					 const struct intel_plane_state
> > > > *plane_state)
> > > > @@ -672,6 +691,7 @@ static void
> > > > intel_fbc_update_state_cache(struct
> > > > intel_crtc *crtc,
> > > >  
> > > >  	cache->fb.format = fb->format;
> > > >  	cache->fb.stride = fb->pitches[0];
> > > > +	cache->fb.modifier = fb->modifier;
> > > >  
> > > >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > > > PLANE_HAS_FENCE &&
> > > >  		    !plane_state->vma->fence);
> > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct
> > > > intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	/* The use of a CPU fence is mandatory in order to detect
> > > > writes
> > > > -	 * by the CPU to the scanout and trigger updates to the FBC.
> > > > +	/* The use of a CPU fence is one of two ways to detect writes
> > > > by the
> > > > +	 * CPU to the scanout and trigger updates to the FBC.
> > > > +	 *
> > > > +	 * The other method is by software tracking(see
> > > > +	 * intel_fbc_invalidate/flush()), it will manually notify FBC
> > > > and nuke
> > > > +	 * the current compressed buffer and recompress it.
> > > >  	 *
> > > >  	 * Note that is possible for a tiled surface to be unmappable
> > > > (and
> > > > -	 * so have no fence associated with it) due to aperture
> > > > constaints
> > > > +	 * so have no fence associated with it) due to aperture
> > > > constraints
> > > >  	 * at the time of pinning.
> > > >  	 *
> > > >  	 * FIXME with 90/270 degree rotation we should use the fence on
> > > >  	 * the normal GTT view (the rotated view doesn't even have a
> > > >  	 * fence). Would need changes to the FBC fence Y offset as
> > > > well.
> > > > -	 * For now this will effecively disable FBC with 90/270 degree
> > > > +	 * For now this will effectively disable FBC with 90/270 degree
> > > >  	 * rotation.
> > > >  	 */
> > > > -	if (cache->fence_id < 0) {
> > > > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> > > >  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> > > >  		return false;
> > > >  	}
> > > > +
> > > > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > > > +		fbc->no_fbc_reason = "tiling unsupported";
> > > > +		return false;
> > > > +	}
> > > > +
> > > >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> > > >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> > > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > > > intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > > >format)) 
> > > > {
> > > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> > > > +				   cache->plane.rotation)) {
> > > >  		fbc->no_fbc_reason = "pixel format is invalid";
> > > >  		return false;
> > > >  	}
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 3330b538d379..bf88663d8217 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -413,6 +413,7 @@ struct intel_fbc {
> > > >  		struct {
> > > >  			const struct drm_format_info *format;
> > > >  			unsigned int stride;
> > > > +			u64 modifier;
> > > >  		} fb;
> > > >  		u16 gen9_wa_cfb_stride;
> > > >  		s8 fence_id;
> > > > -- 
> > > > 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Ville Syrjälä]

On Fri, Mar 06, 2020 at 01:32:12AM +0000, Souza, Jose wrote:
> On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> > wrote:
> > > dGFX have local memory so it do not have aperture and do not
> > > support
> > > CPU fences but even for iGFX it have a small number of fences.
> > > 
> > > As replacement for fences to track frontbuffer modifications by CPU
> > > we have a software tracking that is already in used by FBC and PSR.
> > > PSR don't support fences so it shows that this tracking is
> > > reliable.
> > > 
> > > So lets make fences a nice-to-have to activate FBC for GEN9+, this
> > > will allow us to enable FBC for dGFXs and iGFXs even when there is
> > > no
> > > available fence.
> > > 
> > > We do not set fences to rotated planes but FBC only have
> > > restrictions
> > > against 16bpp, so adding it here.
> > > 
> > > Also adding a new check for the tiling format, fences are only set
> > > to X and Y tiled planes but again FBC don't have any restrictions
> > > against tiling so adding linear as supported as well, other formats
> > > should be added after tested but IGT only supports drawing in thse
> > > 3 formats.
> > > 
> > > intel_fbc_hw_tracking_covers_screen() maybe can also have the same
> > > treatment as fences but BSpec is not clear if the size limitation
> > > is
> > > for hardware tracking or general use of FBC and I don't have a 5K
> > > display to test it, so keeping as is for safety.
> > > 
> > > v2:
> > > - Added tiling and pixel format rotation checks
> > > - Changed the GEN version not requiring fences to 11 from 9, DDX
> > > needs some changes but it don't have support for GEN11+
> > > 
> > > v3:
> > > - Changed back to GEN9+
> > > - Moved GEN test to inside of tiling_is_valid()
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > > ++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > >  2 files changed, 39 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 1d76e3646a25..a0d1d661a006 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > > drm_i915_private *dev_priv,
> > >  }
> > >  
> > >  static bool pixel_format_is_valid(struct drm_i915_private
> > > *dev_priv,
> > > -				  u32 pixel_format)
> > > +				  u32 pixel_format, unsigned int
> > > rotation)
> > >  {
> > >  	switch (pixel_format) {
> > >  	case DRM_FORMAT_XRGB8888:
> > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > > drm_i915_private *dev_priv,
> > >  		/* WaFbcOnly1to1Ratio:ctg */
> > >  		if (IS_G4X(dev_priv))
> > >  			return false;
> > > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > > DRM_MODE_ROTATE_270)) &&
> > > +		    INTEL_GEN(dev_priv) >= 9)
> > > +			return false;
> > 
> > Would still would prefer a rotations_is_valid() or some such thing.
> 
> Like this?
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 5e35c894bdf9..692edd45b769 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
> drm_i915_private *dev_priv,
>                 /* WaFbcOnly1to1Ratio:ctg */
>                 if (IS_G4X(dev_priv))
>                         return false;
> -               if ((rotation & (DRM_MODE_ROTATE_90 |
> DRM_MODE_ROTATE_270)) &&
> -                   INTEL_GEN(dev_priv) >= 9)
> -                       return false;
>                 return true;
>         default:
>                 return false;
>         }
>  }
> 
> +static bool rotations_is_valid(struct drm_i915_private *dev_priv,
> +                              u32 pixel_format, unsigned int rotation)
> +{
> +       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
> DRM_FORMAT_RGB565 &&
> +           rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270))

drm_rotation_90_or_270()

> +               return false;
> +       return true;
> +}
> +
>  /*
>   * For some reason, the hardware tracking starts looking at whatever
> we
>   * programmed as the display plane base address register. It does not
> look at
> @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
> intel_crtc *crtc)
>                 return false;
>         }
> 
> +       if (!rotations_is_valid(dev_priv, cache->fb.format->format,
> +                               cache->plane.rotation)) {
> +               fbc->no_fbc_reason = "plane rotation is invalid";
> +               return false;
> +       }

s/rotations/rotation/ + I'd move the pre-g4x rotation check into
that function as well. With that

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>         if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE
> &&
>             cache->fb.format->has_alpha) {
>                 fbc->no_fbc_reason = "per-pixel alpha blending is
> incompatible with FBC";
> 
> 
> > 
> > >  		return true;
> > >  	default:
> > >  		return false;
> > > @@ -639,6 +642,22 @@ static bool
> > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > >  	return effective_w <= max_w && effective_h <= max_h;
> > >  }
> > >  
> > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> > > +			    uint64_t modifier)
> > > +{
> > > +	switch (modifier) {
> > > +	case DRM_FORMAT_MOD_LINEAR:
> > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > +			return true;
> > 
> > Have we checked that eg. fbcon cursor still blinks correctly
> > with FBC active and all?
> 
> Hum on fbcon FBC is enabled but it never compress, IGT tests with
> fences complete disables are working fine, screen is updated when tests
> asks to with FBC enabled and compressing.
> 
> I will debug fbcon a little more to understand why it is never
> compressing. 
> 
> > 
> > > +		return false;
> > > +	case I915_FORMAT_MOD_X_TILED:
> > > +	case I915_FORMAT_MOD_Y_TILED:
> > > +		return true;
> > > +	default:
> > > +		return false;
> > > +	}
> > > +}
> > > +
> > >  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> > >  					 const struct intel_crtc_state
> > > *crtc_state,
> > >  					 const struct intel_plane_state
> > > *plane_state)
> > > @@ -672,6 +691,7 @@ static void intel_fbc_update_state_cache(struct
> > > intel_crtc *crtc,
> > >  
> > >  	cache->fb.format = fb->format;
> > >  	cache->fb.stride = fb->pitches[0];
> > > +	cache->fb.modifier = fb->modifier;
> > >  
> > >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > > PLANE_HAS_FENCE &&
> > >  		    !plane_state->vma->fence);
> > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct
> > > intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > -	/* The use of a CPU fence is mandatory in order to detect
> > > writes
> > > -	 * by the CPU to the scanout and trigger updates to the FBC.
> > > +	/* The use of a CPU fence is one of two ways to detect writes
> > > by the
> > > +	 * CPU to the scanout and trigger updates to the FBC.
> > > +	 *
> > > +	 * The other method is by software tracking(see
> > > +	 * intel_fbc_invalidate/flush()), it will manually notify FBC
> > > and nuke
> > > +	 * the current compressed buffer and recompress it.
> > >  	 *
> > >  	 * Note that is possible for a tiled surface to be unmappable
> > > (and
> > > -	 * so have no fence associated with it) due to aperture
> > > constaints
> > > +	 * so have no fence associated with it) due to aperture
> > > constraints
> > >  	 * at the time of pinning.
> > >  	 *
> > >  	 * FIXME with 90/270 degree rotation we should use the fence on
> > >  	 * the normal GTT view (the rotated view doesn't even have a
> > >  	 * fence). Would need changes to the FBC fence Y offset as
> > > well.
> > > -	 * For now this will effecively disable FBC with 90/270 degree
> > > +	 * For now this will effectively disable FBC with 90/270 degree
> > >  	 * rotation.
> > >  	 */
> > > -	if (cache->fence_id < 0) {
> > > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> > >  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> > >  		return false;
> > >  	}
> > > +
> > > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > > +		fbc->no_fbc_reason = "tiling unsupported";
> > > +		return false;
> > > +	}
> > > +
> > >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> > >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > > intel_crtc *crtc)
> > >  		return false;
> > >  	}
> > >  
> > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) 
> > > {
> > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> > > +				   cache->plane.rotation)) {
> > >  		fbc->no_fbc_reason = "pixel format is invalid";
> > >  		return false;
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 3330b538d379..bf88663d8217 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -413,6 +413,7 @@ struct intel_fbc {
> > >  		struct {
> > >  			const struct drm_format_info *format;
> > >  			unsigned int stride;
> > > +			u64 modifier;
> > >  		} fb;
> > >  		u16 gen9_wa_cfb_stride;
> > >  		s8 fence_id;
> > > -- 
> > > 2.25.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Souza, Jose]

On Wed, 2020-03-18 at 14:44 +0200, Ville Syrjälä wrote:
> On Thu, Mar 12, 2020 at 12:35:56AM +0000, Souza, Jose wrote:
> > On Thu, 2020-03-05 at 17:33 -0800, José Roberto de Souza wrote:
> > > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > dGFX have local memory so it do not have aperture and do not
> > > > > support
> > > > > CPU fences but even for iGFX it have a small number of
> > > > > fences.
> > > > > 
> > > > > As replacement for fences to track frontbuffer modifications
> > > > > by
> > > > > CPU
> > > > > we have a software tracking that is already in used by FBC
> > > > > and
> > > > > PSR.
> > > > > PSR don't support fences so it shows that this tracking is
> > > > > reliable.
> > > > > 
> > > > > So lets make fences a nice-to-have to activate FBC for GEN9+,
> > > > > this
> > > > > will allow us to enable FBC for dGFXs and iGFXs even when
> > > > > there
> > > > > is
> > > > > no
> > > > > available fence.
> > > > > 
> > > > > We do not set fences to rotated planes but FBC only have
> > > > > restrictions
> > > > > against 16bpp, so adding it here.
> > > > > 
> > > > > Also adding a new check for the tiling format, fences are
> > > > > only
> > > > > set
> > > > > to X and Y tiled planes but again FBC don't have any
> > > > > restrictions
> > > > > against tiling so adding linear as supported as well, other
> > > > > formats
> > > > > should be added after tested but IGT only supports drawing in
> > > > > thse
> > > > > 3 formats.
> > > > > 
> > > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the
> > > > > same
> > > > > treatment as fences but BSpec is not clear if the size
> > > > > limitation
> > > > > is
> > > > > for hardware tracking or general use of FBC and I don't have
> > > > > a 5K
> > > > > display to test it, so keeping as is for safety.
> > > > > 
> > > > > v2:
> > > > > - Added tiling and pixel format rotation checks
> > > > > - Changed the GEN version not requiring fences to 11 from 9,
> > > > > DDX
> > > > > needs some changes but it don't have support for GEN11+
> > > > > 
> > > > > v3:
> > > > > - Changed back to GEN9+
> > > > > - Moved GEN test to inside of tiling_is_valid()
> > > > > 
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > > > > ++++++++++++++++++++----
> > > > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > > > >  2 files changed, 39 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > index 1d76e3646a25..a0d1d661a006 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  }
> > > > >  
> > > > >  static bool pixel_format_is_valid(struct drm_i915_private
> > > > > *dev_priv,
> > > > > -				  u32 pixel_format)
> > > > > +				  u32 pixel_format, unsigned
> > > > > int
> > > > > rotation)
> > > > >  {
> > > > >  	switch (pixel_format) {
> > > > >  	case DRM_FORMAT_XRGB8888:
> > > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  		/* WaFbcOnly1to1Ratio:ctg */
> > > > >  		if (IS_G4X(dev_priv))
> > > > >  			return false;
> > > > > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > > > > DRM_MODE_ROTATE_270)) &&
> > > > > +		    INTEL_GEN(dev_priv) >= 9)
> > > > > +			return false;
> > > > 
> > > > Would still would prefer a rotations_is_valid() or some such
> > > > thing.
> > > 
> > > Like this?
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 5e35c894bdf9..692edd45b769 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
> > > drm_i915_private *dev_priv,
> > >                 /* WaFbcOnly1to1Ratio:ctg */
> > >                 if (IS_G4X(dev_priv))
> > >                         return false;
> > > -               if ((rotation & (DRM_MODE_ROTATE_90 |
> > > DRM_MODE_ROTATE_270)) &&
> > > -                   INTEL_GEN(dev_priv) >= 9)
> > > -                       return false;
> > >                 return true;
> > >         default:
> > >                 return false;
> > >         }
> > >  }
> > > 
> > > +static bool rotations_is_valid(struct drm_i915_private
> > > *dev_priv,
> > > +                              u32 pixel_format, unsigned int
> > > rotation)
> > > +{
> > > +       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
> > > DRM_FORMAT_RGB565 &&
> > > +           rotation & (DRM_MODE_ROTATE_90 |
> > > DRM_MODE_ROTATE_270))
> > > +               return false;
> > > +       return true;
> > > +}
> > > +
> > >  /*
> > >   * For some reason, the hardware tracking starts looking at
> > > whatever
> > > we
> > >   * programmed as the display plane base address register. It
> > > does
> > > not
> > > look at
> > > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
> > > intel_crtc *crtc)
> > >                 return false;
> > >         }
> > > 
> > > +       if (!rotations_is_valid(dev_priv, cache->fb.format-
> > > >format,
> > > +                               cache->plane.rotation)) {
> > > +               fbc->no_fbc_reason = "plane rotation is invalid";
> > > +               return false;
> > > +       }
> > > +
> > >         if (cache->plane.pixel_blend_mode !=
> > > DRM_MODE_BLEND_PIXEL_NONE
> > > &&
> > >             cache->fb.format->has_alpha) {
> > >                 fbc->no_fbc_reason = "per-pixel alpha blending is
> > > incompatible with FBC";
> > > 
> > > 
> > > > >  		return true;
> > > > >  	default:
> > > > >  		return false;
> > > > > @@ -639,6 +642,22 @@ static bool
> > > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > > > >  	return effective_w <= max_w && effective_h <= max_h;
> > > > >  }
> > > > >  
> > > > > +static bool tiling_is_valid(struct drm_i915_private
> > > > > *dev_priv,
> > > > > +			    uint64_t modifier)
> > > > > +{
> > > > > +	switch (modifier) {
> > > > > +	case DRM_FORMAT_MOD_LINEAR:
> > > > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > > > +			return true;
> > > > 
> > > > Have we checked that eg. fbcon cursor still blinks correctly
> > > > with FBC active and all?
> > > 
> > > Hum on fbcon FBC is enabled but it never compress, IGT tests with
> > > fences complete disables are working fine, screen is updated when
> > > tests
> > > asks to with FBC enabled and compressing.
> > > 
> > > I will debug fbcon a little more to understand why it is never
> > > compressing. 
> > 
> > Actually is is compressing but is very rare, I even reduced the
> > blink
> > rate to 1hz but it did not helped.
> 
> Is FBC actually enabled all the time though? By "compressing" I take
> you mean the debugfs thing we have? I don't actually remember what
> that looks at. Number of compressed segments? Or something else?

Exacly that.

> You're polling it fast enough to not simply suffer from poor
> sampling?

Hum in other platforms the compressing is more consistent.

> 
> > I have compared the plane registers
> > between fbcon and IGT both in tiling linear but the only difference
> > is
> > the plane surface address:
> > 
> > # fbcon
> > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
> > [drm:skl_update_plane [i915]] 	plane_stride=78
> > [drm:skl_update_plane [i915]] 	plane_pos=0
> > [drm:skl_update_plane [i915]] 	plane_size=437077f
> > [drm:skl_update_plane [i915]] 	plane_aux_dist=0
> > [drm:skl_update_plane [i915]] 	plane_color_ctl=2000
> > [drm:skl_update_plane [i915]] 	plane_keyval=0
> > [drm:skl_update_plane [i915]] 	plane_keymsk=0
> > [drm:skl_update_plane [i915]] 	plane_keymax=ff000000
> > [drm:skl_update_plane [i915]] 	plane_offset=0
> > [drm:skl_update_plane [i915]] 	plane_ctl=84000000
> > [drm:skl_update_plane [i915]] 	plane_surf=40000
> > 
> > # IGT test with tiling linear framebuffer
> > [drm:skl_update_plane [i915]] skl_program_plane() pipeA plane=A
> > [drm:skl_update_plane [i915]] 	plane_stride=78
> > [drm:skl_update_plane [i915]] 	plane_pos=0
> > [drm:skl_update_plane [i915]] 	plane_size=437077f
> > [drm:skl_update_plane [i915]] 	plane_aux_dist=0
> > [drm:skl_update_plane [i915]] 	plane_color_ctl=2000
> > [drm:skl_update_plane [i915]] 	plane_keyval=0
> > [drm:skl_update_plane [i915]] 	plane_keymsk=0
> > [drm:skl_update_plane [i915]] 	plane_keymax=ff000000
> > [drm:skl_update_plane [i915]] 	plane_offset=0
> > [drm:skl_update_plane [i915]] 	plane_ctl=84000000
> > [drm:skl_update_plane [i915]] 	plane_surf=1040000
> > 
> > Do you have any tips of what could be causing this?
> > 
> > > > > +		return false;
> > > > > +	case I915_FORMAT_MOD_X_TILED:
> > > > > +	case I915_FORMAT_MOD_Y_TILED:
> > > > > +		return true;
> > > > > +	default:
> > > > > +		return false;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  static void intel_fbc_update_state_cache(struct intel_crtc
> > > > > *crtc,
> > > > >  					 const struct
> > > > > intel_crtc_state
> > > > > *crtc_state,
> > > > >  					 const struct
> > > > > intel_plane_state
> > > > > *plane_state)
> > > > > @@ -672,6 +691,7 @@ static void
> > > > > intel_fbc_update_state_cache(struct
> > > > > intel_crtc *crtc,
> > > > >  
> > > > >  	cache->fb.format = fb->format;
> > > > >  	cache->fb.stride = fb->pitches[0];
> > > > > +	cache->fb.modifier = fb->modifier;
> > > > >  
> > > > >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > > > > PLANE_HAS_FENCE &&
> > > > >  		    !plane_state->vma->fence);
> > > > > @@ -720,23 +740,33 @@ static bool
> > > > > intel_fbc_can_activate(struct
> > > > > intel_crtc *crtc)
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > -	/* The use of a CPU fence is mandatory in order to
> > > > > detect
> > > > > writes
> > > > > -	 * by the CPU to the scanout and trigger updates to the
> > > > > FBC.
> > > > > +	/* The use of a CPU fence is one of two ways to detect
> > > > > writes
> > > > > by the
> > > > > +	 * CPU to the scanout and trigger updates to the FBC.
> > > > > +	 *
> > > > > +	 * The other method is by software tracking(see
> > > > > +	 * intel_fbc_invalidate/flush()), it will manually
> > > > > notify FBC
> > > > > and nuke
> > > > > +	 * the current compressed buffer and recompress it.
> > > > >  	 *
> > > > >  	 * Note that is possible for a tiled surface to be
> > > > > unmappable
> > > > > (and
> > > > > -	 * so have no fence associated with it) due to aperture
> > > > > constaints
> > > > > +	 * so have no fence associated with it) due to aperture
> > > > > constraints
> > > > >  	 * at the time of pinning.
> > > > >  	 *
> > > > >  	 * FIXME with 90/270 degree rotation we should use the
> > > > > fence on
> > > > >  	 * the normal GTT view (the rotated view doesn't even
> > > > > have a
> > > > >  	 * fence). Would need changes to the FBC fence Y offset
> > > > > as
> > > > > well.
> > > > > -	 * For now this will effecively disable FBC with 90/270
> > > > > degree
> > > > > +	 * For now this will effectively disable FBC with
> > > > > 90/270 degree
> > > > >  	 * rotation.
> > > > >  	 */
> > > > > -	if (cache->fence_id < 0) {
> > > > > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> > > > >  		fbc->no_fbc_reason = "framebuffer not tiled or
> > > > > fenced";
> > > > >  		return false;
> > > > >  	}
> > > > > +
> > > > > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > > > > +		fbc->no_fbc_reason = "tiling unsupported";
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> > > > >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> > > > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > > > > intel_crtc *crtc)
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > > > > format)) 
> > > > > {
> > > > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > > > >format,
> > > > > +				   cache->plane.rotation)) {
> > > > >  		fbc->no_fbc_reason = "pixel format is invalid";
> > > > >  		return false;
> > > > >  	}
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 3330b538d379..bf88663d8217 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -413,6 +413,7 @@ struct intel_fbc {
> > > > >  		struct {
> > > > >  			const struct drm_format_info *format;
> > > > >  			unsigned int stride;
> > > > > +			u64 modifier;
> > > > >  		} fb;
> > > > >  		u16 gen9_wa_cfb_stride;
> > > > >  		s8 fence_id;
> > > > > -- 
> > > > > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN9+

[Souza, Jose]

On Wed, 2020-03-18 at 15:30 +0200, Ville Syrjälä wrote:
> On Fri, Mar 06, 2020 at 01:32:12AM +0000, Souza, Jose wrote:
> > On Thu, 2020-02-20 at 19:26 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 18, 2020 at 05:42:30PM -0800, José Roberto de Souza
> > > wrote:
> > > > dGFX have local memory so it do not have aperture and do not
> > > > support
> > > > CPU fences but even for iGFX it have a small number of fences.
> > > > 
> > > > As replacement for fences to track frontbuffer modifications by
> > > > CPU
> > > > we have a software tracking that is already in used by FBC and
> > > > PSR.
> > > > PSR don't support fences so it shows that this tracking is
> > > > reliable.
> > > > 
> > > > So lets make fences a nice-to-have to activate FBC for GEN9+,
> > > > this
> > > > will allow us to enable FBC for dGFXs and iGFXs even when there
> > > > is
> > > > no
> > > > available fence.
> > > > 
> > > > We do not set fences to rotated planes but FBC only have
> > > > restrictions
> > > > against 16bpp, so adding it here.
> > > > 
> > > > Also adding a new check for the tiling format, fences are only
> > > > set
> > > > to X and Y tiled planes but again FBC don't have any
> > > > restrictions
> > > > against tiling so adding linear as supported as well, other
> > > > formats
> > > > should be added after tested but IGT only supports drawing in
> > > > thse
> > > > 3 formats.
> > > > 
> > > > intel_fbc_hw_tracking_covers_screen() maybe can also have the
> > > > same
> > > > treatment as fences but BSpec is not clear if the size
> > > > limitation
> > > > is
> > > > for hardware tracking or general use of FBC and I don't have a
> > > > 5K
> > > > display to test it, so keeping as is for safety.
> > > > 
> > > > v2:
> > > > - Added tiling and pixel format rotation checks
> > > > - Changed the GEN version not requiring fences to 11 from 9,
> > > > DDX
> > > > needs some changes but it don't have support for GEN11+
> > > > 
> > > > v3:
> > > > - Changed back to GEN9+
> > > > - Moved GEN test to inside of tiling_is_valid()
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 45
> > > > ++++++++++++++++++++----
> > > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > > >  2 files changed, 39 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 1d76e3646a25..a0d1d661a006 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > > > drm_i915_private *dev_priv,
> > > >  }
> > > >  
> > > >  static bool pixel_format_is_valid(struct drm_i915_private
> > > > *dev_priv,
> > > > -				  u32 pixel_format)
> > > > +				  u32 pixel_format, unsigned
> > > > int
> > > > rotation)
> > > >  {
> > > >  	switch (pixel_format) {
> > > >  	case DRM_FORMAT_XRGB8888:
> > > > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > > > drm_i915_private *dev_priv,
> > > >  		/* WaFbcOnly1to1Ratio:ctg */
> > > >  		if (IS_G4X(dev_priv))
> > > >  			return false;
> > > > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > > > DRM_MODE_ROTATE_270)) &&
> > > > +		    INTEL_GEN(dev_priv) >= 9)
> > > > +			return false;
> > > 
> > > Would still would prefer a rotations_is_valid() or some such
> > > thing.
> > 
> > Like this?
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 5e35c894bdf9..692edd45b769 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -600,15 +600,21 @@ static bool pixel_format_is_valid(struct
> > drm_i915_private *dev_priv,
> >                 /* WaFbcOnly1to1Ratio:ctg */
> >                 if (IS_G4X(dev_priv))
> >                         return false;
> > -               if ((rotation & (DRM_MODE_ROTATE_90 |
> > DRM_MODE_ROTATE_270)) &&
> > -                   INTEL_GEN(dev_priv) >= 9)
> > -                       return false;
> >                 return true;
> >         default:
> >                 return false;
> >         }
> >  }
> > 
> > +static bool rotations_is_valid(struct drm_i915_private *dev_priv,
> > +                              u32 pixel_format, unsigned int
> > rotation)
> > +{
> > +       if (INTEL_GEN(dev_priv) >= 9 && pixel_format ==
> > DRM_FORMAT_RGB565 &&
> > +           rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270))
> 
> drm_rotation_90_or_270()

Done

> 
> > +               return false;
> > +       return true;
> > +}
> > +
> >  /*
> >   * For some reason, the hardware tracking starts looking at
> > whatever
> > we
> >   * programmed as the display plane base address register. It does
> > not
> > look at
> > @@ -810,6 +816,12 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >                 return false;
> >         }
> > 
> > +       if (!rotations_is_valid(dev_priv, cache->fb.format->format,
> > +                               cache->plane.rotation)) {
> > +               fbc->no_fbc_reason = "plane rotation is invalid";
> > +               return false;
> > +       }
> 
> s/rotations/rotation/ + I'd move the pre-g4x rotation check into
> that function as well. With that

Done and done

> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks

> 
> > +
> >         if (cache->plane.pixel_blend_mode !=
> > DRM_MODE_BLEND_PIXEL_NONE
> > &&
> >             cache->fb.format->has_alpha) {
> >                 fbc->no_fbc_reason = "per-pixel alpha blending is
> > incompatible with FBC";
> > 
> > 
> > > >  		return true;
> > > >  	default:
> > > >  		return false;
> > > > @@ -639,6 +642,22 @@ static bool
> > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > > >  	return effective_w <= max_w && effective_h <= max_h;
> > > >  }
> > > >  
> > > > +static bool tiling_is_valid(struct drm_i915_private *dev_priv,
> > > > +			    uint64_t modifier)
> > > > +{
> > > > +	switch (modifier) {
> > > > +	case DRM_FORMAT_MOD_LINEAR:
> > > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > > +			return true;
> > > 
> > > Have we checked that eg. fbcon cursor still blinks correctly
> > > with FBC active and all?
> > 
> > Hum on fbcon FBC is enabled but it never compress, IGT tests with
> > fences complete disables are working fine, screen is updated when
> > tests
> > asks to with FBC enabled and compressing.
> > 
> > I will debug fbcon a little more to understand why it is never
> > compressing. 
> > 
> > > > +		return false;
> > > > +	case I915_FORMAT_MOD_X_TILED:
> > > > +	case I915_FORMAT_MOD_Y_TILED:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > > +
> > > >  static void intel_fbc_update_state_cache(struct intel_crtc
> > > > *crtc,
> > > >  					 const struct
> > > > intel_crtc_state
> > > > *crtc_state,
> > > >  					 const struct
> > > > intel_plane_state
> > > > *plane_state)
> > > > @@ -672,6 +691,7 @@ static void
> > > > intel_fbc_update_state_cache(struct
> > > > intel_crtc *crtc,
> > > >  
> > > >  	cache->fb.format = fb->format;
> > > >  	cache->fb.stride = fb->pitches[0];
> > > > +	cache->fb.modifier = fb->modifier;
> > > >  
> > > >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > > > PLANE_HAS_FENCE &&
> > > >  		    !plane_state->vma->fence);
> > > > @@ -720,23 +740,33 @@ static bool intel_fbc_can_activate(struct
> > > > intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	/* The use of a CPU fence is mandatory in order to
> > > > detect
> > > > writes
> > > > -	 * by the CPU to the scanout and trigger updates to the
> > > > FBC.
> > > > +	/* The use of a CPU fence is one of two ways to detect
> > > > writes
> > > > by the
> > > > +	 * CPU to the scanout and trigger updates to the FBC.
> > > > +	 *
> > > > +	 * The other method is by software tracking(see
> > > > +	 * intel_fbc_invalidate/flush()), it will manually
> > > > notify FBC
> > > > and nuke
> > > > +	 * the current compressed buffer and recompress it.
> > > >  	 *
> > > >  	 * Note that is possible for a tiled surface to be
> > > > unmappable
> > > > (and
> > > > -	 * so have no fence associated with it) due to aperture
> > > > constaints
> > > > +	 * so have no fence associated with it) due to aperture
> > > > constraints
> > > >  	 * at the time of pinning.
> > > >  	 *
> > > >  	 * FIXME with 90/270 degree rotation we should use the
> > > > fence on
> > > >  	 * the normal GTT view (the rotated view doesn't even
> > > > have a
> > > >  	 * fence). Would need changes to the FBC fence Y offset
> > > > as
> > > > well.
> > > > -	 * For now this will effecively disable FBC with 90/270
> > > > degree
> > > > +	 * For now this will effectively disable FBC with
> > > > 90/270 degree
> > > >  	 * rotation.
> > > >  	 */
> > > > -	if (cache->fence_id < 0) {
> > > > +	if (INTEL_GEN(dev_priv) < 9 && cache->fence_id < 0) {
> > > >  		fbc->no_fbc_reason = "framebuffer not tiled or
> > > > fenced";
> > > >  		return false;
> > > >  	}
> > > > +
> > > > +	if (!tiling_is_valid(dev_priv, cache->fb.modifier)) {
> > > > +		fbc->no_fbc_reason = "tiling unsupported";
> > > > +		return false;
> > > > +	}
> > > > +
> > > >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> > > >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> > > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > > @@ -748,7 +778,8 @@ static bool intel_fbc_can_activate(struct
> > > > intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > > >format)) 
> > > > {
> > > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format-
> > > > >format,
> > > > +				   cache->plane.rotation)) {
> > > >  		fbc->no_fbc_reason = "pixel format is invalid";
> > > >  		return false;
> > > >  	}
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 3330b538d379..bf88663d8217 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -413,6 +413,7 @@ struct intel_fbc {
> > > >  		struct {
> > > >  			const struct drm_format_info *format;
> > > >  			unsigned int stride;
> > > > +			u64 modifier;
> > > >  		} fb;
> > > >  		u16 gen9_wa_cfb_stride;
> > > >  		s8 fence_id;
> > > > -- 
> > > > 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx