All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.