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

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

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat