diff mbox series

drm/vmwgfx: Re-introduce drm_crtc_helper_funcs::prepare

Message ID 20240503222916.26552-1-ian.forbes@broadcom.com (mailing list archive)
State New, archived
Headers show
Series drm/vmwgfx: Re-introduce drm_crtc_helper_funcs::prepare | expand

Commit Message

Ian Forbes May 3, 2024, 10:29 p.m. UTC 
This function was removed in the referenced fixes commit and caused a
regression. This is because the presence of this function, even though it
is a noop, changes the behaviour of disable_outputs in
drm_atomic_helper.c:1211.

Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation")
Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Zack Rusin May 4, 2024, 2:46 a.m. UTC | #1 
On Fri, May 3, 2024 at 6:29 PM Ian Forbes <ian.forbes@broadcom.com> wrote:
>
> This function was removed in the referenced fixes commit and caused a
> regression. This is because the presence of this function, even though it
> is a noop, changes the behaviour of disable_outputs in
> drm_atomic_helper.c:1211.
>
> Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation")
> Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 2041c4d48daa..37223f95cbec 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -409,6 +409,10 @@ static void vmw_stdu_crtc_mode_set_nofb(struct drm_crtc *crtc)
>                           crtc->x, crtc->y);
>  }
>
> +static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
> +{
> +}
> +
>  static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
>                                          struct drm_atomic_state *state)
>  {
> @@ -1463,6 +1467,7 @@ drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
>  };
>
>  static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
> +       .prepare = vmw_stdu_crtc_helper_prepare,
>         .mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
>         .atomic_check = vmw_du_crtc_atomic_check,
>         .atomic_begin = vmw_du_crtc_atomic_begin,
> --
> 2.34.1
>

Thanks, but that doesn't look correct. We do want to make sure the
drm_crtc_vblank_off is actually called when outputs are disabled.
Since this is my regression it's perfectly fine if you want to hand it
off to me and work on something else. In general you always want to
understand what the patch that you're sending is doing before sending
it. In this case it's pretty trivial, the commit you mention says that
it fixes kms_pipe_crc_basic and if you run it with your patch (e.g.
sudo ./kms_pipe_crc_basic --run-subtest disable-crc-after-crtc) you
should notice:
May 03 22:25:05 fedora.local kernel: ------------[ cut here ]------------
May 03 22:25:05 fedora.local kernel: driver forgot to call drm_crtc_vblank_off()
May 03 22:25:05 fedora.local kernel: WARNING: CPU: 2 PID: 2204 at
drivers/gpu/drm/drm_atomic_helper.c:1232 disable_outputs+0x345/0x350
May 03 22:25:05 fedora.local kernel: Modules linked in: snd_seq_dummy
snd_hrtimer nf_conntrack_netbios_ns nf_conntrack_broadcast
nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet
nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat
nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables
snd_seq_midi snd_seq_midi_event qrtr vsock_loopback
vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vsock
sunrpc binfmt_misc snd_ens1371 snd_ac97_codec ac97_bus snd_seq
intel_rapl_msr snd_pcm intel_rapl_common vmw_balloon
intel_uncore_frequency_common isst_if_mbox_msr isst_if_common gameport
snd_rawmidi snd_seq_device rapl snd_timer snd vmw_vmci pcspkr
soundcore i2c_piix4 pktcdvd joydev loop nfnetlink zram vmwgfx
crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni
polyval_generic nvme ghash_clmulni_intel nvme_core sha512_ssse3
sha256_ssse3 sha1_ssse3 drm_ttm_helper ttm nvme_auth vmxnet3 serio_raw
ata_generic pata_acpi fuse i2c_dev
May 03 22:25:05 fedora.local kernel: CPU: 2 PID: 2204 Comm:
kms_pipe_crc_ba Not tainted 6.9.0-rc2-vmwgfx #5
May 03 22:25:05 fedora.local kernel: Hardware name: VMware, Inc.
VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00
11/12/2020
May 03 22:25:05 fedora.local kernel: RIP: 0010:disable_outputs+0x345/0x350
... but in most cases it's not going to be so trivial. Whether you
decide to work on this one yourself or hand it off to me - we don't
want to trade bug for bug here, but fix both of those things.

z
Daniel Vetter May 6, 2024, 1 p.m. UTC | #2 
On Fri, May 03, 2024 at 10:46:40PM -0400, Zack Rusin wrote:
> On Fri, May 3, 2024 at 6:29 PM Ian Forbes <ian.forbes@broadcom.com> wrote:
> >
> > This function was removed in the referenced fixes commit and caused a
> > regression. This is because the presence of this function, even though it
> > is a noop, changes the behaviour of disable_outputs in
> > drm_atomic_helper.c:1211.
> >
> > Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation")
> > Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > index 2041c4d48daa..37223f95cbec 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > @@ -409,6 +409,10 @@ static void vmw_stdu_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >                           crtc->x, crtc->y);
> >  }
> >
> > +static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
> > +{
> > +}
> > +
> >  static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
> >                                          struct drm_atomic_state *state)
> >  {
> > @@ -1463,6 +1467,7 @@ drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
> >  };
> >
> >  static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
> > +       .prepare = vmw_stdu_crtc_helper_prepare,
> >         .mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
> >         .atomic_check = vmw_du_crtc_atomic_check,
> >         .atomic_begin = vmw_du_crtc_atomic_begin,
> > --
> > 2.34.1
> >
> 
> Thanks, but that doesn't look correct. We do want to make sure the
> drm_crtc_vblank_off is actually called when outputs are disabled.
> Since this is my regression it's perfectly fine if you want to hand it
> off to me and work on something else. In general you always want to
> understand what the patch that you're sending is doing before sending
> it. In this case it's pretty trivial, the commit you mention says that
> it fixes kms_pipe_crc_basic and if you run it with your patch (e.g.
> sudo ./kms_pipe_crc_basic --run-subtest disable-crc-after-crtc) you
> should notice:

Rather aside, but atomic helpers supporting the ->prepare callback in that
special way is kinda a remnant of the conversion helpers to move legacy
kms drivers to atomic.

Which means we might want to look into whether anyone still needs that, or
whether the ->atomic_disable hook is enough and then nuke that if-ladder
of compat code. Because as this bug shows, it's rather surprising special
case :-/
-Sima

> May 03 22:25:05 fedora.local kernel: ------------[ cut here ]------------
> May 03 22:25:05 fedora.local kernel: driver forgot to call drm_crtc_vblank_off()
> May 03 22:25:05 fedora.local kernel: WARNING: CPU: 2 PID: 2204 at
> drivers/gpu/drm/drm_atomic_helper.c:1232 disable_outputs+0x345/0x350
> May 03 22:25:05 fedora.local kernel: Modules linked in: snd_seq_dummy
> snd_hrtimer nf_conntrack_netbios_ns nf_conntrack_broadcast
> nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet
> nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat
> nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables
> snd_seq_midi snd_seq_midi_event qrtr vsock_loopback
> vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vsock
> sunrpc binfmt_misc snd_ens1371 snd_ac97_codec ac97_bus snd_seq
> intel_rapl_msr snd_pcm intel_rapl_common vmw_balloon
> intel_uncore_frequency_common isst_if_mbox_msr isst_if_common gameport
> snd_rawmidi snd_seq_device rapl snd_timer snd vmw_vmci pcspkr
> soundcore i2c_piix4 pktcdvd joydev loop nfnetlink zram vmwgfx
> crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni
> polyval_generic nvme ghash_clmulni_intel nvme_core sha512_ssse3
> sha256_ssse3 sha1_ssse3 drm_ttm_helper ttm nvme_auth vmxnet3 serio_raw
> ata_generic pata_acpi fuse i2c_dev
> May 03 22:25:05 fedora.local kernel: CPU: 2 PID: 2204 Comm:
> kms_pipe_crc_ba Not tainted 6.9.0-rc2-vmwgfx #5
> May 03 22:25:05 fedora.local kernel: Hardware name: VMware, Inc.
> VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00
> 11/12/2020
> May 03 22:25:05 fedora.local kernel: RIP: 0010:disable_outputs+0x345/0x350
> ... but in most cases it's not going to be so trivial. Whether you
> decide to work on this one yourself or hand it off to me - we don't
> want to trade bug for bug here, but fix both of those things.
> 
> z
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 2041c4d48daa..37223f95cbec 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -409,6 +409,10 @@  static void vmw_stdu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 			  crtc->x, crtc->y);
 }
 
+static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
+{
+}
+
 static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
 					 struct drm_atomic_state *state)
 {
@@ -1463,6 +1467,7 @@  drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
 };
 
 static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
+	.prepare = vmw_stdu_crtc_helper_prepare,
 	.mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
 	.atomic_check = vmw_du_crtc_atomic_check,
 	.atomic_begin = vmw_du_crtc_atomic_begin,