Michael Kelley <mhkli...@outlook.com> writes: Hello Michael,
> From: Javier Martinez Canillas <javi...@redhat.com> Sent: Tuesday, September > 2, 2025 8:41 AM >> >> Thomas Zimmermann <tzimmerm...@suse.de> writes: >> >> [...] >> >> >> >> >> I'm not familiar with hyperv to know whether is a problem or not for the >> >> host to not be notified that the guest display is disabled. But I thought >> >> that should raise this question for the folks familiar with it. >> > >> > The feedback I got at >> > https://lore.kernel.org/dri-devel/sn6pr02mb4157f630284939e084486afed4...@sn6pr02mb4157.namprd02.prod.outlook.com/ >> > >> > is that the vblank timer solves the problem of excessive CPU consumption >> > on hypervdrm. Ans that's also the observation I had with other drivers. >> > I guess, telling the host about the disabled display would still make >> > sense. >> > >> >> Yes, I read the other thread you referenced and that's why I said that >> your patch is correct to solve the issue. >> >> I just wanted to point out, since it could be that as a follow-up the >> driver could need its own .atomic_disable instead of using the default >> drm_crtc_vblank_atomic_disable(). Something like the following maybe: >> >> +static void hyperv_crtc_helper_atomic_disable(struct drm_crtc *crtc, >> + struct drm_atomic_state *state) >> +{ >> + struct hyperv_drm_device *hv = to_hv(crtc->dev); >> + struct drm_plane *plane = &hv->plane; >> + struct drm_plane_state *plane_state = plane->state; >> + struct drm_crtc_state *crtc_state = crtc->state; >> + >> + hyperv_hide_hw_ptr(hv->hdev); >> + /* Notify the host that the guest display is disabled */ >> + hyperv_update_situation(hv->hdev, 0, hv->screen_depth, >> + crtc_state->mode.hdisplay, >> + crtc_state->mode.vdisplay, >> + plane_state->fb->pitches[0]); >> + >> + drm_crtc_vblank_off(crtc); >> +} >> + >> static const struct drm_crtc_helper_funcs hyperv_crtc_helper_funcs = { >> .atomic_check = drm_crtc_helper_atomic_check, >> .atomic_flush = drm_crtc_vblank_atomic_flush, >> .atomic_enable = hyperv_crtc_helper_atomic_enable, >> - .atomic_disable = drm_crtc_vblank_atomic_disable, >> + .atomic_disable = hyperv_crtc_helper_atomic_disable, >> }; > > I have some historical expertise in the Hyper-V fbdev driver from > back when I was a Microsoft employee (I'm now retired). The fbdev > driver is similar to the DRM driver in that it tells the Hyper-V host > that the device is "active" during initial setup, but there's never a > time when the driver tells Hyper-V that the device is "not active". > > I agree that symmetry suggests having disable function that sets > "active" to 0, but I don't know what the effect would be. I don't know > if Hyper-V anticipates any circumstances when the driver should tell > Hyper-V the device is not active. My chances are not good in finding > someone on the Hyper-V team who could give a definitive answer, > as it's probably an area that is not under active development. The > Hyper-V VMBus frame buffer device functionality is what it is, and > isn't likely to be getting enhancements. > > I suggest that we assume it's not necessary to add a "disable" > function, and proceed with Thomas' proposed changes to the Hyper-V > DRM driver. Adding "disable" now risks breaking something due > to effects we're unaware of. If in the future the need arises to mark > the device not active, the "disable" function can be added after > a clarifying conversation with the Hyper-V team. > > If anyone at Microsoft wants to chime in, please do so. :-) > Thanks a lot for the insight. I agree that probably is not worth the risk to notify of a display disable, since is unclear what the effect would be. > Michael > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
