[PATCH 0/2] drm/vkms: Introduces writeback support

[PATCH 0/2] drm/vkms: Introduces writeback support

[Rodrigo Siqueira]

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

This patchset introduces the writeback connector to vkms. The first
patch is required for enabling the virtual encoder to be compatible with
the crtc when we have multiple encoders. The second patch adds the
required implementation to enable writeback in the vkms. With this
patchset, vkms can successfully pass all the kms_writeback tests from
IGT.

Rodrigo Siqueira (2):
  drm/vkms: Use index instead of 0 in possible crtc
  drm/vkms: Add support for writeback

 drivers/gpu/drm/vkms/Makefile         |   9 +-
 drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
 drivers/gpu/drm/vkms/vkms_drv.c       |  12 +-
 drivers/gpu/drm/vkms/vkms_drv.h       |  16 ++-
 drivers/gpu/drm/vkms/vkms_output.c    |  12 +-
 drivers/gpu/drm/vkms/vkms_plane.c     |   4 +-
 drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
 7 files changed, 214 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c

-- 
2.21.0

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

[PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

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

When vkms calls drm_universal_plane_init(), it sets 0 for the
possible_crtcs parameter which works well for a single encoder and
connector; however, this approach is not flexible and does not fit well
for vkms. This commit adds an index parameter for vkms_plane_init()
which makes code flexible and enables vkms to support other DRM features.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
 drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
 drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
 drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 738dd6206d85..92296bd8f623 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	dev->mode_config.max_height = YRES_MAX;
 	dev->mode_config.preferred_depth = 24;
 
-	return vkms_output_init(vkmsdev);
+	return vkms_output_init(vkmsdev, 0);
 }
 
 static int __init vkms_init(void)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 81f1cfbeb936..e81073dea154 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 			       int *max_error, ktime_t *vblank_time,
 			       bool in_vblank_irq);
 
-int vkms_output_init(struct vkms_device *vkmsdev);
+int vkms_output_init(struct vkms_device *vkmsdev, int index);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type);
+				  enum drm_plane_type type, int index);
 
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 3b162b25312e..1442b447c707 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
-int vkms_output_init(struct vkms_device *vkmsdev)
+int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
@@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	struct drm_plane *primary, *cursor = NULL;
 	int ret;
 
-	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
+	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
 	if (enable_cursor) {
-		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
+		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
 		if (IS_ERR(cursor)) {
 			ret = PTR_ERR(cursor);
 			goto err_cursor;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0e67d2d42f0c..20ffc52f9194 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 };
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type)
+				  enum drm_plane_type type, int index)
 {
 	struct drm_device *dev = &vkmsdev->drm;
 	const struct drm_plane_helper_funcs *funcs;
@@ -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 << index,
 				       &vkms_plane_funcs,
 				       formats, nformats,
 				       NULL, type, NULL);
-- 
2.21.0

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

[PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

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

When vkms calls drm_universal_plane_init(), it sets 0 for the
possible_crtcs parameter which works well for a single encoder and
connector; however, this approach is not flexible and does not fit well
for vkms. This commit adds an index parameter for vkms_plane_init()
which makes code flexible and enables vkms to support other DRM features.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
 drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
 drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
 drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 738dd6206d85..92296bd8f623 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	dev->mode_config.max_height = YRES_MAX;
 	dev->mode_config.preferred_depth = 24;
 
-	return vkms_output_init(vkmsdev);
+	return vkms_output_init(vkmsdev, 0);
 }
 
 static int __init vkms_init(void)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 81f1cfbeb936..e81073dea154 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 			       int *max_error, ktime_t *vblank_time,
 			       bool in_vblank_irq);
 
-int vkms_output_init(struct vkms_device *vkmsdev);
+int vkms_output_init(struct vkms_device *vkmsdev, int index);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type);
+				  enum drm_plane_type type, int index);
 
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 3b162b25312e..1442b447c707 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
-int vkms_output_init(struct vkms_device *vkmsdev)
+int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
@@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	struct drm_plane *primary, *cursor = NULL;
 	int ret;
 
-	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
+	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
 	if (enable_cursor) {
-		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
+		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
 		if (IS_ERR(cursor)) {
 			ret = PTR_ERR(cursor);
 			goto err_cursor;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0e67d2d42f0c..20ffc52f9194 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 };
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type)
+				  enum drm_plane_type type, int index)
 {
 	struct drm_device *dev = &vkmsdev->drm;
 	const struct drm_plane_helper_funcs *funcs;
@@ -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 << index,
 				       &vkms_plane_funcs,
 				       formats, nformats,
 				       NULL, type, NULL);
-- 
2.21.0

[-- 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

[PATCH 2/2] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

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

This patch implements the necessary functions to add writeback support
for vkms. This feature is useful for testing compositors if you don’t
have hardware with writeback support.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/Makefile         |   9 +-
 drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
 drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
 drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
 drivers/gpu/drm/vkms/vkms_output.c    |   6 +
 drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
 6 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 89f09bec7b23..90eb7acd618d 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
+vkms-y := \
+	vkms_drv.o \
+	vkms_plane.o \
+	vkms_output.o \
+	vkms_crtc.o \
+	vkms_gem.o \
+	vkms_crc.o \
+	vkms_writeback.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 1bbe099b7db8..ce797e265b1b 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (output->writeback_status == WB_START) {
+		drm_writeback_signal_completion(&output->wb_connector, 0);
+		output->writeback_status = WB_STOP;
+	}
+
 	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
 		goto out_fini;
 	}
 
+	vkms_device->output.writeback_status = WB_DISABLED;
+	if (enable_writeback) {
+		vkms_device->output.writeback_status = WB_STOP;
+		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 e81073dea154..ca1f9ee63ec8 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -7,6 +7,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
@@ -60,14 +61,22 @@ struct vkms_crtc_state {
 	u64 frame_end;
 };
 
+enum wb_status {
+	WB_DISABLED,
+	WB_START,
+	WB_STOP,
+};
+
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct drm_writeback_connector wb_connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
 	bool crc_enabled;
+	enum wb_status writeback_status;
 	/* ordered wq for crc_work */
 	struct workqueue_struct *crc_workq;
 	/* protects concurrent access to crc_data */
@@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 			   size_t *values_cnt);
 void vkms_crc_work_handle(struct work_struct *work);
 
+/* Writeback */
+int enable_writeback_connector(struct vkms_device *vkmsdev);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 1442b447c707..1fc1d4e9585c 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		goto err_attach;
 	}
 
+	if (vkmsdev->output.writeback_status != WB_DISABLED) {
+		ret = enable_writeback_connector(vkmsdev);
+		if (ret)
+			DRM_ERROR("Failed to init writeback connector\n");
+	}
+
 	drm_mode_config_reset(dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
new file mode 100644
index 000000000000..f7b962ae5646
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+#include <drm/drm_writeback.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static const struct drm_connector_funcs vkms_wb_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct drm_framebuffer *fb;
+	const struct drm_display_mode *mode = &crtc_state->mode;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	fb = conn_state->writeback_job->fb;
+	if (fb->width != mode->hdisplay || fb->height != 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;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
+	.atomic_check = vkms_wb_encoder_atomic_check,
+};
+
+static int vkms_wb_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				    dev->mode_config.max_height);
+}
+
+static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
+			       struct drm_writeback_job *job)
+{
+	struct vkms_gem_object *vkms_obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	if (!job->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+	ret = vkms_gem_vmap(gem_obj);
+	if (ret) {
+		DRM_ERROR("vmap failed: %d\n", ret);
+		return ret;
+	}
+
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	job->priv = vkms_obj->vaddr;
+
+	return 0;
+}
+
+static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
+				struct drm_writeback_job *job)
+{
+	struct drm_gem_object *gem_obj;
+
+	if (!job->fb)
+		return;
+
+	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+	vkms_gem_vunmap(gem_obj);
+}
+
+static void vkms_wb_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 *wb_conn = &output->wb_connector;
+	struct drm_connector_state *conn_state = wb_conn->base.state;
+	void *priv_data = conn_state->writeback_job->priv;
+	struct vkms_crc_data *primary_data = NULL;
+	struct drm_framebuffer *fb = NULL;
+	struct vkms_gem_object *vkms_obj;
+	struct drm_gem_object *gem_obj;
+	struct drm_plane *plane;
+
+	if (!conn_state)
+		return;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
+		output->writeback_status = WB_STOP;
+		DRM_DEBUG_DRIVER("Disable writeback\n");
+		return;
+	}
+
+	drm_for_each_plane(plane, &vkmsdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *plane_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		plane_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+			primary_data = plane_data;
+	}
+
+	if (!primary_data)
+		return;
+
+	fb = &primary_data->fb;
+	gem_obj = drm_gem_fb_get_obj(fb, 0);
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+
+	if (!vkms_obj->vaddr || !priv_data)
+		return;
+
+	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
+	drm_writeback_queue_job(wb_conn, state);
+	output->writeback_status = WB_START;
+}
+
+static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
+	.get_modes = vkms_wb_connector_get_modes,
+	.prepare_writeback_job = vkms_wb_prepare_job,
+	.cleanup_writeback_job = vkms_wb_cleanup_job,
+	.atomic_commit = vkms_wb_atomic_commit,
+};
+
+int enable_writeback_connector(struct vkms_device *vkmsdev)
+{
+	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+
+	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
+	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
+
+	return drm_writeback_connector_init(&vkmsdev->drm, wb,
+					    &vkms_wb_connector_funcs,
+					    &vkms_wb_encoder_helper_funcs,
+					    vkms_formats,
+					    ARRAY_SIZE(vkms_formats));
+}
+
-- 
2.21.0

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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Daniel Vetter]

On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> When vkms calls drm_universal_plane_init(), it sets 0 for the
> possible_crtcs parameter which works well for a single encoder and
> connector; however, this approach is not flexible and does not fit well
> for vkms. This commit adds an index parameter for vkms_plane_init()
> which makes code flexible and enables vkms to support other DRM features.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

I think a core patch to WARN_ON if this is NULL would be good. Since
that's indeed a bit broken ... We'd need to check all callers to make sure
there's not other such bugs anywhere ofc.
-Daniel

> ---
>  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
>  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
>  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
>  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 738dd6206d85..92296bd8f623 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.max_height = YRES_MAX;
>  	dev->mode_config.preferred_depth = 24;
>  
> -	return vkms_output_init(vkmsdev);
> +	return vkms_output_init(vkmsdev, 0);
>  }
>  
>  static int __init vkms_init(void)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 81f1cfbeb936..e81073dea154 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>  			       int *max_error, ktime_t *vblank_time,
>  			       bool in_vblank_irq);
>  
> -int vkms_output_init(struct vkms_device *vkmsdev);
> +int vkms_output_init(struct vkms_device *vkmsdev, int index);
>  
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				  enum drm_plane_type type);
> +				  enum drm_plane_type type, int index);
>  
>  /* Gem stuff */
>  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 3b162b25312e..1442b447c707 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  	.get_modes    = vkms_conn_get_modes,
>  };
>  
> -int vkms_output_init(struct vkms_device *vkmsdev)
> +int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  {
>  	struct vkms_output *output = &vkmsdev->output;
>  	struct drm_device *dev = &vkmsdev->drm;
> @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  	struct drm_plane *primary, *cursor = NULL;
>  	int ret;
>  
> -	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> +	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
>  	if (IS_ERR(primary))
>  		return PTR_ERR(primary);
>  
>  	if (enable_cursor) {
> -		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> +		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
>  		if (IS_ERR(cursor)) {
>  			ret = PTR_ERR(cursor);
>  			goto err_cursor;
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 0e67d2d42f0c..20ffc52f9194 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
>  };
>  
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				  enum drm_plane_type type)
> +				  enum drm_plane_type type, int index)
>  {
>  	struct drm_device *dev = &vkmsdev->drm;
>  	const struct drm_plane_helper_funcs *funcs;
> @@ -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 << index,
>  				       &vkms_plane_funcs,
>  				       formats, nformats,
>  				       NULL, type, NULL);
> -- 
> 2.21.0



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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Daniel Vetter]

On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> This patch implements the necessary functions to add writeback support
> for vkms. This feature is useful for testing compositors if you don’t
> have hardware with writeback support.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/Makefile         |   9 +-
>  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
>  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
>  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
>  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
>  6 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..90eb7acd618d 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> +vkms-y := \
> +	vkms_drv.o \
> +	vkms_plane.o \
> +	vkms_output.o \
> +	vkms_crtc.o \
> +	vkms_gem.o \
> +	vkms_crc.o \
> +	vkms_writeback.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 1bbe099b7db8..ce797e265b1b 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
>  
> +	if (output->writeback_status == WB_START) {
> +		drm_writeback_signal_completion(&output->wb_connector, 0);
> +		output->writeback_status = WB_STOP;
> +	}
> +
>  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
>  		goto out_fini;
>  	}
>  
> +	vkms_device->output.writeback_status = WB_DISABLED;
> +	if (enable_writeback) {
> +		vkms_device->output.writeback_status = WB_STOP;
> +		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 e81073dea154..ca1f9ee63ec8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -7,6 +7,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
> @@ -60,14 +61,22 @@ struct vkms_crtc_state {
>  	u64 frame_end;
>  };
>  
> +enum wb_status {
> +	WB_DISABLED,
> +	WB_START,
> +	WB_STOP,
> +};
> +
>  struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
> +	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
>  	bool crc_enabled;
> +	enum wb_status writeback_status;
>  	/* ordered wq for crc_work */
>  	struct workqueue_struct *crc_workq;
>  	/* protects concurrent access to crc_data */
> @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
>  void vkms_crc_work_handle(struct work_struct *work);
>  
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 1442b447c707..1fc1d4e9585c 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  		goto err_attach;
>  	}
>  
> +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> +		ret = enable_writeback_connector(vkmsdev);
> +		if (ret)
> +			DRM_ERROR("Failed to init writeback connector\n");
> +	}
> +
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> new file mode 100644
> index 000000000000..f7b962ae5646
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != mode->hdisplay || fb->height != 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;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> +	.atomic_check = vkms_wb_encoder_atomic_check,
> +};
> +
> +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +
> +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> +				    dev->mode_config.max_height);
> +}
> +
> +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> +			       struct drm_writeback_job *job)
> +{
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	if (!job->fb)
> +		return 0;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	ret = vkms_gem_vmap(gem_obj);
> +	if (ret) {
> +		DRM_ERROR("vmap failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	job->priv = vkms_obj->vaddr;
> +
> +	return 0;
> +}
> +
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> +				struct drm_writeback_job *job)
> +{
> +	struct drm_gem_object *gem_obj;
> +
> +	if (!job->fb)
> +		return;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	vkms_gem_vunmap(gem_obj);
> +}
> +
> +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> +	struct drm_connector_state *conn_state = wb_conn->base.state;
> +	void *priv_data = conn_state->writeback_job->priv;
> +	struct vkms_crc_data *primary_data = NULL;
> +	struct drm_framebuffer *fb = NULL;
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	struct drm_plane *plane;
> +
> +	if (!conn_state)
> +		return;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> +		output->writeback_status = WB_STOP;
> +		DRM_DEBUG_DRIVER("Disable writeback\n");
> +		return;
> +	}
> +
> +	drm_for_each_plane(plane, &vkmsdev->drm) {
> +		struct vkms_plane_state *vplane_state;
> +		struct vkms_crc_data *plane_data;
> +
> +		vplane_state = to_vkms_plane_state(plane->state);
> +		plane_data = vplane_state->crc_data;
> +
> +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> +			continue;
> +
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +			primary_data = plane_data;
> +	}
> +
> +	if (!primary_data)
> +		return;
> +
> +	fb = &primary_data->fb;
> +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +
> +	if (!vkms_obj->vaddr || !priv_data)
> +		return;
> +
> +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> +	drm_writeback_queue_job(wb_conn, state);
> +	output->writeback_status = WB_START;

Hm, if this passes the current writeback tests then I guess those tests
are a bit too simple. Or maybe the test can't use the cursor plane, and
that's why it doesn't notice that we only write back the primary plane.

Writeback is supposed to write back the same image you'd see on the
screen, i.e. with cursor, other planes, any color correction applied and
anything else really. That means vkms writeback needs to be integrated
into the crc computation. We need to run that work either when there's a
writeback job or when we need a crc. Crc would be only computed when
needed, and for writeback we need to put the entire composited/blended
buffer into the writeback buffer.

That's why I said yesterday that your work will conflict with my work to
reorg crc work handling, aside from the final step it needs to do all the
same things.

I guess would be good to improve igt and add a cursor testcase for write
(similar to the crc cursor tests we have), or maybe create support in igt
to use writeback instead of crc, so that we could better check this.
-Daniel

> +}
> +
> +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> +	.get_modes = vkms_wb_connector_get_modes,
> +	.prepare_writeback_job = vkms_wb_prepare_job,
> +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> +	.atomic_commit = vkms_wb_atomic_commit,
> +};
> +
> +int enable_writeback_connector(struct vkms_device *vkmsdev)
> +{
> +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +
> +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +
> +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> +					    &vkms_wb_connector_funcs,
> +					    &vkms_wb_encoder_helper_funcs,
> +					    vkms_formats,
> +					    ARRAY_SIZE(vkms_formats));
> +}
> +
> -- 
> 2.21.0



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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Daniel Vetter]

On Fri, Jun 07, 2019 at 09:48:08AM +0200, Daniel Vetter wrote:
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +	vkms_drv.o \
> > +	vkms_plane.o \
> > +	vkms_output.o \
> > +	vkms_crtc.o \
> > +	vkms_gem.o \
> > +	vkms_crc.o \
> > +	vkms_writeback.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> >  
> > +	if (output->writeback_status == WB_START) {
> > +		drm_writeback_signal_completion(&output->wb_connector, 0);
> > +		output->writeback_status = WB_STOP;
> > +	}
> > +
> >  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >  		goto out_fini;
> >  	}
> >  
> > +	vkms_device->output.writeback_status = WB_DISABLED;
> > +	if (enable_writeback) {
> > +		vkms_device->output.writeback_status = WB_STOP;
> > +		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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >  	u64 frame_end;
> >  };
> >  
> > +enum wb_status {
> > +	WB_DISABLED,
> > +	WB_START,
> > +	WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector wb_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> >  	bool crc_enabled;
> > +	enum wb_status writeback_status;
> >  	/* ordered wq for crc_work */
> >  	struct workqueue_struct *crc_workq;
> >  	/* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			   size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >  
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  		goto err_attach;
> >  	}
> >  
> > +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +		ret = enable_writeback_connector(vkmsdev);
> > +		if (ret)
> > +			DRM_ERROR("Failed to init writeback connector\n");
> > +	}
> > +
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != mode->hdisplay || fb->height != 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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +
> > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +				    dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +			       struct drm_writeback_job *job)
> > +{
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	if (!job->fb)
> > +		return 0;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	ret = vkms_gem_vmap(gem_obj);
> > +	if (ret) {
> > +		DRM_ERROR("vmap failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +	job->priv = vkms_obj->vaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +				struct drm_writeback_job *job)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +
> > +	if (!job->fb)
> > +		return;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	void *priv_data = conn_state->writeback_job->priv;
> > +	struct vkms_crc_data *primary_data = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	struct drm_plane *plane;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +		output->writeback_status = WB_STOP;
> > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > +		return;
> > +	}
> > +
> > +	drm_for_each_plane(plane, &vkmsdev->drm) {
> > +		struct vkms_plane_state *vplane_state;
> > +		struct vkms_crc_data *plane_data;
> > +
> > +		vplane_state = to_vkms_plane_state(plane->state);
> > +		plane_data = vplane_state->crc_data;
> > +
> > +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +			continue;
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +			primary_data = plane_data;
> > +	}
> > +
> > +	if (!primary_data)
> > +		return;
> > +
> > +	fb = &primary_data->fb;
> > +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +	if (!vkms_obj->vaddr || !priv_data)
> > +		return;
> > +
> > +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	drm_writeback_queue_job(wb_conn, state);
> > +	output->writeback_status = WB_START;
> 
> Hm, if this passes the current writeback tests then I guess those tests
> are a bit too simple. Or maybe the test can't use the cursor plane, and
> that's why it doesn't notice that we only write back the primary plane.
> 
> Writeback is supposed to write back the same image you'd see on the
> screen, i.e. with cursor, other planes, any color correction applied and
> anything else really. That means vkms writeback needs to be integrated
> into the crc computation. We need to run that work either when there's a
> writeback job or when we need a crc. Crc would be only computed when
> needed, and for writeback we need to put the entire composited/blended
> buffer into the writeback buffer.
> 
> That's why I said yesterday that your work will conflict with my work to
> reorg crc work handling, aside from the final step it needs to do all the
> same things.
> 
> I guess would be good to improve igt and add a cursor testcase for write
> (similar to the crc cursor tests we have), or maybe create support in igt
> to use writeback instead of crc, so that we could better check this.

For the integration with my rework: I've added new active_planes pointer
to vkms_crtc_state, so that the crc worker can get at the plane state it
needs. I think we can also add an active_writeback pointer like this

	struct drm_writeback_state *active_writeback;

using the same design as with the planes. Also, might be good to rename
vkms_output.crc_enabled to vkms_output.composer_enabled, since that work
will do more than just compute a crc in the future. We might even want to
rename vkms_crc.c to vkms_composer.c and rename the workers and all that.
If you want I guess you could land that as prep (but probably based on my
series to avoid too many conflicts).
-Daniel

> -Daniel
> 
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +	.get_modes = vkms_wb_connector_get_modes,
> > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > +	.atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +					    &vkms_wb_connector_funcs,
> > +					    &vkms_wb_encoder_helper_funcs,
> > +					    vkms_formats,
> > +					    ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > -- 
> > 2.21.0
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Daniel Vetter]

On Fri, Jun 07, 2019 at 09:48:08AM +0200, Daniel Vetter wrote:
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +	vkms_drv.o \
> > +	vkms_plane.o \
> > +	vkms_output.o \
> > +	vkms_crtc.o \
> > +	vkms_gem.o \
> > +	vkms_crc.o \
> > +	vkms_writeback.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> >  
> > +	if (output->writeback_status == WB_START) {
> > +		drm_writeback_signal_completion(&output->wb_connector, 0);
> > +		output->writeback_status = WB_STOP;
> > +	}
> > +
> >  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >  		goto out_fini;
> >  	}
> >  
> > +	vkms_device->output.writeback_status = WB_DISABLED;
> > +	if (enable_writeback) {
> > +		vkms_device->output.writeback_status = WB_STOP;
> > +		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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >  	u64 frame_end;
> >  };
> >  
> > +enum wb_status {
> > +	WB_DISABLED,
> > +	WB_START,
> > +	WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector wb_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> >  	bool crc_enabled;
> > +	enum wb_status writeback_status;
> >  	/* ordered wq for crc_work */
> >  	struct workqueue_struct *crc_workq;
> >  	/* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			   size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >  
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  		goto err_attach;
> >  	}
> >  
> > +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +		ret = enable_writeback_connector(vkmsdev);
> > +		if (ret)
> > +			DRM_ERROR("Failed to init writeback connector\n");
> > +	}
> > +
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != mode->hdisplay || fb->height != 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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +
> > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +				    dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +			       struct drm_writeback_job *job)
> > +{
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	if (!job->fb)
> > +		return 0;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	ret = vkms_gem_vmap(gem_obj);
> > +	if (ret) {
> > +		DRM_ERROR("vmap failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +	job->priv = vkms_obj->vaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +				struct drm_writeback_job *job)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +
> > +	if (!job->fb)
> > +		return;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	void *priv_data = conn_state->writeback_job->priv;
> > +	struct vkms_crc_data *primary_data = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	struct drm_plane *plane;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +		output->writeback_status = WB_STOP;
> > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > +		return;
> > +	}
> > +
> > +	drm_for_each_plane(plane, &vkmsdev->drm) {
> > +		struct vkms_plane_state *vplane_state;
> > +		struct vkms_crc_data *plane_data;
> > +
> > +		vplane_state = to_vkms_plane_state(plane->state);
> > +		plane_data = vplane_state->crc_data;
> > +
> > +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +			continue;
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +			primary_data = plane_data;
> > +	}
> > +
> > +	if (!primary_data)
> > +		return;
> > +
> > +	fb = &primary_data->fb;
> > +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +	if (!vkms_obj->vaddr || !priv_data)
> > +		return;
> > +
> > +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	drm_writeback_queue_job(wb_conn, state);
> > +	output->writeback_status = WB_START;
> 
> Hm, if this passes the current writeback tests then I guess those tests
> are a bit too simple. Or maybe the test can't use the cursor plane, and
> that's why it doesn't notice that we only write back the primary plane.
> 
> Writeback is supposed to write back the same image you'd see on the
> screen, i.e. with cursor, other planes, any color correction applied and
> anything else really. That means vkms writeback needs to be integrated
> into the crc computation. We need to run that work either when there's a
> writeback job or when we need a crc. Crc would be only computed when
> needed, and for writeback we need to put the entire composited/blended
> buffer into the writeback buffer.
> 
> That's why I said yesterday that your work will conflict with my work to
> reorg crc work handling, aside from the final step it needs to do all the
> same things.
> 
> I guess would be good to improve igt and add a cursor testcase for write
> (similar to the crc cursor tests we have), or maybe create support in igt
> to use writeback instead of crc, so that we could better check this.

For the integration with my rework: I've added new active_planes pointer
to vkms_crtc_state, so that the crc worker can get at the plane state it
needs. I think we can also add an active_writeback pointer like this

	struct drm_writeback_state *active_writeback;

using the same design as with the planes. Also, might be good to rename
vkms_output.crc_enabled to vkms_output.composer_enabled, since that work
will do more than just compute a crc in the future. We might even want to
rename vkms_crc.c to vkms_composer.c and rename the workers and all that.
If you want I guess you could land that as prep (but probably based on my
series to avoid too many conflicts).
-Daniel

> -Daniel
> 
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +	.get_modes = vkms_wb_connector_get_modes,
> > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > +	.atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +					    &vkms_wb_connector_funcs,
> > +					    &vkms_wb_encoder_helper_funcs,
> > +					    vkms_formats,
> > +					    ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > -- 
> > 2.21.0
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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: [PATCH 2/2] drm/vkms: Add support for writeback

[Brian Starkey]

Hi Rodrigo,

On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> This patch implements the necessary functions to add writeback support
> for vkms. This feature is useful for testing compositors if you don’t
> have hardware with writeback support.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/Makefile         |   9 +-
>  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
>  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
>  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
>  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
>  6 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..90eb7acd618d 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> +vkms-y := \
> +	vkms_drv.o \
> +	vkms_plane.o \
> +	vkms_output.o \
> +	vkms_crtc.o \
> +	vkms_gem.o \
> +	vkms_crc.o \
> +	vkms_writeback.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 1bbe099b7db8..ce797e265b1b 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
>  
> +	if (output->writeback_status == WB_START) {
> +		drm_writeback_signal_completion(&output->wb_connector, 0);
> +		output->writeback_status = WB_STOP;
> +	}
> +
>  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
>  		goto out_fini;
>  	}
>  
> +	vkms_device->output.writeback_status = WB_DISABLED;
> +	if (enable_writeback) {
> +		vkms_device->output.writeback_status = WB_STOP;
> +		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 e81073dea154..ca1f9ee63ec8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -7,6 +7,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
> @@ -60,14 +61,22 @@ struct vkms_crtc_state {
>  	u64 frame_end;
>  };
>  
> +enum wb_status {
> +	WB_DISABLED,
> +	WB_START,
> +	WB_STOP,
> +};
> +
>  struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
> +	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
>  	bool crc_enabled;
> +	enum wb_status writeback_status;
>  	/* ordered wq for crc_work */
>  	struct workqueue_struct *crc_workq;
>  	/* protects concurrent access to crc_data */
> @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
>  void vkms_crc_work_handle(struct work_struct *work);
>  
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 1442b447c707..1fc1d4e9585c 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  		goto err_attach;
>  	}
>  
> +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> +		ret = enable_writeback_connector(vkmsdev);
> +		if (ret)
> +			DRM_ERROR("Failed to init writeback connector\n");
> +	}
> +
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> new file mode 100644
> index 000000000000..f7b962ae5646
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != mode->hdisplay || fb->height != 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;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> +	.atomic_check = vkms_wb_encoder_atomic_check,
> +};
> +
> +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +
> +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> +				    dev->mode_config.max_height);
> +}
> +
> +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> +			       struct drm_writeback_job *job)
> +{
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	if (!job->fb)
> +		return 0;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	ret = vkms_gem_vmap(gem_obj);
> +	if (ret) {
> +		DRM_ERROR("vmap failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	job->priv = vkms_obj->vaddr;
> +
> +	return 0;
> +}
> +
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> +				struct drm_writeback_job *job)
> +{
> +	struct drm_gem_object *gem_obj;
> +
> +	if (!job->fb)
> +		return;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	vkms_gem_vunmap(gem_obj);
> +}
> +
> +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> +	struct drm_connector_state *conn_state = wb_conn->base.state;
> +	void *priv_data = conn_state->writeback_job->priv;
> +	struct vkms_crc_data *primary_data = NULL;
> +	struct drm_framebuffer *fb = NULL;
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	struct drm_plane *plane;
> +
> +	if (!conn_state)
> +		return;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> +		output->writeback_status = WB_STOP;
> +		DRM_DEBUG_DRIVER("Disable writeback\n");
> +		return;
> +	}
> +
> +	drm_for_each_plane(plane, &vkmsdev->drm) {
> +		struct vkms_plane_state *vplane_state;
> +		struct vkms_crc_data *plane_data;
> +
> +		vplane_state = to_vkms_plane_state(plane->state);
> +		plane_data = vplane_state->crc_data;
> +
> +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> +			continue;

It's not really specific to this patch, but this looks kinda
weird to me. You've got a copy of the fb struct in plane_data?

It seems like that would break a bunch of refcounting if nothing else.

Can't you use plane->state->fb directly, and just check if it's NULL?

> +
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +			primary_data = plane_data;
> +	}
> +
> +	if (!primary_data)
> +		return;
> +
> +	fb = &primary_data->fb;
> +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +
> +	if (!vkms_obj->vaddr || !priv_data)
> +		return;
> +
> +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> +	drm_writeback_queue_job(wb_conn, state);
> +	output->writeback_status = WB_START;

You probably don't need the deferred completion signaling. I'd expect
this to work OK (and it matches the API semantics):

  drm_writeback_queue_job(wb_conn, state);
  memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
  drm_writeback_signal_completion(wb_conn, 0);


Cheers,
-Brian

> +}
> +
> +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> +	.get_modes = vkms_wb_connector_get_modes,
> +	.prepare_writeback_job = vkms_wb_prepare_job,
> +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> +	.atomic_commit = vkms_wb_atomic_commit,
> +};
> +
> +int enable_writeback_connector(struct vkms_device *vkmsdev)
> +{
> +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +
> +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +
> +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> +					    &vkms_wb_connector_funcs,
> +					    &vkms_wb_encoder_helper_funcs,
> +					    vkms_formats,
> +					    ARRAY_SIZE(vkms_formats));
> +}
> +
> -- 
> 2.21.0

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Brian Starkey]

Hi Rodrigo,

On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> This patch implements the necessary functions to add writeback support
> for vkms. This feature is useful for testing compositors if you don’t
> have hardware with writeback support.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/Makefile         |   9 +-
>  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
>  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
>  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
>  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
>  6 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..90eb7acd618d 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> +vkms-y := \
> +	vkms_drv.o \
> +	vkms_plane.o \
> +	vkms_output.o \
> +	vkms_crtc.o \
> +	vkms_gem.o \
> +	vkms_crc.o \
> +	vkms_writeback.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 1bbe099b7db8..ce797e265b1b 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
>  
> +	if (output->writeback_status == WB_START) {
> +		drm_writeback_signal_completion(&output->wb_connector, 0);
> +		output->writeback_status = WB_STOP;
> +	}
> +
>  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
>  		goto out_fini;
>  	}
>  
> +	vkms_device->output.writeback_status = WB_DISABLED;
> +	if (enable_writeback) {
> +		vkms_device->output.writeback_status = WB_STOP;
> +		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 e81073dea154..ca1f9ee63ec8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -7,6 +7,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
> @@ -60,14 +61,22 @@ struct vkms_crtc_state {
>  	u64 frame_end;
>  };
>  
> +enum wb_status {
> +	WB_DISABLED,
> +	WB_START,
> +	WB_STOP,
> +};
> +
>  struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
> +	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
>  	bool crc_enabled;
> +	enum wb_status writeback_status;
>  	/* ordered wq for crc_work */
>  	struct workqueue_struct *crc_workq;
>  	/* protects concurrent access to crc_data */
> @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
>  void vkms_crc_work_handle(struct work_struct *work);
>  
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 1442b447c707..1fc1d4e9585c 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  		goto err_attach;
>  	}
>  
> +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> +		ret = enable_writeback_connector(vkmsdev);
> +		if (ret)
> +			DRM_ERROR("Failed to init writeback connector\n");
> +	}
> +
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> new file mode 100644
> index 000000000000..f7b962ae5646
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != mode->hdisplay || fb->height != 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;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> +	.atomic_check = vkms_wb_encoder_atomic_check,
> +};
> +
> +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +
> +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> +				    dev->mode_config.max_height);
> +}
> +
> +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> +			       struct drm_writeback_job *job)
> +{
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	if (!job->fb)
> +		return 0;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	ret = vkms_gem_vmap(gem_obj);
> +	if (ret) {
> +		DRM_ERROR("vmap failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	job->priv = vkms_obj->vaddr;
> +
> +	return 0;
> +}
> +
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> +				struct drm_writeback_job *job)
> +{
> +	struct drm_gem_object *gem_obj;
> +
> +	if (!job->fb)
> +		return;
> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	vkms_gem_vunmap(gem_obj);
> +}
> +
> +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> +	struct drm_connector_state *conn_state = wb_conn->base.state;
> +	void *priv_data = conn_state->writeback_job->priv;
> +	struct vkms_crc_data *primary_data = NULL;
> +	struct drm_framebuffer *fb = NULL;
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	struct drm_plane *plane;
> +
> +	if (!conn_state)
> +		return;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> +		output->writeback_status = WB_STOP;
> +		DRM_DEBUG_DRIVER("Disable writeback\n");
> +		return;
> +	}
> +
> +	drm_for_each_plane(plane, &vkmsdev->drm) {
> +		struct vkms_plane_state *vplane_state;
> +		struct vkms_crc_data *plane_data;
> +
> +		vplane_state = to_vkms_plane_state(plane->state);
> +		plane_data = vplane_state->crc_data;
> +
> +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> +			continue;

It's not really specific to this patch, but this looks kinda
weird to me. You've got a copy of the fb struct in plane_data?

It seems like that would break a bunch of refcounting if nothing else.

Can't you use plane->state->fb directly, and just check if it's NULL?

> +
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +			primary_data = plane_data;
> +	}
> +
> +	if (!primary_data)
> +		return;
> +
> +	fb = &primary_data->fb;
> +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +
> +	if (!vkms_obj->vaddr || !priv_data)
> +		return;
> +
> +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> +	drm_writeback_queue_job(wb_conn, state);
> +	output->writeback_status = WB_START;

You probably don't need the deferred completion signaling. I'd expect
this to work OK (and it matches the API semantics):

  drm_writeback_queue_job(wb_conn, state);
  memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
  drm_writeback_signal_completion(wb_conn, 0);


Cheers,
-Brian

> +}
> +
> +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> +	.get_modes = vkms_wb_connector_get_modes,
> +	.prepare_writeback_job = vkms_wb_prepare_job,
> +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> +	.atomic_commit = vkms_wb_atomic_commit,
> +};
> +
> +int enable_writeback_connector(struct vkms_device *vkmsdev)
> +{
> +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +
> +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +
> +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> +					    &vkms_wb_connector_funcs,
> +					    &vkms_wb_encoder_helper_funcs,
> +					    vkms_formats,
> +					    ARRAY_SIZE(vkms_formats));
> +}
> +
> -- 
> 2.21.0


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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

On Fri, Jun 7, 2019 at 4:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> > When vkms calls drm_universal_plane_init(), it sets 0 for the
> > possible_crtcs parameter which works well for a single encoder and
> > connector; however, this approach is not flexible and does not fit well
> > for vkms. This commit adds an index parameter for vkms_plane_init()
> > which makes code flexible and enables vkms to support other DRM features.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>
> I think a core patch to WARN_ON if this is NULL would be good. Since
> that's indeed a bit broken ... We'd need to check all callers to make sure
> there's not other such bugs anywhere ofc.
> -Daniel

Do you mean add WARN_ON in `drm_universal_plane_init()` if
`possible_crtcs` is equal to zero?

> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
> >  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
> >  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> >  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
> >  4 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 738dd6206d85..92296bd8f623 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >       dev->mode_config.max_height = YRES_MAX;
> >       dev->mode_config.preferred_depth = 24;
> >
> > -     return vkms_output_init(vkmsdev);
> > +     return vkms_output_init(vkmsdev, 0);
> >  }
> >
> >  static int __init vkms_init(void)
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 81f1cfbeb936..e81073dea154 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> >                              int *max_error, ktime_t *vblank_time,
> >                              bool in_vblank_irq);
> >
> > -int vkms_output_init(struct vkms_device *vkmsdev);
> > +int vkms_output_init(struct vkms_device *vkmsdev, int index);
> >
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > -                               enum drm_plane_type type);
> > +                               enum drm_plane_type type, int index);
> >
> >  /* Gem stuff */
> >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 3b162b25312e..1442b447c707 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> >       .get_modes    = vkms_conn_get_modes,
> >  };
> >
> > -int vkms_output_init(struct vkms_device *vkmsdev)
> > +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  {
> >       struct vkms_output *output = &vkmsdev->output;
> >       struct drm_device *dev = &vkmsdev->drm;
> > @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >       struct drm_plane *primary, *cursor = NULL;
> >       int ret;
> >
> > -     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> > +     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> >       if (IS_ERR(primary))
> >               return PTR_ERR(primary);
> >
> >       if (enable_cursor) {
> > -             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> > +             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> >               if (IS_ERR(cursor)) {
> >                       ret = PTR_ERR(cursor);
> >                       goto err_cursor;
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 0e67d2d42f0c..20ffc52f9194 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> >  };
> >
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > -                               enum drm_plane_type type)
> > +                               enum drm_plane_type type, int index)
> >  {
> >       struct drm_device *dev = &vkmsdev->drm;
> >       const struct drm_plane_helper_funcs *funcs;
> > @@ -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 << index,
> >                                      &vkms_plane_funcs,
> >                                      formats, nformats,
> >                                      NULL, type, NULL);
> > --
> > 2.21.0
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 

Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

On Fri, Jun 7, 2019 at 4:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> > When vkms calls drm_universal_plane_init(), it sets 0 for the
> > possible_crtcs parameter which works well for a single encoder and
> > connector; however, this approach is not flexible and does not fit well
> > for vkms. This commit adds an index parameter for vkms_plane_init()
> > which makes code flexible and enables vkms to support other DRM features.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>
> I think a core patch to WARN_ON if this is NULL would be good. Since
> that's indeed a bit broken ... We'd need to check all callers to make sure
> there's not other such bugs anywhere ofc.
> -Daniel

Do you mean add WARN_ON in `drm_universal_plane_init()` if
`possible_crtcs` is equal to zero?

> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
> >  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
> >  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> >  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
> >  4 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 738dd6206d85..92296bd8f623 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >       dev->mode_config.max_height = YRES_MAX;
> >       dev->mode_config.preferred_depth = 24;
> >
> > -     return vkms_output_init(vkmsdev);
> > +     return vkms_output_init(vkmsdev, 0);
> >  }
> >
> >  static int __init vkms_init(void)
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 81f1cfbeb936..e81073dea154 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> >                              int *max_error, ktime_t *vblank_time,
> >                              bool in_vblank_irq);
> >
> > -int vkms_output_init(struct vkms_device *vkmsdev);
> > +int vkms_output_init(struct vkms_device *vkmsdev, int index);
> >
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > -                               enum drm_plane_type type);
> > +                               enum drm_plane_type type, int index);
> >
> >  /* Gem stuff */
> >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 3b162b25312e..1442b447c707 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> >       .get_modes    = vkms_conn_get_modes,
> >  };
> >
> > -int vkms_output_init(struct vkms_device *vkmsdev)
> > +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  {
> >       struct vkms_output *output = &vkmsdev->output;
> >       struct drm_device *dev = &vkmsdev->drm;
> > @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >       struct drm_plane *primary, *cursor = NULL;
> >       int ret;
> >
> > -     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> > +     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> >       if (IS_ERR(primary))
> >               return PTR_ERR(primary);
> >
> >       if (enable_cursor) {
> > -             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> > +             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> >               if (IS_ERR(cursor)) {
> >                       ret = PTR_ERR(cursor);
> >                       goto err_cursor;
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 0e67d2d42f0c..20ffc52f9194 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> >  };
> >
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > -                               enum drm_plane_type type)
> > +                               enum drm_plane_type type, int index)
> >  {
> >       struct drm_device *dev = &vkmsdev->drm;
> >       const struct drm_plane_helper_funcs *funcs;
> > @@ -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 << index,
> >                                      &vkms_plane_funcs,
> >                                      formats, nformats,
> >                                      NULL, type, NULL);
> > --
> > 2.21.0
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 

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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Daniel Vetter]

On Fri, Jun 07, 2019 at 02:17:20PM +0000, Brian Starkey wrote:
> Hi Rodrigo,
> 
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +	vkms_drv.o \
> > +	vkms_plane.o \
> > +	vkms_output.o \
> > +	vkms_crtc.o \
> > +	vkms_gem.o \
> > +	vkms_crc.o \
> > +	vkms_writeback.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> >  
> > +	if (output->writeback_status == WB_START) {
> > +		drm_writeback_signal_completion(&output->wb_connector, 0);
> > +		output->writeback_status = WB_STOP;
> > +	}
> > +
> >  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >  		goto out_fini;
> >  	}
> >  
> > +	vkms_device->output.writeback_status = WB_DISABLED;
> > +	if (enable_writeback) {
> > +		vkms_device->output.writeback_status = WB_STOP;
> > +		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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >  	u64 frame_end;
> >  };
> >  
> > +enum wb_status {
> > +	WB_DISABLED,
> > +	WB_START,
> > +	WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector wb_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> >  	bool crc_enabled;
> > +	enum wb_status writeback_status;
> >  	/* ordered wq for crc_work */
> >  	struct workqueue_struct *crc_workq;
> >  	/* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			   size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >  
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  		goto err_attach;
> >  	}
> >  
> > +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +		ret = enable_writeback_connector(vkmsdev);
> > +		if (ret)
> > +			DRM_ERROR("Failed to init writeback connector\n");
> > +	}
> > +
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != mode->hdisplay || fb->height != 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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +
> > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +				    dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +			       struct drm_writeback_job *job)
> > +{
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	if (!job->fb)
> > +		return 0;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	ret = vkms_gem_vmap(gem_obj);
> > +	if (ret) {
> > +		DRM_ERROR("vmap failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +	job->priv = vkms_obj->vaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +				struct drm_writeback_job *job)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +
> > +	if (!job->fb)
> > +		return;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	void *priv_data = conn_state->writeback_job->priv;
> > +	struct vkms_crc_data *primary_data = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	struct drm_plane *plane;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +		output->writeback_status = WB_STOP;
> > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > +		return;
> > +	}
> > +
> > +	drm_for_each_plane(plane, &vkmsdev->drm) {
> > +		struct vkms_plane_state *vplane_state;
> > +		struct vkms_crc_data *plane_data;
> > +
> > +		vplane_state = to_vkms_plane_state(plane->state);
> > +		plane_data = vplane_state->crc_data;
> > +
> > +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +			continue;
> 
> It's not really specific to this patch, but this looks kinda
> weird to me. You've got a copy of the fb struct in plane_data?
> 
> It seems like that would break a bunch of refcounting if nothing else.
> 
> Can't you use plane->state->fb directly, and just check if it's NULL?

It's complicated, and I'm working on some patches to fix that.
Prep/infrastructure work is now on the mailing list.
-Daniel

> 
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +			primary_data = plane_data;
> > +	}
> > +
> > +	if (!primary_data)
> > +		return;
> > +
> > +	fb = &primary_data->fb;
> > +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +	if (!vkms_obj->vaddr || !priv_data)
> > +		return;
> > +
> > +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	drm_writeback_queue_job(wb_conn, state);
> > +	output->writeback_status = WB_START;
> 
> You probably don't need the deferred completion signaling. I'd expect
> this to work OK (and it matches the API semantics):
> 
>   drm_writeback_queue_job(wb_conn, state);
>   memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
>   drm_writeback_signal_completion(wb_conn, 0);
> 
> 
> Cheers,
> -Brian
> 
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +	.get_modes = vkms_wb_connector_get_modes,
> > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > +	.atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +					    &vkms_wb_connector_funcs,
> > +					    &vkms_wb_encoder_helper_funcs,
> > +					    vkms_formats,
> > +					    ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > -- 
> > 2.21.0
> 
> 

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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Daniel Vetter]

On Fri, Jun 07, 2019 at 02:17:20PM +0000, Brian Starkey wrote:
> Hi Rodrigo,
> 
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +	vkms_drv.o \
> > +	vkms_plane.o \
> > +	vkms_output.o \
> > +	vkms_crtc.o \
> > +	vkms_gem.o \
> > +	vkms_crc.o \
> > +	vkms_writeback.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> >  
> > +	if (output->writeback_status == WB_START) {
> > +		drm_writeback_signal_completion(&output->wb_connector, 0);
> > +		output->writeback_status = WB_STOP;
> > +	}
> > +
> >  	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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >  		goto out_fini;
> >  	}
> >  
> > +	vkms_device->output.writeback_status = WB_DISABLED;
> > +	if (enable_writeback) {
> > +		vkms_device->output.writeback_status = WB_STOP;
> > +		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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >  	u64 frame_end;
> >  };
> >  
> > +enum wb_status {
> > +	WB_DISABLED,
> > +	WB_START,
> > +	WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector wb_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> >  	bool crc_enabled;
> > +	enum wb_status writeback_status;
> >  	/* ordered wq for crc_work */
> >  	struct workqueue_struct *crc_workq;
> >  	/* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  			   size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >  
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  		goto err_attach;
> >  	}
> >  
> > +	if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +		ret = enable_writeback_connector(vkmsdev);
> > +		if (ret)
> > +			DRM_ERROR("Failed to init writeback connector\n");
> > +	}
> > +
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != mode->hdisplay || fb->height != 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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +
> > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +				    dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +			       struct drm_writeback_job *job)
> > +{
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	if (!job->fb)
> > +		return 0;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	ret = vkms_gem_vmap(gem_obj);
> > +	if (ret) {
> > +		DRM_ERROR("vmap failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +	job->priv = vkms_obj->vaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +				struct drm_writeback_job *job)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +
> > +	if (!job->fb)
> > +		return;
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	void *priv_data = conn_state->writeback_job->priv;
> > +	struct vkms_crc_data *primary_data = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	struct drm_plane *plane;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +		output->writeback_status = WB_STOP;
> > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > +		return;
> > +	}
> > +
> > +	drm_for_each_plane(plane, &vkmsdev->drm) {
> > +		struct vkms_plane_state *vplane_state;
> > +		struct vkms_crc_data *plane_data;
> > +
> > +		vplane_state = to_vkms_plane_state(plane->state);
> > +		plane_data = vplane_state->crc_data;
> > +
> > +		if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +			continue;
> 
> It's not really specific to this patch, but this looks kinda
> weird to me. You've got a copy of the fb struct in plane_data?
> 
> It seems like that would break a bunch of refcounting if nothing else.
> 
> Can't you use plane->state->fb directly, and just check if it's NULL?

It's complicated, and I'm working on some patches to fix that.
Prep/infrastructure work is now on the mailing list.
-Daniel

> 
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +			primary_data = plane_data;
> > +	}
> > +
> > +	if (!primary_data)
> > +		return;
> > +
> > +	fb = &primary_data->fb;
> > +	gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +	if (!vkms_obj->vaddr || !priv_data)
> > +		return;
> > +
> > +	memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +	drm_writeback_queue_job(wb_conn, state);
> > +	output->writeback_status = WB_START;
> 
> You probably don't need the deferred completion signaling. I'd expect
> this to work OK (and it matches the API semantics):
> 
>   drm_writeback_queue_job(wb_conn, state);
>   memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
>   drm_writeback_signal_completion(wb_conn, 0);
> 
> 
> Cheers,
> -Brian
> 
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +	.get_modes = vkms_wb_connector_get_modes,
> > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > +	.atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +					    &vkms_wb_connector_funcs,
> > +					    &vkms_wb_encoder_helper_funcs,
> > +					    vkms_formats,
> > +					    ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > -- 
> > 2.21.0
> 
> 

-- 
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: [PATCH 2/2] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

On Fri, Jun 7, 2019 at 4:48 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> >
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +     vkms_drv.o \
> > +     vkms_plane.o \
> > +     vkms_output.o \
> > +     vkms_crtc.o \
> > +     vkms_gem.o \
> > +     vkms_crc.o \
> > +     vkms_writeback.o
> >
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >       if (!ret)
> >               DRM_ERROR("vkms failure on handling vblank");
> >
> > +     if (output->writeback_status == WB_START) {
> > +             drm_writeback_signal_completion(&output->wb_connector, 0);
> > +             output->writeback_status = WB_STOP;
> > +     }
> > +
> >       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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >               goto out_fini;
> >       }
> >
> > +     vkms_device->output.writeback_status = WB_DISABLED;
> > +     if (enable_writeback) {
> > +             vkms_device->output.writeback_status = WB_STOP;
> > +             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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >       u64 frame_end;
> >  };
> >
> > +enum wb_status {
> > +     WB_DISABLED,
> > +     WB_START,
> > +     WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >       struct drm_crtc crtc;
> >       struct drm_encoder encoder;
> >       struct drm_connector connector;
> > +     struct drm_writeback_connector wb_connector;
> >       struct hrtimer vblank_hrtimer;
> >       ktime_t period_ns;
> >       struct drm_pending_vblank_event *event;
> >       bool crc_enabled;
> > +     enum wb_status writeback_status;
> >       /* ordered wq for crc_work */
> >       struct workqueue_struct *crc_workq;
> >       /* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >                          size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >               goto err_attach;
> >       }
> >
> > +     if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +             ret = enable_writeback_connector(vkmsdev);
> > +             if (ret)
> > +                     DRM_ERROR("Failed to init writeback connector\n");
> > +     }
> > +
> >       drm_mode_config_reset(dev);
> >
> >       return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > +     .destroy = drm_connector_cleanup,
> > +     .reset = drm_atomic_helper_connector_reset,
> > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +                                     struct drm_crtc_state *crtc_state,
> > +                                     struct drm_connector_state *conn_state)
> > +{
> > +     struct drm_framebuffer *fb;
> > +     const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +             return 0;
> > +
> > +     fb = conn_state->writeback_job->fb;
> > +     if (fb->width != mode->hdisplay || fb->height != 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;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +     .atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +
> > +     return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +                                 dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +                            struct drm_writeback_job *job)
> > +{
> > +     struct vkms_gem_object *vkms_obj;
> > +     struct drm_gem_object *gem_obj;
> > +     int ret;
> > +
> > +     if (!job->fb)
> > +             return 0;
> > +
> > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +     ret = vkms_gem_vmap(gem_obj);
> > +     if (ret) {
> > +             DRM_ERROR("vmap failed: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +     job->priv = vkms_obj->vaddr;
> > +
> > +     return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +                             struct drm_writeback_job *job)
> > +{
> > +     struct drm_gem_object *gem_obj;
> > +
> > +     if (!job->fb)
> > +             return;
> > +
> > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +     vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +     struct drm_connector_state *conn_state = wb_conn->base.state;
> > +     void *priv_data = conn_state->writeback_job->priv;
> > +     struct vkms_crc_data *primary_data = NULL;
> > +     struct drm_framebuffer *fb = NULL;
> > +     struct vkms_gem_object *vkms_obj;
> > +     struct drm_gem_object *gem_obj;
> > +     struct drm_plane *plane;
> > +
> > +     if (!conn_state)
> > +             return;
> > +
> > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +             output->writeback_status = WB_STOP;
> > +             DRM_DEBUG_DRIVER("Disable writeback\n");
> > +             return;
> > +     }
> > +
> > +     drm_for_each_plane(plane, &vkmsdev->drm) {
> > +             struct vkms_plane_state *vplane_state;
> > +             struct vkms_crc_data *plane_data;
> > +
> > +             vplane_state = to_vkms_plane_state(plane->state);
> > +             plane_data = vplane_state->crc_data;
> > +
> > +             if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +                     continue;
> > +
> > +             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +                     primary_data = plane_data;
> > +     }
> > +
> > +     if (!primary_data)
> > +             return;
> > +
> > +     fb = &primary_data->fb;
> > +     gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +     if (!vkms_obj->vaddr || !priv_data)
> > +             return;
> > +
> > +     memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +     drm_writeback_queue_job(wb_conn, state);
> > +     output->writeback_status = WB_START;
>
> Hm, if this passes the current writeback tests then I guess those tests
> are a bit too simple. Or maybe the test can't use the cursor plane, and
> that's why it doesn't notice that we only write back the primary plane.

Hi,

Sorry, I knew about that, and I should have highlighted this
information in the cover letter. In my original plan, I wanted to land
a basic version of writeback in the vkms to try to upstream the
kms_writeback; then I wish that I could improve vkms and the tests in
parallel.

In summary, the kms_writeback is not upstream yet, and it needs some
improvements. IHMO, we should try to upstream the current version and
improve it step-by-step by using VKMS. If Brian and Liviu agree, I
would like to try to send a new version of their patchset. What do you
think?

Daniel, about your patchset (drm/vkms: rework crc worker), just give
me a time to go through your patchset. After that, I'll make the
required changes and update the writeback.

Thank you all for the feedback. I'll prepare a V2 with all the suggestions.

> Writeback is supposed to write back the same image you'd see on the
> screen, i.e. with cursor, other planes, any color correction applied and
> anything else really. That means vkms writeback needs to be integrated
> into the crc computation. We need to run that work either when there's a
> writeback job or when we need a crc. Crc would be only computed when
> needed, and for writeback we need to put the entire composited/blended
> buffer into the writeback buffer.
>
> That's why I said yesterday that your work will conflict with my work to
> reorg crc work handling, aside from the final step it needs to do all the
> same things.
>
> I guess would be good to improve igt and add a cursor testcase for write
> (similar to the crc cursor tests we have), or maybe create support in igt
> to use writeback instead of crc, so that we could better check this.
> -Daniel
>
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +     .get_modes = vkms_wb_connector_get_modes,
> > +     .prepare_writeback_job = vkms_wb_prepare_job,
> > +     .cleanup_writeback_job = vkms_wb_cleanup_job,
> > +     .atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +     struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +     vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +     drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +     return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +                                         &vkms_wb_connector_funcs,
> > +                                         &vkms_wb_encoder_helper_funcs,
> > +                                         vkms_formats,
> > +                                         ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > --
> > 2.21.0
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 

Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

On Fri, Jun 7, 2019 at 4:48 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don’t
> > have hardware with writeback support.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> >  6 files changed, 206 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> >
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 89f09bec7b23..90eb7acd618d 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > +vkms-y := \
> > +     vkms_drv.o \
> > +     vkms_plane.o \
> > +     vkms_output.o \
> > +     vkms_crtc.o \
> > +     vkms_gem.o \
> > +     vkms_crc.o \
> > +     vkms_writeback.o
> >
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 1bbe099b7db8..ce797e265b1b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >       if (!ret)
> >               DRM_ERROR("vkms failure on handling vblank");
> >
> > +     if (output->writeback_status == WB_START) {
> > +             drm_writeback_signal_completion(&output->wb_connector, 0);
> > +             output->writeback_status = WB_STOP;
> > +     }
> > +
> >       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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> >               goto out_fini;
> >       }
> >
> > +     vkms_device->output.writeback_status = WB_DISABLED;
> > +     if (enable_writeback) {
> > +             vkms_device->output.writeback_status = WB_STOP;
> > +             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 e81073dea154..ca1f9ee63ec8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,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
> > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> >       u64 frame_end;
> >  };
> >
> > +enum wb_status {
> > +     WB_DISABLED,
> > +     WB_START,
> > +     WB_STOP,
> > +};
> > +
> >  struct vkms_output {
> >       struct drm_crtc crtc;
> >       struct drm_encoder encoder;
> >       struct drm_connector connector;
> > +     struct drm_writeback_connector wb_connector;
> >       struct hrtimer vblank_hrtimer;
> >       ktime_t period_ns;
> >       struct drm_pending_vblank_event *event;
> >       bool crc_enabled;
> > +     enum wb_status writeback_status;
> >       /* ordered wq for crc_work */
> >       struct workqueue_struct *crc_workq;
> >       /* protects concurrent access to crc_data */
> > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >                          size_t *values_cnt);
> >  void vkms_crc_work_handle(struct work_struct *work);
> >
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 1442b447c707..1fc1d4e9585c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >               goto err_attach;
> >       }
> >
> > +     if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > +             ret = enable_writeback_connector(vkmsdev);
> > +             if (ret)
> > +                     DRM_ERROR("Failed to init writeback connector\n");
> > +     }
> > +
> >       drm_mode_config_reset(dev);
> >
> >       return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..f7b962ae5646
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > +     .destroy = drm_connector_cleanup,
> > +     .reset = drm_atomic_helper_connector_reset,
> > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +                                     struct drm_crtc_state *crtc_state,
> > +                                     struct drm_connector_state *conn_state)
> > +{
> > +     struct drm_framebuffer *fb;
> > +     const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +             return 0;
> > +
> > +     fb = conn_state->writeback_job->fb;
> > +     if (fb->width != mode->hdisplay || fb->height != 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;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +     .atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +     struct drm_device *dev = connector->dev;
> > +
> > +     return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +                                 dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +                            struct drm_writeback_job *job)
> > +{
> > +     struct vkms_gem_object *vkms_obj;
> > +     struct drm_gem_object *gem_obj;
> > +     int ret;
> > +
> > +     if (!job->fb)
> > +             return 0;
> > +
> > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +     ret = vkms_gem_vmap(gem_obj);
> > +     if (ret) {
> > +             DRM_ERROR("vmap failed: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +     job->priv = vkms_obj->vaddr;
> > +
> > +     return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +                             struct drm_writeback_job *job)
> > +{
> > +     struct drm_gem_object *gem_obj;
> > +
> > +     if (!job->fb)
> > +             return;
> > +
> > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +     vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > +     struct drm_connector_state *conn_state = wb_conn->base.state;
> > +     void *priv_data = conn_state->writeback_job->priv;
> > +     struct vkms_crc_data *primary_data = NULL;
> > +     struct drm_framebuffer *fb = NULL;
> > +     struct vkms_gem_object *vkms_obj;
> > +     struct drm_gem_object *gem_obj;
> > +     struct drm_plane *plane;
> > +
> > +     if (!conn_state)
> > +             return;
> > +
> > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +             output->writeback_status = WB_STOP;
> > +             DRM_DEBUG_DRIVER("Disable writeback\n");
> > +             return;
> > +     }
> > +
> > +     drm_for_each_plane(plane, &vkmsdev->drm) {
> > +             struct vkms_plane_state *vplane_state;
> > +             struct vkms_crc_data *plane_data;
> > +
> > +             vplane_state = to_vkms_plane_state(plane->state);
> > +             plane_data = vplane_state->crc_data;
> > +
> > +             if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > +                     continue;
> > +
> > +             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +                     primary_data = plane_data;
> > +     }
> > +
> > +     if (!primary_data)
> > +             return;
> > +
> > +     fb = &primary_data->fb;
> > +     gem_obj = drm_gem_fb_get_obj(fb, 0);
> > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > +     if (!vkms_obj->vaddr || !priv_data)
> > +             return;
> > +
> > +     memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > +     drm_writeback_queue_job(wb_conn, state);
> > +     output->writeback_status = WB_START;
>
> Hm, if this passes the current writeback tests then I guess those tests
> are a bit too simple. Or maybe the test can't use the cursor plane, and
> that's why it doesn't notice that we only write back the primary plane.

Hi,

Sorry, I knew about that, and I should have highlighted this
information in the cover letter. In my original plan, I wanted to land
a basic version of writeback in the vkms to try to upstream the
kms_writeback; then I wish that I could improve vkms and the tests in
parallel.

In summary, the kms_writeback is not upstream yet, and it needs some
improvements. IHMO, we should try to upstream the current version and
improve it step-by-step by using VKMS. If Brian and Liviu agree, I
would like to try to send a new version of their patchset. What do you
think?

Daniel, about your patchset (drm/vkms: rework crc worker), just give
me a time to go through your patchset. After that, I'll make the
required changes and update the writeback.

Thank you all for the feedback. I'll prepare a V2 with all the suggestions.

> Writeback is supposed to write back the same image you'd see on the
> screen, i.e. with cursor, other planes, any color correction applied and
> anything else really. That means vkms writeback needs to be integrated
> into the crc computation. We need to run that work either when there's a
> writeback job or when we need a crc. Crc would be only computed when
> needed, and for writeback we need to put the entire composited/blended
> buffer into the writeback buffer.
>
> That's why I said yesterday that your work will conflict with my work to
> reorg crc work handling, aside from the final step it needs to do all the
> same things.
>
> I guess would be good to improve igt and add a cursor testcase for write
> (similar to the crc cursor tests we have), or maybe create support in igt
> to use writeback instead of crc, so that we could better check this.
> -Daniel
>
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +     .get_modes = vkms_wb_connector_get_modes,
> > +     .prepare_writeback_job = vkms_wb_prepare_job,
> > +     .cleanup_writeback_job = vkms_wb_cleanup_job,
> > +     .atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +     struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +     vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +     drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +     return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +                                         &vkms_wb_connector_funcs,
> > +                                         &vkms_wb_encoder_helper_funcs,
> > +                                         vkms_formats,
> > +                                         ARRAY_SIZE(vkms_formats));
> > +}
> > +
> > --
> > 2.21.0
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 

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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Daniel Vetter]

On Fri, Jun 07, 2019 at 11:37:55AM -0300, Rodrigo Siqueira wrote:
> On Fri, Jun 7, 2019 at 4:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> > > When vkms calls drm_universal_plane_init(), it sets 0 for the
> > > possible_crtcs parameter which works well for a single encoder and
> > > connector; however, this approach is not flexible and does not fit well
> > > for vkms. This commit adds an index parameter for vkms_plane_init()
> > > which makes code flexible and enables vkms to support other DRM features.
> > >
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >
> > I think a core patch to WARN_ON if this is NULL would be good. Since
> > that's indeed a bit broken ... We'd need to check all callers to make sure
> > there's not other such bugs anywhere ofc.
> > -Daniel
> 
> Do you mean add WARN_ON in `drm_universal_plane_init()` if
> `possible_crtcs` is equal to zero?

Yeah, and same for endcoders I guess too. Altough with encoders I think
there's a ton of broken drivers.
-Daniel

> 
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
> > >  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
> > >  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> > >  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
> > >  4 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 738dd6206d85..92296bd8f623 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > >       dev->mode_config.max_height = YRES_MAX;
> > >       dev->mode_config.preferred_depth = 24;
> > >
> > > -     return vkms_output_init(vkmsdev);
> > > +     return vkms_output_init(vkmsdev, 0);
> > >  }
> > >
> > >  static int __init vkms_init(void)
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 81f1cfbeb936..e81073dea154 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > >                              int *max_error, ktime_t *vblank_time,
> > >                              bool in_vblank_irq);
> > >
> > > -int vkms_output_init(struct vkms_device *vkmsdev);
> > > +int vkms_output_init(struct vkms_device *vkmsdev, int index);
> > >
> > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > -                               enum drm_plane_type type);
> > > +                               enum drm_plane_type type, int index);
> > >
> > >  /* Gem stuff */
> > >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > index 3b162b25312e..1442b447c707 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> > >       .get_modes    = vkms_conn_get_modes,
> > >  };
> > >
> > > -int vkms_output_init(struct vkms_device *vkmsdev)
> > > +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > >  {
> > >       struct vkms_output *output = &vkmsdev->output;
> > >       struct drm_device *dev = &vkmsdev->drm;
> > > @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > >       struct drm_plane *primary, *cursor = NULL;
> > >       int ret;
> > >
> > > -     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> > > +     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> > >       if (IS_ERR(primary))
> > >               return PTR_ERR(primary);
> > >
> > >       if (enable_cursor) {
> > > -             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> > > +             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> > >               if (IS_ERR(cursor)) {
> > >                       ret = PTR_ERR(cursor);
> > >                       goto err_cursor;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index 0e67d2d42f0c..20ffc52f9194 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> > >  };
> > >
> > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > -                               enum drm_plane_type type)
> > > +                               enum drm_plane_type type, int index)
> > >  {
> > >       struct drm_device *dev = &vkmsdev->drm;
> > >       const struct drm_plane_helper_funcs *funcs;
> > > @@ -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 << index,
> > >                                      &vkms_plane_funcs,
> > >                                      formats, nformats,
> > >                                      NULL, type, NULL);
> > > --
> > > 2.21.0
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 
> 
> -- 
> 
> Rodrigo Siqueira
> https://siqueira.tech

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

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Liviu Dudau]

On Fri, Jun 07, 2019 at 11:58:04AM -0300, Rodrigo Siqueira wrote:
> On Fri, Jun 7, 2019 at 4:48 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > > This patch implements the necessary functions to add writeback support
> > > for vkms. This feature is useful for testing compositors if you don’t
> > > have hardware with writeback support.
> > >
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > ---
> > >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> > >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> > >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> > >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> > >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> > >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> > >  6 files changed, 206 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > >
> > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > index 89f09bec7b23..90eb7acd618d 100644
> > > --- a/drivers/gpu/drm/vkms/Makefile
> > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > @@ -1,4 +1,11 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > > +vkms-y := \
> > > +     vkms_drv.o \
> > > +     vkms_plane.o \
> > > +     vkms_output.o \
> > > +     vkms_crtc.o \
> > > +     vkms_gem.o \
> > > +     vkms_crc.o \
> > > +     vkms_writeback.o
> > >
> > >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > index 1bbe099b7db8..ce797e265b1b 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > >       if (!ret)
> > >               DRM_ERROR("vkms failure on handling vblank");
> > >
> > > +     if (output->writeback_status == WB_START) {
> > > +             drm_writeback_signal_completion(&output->wb_connector, 0);
> > > +             output->writeback_status = WB_STOP;
> > > +     }
> > > +
> > >       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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> > >               goto out_fini;
> > >       }
> > >
> > > +     vkms_device->output.writeback_status = WB_DISABLED;
> > > +     if (enable_writeback) {
> > > +             vkms_device->output.writeback_status = WB_STOP;
> > > +             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 e81073dea154..ca1f9ee63ec8 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -7,6 +7,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
> > > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> > >       u64 frame_end;
> > >  };
> > >
> > > +enum wb_status {
> > > +     WB_DISABLED,
> > > +     WB_START,
> > > +     WB_STOP,
> > > +};
> > > +
> > >  struct vkms_output {
> > >       struct drm_crtc crtc;
> > >       struct drm_encoder encoder;
> > >       struct drm_connector connector;
> > > +     struct drm_writeback_connector wb_connector;
> > >       struct hrtimer vblank_hrtimer;
> > >       ktime_t period_ns;
> > >       struct drm_pending_vblank_event *event;
> > >       bool crc_enabled;
> > > +     enum wb_status writeback_status;
> > >       /* ordered wq for crc_work */
> > >       struct workqueue_struct *crc_workq;
> > >       /* protects concurrent access to crc_data */
> > > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > >                          size_t *values_cnt);
> > >  void vkms_crc_work_handle(struct work_struct *work);
> > >
> > > +/* Writeback */
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > > +
> > >  #endif /* _VKMS_DRV_H_ */
> > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > index 1442b447c707..1fc1d4e9585c 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > >               goto err_attach;
> > >       }
> > >
> > > +     if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > > +             ret = enable_writeback_connector(vkmsdev);
> > > +             if (ret)
> > > +                     DRM_ERROR("Failed to init writeback connector\n");
> > > +     }
> > > +
> > >       drm_mode_config_reset(dev);
> > >
> > >       return 0;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > new file mode 100644
> > > index 000000000000..f7b962ae5646
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > @@ -0,0 +1,165 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +#include "vkms_drv.h"
> > > +#include <drm/drm_writeback.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > +
> > > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > > +     .destroy = drm_connector_cleanup,
> > > +     .reset = drm_atomic_helper_connector_reset,
> > > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > +};
> > > +
> > > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > > +                                     struct drm_crtc_state *crtc_state,
> > > +                                     struct drm_connector_state *conn_state)
> > > +{
> > > +     struct drm_framebuffer *fb;
> > > +     const struct drm_display_mode *mode = &crtc_state->mode;
> > > +
> > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > > +             return 0;
> > > +
> > > +     fb = conn_state->writeback_job->fb;
> > > +     if (fb->width != mode->hdisplay || fb->height != 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;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > > +     .atomic_check = vkms_wb_encoder_atomic_check,
> > > +};
> > > +
> > > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > > +{
> > > +     struct drm_device *dev = connector->dev;
> > > +
> > > +     return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > > +                                 dev->mode_config.max_height);
> > > +}
> > > +
> > > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > > +                            struct drm_writeback_job *job)
> > > +{
> > > +     struct vkms_gem_object *vkms_obj;
> > > +     struct drm_gem_object *gem_obj;
> > > +     int ret;
> > > +
> > > +     if (!job->fb)
> > > +             return 0;
> > > +
> > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > +     ret = vkms_gem_vmap(gem_obj);
> > > +     if (ret) {
> > > +             DRM_ERROR("vmap failed: %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > +     job->priv = vkms_obj->vaddr;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > > +                             struct drm_writeback_job *job)
> > > +{
> > > +     struct drm_gem_object *gem_obj;
> > > +
> > > +     if (!job->fb)
> > > +             return;
> > > +
> > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > +     vkms_gem_vunmap(gem_obj);
> > > +}
> > > +
> > > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > > +     struct drm_connector_state *conn_state = wb_conn->base.state;
> > > +     void *priv_data = conn_state->writeback_job->priv;
> > > +     struct vkms_crc_data *primary_data = NULL;
> > > +     struct drm_framebuffer *fb = NULL;
> > > +     struct vkms_gem_object *vkms_obj;
> > > +     struct drm_gem_object *gem_obj;
> > > +     struct drm_plane *plane;
> > > +
> > > +     if (!conn_state)
> > > +             return;
> > > +
> > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > > +             output->writeback_status = WB_STOP;
> > > +             DRM_DEBUG_DRIVER("Disable writeback\n");
> > > +             return;
> > > +     }
> > > +
> > > +     drm_for_each_plane(plane, &vkmsdev->drm) {
> > > +             struct vkms_plane_state *vplane_state;
> > > +             struct vkms_crc_data *plane_data;
> > > +
> > > +             vplane_state = to_vkms_plane_state(plane->state);
> > > +             plane_data = vplane_state->crc_data;
> > > +
> > > +             if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > > +                     continue;
> > > +
> > > +             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > +                     primary_data = plane_data;
> > > +     }
> > > +
> > > +     if (!primary_data)
> > > +             return;
> > > +
> > > +     fb = &primary_data->fb;
> > > +     gem_obj = drm_gem_fb_get_obj(fb, 0);
> > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > +
> > > +     if (!vkms_obj->vaddr || !priv_data)
> > > +             return;
> > > +
> > > +     memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > > +     drm_writeback_queue_job(wb_conn, state);
> > > +     output->writeback_status = WB_START;
> >
> > Hm, if this passes the current writeback tests then I guess those tests
> > are a bit too simple. Or maybe the test can't use the cursor plane, and
> > that's why it doesn't notice that we only write back the primary plane.
> 
> Hi,

Hi Rodrigo,

> 
> Sorry, I knew about that, and I should have highlighted this
> information in the cover letter. In my original plan, I wanted to land
> a basic version of writeback in the vkms to try to upstream the
> kms_writeback; then I wish that I could improve vkms and the tests in
> parallel.
> 
> In summary, the kms_writeback is not upstream yet, and it needs some
> improvements. IHMO, we should try to upstream the current version and
> improve it step-by-step by using VKMS. If Brian and Liviu agree, I
> would like to try to send a new version of their patchset. What do you
> think?

I'll be happy to see the changes you've made to kms_writeback tests. I have
some stashed changes that were addressing the latest comments I've got, so if
we coordinate we should be able to release a new version of the patchset for
the tests that hopefully will get merged.

Best regards,
Liviu

> 
> Daniel, about your patchset (drm/vkms: rework crc worker), just give
> me a time to go through your patchset. After that, I'll make the
> required changes and update the writeback.
> 
> Thank you all for the feedback. I'll prepare a V2 with all the suggestions.
> 
> > Writeback is supposed to write back the same image you'd see on the
> > screen, i.e. with cursor, other planes, any color correction applied and
> > anything else really. That means vkms writeback needs to be integrated
> > into the crc computation. We need to run that work either when there's a
> > writeback job or when we need a crc. Crc would be only computed when
> > needed, and for writeback we need to put the entire composited/blended
> > buffer into the writeback buffer.
> >
> > That's why I said yesterday that your work will conflict with my work to
> > reorg crc work handling, aside from the final step it needs to do all the
> > same things.
> >
> > I guess would be good to improve igt and add a cursor testcase for write
> > (similar to the crc cursor tests we have), or maybe create support in igt
> > to use writeback instead of crc, so that we could better check this.
> > -Daniel
> >
> > > +}
> > > +
> > > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > > +     .get_modes = vkms_wb_connector_get_modes,
> > > +     .prepare_writeback_job = vkms_wb_prepare_job,
> > > +     .cleanup_writeback_job = vkms_wb_cleanup_job,
> > > +     .atomic_commit = vkms_wb_atomic_commit,
> > > +};
> > > +
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > > +{
> > > +     struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > +
> > > +     vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > > +     drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > > +
> > > +     return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > > +                                         &vkms_wb_connector_funcs,
> > > +                                         &vkms_wb_encoder_helper_funcs,
> > > +                                         vkms_formats,
> > > +                                         ARRAY_SIZE(vkms_formats));
> > > +}
> > > +
> > > --
> > > 2.21.0
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 
> 
> -- 
> 
> Rodrigo Siqueira
> https://siqueira.tech

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

On Mon, Jun 10, 2019 at 12:39 PM Liviu Dudau <liviu.dudau@arm.com> wrote:
>
> On Fri, Jun 07, 2019 at 11:58:04AM -0300, Rodrigo Siqueira wrote:
> > On Fri, Jun 7, 2019 at 4:48 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > > > This patch implements the necessary functions to add writeback support
> > > > for vkms. This feature is useful for testing compositors if you don’t
> > > > have hardware with writeback support.
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> > > >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> > > >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> > > >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> > > >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> > > >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> > > >  6 files changed, 206 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > > index 89f09bec7b23..90eb7acd618d 100644
> > > > --- a/drivers/gpu/drm/vkms/Makefile
> > > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > > @@ -1,4 +1,11 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > > > +vkms-y := \
> > > > +     vkms_drv.o \
> > > > +     vkms_plane.o \
> > > > +     vkms_output.o \
> > > > +     vkms_crtc.o \
> > > > +     vkms_gem.o \
> > > > +     vkms_crc.o \
> > > > +     vkms_writeback.o
> > > >
> > > >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > index 1bbe099b7db8..ce797e265b1b 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > >       if (!ret)
> > > >               DRM_ERROR("vkms failure on handling vblank");
> > > >
> > > > +     if (output->writeback_status == WB_START) {
> > > > +             drm_writeback_signal_completion(&output->wb_connector, 0);
> > > > +             output->writeback_status = WB_STOP;
> > > > +     }
> > > > +
> > > >       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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> > > >               goto out_fini;
> > > >       }
> > > >
> > > > +     vkms_device->output.writeback_status = WB_DISABLED;
> > > > +     if (enable_writeback) {
> > > > +             vkms_device->output.writeback_status = WB_STOP;
> > > > +             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 e81073dea154..ca1f9ee63ec8 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -7,6 +7,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
> > > > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> > > >       u64 frame_end;
> > > >  };
> > > >
> > > > +enum wb_status {
> > > > +     WB_DISABLED,
> > > > +     WB_START,
> > > > +     WB_STOP,
> > > > +};
> > > > +
> > > >  struct vkms_output {
> > > >       struct drm_crtc crtc;
> > > >       struct drm_encoder encoder;
> > > >       struct drm_connector connector;
> > > > +     struct drm_writeback_connector wb_connector;
> > > >       struct hrtimer vblank_hrtimer;
> > > >       ktime_t period_ns;
> > > >       struct drm_pending_vblank_event *event;
> > > >       bool crc_enabled;
> > > > +     enum wb_status writeback_status;
> > > >       /* ordered wq for crc_work */
> > > >       struct workqueue_struct *crc_workq;
> > > >       /* protects concurrent access to crc_data */
> > > > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > > >                          size_t *values_cnt);
> > > >  void vkms_crc_work_handle(struct work_struct *work);
> > > >
> > > > +/* Writeback */
> > > > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > > > +
> > > >  #endif /* _VKMS_DRV_H_ */
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > index 1442b447c707..1fc1d4e9585c 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >               goto err_attach;
> > > >       }
> > > >
> > > > +     if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > > > +             ret = enable_writeback_connector(vkmsdev);
> > > > +             if (ret)
> > > > +                     DRM_ERROR("Failed to init writeback connector\n");
> > > > +     }
> > > > +
> > > >       drm_mode_config_reset(dev);
> > > >
> > > >       return 0;
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > new file mode 100644
> > > > index 000000000000..f7b962ae5646
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > @@ -0,0 +1,165 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +
> > > > +#include "vkms_drv.h"
> > > > +#include <drm/drm_writeback.h>
> > > > +#include <drm/drm_probe_helper.h>
> > > > +#include <drm/drm_atomic_helper.h>
> > > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > > +
> > > > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > > > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > > > +     .destroy = drm_connector_cleanup,
> > > > +     .reset = drm_atomic_helper_connector_reset,
> > > > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > > +};
> > > > +
> > > > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > > > +                                     struct drm_crtc_state *crtc_state,
> > > > +                                     struct drm_connector_state *conn_state)
> > > > +{
> > > > +     struct drm_framebuffer *fb;
> > > > +     const struct drm_display_mode *mode = &crtc_state->mode;
> > > > +
> > > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > > > +             return 0;
> > > > +
> > > > +     fb = conn_state->writeback_job->fb;
> > > > +     if (fb->width != mode->hdisplay || fb->height != 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;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > > > +     .atomic_check = vkms_wb_encoder_atomic_check,
> > > > +};
> > > > +
> > > > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > > > +{
> > > > +     struct drm_device *dev = connector->dev;
> > > > +
> > > > +     return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > > > +                                 dev->mode_config.max_height);
> > > > +}
> > > > +
> > > > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > > > +                            struct drm_writeback_job *job)
> > > > +{
> > > > +     struct vkms_gem_object *vkms_obj;
> > > > +     struct drm_gem_object *gem_obj;
> > > > +     int ret;
> > > > +
> > > > +     if (!job->fb)
> > > > +             return 0;
> > > > +
> > > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > > +     ret = vkms_gem_vmap(gem_obj);
> > > > +     if (ret) {
> > > > +             DRM_ERROR("vmap failed: %d\n", ret);
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > > +     job->priv = vkms_obj->vaddr;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > > > +                             struct drm_writeback_job *job)
> > > > +{
> > > > +     struct drm_gem_object *gem_obj;
> > > > +
> > > > +     if (!job->fb)
> > > > +             return;
> > > > +
> > > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > > +     vkms_gem_vunmap(gem_obj);
> > > > +}
> > > > +
> > > > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > > > +     struct drm_connector_state *conn_state = wb_conn->base.state;
> > > > +     void *priv_data = conn_state->writeback_job->priv;
> > > > +     struct vkms_crc_data *primary_data = NULL;
> > > > +     struct drm_framebuffer *fb = NULL;
> > > > +     struct vkms_gem_object *vkms_obj;
> > > > +     struct drm_gem_object *gem_obj;
> > > > +     struct drm_plane *plane;
> > > > +
> > > > +     if (!conn_state)
> > > > +             return;
> > > > +
> > > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > > > +             output->writeback_status = WB_STOP;
> > > > +             DRM_DEBUG_DRIVER("Disable writeback\n");
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     drm_for_each_plane(plane, &vkmsdev->drm) {
> > > > +             struct vkms_plane_state *vplane_state;
> > > > +             struct vkms_crc_data *plane_data;
> > > > +
> > > > +             vplane_state = to_vkms_plane_state(plane->state);
> > > > +             plane_data = vplane_state->crc_data;
> > > > +
> > > > +             if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > > > +                     continue;
> > > > +
> > > > +             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > > +                     primary_data = plane_data;
> > > > +     }
> > > > +
> > > > +     if (!primary_data)
> > > > +             return;
> > > > +
> > > > +     fb = &primary_data->fb;
> > > > +     gem_obj = drm_gem_fb_get_obj(fb, 0);
> > > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > > +
> > > > +     if (!vkms_obj->vaddr || !priv_data)
> > > > +             return;
> > > > +
> > > > +     memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > > > +     drm_writeback_queue_job(wb_conn, state);
> > > > +     output->writeback_status = WB_START;
> > >
> > > Hm, if this passes the current writeback tests then I guess those tests
> > > are a bit too simple. Or maybe the test can't use the cursor plane, and
> > > that's why it doesn't notice that we only write back the primary plane.
> >
> > Hi,
>
> Hi Rodrigo,
>
> >
> > Sorry, I knew about that, and I should have highlighted this
> > information in the cover letter. In my original plan, I wanted to land
> > a basic version of writeback in the vkms to try to upstream the
> > kms_writeback; then I wish that I could improve vkms and the tests in
> > parallel.
> >
> > In summary, the kms_writeback is not upstream yet, and it needs some
> > improvements. IHMO, we should try to upstream the current version and
> > improve it step-by-step by using VKMS. If Brian and Liviu agree, I
> > would like to try to send a new version of their patchset. What do you
> > think?
>
> I'll be happy to see the changes you've made to kms_writeback tests. I have
> some stashed changes that were addressing the latest comments I've got, so if
> we coordinate we should be able to release a new version of the patchset for
> the tests that hopefully will get merged.

Hi Liviu,

I worked based on v5 version
(https://patchwork.kernel.org/cover/10764963/). This week I will
rework some of the commits from the v5, and I’ll send a v6. Does that
work for you?

Best Regards

> Best regards,
> Liviu
>
> >
> > Daniel, about your patchset (drm/vkms: rework crc worker), just give
> > me a time to go through your patchset. After that, I'll make the
> > required changes and update the writeback.
> >
> > Thank you all for the feedback. I'll prepare a V2 with all the suggestions.
> >
> > > Writeback is supposed to write back the same image you'd see on the
> > > screen, i.e. with cursor, other planes, any color correction applied and
> > > anything else really. That means vkms writeback needs to be integrated
> > > into the crc computation. We need to run that work either when there's a
> > > writeback job or when we need a crc. Crc would be only computed when
> > > needed, and for writeback we need to put the entire composited/blended
> > > buffer into the writeback buffer.
> > >
> > > That's why I said yesterday that your work will conflict with my work to
> > > reorg crc work handling, aside from the final step it needs to do all the
> > > same things.
> > >
> > > I guess would be good to improve igt and add a cursor testcase for write
> > > (similar to the crc cursor tests we have), or maybe create support in igt
> > > to use writeback instead of crc, so that we could better check this.
> > > -Daniel
> > >
> > > > +}
> > > > +
> > > > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > > > +     .get_modes = vkms_wb_connector_get_modes,
> > > > +     .prepare_writeback_job = vkms_wb_prepare_job,
> > > > +     .cleanup_writeback_job = vkms_wb_cleanup_job,
> > > > +     .atomic_commit = vkms_wb_atomic_commit,
> > > > +};
> > > > +
> > > > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > > > +{
> > > > +     struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > > +
> > > > +     vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > > > +     drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > > > +
> > > > +     return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > > > +                                         &vkms_wb_connector_funcs,
> > > > +                                         &vkms_wb_encoder_helper_funcs,
> > > > +                                         vkms_formats,
> > > > +                                         ARRAY_SIZE(vkms_formats));
> > > > +}
> > > > +
> > > > --
> > > > 2.21.0
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> >
> >
> > --
> >
> > Rodrigo Siqueira
> > https://siqueira.tech
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 

Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH 2/2] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

On Mon, Jun 10, 2019 at 12:39 PM Liviu Dudau <liviu.dudau@arm.com> wrote:
>
> On Fri, Jun 07, 2019 at 11:58:04AM -0300, Rodrigo Siqueira wrote:
> > On Fri, Jun 7, 2019 at 4:48 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
> > > > This patch implements the necessary functions to add writeback support
> > > > for vkms. This feature is useful for testing compositors if you don’t
> > > > have hardware with writeback support.
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> > > >  drivers/gpu/drm/vkms/vkms_crtc.c      |   5 +
> > > >  drivers/gpu/drm/vkms/vkms_drv.c       |  10 ++
> > > >  drivers/gpu/drm/vkms/vkms_drv.h       |  12 ++
> > > >  drivers/gpu/drm/vkms/vkms_output.c    |   6 +
> > > >  drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
> > > >  6 files changed, 206 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > > index 89f09bec7b23..90eb7acd618d 100644
> > > > --- a/drivers/gpu/drm/vkms/Makefile
> > > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > > @@ -1,4 +1,11 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> > > > +vkms-y := \
> > > > +     vkms_drv.o \
> > > > +     vkms_plane.o \
> > > > +     vkms_output.o \
> > > > +     vkms_crtc.o \
> > > > +     vkms_gem.o \
> > > > +     vkms_crc.o \
> > > > +     vkms_writeback.o
> > > >
> > > >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > index 1bbe099b7db8..ce797e265b1b 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > >       if (!ret)
> > > >               DRM_ERROR("vkms failure on handling vblank");
> > > >
> > > > +     if (output->writeback_status == WB_START) {
> > > > +             drm_writeback_signal_completion(&output->wb_connector, 0);
> > > > +             output->writeback_status = WB_STOP;
> > > > +     }
> > > > +
> > > >       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 92296bd8f623..d5917d5a45e3 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,12 @@ static int __init vkms_init(void)
> > > >               goto out_fini;
> > > >       }
> > > >
> > > > +     vkms_device->output.writeback_status = WB_DISABLED;
> > > > +     if (enable_writeback) {
> > > > +             vkms_device->output.writeback_status = WB_STOP;
> > > > +             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 e81073dea154..ca1f9ee63ec8 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -7,6 +7,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
> > > > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
> > > >       u64 frame_end;
> > > >  };
> > > >
> > > > +enum wb_status {
> > > > +     WB_DISABLED,
> > > > +     WB_START,
> > > > +     WB_STOP,
> > > > +};
> > > > +
> > > >  struct vkms_output {
> > > >       struct drm_crtc crtc;
> > > >       struct drm_encoder encoder;
> > > >       struct drm_connector connector;
> > > > +     struct drm_writeback_connector wb_connector;
> > > >       struct hrtimer vblank_hrtimer;
> > > >       ktime_t period_ns;
> > > >       struct drm_pending_vblank_event *event;
> > > >       bool crc_enabled;
> > > > +     enum wb_status writeback_status;
> > > >       /* ordered wq for crc_work */
> > > >       struct workqueue_struct *crc_workq;
> > > >       /* protects concurrent access to crc_data */
> > > > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > > >                          size_t *values_cnt);
> > > >  void vkms_crc_work_handle(struct work_struct *work);
> > > >
> > > > +/* Writeback */
> > > > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > > > +
> > > >  #endif /* _VKMS_DRV_H_ */
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > index 1442b447c707..1fc1d4e9585c 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >               goto err_attach;
> > > >       }
> > > >
> > > > +     if (vkmsdev->output.writeback_status != WB_DISABLED) {
> > > > +             ret = enable_writeback_connector(vkmsdev);
> > > > +             if (ret)
> > > > +                     DRM_ERROR("Failed to init writeback connector\n");
> > > > +     }
> > > > +
> > > >       drm_mode_config_reset(dev);
> > > >
> > > >       return 0;
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > new file mode 100644
> > > > index 000000000000..f7b962ae5646
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > > @@ -0,0 +1,165 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +
> > > > +#include "vkms_drv.h"
> > > > +#include <drm/drm_writeback.h>
> > > > +#include <drm/drm_probe_helper.h>
> > > > +#include <drm/drm_atomic_helper.h>
> > > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > > +
> > > > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > > > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > > > +     .destroy = drm_connector_cleanup,
> > > > +     .reset = drm_atomic_helper_connector_reset,
> > > > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > > +};
> > > > +
> > > > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > > > +                                     struct drm_crtc_state *crtc_state,
> > > > +                                     struct drm_connector_state *conn_state)
> > > > +{
> > > > +     struct drm_framebuffer *fb;
> > > > +     const struct drm_display_mode *mode = &crtc_state->mode;
> > > > +
> > > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > > > +             return 0;
> > > > +
> > > > +     fb = conn_state->writeback_job->fb;
> > > > +     if (fb->width != mode->hdisplay || fb->height != 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;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > > > +     .atomic_check = vkms_wb_encoder_atomic_check,
> > > > +};
> > > > +
> > > > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > > > +{
> > > > +     struct drm_device *dev = connector->dev;
> > > > +
> > > > +     return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > > > +                                 dev->mode_config.max_height);
> > > > +}
> > > > +
> > > > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > > > +                            struct drm_writeback_job *job)
> > > > +{
> > > > +     struct vkms_gem_object *vkms_obj;
> > > > +     struct drm_gem_object *gem_obj;
> > > > +     int ret;
> > > > +
> > > > +     if (!job->fb)
> > > > +             return 0;
> > > > +
> > > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > > +     ret = vkms_gem_vmap(gem_obj);
> > > > +     if (ret) {
> > > > +             DRM_ERROR("vmap failed: %d\n", ret);
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > > +     job->priv = vkms_obj->vaddr;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > > > +                             struct drm_writeback_job *job)
> > > > +{
> > > > +     struct drm_gem_object *gem_obj;
> > > > +
> > > > +     if (!job->fb)
> > > > +             return;
> > > > +
> > > > +     gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > > +     vkms_gem_vunmap(gem_obj);
> > > > +}
> > > > +
> > > > +static void vkms_wb_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 *wb_conn = &output->wb_connector;
> > > > +     struct drm_connector_state *conn_state = wb_conn->base.state;
> > > > +     void *priv_data = conn_state->writeback_job->priv;
> > > > +     struct vkms_crc_data *primary_data = NULL;
> > > > +     struct drm_framebuffer *fb = NULL;
> > > > +     struct vkms_gem_object *vkms_obj;
> > > > +     struct drm_gem_object *gem_obj;
> > > > +     struct drm_plane *plane;
> > > > +
> > > > +     if (!conn_state)
> > > > +             return;
> > > > +
> > > > +     if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > > > +             output->writeback_status = WB_STOP;
> > > > +             DRM_DEBUG_DRIVER("Disable writeback\n");
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     drm_for_each_plane(plane, &vkmsdev->drm) {
> > > > +             struct vkms_plane_state *vplane_state;
> > > > +             struct vkms_crc_data *plane_data;
> > > > +
> > > > +             vplane_state = to_vkms_plane_state(plane->state);
> > > > +             plane_data = vplane_state->crc_data;
> > > > +
> > > > +             if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
> > > > +                     continue;
> > > > +
> > > > +             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > > +                     primary_data = plane_data;
> > > > +     }
> > > > +
> > > > +     if (!primary_data)
> > > > +             return;
> > > > +
> > > > +     fb = &primary_data->fb;
> > > > +     gem_obj = drm_gem_fb_get_obj(fb, 0);
> > > > +     vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > > +
> > > > +     if (!vkms_obj->vaddr || !priv_data)
> > > > +             return;
> > > > +
> > > > +     memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > > > +     drm_writeback_queue_job(wb_conn, state);
> > > > +     output->writeback_status = WB_START;
> > >
> > > Hm, if this passes the current writeback tests then I guess those tests
> > > are a bit too simple. Or maybe the test can't use the cursor plane, and
> > > that's why it doesn't notice that we only write back the primary plane.
> >
> > Hi,
>
> Hi Rodrigo,
>
> >
> > Sorry, I knew about that, and I should have highlighted this
> > information in the cover letter. In my original plan, I wanted to land
> > a basic version of writeback in the vkms to try to upstream the
> > kms_writeback; then I wish that I could improve vkms and the tests in
> > parallel.
> >
> > In summary, the kms_writeback is not upstream yet, and it needs some
> > improvements. IHMO, we should try to upstream the current version and
> > improve it step-by-step by using VKMS. If Brian and Liviu agree, I
> > would like to try to send a new version of their patchset. What do you
> > think?
>
> I'll be happy to see the changes you've made to kms_writeback tests. I have
> some stashed changes that were addressing the latest comments I've got, so if
> we coordinate we should be able to release a new version of the patchset for
> the tests that hopefully will get merged.

Hi Liviu,

I worked based on v5 version
(https://patchwork.kernel.org/cover/10764963/). This week I will
rework some of the commits from the v5, and I’ll send a v6. Does that
work for you?

Best Regards

> Best regards,
> Liviu
>
> >
> > Daniel, about your patchset (drm/vkms: rework crc worker), just give
> > me a time to go through your patchset. After that, I'll make the
> > required changes and update the writeback.
> >
> > Thank you all for the feedback. I'll prepare a V2 with all the suggestions.
> >
> > > Writeback is supposed to write back the same image you'd see on the
> > > screen, i.e. with cursor, other planes, any color correction applied and
> > > anything else really. That means vkms writeback needs to be integrated
> > > into the crc computation. We need to run that work either when there's a
> > > writeback job or when we need a crc. Crc would be only computed when
> > > needed, and for writeback we need to put the entire composited/blended
> > > buffer into the writeback buffer.
> > >
> > > That's why I said yesterday that your work will conflict with my work to
> > > reorg crc work handling, aside from the final step it needs to do all the
> > > same things.
> > >
> > > I guess would be good to improve igt and add a cursor testcase for write
> > > (similar to the crc cursor tests we have), or maybe create support in igt
> > > to use writeback instead of crc, so that we could better check this.
> > > -Daniel
> > >
> > > > +}
> > > > +
> > > > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > > > +     .get_modes = vkms_wb_connector_get_modes,
> > > > +     .prepare_writeback_job = vkms_wb_prepare_job,
> > > > +     .cleanup_writeback_job = vkms_wb_cleanup_job,
> > > > +     .atomic_commit = vkms_wb_atomic_commit,
> > > > +};
> > > > +
> > > > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > > > +{
> > > > +     struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > > +
> > > > +     vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > > > +     drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > > > +
> > > > +     return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > > > +                                         &vkms_wb_connector_funcs,
> > > > +                                         &vkms_wb_encoder_helper_funcs,
> > > > +                                         vkms_formats,
> > > > +                                         ARRAY_SIZE(vkms_formats));
> > > > +}
> > > > +
> > > > --
> > > > 2.21.0
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> >
> >
> > --
> >
> > Rodrigo Siqueira
> > https://siqueira.tech
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 

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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

On 06/07, Daniel Vetter wrote:
> On Fri, Jun 07, 2019 at 11:37:55AM -0300, Rodrigo Siqueira wrote:
> > On Fri, Jun 7, 2019 at 4:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> > > > When vkms calls drm_universal_plane_init(), it sets 0 for the
> > > > possible_crtcs parameter which works well for a single encoder and
> > > > connector; however, this approach is not flexible and does not fit well
> > > > for vkms. This commit adds an index parameter for vkms_plane_init()
> > > > which makes code flexible and enables vkms to support other DRM features.
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > >
> > > I think a core patch to WARN_ON if this is NULL would be good. Since
> > > that's indeed a bit broken ... We'd need to check all callers to make sure
> > > there's not other such bugs anywhere ofc.
> > > -Daniel
> > 
> > Do you mean add WARN_ON in `drm_universal_plane_init()` if
> > `possible_crtcs` is equal to zero?
> 
> Yeah, and same for endcoders I guess too. Altough with encoders I think
> there's a ton of broken drivers.

I made the patch, but when I started to write the commit message, I just
realized that I did not understand why possible_crtcs should not be
equal zero. Why can we not use zero?

> -Daniel
> 
> > 
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
> > > >  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
> > > >  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> > > >  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
> > > >  4 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > index 738dd6206d85..92296bd8f623 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > > >       dev->mode_config.max_height = YRES_MAX;
> > > >       dev->mode_config.preferred_depth = 24;
> > > >
> > > > -     return vkms_output_init(vkmsdev);
> > > > +     return vkms_output_init(vkmsdev, 0);
> > > >  }
> > > >
> > > >  static int __init vkms_init(void)
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index 81f1cfbeb936..e81073dea154 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > > >                              int *max_error, ktime_t *vblank_time,
> > > >                              bool in_vblank_irq);
> > > >
> > > > -int vkms_output_init(struct vkms_device *vkmsdev);
> > > > +int vkms_output_init(struct vkms_device *vkmsdev, int index);
> > > >
> > > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > > -                               enum drm_plane_type type);
> > > > +                               enum drm_plane_type type, int index);
> > > >
> > > >  /* Gem stuff */
> > > >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > index 3b162b25312e..1442b447c707 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > > @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> > > >       .get_modes    = vkms_conn_get_modes,
> > > >  };
> > > >
> > > > -int vkms_output_init(struct vkms_device *vkmsdev)
> > > > +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >  {
> > > >       struct vkms_output *output = &vkmsdev->output;
> > > >       struct drm_device *dev = &vkmsdev->drm;
> > > > @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > > >       struct drm_plane *primary, *cursor = NULL;
> > > >       int ret;
> > > >
> > > > -     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> > > > +     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> > > >       if (IS_ERR(primary))
> > > >               return PTR_ERR(primary);
> > > >
> > > >       if (enable_cursor) {
> > > > -             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> > > > +             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> > > >               if (IS_ERR(cursor)) {
> > > >                       ret = PTR_ERR(cursor);
> > > >                       goto err_cursor;
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > index 0e67d2d42f0c..20ffc52f9194 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> > > >  };
> > > >
> > > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > > -                               enum drm_plane_type type)
> > > > +                               enum drm_plane_type type, int index)
> > > >  {
> > > >       struct drm_device *dev = &vkmsdev->drm;
> > > >       const struct drm_plane_helper_funcs *funcs;
> > > > @@ -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 << index,
> > > >                                      &vkms_plane_funcs,
> > > >                                      formats, nformats,
> > > >                                      NULL, type, NULL);
> > > > --
> > > > 2.21.0
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > 
> > 
> > -- 
> > 
> > Rodrigo Siqueira
> > https://siqueira.tech
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

On 06/07, Daniel Vetter wrote:
> On Fri, Jun 07, 2019 at 11:37:55AM -0300, Rodrigo Siqueira wrote:
> > On Fri, Jun 7, 2019 at 4:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jun 06, 2019 at 07:40:38PM -0300, Rodrigo Siqueira wrote:
> > > > When vkms calls drm_universal_plane_init(), it sets 0 for the
> > > > possible_crtcs parameter which works well for a single encoder and
> > > > connector; however, this approach is not flexible and does not fit well
> > > > for vkms. This commit adds an index parameter for vkms_plane_init()
> > > > which makes code flexible and enables vkms to support other DRM features.
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > >
> > > I think a core patch to WARN_ON if this is NULL would be good. Since
> > > that's indeed a bit broken ... We'd need to check all callers to make sure
> > > there's not other such bugs anywhere ofc.
> > > -Daniel
> > 
> > Do you mean add WARN_ON in `drm_universal_plane_init()` if
> > `possible_crtcs` is equal to zero?
> 
> Yeah, and same for endcoders I guess too. Altough with encoders I think
> there's a ton of broken drivers.

I made the patch, but when I started to write the commit message, I just
realized that I did not understand why possible_crtcs should not be
equal zero. Why can we not use zero?

> -Daniel
> 
> > 
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
> > > >  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
> > > >  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> > > >  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
> > > >  4 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > index 738dd6206d85..92296bd8f623 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -92,7 +92,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> > > >       dev->mode_config.max_height = YRES_MAX;
> > > >       dev->mode_config.preferred_depth = 24;
> > > >
> > > > -     return vkms_output_init(vkmsdev);
> > > > +     return vkms_output_init(vkmsdev, 0);
> > > >  }
> > > >
> > > >  static int __init vkms_init(void)
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index 81f1cfbeb936..e81073dea154 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -113,10 +113,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> > > >                              int *max_error, ktime_t *vblank_time,
> > > >                              bool in_vblank_irq);
> > > >
> > > > -int vkms_output_init(struct vkms_device *vkmsdev);
> > > > +int vkms_output_init(struct vkms_device *vkmsdev, int index);
> > > >
> > > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > > -                               enum drm_plane_type type);
> > > > +                               enum drm_plane_type type, int index);
> > > >
> > > >  /* Gem stuff */
> > > >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > > index 3b162b25312e..1442b447c707 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > > @@ -36,7 +36,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> > > >       .get_modes    = vkms_conn_get_modes,
> > > >  };
> > > >
> > > > -int vkms_output_init(struct vkms_device *vkmsdev)
> > > > +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > > >  {
> > > >       struct vkms_output *output = &vkmsdev->output;
> > > >       struct drm_device *dev = &vkmsdev->drm;
> > > > @@ -46,12 +46,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > > >       struct drm_plane *primary, *cursor = NULL;
> > > >       int ret;
> > > >
> > > > -     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> > > > +     primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> > > >       if (IS_ERR(primary))
> > > >               return PTR_ERR(primary);
> > > >
> > > >       if (enable_cursor) {
> > > > -             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> > > > +             cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> > > >               if (IS_ERR(cursor)) {
> > > >                       ret = PTR_ERR(cursor);
> > > >                       goto err_cursor;
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > index 0e67d2d42f0c..20ffc52f9194 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > @@ -168,7 +168,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> > > >  };
> > > >
> > > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > > > -                               enum drm_plane_type type)
> > > > +                               enum drm_plane_type type, int index)
> > > >  {
> > > >       struct drm_device *dev = &vkmsdev->drm;
> > > >       const struct drm_plane_helper_funcs *funcs;
> > > > @@ -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 << index,
> > > >                                      &vkms_plane_funcs,
> > > >                                      formats, nformats,
> > > >                                      NULL, type, NULL);
> > > > --
> > > > 2.21.0
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > 
> > 
> > -- 
> > 
> > Rodrigo Siqueira
> > https://siqueira.tech
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Simon Ser]

On Tuesday, June 18, 2019 5:19 AM, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> I made the patch, but when I started to write the commit message, I just
> realized that I did not understand why possible_crtcs should not be
> equal zero. Why can we not use zero?

Hi,

possible_crtcs is a bitfield. If it's zero, it means the plane cannot
be attached to any CRTC, which makes it rather useless.

Thanks,

Simon

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Simon Ser]

On Tuesday, June 18, 2019 5:19 AM, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> I made the patch, but when I started to write the commit message, I just
> realized that I did not understand why possible_crtcs should not be
> equal zero. Why can we not use zero?

Hi,

possible_crtcs is a bitfield. If it's zero, it means the plane cannot
be attached to any CRTC, which makes it rather useless.

Thanks,

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

Re: [PATCH 1/2] drm/vkms: Use index instead of 0 in possible crtc

[Rodrigo Siqueira]

On Tue, Jun 18, 2019 at 2:18 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, June 18, 2019 5:19 AM, Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> > I made the patch, but when I started to write the commit message, I just
> > realized that I did not understand why possible_crtcs should not be
> > equal zero. Why can we not use zero?
>
> Hi,
>
> possible_crtcs is a bitfield. If it's zero, it means the plane cannot
> be attached to any CRTC, which makes it rather useless.

Hi,

Thank you very much for your explanation. I'll try to finish the patch.

> Thanks,
>
> Simon



-- 

Rodrigo Siqueira
https://siqueira.tech