* [PATCH] drm/i915: Disable FBC on fastset if necessary
@ 2018-12-18 15:27 Maarten Lankhorst
2018-12-18 15:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Maarten Lankhorst @ 2018-12-18 15:27 UTC (permalink / raw)
To: intel-gfx
Without this, we will get a dmesg-warn when enable_fbc is cleared on a fastset:
WARN_ON(!crtc_state->enable_fbc)
WARNING: CPU: 0 PID: 1090 at drivers/gpu/drm/i915/intel_fbc.c:1091 intel_fbc_enable+0x2ce/0x580 [i915]
RIP: 0010:intel_fbc_enable+0x2ce/0x580 [i915]
Call Trace:
? __mutex_unlock_slowpath+0x46/0x2b0
intel_update_crtc+0x6f/0x2b0 [i915]
skl_update_crtcs+0x1d1/0x2b0 [i915]
intel_atomic_commit_tail+0x1ea/0xdb0 [i915]
intel_atomic_commit+0x244/0x330 [i915]
drm_mode_atomic_ioctl+0x85d/0x950
? drm_atomic_set_property+0x970/0x970
drm_ioctl_kernel+0x81/0xf0
drm_ioctl+0x2de/0x390
? drm_atomic_set_property+0x970/0x970
? __handle_mm_fault+0x81b/0xfc0
do_vfs_ioctl+0xa0/0x6e0
? __do_page_fault+0x2a5/0x550
ksys_ioctl+0x35/0x60
__x64_sys_ioctl+0x11/0x20
do_syscall_64+0x55/0x190
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 3 +++
drivers/gpu/drm/i915/intel_fbc.c | 2 --
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 825d9b9e7f28..a0fae61028d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5355,6 +5355,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config))
hsw_disable_ips(old_crtc_state);
+ if (pipe_config->update_pipe && !pipe_config->enable_fbc)
+ intel_fbc_disable(crtc);
+
if (old_primary_state) {
struct intel_plane_state *new_primary_state =
intel_atomic_get_new_plane_state(old_intel_state,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1d3ff026d1bc..ccd5e110a19c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1129,8 +1129,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
if (!fbc_supported(dev_priv))
return;
- WARN_ON(crtc->active);
-
mutex_lock(&fbc->lock);
if (fbc->crtc == crtc)
__intel_fbc_disable(dev_priv);
--
2.19.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Disable FBC on fastset if necessary 2018-12-18 15:27 [PATCH] drm/i915: Disable FBC on fastset if necessary Maarten Lankhorst @ 2018-12-18 15:51 ` Patchwork 2018-12-18 16:10 ` ✓ Fi.CI.BAT: success " Patchwork ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Patchwork @ 2018-12-18 15:51 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/i915: Disable FBC on fastset if necessary URL : https://patchwork.freedesktop.org/series/54226/ State : warning == Summary == $ dim checkpatch origin/drm-tip 0af44790eb9f drm/i915: Disable FBC on fastset if necessary -:6: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #6: Without this, we will get a dmesg-warn when enable_fbc is cleared on a fastset: total: 0 errors, 1 warnings, 0 checks, 17 lines checked _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Disable FBC on fastset if necessary 2018-12-18 15:27 [PATCH] drm/i915: Disable FBC on fastset if necessary Maarten Lankhorst 2018-12-18 15:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork @ 2018-12-18 16:10 ` Patchwork 2018-12-18 22:18 ` ✓ Fi.CI.IGT: " Patchwork 2018-12-20 13:43 ` Hans de Goede 3 siblings, 0 replies; 7+ messages in thread From: Patchwork @ 2018-12-18 16:10 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/i915: Disable FBC on fastset if necessary URL : https://patchwork.freedesktop.org/series/54226/ State : success == Summary == CI Bug Log - changes from CI_DRM_5331 -> Patchwork_11116 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/54226/revisions/1/mbox/ Known issues ------------ Here are the changes found in Patchwork_11116 that come from known issues: ### IGT changes ### #### Issues hit #### * {igt@runner@aborted}: - fi-icl-y: NOTRUN -> FAIL [fdo#108070] #### Possible fixes #### * igt@i915_module_load@reload-with-fault-injection: - fi-byt-clapper: WARN [fdo#108688] -> PASS * igt@i915_selftest@live_hangcheck: - fi-skl-guc: DMESG-FAIL [fdo#108593] -> PASS - fi-kbl-7560u: INCOMPLETE [fdo#108044] -> PASS * igt@kms_chamelium@hdmi-hpd-fast: - fi-kbl-7500u: FAIL [fdo#108767] -> PASS * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a: - fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS * igt@pm_rpm@module-reload: - fi-byt-clapper: FAIL [fdo#108675] -> PASS {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362 [fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044 [fdo#108070]: https://bugs.freedesktop.org/show_bug.cgi?id=108070 [fdo#108593]: https://bugs.freedesktop.org/show_bug.cgi?id=108593 [fdo#108675]: https://bugs.freedesktop.org/show_bug.cgi?id=108675 [fdo#108688]: https://bugs.freedesktop.org/show_bug.cgi?id=108688 [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767 Participating hosts (50 -> 45) ------------------------------ Additional (1): fi-icl-y Missing (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 Build changes ------------- * Linux: CI_DRM_5331 -> Patchwork_11116 CI_DRM_5331: 9373de80d37b523811cea7cfbf4de7b996096bcd @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4749: 270da20849db4d170db09673c6b67712c90ec9fe @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_11116: 0af44790eb9f3c2599008b7fae6bcfb87d6818fc @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == 0af44790eb9f drm/i915: Disable FBC on fastset if necessary == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11116/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Disable FBC on fastset if necessary 2018-12-18 15:27 [PATCH] drm/i915: Disable FBC on fastset if necessary Maarten Lankhorst 2018-12-18 15:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2018-12-18 16:10 ` ✓ Fi.CI.BAT: success " Patchwork @ 2018-12-18 22:18 ` Patchwork 2018-12-20 13:43 ` Hans de Goede 3 siblings, 0 replies; 7+ messages in thread From: Patchwork @ 2018-12-18 22:18 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/i915: Disable FBC on fastset if necessary URL : https://patchwork.freedesktop.org/series/54226/ State : success == Summary == CI Bug Log - changes from CI_DRM_5331_full -> Patchwork_11116_full ==================================================== Summary ------- **WARNING** Minor unknown changes coming with Patchwork_11116_full need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_11116_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_11116_full: ### IGT changes ### #### Warnings #### * igt@drm_read@invalid-buffer: - shard-snb: PASS -> SKIP +2 Known issues ------------ Here are the changes found in Patchwork_11116_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_eio@in-flight-contexts-immediate: - shard-glk: PASS -> FAIL [fdo#107799] * igt@gem_ppgtt@blt-vs-render-ctxn: - shard-apl: PASS -> INCOMPLETE [fdo#103927] * igt@gem_softpin@noreloc-s3: - shard-iclb: PASS -> INCOMPLETE [fdo#107713] * igt@gem_userptr_blits@readonly-unsync: - shard-skl: NOTRUN -> TIMEOUT [fdo#108887] * igt@gem_workarounds@suspend-resume-fd: - shard-iclb: PASS -> FAIL [fdo#103375] +1 * igt@i915_selftest@live_workarounds: - shard-iclb: PASS -> DMESG-FAIL [fdo#108954] * igt@kms_busy@extended-modeset-hang-newfb-render-a: - shard-iclb: NOTRUN -> DMESG-WARN [fdo#107956] * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c: - shard-skl: NOTRUN -> DMESG-WARN [fdo#107956] +1 * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a: - shard-glk: NOTRUN -> DMESG-WARN [fdo#107956] * igt@kms_ccs@pipe-a-crc-sprite-planes-basic: - shard-iclb: NOTRUN -> FAIL [fdo#107725] * igt@kms_color@pipe-c-legacy-gamma: - shard-apl: PASS -> FAIL [fdo#104782] * igt@kms_cursor_crc@cursor-128x128-random: - shard-apl: PASS -> FAIL [fdo#103232] +2 * igt@kms_cursor_crc@cursor-64x64-sliding: - shard-iclb: NOTRUN -> FAIL [fdo#103232] +2 * igt@kms_fbcon_fbt@psr-suspend: - shard-skl: NOTRUN -> FAIL [fdo#107882] * igt@kms_flip@flip-vs-expired-vblank: - shard-skl: PASS -> FAIL [fdo#105363] * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite: - shard-iclb: PASS -> FAIL [fdo#103167] +3 * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb: - shard-skl: NOTRUN -> FAIL [fdo#108145] * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf: - shard-apl: PASS -> FAIL [fdo#103166] - shard-iclb: PASS -> FAIL [fdo#103166] * igt@kms_rmfb@close-fd: - shard-iclb: NOTRUN -> DMESG-WARN [fdo#107724] +1 * igt@kms_sysfs_edid_timing: - shard-skl: NOTRUN -> FAIL [fdo#100047] * igt@pm_rpm@cursor-dpms: - shard-skl: PASS -> INCOMPLETE [fdo#107807] +1 * igt@pm_rpm@modeset-non-lpsp-stress: - shard-iclb: SKIP -> INCOMPLETE [fdo#108840] #### Possible fixes #### * igt@gem_workarounds@suspend-resume-context: - shard-iclb: INCOMPLETE [fdo#107713] -> PASS * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b: - shard-glk: DMESG-WARN [fdo#107956] -> PASS * igt@kms_cursor_crc@cursor-128x128-dpms: - shard-apl: FAIL [fdo#103232] -> PASS * igt@kms_cursor_crc@cursor-256x256-suspend: - shard-skl: INCOMPLETE [fdo#104108] / [fdo#107773] -> PASS * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size: - shard-skl: FAIL [fdo#102670] / [fdo#106081] -> PASS * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled: - shard-iclb: WARN [fdo#108336] -> PASS +2 * igt@kms_flip@flip-vs-expired-vblank-interruptible: - shard-skl: FAIL [fdo#105363] -> PASS * igt@kms_flip@modeset-vs-vblank-race-interruptible: - shard-glk: FAIL [fdo#103060] -> PASS * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite: - shard-iclb: FAIL [fdo#103167] -> PASS +3 * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-wc: - shard-glk: FAIL [fdo#103167] -> PASS +4 * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite: - shard-iclb: DMESG-FAIL [fdo#107724] -> PASS +4 * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping: - shard-glk: FAIL [fdo#108948] -> PASS * igt@kms_plane@plane-position-covered-pipe-c-planes: - shard-iclb: FAIL [fdo#103166] -> PASS +1 * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc: - shard-skl: FAIL [fdo#107815] / [fdo#108145] -> PASS * igt@kms_rotation_crc@primary-rotation-180: - shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +12 * igt@kms_setmode@basic: - shard-kbl: FAIL [fdo#99912] -> PASS * igt@pm_backlight@basic-brightness: - shard-iclb: DMESG-WARN [fdo#107724] -> PASS +22 * igt@pm_rpm@modeset-non-lpsp-stress-no-wait: - shard-iclb: INCOMPLETE [fdo#108840] -> SKIP * igt@pm_rpm@modeset-pc8-residency-stress: - shard-skl: INCOMPLETE [fdo#107807] -> SKIP #### Warnings #### * igt@kms_ccs@pipe-a-crc-primary-basic: - shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#107725] [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047 [fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670 [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060 [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166 [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232 [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375 [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108 [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782 [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363 [fdo#106081]: https://bugs.freedesktop.org/show_bug.cgi?id=106081 [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713 [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724 [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725 [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773 [fdo#107799]: https://bugs.freedesktop.org/show_bug.cgi?id=107799 [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807 [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815 [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882 [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956 [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145 [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336 [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840 [fdo#108887]: https://bugs.freedesktop.org/show_bug.cgi?id=108887 [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948 [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954 [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912 Participating hosts (7 -> 7) ------------------------------ No changes in participating hosts Build changes ------------- * Linux: CI_DRM_5331 -> Patchwork_11116 CI_DRM_5331: 9373de80d37b523811cea7cfbf4de7b996096bcd @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4749: 270da20849db4d170db09673c6b67712c90ec9fe @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_11116: 0af44790eb9f3c2599008b7fae6bcfb87d6818fc @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11116/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/i915: Disable FBC on fastset if necessary 2018-12-18 15:27 [PATCH] drm/i915: Disable FBC on fastset if necessary Maarten Lankhorst ` (2 preceding siblings ...) 2018-12-18 22:18 ` ✓ Fi.CI.IGT: " Patchwork @ 2018-12-20 13:43 ` Hans de Goede 2018-12-20 13:56 ` Hans de Goede 3 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2018-12-20 13:43 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx Hi, On 18-12-18 16:27, Maarten Lankhorst wrote: > Without this, we will get a dmesg-warn when enable_fbc is cleared on a fastset: > WARN_ON(!crtc_state->enable_fbc) > WARNING: CPU: 0 PID: 1090 at drivers/gpu/drm/i915/intel_fbc.c:1091 intel_fbc_enable+0x2ce/0x580 [i915] > RIP: 0010:intel_fbc_enable+0x2ce/0x580 [i915] > Call Trace: > ? __mutex_unlock_slowpath+0x46/0x2b0 > intel_update_crtc+0x6f/0x2b0 [i915] > skl_update_crtcs+0x1d1/0x2b0 [i915] > intel_atomic_commit_tail+0x1ea/0xdb0 [i915] > intel_atomic_commit+0x244/0x330 [i915] > drm_mode_atomic_ioctl+0x85d/0x950 > ? drm_atomic_set_property+0x970/0x970 > drm_ioctl_kernel+0x81/0xf0 > drm_ioctl+0x2de/0x390 > ? drm_atomic_set_property+0x970/0x970 > ? __handle_mm_fault+0x81b/0xfc0 > do_vfs_ioctl+0xa0/0x6e0 > ? __do_page_fault+0x2a5/0x550 > ksys_ioctl+0x35/0x60 > __x64_sys_ioctl+0x11/0x20 > do_syscall_64+0x55/0x190 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > drivers/gpu/drm/i915/intel_fbc.c | 2 -- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 825d9b9e7f28..a0fae61028d6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5355,6 +5355,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, > if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config)) > hsw_disable_ips(old_crtc_state); > > + if (pipe_config->update_pipe && !pipe_config->enable_fbc) > + intel_fbc_disable(crtc); > + > if (old_primary_state) { > struct intel_plane_state *new_primary_state = > intel_atomic_get_new_plane_state(old_intel_state, > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 1d3ff026d1bc..ccd5e110a19c 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -1129,8 +1129,6 @@ void intel_fbc_disable(struct intel_crtc *crtc) > if (!fbc_supported(dev_priv)) > return; > > - WARN_ON(crtc->active); > - > mutex_lock(&fbc->lock); > if (fbc->crtc == crtc) > __intel_fbc_disable(dev_priv); Hmm, normally we call intel_fbc_disable() from the first for_each_oldnew_crtc_in_state() loop in intel_atomic_commit_tail() that has a: if (!needs_modeset(new_crtc_state)) continue; Causing it to be skipped on fastsets. But on a full modeset that first loop also calls intel_pre_plane_update() directly after the needs_modeset() call and before it will call intel_fbc_disable() itself. So withn a full modeset your patch is causing disable_fbc to be called earlier (when moving to a new state without fbc) then before. Before your patch on a full modeset we would do: intel_pre_plane_update() intel_crtc_disable_planes() dev_priv->display.crtc_disable() intel_fbc_disable(intel_crtc); On a full modeset, but now we are doing: intel_pre_plane_update() -> intel_fbc_disable(intel_crtc); intel_crtc_disable_planes() dev_priv->display.crtc_disable() intel_fbc_disable(intel_crtc); /* Second call is a no-op */ Which seems like an undesirable side-effect of this patch. I think it would be better to instead add the: if (pipe_config->update_pipe && !pipe_config->enable_fbc) intel_fbc_disable(crtc); In intel_update_crtc() in the else block of the if (modeset) {} else {} in that function, after the intel_pre_plane_update() call, so that we do the pre_plane_update() vs fbc_disable() in the same order as in the full modeset path and so that we don't change the call order of the full modeset path. This will also nicely group it together with the intel_encoders_update_pipe() call which my fastset PSR / DRRS patches add (*). I'm still learning the i915 code so I may be wrong here, but the approach I'm suggesting seems better to me. Regards, Hans *) Which will cause a conflict when merging both, but fixing that is easy. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/i915: Disable FBC on fastset if necessary 2018-12-20 13:43 ` Hans de Goede @ 2018-12-20 13:56 ` Hans de Goede 2018-12-20 15:24 ` Maarten Lankhorst 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2018-12-20 13:56 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx Hi, On 20-12-18 14:43, Hans de Goede wrote: > Hi, > > On 18-12-18 16:27, Maarten Lankhorst wrote: >> Without this, we will get a dmesg-warn when enable_fbc is cleared on a fastset: >> WARN_ON(!crtc_state->enable_fbc) >> WARNING: CPU: 0 PID: 1090 at drivers/gpu/drm/i915/intel_fbc.c:1091 intel_fbc_enable+0x2ce/0x580 [i915] >> RIP: 0010:intel_fbc_enable+0x2ce/0x580 [i915] >> Call Trace: >> ? __mutex_unlock_slowpath+0x46/0x2b0 >> intel_update_crtc+0x6f/0x2b0 [i915] >> skl_update_crtcs+0x1d1/0x2b0 [i915] >> intel_atomic_commit_tail+0x1ea/0xdb0 [i915] >> intel_atomic_commit+0x244/0x330 [i915] >> drm_mode_atomic_ioctl+0x85d/0x950 >> ? drm_atomic_set_property+0x970/0x970 >> drm_ioctl_kernel+0x81/0xf0 >> drm_ioctl+0x2de/0x390 >> ? drm_atomic_set_property+0x970/0x970 >> ? __handle_mm_fault+0x81b/0xfc0 >> do_vfs_ioctl+0xa0/0x6e0 >> ? __do_page_fault+0x2a5/0x550 >> ksys_ioctl+0x35/0x60 >> __x64_sys_ioctl+0x11/0x20 >> do_syscall_64+0x55/0x190 >> entry_SYSCALL_64_after_hwframe+0x49/0xbe >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 3 +++ >> drivers/gpu/drm/i915/intel_fbc.c | 2 -- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 825d9b9e7f28..a0fae61028d6 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5355,6 +5355,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, >> if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config)) >> hsw_disable_ips(old_crtc_state); >> + if (pipe_config->update_pipe && !pipe_config->enable_fbc) >> + intel_fbc_disable(crtc); >> + >> if (old_primary_state) { >> struct intel_plane_state *new_primary_state = >> intel_atomic_get_new_plane_state(old_intel_state, >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index 1d3ff026d1bc..ccd5e110a19c 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -1129,8 +1129,6 @@ void intel_fbc_disable(struct intel_crtc *crtc) >> if (!fbc_supported(dev_priv)) >> return; >> - WARN_ON(crtc->active); >> - >> mutex_lock(&fbc->lock); >> if (fbc->crtc == crtc) >> __intel_fbc_disable(dev_priv); > > Hmm, normally we call intel_fbc_disable() from the first > for_each_oldnew_crtc_in_state() loop in intel_atomic_commit_tail() > that has a: > > if (!needs_modeset(new_crtc_state)) > continue; > > Causing it to be skipped on fastsets. But on a full modeset > that first loop also calls intel_pre_plane_update() directly > after the needs_modeset() call and before it will call > intel_fbc_disable() itself. > > So withn a full modeset your patch is causing disable_fbc to be > called earlier (when moving to a new state without fbc) then before. > > Before your patch on a full modeset we would do: > > intel_pre_plane_update() > intel_crtc_disable_planes() > dev_priv->display.crtc_disable() > intel_fbc_disable(intel_crtc); > > On a full modeset, but now we are doing: > > intel_pre_plane_update() -> > intel_fbc_disable(intel_crtc); > intel_crtc_disable_planes() > dev_priv->display.crtc_disable() > intel_fbc_disable(intel_crtc); /* Second call is a no-op */ > > Which seems like an undesirable side-effect of this patch. > > I think it would be better to instead add the: > > if (pipe_config->update_pipe && !pipe_config->enable_fbc) > intel_fbc_disable(crtc); > > In intel_update_crtc() in the else block of the if (modeset) {} else {} > in that function, after the intel_pre_plane_update() call, so that we > do the pre_plane_update() vs fbc_disable() in the same order as > in the full modeset path and so that we don't change the call > order of the full modeset path. > > This will also nicely group it together with the > intel_encoders_update_pipe() call which my fastset PSR / DRRS > patches add (*). p.s. And this will also put it just before the only place where we call intel_fbc_enable(), so also grouping it with that call, which feels like the right thing to do to me. > I'm still learning the i915 code so I may be wrong here, > but the approach I'm suggesting seems better to me. Regards, Hans > *) Which will cause a conflict when merging both, but fixing that > is easy. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drm/i915: Disable FBC on fastset if necessary 2018-12-20 13:56 ` Hans de Goede @ 2018-12-20 15:24 ` Maarten Lankhorst 0 siblings, 0 replies; 7+ messages in thread From: Maarten Lankhorst @ 2018-12-20 15:24 UTC (permalink / raw) To: Hans de Goede, intel-gfx Op 20-12-2018 om 14:56 schreef Hans de Goede: > Hi, > > On 20-12-18 14:43, Hans de Goede wrote: >> Hi, >> >> On 18-12-18 16:27, Maarten Lankhorst wrote: >>> Without this, we will get a dmesg-warn when enable_fbc is cleared on a fastset: >>> WARN_ON(!crtc_state->enable_fbc) >>> WARNING: CPU: 0 PID: 1090 at drivers/gpu/drm/i915/intel_fbc.c:1091 intel_fbc_enable+0x2ce/0x580 [i915] >>> RIP: 0010:intel_fbc_enable+0x2ce/0x580 [i915] >>> Call Trace: >>> ? __mutex_unlock_slowpath+0x46/0x2b0 >>> intel_update_crtc+0x6f/0x2b0 [i915] >>> skl_update_crtcs+0x1d1/0x2b0 [i915] >>> intel_atomic_commit_tail+0x1ea/0xdb0 [i915] >>> intel_atomic_commit+0x244/0x330 [i915] >>> drm_mode_atomic_ioctl+0x85d/0x950 >>> ? drm_atomic_set_property+0x970/0x970 >>> drm_ioctl_kernel+0x81/0xf0 >>> drm_ioctl+0x2de/0x390 >>> ? drm_atomic_set_property+0x970/0x970 >>> ? __handle_mm_fault+0x81b/0xfc0 >>> do_vfs_ioctl+0xa0/0x6e0 >>> ? __do_page_fault+0x2a5/0x550 >>> ksys_ioctl+0x35/0x60 >>> __x64_sys_ioctl+0x11/0x20 >>> do_syscall_64+0x55/0x190 >>> entry_SYSCALL_64_after_hwframe+0x49/0xbe >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 3 +++ >>> drivers/gpu/drm/i915/intel_fbc.c | 2 -- >>> 2 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 825d9b9e7f28..a0fae61028d6 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -5355,6 +5355,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state, >>> if (hsw_pre_update_disable_ips(old_crtc_state, pipe_config)) >>> hsw_disable_ips(old_crtc_state); >>> + if (pipe_config->update_pipe && !pipe_config->enable_fbc) >>> + intel_fbc_disable(crtc); >>> + >>> if (old_primary_state) { >>> struct intel_plane_state *new_primary_state = >>> intel_atomic_get_new_plane_state(old_intel_state, >>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >>> index 1d3ff026d1bc..ccd5e110a19c 100644 >>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>> @@ -1129,8 +1129,6 @@ void intel_fbc_disable(struct intel_crtc *crtc) >>> if (!fbc_supported(dev_priv)) >>> return; >>> - WARN_ON(crtc->active); >>> - >>> mutex_lock(&fbc->lock); >>> if (fbc->crtc == crtc) >>> __intel_fbc_disable(dev_priv); >> >> Hmm, normally we call intel_fbc_disable() from the first >> for_each_oldnew_crtc_in_state() loop in intel_atomic_commit_tail() >> that has a: >> >> if (!needs_modeset(new_crtc_state)) >> continue; >> >> Causing it to be skipped on fastsets. But on a full modeset >> that first loop also calls intel_pre_plane_update() directly >> after the needs_modeset() call and before it will call >> intel_fbc_disable() itself. >> >> So withn a full modeset your patch is causing disable_fbc to be >> called earlier (when moving to a new state without fbc) then before. >> >> Before your patch on a full modeset we would do: >> >> intel_pre_plane_update() >> intel_crtc_disable_planes() >> dev_priv->display.crtc_disable() >> intel_fbc_disable(intel_crtc); >> >> On a full modeset, but now we are doing: >> >> intel_pre_plane_update() -> >> intel_fbc_disable(intel_crtc); >> intel_crtc_disable_planes() >> dev_priv->display.crtc_disable() >> intel_fbc_disable(intel_crtc); /* Second call is a no-op */ >> >> Which seems like an undesirable side-effect of this patch. >> >> I think it would be better to instead add the: >> >> if (pipe_config->update_pipe && !pipe_config->enable_fbc) >> intel_fbc_disable(crtc); >> >> In intel_update_crtc() in the else block of the if (modeset) {} else {} >> in that function, after the intel_pre_plane_update() call, so that we >> do the pre_plane_update() vs fbc_disable() in the same order as >> in the full modeset path and so that we don't change the call >> order of the full modeset path. >> >> This will also nicely group it together with the >> intel_encoders_update_pipe() call which my fastset PSR / DRRS >> patches add (*). > > p.s. > > And this will also put it just before the only place where we > call intel_fbc_enable(), so also grouping it with that call, > which feels like the right thing to do to me. > >> I'm still learning the i915 code so I may be wrong here, >> but the approach I'm suggesting seems better to me. Hey, Correctly observed. I don't think it's harmful in general though. But just in case I posted a v2. :) ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-20 15:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-18 15:27 [PATCH] drm/i915: Disable FBC on fastset if necessary Maarten Lankhorst 2018-12-18 15:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2018-12-18 16:10 ` ✓ Fi.CI.BAT: success " Patchwork 2018-12-18 22:18 ` ✓ Fi.CI.IGT: " Patchwork 2018-12-20 13:43 ` Hans de Goede 2018-12-20 13:56 ` Hans de Goede 2018-12-20 15:24 ` Maarten Lankhorst
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.