[RFC] drm/vkms: Add writeback support

[RFC] drm/vkms: Add writeback support

[Rodrigo Siqueira]

Hi,

In the last few weeks, I was focused on implementing the writeback on
VKMS by using the feedback that I got from Brian Starkey and Daniel
Vetter in my previous attempt [1]. For testing/guiding my
implementation, I’m using a patchset made by Liviu and Brian that adds
writeback tests to IGT [2]. This patch, already pass the basic
requirements and some of the other tests.

So, in this implementation I adopted the following design: you can use
writeback_connector, or you can have a virtual connector, but you cannot
have both. Should I keep the virtual connector always available and make
it possible to add writeback connector as the secondary connector? If
so, there's any IGT test for this?

Secondly, I’m facing problems related to
‘drm_writeback_signal_completion()’, for some reason that I did not know
yet I continuously get the following error in a loop:

[  +0.000060] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G      D W         5.0.0-VKMS #102
[  +0.000004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
[  +0.000033] RIP: 0010:drm_writeback_signal_completion+0x10c/0x130 [drm]
[  +0.000006] Code: 01 00 00 48 89 43 e8 48 89 43 f0 48 8b 35 3c 67 b3 e4 5b 5d 41 5c 41 5d 41 5e e9 0f 7d aa e3 4c 89 ee 4c 89 e7 e8 a4 4e 18 e4 <0f> 0b 5b 5d 41 5c 41 5d 41 5e c3 e8 54 2f f8 e3 eb 9a 0f 0b e9 60
[  +0.000004] RSP: 0018:ffff98593cb83ec0 EFLAGS: 00010082
[  +0.000006] RAX: 0000000080010002 RBX: ffff98593bbb54b0 RCX: 0000000000000000
[  +0.000005] RDX: 0000000000000001 RSI: 0000000000000082 RDI: 00000000ffffffff
[  +0.000004] RBP: ffff98593bbb54b0 R08: 0000000000000000 R09: ffff98593cb83e20
[  +0.000004] R10: ffff98593a07d600 R11: 0000000000000000 R12: ffff98593bbb54a8
[  +0.000004] R13: 0000000000000082 R14: 0000000000000000 R15: ffff98593cb9cd38
[  +0.000006] FS:  0000000000000000(0000) GS:ffff98593cb80000(0000) knlGS:0000000000000000
[  +0.000004] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  +0.000005] CR2: 00007f3cea2e8000 CR3: 00000000ba6ec000 CR4: 00000000000006e0
[  +0.000010] Call Trace:
[  +0.000007]  <IRQ>
[  +0.000013]  ? vkms_disable_vblank+0x20/0x20 [vkms]
[  +0.000008]  vkms_vblank_simulate+0xef/0x110 [vkms]
[  +0.000008]  ? vkms_disable_vblank+0x20/0x20 [vkms]
[  +0.000007]  __hrtimer_run_queues+0x100/0x2d0
[  +0.000008]  hrtimer_interrupt+0x10a/0x220
[  +0.000008]  ? kvm_sched_clock_read+0x5/0x10
[  +0.000010]  smp_apic_timer_interrupt+0x69/0x160
[  +0.000007]  apic_timer_interrupt+0xf/0x20
[  +0.000004]  </IRQ>
[  +0.000008] RIP: 0010:native_safe_halt+0x2/0x10
[  +0.000005] Code: 5b ff ff ff 7f 5b c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 75 c3 eb 8b 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 f4 c3 90 90 90 90 90 90
[  +0.000005] RSP: 0018:ffffb937c06b3eb8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
[  +0.000005] RAX: 0000000000000003 RBX: 0000000000000003 RCX: 0000000000000001
[  +0.000004] RDX: 0000000000000001 RSI: ffffffffa4e6da3e RDI: ffffffffa4e6dd30
[  +0.000005] RBP: ffffffffa51252e0 R08: ffffffffa55e7438 R09: 0000000000000000
[  +0.000003] R10: 0000000000000000 R11: ffffffffa5049fb8 R12: 0000000000000000
[  +0.000004] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  +0.000012]  default_idle+0x1a/0x170
[  +0.000010]  do_idle+0x1d5/0x250
[  +0.000007]  cpu_startup_entry+0x19/0x20
[  +0.000007]  start_secondary+0x1aa/0x200
[  +0.000008]  secondary_startup_64+0xa4/0xb0
[  +0.000008] ---[ end trace ec12666e4ecb96f7 ]---

Any ideas? Or any comments?

Thanks
Best Regards

1. https://patchwork.freedesktop.org/patch/257771/?series=51325&rev=1
2. https://patchwork.freedesktop.org/series/39229/

---
 drivers/gpu/drm/vkms/vkms_crtc.c   |   3 +
 drivers/gpu/drm/vkms/vkms_drv.c    |   8 ++
 drivers/gpu/drm/vkms/vkms_drv.h    |  21 ++++
 drivers/gpu/drm/vkms/vkms_output.c | 176 ++++++++++++++++++++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c  |   2 +-
 5 files changed, 195 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 8a9aeb0a9ea8..d9948208e8eb 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -19,6 +19,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (output->writeback_enabled)
+		drm_writeback_signal_completion(&output->mw_connector, 0);
+
 	if (state && output->crc_enabled) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 738dd6206d85..e0d7f94e3972 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -29,6 +29,10 @@ bool enable_cursor;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
 
+int enable_writeback;
+module_param_named(enable_writeback, enable_writeback, int, 0444);
+MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
+
 static const struct file_operations vkms_driver_fops = {
 	.owner		= THIS_MODULE,
 	.open		= drm_open,
@@ -123,6 +127,10 @@ static int __init vkms_init(void)
 		goto out_fini;
 	}
 
+	vkms_device->output.writeback_enabled = enable_writeback;
+	if (enable_writeback)
+		DRM_INFO("Writeback connector enabled");
+
 	ret = vkms_modeset_init(vkms_device);
 	if (ret)
 		goto out_fini;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 81f1cfbeb936..2c26fe686b93 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -8,6 +8,7 @@
 #include <drm/drm_gem.h>
 #include <drm/drm_encoder.h>
 #include <linux/hrtimer.h>
+#include <drm/drm_writeback.h>
 
 #define XRES_MIN    20
 #define YRES_MIN    20
@@ -46,6 +47,16 @@ struct vkms_plane_state {
 	struct vkms_crc_data *crc_data;
 };
 
+/**
+ * vkms_connector_state - Driver connector specific state
+ * @base: base connector state
+ * @writeback_vaddr: keep track for writeback framebuffer destination
+ */
+struct vkms_connector_state {
+	struct drm_connector_state base;
+	void *writeback_vaddr;
+};
+
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
@@ -64,10 +75,12 @@ struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct drm_writeback_connector mw_connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
 	bool crc_enabled;
+	int writeback_enabled;
 	/* ordered wq for crc_work */
 	struct workqueue_struct *crc_workq;
 	/* protects concurrent access to crc_data */
@@ -93,6 +106,12 @@ struct vkms_gem_object {
 #define drm_crtc_to_vkms_output(target) \
 	container_of(target, struct vkms_output, crtc)
 
+#define drm_connector_to_wb_connector(target) \
+	container_of(target, struct drm_writeback_connector, base)
+
+#define drm_wb_to_vkms_output(target) \
+	container_of(target, struct vkms_output, mw_connector)
+
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
@@ -105,6 +124,8 @@ struct vkms_gem_object {
 #define to_vkms_plane_state(target)\
 	container_of(target, struct vkms_plane_state, base)
 
+#define to_wb_state(_state) (struct vkms_connector_state *)(_state)
+
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor);
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 3b162b25312e..0e55e7299d2e 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -3,6 +3,8 @@
 #include "vkms_drv.h"
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_writeback.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 static void vkms_connector_destroy(struct drm_connector *connector)
 {
@@ -10,14 +12,92 @@ static void vkms_connector_destroy(struct drm_connector *connector)
 	drm_connector_cleanup(connector);
 }
 
+static void vkms_connector_reset(struct drm_connector *connector)
+{
+	struct vkms_connector_state *mw_state;
+	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
+	struct vkms_output *output = drm_wb_to_vkms_output(wb);
+
+	if (!output->writeback_enabled)
+		return drm_atomic_helper_connector_reset(connector);
+
+	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
+	if (!mw_state) {
+		DRM_ERROR("Failed to allocate memory for connector state");
+		return;
+	}
+
+	if (connector->state)
+		__drm_atomic_helper_connector_destroy_state(connector->state);
+
+	kfree(connector->state);
+	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+}
+
+static struct drm_connector_state *
+vkms_connector_duplicate_state(struct drm_connector *connector)
+{
+	struct vkms_connector_state *mw_state, *mw_current_state;
+	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
+	struct vkms_output *output = drm_wb_to_vkms_output(wb);
+
+	if (!output->writeback_enabled)
+		return drm_atomic_helper_connector_duplicate_state(connector);
+
+	if (WARN_ON(!connector->state))
+		return NULL;
+
+	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
+	if (!mw_state)
+		return NULL;
+
+	mw_current_state = to_wb_state(connector->state);
+	mw_state->writeback_vaddr = mw_current_state->writeback_vaddr;
+
+	__drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
+
+	return &mw_state->base;
+}
+
 static const struct drm_connector_funcs vkms_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = vkms_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.reset = vkms_connector_reset,
+	.atomic_duplicate_state = vkms_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static int vkms_mw_encoder_atomic_check(struct drm_encoder *encoder,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct drm_framebuffer *fb;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	fb = conn_state->writeback_job->fb;
+	if (fb->width != crtc_state->mode.hdisplay ||
+	    fb->height != crtc_state->mode.vdisplay) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+			      fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	if (fb->format->format != DRM_FORMAT_XRGB8888) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+			      drm_get_format_name(fb->format->format,
+						  &format_name));
+		return -EINVAL;
+	}
+
+	// TODO: Do we need to validate each plane?
+
+	return 0;
+}
+
 static const struct drm_encoder_funcs vkms_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
@@ -32,8 +112,57 @@ static int vkms_conn_get_modes(struct drm_connector *connector)
 	return count;
 }
 
+static void vkms_mw_atomic_commit(struct drm_connector *conn,
+				  struct drm_connector_state *state)
+{
+	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
+	struct vkms_output *output = &vkmsdev->output;
+	struct drm_writeback_connector *mw_conn = &output->mw_connector;
+
+	struct drm_connector_state *conn_state = mw_conn->base.state;
+
+	struct vkms_connector_state *vkms_conn = to_wb_state(conn_state);
+	struct vkms_crc_data *primary_plane = NULL;
+	struct drm_plane *plane;
+
+	if (!conn_state)
+		return;
+
+	if (conn_state->writeback_job && conn_state->writeback_job->fb) {
+		struct drm_framebuffer *fb = conn_state->writeback_job->fb;
+		struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+		struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+
+		vkms_conn->writeback_vaddr = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
+
+		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
+		conn_state->writeback_job = NULL;
+
+		drm_for_each_plane(plane, conn->dev) {
+			struct vkms_plane_state *vplane_state;
+			struct vkms_crc_data *crc_data;
+
+			vplane_state = to_vkms_plane_state(plane->state);
+			crc_data = vplane_state->crc_data;
+
+			if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+				continue;
+
+			if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+				primary_plane = crc_data;
+		}
+
+		memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);
+	}
+}
+
 static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
+	.atomic_commit = vkms_mw_atomic_commit,
+};
+
+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
+	.atomic_check = vkms_mw_encoder_atomic_check,
 };
 
 int vkms_output_init(struct vkms_device *vkmsdev)
@@ -62,8 +191,24 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	if (ret)
 		goto err_crtc;
 
-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
+	if (vkmsdev->output.writeback_enabled) {
+		const u32 *formats = vkms_formats;
+		int n_formats = ARRAY_SIZE(vkms_formats);
+		struct drm_writeback_connector *wb_connector;
+
+		vkmsdev->output.mw_connector.encoder.possible_crtcs = 1;
+
+		wb_connector = &vkmsdev->output.mw_connector;
+		connector = &wb_connector->base;
+		ret = drm_writeback_connector_init(dev, wb_connector,
+						   &vkms_connector_funcs,
+						   &vkms_encoder_helper_funcs,
+						   formats, n_formats);
+	} else {
+		ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
+					 DRM_MODE_CONNECTOR_VIRTUAL);
+	}
+
 	if (ret) {
 		DRM_ERROR("Failed to init connector\n");
 		goto err_connector;
@@ -77,18 +222,21 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 		goto err_connector_register;
 	}
 
-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
-	if (ret) {
-		DRM_ERROR("Failed to init encoder\n");
-		goto err_encoder;
-	}
 	encoder->possible_crtcs = 1;
 
-	ret = drm_connector_attach_encoder(connector, encoder);
-	if (ret) {
-		DRM_ERROR("Failed to attach connector to encoder\n");
-		goto err_attach;
+	if (!vkmsdev->output.writeback_enabled) {
+		ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
+				       DRM_MODE_ENCODER_VIRTUAL, NULL);
+		if (ret) {
+			DRM_ERROR("Failed to init encoder\n");
+			goto err_encoder;
+		}
+
+		ret = drm_connector_attach_encoder(connector, encoder);
+		if (ret) {
+			DRM_ERROR("Failed to attach connector to encoder\n");
+			goto err_attach;
+		}
 	}
 
 	drm_mode_config_reset(dev);
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0e67d2d42f0c..14259ffe47d8 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -190,7 +190,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 		funcs = &vkms_primary_helper_funcs;
 	}
 
-	ret = drm_universal_plane_init(dev, plane, 0,
+	ret = drm_universal_plane_init(dev, plane, 1,
 				       &vkms_plane_funcs,
 				       formats, nformats,
 				       NULL, type, NULL);
-- 
2.21.0


-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/vkms: Add writeback support

[Liviu Dudau]

On Sun, Mar 10, 2019 at 06:20:34PM -0300, Rodrigo Siqueira wrote:
> Hi,

Hi Rodrigo,

> 
> In the last few weeks, I was focused on implementing the writeback on
> VKMS by using the feedback that I got from Brian Starkey and Daniel
> Vetter in my previous attempt [1]. For testing/guiding my
> implementation, I’m using a patchset made by Liviu and Brian that adds
> writeback tests to IGT [2]. This patch, already pass the basic
> requirements and some of the other tests.
> 
> So, in this implementation I adopted the following design: you can use
> writeback_connector, or you can have a virtual connector, but you cannot
> have both. Should I keep the virtual connector always available and make
> it possible to add writeback connector as the secondary connector? If
> so, there's any IGT test for this?

Given that the writeback is one-shot (i.e. no output will be generated
after the commit that provided a writeback framebuffer), how does that play
with your userspace? I think that unless you have a specific reason for it,
having both connectors available all the time makes sense, and the writeback
connector only gets used by the userspace that sets the right CAPS.

> 
> Secondly, I’m facing problems related to
> ‘drm_writeback_signal_completion()’, for some reason that I did not know
> yet I continuously get the following error in a loop:
> 
> [  +0.000060] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G      D W         5.0.0-VKMS #102
> [  +0.000004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> [  +0.000033] RIP: 0010:drm_writeback_signal_completion+0x10c/0x130 [drm]
> [  +0.000006] Code: 01 00 00 48 89 43 e8 48 89 43 f0 48 8b 35 3c 67 b3 e4 5b 5d 41 5c 41 5d 41 5e e9 0f 7d aa e3 4c 89 ee 4c 89 e7 e8 a4 4e 18 e4 <0f> 0b 5b 5d 41 5c 41 5d 41 5e c3 e8 54 2f f8 e3 eb 9a 0f 0b e9 60
> [  +0.000004] RSP: 0018:ffff98593cb83ec0 EFLAGS: 00010082
> [  +0.000006] RAX: 0000000080010002 RBX: ffff98593bbb54b0 RCX: 0000000000000000
> [  +0.000005] RDX: 0000000000000001 RSI: 0000000000000082 RDI: 00000000ffffffff
> [  +0.000004] RBP: ffff98593bbb54b0 R08: 0000000000000000 R09: ffff98593cb83e20
> [  +0.000004] R10: ffff98593a07d600 R11: 0000000000000000 R12: ffff98593bbb54a8
> [  +0.000004] R13: 0000000000000082 R14: 0000000000000000 R15: ffff98593cb9cd38
> [  +0.000006] FS:  0000000000000000(0000) GS:ffff98593cb80000(0000) knlGS:0000000000000000
> [  +0.000004] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  +0.000005] CR2: 00007f3cea2e8000 CR3: 00000000ba6ec000 CR4: 00000000000006e0
> [  +0.000010] Call Trace:
> [  +0.000007]  <IRQ>
> [  +0.000013]  ? vkms_disable_vblank+0x20/0x20 [vkms]
> [  +0.000008]  vkms_vblank_simulate+0xef/0x110 [vkms]
> [  +0.000008]  ? vkms_disable_vblank+0x20/0x20 [vkms]
> [  +0.000007]  __hrtimer_run_queues+0x100/0x2d0
> [  +0.000008]  hrtimer_interrupt+0x10a/0x220
> [  +0.000008]  ? kvm_sched_clock_read+0x5/0x10
> [  +0.000010]  smp_apic_timer_interrupt+0x69/0x160
> [  +0.000007]  apic_timer_interrupt+0xf/0x20
> [  +0.000004]  </IRQ>
> [  +0.000008] RIP: 0010:native_safe_halt+0x2/0x10
> [  +0.000005] Code: 5b ff ff ff 7f 5b c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 75 c3 eb 8b 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 f4 c3 90 90 90 90 90 90
> [  +0.000005] RSP: 0018:ffffb937c06b3eb8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
> [  +0.000005] RAX: 0000000000000003 RBX: 0000000000000003 RCX: 0000000000000001
> [  +0.000004] RDX: 0000000000000001 RSI: ffffffffa4e6da3e RDI: ffffffffa4e6dd30
> [  +0.000005] RBP: ffffffffa51252e0 R08: ffffffffa55e7438 R09: 0000000000000000
> [  +0.000003] R10: 0000000000000000 R11: ffffffffa5049fb8 R12: 0000000000000000
> [  +0.000004] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [  +0.000012]  default_idle+0x1a/0x170
> [  +0.000010]  do_idle+0x1d5/0x250
> [  +0.000007]  cpu_startup_entry+0x19/0x20
> [  +0.000007]  start_secondary+0x1aa/0x200
> [  +0.000008]  secondary_startup_64+0xa4/0xb0
> [  +0.000008] ---[ end trace ec12666e4ecb96f7 ]---
> 
> Any ideas? Or any comments?

What line does the "drm_writeback_signal_completion+0x10c/0x130" address
resolves to? Also, am I reading correctly that vkms_disable_vblank()
gets called (twice) in the IRQ handler? Does that make sense?

The only thing that I can suggest at the moment is to have a look in your
code and check if there are any assumptions regarding vblank interrupts
being generated for writeback connectors. Usually the hardware that supports
writeback functionality is only going to generate an userspace event at
the end of the writeback so you might have to use the drm_atomic_helper_fake_vblank()
function for the rest of the time.

Best regards,
Liviu

> 
> Thanks
> Best Regards
> 
> 1. https://patchwork.freedesktop.org/patch/257771/?series=51325&rev=1
> 2. https://patchwork.freedesktop.org/series/39229/
> 
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c   |   3 +
>  drivers/gpu/drm/vkms/vkms_drv.c    |   8 ++
>  drivers/gpu/drm/vkms/vkms_drv.h    |  21 ++++
>  drivers/gpu/drm/vkms/vkms_output.c | 176 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/vkms/vkms_plane.c  |   2 +-
>  5 files changed, 195 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..d9948208e8eb 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -19,6 +19,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
>  
> +	if (output->writeback_enabled)
> +		drm_writeback_signal_completion(&output->mw_connector, 0);
> +
>  	if (state && output->crc_enabled) {
>  		u64 frame = drm_crtc_accurate_vblank_count(crtc);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 738dd6206d85..e0d7f94e3972 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -29,6 +29,10 @@ bool enable_cursor;
>  module_param_named(enable_cursor, enable_cursor, bool, 0444);
>  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>  
> +int enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, int, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
>  static const struct file_operations vkms_driver_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= drm_open,
> @@ -123,6 +127,10 @@ static int __init vkms_init(void)
>  		goto out_fini;
>  	}
>  
> +	vkms_device->output.writeback_enabled = enable_writeback;
> +	if (enable_writeback)
> +		DRM_INFO("Writeback connector enabled");
> +
>  	ret = vkms_modeset_init(vkms_device);
>  	if (ret)
>  		goto out_fini;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 81f1cfbeb936..2c26fe686b93 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
>  #include <drm/drm_gem.h>
>  #include <drm/drm_encoder.h>
>  #include <linux/hrtimer.h>
> +#include <drm/drm_writeback.h>
>  
>  #define XRES_MIN    20
>  #define YRES_MIN    20
> @@ -46,6 +47,16 @@ struct vkms_plane_state {
>  	struct vkms_crc_data *crc_data;
>  };
>  
> +/**
> + * vkms_connector_state - Driver connector specific state
> + * @base: base connector state
> + * @writeback_vaddr: keep track for writeback framebuffer destination
> + */
> +struct vkms_connector_state {
> +	struct drm_connector_state base;
> +	void *writeback_vaddr;
> +};
> +
>  /**
>   * vkms_crtc_state - Driver specific CRTC state
>   * @base: base CRTC state
> @@ -64,10 +75,12 @@ struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
> +	struct drm_writeback_connector mw_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
>  	bool crc_enabled;
> +	int writeback_enabled;
>  	/* ordered wq for crc_work */
>  	struct workqueue_struct *crc_workq;
>  	/* protects concurrent access to crc_data */
> @@ -93,6 +106,12 @@ struct vkms_gem_object {
>  #define drm_crtc_to_vkms_output(target) \
>  	container_of(target, struct vkms_output, crtc)
>  
> +#define drm_connector_to_wb_connector(target) \
> +	container_of(target, struct drm_writeback_connector, base)
> +
> +#define drm_wb_to_vkms_output(target) \
> +	container_of(target, struct vkms_output, mw_connector)
> +
>  #define drm_device_to_vkms_device(target) \
>  	container_of(target, struct vkms_device, drm)
>  
> @@ -105,6 +124,8 @@ struct vkms_gem_object {
>  #define to_vkms_plane_state(target)\
>  	container_of(target, struct vkms_plane_state, base)
>  
> +#define to_wb_state(_state) (struct vkms_connector_state *)(_state)
> +
>  /* CRTC */
>  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		   struct drm_plane *primary, struct drm_plane *cursor);
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 3b162b25312e..0e55e7299d2e 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -3,6 +3,8 @@
>  #include "vkms_drv.h"
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  
>  static void vkms_connector_destroy(struct drm_connector *connector)
>  {
> @@ -10,14 +12,92 @@ static void vkms_connector_destroy(struct drm_connector *connector)
>  	drm_connector_cleanup(connector);
>  }
>  
> +static void vkms_connector_reset(struct drm_connector *connector)
> +{
> +	struct vkms_connector_state *mw_state;
> +	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> +	struct vkms_output *output = drm_wb_to_vkms_output(wb);
> +
> +	if (!output->writeback_enabled)
> +		return drm_atomic_helper_connector_reset(connector);
> +
> +	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> +	if (!mw_state) {
> +		DRM_ERROR("Failed to allocate memory for connector state");
> +		return;
> +	}
> +
> +	if (connector->state)
> +		__drm_atomic_helper_connector_destroy_state(connector->state);
> +
> +	kfree(connector->state);
> +	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +}
> +
> +static struct drm_connector_state *
> +vkms_connector_duplicate_state(struct drm_connector *connector)
> +{
> +	struct vkms_connector_state *mw_state, *mw_current_state;
> +	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> +	struct vkms_output *output = drm_wb_to_vkms_output(wb);
> +
> +	if (!output->writeback_enabled)
> +		return drm_atomic_helper_connector_duplicate_state(connector);
> +
> +	if (WARN_ON(!connector->state))
> +		return NULL;
> +
> +	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> +	if (!mw_state)
> +		return NULL;
> +
> +	mw_current_state = to_wb_state(connector->state);
> +	mw_state->writeback_vaddr = mw_current_state->writeback_vaddr;
> +
> +	__drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
> +
> +	return &mw_state->base;
> +}
> +
>  static const struct drm_connector_funcs vkms_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = vkms_connector_destroy,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.reset = vkms_connector_reset,
> +	.atomic_duplicate_state = vkms_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static int vkms_mw_encoder_atomic_check(struct drm_encoder *encoder,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != crtc_state->mode.hdisplay ||
> +	    fb->height != crtc_state->mode.vdisplay) {
> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> +			      fb->width, fb->height);
> +		return -EINVAL;
> +	}
> +
> +	if (fb->format->format != DRM_FORMAT_XRGB8888) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(fb->format->format,
> +						  &format_name));
> +		return -EINVAL;
> +	}
> +
> +	// TODO: Do we need to validate each plane?
> +
> +	return 0;
> +}
> +
>  static const struct drm_encoder_funcs vkms_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
> @@ -32,8 +112,57 @@ static int vkms_conn_get_modes(struct drm_connector *connector)
>  	return count;
>  }
>  
> +static void vkms_mw_atomic_commit(struct drm_connector *conn,
> +				  struct drm_connector_state *state)
> +{
> +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> +	struct vkms_output *output = &vkmsdev->output;
> +	struct drm_writeback_connector *mw_conn = &output->mw_connector;
> +
> +	struct drm_connector_state *conn_state = mw_conn->base.state;
> +
> +	struct vkms_connector_state *vkms_conn = to_wb_state(conn_state);
> +	struct vkms_crc_data *primary_plane = NULL;
> +	struct drm_plane *plane;
> +
> +	if (!conn_state)
> +		return;
> +
> +	if (conn_state->writeback_job && conn_state->writeback_job->fb) {
> +		struct drm_framebuffer *fb = conn_state->writeback_job->fb;
> +		struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> +		struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +
> +		vkms_conn->writeback_vaddr = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> +
> +		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
> +		conn_state->writeback_job = NULL;
> +
> +		drm_for_each_plane(plane, conn->dev) {
> +			struct vkms_plane_state *vplane_state;
> +			struct vkms_crc_data *crc_data;
> +
> +			vplane_state = to_vkms_plane_state(plane->state);
> +			crc_data = vplane_state->crc_data;
> +
> +			if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> +				continue;
> +
> +			if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +				primary_plane = crc_data;
> +		}
> +
> +		memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);
> +	}
> +}
> +
>  static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  	.get_modes    = vkms_conn_get_modes,
> +	.atomic_commit = vkms_mw_atomic_commit,
> +};
> +
> +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> +	.atomic_check = vkms_mw_encoder_atomic_check,
>  };
>  
>  int vkms_output_init(struct vkms_device *vkmsdev)
> @@ -62,8 +191,24 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  	if (ret)
>  		goto err_crtc;
>  
> -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VIRTUAL);
> +	if (vkmsdev->output.writeback_enabled) {
> +		const u32 *formats = vkms_formats;
> +		int n_formats = ARRAY_SIZE(vkms_formats);
> +		struct drm_writeback_connector *wb_connector;
> +
> +		vkmsdev->output.mw_connector.encoder.possible_crtcs = 1;
> +
> +		wb_connector = &vkmsdev->output.mw_connector;
> +		connector = &wb_connector->base;
> +		ret = drm_writeback_connector_init(dev, wb_connector,
> +						   &vkms_connector_funcs,
> +						   &vkms_encoder_helper_funcs,
> +						   formats, n_formats);
> +	} else {
> +		ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> +					 DRM_MODE_CONNECTOR_VIRTUAL);
> +	}
> +
>  	if (ret) {
>  		DRM_ERROR("Failed to init connector\n");
>  		goto err_connector;
> @@ -77,18 +222,21 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  		goto err_connector_register;
>  	}
>  
> -	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> -	if (ret) {
> -		DRM_ERROR("Failed to init encoder\n");
> -		goto err_encoder;
> -	}
>  	encoder->possible_crtcs = 1;
>  
> -	ret = drm_connector_attach_encoder(connector, encoder);
> -	if (ret) {
> -		DRM_ERROR("Failed to attach connector to encoder\n");
> -		goto err_attach;
> +	if (!vkmsdev->output.writeback_enabled) {
> +		ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> +				       DRM_MODE_ENCODER_VIRTUAL, NULL);
> +		if (ret) {
> +			DRM_ERROR("Failed to init encoder\n");
> +			goto err_encoder;
> +		}
> +
> +		ret = drm_connector_attach_encoder(connector, encoder);
> +		if (ret) {
> +			DRM_ERROR("Failed to attach connector to encoder\n");
> +			goto err_attach;
> +		}
>  	}
>  
>  	drm_mode_config_reset(dev);
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 0e67d2d42f0c..14259ffe47d8 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -190,7 +190,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  		funcs = &vkms_primary_helper_funcs;
>  	}
>  
> -	ret = drm_universal_plane_init(dev, plane, 0,
> +	ret = drm_universal_plane_init(dev, plane, 1,
>  				       &vkms_plane_funcs,
>  				       formats, nformats,
>  				       NULL, type, NULL);
> -- 
> 2.21.0
> 
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech
> Graduate Student
> Department of Computer Science
> University of São Paulo

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/vkms: Add writeback support

[Rodrigo Siqueira]

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

On 03/11, Liviu Dudau wrote:
> On Sun, Mar 10, 2019 at 06:20:34PM -0300, Rodrigo Siqueira wrote:
> > Hi,
> 
> Hi Rodrigo,
> 
> > 
> > In the last few weeks, I was focused on implementing the writeback on
> > VKMS by using the feedback that I got from Brian Starkey and Daniel
> > Vetter in my previous attempt [1]. For testing/guiding my
> > implementation, I’m using a patchset made by Liviu and Brian that adds
> > writeback tests to IGT [2]. This patch, already pass the basic
> > requirements and some of the other tests.
> > 
> > So, in this implementation I adopted the following design: you can use
> > writeback_connector, or you can have a virtual connector, but you cannot
> > have both. Should I keep the virtual connector always available and make
> > it possible to add writeback connector as the secondary connector? If
> > so, there's any IGT test for this?
> 
> Given that the writeback is one-shot (i.e. no output will be generated
> after the commit that provided a writeback framebuffer), how does that play
> with your userspace? I think that unless you have a specific reason for it,
> having both connectors available all the time makes sense, and the writeback
> connector only gets used by the userspace that sets the right CAPS.

Hi Liviu,

First of all, thank you for the feedback.

Originally, my final goal was to use writeback feature for adding a
visual output for VKMS. I want to make the userspace aware of the
writeback in the VKMS, and use it for displaying things. However, at
this moment, I just want to make sure that VKMS can pass in all of the
tests in the kms_writeback.

Anyway, I agree with your point about keep both connectors enabled. I
will fix it, thanks for shed lights on this issue.

Also, thank you for the given ideas related to the to
'drm_writeback_signal_completion()' problem in my implementation. I will
take a careful look at it; I’m suspecting that I’m making some wrong
assumptions...

Best Regards
Rodrigo Siqueira
 
> > 
> > Secondly, I’m facing problems related to
> > ‘drm_writeback_signal_completion()’, for some reason that I did not know
> > yet I continuously get the following error in a loop:
> > 
> > [  +0.000060] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G      D W         5.0.0-VKMS #102
> > [  +0.000004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> > [  +0.000033] RIP: 0010:drm_writeback_signal_completion+0x10c/0x130 [drm]
> > [  +0.000006] Code: 01 00 00 48 89 43 e8 48 89 43 f0 48 8b 35 3c 67 b3 e4 5b 5d 41 5c 41 5d 41 5e e9 0f 7d aa e3 4c 89 ee 4c 89 e7 e8 a4 4e 18 e4 <0f> 0b 5b 5d 41 5c 41 5d 41 5e c3 e8 54 2f f8 e3 eb 9a 0f 0b e9 60
> > [  +0.000004] RSP: 0018:ffff98593cb83ec0 EFLAGS: 00010082
> > [  +0.000006] RAX: 0000000080010002 RBX: ffff98593bbb54b0 RCX: 0000000000000000
> > [  +0.000005] RDX: 0000000000000001 RSI: 0000000000000082 RDI: 00000000ffffffff
> > [  +0.000004] RBP: ffff98593bbb54b0 R08: 0000000000000000 R09: ffff98593cb83e20
> > [  +0.000004] R10: ffff98593a07d600 R11: 0000000000000000 R12: ffff98593bbb54a8
> > [  +0.000004] R13: 0000000000000082 R14: 0000000000000000 R15: ffff98593cb9cd38
> > [  +0.000006] FS:  0000000000000000(0000) GS:ffff98593cb80000(0000) knlGS:0000000000000000
> > [  +0.000004] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  +0.000005] CR2: 00007f3cea2e8000 CR3: 00000000ba6ec000 CR4: 00000000000006e0
> > [  +0.000010] Call Trace:
> > [  +0.000007]  <IRQ>
> > [  +0.000013]  ? vkms_disable_vblank+0x20/0x20 [vkms]
> > [  +0.000008]  vkms_vblank_simulate+0xef/0x110 [vkms]
> > [  +0.000008]  ? vkms_disable_vblank+0x20/0x20 [vkms]
> > [  +0.000007]  __hrtimer_run_queues+0x100/0x2d0
> > [  +0.000008]  hrtimer_interrupt+0x10a/0x220
> > [  +0.000008]  ? kvm_sched_clock_read+0x5/0x10
> > [  +0.000010]  smp_apic_timer_interrupt+0x69/0x160
> > [  +0.000007]  apic_timer_interrupt+0xf/0x20
> > [  +0.000004]  </IRQ>
> > [  +0.000008] RIP: 0010:native_safe_halt+0x2/0x10
> > [  +0.000005] Code: 5b ff ff ff 7f 5b c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 75 c3 eb 8b 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 f4 c3 90 90 90 90 90 90
> > [  +0.000005] RSP: 0018:ffffb937c06b3eb8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
> > [  +0.000005] RAX: 0000000000000003 RBX: 0000000000000003 RCX: 0000000000000001
> > [  +0.000004] RDX: 0000000000000001 RSI: ffffffffa4e6da3e RDI: ffffffffa4e6dd30
> > [  +0.000005] RBP: ffffffffa51252e0 R08: ffffffffa55e7438 R09: 0000000000000000
> > [  +0.000003] R10: 0000000000000000 R11: ffffffffa5049fb8 R12: 0000000000000000
> > [  +0.000004] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > [  +0.000012]  default_idle+0x1a/0x170
> > [  +0.000010]  do_idle+0x1d5/0x250
> > [  +0.000007]  cpu_startup_entry+0x19/0x20
> > [  +0.000007]  start_secondary+0x1aa/0x200
> > [  +0.000008]  secondary_startup_64+0xa4/0xb0
> > [  +0.000008] ---[ end trace ec12666e4ecb96f7 ]---
> > 
> > Any ideas? Or any comments?
> 
> What line does the "drm_writeback_signal_completion+0x10c/0x130" address
> resolves to? Also, am I reading correctly that vkms_disable_vblank()
> gets called (twice) in the IRQ handler? Does that make sense?
> 
> The only thing that I can suggest at the moment is to have a look in your
> code and check if there are any assumptions regarding vblank interrupts
> being generated for writeback connectors. Usually the hardware that supports
> writeback functionality is only going to generate an userspace event at
> the end of the writeback so you might have to use the drm_atomic_helper_fake_vblank()
> function for the rest of the time.
> 
> Best regards,
> Liviu
> 
> > 
> > Thanks
> > Best Regards
> > 
> > 1. https://patchwork.freedesktop.org/patch/257771/?series=51325&rev=1
> > 2. https://patchwork.freedesktop.org/series/39229/
> > 
> > ---
> >  drivers/gpu/drm/vkms/vkms_crtc.c   |   3 +
> >  drivers/gpu/drm/vkms/vkms_drv.c    |   8 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h    |  21 ++++
> >  drivers/gpu/drm/vkms/vkms_output.c | 176 ++++++++++++++++++++++++++---
> >  drivers/gpu/drm/vkms/vkms_plane.c  |   2 +-
> >  5 files changed, 195 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 8a9aeb0a9ea8..d9948208e8eb 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -19,6 +19,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> >  
> > +	if (output->writeback_enabled)
> > +		drm_writeback_signal_completion(&output->mw_connector, 0);
> > +
> >  	if (state && output->crc_enabled) {
> >  		u64 frame = drm_crtc_accurate_vblank_count(crtc);
> >  
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 738dd6206d85..e0d7f94e3972 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -29,6 +29,10 @@ bool enable_cursor;
> >  module_param_named(enable_cursor, enable_cursor, bool, 0444);
> >  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> >  
> > +int enable_writeback;
> > +module_param_named(enable_writeback, enable_writeback, int, 0444);
> > +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> > +
> >  static const struct file_operations vkms_driver_fops = {
> >  	.owner		= THIS_MODULE,
> >  	.open		= drm_open,
> > @@ -123,6 +127,10 @@ static int __init vkms_init(void)
> >  		goto out_fini;
> >  	}
> >  
> > +	vkms_device->output.writeback_enabled = enable_writeback;
> > +	if (enable_writeback)
> > +		DRM_INFO("Writeback connector enabled");
> > +
> >  	ret = vkms_modeset_init(vkms_device);
> >  	if (ret)
> >  		goto out_fini;
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 81f1cfbeb936..2c26fe686b93 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -8,6 +8,7 @@
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_encoder.h>
> >  #include <linux/hrtimer.h>
> > +#include <drm/drm_writeback.h>
> >  
> >  #define XRES_MIN    20
> >  #define YRES_MIN    20
> > @@ -46,6 +47,16 @@ struct vkms_plane_state {
> >  	struct vkms_crc_data *crc_data;
> >  };
> >  
> > +/**
> > + * vkms_connector_state - Driver connector specific state
> > + * @base: base connector state
> > + * @writeback_vaddr: keep track for writeback framebuffer destination
> > + */
> > +struct vkms_connector_state {
> > +	struct drm_connector_state base;
> > +	void *writeback_vaddr;
> > +};
> > +
> >  /**
> >   * vkms_crtc_state - Driver specific CRTC state
> >   * @base: base CRTC state
> > @@ -64,10 +75,12 @@ struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector mw_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> >  	bool crc_enabled;
> > +	int writeback_enabled;
> >  	/* ordered wq for crc_work */
> >  	struct workqueue_struct *crc_workq;
> >  	/* protects concurrent access to crc_data */
> > @@ -93,6 +106,12 @@ struct vkms_gem_object {
> >  #define drm_crtc_to_vkms_output(target) \
> >  	container_of(target, struct vkms_output, crtc)
> >  
> > +#define drm_connector_to_wb_connector(target) \
> > +	container_of(target, struct drm_writeback_connector, base)
> > +
> > +#define drm_wb_to_vkms_output(target) \
> > +	container_of(target, struct vkms_output, mw_connector)
> > +
> >  #define drm_device_to_vkms_device(target) \
> >  	container_of(target, struct vkms_device, drm)
> >  
> > @@ -105,6 +124,8 @@ struct vkms_gem_object {
> >  #define to_vkms_plane_state(target)\
> >  	container_of(target, struct vkms_plane_state, base)
> >  
> > +#define to_wb_state(_state) (struct vkms_connector_state *)(_state)
> > +
> >  /* CRTC */
> >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >  		   struct drm_plane *primary, struct drm_plane *cursor);
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 3b162b25312e..0e55e7299d2e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -3,6 +3,8 @@
> >  #include "vkms_drv.h"
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_probe_helper.h>
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> >  
> >  static void vkms_connector_destroy(struct drm_connector *connector)
> >  {
> > @@ -10,14 +12,92 @@ static void vkms_connector_destroy(struct drm_connector *connector)
> >  	drm_connector_cleanup(connector);
> >  }
> >  
> > +static void vkms_connector_reset(struct drm_connector *connector)
> > +{
> > +	struct vkms_connector_state *mw_state;
> > +	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> > +	struct vkms_output *output = drm_wb_to_vkms_output(wb);
> > +
> > +	if (!output->writeback_enabled)
> > +		return drm_atomic_helper_connector_reset(connector);
> > +
> > +	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> > +	if (!mw_state) {
> > +		DRM_ERROR("Failed to allocate memory for connector state");
> > +		return;
> > +	}
> > +
> > +	if (connector->state)
> > +		__drm_atomic_helper_connector_destroy_state(connector->state);
> > +
> > +	kfree(connector->state);
> > +	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> > +}
> > +
> > +static struct drm_connector_state *
> > +vkms_connector_duplicate_state(struct drm_connector *connector)
> > +{
> > +	struct vkms_connector_state *mw_state, *mw_current_state;
> > +	struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> > +	struct vkms_output *output = drm_wb_to_vkms_output(wb);
> > +
> > +	if (!output->writeback_enabled)
> > +		return drm_atomic_helper_connector_duplicate_state(connector);
> > +
> > +	if (WARN_ON(!connector->state))
> > +		return NULL;
> > +
> > +	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> > +	if (!mw_state)
> > +		return NULL;
> > +
> > +	mw_current_state = to_wb_state(connector->state);
> > +	mw_state->writeback_vaddr = mw_current_state->writeback_vaddr;
> > +
> > +	__drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
> > +
> > +	return &mw_state->base;
> > +}
> > +
> >  static const struct drm_connector_funcs vkms_connector_funcs = {
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> >  	.destroy = vkms_connector_destroy,
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.reset = vkms_connector_reset,
> > +	.atomic_duplicate_state = vkms_connector_duplicate_state,
> >  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >  };
> >  
> > +static int vkms_mw_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != crtc_state->mode.hdisplay ||
> > +	    fb->height != crtc_state->mode.vdisplay) {
> > +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > +			      fb->width, fb->height);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fb->format->format != DRM_FORMAT_XRGB8888) {
> > +		struct drm_format_name_buf format_name;
> > +
> > +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > +			      drm_get_format_name(fb->format->format,
> > +						  &format_name));
> > +		return -EINVAL;
> > +	}
> > +
> > +	// TODO: Do we need to validate each plane?
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct drm_encoder_funcs vkms_encoder_funcs = {
> >  	.destroy = drm_encoder_cleanup,
> >  };
> > @@ -32,8 +112,57 @@ static int vkms_conn_get_modes(struct drm_connector *connector)
> >  	return count;
> >  }
> >  
> > +static void vkms_mw_atomic_commit(struct drm_connector *conn,
> > +				  struct drm_connector_state *state)
> > +{
> > +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> > +	struct vkms_output *output = &vkmsdev->output;
> > +	struct drm_writeback_connector *mw_conn = &output->mw_connector;
> > +
> > +	struct drm_connector_state *conn_state = mw_conn->base.state;
> > +
> > +	struct vkms_connector_state *vkms_conn = to_wb_state(conn_state);
> > +	struct vkms_crc_data *primary_plane = NULL;
> > +	struct drm_plane *plane;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (conn_state->writeback_job && conn_state->writeback_job->fb) {
> > +		struct drm_framebuffer *fb = conn_state->writeback_job->fb;
> > +		struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +		struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +		vkms_conn->writeback_vaddr = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> > +
> > +		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
> > +		conn_state->writeback_job = NULL;
> > +
> > +		drm_for_each_plane(plane, conn->dev) {
> > +			struct vkms_plane_state *vplane_state;
> > +			struct vkms_crc_data *crc_data;
> > +
> > +			vplane_state = to_vkms_plane_state(plane->state);
> > +			crc_data = vplane_state->crc_data;
> > +
> > +			if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > +				continue;
> > +
> > +			if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +				primary_plane = crc_data;
> > +		}
> > +
> > +		memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	}
> > +}
> > +
> >  static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> >  	.get_modes    = vkms_conn_get_modes,
> > +	.atomic_commit = vkms_mw_atomic_commit,
> > +};
> > +
> > +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> > +	.atomic_check = vkms_mw_encoder_atomic_check,
> >  };
> >  
> >  int vkms_output_init(struct vkms_device *vkmsdev)
> > @@ -62,8 +191,24 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >  	if (ret)
> >  		goto err_crtc;
> >  
> > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > +	if (vkmsdev->output.writeback_enabled) {
> > +		const u32 *formats = vkms_formats;
> > +		int n_formats = ARRAY_SIZE(vkms_formats);
> > +		struct drm_writeback_connector *wb_connector;
> > +
> > +		vkmsdev->output.mw_connector.encoder.possible_crtcs = 1;
> > +
> > +		wb_connector = &vkmsdev->output.mw_connector;
> > +		connector = &wb_connector->base;
> > +		ret = drm_writeback_connector_init(dev, wb_connector,
> > +						   &vkms_connector_funcs,
> > +						   &vkms_encoder_helper_funcs,
> > +						   formats, n_formats);
> > +	} else {
> > +		ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > +					 DRM_MODE_CONNECTOR_VIRTUAL);
> > +	}
> > +
> >  	if (ret) {
> >  		DRM_ERROR("Failed to init connector\n");
> >  		goto err_connector;
> > @@ -77,18 +222,21 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >  		goto err_connector_register;
> >  	}
> >  
> > -	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> > -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to init encoder\n");
> > -		goto err_encoder;
> > -	}
> >  	encoder->possible_crtcs = 1;
> >  
> > -	ret = drm_connector_attach_encoder(connector, encoder);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to attach connector to encoder\n");
> > -		goto err_attach;
> > +	if (!vkmsdev->output.writeback_enabled) {
> > +		ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> > +				       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > +		if (ret) {
> > +			DRM_ERROR("Failed to init encoder\n");
> > +			goto err_encoder;
> > +		}
> > +
> > +		ret = drm_connector_attach_encoder(connector, encoder);
> > +		if (ret) {
> > +			DRM_ERROR("Failed to attach connector to encoder\n");
> > +			goto err_attach;
> > +		}
> >  	}
> >  
> >  	drm_mode_config_reset(dev);
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 0e67d2d42f0c..14259ffe47d8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -190,7 +190,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> >  		funcs = &vkms_primary_helper_funcs;
> >  	}
> >  
> > -	ret = drm_universal_plane_init(dev, plane, 0,
> > +	ret = drm_universal_plane_init(dev, plane, 1,
> >  				       &vkms_plane_funcs,
> >  				       formats, nformats,
> >  				       NULL, type, NULL);
> > -- 
> > 2.21.0
> > 
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

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

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

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

[RFC] drm/vkms: Add writeback support

[Rodrigo Siqueira]

Hi,

I started to work for add the writeback feature into the VKMS, and this
RFC represents my first draft. As a result, I have some questions which
I list below.

1. In this patch, I replaced the virtual connector by the writeback
connector. Is it a good idea? Or should I keep both connectors? If I
keep both connectors, should I add a module parameter to enable the
writeback?

2. When I added the writeback connector (in
drm_writeback_connector_init()), I noticed that I had to indicate the
struct drm_encoder_helper_funcs. However, I think that VKMS does not
need anything sophisticated related to encoders at the writeback
perspective. Am I right? Or did I missed something?

3. I just discovered that the writeback connectors are not exposed as a
connector for the userspace, as a result, IGT tests do not work and
Wayland is unable to run as well. Simon Ser and Daniel Stone, explained
to me that I have to update the compositor (in my case Weston) to set
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do
not have experience with userspace, anyone can point me out another
aspect that I have to take a look to make the writeback work in the user
space? Also, Is it possible to update the IGT test or do I need to
create a new set of tests?

Thanks!

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_crc.c    |   9 ++-
 drivers/gpu/drm/vkms/vkms_crtc.c   |   2 +
 drivers/gpu/drm/vkms/vkms_drv.h    |   3 +-
 drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++--------
 4 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 9d9e8146db90..5d9ac45e00f2 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
 }
 
 static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
-			      struct vkms_crc_data *cursor_crc)
+			      struct vkms_crc_data *cursor_crc,
+			      struct vkms_output *out)
 {
 	struct drm_framebuffer *fb = &primary_crc->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
 	}
 
 	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
+	// TODO: Remove this copy from crc, and make this sort of operation
+	// generic since we can use in different places
+	memcpy(out->connector.base.state->writeback_job->fb,
+	       vkms_obj->vaddr, vkms_obj->gem.size);
 	mutex_unlock(&vkms_obj->pages_lock);
 
 	if (cursor_crc)
@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work)
 	}
 
 	if (primary_crc)
-		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
+		crc32 = _vkms_get_crc(primary_crc, cursor_crc, out);
 
 	frame_end = drm_crtc_accurate_vblank_count(crtc);
 
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 177bbcb38306..d33ddcc40a29 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 
 	_vblank_handle(output);
 
+	drm_writeback_signal_completion(&output->connector, 0);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 1c93990693e3..1e8184ad872f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -5,6 +5,7 @@
 #include <drm/drm.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_writeback.h>
 #include <linux/hrtimer.h>
 
 #define XRES_MIN    20
@@ -61,7 +62,7 @@ struct vkms_crtc_state {
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
-	struct drm_connector connector;
+	struct drm_writeback_connector connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 271a0eb9042c..46622d012107 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -9,6 +9,13 @@
 #include "vkms_drv.h"
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_writeback.h>
+
+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state)
+
+struct vkms_writeback_connector_state {
+	struct drm_connector_state base;
+};
 
 static void vkms_connector_destroy(struct drm_connector *connector)
 {
@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector)
 	drm_connector_cleanup(connector);
 }
 
+static void vkms_connector_reset(struct drm_connector *connector)
+{
+	struct vkms_writeback_connector_state *wb_state =
+		kzalloc(sizeof(*wb_state), GFP_KERNEL);
+
+	if (connector->state)
+		__drm_atomic_helper_connector_destroy_state(connector->state);
+
+	kfree(connector->state);
+	__drm_atomic_helper_connector_reset(connector, &wb_state->base);
+}
+
+static struct drm_connector_state *
+vkms_connector_duplicate_state(struct drm_connector *connector)
+{
+	struct vkms_writeback_connector_state *wb_state;
+
+	if (WARN_ON(!connector->state))
+		return NULL;
+
+	wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
+	if (!wb_state)
+		return NULL;
+
+	__drm_atomic_helper_connector_duplicate_state(connector,
+						      &wb_state->base);
+
+	return &wb_state->base;
+}
+
+static enum drm_connector_status
+vkms_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
 static const struct drm_connector_funcs vkms_connector_funcs = {
+	.reset = vkms_connector_reset,
+	.detect = vkms_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = vkms_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_duplicate_state = vkms_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
+static int vkms_encoder_atomic_check(struct drm_encoder *encoder,
+				     struct drm_crtc_state *crtc_state,
+				     struct drm_connector_state *conn_state)
+{
+	struct drm_framebuffer *fb;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	fb = conn_state->writeback_job->fb;
+	if (fb->width != crtc_state->mode.hdisplay ||
+	    fb->height != crtc_state->mode.vdisplay) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+			      fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
+	.atomic_check = vkms_encoder_atomic_check,
+};
+
 int vkms_output_init(struct vkms_device *vkmsdev)
 {
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
-	struct drm_connector *connector = &output->connector;
-	struct drm_encoder *encoder = &output->encoder;
+	struct drm_writeback_connector *connector = &output->connector;
 	struct drm_crtc *crtc = &output->crtc;
 	struct drm_plane *primary, *cursor = NULL;
 	int ret;
@@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	if (ret)
 		goto err_crtc;
 
-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
+	ret = drm_writeback_connector_init(dev, connector,
+					   &vkms_connector_funcs,
+					   &vkms_encoder_helper_funcs,
+					   vkms_formats, 1);
 	if (ret) {
 		DRM_ERROR("Failed to init connector\n");
 		goto err_connector;
 	}
 
-	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
+	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
 
-	ret = drm_connector_register(connector);
+	ret = drm_connector_register(&connector->base);
 	if (ret) {
 		DRM_ERROR("Failed to register connector\n");
 		goto err_connector_register;
 	}
 
-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
-	if (ret) {
-		DRM_ERROR("Failed to init encoder\n");
-		goto err_encoder;
-	}
-	encoder->possible_crtcs = 1;
-
-	ret = drm_connector_attach_encoder(connector, encoder);
-	if (ret) {
-		DRM_ERROR("Failed to attach connector to encoder\n");
-		goto err_attach;
-	}
-
 	drm_mode_config_reset(dev);
 
 	return 0;
 
-err_attach:
-	drm_encoder_cleanup(encoder);
-
-err_encoder:
-	drm_connector_unregister(connector);
-
 err_connector_register:
-	drm_connector_cleanup(connector);
+	drm_connector_cleanup(&connector->base);
 
 err_connector:
 	drm_crtc_cleanup(crtc);
-- 
2.19.1


-- 
Rodrigo Siqueira
https://siqueira.tech
https://twitter.com/siqueirajordao
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/vkms: Add writeback support

[Daniel Vetter]

On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote:
> Hi,
> 
> I started to work for add the writeback feature into the VKMS, and this
> RFC represents my first draft. As a result, I have some questions which
> I list below.
> 
> 1. In this patch, I replaced the virtual connector by the writeback
> connector. Is it a good idea? Or should I keep both connectors? If I
> keep both connectors, should I add a module parameter to enable the
> writeback?

Yeah separate connector is imo better, with module config knob. Eventually
we can move that into configfs or similar.

> 2. When I added the writeback connector (in
> drm_writeback_connector_init()), I noticed that I had to indicate the
> struct drm_encoder_helper_funcs. However, I think that VKMS does not
> need anything sophisticated related to encoders at the writeback
> perspective. Am I right? Or did I missed something?

Yeah, you don't need anything fancy in the drm_encoder. I think you could
reuse the same dummy drm_encoder as we use for the normal connector.

> 3. I just discovered that the writeback connectors are not exposed as a
> connector for the userspace, as a result, IGT tests do not work and
> Wayland is unable to run as well. Simon Ser and Daniel Stone, explained
> to me that I have to update the compositor (in my case Weston) to set
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do
> not have experience with userspace, anyone can point me out another
> aspect that I have to take a look to make the writeback work in the user
> space? Also, Is it possible to update the IGT test or do I need to
> create a new set of tests?

Yeah, writeback connector doesn't work like a normal connector, so it's
hidden by default. There should be special writeback connector tests in
igt though, but I guess those haven't been merged yet. Ask Liviu and Brian
from arm where they're stuck. Without those it's going to be rather hard
to make this work correctly.

Cheers, Daniel

> 
> Thanks!
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crc.c    |   9 ++-
>  drivers/gpu/drm/vkms/vkms_crtc.c   |   2 +
>  drivers/gpu/drm/vkms/vkms_drv.h    |   3 +-
>  drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++--------
>  4 files changed, 89 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index 9d9e8146db90..5d9ac45e00f2 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
>  }
>  
>  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> -			      struct vkms_crc_data *cursor_crc)
> +			      struct vkms_crc_data *cursor_crc,
> +			      struct vkms_output *out)
>  {
>  	struct drm_framebuffer *fb = &primary_crc->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> @@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>  	}
>  
>  	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> +	// TODO: Remove this copy from crc, and make this sort of operation
> +	// generic since we can use in different places
> +	memcpy(out->connector.base.state->writeback_job->fb,
> +	       vkms_obj->vaddr, vkms_obj->gem.size);
>  	mutex_unlock(&vkms_obj->pages_lock);
>  
>  	if (cursor_crc)
> @@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	}
>  
>  	if (primary_crc)
> -		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> +		crc32 = _vkms_get_crc(primary_crc, cursor_crc, out);
>  
>  	frame_end = drm_crtc_accurate_vblank_count(crtc);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 177bbcb38306..d33ddcc40a29 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  
>  	_vblank_handle(output);
>  
> +	drm_writeback_signal_completion(&output->connector, 0);
> +
>  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
>  					  output->period_ns);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 1c93990693e3..1e8184ad872f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -5,6 +5,7 @@
>  #include <drm/drm.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
>  #include <linux/hrtimer.h>
>  
>  #define XRES_MIN    20
> @@ -61,7 +62,7 @@ struct vkms_crtc_state {
>  struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
> -	struct drm_connector connector;
> +	struct drm_writeback_connector connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 271a0eb9042c..46622d012107 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -9,6 +9,13 @@
>  #include "vkms_drv.h"
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_writeback.h>
> +
> +#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state)
> +
> +struct vkms_writeback_connector_state {
> +	struct drm_connector_state base;
> +};
>  
>  static void vkms_connector_destroy(struct drm_connector *connector)
>  {
> @@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector)
>  	drm_connector_cleanup(connector);
>  }
>  
> +static void vkms_connector_reset(struct drm_connector *connector)
> +{
> +	struct vkms_writeback_connector_state *wb_state =
> +		kzalloc(sizeof(*wb_state), GFP_KERNEL);
> +
> +	if (connector->state)
> +		__drm_atomic_helper_connector_destroy_state(connector->state);
> +
> +	kfree(connector->state);
> +	__drm_atomic_helper_connector_reset(connector, &wb_state->base);
> +}
> +
> +static struct drm_connector_state *
> +vkms_connector_duplicate_state(struct drm_connector *connector)
> +{
> +	struct vkms_writeback_connector_state *wb_state;
> +
> +	if (WARN_ON(!connector->state))
> +		return NULL;
> +
> +	wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
> +	if (!wb_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_connector_duplicate_state(connector,
> +						      &wb_state->base);
> +
> +	return &wb_state->base;
> +}
> +
> +static enum drm_connector_status
> +vkms_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	return connector_status_connected;
> +}
> +
>  static const struct drm_connector_funcs vkms_connector_funcs = {
> +	.reset = vkms_connector_reset,
> +	.detect = vkms_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = vkms_connector_destroy,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_duplicate_state = vkms_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> @@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  	.get_modes    = vkms_conn_get_modes,
>  };
>  
> +static int vkms_encoder_atomic_check(struct drm_encoder *encoder,
> +				     struct drm_crtc_state *crtc_state,
> +				     struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != crtc_state->mode.hdisplay ||
> +	    fb->height != crtc_state->mode.vdisplay) {
> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> +			      fb->width, fb->height);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> +	.atomic_check = vkms_encoder_atomic_check,
> +};
> +
>  int vkms_output_init(struct vkms_device *vkmsdev)
>  {
>  	struct vkms_output *output = &vkmsdev->output;
>  	struct drm_device *dev = &vkmsdev->drm;
> -	struct drm_connector *connector = &output->connector;
> -	struct drm_encoder *encoder = &output->encoder;
> +	struct drm_writeback_connector *connector = &output->connector;
>  	struct drm_crtc *crtc = &output->crtc;
>  	struct drm_plane *primary, *cursor = NULL;
>  	int ret;
> @@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  	if (ret)
>  		goto err_crtc;
>  
> -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VIRTUAL);
> +	ret = drm_writeback_connector_init(dev, connector,
> +					   &vkms_connector_funcs,
> +					   &vkms_encoder_helper_funcs,
> +					   vkms_formats, 1);
>  	if (ret) {
>  		DRM_ERROR("Failed to init connector\n");
>  		goto err_connector;
>  	}
>  
> -	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> +	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
>  
> -	ret = drm_connector_register(connector);
> +	ret = drm_connector_register(&connector->base);
>  	if (ret) {
>  		DRM_ERROR("Failed to register connector\n");
>  		goto err_connector_register;
>  	}
>  
> -	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> -	if (ret) {
> -		DRM_ERROR("Failed to init encoder\n");
> -		goto err_encoder;
> -	}
> -	encoder->possible_crtcs = 1;
> -
> -	ret = drm_connector_attach_encoder(connector, encoder);
> -	if (ret) {
> -		DRM_ERROR("Failed to attach connector to encoder\n");
> -		goto err_attach;
> -	}
> -
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
>  
> -err_attach:
> -	drm_encoder_cleanup(encoder);
> -
> -err_encoder:
> -	drm_connector_unregister(connector);
> -
>  err_connector_register:
> -	drm_connector_cleanup(connector);
> +	drm_connector_cleanup(&connector->base);
>  
>  err_connector:
>  	drm_crtc_cleanup(crtc);
> -- 
> 2.19.1
> 
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech
> https://twitter.com/siqueirajordao
> Graduate Student
> Department of Computer Science
> University of São Paulo

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/vkms: Add writeback support

[Rodrigo Siqueira]

On 10/22, Daniel Vetter wrote:
> On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote:
> > Hi,
> > 
> > I started to work for add the writeback feature into the VKMS, and this
> > RFC represents my first draft. As a result, I have some questions which
> > I list below.
> > 
> > 1. In this patch, I replaced the virtual connector by the writeback
> > connector. Is it a good idea? Or should I keep both connectors? If I
> > keep both connectors, should I add a module parameter to enable the
> > writeback?
> 
> Yeah separate connector is imo better, with module config knob. Eventually
> we can move that into configfs or similar.
> 
> > 2. When I added the writeback connector (in
> > drm_writeback_connector_init()), I noticed that I had to indicate the
> > struct drm_encoder_helper_funcs. However, I think that VKMS does not
> > need anything sophisticated related to encoders at the writeback
> > perspective. Am I right? Or did I missed something?
> 
> Yeah, you don't need anything fancy in the drm_encoder. I think you could
> reuse the same dummy drm_encoder as we use for the normal connector.
> 
> > 3. I just discovered that the writeback connectors are not exposed as a
> > connector for the userspace, as a result, IGT tests do not work and
> > Wayland is unable to run as well. Simon Ser and Daniel Stone, explained
> > to me that I have to update the compositor (in my case Weston) to set
> > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do
> > not have experience with userspace, anyone can point me out another
> > aspect that I have to take a look to make the writeback work in the user
> > space? Also, Is it possible to update the IGT test or do I need to
> > create a new set of tests?
> 
> Yeah, writeback connector doesn't work like a normal connector, so it's
> hidden by default. There should be special writeback connector tests in
> igt though, but I guess those haven't been merged yet. Ask Liviu and Brian
> from arm where they're stuck. Without those it's going to be rather hard
> to make this work correctly.

Hi, thanks for the feedbacks

I will improve the code based on your comments, and for the testing
mechanism, I want to try to make rootston run on top of VKMS.

Best Regards
 
> Cheers, Daniel
> 
> > 
> > Thanks!
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_crc.c    |   9 ++-
> >  drivers/gpu/drm/vkms/vkms_crtc.c   |   2 +
> >  drivers/gpu/drm/vkms/vkms_drv.h    |   3 +-
> >  drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++--------
> >  4 files changed, 89 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > index 9d9e8146db90..5d9ac45e00f2 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > @@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
> >  }
> >  
> >  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > -			      struct vkms_crc_data *cursor_crc)
> > +			      struct vkms_crc_data *cursor_crc,
> > +			      struct vkms_output *out)
> >  {
> >  	struct drm_framebuffer *fb = &primary_crc->fb;
> >  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > @@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> >  	}
> >  
> >  	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	// TODO: Remove this copy from crc, and make this sort of operation
> > +	// generic since we can use in different places
> > +	memcpy(out->connector.base.state->writeback_job->fb,
> > +	       vkms_obj->vaddr, vkms_obj->gem.size);
> >  	mutex_unlock(&vkms_obj->pages_lock);
> >  
> >  	if (cursor_crc)
> > @@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> >  	}
> >  
> >  	if (primary_crc)
> > -		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc, out);
> >  
> >  	frame_end = drm_crtc_accurate_vblank_count(crtc);
> >  
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 177bbcb38306..d33ddcc40a29 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  
> >  	_vblank_handle(output);
> >  
> > +	drm_writeback_signal_completion(&output->connector, 0);
> > +
> >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> >  					  output->period_ns);
> >  
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 1c93990693e3..1e8184ad872f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -5,6 +5,7 @@
> >  #include <drm/drm.h>
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_writeback.h>
> >  #include <linux/hrtimer.h>
> >  
> >  #define XRES_MIN    20
> > @@ -61,7 +62,7 @@ struct vkms_crtc_state {
> >  struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> > -	struct drm_connector connector;
> > +	struct drm_writeback_connector connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 271a0eb9042c..46622d012107 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -9,6 +9,13 @@
> >  #include "vkms_drv.h"
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_writeback.h>
> > +
> > +#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state)
> > +
> > +struct vkms_writeback_connector_state {
> > +	struct drm_connector_state base;
> > +};
> >  
> >  static void vkms_connector_destroy(struct drm_connector *connector)
> >  {
> > @@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector)
> >  	drm_connector_cleanup(connector);
> >  }
> >  
> > +static void vkms_connector_reset(struct drm_connector *connector)
> > +{
> > +	struct vkms_writeback_connector_state *wb_state =
> > +		kzalloc(sizeof(*wb_state), GFP_KERNEL);
> > +
> > +	if (connector->state)
> > +		__drm_atomic_helper_connector_destroy_state(connector->state);
> > +
> > +	kfree(connector->state);
> > +	__drm_atomic_helper_connector_reset(connector, &wb_state->base);
> > +}
> > +
> > +static struct drm_connector_state *
> > +vkms_connector_duplicate_state(struct drm_connector *connector)
> > +{
> > +	struct vkms_writeback_connector_state *wb_state;
> > +
> > +	if (WARN_ON(!connector->state))
> > +		return NULL;
> > +
> > +	wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
> > +	if (!wb_state)
> > +		return NULL;
> > +
> > +	__drm_atomic_helper_connector_duplicate_state(connector,
> > +						      &wb_state->base);
> > +
> > +	return &wb_state->base;
> > +}
> > +
> > +static enum drm_connector_status
> > +vkms_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +	return connector_status_connected;
> > +}
> > +
> >  static const struct drm_connector_funcs vkms_connector_funcs = {
> > +	.reset = vkms_connector_reset,
> > +	.detect = vkms_connector_detect,
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> >  	.destroy = vkms_connector_destroy,
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_duplicate_state = vkms_connector_duplicate_state,
> >  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >  };
> >  
> > @@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> >  	.get_modes    = vkms_conn_get_modes,
> >  };
> >  
> > +static int vkms_encoder_atomic_check(struct drm_encoder *encoder,
> > +				     struct drm_crtc_state *crtc_state,
> > +				     struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != crtc_state->mode.hdisplay ||
> > +	    fb->height != crtc_state->mode.vdisplay) {
> > +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > +			      fb->width, fb->height);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> > +	.atomic_check = vkms_encoder_atomic_check,
> > +};
> > +
> >  int vkms_output_init(struct vkms_device *vkmsdev)
> >  {
> >  	struct vkms_output *output = &vkmsdev->output;
> >  	struct drm_device *dev = &vkmsdev->drm;
> > -	struct drm_connector *connector = &output->connector;
> > -	struct drm_encoder *encoder = &output->encoder;
> > +	struct drm_writeback_connector *connector = &output->connector;
> >  	struct drm_crtc *crtc = &output->crtc;
> >  	struct drm_plane *primary, *cursor = NULL;
> >  	int ret;
> > @@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >  	if (ret)
> >  		goto err_crtc;
> >  
> > -	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_VIRTUAL);
> > +	ret = drm_writeback_connector_init(dev, connector,
> > +					   &vkms_connector_funcs,
> > +					   &vkms_encoder_helper_funcs,
> > +					   vkms_formats, 1);
> >  	if (ret) {
> >  		DRM_ERROR("Failed to init connector\n");
> >  		goto err_connector;
> >  	}
> >  
> > -	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> > +	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
> >  
> > -	ret = drm_connector_register(connector);
> > +	ret = drm_connector_register(&connector->base);
> >  	if (ret) {
> >  		DRM_ERROR("Failed to register connector\n");
> >  		goto err_connector_register;
> >  	}
> >  
> > -	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> > -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to init encoder\n");
> > -		goto err_encoder;
> > -	}
> > -	encoder->possible_crtcs = 1;
> > -
> > -	ret = drm_connector_attach_encoder(connector, encoder);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to attach connector to encoder\n");
> > -		goto err_attach;
> > -	}
> > -
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> >  
> > -err_attach:
> > -	drm_encoder_cleanup(encoder);
> > -
> > -err_encoder:
> > -	drm_connector_unregister(connector);
> > -
> >  err_connector_register:
> > -	drm_connector_cleanup(connector);
> > +	drm_connector_cleanup(&connector->base);
> >  
> >  err_connector:
> >  	drm_crtc_cleanup(crtc);
> > -- 
> > 2.19.1
> > 
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> > https://twitter.com/siqueirajordao
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [RFC] drm/vkms: Add writeback support

[Brian Starkey]

Hi Rodrigo,

On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote:
>Hi,
>
>I started to work for add the writeback feature into the VKMS, and this
>RFC represents my first draft. As a result, I have some questions which
>I list below.
>
>1. In this patch, I replaced the virtual connector by the writeback
>connector. Is it a good idea? Or should I keep both connectors? If I
>keep both connectors, should I add a module parameter to enable the
>writeback?
>
>2. When I added the writeback connector (in
>drm_writeback_connector_init()), I noticed that I had to indicate the
>struct drm_encoder_helper_funcs. However, I think that VKMS does not
>need anything sophisticated related to encoders at the writeback
>perspective. Am I right? Or did I missed something?
>
>3. I just discovered that the writeback connectors are not exposed as a
>connector for the userspace, as a result, IGT tests do not work and
>Wayland is unable to run as well. Simon Ser and Daniel Stone, explained
>to me that I have to update the compositor (in my case Weston) to set
>DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do
>not have experience with userspace, anyone can point me out another
>aspect that I have to take a look to make the writeback work in the user
>space? Also, Is it possible to update the IGT test or do I need to
>create a new set of tests?

Alex added support for writeback connectors into drm_hwcomposer - you
can see that here[1]

There's also some igt tests like Daniel mentioned - I think Liviu
already pointed you at those.

Some more review below,

Cheers!
-Brian

[1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3

>
>Thanks!
>
>Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>---
> drivers/gpu/drm/vkms/vkms_crc.c    |   9 ++-
> drivers/gpu/drm/vkms/vkms_crtc.c   |   2 +
> drivers/gpu/drm/vkms/vkms_drv.h    |   3 +-
> drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++--------
> 4 files changed, 89 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
>index 9d9e8146db90..5d9ac45e00f2 100644
>--- a/drivers/gpu/drm/vkms/vkms_crc.c
>+++ b/drivers/gpu/drm/vkms/vkms_crc.c
>@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
> }
>
> static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>-			      struct vkms_crc_data *cursor_crc)
>+			      struct vkms_crc_data *cursor_crc,
>+			      struct vkms_output *out)
> {
> 	struct drm_framebuffer *fb = &primary_crc->fb;
> 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> 	}
>
> 	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>+	// TODO: Remove this copy from crc, and make this sort of operation
>+	// generic since we can use in different places
>+	memcpy(out->connector.base.state->writeback_job->fb,
>+	       vkms_obj->vaddr, vkms_obj->gem.size);

This looks like it would crash terribly, you're copying the pixel data
(from vkms_obj->vaddr) into a "struct drm_framebuffer *" which is a
pointer to the struct, rather than somewhere to put the actual
pixels.

The same way you get the vaddr for the input framebuffer, you need to
get the vaddr for the output framebuffer too - via
drm_gem_fb_get_obj() and then converting that to a vkms_gem_object.
Then use that vaddr as the destination of the memcpy. However, see my
comments at the bottom about drm_writeback_queue_job(). You might be
better getting and storing the vaddr during atomic_commit(), rather
than in this function.

In order for this to be a straight memcpy, you need to make sure that
the input and output buffers have the same size, pixel format, same
stride etc. as well so you'll need to check that in atomic_check().
Once it's working you can relax those restrictions by copying a line
at a time or doing pixel format conversion - but I don't think it's
necessary to start with.

There might be an opportunity to skip a copy here too - if you have a
framebuffer attached to the writeback connector, you can skip
allocating "vaddr_out" and just compose the cursor directly into the
output framebuffer, and calculate the CRC on that. That's definitely
an optimisation rather than a requirement though.

> 	mutex_unlock(&vkms_obj->pages_lock);
>
> 	if (cursor_crc)
>@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> 	}
>
> 	if (primary_crc)
>-		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
>+		crc32 = _vkms_get_crc(primary_crc, cursor_crc, out);
>
> 	frame_end = drm_crtc_accurate_vblank_count(crtc);
>
>diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
>index 177bbcb38306..d33ddcc40a29 100644
>--- a/drivers/gpu/drm/vkms/vkms_crtc.c
>+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>
> 	_vblank_handle(output);
>
>+	drm_writeback_signal_completion(&output->connector, 0);
>+
> 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> 					  output->period_ns);
>
>diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>index 1c93990693e3..1e8184ad872f 100644
>--- a/drivers/gpu/drm/vkms/vkms_drv.h
>+++ b/drivers/gpu/drm/vkms/vkms_drv.h
>@@ -5,6 +5,7 @@
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> #include <drm/drm_encoder.h>
>+#include <drm/drm_writeback.h>
> #include <linux/hrtimer.h>
>
> #define XRES_MIN    20
>@@ -61,7 +62,7 @@ struct vkms_crtc_state {
> struct vkms_output {
> 	struct drm_crtc crtc;
> 	struct drm_encoder encoder;
>-	struct drm_connector connector;
>+	struct drm_writeback_connector connector;
> 	struct hrtimer vblank_hrtimer;
> 	ktime_t period_ns;
> 	struct drm_pending_vblank_event *event;
>diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
>index 271a0eb9042c..46622d012107 100644
>--- a/drivers/gpu/drm/vkms/vkms_output.c
>+++ b/drivers/gpu/drm/vkms/vkms_output.c
>@@ -9,6 +9,13 @@
> #include "vkms_drv.h"
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_atomic_helper.h>
>+#include <drm/drm_writeback.h>
>+
>+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state)
>+
>+struct vkms_writeback_connector_state {
>+	struct drm_connector_state base;
>+};
>
> static void vkms_connector_destroy(struct drm_connector *connector)
> {
>@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector)
> 	drm_connector_cleanup(connector);
> }
>
>+static void vkms_connector_reset(struct drm_connector *connector)
>+{
>+	struct vkms_writeback_connector_state *wb_state =
>+		kzalloc(sizeof(*wb_state), GFP_KERNEL);
>+
>+	if (connector->state)
>+		__drm_atomic_helper_connector_destroy_state(connector->state);
>+
>+	kfree(connector->state);
>+	__drm_atomic_helper_connector_reset(connector, &wb_state->base);
>+}
>+
>+static struct drm_connector_state *
>+vkms_connector_duplicate_state(struct drm_connector *connector)
>+{
>+	struct vkms_writeback_connector_state *wb_state;
>+
>+	if (WARN_ON(!connector->state))
>+		return NULL;
>+
>+	wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
>+	if (!wb_state)
>+		return NULL;
>+
>+	__drm_atomic_helper_connector_duplicate_state(connector,
>+						      &wb_state->base);
>+
>+	return &wb_state->base;
>+}
>+
>+static enum drm_connector_status
>+vkms_connector_detect(struct drm_connector *connector, bool force)
>+{
>+	return connector_status_connected;
>+}
>+
> static const struct drm_connector_funcs vkms_connector_funcs = {
>+	.reset = vkms_connector_reset,
>+	.detect = vkms_connector_detect,
> 	.fill_modes = drm_helper_probe_single_connector_modes,
> 	.destroy = vkms_connector_destroy,
>-	.reset = drm_atomic_helper_connector_reset,
>-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>+	.atomic_duplicate_state = vkms_connector_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };

You're missing the call to drm_writeback_queue_job(), which is
required. Your connector needs to have an atomic_commit callback in
its drm_connector_helper_funcs, and in there you need to "queue" the
job, something like:

  struct drm_writeback_connector *wb_conn = &out->connector;
  drm_writeback_queue_job(wb_conn, wb_conn->base.state->writeback_job);
  wb_conn->base.state->writeback_job = NULL;

Because you need to set the job pointer to NULL there, before that
you'll need to stash the destination somewhere else, to be used as the
destination for the memcpy() in _vkms_get_crc(). The simplest thing to
do is probably to just add a new void *writeback_vaddr to
vkms_writeback_connector_state, and set it to the output framebuffer
vaddr before you call _queue_job(). The core will make sure the
framebuffer is properly refcounted until you call
drm_writeback_signal_completion(), so as long as you only call that
after you've finished copying the data, you're safe.

In a "real" HW device, we write that pointer directly to a HW
register, so we don't need to track it in our connector state. But you
don't have a HW register to put it in, so you'll need to track it
yourself :-)

>
>@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> 	.get_modes    = vkms_conn_get_modes,
> };
>
>+static int vkms_encoder_atomic_check(struct drm_encoder *encoder,
>+				     struct drm_crtc_state *crtc_state,
>+				     struct drm_connector_state *conn_state)
>+{
>+	struct drm_framebuffer *fb;
>+
>+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>+		return 0;
>+
>+	fb = conn_state->writeback_job->fb;
>+	if (fb->width != crtc_state->mode.hdisplay ||
>+	    fb->height != crtc_state->mode.vdisplay) {
>+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
>+			      fb->width, fb->height);
>+		return -EINVAL;
>+	}
>+
>+	return 0;
>+}
>+
>+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
>+	.atomic_check = vkms_encoder_atomic_check,
>+};
>+
> int vkms_output_init(struct vkms_device *vkmsdev)
> {
> 	struct vkms_output *output = &vkmsdev->output;
> 	struct drm_device *dev = &vkmsdev->drm;
>-	struct drm_connector *connector = &output->connector;
>-	struct drm_encoder *encoder = &output->encoder;
>+	struct drm_writeback_connector *connector = &output->connector;
> 	struct drm_crtc *crtc = &output->crtc;
> 	struct drm_plane *primary, *cursor = NULL;
> 	int ret;
>@@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> 	if (ret)
> 		goto err_crtc;
>
>-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
>-				 DRM_MODE_CONNECTOR_VIRTUAL);
>+	ret = drm_writeback_connector_init(dev, connector,
>+					   &vkms_connector_funcs,
>+					   &vkms_encoder_helper_funcs,
>+					   vkms_formats, 1);
> 	if (ret) {
> 		DRM_ERROR("Failed to init connector\n");
> 		goto err_connector;
> 	}
>
>-	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
>+	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
>
>-	ret = drm_connector_register(connector);
>+	ret = drm_connector_register(&connector->base);
> 	if (ret) {
> 		DRM_ERROR("Failed to register connector\n");
> 		goto err_connector_register;
> 	}
>
>-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
>-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>-	if (ret) {
>-		DRM_ERROR("Failed to init encoder\n");
>-		goto err_encoder;
>-	}
>-	encoder->possible_crtcs = 1;
>-
>-	ret = drm_connector_attach_encoder(connector, encoder);
>-	if (ret) {
>-		DRM_ERROR("Failed to attach connector to encoder\n");
>-		goto err_attach;
>-	}
>-
> 	drm_mode_config_reset(dev);
>
> 	return 0;
>
>-err_attach:
>-	drm_encoder_cleanup(encoder);
>-
>-err_encoder:
>-	drm_connector_unregister(connector);
>-
> err_connector_register:
>-	drm_connector_cleanup(connector);
>+	drm_connector_cleanup(&connector->base);
>
> err_connector:
> 	drm_crtc_cleanup(crtc);
>-- 
>2.19.1
>
>
>-- 
>Rodrigo Siqueira
>https://siqueira.tech
>https://twitter.com/siqueirajordao
>Graduate Student
>Department of Computer Science
>University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm/vkms: Add writeback support

[Rodrigo Siqueira]

On 10/24, Brian Starkey wrote:
> Hi Rodrigo,

Hi Brian,

Thanks a lot for your feedback! I learned a lot with all of them :)

I will work for make a real PATCH soon.

Again, thanks a lot.

Best Regards

> On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote:
> >Hi,
> >
> >I started to work for add the writeback feature into the VKMS, and this
> >RFC represents my first draft. As a result, I have some questions which
> >I list below.
> >
> >1. In this patch, I replaced the virtual connector by the writeback
> >connector. Is it a good idea? Or should I keep both connectors? If I
> >keep both connectors, should I add a module parameter to enable the
> >writeback?
> >
> >2. When I added the writeback connector (in
> >drm_writeback_connector_init()), I noticed that I had to indicate the
> >struct drm_encoder_helper_funcs. However, I think that VKMS does not
> >need anything sophisticated related to encoders at the writeback
> >perspective. Am I right? Or did I missed something?
> >
> >3. I just discovered that the writeback connectors are not exposed as a
> >connector for the userspace, as a result, IGT tests do not work and
> >Wayland is unable to run as well. Simon Ser and Daniel Stone, explained
> >to me that I have to update the compositor (in my case Weston) to set
> >DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do
> >not have experience with userspace, anyone can point me out another
> >aspect that I have to take a look to make the writeback work in the user
> >space? Also, Is it possible to update the IGT test or do I need to
> >create a new set of tests?
> 
> Alex added support for writeback connectors into drm_hwcomposer - you
> can see that here[1]
> 
> There's also some igt tests like Daniel mentioned - I think Liviu
> already pointed you at those.
> 
> Some more review below,
> 
> Cheers!
> -Brian
> 
> [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
> 
> >
> >Thanks!
> >
> >Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >---
> > drivers/gpu/drm/vkms/vkms_crc.c    |   9 ++-
> > drivers/gpu/drm/vkms/vkms_crtc.c   |   2 +
> > drivers/gpu/drm/vkms/vkms_drv.h    |   3 +-
> > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++--------
> > 4 files changed, 89 insertions(+), 32 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> >index 9d9e8146db90..5d9ac45e00f2 100644
> >--- a/drivers/gpu/drm/vkms/vkms_crc.c
> >+++ b/drivers/gpu/drm/vkms/vkms_crc.c
> >@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > }
> >
> > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> >-			      struct vkms_crc_data *cursor_crc)
> >+			      struct vkms_crc_data *cursor_crc,
> >+			      struct vkms_output *out)
> > {
> > 	struct drm_framebuffer *fb = &primary_crc->fb;
> > 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> >@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > 	}
> >
> > 	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> >+	// TODO: Remove this copy from crc, and make this sort of operation
> >+	// generic since we can use in different places
> >+	memcpy(out->connector.base.state->writeback_job->fb,
> >+	       vkms_obj->vaddr, vkms_obj->gem.size);
> 
> This looks like it would crash terribly, you're copying the pixel data
> (from vkms_obj->vaddr) into a "struct drm_framebuffer *" which is a
> pointer to the struct, rather than somewhere to put the actual
> pixels.
> 
> The same way you get the vaddr for the input framebuffer, you need to
> get the vaddr for the output framebuffer too - via
> drm_gem_fb_get_obj() and then converting that to a vkms_gem_object.
> Then use that vaddr as the destination of the memcpy. However, see my
> comments at the bottom about drm_writeback_queue_job(). You might be
> better getting and storing the vaddr during atomic_commit(), rather
> than in this function.
> 
> In order for this to be a straight memcpy, you need to make sure that
> the input and output buffers have the same size, pixel format, same
> stride etc. as well so you'll need to check that in atomic_check().
> Once it's working you can relax those restrictions by copying a line
> at a time or doing pixel format conversion - but I don't think it's
> necessary to start with.
> 
> There might be an opportunity to skip a copy here too - if you have a
> framebuffer attached to the writeback connector, you can skip
> allocating "vaddr_out" and just compose the cursor directly into the
> output framebuffer, and calculate the CRC on that. That's definitely
> an optimisation rather than a requirement though.
> 
> > 	mutex_unlock(&vkms_obj->pages_lock);
> >
> > 	if (cursor_crc)
> >@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> > 	}
> >
> > 	if (primary_crc)
> >-		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> >+		crc32 = _vkms_get_crc(primary_crc, cursor_crc, out);
> >
> > 	frame_end = drm_crtc_accurate_vblank_count(crtc);
> >
> >diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> >index 177bbcb38306..d33ddcc40a29 100644
> >--- a/drivers/gpu/drm/vkms/vkms_crtc.c
> >+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> >@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >
> > 	_vblank_handle(output);
> >
> >+	drm_writeback_signal_completion(&output->connector, 0);
> >+
> > 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > 					  output->period_ns);
> >
> >diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >index 1c93990693e3..1e8184ad872f 100644
> >--- a/drivers/gpu/drm/vkms/vkms_drv.h
> >+++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >@@ -5,6 +5,7 @@
> > #include <drm/drm.h>
> > #include <drm/drm_gem.h>
> > #include <drm/drm_encoder.h>
> >+#include <drm/drm_writeback.h>
> > #include <linux/hrtimer.h>
> >
> > #define XRES_MIN    20
> >@@ -61,7 +62,7 @@ struct vkms_crtc_state {
> > struct vkms_output {
> > 	struct drm_crtc crtc;
> > 	struct drm_encoder encoder;
> >-	struct drm_connector connector;
> >+	struct drm_writeback_connector connector;
> > 	struct hrtimer vblank_hrtimer;
> > 	ktime_t period_ns;
> > 	struct drm_pending_vblank_event *event;
> >diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> >index 271a0eb9042c..46622d012107 100644
> >--- a/drivers/gpu/drm/vkms/vkms_output.c
> >+++ b/drivers/gpu/drm/vkms/vkms_output.c
> >@@ -9,6 +9,13 @@
> > #include "vkms_drv.h"
> > #include <drm/drm_crtc_helper.h>
> > #include <drm/drm_atomic_helper.h>
> >+#include <drm/drm_writeback.h>
> >+
> >+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state)
> >+
> >+struct vkms_writeback_connector_state {
> >+	struct drm_connector_state base;
> >+};
> >
> > static void vkms_connector_destroy(struct drm_connector *connector)
> > {
> >@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector)
> > 	drm_connector_cleanup(connector);
> > }
> >
> >+static void vkms_connector_reset(struct drm_connector *connector)
> >+{
> >+	struct vkms_writeback_connector_state *wb_state =
> >+		kzalloc(sizeof(*wb_state), GFP_KERNEL);
> >+
> >+	if (connector->state)
> >+		__drm_atomic_helper_connector_destroy_state(connector->state);
> >+
> >+	kfree(connector->state);
> >+	__drm_atomic_helper_connector_reset(connector, &wb_state->base);
> >+}
> >+
> >+static struct drm_connector_state *
> >+vkms_connector_duplicate_state(struct drm_connector *connector)
> >+{
> >+	struct vkms_writeback_connector_state *wb_state;
> >+
> >+	if (WARN_ON(!connector->state))
> >+		return NULL;
> >+
> >+	wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
> >+	if (!wb_state)
> >+		return NULL;
> >+
> >+	__drm_atomic_helper_connector_duplicate_state(connector,
> >+						      &wb_state->base);
> >+
> >+	return &wb_state->base;
> >+}
> >+
> >+static enum drm_connector_status
> >+vkms_connector_detect(struct drm_connector *connector, bool force)
> >+{
> >+	return connector_status_connected;
> >+}
> >+
> > static const struct drm_connector_funcs vkms_connector_funcs = {
> >+	.reset = vkms_connector_reset,
> >+	.detect = vkms_connector_detect,
> > 	.fill_modes = drm_helper_probe_single_connector_modes,
> > 	.destroy = vkms_connector_destroy,
> >-	.reset = drm_atomic_helper_connector_reset,
> >-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> >+	.atomic_duplicate_state = vkms_connector_duplicate_state,
> > 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > };
> 
> You're missing the call to drm_writeback_queue_job(), which is
> required. Your connector needs to have an atomic_commit callback in
> its drm_connector_helper_funcs, and in there you need to "queue" the
> job, something like:
> 
>   struct drm_writeback_connector *wb_conn = &out->connector;
>   drm_writeback_queue_job(wb_conn, wb_conn->base.state->writeback_job);
>   wb_conn->base.state->writeback_job = NULL;
> 
> Because you need to set the job pointer to NULL there, before that
> you'll need to stash the destination somewhere else, to be used as the
> destination for the memcpy() in _vkms_get_crc(). The simplest thing to
> do is probably to just add a new void *writeback_vaddr to
> vkms_writeback_connector_state, and set it to the output framebuffer
> vaddr before you call _queue_job(). The core will make sure the
> framebuffer is properly refcounted until you call
> drm_writeback_signal_completion(), so as long as you only call that
> after you've finished copying the data, you're safe.
> 
> In a "real" HW device, we write that pointer directly to a HW
> register, so we don't need to track it in our connector state. But you
> don't have a HW register to put it in, so you'll need to track it
> yourself :-)
> 
> >
> >@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> > 	.get_modes    = vkms_conn_get_modes,
> > };
> >
> >+static int vkms_encoder_atomic_check(struct drm_encoder *encoder,
> >+				     struct drm_crtc_state *crtc_state,
> >+				     struct drm_connector_state *conn_state)
> >+{
> >+	struct drm_framebuffer *fb;
> >+
> >+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> >+		return 0;
> >+
> >+	fb = conn_state->writeback_job->fb;
> >+	if (fb->width != crtc_state->mode.hdisplay ||
> >+	    fb->height != crtc_state->mode.vdisplay) {
> >+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> >+			      fb->width, fb->height);
> >+		return -EINVAL;
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> >+	.atomic_check = vkms_encoder_atomic_check,
> >+};
> >+
> > int vkms_output_init(struct vkms_device *vkmsdev)
> > {
> > 	struct vkms_output *output = &vkmsdev->output;
> > 	struct drm_device *dev = &vkmsdev->drm;
> >-	struct drm_connector *connector = &output->connector;
> >-	struct drm_encoder *encoder = &output->encoder;
> >+	struct drm_writeback_connector *connector = &output->connector;
> > 	struct drm_crtc *crtc = &output->crtc;
> > 	struct drm_plane *primary, *cursor = NULL;
> > 	int ret;
> >@@ -68,47 +135,29 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > 	if (ret)
> > 		goto err_crtc;
> >
> >-	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> >-				 DRM_MODE_CONNECTOR_VIRTUAL);
> >+	ret = drm_writeback_connector_init(dev, connector,
> >+					   &vkms_connector_funcs,
> >+					   &vkms_encoder_helper_funcs,
> >+					   vkms_formats, 1);
> > 	if (ret) {
> > 		DRM_ERROR("Failed to init connector\n");
> > 		goto err_connector;
> > 	}
> >
> >-	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> >+	drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs);
> >
> >-	ret = drm_connector_register(connector);
> >+	ret = drm_connector_register(&connector->base);
> > 	if (ret) {
> > 		DRM_ERROR("Failed to register connector\n");
> > 		goto err_connector_register;
> > 	}
> >
> >-	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> >-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> >-	if (ret) {
> >-		DRM_ERROR("Failed to init encoder\n");
> >-		goto err_encoder;
> >-	}
> >-	encoder->possible_crtcs = 1;
> >-
> >-	ret = drm_connector_attach_encoder(connector, encoder);
> >-	if (ret) {
> >-		DRM_ERROR("Failed to attach connector to encoder\n");
> >-		goto err_attach;
> >-	}
> >-
> > 	drm_mode_config_reset(dev);
> >
> > 	return 0;
> >
> >-err_attach:
> >-	drm_encoder_cleanup(encoder);
> >-
> >-err_encoder:
> >-	drm_connector_unregister(connector);
> >-
> > err_connector_register:
> >-	drm_connector_cleanup(connector);
> >+	drm_connector_cleanup(&connector->base);
> >
> > err_connector:
> > 	drm_crtc_cleanup(crtc);
> >-- 
> >2.19.1
> >
> >
> >-- 
> >Rodrigo Siqueira
> >https://siqueira.tech
> >https://twitter.com/siqueirajordao
> >Graduate Student
> >Department of Computer Science
> >University of São Paulo

-- 
Rodrigo Siqueira
https://siqueira.tech
https://twitter.com/siqueirajordao
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel