[PATCH V4 0/2] Add virtual hardware module

[PATCH V4 0/2] Add virtual hardware module

[Sumera Priyadarsini]

This patchset adds support for emulating virtual hardware with VKMS.
The virtual hardware mode can be enabled by using the following command
while loading the module:
        sudo modprobe vkms enable_virtual_hw=1

The first patch is prep work for adding virtual_hw mode and refactors
the plane composition in vkms by adding a helper function vkms_composer_common()
which can be used for both vblank mode and virtual mode.

The second patch adds virtual hardware support as a module option. It
adds new atomic helper functions for the virtual mode
and modifies the existing atomic helpers for usage by the vblank mode
This gives us two sets of drm_crtc_helper_funcs struct for both modes,
making the code flow cleaner and easier to debug.

This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for
subtests related to crc reads and skips tests that rely on vertical
blanking. This patchset must be tested after incorporating the
igt-tests patch: https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html

Sumera Priyadarsini (2):
  drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode
  drm/vkms: Add support for virtual hardware mode

 drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
 drivers/gpu/drm/vkms/vkms_crtc.c     | 51 +++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c      | 18 ++++--
 drivers/gpu/drm/vkms/vkms_drv.h      |  4 ++
 4 files changed, 109 insertions(+), 52 deletions(-)

-- 
2.25.1

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

[PATCH V4 1/2] drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode

[Sumera Priyadarsini]

Add two new functions vkms_composer_common() and vkms_crtc_composer().
The actual plane composition work has been moved to the helper function,
vkms_composer_common() which is called by both vkms_composer_worker()
and vkms_crtc_composer(). vkms_crtc_composer() can be used when we
implement virtual_hw mode.

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
---
Changes in V4:
- Fix warning
Changes in V3:
- Refactor patchset (Melissa)
Change in V2:
- Add vkms_composer_common() (Daniel)
---
 drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
 drivers/gpu/drm/vkms/vkms_drv.h      |  3 +
 2 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 66c6842d70db..0d2bad3ff849 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -169,6 +169,44 @@ static int compose_planes(void **vaddr_out,
 	return 0;
 }
 
+int vkms_composer_common(struct vkms_crtc_state *crtc_state,
+			 struct vkms_output *out, bool wb_pending, uint32_t *crc32)
+{
+	struct vkms_composer *primary_composer = NULL;
+	struct vkms_composer *cursor_composer = NULL;
+	void *vaddr_out = NULL;
+	int ret;
+
+	if (crtc_state->num_active_planes >= 1)
+		primary_composer = crtc_state->active_planes[0]->composer;
+	if (crtc_state->num_active_planes == 2)
+		cursor_composer = crtc_state->active_planes[1]->composer;
+	if (!primary_composer)
+		return -EINVAL;
+	if (wb_pending)
+		vaddr_out = crtc_state->active_writeback;
+
+	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
+	if (ret) {
+		if (ret == -EINVAL && !wb_pending)
+			kfree(vaddr_out);
+		return -EINVAL;
+	}
+
+	*crc32 = compute_crc(vaddr_out, primary_composer);
+
+	if (wb_pending) {
+		drm_writeback_signal_completion(&out->wb_connector, 0);
+		spin_lock_irq(&out->composer_lock);
+		crtc_state->wb_pending = false;
+		spin_unlock_irq(&out->composer_lock);
+	} else {
+		kfree(vaddr_out);
+	}
+
+	return 0;
+}
+
 /**
  * vkms_composer_worker - ordered work_struct to compute CRC
  *
@@ -185,12 +223,9 @@ void vkms_composer_worker(struct work_struct *work)
 						composer_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
-	struct vkms_composer *primary_composer = NULL;
-	struct vkms_composer *cursor_composer = NULL;
 	bool crc_pending, wb_pending;
-	void *vaddr_out = NULL;
-	u32 crc32 = 0;
 	u64 frame_start, frame_end;
+	u32 crc32 = 0;
 	int ret;
 
 	spin_lock_irq(&out->composer_lock);
@@ -210,36 +245,9 @@ void vkms_composer_worker(struct work_struct *work)
 	if (!crc_pending)
 		return;
 
-	if (crtc_state->num_active_planes >= 1)
-		primary_composer = crtc_state->active_planes[0]->composer;
-
-	if (crtc_state->num_active_planes == 2)
-		cursor_composer = crtc_state->active_planes[1]->composer;
-
-	if (!primary_composer)
-		return;
-
-	if (wb_pending)
-		vaddr_out = crtc_state->active_writeback;
-
-	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
-	if (ret) {
-		if (ret == -EINVAL && !wb_pending)
-			kfree(vaddr_out);
+	ret = vkms_composer_common(crtc_state, out, wb_pending, &crc32);
+	if (ret == -EINVAL)
 		return;
-	}
-
-	crc32 = compute_crc(vaddr_out, primary_composer);
-
-	if (wb_pending) {
-		drm_writeback_signal_completion(&out->wb_connector, 0);
-		spin_lock_irq(&out->composer_lock);
-		crtc_state->wb_pending = false;
-		spin_unlock_irq(&out->composer_lock);
-	} else {
-		kfree(vaddr_out);
-	}
-
 	/*
 	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
 	 */
@@ -247,6 +255,20 @@ void vkms_composer_worker(struct work_struct *work)
 		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
 }
 
+void vkms_crtc_composer(struct vkms_crtc_state *crtc_state)
+{
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	u32 crc32 = 0;
+	int ret;
+
+	ret = vkms_composer_common(crtc_state, out, crtc_state->wb_pending, &crc32);
+	if (ret == -EINVAL)
+		return;
+
+	drm_crtc_add_crc_entry(crtc, true, 0, &crc32);
+}
+
 static const char * const pipe_crc_sources[] = {"auto"};
 
 const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 35540c7c4416..538315140585 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -125,8 +125,11 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 			   size_t *values_cnt);
 
 /* Composer Support */
+int vkms_composer_common(struct vkms_crtc_state *crtc_state, struct vkms_output *out,
+			 bool wb_pending, uint32_t *crcs);
 void vkms_composer_worker(struct work_struct *work);
 void vkms_set_composer(struct vkms_output *out, bool enabled);
+void vkms_crtc_composer(struct vkms_crtc_state *crtc_state);
 
 /* Writeback */
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
-- 
2.25.1

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

[PATCH V4 2/2] drm/vkms: Add support for virtual hardware mode

[Sumera Priyadarsini]

Add a virtual hardware or vblank-less mode as a module to
enable VKMS to emulate virtual graphic drivers.

Add a new drm_crtc_helper_funcs struct,
vkms_virtual_crtc_helper_funcs() which holds the atomic helpers
for virtual hardware mode. Change the existing
vkms_crtc_helper_funcs struct to vkms_vblank_crtc_helper_funcs
which holds atomic helpers for the vblank mode.
This makes the code flow clearer and easier to test
virtual hardware mode.

The first patch of this patchset added the function vkms_crtc_composer()
for plane composition which is used in case of vblank-less mode and
is directly called in the atomic hook in vkms_crtc_atomic_begin().
However, some crc captures still use vblanks which causes the crc-based
igt tests to crash. Currently, I am unsure about how to approach the
one-shot implementation of crc reads so I am still working on that.

This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for
subtests related to crc reads and skips tests that rely on vertical
blanking. This patchset must be tested after incorporating the
igt-tests patch:
https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html .

The patch is based on Rodrigo Siqueira's
patch(https://patchwork.freedesktop.org/patch/316851/?series=48469&rev=3)
and the ensuing review.

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
---
Changes in V3:
- Refactor patchset(Melissa)
Changes in V2:
- Add atomic helper functions in a separate struct for virtual hardware
mode (Daniel)
- Remove spinlock across 'vkms_output->lock' in vkms_crtc.c(Daniel)
- Add vkms_composer_common() (Daniel)
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 51 +++++++++++++++++++++++---------
 drivers/gpu/drm/vkms/vkms_drv.c  | 18 +++++++----
 drivers/gpu/drm/vkms/vkms_drv.h  |  1 +
 3 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 57bbd32e9beb..e6286f98d5b6 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -222,20 +222,20 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
-static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
-				    struct drm_atomic_state *state)
+static void vkms_vblank_crtc_atomic_enable(struct drm_crtc *crtc,
+					   struct drm_atomic_state *state)
 {
 	drm_crtc_vblank_on(crtc);
 }
 
-static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
-				     struct drm_atomic_state *state)
+static void vkms_vblank_crtc_atomic_disable(struct drm_crtc *crtc,
+		struct drm_atomic_state *state)
 {
 	drm_crtc_vblank_off(crtc);
 }
 
-static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
-				   struct drm_atomic_state *state)
+static void vkms_vblank_crtc_atomic_begin(struct drm_crtc *crtc,
+		struct drm_atomic_state *state)
 {
 	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 
@@ -245,8 +245,8 @@ static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
 	spin_lock_irq(&vkms_output->lock);
 }
 
-static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
-				   struct drm_atomic_state *state)
+static void vkms_vblank_crtc_atomic_flush(struct drm_crtc *crtc,
+		struct drm_atomic_state *state)
 {
 	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 
@@ -268,18 +268,38 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 	spin_unlock_irq(&vkms_output->lock);
 }
 
-static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+/*
+ * Crtc functions for virtual hardware/vblankless mode
+ */
+static void vkms_virtual_crtc_atomic_flush(struct drm_crtc *crtc,
+		struct drm_atomic_state *state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(crtc->state);
+
+	vkms_crtc_composer(vkms_state);
+
+	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
+}
+
+static const struct drm_crtc_helper_funcs vkms_vblank_crtc_helper_funcs = {
 	.atomic_check	= vkms_crtc_atomic_check,
-	.atomic_begin	= vkms_crtc_atomic_begin,
-	.atomic_flush	= vkms_crtc_atomic_flush,
-	.atomic_enable	= vkms_crtc_atomic_enable,
-	.atomic_disable	= vkms_crtc_atomic_disable,
+	.atomic_begin	= vkms_vblank_crtc_atomic_begin,
+	.atomic_flush	= vkms_vblank_crtc_atomic_flush,
+	.atomic_enable	= vkms_vblank_crtc_atomic_enable,
+	.atomic_disable	= vkms_vblank_crtc_atomic_disable,
+};
+
+static const struct drm_crtc_helper_funcs vkms_virtual_crtc_helper_funcs = {
+	.atomic_check	= vkms_crtc_atomic_check,
+	.atomic_flush	= vkms_virtual_crtc_atomic_flush,
 };
 
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
 	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
@@ -289,7 +309,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		return ret;
 	}
 
-	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
+	if (vkmsdev->config->virtual_hw)
+		drm_crtc_helper_add(crtc, &vkms_virtual_crtc_helper_funcs);
+	else
+		drm_crtc_helper_add(crtc, &vkms_vblank_crtc_helper_funcs);
 
 	spin_lock_init(&vkms_out->lock);
 	spin_lock_init(&vkms_out->composer_lock);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 2173b82606f6..945c4495d62a 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -44,6 +44,11 @@ static bool enable_writeback = true;
 module_param_named(enable_writeback, enable_writeback, bool, 0444);
 MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector support");
 
+static bool enable_virtual_hw = false;
+module_param_named(enable_virtual_hw, enable_virtual_hw, bool, 0444);
+MODULE_PARM_DESC(enable_virtual_hw, "Enable/Disable virtual hardware mode(virtual \
+hardware mode disables vblank interrupts)");
+
 DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
 static void vkms_release(struct drm_device *dev)
@@ -159,12 +164,14 @@ static int vkms_create(struct vkms_config *config)
 		goto out_devres;
 	}
 
-	vkms_device->drm.irq_enabled = true;
+	vkms_device->drm.irq_enabled = !vkms_device->config->virtual_hw;
 
-	ret = drm_vblank_init(&vkms_device->drm, 1);
-	if (ret) {
-		DRM_ERROR("Failed to vblank\n");
-		goto out_devres;
+	if (!vkms_device->config->virtual_hw) {
+		ret = drm_vblank_init(&vkms_device->drm, 1);
+		if (ret) {
+			DRM_ERROR("Failed to vblank\n");
+			goto out_devres;
+		}
 	}
 
 	ret = vkms_modeset_init(vkms_device);
@@ -198,6 +205,7 @@ static int __init vkms_init(void)
 
 	config->cursor = enable_cursor;
 	config->writeback = enable_writeback;
+	config->virtual_hw = enable_virtual_hw;
 
 	return vkms_create(config);
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 538315140585..a44f530ffaf0 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -85,6 +85,7 @@ struct vkms_device;
 struct vkms_config {
 	bool writeback;
 	bool cursor;
+	bool virtual_hw;
 	/* only set when instantiated */
 	struct vkms_device *dev;
 };
-- 
2.25.1

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

Re: [PATCH V4 0/2] Add virtual hardware module

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2689 bytes --]

On Mon, 5 Apr 2021 11:41:50 +0530
Sumera Priyadarsini <sylphrenadin@gmail.com> wrote:

> This patchset adds support for emulating virtual hardware with VKMS.
> The virtual hardware mode can be enabled by using the following command
> while loading the module:
>         sudo modprobe vkms enable_virtual_hw=1

Hi,

every time I see this cover letter subject, I start wondering "what is
this virtual hardware module, yet another one?" and then I read the
cover letter and realise it is about adding an option to VKMS.

The next time you revise this series, could you perhaps clarify the
subject?

The idea of having a mode where VKMS behaves like a virtual hardware
driver is good, IMO. I do think "vblank-less mode" describes it better
though, because I would assume things like USB display drivers to work
like this too, and VKMS is already a virtual driver anyway.

To clarify, as a userspace programmer what I would expect "vblank-less
mode" to be is that the DRM driver completes pageflips and modesets at
arbitrary times, perhaps always immediately or perhaps with a variable
delay that depends on how much processing is needed for the update.
Also vblank events do not fire and vblank counters do not advance. Is
this correct?


Thanks,
pq

> 
> The first patch is prep work for adding virtual_hw mode and refactors
> the plane composition in vkms by adding a helper function vkms_composer_common()
> which can be used for both vblank mode and virtual mode.
> 
> The second patch adds virtual hardware support as a module option. It
> adds new atomic helper functions for the virtual mode
> and modifies the existing atomic helpers for usage by the vblank mode
> This gives us two sets of drm_crtc_helper_funcs struct for both modes,
> making the code flow cleaner and easier to debug.
> 
> This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
> kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for
> subtests related to crc reads and skips tests that rely on vertical
> blanking. This patchset must be tested after incorporating the
> igt-tests patch: https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html
> 
> Sumera Priyadarsini (2):
>   drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode
>   drm/vkms: Add support for virtual hardware mode
> 
>  drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
>  drivers/gpu/drm/vkms/vkms_crtc.c     | 51 +++++++++++-----
>  drivers/gpu/drm/vkms/vkms_drv.c      | 18 ++++--
>  drivers/gpu/drm/vkms/vkms_drv.h      |  4 ++
>  4 files changed, 109 insertions(+), 52 deletions(-)
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH V4 1/2] drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode

[Melissa Wen]

On 04/05, Sumera Priyadarsini wrote:
> Add two new functions vkms_composer_common() and vkms_crtc_composer().
> The actual plane composition work has been moved to the helper function,
> vkms_composer_common() which is called by both vkms_composer_worker()
> and vkms_crtc_composer(). vkms_crtc_composer() can be used when we
> implement virtual_hw mode.

Hi Sumera,

At this point, vkms_crtc_composer is never called. You should move this
function to the patch where you actually add virtual_hw mode, giving
meaning to create a different function (suitable for composition on
vblank-less mode). keep here only the prep work to add virtual hardware.

Thanks,

Melissa

> 
> Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
> ---
> Changes in V4:
> - Fix warning
> Changes in V3:
> - Refactor patchset (Melissa)
> Change in V2:
> - Add vkms_composer_common() (Daniel)
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
>  drivers/gpu/drm/vkms/vkms_drv.h      |  3 +
>  2 files changed, 58 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 66c6842d70db..0d2bad3ff849 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -169,6 +169,44 @@ static int compose_planes(void **vaddr_out,
>  	return 0;
>  }
>  
> +int vkms_composer_common(struct vkms_crtc_state *crtc_state,
> +			 struct vkms_output *out, bool wb_pending, uint32_t *crc32)
> +{
> +	struct vkms_composer *primary_composer = NULL;
> +	struct vkms_composer *cursor_composer = NULL;
> +	void *vaddr_out = NULL;
> +	int ret;
> +
> +	if (crtc_state->num_active_planes >= 1)
> +		primary_composer = crtc_state->active_planes[0]->composer;
> +	if (crtc_state->num_active_planes == 2)
> +		cursor_composer = crtc_state->active_planes[1]->composer;
> +	if (!primary_composer)
> +		return -EINVAL;
> +	if (wb_pending)
> +		vaddr_out = crtc_state->active_writeback;
> +
> +	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> +	if (ret) {
> +		if (ret == -EINVAL && !wb_pending)
> +			kfree(vaddr_out);
> +		return -EINVAL;
> +	}
> +
> +	*crc32 = compute_crc(vaddr_out, primary_composer);
> +
> +	if (wb_pending) {
> +		drm_writeback_signal_completion(&out->wb_connector, 0);
> +		spin_lock_irq(&out->composer_lock);
> +		crtc_state->wb_pending = false;
> +		spin_unlock_irq(&out->composer_lock);
> +	} else {
> +		kfree(vaddr_out);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * vkms_composer_worker - ordered work_struct to compute CRC
>   *
> @@ -185,12 +223,9 @@ void vkms_composer_worker(struct work_struct *work)
>  						composer_work);
>  	struct drm_crtc *crtc = crtc_state->base.crtc;
>  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> -	struct vkms_composer *primary_composer = NULL;
> -	struct vkms_composer *cursor_composer = NULL;
>  	bool crc_pending, wb_pending;
> -	void *vaddr_out = NULL;
> -	u32 crc32 = 0;
>  	u64 frame_start, frame_end;
> +	u32 crc32 = 0;
>  	int ret;
>  
>  	spin_lock_irq(&out->composer_lock);
> @@ -210,36 +245,9 @@ void vkms_composer_worker(struct work_struct *work)
>  	if (!crc_pending)
>  		return;
>  
> -	if (crtc_state->num_active_planes >= 1)
> -		primary_composer = crtc_state->active_planes[0]->composer;
> -
> -	if (crtc_state->num_active_planes == 2)
> -		cursor_composer = crtc_state->active_planes[1]->composer;
> -
> -	if (!primary_composer)
> -		return;
> -
> -	if (wb_pending)
> -		vaddr_out = crtc_state->active_writeback;
> -
> -	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> -	if (ret) {
> -		if (ret == -EINVAL && !wb_pending)
> -			kfree(vaddr_out);
> +	ret = vkms_composer_common(crtc_state, out, wb_pending, &crc32);
> +	if (ret == -EINVAL)
>  		return;
> -	}
> -
> -	crc32 = compute_crc(vaddr_out, primary_composer);
> -
> -	if (wb_pending) {
> -		drm_writeback_signal_completion(&out->wb_connector, 0);
> -		spin_lock_irq(&out->composer_lock);
> -		crtc_state->wb_pending = false;
> -		spin_unlock_irq(&out->composer_lock);
> -	} else {
> -		kfree(vaddr_out);
> -	}
> -
>  	/*
>  	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
>  	 */
> @@ -247,6 +255,20 @@ void vkms_composer_worker(struct work_struct *work)
>  		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
>  }
>  
> +void vkms_crtc_composer(struct vkms_crtc_state *crtc_state)
> +{
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> +	u32 crc32 = 0;
> +	int ret;
> +
> +	ret = vkms_composer_common(crtc_state, out, crtc_state->wb_pending, &crc32);
> +	if (ret == -EINVAL)
> +		return;
> +
> +	drm_crtc_add_crc_entry(crtc, true, 0, &crc32);
> +}
> +
>  static const char * const pipe_crc_sources[] = {"auto"};
>  
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 35540c7c4416..538315140585 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -125,8 +125,11 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
>  
>  /* Composer Support */
> +int vkms_composer_common(struct vkms_crtc_state *crtc_state, struct vkms_output *out,
> +			 bool wb_pending, uint32_t *crcs);
>  void vkms_composer_worker(struct work_struct *work);
>  void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_crtc_composer(struct vkms_crtc_state *crtc_state);
>  
>  /* Writeback */
>  int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> -- 
> 2.25.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V4 2/2] drm/vkms: Add support for virtual hardware mode

[Melissa Wen]

On 04/05, Sumera Priyadarsini wrote:
> Add a virtual hardware or vblank-less mode as a module to
> enable VKMS to emulate virtual graphic drivers.
> 
> Add a new drm_crtc_helper_funcs struct,
> vkms_virtual_crtc_helper_funcs() which holds the atomic helpers
> for virtual hardware mode. Change the existing
> vkms_crtc_helper_funcs struct to vkms_vblank_crtc_helper_funcs
> which holds atomic helpers for the vblank mode.

I would say `rename' not `change'

> This makes the code flow clearer and easier to test
> virtual hardware mode.
> 
> The first patch of this patchset added the function vkms_crtc_composer()
> for plane composition which is used in case of vblank-less mode and
> is directly called in the atomic hook in vkms_crtc_atomic_begin().

Probably this statement will change when you move the vkms_crtc_composer
to here.

> However, some crc captures still use vblanks which causes the crc-based
> igt tests to crash. Currently, I am unsure about how to approach the
> one-shot implementation of crc reads so I am still working on that.
> 
> This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
> kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for

Although the test results remain the same, you should also check the
log. In kms_flip, the substests flip-vs-panning-vs-hang and
flip-vs-panning-interruptible raised an error:
[drm:vkms_composer_common [vkms]] *ERROR* Cannot allocate memory for output frame.
Please, also check for leak.

> subtests related to crc reads and skips tests that rely on vertical
> blanking. This patchset must be tested after incorporating the
> igt-tests patch:
> https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html .

prefer to use the patckwork link

> 
> The patch is based on Rodrigo Siqueira's
> patch(https://patchwork.freedesktop.org/patch/316851/?series=48469&rev=3)
> and the ensuing review.
> 
> Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
> ---
> Changes in V3:
> - Refactor patchset(Melissa)
> Changes in V2:
> - Add atomic helper functions in a separate struct for virtual hardware
> mode (Daniel)
> - Remove spinlock across 'vkms_output->lock' in vkms_crtc.c(Daniel)
> - Add vkms_composer_common() (Daniel)
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 51 +++++++++++++++++++++++---------
>  drivers/gpu/drm/vkms/vkms_drv.c  | 18 +++++++----
>  drivers/gpu/drm/vkms/vkms_drv.h  |  1 +
>  3 files changed, 51 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..e6286f98d5b6 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -222,20 +222,20 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> -				    struct drm_atomic_state *state)
> +static void vkms_vblank_crtc_atomic_enable(struct drm_crtc *crtc,
> +					   struct drm_atomic_state *state)
>  {
>  	drm_crtc_vblank_on(crtc);
>  }
>  
> -static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> -				     struct drm_atomic_state *state)
> +static void vkms_vblank_crtc_atomic_disable(struct drm_crtc *crtc,
> +		struct drm_atomic_state *state)
>  {
>  	drm_crtc_vblank_off(crtc);
>  }
>  
> -static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> -				   struct drm_atomic_state *state)
> +static void vkms_vblank_crtc_atomic_begin(struct drm_crtc *crtc,
> +		struct drm_atomic_state *state)
>  {
>  	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
>  
> @@ -245,8 +245,8 @@ static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
>  	spin_lock_irq(&vkms_output->lock);
>  }
>  
> -static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> -				   struct drm_atomic_state *state)
> +static void vkms_vblank_crtc_atomic_flush(struct drm_crtc *crtc,
> +		struct drm_atomic_state *state)
>  {
>  	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
>  
> @@ -268,18 +268,38 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>  	spin_unlock_irq(&vkms_output->lock);
>  }
>  
> -static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> +/*
> + * Crtc functions for virtual hardware/vblankless mode
> + */
> +static void vkms_virtual_crtc_atomic_flush(struct drm_crtc *crtc,
> +		struct drm_atomic_state *state)
> +{
> +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> +	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(crtc->state);
> +
> +	vkms_crtc_composer(vkms_state);
> +
> +	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
> +}
> +
> +static const struct drm_crtc_helper_funcs vkms_vblank_crtc_helper_funcs = {
>  	.atomic_check	= vkms_crtc_atomic_check,
> -	.atomic_begin	= vkms_crtc_atomic_begin,
> -	.atomic_flush	= vkms_crtc_atomic_flush,
> -	.atomic_enable	= vkms_crtc_atomic_enable,
> -	.atomic_disable	= vkms_crtc_atomic_disable,
> +	.atomic_begin	= vkms_vblank_crtc_atomic_begin,
> +	.atomic_flush	= vkms_vblank_crtc_atomic_flush,
> +	.atomic_enable	= vkms_vblank_crtc_atomic_enable,
> +	.atomic_disable	= vkms_vblank_crtc_atomic_disable,
> +};
> +
> +static const struct drm_crtc_helper_funcs vkms_virtual_crtc_helper_funcs = {
> +	.atomic_check	= vkms_crtc_atomic_check,
> +	.atomic_flush	= vkms_virtual_crtc_atomic_flush,
>  };
>  
>  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		   struct drm_plane *primary, struct drm_plane *cursor)
>  {
>  	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
>  	int ret;
>  
>  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> @@ -289,7 +309,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		return ret;
>  	}
>  
> -	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> +	if (vkmsdev->config->virtual_hw)
> +		drm_crtc_helper_add(crtc, &vkms_virtual_crtc_helper_funcs);
> +	else
> +		drm_crtc_helper_add(crtc, &vkms_vblank_crtc_helper_funcs);
>  
>  	spin_lock_init(&vkms_out->lock);
>  	spin_lock_init(&vkms_out->composer_lock);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 2173b82606f6..945c4495d62a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -44,6 +44,11 @@ static bool enable_writeback = true;
>  module_param_named(enable_writeback, enable_writeback, bool, 0444);
>  MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector support");
>  
> +static bool enable_virtual_hw = false;
> +module_param_named(enable_virtual_hw, enable_virtual_hw, bool, 0444);
> +MODULE_PARM_DESC(enable_virtual_hw, "Enable/Disable virtual hardware mode(virtual \
> +hardware mode disables vblank interrupts)");
> +
>  DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  
>  static void vkms_release(struct drm_device *dev)
> @@ -159,12 +164,14 @@ static int vkms_create(struct vkms_config *config)
>  		goto out_devres;
>  	}
>  
> -	vkms_device->drm.irq_enabled = true;
> +	vkms_device->drm.irq_enabled = !vkms_device->config->virtual_hw;
>  
> -	ret = drm_vblank_init(&vkms_device->drm, 1);
> -	if (ret) {
> -		DRM_ERROR("Failed to vblank\n");
> -		goto out_devres;
> +	if (!vkms_device->config->virtual_hw) {
> +		ret = drm_vblank_init(&vkms_device->drm, 1);
> +		if (ret) {
> +			DRM_ERROR("Failed to vblank\n");
> +			goto out_devres;
> +		}
>  	}
>  
>  	ret = vkms_modeset_init(vkms_device);
> @@ -198,6 +205,7 @@ static int __init vkms_init(void)
>  
>  	config->cursor = enable_cursor;
>  	config->writeback = enable_writeback;
> +	config->virtual_hw = enable_virtual_hw;
>  
>  	return vkms_create(config);
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 538315140585..a44f530ffaf0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -85,6 +85,7 @@ struct vkms_device;
>  struct vkms_config {
>  	bool writeback;
>  	bool cursor;
> +	bool virtual_hw;
>  	/* only set when instantiated */
>  	struct vkms_device *dev;

Checkpatch complained of several lines. Please, take a look at this too.

Thanks,

Melissa

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

Re: [PATCH V4 0/2] Add virtual hardware module

[Melissa Wen]

On 04/07, Pekka Paalanen wrote:
> On Mon, 5 Apr 2021 11:41:50 +0530
> Sumera Priyadarsini <sylphrenadin@gmail.com> wrote:
> 
> > This patchset adds support for emulating virtual hardware with VKMS.
> > The virtual hardware mode can be enabled by using the following command
> > while loading the module:
> >         sudo modprobe vkms enable_virtual_hw=1
> 
> Hi,
> 
> every time I see this cover letter subject, I start wondering "what is
> this virtual hardware module, yet another one?" and then I read the
> cover letter and realise it is about adding an option to VKMS.
> 
> The next time you revise this series, could you perhaps clarify the
> subject?
+1
> 
> The idea of having a mode where VKMS behaves like a virtual hardware
> driver is good, IMO. I do think "vblank-less mode" describes it better
> though, because I would assume things like USB display drivers to work
> like this too, and VKMS is already a virtual driver anyway.
> 
> To clarify, as a userspace programmer what I would expect "vblank-less
> mode" to be is that the DRM driver completes pageflips and modesets at
> arbitrary times, perhaps always immediately or perhaps with a variable
> delay that depends on how much processing is needed for the update.
> Also vblank events do not fire and vblank counters do not advance. Is
> this correct?
> 
yes. And I think this description should be clear in both the cover
letter and also the commit message of the patch that add the module
option to vkms.
> 
> Thanks,
> pq
> 
> > 
> > The first patch is prep work for adding virtual_hw mode and refactors
> > the plane composition in vkms by adding a helper function vkms_composer_common()
> > which can be used for both vblank mode and virtual mode.
> > 
> > The second patch adds virtual hardware support as a module option. It
> > adds new atomic helper functions for the virtual mode
> > and modifies the existing atomic helpers for usage by the vblank mode
> > This gives us two sets of drm_crtc_helper_funcs struct for both modes,
> > making the code flow cleaner and easier to debug.
> > 
> > This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
> > kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for
> > subtests related to crc reads and skips tests that rely on vertical
> > blanking. This patchset must be tested after incorporating the
> > igt-tests patch: https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html

Sumera,

Thanks for your patches.

In addition to Pekka's comments, consider what I comment in each patch
of the series for a next version.

Best regards,

Melissa
> > 
> > Sumera Priyadarsini (2):
> >   drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode
> >   drm/vkms: Add support for virtual hardware mode
> > 
> >  drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
> >  drivers/gpu/drm/vkms/vkms_crtc.c     | 51 +++++++++++-----
> >  drivers/gpu/drm/vkms/vkms_drv.c      | 18 ++++--
> >  drivers/gpu/drm/vkms/vkms_drv.h      |  4 ++
> >  4 files changed, 109 insertions(+), 52 deletions(-)
> > 
> 


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