Thanks for the feedbacks,
I will prepare a V2 based on your suggestions.
On 08/21, Daniel Vetter wrote:
On Tue, Aug 21, 2018 at 4:52 PM, Rodrigo Siqueira rodrigosiqueiramelo@gmail.com wrote:
On 08/21, Daniel Vetter wrote:
On Mon, Aug 20, 2018 at 08:55:44PM -0300, Rodrigo Siqueira wrote:
Currently, vkms needs VBlank to work well. This patch adds another operation model that make vkms works without VBlank support.
Signed-off-by: Rodrigo Siqueira rodrigosiqueiramelo@gmail.com
drivers/gpu/drm/vkms/vkms_crtc.c | 10 ++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 12 ++++++++++-- drivers/gpu/drm/vkms/vkms_drv.h | 1 + 3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index bfe6e0312cc4..001fa5c1d326 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -145,12 +145,22 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) {
- struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
- if (vkms_out->disable_vblank)
return;- drm_crtc_vblank_on(crtc);
}
static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) {
- struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
- if (vkms_out->disable_vblank)
return;- drm_crtc_vblank_off(crtc);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 5d78bd97e69c..f30c4113d4c4 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -21,6 +21,10 @@
static struct vkms_device *vkms_device;
+static bool disable_vblank; +module_param_named(disablevblank, disable_vblank, bool, 0444); +MODULE_PARM_DESC(disablevblank, "Disable vblank interrupt");
I think the info output is much clearer, I'd rename this to virtual_hw, with 0 as default, and a help text along of "Enable virtual HW mode (disables vblanks and immediately completes flips)."
You right, virtual_hw is much better :) I will fix it.
static const struct file_operations vkms_driver_fops = { .owner = THIS_MODULE, .open = drm_open, @@ -105,9 +109,13 @@ static int __init vkms_init(void) goto out_fini; }
- vkms_device->drm.irq_enabled = true;
- vkms_device->output.disable_vblank = disable_vblank;
- vkms_device->drm.irq_enabled = !disable_vblank ? true : false;
- if (disable_vblank)
DRM_INFO("Virtual Hardware mode enabled");
- ret = drm_vblank_init(&vkms_device->drm, 1);
- ret = (disable_vblank) ? 0 : drm_vblank_init(&vkms_device->drm, 1); if (ret) { DRM_ERROR("Failed to vblank\n"); goto out_fini;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index f156c930366a..9ee862e6f7e5 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -52,6 +52,7 @@ struct vkms_output { struct drm_encoder encoder; struct drm_connector connector; struct hrtimer vblank_hrtimer;
- bool disable_vblank; ktime_t period_ns; struct drm_pending_vblank_event *event; bool crc_enabled;
The vblank handling parts look correct to me.
Does kms_flip work with this? Virtual hw immediately completes the drm_crtc_state->event in the atomic_update callback, that part seems to be missing from your patch.
I tested with patched version of kms_flip to skip vblank. Here is the patch:
https://patchwork.kernel.org/patch/10570861/
About the drm_crtc_state->event, afaiu, the code in the function vkms_crtc_atomic_flush immediately send the vblank event to the user space via drm_crtc_send_vblank_event which make things work in the user space. Make sense? Or I missed something?
Ah, the magic of the vblank_get() == 0 check. A bit tricky, but yes this indeed works. I think it'd be good to explain this in the commit message.
Also I like the idea of switching away from the generic -EINVAL to -ENOTTY for when vblanks aren't supported. Do you have that somewhere (as a separate patch).
I already started to investigate if the change between -EINVAL to -ENOTTY will break an existing kms userspace compositors. I already checked:
- kwin
- sway
- wayland-core
- weston
I still have to check the X drivers, chromium, and drm_hwc.
Awesome, keep up the great work.
Cheers, Daniel
Thanks for the feedback.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Rodrigo Siqueira http://siqueira.tech Graduate Student Department of Computer Science University of São Paulo
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch