Expanding the basic vkms features

This commit appends the essential CRTCs infrastructure for VKMS. CRTCs demands other elements to work correctly, in this sense, this patch adds a new data struct that centralizes the data structures related to the output, simple planes management, and single encoder attached to the connector.

Signed-off-by Rodrigo Siqueira <[email protected]>

---
 drivers/gpu/drm/vkms/Makefile      |   2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c    |  66 +++++-----------
 drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
 drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
 6 files changed, 275 insertions(+), 53 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 2aef948d3a34..3f774a6a9c58 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crtc.c new file +47 -0

new file mode 100644
index 000000000000..4ec1424ed8b8
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+
+static const struct drm_crtc_funcs vkms_crtc_funcs = {
+	.set_config             = drm_atomic_helper_set_config,
+	.destroy                = drm_crtc_cleanup,
+	.page_flip              = drm_atomic_helper_page_flip,
+	.reset                  = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
+};
+
+static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
+				  struct drm_crtc_state *state)
+{
+	return 0;
+}
+
+static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_check  = vkms_crtc_atomic_check,
+};
+
+int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+		   struct drm_plane *primary, struct drm_plane *cursor)
+{
+	int ret;
+
+	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
+					&vkms_crtc_funcs, NULL);
+	if (ret) {
+		DRM_ERROR("Failed to init CRTC\n");
+		return ret;
+	}
+
+	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_drv.c +17 -49

index aec3f180f96d..eae5f1f293d0 100644
@@ -6,7 +6,6 @@
  */
 
 #include <linux/module.h>
-#include <drm/drmP.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>

@@ -18,12 +17,6 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
-#define XRES_MIN    32
-#define YRES_MIN    32
-
-#define XRES_MAX  8192
-#define YRES_MAX  8192
-
 static struct vkms_device *vkms_device;
 
 static const struct file_operations vkms_driver_fops = {

@@ -59,25 +52,24 @@ static struct drm_driver vkms_driver = {
 	.minor			= DRIVER_MINOR,
 };
 
-static const u32 vkms_formats[] = {
-	DRM_FORMAT_XRGB8888,
+static const struct drm_mode_config_funcs vkms_mode_funcs = {
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
-static void vkms_connector_destroy(struct drm_connector *connector)
+static int vkms_modeset_init(struct vkms_device *vkmsdev)
 {
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
+	struct drm_device *dev = &vkmsdev->drm;
 
-static const struct drm_connector_funcs vkms_connector_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = vkms_connector_destroy,
-};
+	drm_mode_config_init(dev);
+	dev->mode_config.funcs = &vkms_mode_funcs;
+	dev->mode_config.min_width = XRES_MIN;
+	dev->mode_config.min_height = YRES_MIN;
+	dev->mode_config.max_width = XRES_MAX;
+	dev->mode_config.max_height = YRES_MAX;
 
-static const struct drm_mode_config_funcs vkms_mode_funcs = {
-	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = drm_atomic_helper_commit,
-};
+	return vkms_output_init(vkmsdev);
+}
 
 static int __init vkms_init(void)
 {

@@ -98,48 +90,24 @@ static int __init vkms_init(void)
 		goto out_fini;
 	}
 
-	drm_mode_config_init(&vkms_device->drm);
-	vkms_device->drm.mode_config.funcs = &vkms_mode_funcs;
-	vkms_device->drm.mode_config.min_width = XRES_MIN;
-	vkms_device->drm.mode_config.min_height = YRES_MIN;
-	vkms_device->drm.mode_config.max_width = XRES_MAX;
-	vkms_device->drm.mode_config.max_height = YRES_MAX;
-
-	ret = drm_connector_init(&vkms_device->drm, &vkms_device->connector,
-				 &vkms_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
-	if (ret < 0) {
-		DRM_ERROR("Failed to init connector\n");
-		goto out_unregister;
-	}
-
-	ret = drm_simple_display_pipe_init(&vkms_device->drm,
-					   &vkms_device->pipe,
-					   NULL,
-					   vkms_formats,
-					   ARRAY_SIZE(vkms_formats),
-					   NULL,
-					   &vkms_device->connector);
-	if (ret < 0) {
-		DRM_ERROR("Cannot setup simple display pipe\n");
+	ret = vkms_modeset_init(vkms_device);
+	if (ret)
 		goto out_unregister;
-	}
 
 	ret = drm_dev_register(&vkms_device->drm, 0);
 	if (ret)
 		goto out_unregister;
 
-	drm_connector_register(&vkms_device->connector);
-
 	return 0;
 
 out_unregister:
 	platform_device_unregister(vkms_device->platform);
+
 out_fini:
 	drm_dev_fini(&vkms_device->drm);
+
 out_free:
 	kfree(vkms_device);
-
 	return ret;
 }
 

drivers/gpu/drm/vkms/vkms_drv.h +30 -3

index c77c5bf5032a..a9f8b6695683 100644
@@ -1,13 +1,40 @@
 #ifndef _VKMS_DRV_H_
 #define _VKMS_DRV_H_
 
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drmP.h>
+#include <drm/drm.h>
+#include <drm/drm_encoder.h>
+
+#define XRES_MIN    32
+#define YRES_MIN    32
+
+#define XRES_DEF  1024
+#define YRES_DEF   768
+
+#define XRES_MAX  8192
+#define YRES_MAX  8192
+
+static const u32 vkms_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+struct vkms_output {
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+};
 
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
-	struct drm_simple_display_pipe pipe;
-	struct drm_connector connector;
+	struct vkms_output output;
 };
 
+int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+		   struct drm_plane *primary, struct drm_plane *cursor);
+
+int vkms_output_init(struct vkms_device *vkmsdev);
+
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_output.c new file +118 -0

new file mode 100644
index 000000000000..01e701691ea8
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+static void vkms_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs vkms_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = vkms_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int vkms_conn_get_modes(struct drm_connector *connector)
+{
+	int count;
+
+	count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
+	drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);
+
+	return count;
+}
+
+static int vkms_conn_mode_valid(struct drm_connector *connector,
+				struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
+	.get_modes    = vkms_conn_get_modes,
+	.mode_valid   = vkms_conn_mode_valid,
+};
+
+static const struct drm_encoder_funcs vkms_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+int vkms_output_init(struct vkms_device *vkmsdev)
+{
+	struct vkms_output *output = &vkmsdev->output;
+	struct drm_device *dev = &vkmsdev->drm;
+	struct drm_connector *connector = &output->connector;
+	struct drm_encoder *encoder = &output->encoder;
+	struct drm_crtc *crtc = &output->crtc;
+	struct drm_plane *primary;
+	int ret;
+
+	primary = vkms_plane_init(vkmsdev);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
+
+	ret = vkms_crtc_init(dev, crtc, primary, NULL);
+	if (ret)
+		goto err_crtc;
+
+	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
+				 DRM_MODE_CONNECTOR_VIRTUAL);
+	if (ret) {
+		DRM_ERROR("Failed to init connector\n");
+		goto err_connector;
+	}
+
+	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
+
+	ret = drm_connector_register(connector);
+	if (ret) {
+		DRM_ERROR("Failed to register connector\n");
+		goto err_connector_register;
+	}
+
+	ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret) {
+		DRM_ERROR("Failed to init encoder\n");
+		goto err_encoder;
+	}
+	encoder->possible_crtcs = 1;
+
+	ret = drm_mode_connector_attach_encoder(connector, encoder);
+	if (ret) {
+		DRM_ERROR("Failed to attach connector to encoder\n");
+		goto err_attach;
+	}
+
+	drm_mode_config_reset(dev);
+
+	return 0;
+
+err_attach:
+	drm_encoder_cleanup(encoder);
+
+err_encoder:
+	drm_connector_unregister(connector);
+
+err_connector_register:
+	drm_connector_cleanup(connector);
+
+err_connector:
+	drm_crtc_cleanup(crtc);
+
+err_crtc:
+	drm_plane_cleanup(primary);
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_plane.c new file +27 -0

new file mode 100644
index 000000000000..b6231c1e3462
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+static const struct drm_plane_funcs vkms_plane_funcs = {
+	.update_plane		= drm_atomic_helper_update_plane,
+	.disable_plane		= drm_atomic_helper_disable_plane,
+	.destroy		= drm_plane_cleanup,
+	.reset			= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+};
+
+static int vkms_plane_atomic_check(struct drm_plane *plane,
+				   struct drm_plane_state *state)
+{
+	return 0;
+}
+

drivers/gpu/drm/vkms/vkms_plane.c new file +25 -0

+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
+{
+	struct drm_device *dev = &vkmsdev->drm;
+	struct drm_plane *plane;
+	const u32 *formats;
+	int ret, nformats;
+
+	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
+	if (!plane)
+		return ERR_PTR(-ENOMEM);
+
+	formats = vkms_formats;
+	nformats = ARRAY_SIZE(vkms_formats);
+
+	ret = drm_universal_plane_init(dev, plane, 0,
+				       &vkms_plane_funcs,
+				       formats, nformats,
+				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret) {
+		kfree(plane);
+		return ERR_PTR(ret);
+	}
+
+	return plane;
+}

Signature · 1 line

2.17.0

drivers/gpu/drm/vkms/

Quoted 20 lines · Rodrigo Siqueira

This commit appends the essential CRTCs infrastructure for VKMS. CRTCs demands other elements to work correctly, in this sense, this patch adds a new data struct that centralizes the data structures related to the output, simple planes management, and single encoder attached to the connector.

Signed-off-by Rodrigo Siqueira <[email protected]>

---
 drivers/gpu/drm/vkms/Makefile      |   2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c    |  66 +++++-----------
 drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
 drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
 6 files changed, 275 insertions(+), 53 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c

drivers/gpu/drm/vkms/ renamed from drivers/gpu/drm/vkms/Makefile +0 -0

Makefile

Review

Quoted 9 lines

index 2aef948d3a34..3f774a6a9c58 100644

drivers/gpu/drm/vkms/Makefile +1 -1

@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

(quoted diff) +0 -0

b/drivers/gpu/drm/vkms/vkms_crtc.c

Review

Quoted 53 lines

new file mode 100644
index 000000000000..4ec1424ed8b8

drivers/gpu/drm/vkms/vkms_crtc.c new file +47 -0

@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+
+static const struct drm_crtc_funcs vkms_crtc_funcs = {
+     .set_config             = drm_atomic_helper_set_config,
+     .destroy                = drm_crtc_cleanup,
+     .page_flip              = drm_atomic_helper_page_flip,
+     .reset                  = drm_atomic_helper_crtc_reset,
+     .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+     .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
+};
+
+static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
+                               struct drm_crtc_state *state)
+{
+     return 0;
+}
+
+static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+     .atomic_check  = vkms_crtc_atomic_check,
+};
+
+int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+                struct drm_plane *primary, struct drm_plane *cursor)
+{
+     int ret;
+
+     ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
+                                     &vkms_crtc_funcs, NULL);
+     if (ret) {
+             DRM_ERROR("Failed to init CRTC\n");
+             return ret;
+     }
+
+     drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
+
+     return ret;
+}

(quoted diff) +0 -0

b/drivers/gpu/drm/vkms/vkms_drv.c

Quoted 65 lines

index aec3f180f96d..eae5f1f293d0 100644

drivers/gpu/drm/vkms/vkms_drv.c +13 -21

@@ -6,7 +6,6 @@
  */
 
 #include <linux/module.h>
-#include <drm/drmP.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>

@@ -18,12 +17,6 @@
 #define DRIVER_MAJOR 1
 #define DRIVER_MINOR 0
 
-#define XRES_MIN    32
-#define YRES_MIN    32
-
-#define XRES_MAX  8192
-#define YRES_MAX  8192
-
 static struct vkms_device *vkms_device;
 
 static const struct file_operations vkms_driver_fops = {

@@ -59,25 +52,24 @@ static struct drm_driver vkms_driver = {
      .minor                  = DRIVER_MINOR,
 };
 
-static const u32 vkms_formats[] = {
-     DRM_FORMAT_XRGB8888,
+static const struct drm_mode_config_funcs vkms_mode_funcs = {
+     .atomic_check = drm_atomic_helper_check,
+     .atomic_commit = drm_atomic_helper_commit,
 };
 
-static void vkms_connector_destroy(struct drm_connector *connector)
+static int vkms_modeset_init(struct vkms_device *vkmsdev)
 {
-     drm_connector_unregister(connector);
-     drm_connector_cleanup(connector);
-}
+     struct drm_device *dev = &vkmsdev->drm;
 
-static const struct drm_connector_funcs vkms_connector_funcs = {
-     .fill_modes = drm_helper_probe_single_connector_modes,
-     .destroy = vkms_connector_destroy,
-};
+     drm_mode_config_init(dev);
+     dev->mode_config.funcs = &vkms_mode_funcs;
+     dev->mode_config.min_width = XRES_MIN;
+     dev->mode_config.min_height = YRES_MIN;
+     dev->mode_config.max_width = XRES_MAX;
+     dev->mode_config.max_height = YRES_MAX;
 
-static const struct drm_mode_config_funcs vkms_mode_funcs = {
-     .atomic_check = drm_atomic_helper_check,
-     .atomic_commit = drm_atomic_helper_commit,
-};
+     return vkms_output_init(vkmsdev);
+}
 
 static int __init vkms_init(void)
 {

@@ -98,48 +90,24 @@ static int __init vkms_init(void)
              goto out_fini;

drivers/gpu/drm/vkms/vkms_drv.c static int __init vkms_init(void) · line 108

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_drv.c +0 -8

      }
 
-     drm_mode_config_init(&vkms_device->drm);
-     vkms_device->drm.mode_config.funcs = &vkms_mode_funcs;
-     vkms_device->drm.mode_config.min_width = XRES_MIN;
-     vkms_device->drm.mode_config.min_height = YRES_MIN;
-     vkms_device->drm.mode_config.max_width = XRES_MAX;
-     vkms_device->drm.mode_config.max_height = YRES_MAX;
-
-     ret = drm_connector_init(&vkms_device->drm,

&vkms_device->connector,

Review

Quoted 42 lines

drivers/gpu/drm/vkms/vkms_drv.c +4 -20

-                              &vkms_connector_funcs,
-                              DRM_MODE_CONNECTOR_VIRTUAL);
-     if (ret < 0) {
-             DRM_ERROR("Failed to init connector\n");
-             goto out_unregister;
-     }
-
-     ret = drm_simple_display_pipe_init(&vkms_device->drm,
-                                        &vkms_device->pipe,
-                                        NULL,
-                                        vkms_formats,
-                                        ARRAY_SIZE(vkms_formats),
-                                        NULL,
-                                        &vkms_device->connector);
-     if (ret < 0) {
-             DRM_ERROR("Cannot setup simple display pipe\n");
+     ret = vkms_modeset_init(vkms_device);
+     if (ret)
              goto out_unregister;
-     }
 
      ret = drm_dev_register(&vkms_device->drm, 0);
      if (ret)
              goto out_unregister;
 
-     drm_connector_register(&vkms_device->connector);
-
      return 0;
 
 out_unregister:
      platform_device_unregister(vkms_device->platform);
+
 out_fini:
      drm_dev_fini(&vkms_device->drm);
+
 out_free:
      kfree(vkms_device);
-
      return ret;
 }
 

(quoted diff) +0 -0

b/drivers/gpu/drm/vkms/vkms_drv.h

Review

Quoted 48 lines

index c77c5bf5032a..a9f8b6695683 100644

drivers/gpu/drm/vkms/vkms_drv.h +30 -3

@@ -1,13 +1,40 @@
 #ifndef _VKMS_DRV_H_
 #define _VKMS_DRV_H_
 
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drmP.h>
+#include <drm/drm.h>
+#include <drm/drm_encoder.h>
+
+#define XRES_MIN    32
+#define YRES_MIN    32
+
+#define XRES_DEF  1024
+#define YRES_DEF   768
+
+#define XRES_MAX  8192
+#define YRES_MAX  8192
+
+static const u32 vkms_formats[] = {
+     DRM_FORMAT_XRGB8888,
+};
+
+struct vkms_output {
+     struct drm_crtc crtc;
+     struct drm_encoder encoder;
+     struct drm_connector connector;
+};
 
 struct vkms_device {
      struct drm_device drm;
      struct platform_device *platform;
-     struct drm_simple_display_pipe pipe;
-     struct drm_connector connector;
+     struct vkms_output output;
 };
 
+int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+                struct drm_plane *primary, struct drm_plane *cursor);
+
+int vkms_output_init(struct vkms_device *vkmsdev);
+
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
+
 #endif /* _VKMS_DRV_H_ */

(quoted diff) +0 -0

b/drivers/gpu/drm/vkms/vkms_output.c

Quoted 18 lines

new file mode 100644
index 000000000000..01e701691ea8

drivers/gpu/drm/vkms/vkms_output.c new file +13 -0

@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+static void vkms_connector_destroy(struct drm_connector *connector)

drivers/gpu/drm/vkms/vkms_output.c line 23

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_output.c new file +10 -0

+{
+     drm_connector_unregister(connector);
+     drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs vkms_connector_funcs = {
+     .fill_modes = drm_helper_probe_single_connector_modes,
+     .destroy = vkms_connector_destroy,
+     .reset = drm_atomic_helper_connector_reset,
+     .atomic_duplicate_state = drm_atomic_helper_connector_

duplicate_state,

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_output.c new file +10 -0

+     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int vkms_conn_get_modes(struct drm_connector *connector)
+{
+     int count;
+
+     count = drm_add_modes_noedid(connector, XRES_MAX, YRES_MAX);
+     drm_set_preferred_mode(connector, XRES_DEF, YRES_DEF);
+

drivers/gpu/drm/vkms/vkms_output.c

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_output.c new file +10 -0

+     return count;
+}
+
+static int vkms_conn_mode_valid(struct drm_connector *connector,
+                             struct drm_display_mode *mode)
+{
+     return MODE_OK;
+}
+
+static const struct drm_connector_helper_funcs vkms_conn_helper_funcs =

{

Review

Quoted 76 lines

drivers/gpu/drm/vkms/vkms_output.c new file +75 -0

+     .get_modes    = vkms_conn_get_modes,
+     .mode_valid   = vkms_conn_mode_valid,
+};
+
+static const struct drm_encoder_funcs vkms_encoder_funcs = {
+     .destroy = drm_encoder_cleanup,
+};
+
+int vkms_output_init(struct vkms_device *vkmsdev)
+{
+     struct vkms_output *output = &vkmsdev->output;
+     struct drm_device *dev = &vkmsdev->drm;
+     struct drm_connector *connector = &output->connector;
+     struct drm_encoder *encoder = &output->encoder;
+     struct drm_crtc *crtc = &output->crtc;
+     struct drm_plane *primary;
+     int ret;
+
+     primary = vkms_plane_init(vkmsdev);
+     if (IS_ERR(primary))
+             return PTR_ERR(primary);
+
+     ret = vkms_crtc_init(dev, crtc, primary, NULL);
+     if (ret)
+             goto err_crtc;
+
+     ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
+                              DRM_MODE_CONNECTOR_VIRTUAL);
+     if (ret) {
+             DRM_ERROR("Failed to init connector\n");
+             goto err_connector;
+     }
+
+     drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
+
+     ret = drm_connector_register(connector);
+     if (ret) {
+             DRM_ERROR("Failed to register connector\n");
+             goto err_connector_register;
+     }
+
+     ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs,
+                            DRM_MODE_ENCODER_VIRTUAL, NULL);
+     if (ret) {
+             DRM_ERROR("Failed to init encoder\n");
+             goto err_encoder;
+     }
+     encoder->possible_crtcs = 1;
+
+     ret = drm_mode_connector_attach_encoder(connector, encoder);
+     if (ret) {
+             DRM_ERROR("Failed to attach connector to encoder\n");
+             goto err_attach;
+     }
+
+     drm_mode_config_reset(dev);
+
+     return 0;
+
+err_attach:
+     drm_encoder_cleanup(encoder);
+
+err_encoder:
+     drm_connector_unregister(connector);
+
+err_connector_register:
+     drm_connector_cleanup(connector);
+
+err_connector:
+     drm_crtc_cleanup(crtc);
+
+err_crtc:
+     drm_plane_cleanup(primary);
+     return ret;
+}

(quoted diff) +0 -0

b/drivers/gpu/drm/vkms/vkms_plane.c

Quoted 32 lines

new file mode 100644
index 000000000000..b6231c1e3462

drivers/gpu/drm/vkms/vkms_plane.c new file +27 -0

@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "vkms_drv.h"
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+static const struct drm_plane_funcs vkms_plane_funcs = {
+     .update_plane           = drm_atomic_helper_update_plane,
+     .disable_plane          = drm_atomic_helper_disable_plane,
+     .destroy                = drm_plane_cleanup,
+     .reset                  = drm_atomic_helper_plane_reset,
+     .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+     .atomic_destroy_state   = drm_atomic_helper_plane_destroy_state,
+};
+
+static int vkms_plane_atomic_check(struct drm_plane *plane,
+                                struct drm_plane_state *state)
+{
+     return 0;
+}
+

drivers/gpu/drm/vkms/vkms_plane.c line 37

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_plane.c new file +10 -0

+static void vkms_primary_plane_update(struct drm_plane *plane,
+                                   struct drm_plane_state *old_state)
+{
+}
+
+static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
+     .atomic_check           = vkms_plane_atomic_check,
+     .atomic_update          = vkms_primary_plane_update,
+};
+

I think you forgot to use the plane helper below :)

Quoted 28 lines

drivers/gpu/drm/vkms/vkms_plane.c new file +25 -1

+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
+{
+     struct drm_device *dev = &vkmsdev->drm;
+     struct drm_plane *plane;
+     const u32 *formats;
+     int ret, nformats;
+
+     plane = kzalloc(sizeof(*plane), GFP_KERNEL);
+     if (!plane)
+             return ERR_PTR(-ENOMEM);
+
+     formats = vkms_formats;
+     nformats = ARRAY_SIZE(vkms_formats);
+
+     ret = drm_universal_plane_init(dev, plane, 0,
+                                    &vkms_plane_funcs,
+                                    formats, nformats,
+                                    NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+     if (ret) {
+             kfree(plane);
+             return ERR_PTR(ret);
+     }
+
+     return plane;
+}
--

2.17.0

+// SPDX-License-Identifier: GPL-2.0

+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */

Hi all,

Quoted 1 line · Rodrigo Siqueira

+// SPDX-License-Identifier: GPL-2.0

This

Quoted 6 lines

+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */

and this are not the same thing.

The former is 2.0-only, while the explicit text is 2.0-or-later.

Personally, I'd opt for 2.0-only, or even make it a dual 2.0-only OR MIT. Yet again, this is not my code, so not my decision to make.

From a quick glance other vkms files have the same mismatch.

HTH
Emil

Quoted 1 line · Emil Velikov

Hi all,

Hi Emil,

Thanks for your feedback.

Quoted 18 lines

Quoted 1 line · Rodrigo Siqueira

+// SPDX-License-Identifier: GPL-2.0

This

Quoted 6 lines

+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */

and this are not the same thing.

The former is 2.0-only, while the explicit text is 2.0-or-later.

Quoted 2 lines

Personally, I'd opt for 2.0-only, or even make it a dual 2.0-only OR MIT. Yet again, this is not my code, so not my decision to make.

I have to admit that I do not understand too much about these issues, however, now it is time to for learning more about it :)

Do you have any documentation to recommend me about the issue? I want to read it, and fix the problems that you highlighted.

This series of patches add a centralized initialization mechanism, a single CRTC with a plane, an encoder, and extra module information.

Changes in v2:
- Remove unused definitions
- Improve file names
- Improve code separation
- Remove unnecessary formats

Changes in v3:
- Adds drm_crtc_helper_funcs with a simple atomic_check

- Adds extra hooks for drm_connector_funcs hooks (reset, atomic_duplicate_state, atomic_destroy_state)

- Adds drm_connector_helper_funcs
- Adds drm_plane_helper_funcs

- Changes in the commit messages

Rodrigo Siqueira (3):
drm/vkms: Add mode_config initialization
drm/vkms: Add basic CRTC initialization
drm/vkms: Add extra information about vkms

 drivers/gpu/drm/Kconfig            |   8 +-
 drivers/gpu/drm/vkms/Makefile      |   2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c    |  55 +++++---------
 drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
 drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
 7 files changed, 285 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c

Signature · 1 line

2.17.0

Quoted 8 lines · Rodrigo Siqueira

This series of patches add a centralized initialization mechanism, a single CRTC with a plane, an encoder, and extra module information.

Changes in v2:
- Remove unused definitions
- Improve file names
- Improve code separation
- Remove unnecessary formats

Oops, I merged v2 already. I think we need follow-up patches.

Quoted 2 lines

Changes in v3:
- Adds drm_crtc_helper_funcs with a simple atomic_check

Sorry for the late comment on v2, but I think we need to figure out why this goes boom first. It imo shouldn't.

Quoted 2 lines

- Adds extra hooks for drm_connector_funcs hooks (reset, atomic_duplicate_state, atomic_destroy_state)

Hm, reset shouldn't be required. Why do you need it?

Wrt duplicate/destroy state, those are mandatory. I think it'd be good to have checks for those in the drm_*_init functions, but only for atomic drivers. You can use drm_drv_uses_atomic_modeset() and WARN_ON(). There's a bunch of examples already for checking for this stuff, see e.g. dma_fence_init().

Quoted 2 lines

- Adds drm_connector_helper_funcs
- Adds drm_plane_helper_funcs

Same here, would be good to add WARN_ON to the relevant _init() functions to make sure all the mandatory stuff is there to begin with.

Since Rodrigo has typed the fixes to vkms already, could you Haneen look into adding these checks to the core drm core?

Thanks, Daniel

Quoted 19 lines

- Changes in the commit messages

Rodrigo Siqueira (3):
drm/vkms: Add mode_config initialization
drm/vkms: Add basic CRTC initialization
drm/vkms: Add extra information about vkms

 drivers/gpu/drm/Kconfig            |   8 +-
 drivers/gpu/drm/vkms/Makefile      |   2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c    |  55 +++++---------
 drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
 drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
 7 files changed, 285 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c

Quoted 3 lines

Signature · 1 line

2.17.0

Signature · 3 lines

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

Quoted 21 lines · Daniel Vetter

Quoted 8 lines · Rodrigo Siqueira

This series of patches add a centralized initialization mechanism, a single CRTC with a plane, an encoder, and extra module information.

Changes in v2:
- Remove unused definitions
- Improve file names
- Improve code separation
- Remove unnecessary formats

Oops, I merged v2 already. I think we need follow-up patches.

Quoted 2 lines

Changes in v3:
- Adds drm_crtc_helper_funcs with a simple atomic_check

Sorry for the late comment on v2, but I think we need to figure out why this goes boom first. It imo shouldn't.

Quoted 2 lines

- Adds extra hooks for drm_connector_funcs hooks (reset, atomic_duplicate_state, atomic_destroy_state)

Quoted 2 lines

Hm, reset shouldn't be required. Why do you need it?

I've checked the code, and I think the memory for [plane/connector/crtc] states that are used later in duplicate/destroy are allocated in drm_atomic_helper_*_reset. Otherwise checking /sys/kernel/debug/dri/0/state or running the igt
tests causes null pointer dereference error.

Quoted 10 lines

Wrt duplicate/destroy state, those are mandatory. I think it'd be good to have checks for those in the drm_*_init functions, but only for atomic drivers. You can use drm_drv_uses_atomic_modeset() and WARN_ON(). There's a bunch of examples already for checking for this stuff, see e.g. dma_fence_init().

Quoted 2 lines

- Adds drm_connector_helper_funcs
- Adds drm_plane_helper_funcs

Quoted 6 lines

Same here, would be good to add WARN_ON to the relevant _init() functions to make sure all the mandatory stuff is there to begin with.

Since Rodrigo has typed the fixes to vkms already, could you Haneen look into adding these checks to the core drm core?

Sure, I'll work on that.

Quoted 30 lines

Thanks, Daniel

Quoted 22 lines

- Changes in the commit messages

Rodrigo Siqueira (3):
drm/vkms: Add mode_config initialization
drm/vkms: Add basic CRTC initialization
drm/vkms: Add extra information about vkms

 drivers/gpu/drm/Kconfig            |   8 +-
 drivers/gpu/drm/vkms/Makefile      |   2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c    |  55 +++++---------
 drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
 drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
 7 files changed, 285 insertions(+), 40 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c

Signature · 1 line

2.17.0

Signature · 3 lines

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