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

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

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

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@gmail.com>
---
 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;
+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 */
-- 
2.21.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Vetter]

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@gmail.com>
> ---
>  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 ...

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

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

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 5270 bytes --]

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@gmail.com>
> > ---
> >  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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Vetter]

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@gmail.com>
> > > ---
> > >  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

> 
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

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

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 6857 bytes --]

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@gmail.com>
> > > > ---
> > > >  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?
 
> > 
> > 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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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@gmail.com>
> > > > > ---
> > > > >  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@lists.freedesktop.org
> > > 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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