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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 02.09.25 um 10:30 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
>> HyperV's virtual hardware does not provide vblank interrupts. Use a
>> vblank timer to simulate the interrupt. Rate-limits the display's
>> update frequency to the display-mode settings. Avoids excessive CPU
>> overhead with compositors that do not rate-limit their output.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
> [...]
>
>>   
>> @@ -111,11 +113,15 @@ static void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>>   				crtc_state->mode.hdisplay,
>>   				crtc_state->mode.vdisplay,
>>   				plane_state->fb->pitches[0]);
>> +
>> +	drm_crtc_vblank_on(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,
>>   };
>>   
> I think your patch is correct due the driver not having an .atomic_disable
> callback. But looking at the driver, I see that its .atomic_enable does:
>
> static void hyperv_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>                                               struct drm_atomic_state *state)
> {
> ...
>          hyperv_update_situation(hv->hdev, 1,  hv->screen_depth,
>                                  crtc_state->mode.hdisplay,
>                                  crtc_state->mode.vdisplay,
>                                  plane_state->fb->pitches[0]);
> }
>
> and this function in turn does:
>
> int hyperv_update_situation(struct hv_device *hdev, u8 active, u32 bpp,
>                              u32 w, u32 h, u32 pitch)
> {
> ...
>          msg.situ.video_output[0].active = active;
> ...
> }
>
> So I wonder if it should instead have a custom .atomic_disable that calls:
>
>          hyperv_update_situation(hv->hdev, 0,  hv->screen_depth,
>                                  crtc_state->mode.hdisplay,
>                                  crtc_state->mode.vdisplay,
>                                  plane_state->fb->pitches[0]);
>
> 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.

Best regards
Thomas


>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)