Re: [PATCH V3] drm/vkms: Add support for vkms work without vblank

[Daniel Vetter]

On Tue, Jul 16, 2019 at 11:07:37PM -0300, Rodrigo Siqueira wrote:
> On 07/16, Daniel Vetter wrote:
> > On Thu, Jul 11, 2019 at 11:57:18PM -0300, Rodrigo Siqueira wrote:
> > > On 07/10, Daniel Vetter wrote:
> > > > On Tue, Jul 09, 2019 at 10:55:14PM -0300, Rodrigo Siqueira wrote:
> > > > > Currently, vkms only work with enabled VBlank. This patch adds another
> > > > > operation model that allows vkms to work without VBlank support. In this
> > > > > scenario, vblank signaling is faked by calling drm_send_vblank_event()
> > > > > in vkms_crtc_atomic_flush(); this approach works due to the
> > > > > drm_vblank_get() == 0 checking.
> > > > > 
> > > > > Changes since V2:
> > > > >  - Rebase
> > > > > 
> > > > > Changes since V1:
> > > > >   Daniel Vetter:
> > > > >   - Change module parameter name from disablevblank to virtual_hw
> > > > >   - Improve parameter description
> > > > >   - Improve commit message
> > > > > 
> > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_crtc.c | 10 ++++++++++
> > > > >  drivers/gpu/drm/vkms/vkms_drv.c  | 13 +++++++++++--
> > > > >  drivers/gpu/drm/vkms/vkms_drv.h  |  2 ++
> > > > >  3 files changed, 23 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > index 49a8ec2cb1c1..a0c75b8c4335 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > @@ -207,12 +207,22 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
> > > > >  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 152d7de24a76..542a002ef9d5 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > @@ -34,6 +34,11 @@ bool enable_writeback;
> > > > >  module_param_named(enable_writeback, enable_writeback, bool, 0444);
> > > > >  MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> > > > >  
> > > > > +bool virtual_hw;
> > > > 
> > > > Can be static, you only use this in vkms_drv.c.
> > > > 
> > > > > +module_param_named(virtual_hw, virtual_hw, bool, 0444);
> > > > > +MODULE_PARM_DESC(virtual_hw,
> > > > > +		 "Enable virtual hardware mode (disables vblanks and immediately completes flips)");
> > > > > +
> > > > >  static const struct file_operations vkms_driver_fops = {
> > > > >  	.owner		= THIS_MODULE,
> > > > >  	.open		= drm_open,
> > > > > @@ -154,9 +159,13 @@ static int __init vkms_init(void)
> > > > >  	if (ret)
> > > > >  		goto out_unregister;
> > > > >  
> > > > > -	vkms_device->drm.irq_enabled = true;
> > > > > +	vkms_device->output.disable_vblank = virtual_hw;
> > > > > +	vkms_device->drm.irq_enabled = !virtual_hw;
> > > > > +
> > > > > +	if (virtual_hw)
> > > > > +		DRM_INFO("Virtual hardware mode enabled");
> > > > >  
> > > > > -	ret = drm_vblank_init(&vkms_device->drm, 1);
> > > > > +	ret = (virtual_hw) ? 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 9ff2cd4ebf81..256e5e65c947 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > @@ -21,6 +21,7 @@
> > > > >  
> > > > >  extern bool enable_cursor;
> > > > >  extern bool enable_writeback;
> > > > > +extern bool virtual_hw;
> > > > >  
> > > > >  struct vkms_composer {
> > > > >  	struct drm_framebuffer fb;
> > > > > @@ -69,6 +70,7 @@ struct vkms_output {
> > > > >  	struct drm_connector connector;
> > > > >  	struct drm_writeback_connector wb_connector;
> > > > >  	struct hrtimer vblank_hrtimer;
> > > > > +	bool disable_vblank;
> > > > >  	ktime_t period_ns;
> > > > >  	struct drm_pending_vblank_event *event;
> > > > >  	/* ordered wq for composer_work */
> > > > 
> > > > I'm kinda wondering how this works at all ... does writeback/crc still
> > > > work if you set virtual mode? Writeback since this seems based on the
> > > > writeback series ...
> > > 
> > > Hi,
> > > 
> > > I tested this patch with kms_flip with the "drm/drm_vblank: Change
> > > EINVAL by the correct errno" patch [1]. However, youâre right about the
> > > writeback/crc tests, they does not pass because this patch disables
> > > vblank which in turn does not invoke "vkms_vblank_simulate()".
> > > 
> > > In this sense, Iâm a little bit confused about how should I handle this
> > > issue. If I correctly understood, without vblank support we have to
> > > trigger vkms_composer_worker() when the userspace invoke
> > > drmModePageFlip(), am I right?  If so, an approach could add page_flip()
> > > callback to vkms and invoke vkms_composer_worker(); however, the
> > > documentation says that page_flip is a legacy entry point. Do you have a
> > > suggestion?
> > 
> > We need this in atomic, for everyone. So instead of launching a vblank and
> > letting the vblank simulation kick off the crc/writeback work,
> > atomic_flush needs to also kick of the crc/vblank work. And make sure that
> > we feed the same vblank frame count values to the generated vblank even as
> > to the crc work (or maybe those are hardcoded to 0 for vblank-less, not
> > sure).
> > -Daniel
> 
> Just for checking if I correctly understood... the idea is reworking
> vkms_vblank_simulate() a little bit in order to decouple the kick of
> crc/writeback to atomic_flush(). Right?

Yes, but only for virtual mode, where we do not have the vblank. For
vblank mode we obviously still need the vblank simulation. I haven't
looked into what/how you should set the vblank counts for the crc/composer
work in the virtual case, that probably needs some figuring out.
-Daniel
>  
> > > 
> > > 1. https://patchwork.freedesktop.org/patch/314399/?series=50697&rev=7
> > > 
> > > Thanks
> > >  
> > > > btw if you send out patches that need other patches, point at that other
> > > > series in the cover letter or patch. Gets confusing fast otherwise.
> > > > -Daniel
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > Rodrigo Siqueira
> > > https://siqueira.tech
> > 
> > 
> > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech



> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch