RE: [PATCH v2 4/4] drm/hypervdrm: Use vblank timer

[Javier Martinez Canillas <javierm@redhat.com>]

Michael Kelley <mhklinux@outlook.com> writes:

Hello Michael,

> From: Javier Martinez Canillas <javierm@redhat.com> Sent: Tuesday, September 2, 2025 8:41 AM
>> 
>> Thomas Zimmermann <tzimmermann@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/SN6PR02MB4157F630284939E084486AFED46FA@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