[Intel-gfx] drm/i915: Disable FBC on fastset if necessary

[Hans de Goede]

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 at 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.