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

Tue Sep 6 16:21:49 UTC 2022

On Tue, Sep 06, 2022 at 06:14:23PM +0200, Andrzej Hajda wrote:
> 
> 
> 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.

Ah. We'll that's basically the same thing as what I listed, except
in step 2 we just do a commit w/o any planes. It still clears out the
async_flip flags and then step 3 gets confused.

> 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

-- 
Ville Syrjälä
Intel