Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 12017 bytes --]

On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:

The KernelCI bisection bot found regressions in at least two KMS tests
in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
merged up mainline:

   igt-kms-rockchip.kms_vblank.pipe-A-wait-forked 
   igt-kms-rockchip.kms_vblank.pipe-A-query-busy

which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
PSR-exit to disable transition").  I'm not *100%* sure I trust the
bisection but it sure is suspicous that two separate bisects for related
issues landed on the same commit.

Below is the full report for the bisect for the first test, the bisect
for the latter looks identical.  It's got links to full logs for the
test run and a Reported-by for the bot - I do see some backtraces from
userspace in the output, the first is:

| IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
| <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
| Starting subtest: short-buffer-wakeup
| 
| (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
| (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
| Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
| 
| (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
| Stack trace:
| 
|   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
|   #1 [<unknown>+0xd5362770]
|   #2 [<unknown>+0xd536193c]
|   #3 [__libc_start_main+0xe8]
|   #4 [<unknown>+0xd5361974]
|   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
| Subtest short-buffer-wakeup failed.

Unfortunately we don't have current results from mainline or -next.

> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> * This automated bisection report was sent to you on the basis  *
> * that you may be involved with the breaking commit it has      *
> * found.  No manual investigation has been done to verify it,   *
> * and the root cause of the problem may be somewhere else.      *
> *                                                               *
> * If you do send a fix, please include this trailer:            *
> *   Reported-by: "kernelci.org bot" <bot@kernelci.org>          *
> *                                                               *
> * Hope this helps!                                              *
> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> 
> renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
> 
> Summary:
>   Start:      710ce3a8a6fbfc Merge tag 'v6.1' into renesas-devel
>   Plain log:  https://storage.kernelci.org/renesas/master/renesas-devel-2022-12-12-v6.1/arm64/defconfig+arm64-chromebook/gcc-10/lab-collabora/igt-kms-rockchip-rk3399-gru-kevin.txt
>   HTML log:   https://storage.kernelci.org/renesas/master/renesas-devel-2022-12-12-v6.1/arm64/defconfig+arm64-chromebook/gcc-10/lab-collabora/igt-kms-rockchip-rk3399-gru-kevin.html
>   Result:     ca871659ec1606 drm/bridge: analogix_dp: Support PSR-exit to disable transition
> 
> Checks:
>   revert:     PASS
>   verify:     PASS
> 
> Parameters:
>   Tree:       renesas
>   URL:        https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git
>   Branch:     master
>   Target:     rk3399-gru-kevin
>   CPU arch:   arm64
>   Lab:        lab-collabora
>   Compiler:   gcc-10
>   Config:     defconfig+arm64-chromebook
>   Test case:  igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
> 
> Breaking commit found:
> 
> -------------------------------------------------------------------------------
> commit ca871659ec1606d33b1e76de8d4cf924cf627e34
> Author: Brian Norris <briannorris@chromium.org>
> Date:   Mon Feb 28 12:25:31 2022 -0800
> 
>     drm/bridge: analogix_dp: Support PSR-exit to disable transition
>     
>     Most eDP panel functions only work correctly when the panel is not in
>     self-refresh. In particular, analogix_dp_bridge_disable() tends to hit
>     AUX channel errors if the panel is in self-refresh.
>     
>     Given the above, it appears that so far, this driver assumes that we are
>     never in self-refresh when it comes time to fully disable the bridge.
>     Prior to commit 846c7dfc1193 ("drm/atomic: Try to preserve the crtc
>     enabled state in drm_atomic_remove_fb, v2."), this tended to be true,
>     because we would automatically disable the pipe when framebuffers were
>     removed, and so we'd typically disable the bridge shortly after the last
>     display activity.
>     
>     However, that is not guaranteed: an idle (self-refresh) display pipe may
>     be disabled, e.g., when switching CRTCs. We need to exit PSR first.
>     
>     Stable notes: this is definitely a bugfix, and the bug has likely
>     existed in some form for quite a while. It may predate the "PSR helpers"
>     refactor, but the code looked very different before that, and it's
>     probably not worth rewriting the fix.
>     
>     Cc: <stable@vger.kernel.org>
>     Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
>     Signed-off-by: Brian Norris <briannorris@chromium.org>
>     Reviewed-by: Sean Paul <seanpaul@chromium.org>
>     Signed-off-by: Douglas Anderson <dianders@chromium.org>
>     Link: https://patchwork.freedesktop.org/patch/msgid/20220228122522.v2.1.I161904be17ba14526f78536ccd78b85818449b51@changeid
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index eb590fb8e8d0dd..0300f670a4fde7 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1268,6 +1268,25 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
>  	return 0;
>  }
>  
> +static
> +struct drm_crtc *analogix_dp_get_old_crtc(struct analogix_dp_device *dp,
> +					  struct drm_atomic_state *state)
> +{
> +	struct drm_encoder *encoder = dp->encoder;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +
> +	connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> +	if (!connector)
> +		return NULL;
> +
> +	conn_state = drm_atomic_get_old_connector_state(state, connector);
> +	if (!conn_state)
> +		return NULL;
> +
> +	return conn_state->crtc;
> +}
> +
>  static
>  struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
>  					  struct drm_atomic_state *state)
> @@ -1448,14 +1467,16 @@ analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  {
>  	struct drm_atomic_state *old_state = old_bridge_state->base.state;
>  	struct analogix_dp_device *dp = bridge->driver_private;
> -	struct drm_crtc *crtc;
> +	struct drm_crtc *old_crtc, *new_crtc;
> +	struct drm_crtc_state *old_crtc_state = NULL;
>  	struct drm_crtc_state *new_crtc_state = NULL;
> +	int ret;
>  
> -	crtc = analogix_dp_get_new_crtc(dp, old_state);
> -	if (!crtc)
> +	new_crtc = analogix_dp_get_new_crtc(dp, old_state);
> +	if (!new_crtc)
>  		goto out;
>  
> -	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> +	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, new_crtc);
>  	if (!new_crtc_state)
>  		goto out;
>  
> @@ -1464,6 +1485,19 @@ analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  		return;
>  
>  out:
> +	old_crtc = analogix_dp_get_old_crtc(dp, old_state);
> +	if (old_crtc) {
> +		old_crtc_state = drm_atomic_get_old_crtc_state(old_state,
> +							       old_crtc);
> +
> +		/* When moving from PSR to fully disabled, exit PSR first. */
> +		if (old_crtc_state && old_crtc_state->self_refresh_active) {
> +			ret = analogix_dp_disable_psr(dp);
> +			if (ret)
> +				DRM_ERROR("Failed to disable psr (%d)\n", ret);
> +		}
> +	}
> +
>  	analogix_dp_bridge_disable(bridge);
>  }
> -------------------------------------------------------------------------------
> 
> 
> Git bisection log:
> 
> -------------------------------------------------------------------------------
> git bisect start
> # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch 'renesas-next' into renesas-devel
> git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7
> # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag 'v6.1' into renesas-devel
> git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e
> # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag 'ata-6.0-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata
> git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c
> # bad: [12b68040a5e468068fd7f4af1150eab8f6e96235] Merge tag 'media/v5.20-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> git bisect bad 12b68040a5e468068fd7f4af1150eab8f6e96235
> # bad: [416e05e5b7ce63402a2c342472799d340559f10e] Merge tag 'regulator-v5.20' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator
> git bisect bad 416e05e5b7ce63402a2c342472799d340559f10e
> # bad: [2b18593e4b9f5781a7683fca256036515bd9b946] Merge tag 'perf_urgent_for_v5.19_rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> git bisect bad 2b18593e4b9f5781a7683fca256036515bd9b946
> # bad: [be129fab66f284c239251ec5b6e30c6e903d8881] Merge tag 'for-5.19/fbdev-2' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev
> git bisect bad be129fab66f284c239251ec5b6e30c6e903d8881
> # bad: [5c0cd3d4a976b906c3953ff0a0595ba37e04aaa6] Merge tag 'fs_for_v5.19-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
> git bisect bad 5c0cd3d4a976b906c3953ff0a0595ba37e04aaa6
> # bad: [cfd80687a5388e731b3db65ad6a557ede9b45905] net: hns3: modify the ring param print info
> git bisect bad cfd80687a5388e731b3db65ad6a557ede9b45905
> # good: [68171bbd1a9adaadac0c6a85c8558eaf0b718387] Merge tag 'net-5.19-rc2-2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> git bisect good 68171bbd1a9adaadac0c6a85c8558eaf0b718387
> # bad: [8f7ac50c97d30ae5ae48da3b516510d05a67b9e5] Merge tag 'sound-5.19-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
> git bisect bad 8f7ac50c97d30ae5ae48da3b516510d05a67b9e5
> # good: [0a178750647e47de1700edb2cbd9b0854122f4b9] Merge tag 'amd-drm-fixes-5.19-2022-06-08' of https://gitlab.freedesktop.org/agd5f/linux into drm-fixes
> git bisect good 0a178750647e47de1700edb2cbd9b0854122f4b9
> # good: [2abdf9f80019e8244d3806ed0e1c9f725e50b452] ASoC: wm_adsp: Fix event generation for wm_adsp_fw_put()
> git bisect good 2abdf9f80019e8244d3806ed0e1c9f725e50b452
> # bad: [8dd77d44795d708f5f4f783b81c5197c5b994d74] Merge tag 'drm-fixes-2022-06-10' of git://anongit.freedesktop.org/drm/drm
> git bisect bad 8dd77d44795d708f5f4f783b81c5197c5b994d74
> # bad: [e54a4424925a27ed94dff046db3ce5caf4b1e748] drm/atomic: Force bridge self-refresh-exit on CRTC switch
> git bisect bad e54a4424925a27ed94dff046db3ce5caf4b1e748
> # good: [6e516faf04317db2c46cbec4e3b78b4653a5b109] drm/panfrost: Job should reference MMU not file_priv
> git bisect good 6e516faf04317db2c46cbec4e3b78b4653a5b109
> # bad: [ca871659ec1606d33b1e76de8d4cf924cf627e34] drm/bridge: analogix_dp: Support PSR-exit to disable transition
> git bisect bad ca871659ec1606d33b1e76de8d4cf924cf627e34
> # first bad commit: [ca871659ec1606d33b1e76de8d4cf924cf627e34] drm/bridge: analogix_dp: Support PSR-exit to disable transition
> -------------------------------------------------------------------------------
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#35500): https://groups.io/g/kernelci-results/message/35500
> Mute This Topic: https://groups.io/mt/95644443/1131744
> Group Owner: kernelci-results+owner@groups.io
> Unsubscribe: https://groups.io/g/kernelci-results/unsub [broonie@kernel.org]
> -=-=-=-=-=-=-=-=-=-=-=-
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Geert Uytterhoeven]

Hi Mark,

Thanks for your report!

On Tue, Dec 13, 2022 at 5:58 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
> The KernelCI bisection bot found regressions in at least two KMS tests
> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
> merged up mainline:
>
>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
>
> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
> PSR-exit to disable transition").  I'm not *100%* sure I trust the
> bisection but it sure is suspicous that two separate bisects for related
> issues landed on the same commit.

... which is an old commit, added in v5.19-rc2, and which did not
enter through the renesas tree at all?

> Below is the full report for the bisect for the first test, the bisect
> for the latter looks identical.  It's got links to full logs for the
> test run and a Reported-by for the bot - I do see some backtraces from
> userspace in the output, the first is:
>
> | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
> | <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
> | Starting subtest: short-buffer-wakeup
> |
> | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
> | (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
> | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
> |
> | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
> | Stack trace:
> |
> |   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
> |   #1 [<unknown>+0xd5362770]
> |   #2 [<unknown>+0xd536193c]
> |   #3 [__libc_start_main+0xe8]
> |   #4 [<unknown>+0xd5361974]
> |   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
> | Subtest short-buffer-wakeup failed.
>
> Unfortunately we don't have current results from mainline or -next.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Guillaume Tucker]

Hi Mark,

Maybe you could retrieve the original thread and rely to it with
the report?  That's the ideal way of following up on a patch I
think.  You can get the mbox file this way:

./kci_bisect get_mbox \
  --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 \
  --kdir ~/src/linux


On 14/12/2022 11:06, Geert Uytterhoeven wrote:
> Hi Mark,
> 
> Thanks for your report!
> 
> On Tue, Dec 13, 2022 at 5:58 PM Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
>> The KernelCI bisection bot found regressions in at least two KMS tests
>> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
>> merged up mainline:
>>
>>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
>>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
>>
>> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
>> PSR-exit to disable transition").  I'm not *100%* sure I trust the
>> bisection but it sure is suspicous that two separate bisects for related
>> issues landed on the same commit.
> 
> ... which is an old commit, added in v5.19-rc2, and which did not
> enter through the renesas tree at all?

Do you mean this report shouldn't have been sent to you?

FYI The renesas tree got rebased and inherited a regression which
got bisected.  The same thing probably happened to other trees.
Yes it would be good to have 2nd order regressions based on a
reference branch (e.g. mainline in this case) in KernelCI but
that's for next year.  Regardless, it found a commit and the
bisection looks legit.

>> Below is the full report for the bisect for the first test, the bisect
>> for the latter looks identical.  It's got links to full logs for the
>> test run and a Reported-by for the bot - I do see some backtraces from
>> userspace in the output, the first is:
>>
>> | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
>> | <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
>> | Starting subtest: short-buffer-wakeup
>> |
>> | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
>> | (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
>> | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
>> |
>> | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
>> | Stack trace:
>> |
>> |   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
>> |   #1 [<unknown>+0xd5362770]
>> |   #2 [<unknown>+0xd536193c]
>> |   #3 [__libc_start_main+0xe8]
>> |   #4 [<unknown>+0xd5361974]
>> |   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
>> | Subtest short-buffer-wakeup failed.
>>
>> Unfortunately we don't have current results from mainline or -next.

Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 982 bytes --]

On Wed, Dec 14, 2022 at 01:55:03PM +0100, Guillaume Tucker wrote:

> Maybe you could retrieve the original thread and rely to it with
> the report?  That's the ideal way of following up on a patch I
> think.  You can get the mbox file this way:

> ./kci_bisect get_mbox \
>   --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 \
>   --kdir ~/src/linux

As a developer I tend to find this unhelpful, it makes it much more
likely that the mail will get missed.  As a reporter it means there's
more information to copy into the report.

> > ... which is an old commit, added in v5.19-rc2, and which did not
> > enter through the renesas tree at all?

> Do you mean this report shouldn't have been sent to you?

I do notice that the Renesas tree tends to get a *lot* of the bisection
reports generated for some reason (vastly more than any other tree
including mainline or -next), however this wasn't sent based on the tree
at all - I just looked at the people involved with the commit.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Guillaume Tucker]

On 14/12/2022 14:50, Mark Brown wrote:
> On Wed, Dec 14, 2022 at 01:55:03PM +0100, Guillaume Tucker wrote:
> 
>> Maybe you could retrieve the original thread and rely to it with
>> the report?  That's the ideal way of following up on a patch I
>> think.  You can get the mbox file this way:
> 
>> ./kci_bisect get_mbox \
>>   --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 \
>>   --kdir ~/src/linux
> 
> As a developer I tend to find this unhelpful, it makes it much more
> likely that the mail will get missed.  As a reporter it means there's
> more information to copy into the report.


Well it's up to you or anyone reporting the bisection result.
Base on my personal experience, I always got very quick replies
when doing this.

I don't see your point about copying more information though, I
would just open the mbox in my mail client to reply and paste the
content of the bisection report.  With a bit more work this could
be fully automated but that should be part of the bisection
rework using the new API & pipeline so sometime later in 2023...

>>> ... which is an old commit, added in v5.19-rc2, and which did not
>>> enter through the renesas tree at all?
> 
>> Do you mean this report shouldn't have been sent to you?
> 
> I do notice that the Renesas tree tends to get a *lot* of the bisection
> reports generated for some reason (vastly more than any other tree
> including mainline or -next), however this wasn't sent based on the tree
> at all - I just looked at the people involved with the commit.

In the past month, there were 15 bisection reports on renesas, 7
on linux-next and 28 on mainline for a total of 79 so 29 in other
trees.  So it's true renesas is getting quite a lot of them, it's
not entirely clear to me why that's the case but it's worth
investigating a bit.

See my other email about "kci_bisect get_recipients".

Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1751 bytes --]

On Wed, Dec 14, 2022 at 03:27:56PM +0100, Guillaume Tucker wrote:
> On 14/12/2022 14:50, Mark Brown wrote:

> > As a developer I tend to find this unhelpful, it makes it much more
> > likely that the mail will get missed.  As a reporter it means there's
> > more information to copy into the report.

> Well it's up to you or anyone reporting the bisection result.
> Base on my personal experience, I always got very quick replies
> when doing this.

For me on the recipient side it's more a question of if you get any at
all.

> I don't see your point about copying more information though, I
> would just open the mbox in my mail client to reply and paste the
> content of the bisection report.  With a bit more work this could
> be fully automated but that should be part of the bisection
> rework using the new API & pipeline so sometime later in 2023...

If I'm manually pasing stuff I either have to quote it by hand or feel
like I need to edit the automatically generated bits.

> > I do notice that the Renesas tree tends to get a *lot* of the bisection
> > reports generated for some reason (vastly more than any other tree
> > including mainline or -next), however this wasn't sent based on the tree
> > at all - I just looked at the people involved with the commit.

> In the past month, there were 15 bisection reports on renesas, 7
> on linux-next and 28 on mainline for a total of 79 so 29 in other
> trees.  So it's true renesas is getting quite a lot of them, it's
> not entirely clear to me why that's the case but it's worth
> investigating a bit.

Yeah, that's vastly more than I'd expect and the overwhelming majority
of them are quite clearly not specific to the Renesas tree (things like
bootrr failures for non-Renesas boards).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Geert Uytterhoeven]

Hi Guillaume,

On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker
<guillaume.tucker@collabora.com> wrote:
> On 14/12/2022 11:06, Geert Uytterhoeven wrote:
> > On Tue, Dec 13, 2022 at 5:58 PM Mark Brown <broonie@kernel.org> wrote:
> >> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
> >> The KernelCI bisection bot found regressions in at least two KMS tests
> >> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
> >> merged up mainline:
> >>
> >>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
> >>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
> >>
> >> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
> >> PSR-exit to disable transition").  I'm not *100%* sure I trust the
> >> bisection but it sure is suspicous that two separate bisects for related
> >> issues landed on the same commit.
> >
> > ... which is an old commit, added in v5.19-rc2, and which did not
> > enter through the renesas tree at all?
>
> Do you mean this report shouldn't have been sent to you?

Exactly.

> FYI The renesas tree got rebased and inherited a regression which
> got bisected.

Renesas/master is (usually) never rebased.
So when did this rebase happen, and why is it relevant?

>  The same thing probably happened to other trees.

Indeed, I expect any tree that merged v5.19-rc2 or later to contain
this regression.  That doesn't mean it's a good idea to tell all
consumers of v5.19-rc2 and later they got a regression (and make it
sound like the problem was introduced in their trees).

> Yes it would be good to have 2nd order regressions based on a
> reference branch (e.g. mainline in this case) in KernelCI but

Sorry, I don't know what is a "2nd order regression".

> that's for next year.  Regardless, it found a commit and the
> bisection looks legit.

Good. So please report it to the relevant parties.

While bisecting, as soon as you happen to arrive at a commit
that is already upstream...

    > git bisect start
    > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch
'renesas-next' into renesas-devel
    > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7
    > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag
'v6.1' into renesas-devel
    > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e
    > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag
'ata-6.0-rc2' of
git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata
    > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c
(which happens here  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)

... there is no point in "blaming" any of the bisection points before.

The git command to check this is (assumed "linus" is a remote pointing
to Linus' tree):

    git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c
linus/master

You can use similar commands to find the maintainer's tree for commits
that are in linux-next and in a for-next branch, but not yet upstream.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Guillaume Tucker]

On 14/12/2022 15:03, Geert Uytterhoeven wrote:
> Hi Guillaume,
> 
> On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker
> <guillaume.tucker@collabora.com> wrote:
>> On 14/12/2022 11:06, Geert Uytterhoeven wrote:
>>> On Tue, Dec 13, 2022 at 5:58 PM Mark Brown <broonie@kernel.org> wrote:
>>>> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
>>>> The KernelCI bisection bot found regressions in at least two KMS tests
>>>> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
>>>> merged up mainline:
>>>>
>>>>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
>>>>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
>>>>
>>>> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
>>>> PSR-exit to disable transition").  I'm not *100%* sure I trust the
>>>> bisection but it sure is suspicous that two separate bisects for related
>>>> issues landed on the same commit.
>>>
>>> ... which is an old commit, added in v5.19-rc2, and which did not
>>> enter through the renesas tree at all?
>>
>> Do you mean this report shouldn't have been sent to you?
> 
> Exactly.

OK.

Mark, how did you get the list of recipients?

There's a command for this btw, which was used when the reports
were automatically sent to the recipients before we reverted to
manual filtering to reduce the noise:

$ ./kci_bisect get_recipients --kdir ~/src/linux --commit ca871659ec1606d33b1e76de8d4cf924cf627e34
To:
  Douglas Anderson <dianders@chromium.org>
  Brian Norris <briannorris@chromium.org>
  Sean Paul <seanpaul@chromium.org>
Cc:
  Miaoqian Lin <linmq006@gmail.com>
  Andrzej Hajda <a.hajda@samsung.com>
  Jonas Karlman <jonas@kwiboo.se>
  Daniel Vetter <daniel@ffwll.ch>
  Robert Foss <robert.foss@linaro.org>
  Jernej Skrabec <jernej.skrabec@gmail.com>
  Heiko Stuebner <heiko@sntech.de>
  Sasha Levin <sashal@kernel.org>
  linux-kernel@vger.kernel.org
  dri-devel@lists.freedesktop.org
  Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
  Neil Armstrong <narmstrong@baylibre.com>
  Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  David Airlie <airlied@linux.ie>

As you can see, Geert is not listed there.

>> FYI The renesas tree got rebased and inherited a regression which
>> got bisected.
> 
> Renesas/master is (usually) never rebased.
> So when did this rebase happen, and why is it relevant?

Sorry then I guess it wasn't rebased but if mainline was merged
into the branch then it inherited the regressions from mainline
at that point.

>>  The same thing probably happened to other trees.
> 
> Indeed, I expect any tree that merged v5.19-rc2 or later to contain
> this regression.  That doesn't mean it's a good idea to tell all
> consumers of v5.19-rc2 and later they got a regression (and make it
> sound like the problem was introduced in their trees).

No, the issues aren't reported more than once.  And again, the
tree used to do the bisection is not relevant as what matters is
the commit that it found.

>> Yes it would be good to have 2nd order regressions based on a
>> reference branch (e.g. mainline in this case) in KernelCI but
> 
> Sorry, I don't know what is a "2nd order regression".

Regressions are basically a delta between results in a given
revision and the previous one on the same branch (new failures).
What I call "2nd order" regressions are a delta of these
regressions compared to another reference branch.  For example,
regressions that are in a particular tree and aren't also in
mainline such as the one at hand which isn't specific to renesas.

>> that's for next year.  Regardless, it found a commit and the
>> bisection looks legit.
> 
> Good. So please report it to the relevant parties.
> 
> While bisecting, as soon as you happen to arrive at a commit
> that is already upstream...
> 
>     > git bisect start
>     > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch
> 'renesas-next' into renesas-devel
>     > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7
>     > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag
> 'v6.1' into renesas-devel
>     > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e
>     > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag
> 'ata-6.0-rc2' of
> git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata
>     > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c
> (which happens here  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)
> 
> ... there is no point in "blaming" any of the bisection points before.
> 
> The git command to check this is (assumed "linus" is a remote pointing
> to Linus' tree):
> 
>     git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c
> linus/master
> 
> You can use similar commands to find the maintainer's tree for commits
> that are in linux-next and in a for-next branch, but not yet upstream.

I think it won't be to implement this as soon as we start
tracking regressions specific to each tree since we'll have valid
good/bad revisions that are relevant to the tree in the first
place (if I understand correctly what you mean here).

Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 3281 bytes --]

On Wed, Dec 14, 2022 at 03:16:33PM +0100, Guillaume Tucker wrote:

> Mark, how did you get the list of recipients?

> There's a command for this btw, which was used when the reports
> were automatically sent to the recipients before we reverted to
> manual filtering to reduce the noise:

My standard thing is to look at who touched the commit, possibly also
adding seemingly relevant maintainers depending on how good the list
from the commit was (IIRC in this case the commit went entirely through
ChromeOS people so I added relevant DRM submaintainers which turned out
to be a surprisingly large number of people), and relevant lists.

> As you can see, Geert is not listed there.

I didn't send the report to Geert as far as I can see, I imagine he saw
it as a result of it going to one of the lists and noticed the mention
of Renesas as the tree, possibly he's got some filter set up to find
things that mention it.  The recipient list I have is:

| To: kernelci-results@groups.io, bot@kernelci.org, Brian Norris
|         <briannorris@chromium.org>, Sean Paul <seanpaul@chromium.org>, Douglas
|         Anderson <dianders@chromium.org>
| Cc: gtucker@collabora.com, dri-devel@lists.freedesktop.org,
|         linux-arm-kernel@lists.infradead.org, Andrzej Hajda
|         <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>,
|         Robert Foss <robert.foss@linaro.org>, Laurent Pinchart
|         <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>,
|         Jernej Skrabec <jernej.skrabec@gmail.com>

which doesn't mention him at all.

> >> Yes it would be good to have 2nd order regressions based on a
> >> reference branch (e.g. mainline in this case) in KernelCI but
> > 
> > Sorry, I don't know what is a "2nd order regression".
> 
> Regressions are basically a delta between results in a given
> revision and the previous one on the same branch (new failures).
> What I call "2nd order" regressions are a delta of these
> regressions compared to another reference branch.  For example,
> regressions that are in a particular tree and aren't also in
> mainline such as the one at hand which isn't specific to renesas.

Like I said in the other mail there is something going on which means
that a *very* lerge proportion of our bisection results are found in the
Renesas tree.

> >> that's for next year.  Regardless, it found a commit and the
> >> bisection looks legit.

> > Good. So please report it to the relevant parties.

That's what I did...

> > While bisecting, as soon as you happen to arrive at a commit
> > that is already upstream...

> > 'ata-6.0-rc2' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata
> >     > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c
> > (which happens here  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)

> > ... there is no point in "blaming" any of the bisection points before.

I'm not sure I understand the logic here?  The goal with a bisection is
to identify which commit caused an issue to aid in resolving whatever
the problem is.  It doesn't really matter if this happens before or
after the commit lands in Linus' tree.  I do agree that the tree
mentioned becomes a bit misleading though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Geert Uytterhoeven]

Hi Mark,

On Wed, Dec 14, 2022 at 3:39 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 14, 2022 at 03:16:33PM +0100, Guillaume Tucker wrote:
> > Mark, how did you get the list of recipients?
>
> > There's a command for this btw, which was used when the reports
> > were automatically sent to the recipients before we reverted to
> > manual filtering to reduce the noise:
>
> My standard thing is to look at who touched the commit, possibly also
> adding seemingly relevant maintainers depending on how good the list
> from the commit was (IIRC in this case the commit went entirely through
> ChromeOS people so I added relevant DRM submaintainers which turned out
> to be a surprisingly large number of people), and relevant lists.
>
> > As you can see, Geert is not listed there.
>
> I didn't send the report to Geert as far as I can see, I imagine he saw
> it as a result of it going to one of the lists and noticed the mention
> of Renesas as the tree, possibly he's got some filter set up to find
> things that mention it.  The recipient list I have is:
>
> | To: kernelci-results@groups.io, bot@kernelci.org, Brian Norris
> |         <briannorris@chromium.org>, Sean Paul <seanpaul@chromium.org>, Douglas
> |         Anderson <dianders@chromium.org>
> | Cc: gtucker@collabora.com, dri-devel@lists.freedesktop.org,
> |         linux-arm-kernel@lists.infradead.org, Andrzej Hajda
> |         <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>,
> |         Robert Foss <robert.foss@linaro.org>, Laurent Pinchart
> |         <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>,
> |         Jernej Skrabec <jernej.skrabec@gmail.com>
>
> which doesn't mention him at all.

Right. I noticed the email because my name was in the body (it's part
of the git repo name).
The "Re: renesas/master bisection" in the subject immediately triggered
my interest.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Geert Uytterhoeven]

Hi Guillaume,

On Wed, Dec 14, 2022 at 3:16 PM Guillaume Tucker
<guillaume.tucker@collabora.com> wrote:
> On 14/12/2022 15:03, Geert Uytterhoeven wrote:
> > On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker
> > <guillaume.tucker@collabora.com> wrote:
> >> On 14/12/2022 11:06, Geert Uytterhoeven wrote:
> >>> On Tue, Dec 13, 2022 at 5:58 PM Mark Brown <broonie@kernel.org> wrote:
> >>>> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
> >>>> The KernelCI bisection bot found regressions in at least two KMS tests
> >>>> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
> >>>> merged up mainline:
> >>>>
> >>>>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked
> >>>>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
> >>>>
> >>>> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
> >>>> PSR-exit to disable transition").  I'm not *100%* sure I trust the
> >>>> bisection but it sure is suspicous that two separate bisects for related
> >>>> issues landed on the same commit.
> >>>
> >>> ... which is an old commit, added in v5.19-rc2, and which did not
> >>> enter through the renesas tree at all?
> >>
> >> Do you mean this report shouldn't have been sent to you?
> >
> > Exactly.

> As you can see, Geert is not listed there.

Sorry, it was indeed not sent explicitly to me, but I was mentioned,
so I noticed.

> >> FYI The renesas tree got rebased and inherited a regression which
> >> got bisected.
> >
> > Renesas/master is (usually) never rebased.
> > So when did this rebase happen, and why is it relevant?
>
> Sorry then I guess it wasn't rebased but if mainline was merged
> into the branch then it inherited the regressions from mainline
> at that point.

Yep.

> >>  The same thing probably happened to other trees.
> >
> > Indeed, I expect any tree that merged v5.19-rc2 or later to contain
> > this regression.  That doesn't mean it's a good idea to tell all
> > consumers of v5.19-rc2 and later they got a regression (and make it
> > sound like the problem was introduced in their trees).
>
> No, the issues aren't reported more than once.  And again, the
> tree used to do the bisection is not relevant as what matters is
> the commit that it found.
>
> >> Yes it would be good to have 2nd order regressions based on a
> >> reference branch (e.g. mainline in this case) in KernelCI but
> >
> > Sorry, I don't know what is a "2nd order regression".
>
> Regressions are basically a delta between results in a given
> revision and the previous one on the same branch (new failures).
> What I call "2nd order" regressions are a delta of these
> regressions compared to another reference branch.  For example,
> regressions that are in a particular tree and aren't also in
> mainline such as the one at hand which isn't specific to renesas.

This regression must also be present in mainline (in v6.1).

> >> that's for next year.  Regardless, it found a commit and the
> >> bisection looks legit.
> >
> > Good. So please report it to the relevant parties.
> >
> > While bisecting, as soon as you happen to arrive at a commit
> > that is already upstream...
> >
> >     > git bisect start
> >     > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch
> > 'renesas-next' into renesas-devel
> >     > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7
> >     > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag
> > 'v6.1' into renesas-devel
> >     > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e
> >     > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag
> > 'ata-6.0-rc2' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata
> >     > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c
> > (which happens here  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)
> >
> > ... there is no point in "blaming" any of the bisection points before.
> >
> > The git command to check this is (assumed "linus" is a remote pointing
> > to Linus' tree):
> >
> >     git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c
> > linus/master
> >
> > You can use similar commands to find the maintainer's tree for commits
> > that are in linux-next and in a for-next branch, but not yet upstream.
>
> I think it won't be to implement this as soon as we start
> tracking regressions specific to each tree since we'll have valid
> good/bad revisions that are relevant to the tree in the first
> place (if I understand correctly what you mean here).

My point is that regressions should be reported against the tree where
they truly originated, not against a random tree that merged upstream.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

On Tue, Dec 13, 2022 at 04:51:11PM +0000, Mark Brown wrote:
> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
> 
> The KernelCI bisection bot found regressions in at least two KMS tests
> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
> merged up mainline:
> 
>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked 
>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
> 
> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
> PSR-exit to disable transition").  I'm not *100%* sure I trust the
> bisection but it sure is suspicous that two separate bisects for related
> issues landed on the same commit.
> 
> Below is the full report for the bisect for the first test, the bisect
> for the latter looks identical.  It's got links to full logs for the
> test run and a Reported-by for the bot - I do see some backtraces from
> userspace in the output, the first is:

I think the backtraces are just because IGT calls assert().

> | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
> | <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
> | Starting subtest: short-buffer-wakeup
> | 
> | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
> | (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
> | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
> | 
> | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
> | Stack trace:
> | 
> |   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
> |   #1 [<unknown>+0xd5362770]
> |   #2 [<unknown>+0xd536193c]
> |   #3 [__libc_start_main+0xe8]
> |   #4 [<unknown>+0xd5361974]
> |   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
> | Subtest short-buffer-wakeup failed.
> 
> Unfortunately we don't have current results from mainline or -next.

Thanks for the notification. I'm running:

  6e516faf0431 drm/panfrost: Job should reference MMU not file_priv

which is allegedly a "good" kernel. And running this:

  ### First kill my display manager, etc.
  ## Then:
  IGT_FORCE_DRIVER=rockchip /usr/libexec/igt-gpu-tools/kms_vblank --run-subtest pipe-A-wait-forked

Gives the appended log [1]. If I'm looking right, it seems like it's
failing the same way as the "regression."

I also tested this:

  # the 5.10.x backport, and its parent:
  dbe04e874d4fbd56be64fdfcb29410241b6ad08a^ -- i.e.:
  61297ee0c329 Input: bcm5974 - set missing URB_NO_TRANSFER_DMA_MAP urb flag

and saw the same failures, and I also see the failures I was trying to
avoid in this series (see e54a4424925a ("drm/atomic: Force bridge
self-refresh-exit on CRTC switch")). Perhaps that's because of the
"First kill my display manager, etc." -- because that step means we
might end up switching CRTCs (VOPs) when the test starts, and triggering
the bug.

I'll look some more, but this might be a difference of test setup, such
that my setup has the issue before and after that commit, but your setup
sees a regression.

Small tangent: It's possible this is similar to stuff I had to debug a
while back in this space:

atomictest: Disable CRTCs before test
https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/3309960
null_platform_test: Disable CRTCs first
https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/3315478

In that case, there was actually an underlying kernel regression due to:

  846c7dfc1193 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.

But our tests were broken too (assuming an initial state that wasn't
guaranteed), so we just fixed the tests.

Anyway, I'll confirm when I get some fresh eyes and can try a few more
things (like neutering the ChromeOS display framework for my tests).

Brian

[1]

IGT-Version: 1.26-gf8a4a0b5 (arm) (Linux: 5.18.0-rc6+ aarch64)
Starting subtest: pipe-A-wait-forked
Beginning pipe-A-wait-forked on pipe A, connector eDP-1
(kms_vblank:2532) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2535) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2531) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2532) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2534) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2535) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2531) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2536) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2532) CRITICAL: Last errno: 22, Invalid argument
(kms_vblank:2534) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2536) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2532) CRITICAL: error: -22 != 0
(kms_vblank:2531) CRITICAL: Last errno: 22, Invalid argument
(kms_vblank:2535) CRITICAL: Last errno: 22, Invalid argument
(kms_vblank:2531) CRITICAL: error: -22 != 0
(kms_vblank:2534) CRITICAL: Last errno: 22, Invalid argument
(kms_vblank:2536) CRITICAL: Last errno: 22, Invalid argument
(kms_vblank:2535) CRITICAL: error: -22 != 0
(kms_vblank:2534) CRITICAL: error: -22 != 0
(kms_vblank:2536) CRITICAL: error: -22 != 0
(kms_vblank:2533) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319:
(kms_vblank:2533) CRITICAL: Failed assertion: wait_vblank(fd, &vbl) == 0
(kms_vblank:2533) CRITICAL: Last errno: 22, Invalid argument
Stack trace:
Stack trace:
(kms_vblank:2533) CRITICAL: error: -22 != 0
Stack trace:
Stack trace:
Stack trace:
Stack trace:
child 1 failed with exit status 98
Subtest pipe-A-wait-forked failed.
**** DEBUG ****
(kms_vblank:2526) igt_kms-DEBUG: display: eDP-1: set_pipe(A)
(kms_vblank:2526) igt_kms-DEBUG: display: eDP-1: Selecting pipe A
(kms_vblank:2526) igt_fb-DEBUG: igt_create_fb_with_bo_size(width=2400, height=1600, format=XR24(0x34325258), modifier=0x0, size=0)
(kms_vblank:2526) igt_fb-DEBUG: igt_create_fb_with_bo_size(handle=1, pitch=9600)
(kms_vblank:2526) ioctl_wrappers-DEBUG: Test requirement passed: igt_has_fb_modifiers(fd)
(kms_vblank:2526) igt_fb-DEBUG: Test requirement passed: cairo_surface_status(fb->cairo_surface) == CAIRO_STATUS_SUCCESS
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display: A.0: plane_set_fb(56)
(kms_vblank:2526) igt_kms-DEBUG: display: A.0: plane_set_size (2400x1600)
(kms_vblank:2526) igt_kms-DEBUG: display: A.0: fb_set_position(0,0)
(kms_vblank:2526) igt_kms-DEBUG: display: A.0: fb_set_size(2400x1600)
(kms_vblank:2526) igt_kms-DEBUG: display: commit {
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe A, plane 0
(kms_vblank:2526) igt_kms-DEBUG: display:     eDP-1: SetCrtc pipe A, fb 56, src (0, 0), mode 2400x1600
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe A, plane 1
(kms_vblank:2526) igt_kms-DEBUG: display:     SetCursor pipe A, disabling
(kms_vblank:2526) igt_kms-DEBUG: display:     MoveCursor pipe A, (0, 0)
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe B, plane 0
(kms_vblank:2526) igt_kms-DEBUG: display:     SetCrtc pipe B, disabling
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe B, plane 1
(kms_vblank:2526) igt_kms-DEBUG: display:     SetPlane pipe B, plane 1, disabling
(kms_vblank:2526) igt_kms-DEBUG: display:     SetProp plane B.1 "rotation" to 0x1/1
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe B, plane 2
(kms_vblank:2526) igt_kms-DEBUG: display:     SetPlane pipe B, plane 2, disabling
(kms_vblank:2526) igt_kms-DEBUG: display:     SetProp plane B.2 "rotation" to 0x1/1
(kms_vblank:2526) igt_kms-DEBUG: Test requirement passed: plane_idx >= 0 && plane_idx < pipe->n_planes
(kms_vblank:2526) igt_kms-DEBUG: display:     Fixing up initial rotation pipe B, plane 3
(kms_vblank:2526) igt_kms-DEBUG: display:     SetCursor pipe B, disabling
(kms_vblank:2526) igt_kms-DEBUG: display:     MoveCursor pipe B, (0, 0)
(kms_vblank:2526) igt_kms-DEBUG: display: }
(kms_vblank:2526) igt_debugfs-DEBUG: Opening debugfs directory '/sys/kernel/debug/dri/1'
(kms_vblank:2526) INFO: Beginning pipe-A-wait-forked on pipe A, connector eDP-1
(kms_vblank:2526) DEBUG: Spawning 6 threads
****  END  ****
Subtest pipe-A-wait-forked: FAIL (0.938s)
Test requirement not met in function igt_require_pipe, file ../igt-gpu-tools-1.25/lib/igt_kms.c:2360:
Test requirement: !(pipe >= display->n_pipes || !display->pipes[pipe].enabled)
Pipe C does not exist or not enabled
Test requirement not met in function igt_require_pipe, file ../igt-gpu-tools-1.25/lib/igt_kms.c:2360:
Test requirement: !(pipe >= display->n_pipes || !display->pipes[pipe].enabled)
Pipe D does not exist or not enabled
Test requirement not met in function igt_require_pipe, file ../igt-gpu-tools-1.25/lib/igt_kms.c:2360:
Test requirement: !(pipe >= display->n_pipes || !display->pipes[pipe].enabled)
Pipe E does not exist or not enabled
Test requirement not met in function igt_require_pipe, file ../igt-gpu-tools-1.25/lib/igt_kms.c:2360:
Test requirement: !(pipe >= display->n_pipes || !display->pipes[pipe].enabled)
Pipe F does not exist or not enabled

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

Hi Mark, Sean, (and dri-devel)

On Wed, Dec 14, 2022 at 07:04:37PM -0800, Brian Norris wrote:
> On Tue, Dec 13, 2022 at 04:51:11PM +0000, Mark Brown wrote:
> > On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
> > 
> > The KernelCI bisection bot found regressions in at least two KMS tests
> > in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
> > merged up mainline:
> > 
> >    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked 
> >    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
> > 
> > which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
> > PSR-exit to disable transition").  I'm not *100%* sure I trust the
> > bisection but it sure is suspicous that two separate bisects for related
> > issues landed on the same commit.

...

> > | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
> > | <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
> > | Starting subtest: short-buffer-wakeup
> > | 
> > | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
> > | (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
> > | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
> > | 
> > | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
> > | Stack trace:
> > | 
> > |   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
> > |   #1 [<unknown>+0xd5362770]
> > |   #2 [<unknown>+0xd536193c]
> > |   #3 [__libc_start_main+0xe8]
> > |   #4 [<unknown>+0xd5361974]
> > |   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
> > | Subtest short-buffer-wakeup failed.

...

> I'll look some more, but this might be a difference of test setup, such
> that my setup has the issue before and after that commit, but your setup
> sees a regression.

I believe this is the case; things are broken both before and after, but
depending on test sequencing, etc., they might have seemed less broken
before.

When we're failing, we're getting EINVAL from drm_vblank_get(). That
essentially means that vblank has been disabled (drm_crtc_vblank_off()).
As it happens, this is exactly what we do when we enter PSR (Panel Self
Refresh).

Now, these test cases (especially 'kms_vblank.pipe-A-wait-forked') seem
to display a static image, and then wait for a bunch of vblank events.
This is exactly the sort of case where we should enter PSR, and so we're
likely to disable vblank events. Thus, if everything is working right,
we will often hit EINVAL in ioctl(DRM_IOCTL_WAIT_VBLANK) ... ->
drm_vblank_get(), and fail the test.

As to why this appears to be a regression: like mentioned in my previous
mail, these tests tend to hose the Analogix PSR state before my patch
set. When PSR is hosed, we tend to stay with PSR disabled, and so
drm_vblank_get() doesn't return EINVAL, and the test is happy. (Never
mind that the display is likely non-functional.) [0]

So how to fix this?

1. ignore it; it's "just" a test failure, IIUC [1]
2. change the test, to skip this test if the device has PSR
3. leave vblank enabled even in the presence of PSR
4. otherwise modify the vblank ioctl interface, such that one can "wait"
   for a vblank that won't come (because the display is in self-refresh
   / there are no new frames or vblanks)

I don't know how to do #2, because this variant of PSR is intentionally
opaque to user space.

For #3: the downstream PSR support that these systems shipped with
initially did not disable vblank in PSR. But that was massively
rewritten/refactored by Sean Paul before it made it upstream, and now it
does. I tried briefly to factor that part out
(drivers/gpu/drm/rockchip/rockchip_drm_vop.c /
drm_crtc_vblank_{on,off}()), but because drm_self_refresh_helper.c
(ab?)uses the commit step to "disable" the CRTC for PSR (even though the
CRTC is not fully disabled), I tend to hit this in
drm_atomic_helper_commit_modeset_disables()->disable_outputs():

		ret = drm_crtc_vblank_get(crtc);
		WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n");

And I hit a few more problems too...

...I'm sure I could hack my way through this somehow, but this is all
sounding like it could use some advice from someone more familiar with
DRM and/or the drm_self_refresh_helper design. I've learned my way
around this a bit by necessity, but I still feel like I don't know all
the right answers here. (*cough*, Sean?)

Brian

[0] I definitely reproduce this part locally, before my patches. I can't
find non-expired CI logs for "passing" runs to be sure that's what the
CI is seeing though.

[1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI
    contract in the presence of PSR, but I believe the upstream PSR
    support has always worked this way. I could imagine
    DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here
    though.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Michel Dänzer]

On 12/21/22 23:02, Brian Norris wrote:
> Hi Mark, Sean, (and dri-devel)
> 
> On Wed, Dec 14, 2022 at 07:04:37PM -0800, Brian Norris wrote:
>> On Tue, Dec 13, 2022 at 04:51:11PM +0000, Mark Brown wrote:
>>> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote:
>>>
>>> The KernelCI bisection bot found regressions in at least two KMS tests
>>> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree
>>> merged up mainline:
>>>
>>>    igt-kms-rockchip.kms_vblank.pipe-A-wait-forked 
>>>    igt-kms-rockchip.kms_vblank.pipe-A-query-busy
>>>
>>> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support
>>> PSR-exit to disable transition").  I'm not *100%* sure I trust the
>>> bisection but it sure is suspicous that two separate bisects for related
>>> issues landed on the same commit.
> 
> ...
> 
>>> | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64)
>>> | <14>[   35.444448] [IGT] drm_read: starting subtest short-buffer-wakeup
>>> | Starting subtest: short-buffer-wakeup
>>> | 
>>> | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, file ../tests/drm_read.c:65:
>>> | (drm_read:350) CRITICAL: <14>[   36.155642] [IGT] drm_read: exiting, ret=98
>>> | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT)
>>> | 
>>> | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument
>>> | Stack trace:
>>> | 
>>> |   #0 ../lib/igt_core.c:1933 __igt_fail_assert()
>>> |   #1 [<unknown>+0xd5362770]
>>> |   #2 [<unknown>+0xd536193c]
>>> |   #3 [__libc_start_main+0xe8]
>>> |   #4 [<unknown>+0xd5361974]
>>> |   #5 [<unknown<6>[   36.162851] Console: switching to colour frame buffer device 300x100>+0xd5361974]
>>> | Subtest short-buffer-wakeup failed.
> 
> ...
> 
>> I'll look some more, but this might be a difference of test setup, such
>> that my setup has the issue before and after that commit, but your setup
>> sees a regression.
> 
> I believe this is the case; things are broken both before and after, but
> depending on test sequencing, etc., they might have seemed less broken
> before.
> 
> When we're failing, we're getting EINVAL from drm_vblank_get(). That
> essentially means that vblank has been disabled (drm_crtc_vblank_off()).
> As it happens, this is exactly what we do when we enter PSR (Panel Self
> Refresh).
> 
> Now, these test cases (especially 'kms_vblank.pipe-A-wait-forked') seem
> to display a static image, and then wait for a bunch of vblank events.
> This is exactly the sort of case where we should enter PSR, and so we're
> likely to disable vblank events. Thus, if everything is working right,
> we will often hit EINVAL in ioctl(DRM_IOCTL_WAIT_VBLANK) ... ->
> drm_vblank_get(), and fail the test.
> 
> As to why this appears to be a regression: like mentioned in my previous
> mail, these tests tend to hose the Analogix PSR state before my patch
> set. When PSR is hosed, we tend to stay with PSR disabled, and so
> drm_vblank_get() doesn't return EINVAL, and the test is happy. (Never
> mind that the display is likely non-functional.) [0]
> 
> So how to fix this?
> 
> 1. ignore it; it's "just" a test failure, IIUC [1]
> 2. change the test, to skip this test if the device has PSR
> 3. leave vblank enabled even in the presence of PSR
> 4. otherwise modify the vblank ioctl interface, such that one can "wait"
>    for a vblank that won't come (because the display is in self-refresh
>    / there are no new frames or vblanks)

FWIW, a couple more alternatives:

5. Go/stay out of PSR while vblank interrupts are enabled (probably want to make sure vblankoffdelay is set up such that vblank interrupts are disabled ASAP)
6. Use fallback timers for vblank events while in PSR (there might be some infrastructure for this, since some drivers don't have real vblank interrupts)


> [1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI
>     contract in the presence of PSR, but I believe the upstream PSR
>     support has always worked this way. I could imagine
>     DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here
>     though.

Yeah, that's pretty likely to cause issues with existing real-world user space.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

Hi Michel,

Thanks for your thoughts. I'll give my attempt at weighing my and your
options, with the caveat that I'm still not much of a DRM expert.

On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote:
> On 12/21/22 23:02, Brian Norris wrote:
> > So how to fix this?
> > 
> > 1. ignore it; it's "just" a test failure, IIUC [1]

Probably discarded, per Michel's notes.

> > 2. change the test, to skip this test if the device has PSR

Doesn't seem great, and not just because PSR is not directly detectable
in user space.

> > 3. leave vblank enabled even in the presence of PSR

I'm leaning toward this.

> > 4. otherwise modify the vblank ioctl interface, such that one can "wait"
> >    for a vblank that won't come (because the display is in self-refresh
> >    / there are no new frames or vblanks)

That doesn't sound so great of an API, to essentially induce hangs,
pending other activity. (Assuming I understand the DRM APIs here
correctly.)

> FWIW, a couple more alternatives:
> 
> 5. Go/stay out of PSR while vblank interrupts are enabled (probably want to make sure vblankoffdelay is set up such that vblank interrupts are disabled ASAP)

That seems not extremely nice, to waste power. Based on the earlier
downstream implementation (which left vblank interrupts enabled), I'd
wager there's a much larger power win from PSR (on the display-transport
and memory-bandwidth costs), relative to the power cost of vblank
interrupts.

Also, messing with vblankoffdelay sounds likely to trigger the races
mentioned in the drm_vblank.c kerneldoc.

> 6. Use fallback timers for vblank events while in PSR (there might be some infrastructure for this, since some drivers don't have real vblank interrupts)

There's drm_atomic_helper_fake_vblank(), but I don't think that's really
hooked up to a timer. That's potentially promising though.

> > [1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI
> >     contract in the presence of PSR, but I believe the upstream PSR
> >     support has always worked this way. I could imagine
> >     DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here
> >     though.
> 
> Yeah, that's pretty likely to cause issues with existing real-world user space.

OK. Any hints as to what kind of user space uses this? A cursory look
doesn't show that any of the ChromeOS user space uses it, which may be
why it was overlooked in the initial PSR development and upstreaming.
I'm in part simply curious, but I'm also wondering what the
error-handling and reliability (e.g., what if vblanks don't come?)
expectations might be in practice.

All in all, it's seeming like maybe option 3 or 6 are best. I'd lean
toward 3, assuming I can hack my way through "driver forgot to call
drm_crtc_vblank_off()" and similar problems, in part because it's ground
that has partially been tread before. I hope that doesn't complicate the
DRM state machine even more though, now that I'll have to better
coordinate the "enabled"/"self_refresh_active" states with "vblank
enabled"...

Thoughts still welcome of course.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Michel Dänzer]

On 1/4/23 03:11, Brian Norris wrote:
> On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote:
>> On 12/21/22 23:02, Brian Norris wrote:
> 
>>> 3. leave vblank enabled even in the presence of PSR
> 
> I'm leaning toward this.

If this means vblank interrupts will arrive as expected even while in PSR, that may be the ideal solution indeed.


>> 5. Go/stay out of PSR while vblank interrupts are enabled (probably want to make sure vblankoffdelay is set up such that vblank interrupts are disabled ASAP)
> 
> That seems not extremely nice, to waste power. Based on the earlier
> downstream implementation (which left vblank interrupts enabled), I'd
> wager there's a much larger power win from PSR (on the display-transport
> and memory-bandwidth costs), relative to the power cost of vblank
> interrupts.
> 
> Also, messing with vblankoffdelay sounds likely to trigger the races
> mentioned in the drm_vblank.c kerneldoc.

Not sure how likely that is, quite a few drivers are setting dev->vblank_disable_immediate = true.

With that, vblank interrupts should generally be enabled only while there are screen updates as well[0], in which case PSR shouldn't be possible anyway.

[0] There may be user space which uses the vblank ioctls even while there are no screen updates though, which would prevent PSR in this case.


>>> [1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI
>>>     contract in the presence of PSR, but I believe the upstream PSR
>>>     support has always worked this way. I could imagine
>>>     DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here
>>>     though.
>>
>> Yeah, that's pretty likely to cause issues with existing real-world user space.
> 
> OK. Any hints as to what kind of user space uses this?

I don't have any specific example, just thinking about how user space could respond to the vblank ioctls returning an error, and it would seem to be generally either of:

* Just run non-throttled, which might negate any energy savings from PSR
* Fail to work altogether


> I'm in part simply curious, but I'm also wondering what the
> error-handling and reliability (e.g., what if vblanks don't come?)
> expectations might be in practice.

If vblank events never come, user space might freeze.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

On Wed, Jan 04, 2023 at 10:11:46AM +0100, Michel Dänzer wrote:
> On 1/4/23 03:11, Brian Norris wrote:
> > On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote:
> >> On 12/21/22 23:02, Brian Norris wrote:
> > 
> >>> 3. leave vblank enabled even in the presence of PSR
> > 
> > I'm leaning toward this.
> 
> If this means vblank interrupts will arrive as expected even while in PSR, that may be the ideal solution indeed.

Yes. And I think I have a working patchset for this now. It passes all
the igt-gpu-tools/kms_vblank tests for me now, and I don't think it
causes any other issues. I'll send it out shortly, after a bit more
testing.

Side note: I believe this vblank-disabled issue actually came in as an
upstream regression at commit 6c836d965bad ("drm/rockchip: Use the
helpers for PSR"); before that, we were doing this very differently, and
didn't touch vblank at all for PSR, similar to the "downstream" stuff I
mentioned earlier.

> >> 5. Go/stay out of PSR while vblank interrupts are enabled (probably want to make sure vblankoffdelay is set up such that vblank interrupts are disabled ASAP)
> > 
> > That seems not extremely nice, to waste power. Based on the earlier
> > downstream implementation (which left vblank interrupts enabled), I'd
> > wager there's a much larger power win from PSR (on the display-transport
> > and memory-bandwidth costs), relative to the power cost of vblank
> > interrupts.
> > 
> > Also, messing with vblankoffdelay sounds likely to trigger the races
> > mentioned in the drm_vblank.c kerneldoc.
> 
> Not sure how likely that is, quite a few drivers are setting dev->vblank_disable_immediate = true.
> 
> With that, vblank interrupts should generally be enabled only while there are screen updates as well[0], in which case PSR shouldn't be possible anyway.
> 
> [0] There may be user space which uses the vblank ioctls even while there are no screen updates though, which would prevent PSR in this case.

OK. I'm just reading docs here; definitely not an expert.

> >>> [1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI
> >>>     contract in the presence of PSR, but I believe the upstream PSR
> >>>     support has always worked this way. I could imagine
> >>>     DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here
> >>>     though.
> >>
> >> Yeah, that's pretty likely to cause issues with existing real-world user space.
> > 
> > OK. Any hints as to what kind of user space uses this?
> 
> I don't have any specific example, just thinking about how user space could respond to the vblank ioctls returning an error, and it would seem to be generally either of:
> 
> * Just run non-throttled, which might negate any energy savings from PSR
> * Fail to work altogether
> 
> 
> > I'm in part simply curious, but I'm also wondering what the
> > error-handling and reliability (e.g., what if vblanks don't come?)
> > expectations might be in practice.
> 
> If vblank events never come, user space might freeze.

Thanks. If my patchset works out like I expect, this should no longer be
noticeable to user space, so I don't really have to test out your
educated guesses :)

Thanks for the idea-bouncing.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

On Thu, Jan 05, 2023 at 04:59:55PM -0800, Brian Norris wrote:
> On Wed, Jan 04, 2023 at 10:11:46AM +0100, Michel Dänzer wrote:
> > On 1/4/23 03:11, Brian Norris wrote:
> > > On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote:
> > >> On 12/21/22 23:02, Brian Norris wrote:
> > >>> 3. leave vblank enabled even in the presence of PSR
> > > 
> > > I'm leaning toward this.
> > 
> > If this means vblank interrupts will arrive as expected even while in PSR, that may be the ideal solution indeed.
> 
> Yes. And I think I have a working patchset for this now. It passes all
> the igt-gpu-tools/kms_vblank tests for me now, and I don't think it
> causes any other issues. I'll send it out shortly, after a bit more
> testing.

For the record, that's here:
https://lore.kernel.org/lkml/20230105174001.2.Ic07cba4ab9a7bd3618a9e4258b8f92ea7d10ae5a@changeid/

I didn't CC everyone who got this report. In included a:

  Reported-by: "kernelci.org bot" <bot@kernelci.org>

even though it didn't really bisect the right thing. Let me know if
there's a different/better way to give credit.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel