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@xxxxxxxxxxxxxxx> --- 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
