From: Zack Rusin <zack.rusin@broadcom.com>
To: Ian Forbes <ian.forbes@broadcom.com>
Cc: dri-devel@lists.freedesktop.org,
bcm-kernel-feedback-list@broadcom.com,
martin.krastev@broadcom.com, maaz.mombasawala@broadcom.com
Subject: Re: [PATCH] drm/vmwgfx: Re-introduce drm_crtc_helper_funcs::prepare
Date: Fri, 3 May 2024 22:46:40 -0400 [thread overview]
Message-ID: <CABQX2QM=U+6HnO6k=3-=aV_katM20jOeDTWRAjiPSV_N_3Ktzw@mail.gmail.com> (raw)
In-Reply-To: <20240503222916.26552-1-ian.forbes@broadcom.com>
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
next prev parent reply other threads:[~2024-05-04 2:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-03 22:29 [PATCH] drm/vmwgfx: Re-introduce drm_crtc_helper_funcs::prepare Ian Forbes
2024-05-04 2:46 ` Zack Rusin [this message]
2024-05-06 13:00 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABQX2QM=U+6HnO6k=3-=aV_katM20jOeDTWRAjiPSV_N_3Ktzw@mail.gmail.com' \
--to=zack.rusin@broadcom.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=ian.forbes@broadcom.com \
--cc=maaz.mombasawala@broadcom.com \
--cc=martin.krastev@broadcom.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).