Re: drm/i915: Disable FBC on fastset if necessary

[Hans de Goede <hdegoede@redhat.com>]

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