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@xxxxxxxxx> 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@xxxxxxxxx>
> >> > ---
> >> > 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
--
Rodrigo Siqueira
http://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
