[Intel-gfx] [PATCH] drm/i915: do not reset PLANE_SURF on plane disable on older gens

Tue Sep 6 16:14:23 UTC 2022


On 06.09.2022 16:43, Ville Syrjälä wrote:
> On Tue, Sep 06, 2022 at 05:36:05PM +0300, Ville Syrjälä wrote:
>> On Tue, Sep 06, 2022 at 05:14:56PM +0300, Ville Syrjälä wrote:
>>> On Tue, Sep 06, 2022 at 03:57:37PM +0200, Andrzej Hajda wrote:
>>>>
>>>> On 06.09.2022 13:22, Ville Syrjälä wrote:
>>>>> On Tue, Sep 06, 2022 at 01:09:16PM +0200, Andrzej Hajda wrote:
>>>>>> On 05.09.2022 19:44, Ville Syrjälä wrote:
>>>>>>> On Mon, Sep 05, 2022 at 07:02:40PM +0200, Andrzej Hajda wrote:
>>>>>>>> On 05.09.2022 13:48, Ville Syrjälä wrote:
>>>>>>>>> On Mon, Sep 05, 2022 at 10:05:00AM +0200, Andrzej Hajda wrote:
>>>>>>>>>> In case of ICL and older generations disabling plane and/or disabling
>>>>>>>>>> async update is always performed on vblank,
>>>>>>>>> It should only be broken on bdw-glk (see. need_async_flip_disable_wa).
>>>>>>>> On CFL it is reported every drmtip run:
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip.html?testfilter=tiled-max-hw
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html#dmesg-warnings402
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html#dmesg-warnings402
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1208/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html
>>>>>>>> ...
>>>>>>>> On APL it is less frequent, probably due to other bugs preventing run of
>>>>>>>> this test, last seen at:
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1190/fi-apl-guc/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
>>>>>>>> Similar for SKL:
>>>>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1181/fi-skl-guc/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html
>>>>>>>>
>>>>>>>> I am not sure if I correctly read the docs but [1] says that 9th bit of
>>>>>>>> PLANE_CFG (Async Address Update Enable) is "not double buffered and the
>>>>>>>> changes will apply immediately" only for ICL, JSL, LKF1.
>>>>>>> It got broken in bdw and fixed again in icl.
>>>>>>>
>>>>>>>> So the change is not necessary in case of icl_plane_disable_arm.
>>>>>>>>
>>>>>>>> [1]: https://gfxspecs.intel.com/Predator/Home/Index/7656
>>>>>>>>>> but if async update is enabled
>>>>>>>>>> PLANE_SURF register is updated asynchronously. Writing 0 to PLANE_SURF
>>>>>>>>>> when plane is still enabled can cause DMAR/PIPE errors.
>>>>>>>>>> On the other side PLANE_SURF is used to arm plane registers - we need to
>>>>>>>>>> write to it to trigger update on VBLANK, writting current value should
>>>>>>>>>> be safe - the buffer address is valid till vblank.
>>>>>>>>> I think you're effectively saying that somehow the async
>>>>>>>>> flip disable w/a is not kicking in sometimes.
>>>>>>>> I was not aware of existence of this w/a and I am little lost in
>>>>>>>> figuring out how this w/a can prevent zeroing PLANE_SURF too early.
>>>>>>> When it works as designed it should:
>>>>>>> 1. turn off the async flip bit
>>>>>>> 2. wait for vblank so that gets latched
>>>>>>> 3. do the sync plane update/disable normally
>>>>>> After debugging this terra incognita, I've figured out that plane states
>>>>>> are not populated in intel_crtc_async_flip_disable_wa
>>>>>> so for_each_old_intel_plane_in_state does not iterate over affected
>>>>>> planes and w/a does not work at all.
>>>>>> I have no idea where affected plane states should be added.
>>>>>> Adding them to the beginning of intel_atomic_check helped, but this is
>>>>>> just blind shot:
>>>>>>
>>>>>> @@ -6778,10 +6778,14 @@ static int intel_atomic_check(struct drm_device
>>>>>> *dev,
>>>>>>                 new_crtc_state->uapi.mode_changed = true;
>>>>>>
>>>>>>             if (new_crtc_state->uapi.scaling_filter !=
>>>>>>                 old_crtc_state->uapi.scaling_filter)
>>>>>>                 new_crtc_state->uapi.mode_changed = true;
>>>>>> +
>>>>>> +        ret = intel_atomic_add_affected_planes(state, crtc);
>>>>>> +        if (ret)
>>>>>> +            goto fail;
>>>>>>         }
>>>>>>
>>>>>>         intel_vrr_check_modeset(state);
>>>>>>
>>>>>>         ret = drm_atomic_helper_check_modeset(dev, &state->base);
>>>>>                 ^
>>>>> This guy should be adding them for any crtc that has been flagged
>>>>> for modeset ahead of time. For modesets flagged later we have to
>>>>> add them by hand (eg. in intel_modeset_all_pipes()).
>>>> This is no-modeset scenario, drm_atomic_helper_check_modeset does not
>>>> add planes in this case.
>>> Then he mystery is how intel_crtc_async_flip_disable_wa() manages
>>> to not disable async flip for some planes...
>> After a few minutes of pondering I have a theory:
>> 1. async flip on plane 1
>>     crtc_state.*async_flip: false -> true
>> 2. sync flip on plane 2, plane 1 not include in state
>>     crtc_state.*async_flip: true -> false, but plane 1 still remains in
>>     async flip mode
>> 3. sync update/disable plane 1
>>     crtc_state.*async_flip = true -> true, so the async flip disable w/a
>                                ^^^^^^^^^^^^
> Meant to write false->false there.

There is only one plane in game.
Apparently there is an issue with intel_crtc_crc_setup_workarounds.
It calls intel_atomic_get_crtc_state for fresh state, causing state 
duplication, but async_flip flag is set always to false in new state.
In cases full modeset is not performed hw and sw state of async_flip 
will differ after commit of this state.
Ugly/racy workaround for this below:
---
@@ -316,10 +316,13 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc 
*crtc, bool enable)
      if (IS_HASWELL(dev_priv) &&
          pipe_config->hw.active && crtc->pipe == PIPE_A &&
          pipe_config->cpu_transcoder == TRANSCODER_EDP)
          pipe_config->uapi.mode_changed = true;

+    if (!pipe_config->uapi.mode_changed)
+        pipe_config->uapi.async_flip = crtc->base.state->async_flip;
+
      ret = drm_atomic_commit(state);

  put_state:
      if (ret == -EDEADLK) {
          drm_atomic_state_clear(state);
---

Regards
Andrzej
>
>>     is not triggeed
>>
>> Should be easy to type up a dedicated test case for that.
>>
>> I think there are two options of handling this:
>> - Switch all planes out of async flip mode for any sync update.
>>    Not great because then you can't mix async flips with any other
>>    sync updates on the same crtc
>> - Start tracking which planes are in async flip mode vs. not
>>    Should allow more freedom in mixing async flips with other updates
>>    on the crtc
>>
>> -- 
>> Ville Syrjälä
>> Intel

