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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx#dmesg-warnings402
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx#dmesg-warnings402
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1208/fi-cfl-8109u/igt@kms_big_fb@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
...
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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Similar for SKL:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1181/fi-skl-guc/igt@kms_big_fb@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
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
