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.lankho...@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