[PATCH 1/3] drm: qxl: Let DRM core handle connector registering

[PATCH 1/3] drm: qxl: Let DRM core handle connector registering

[Gabriel Krisman Bertazi]

Registering the connector explicitly right after creation is not
necessary for modesetting drivers, because drm_dev_register already takes
care of this on the core side, by calling drm_modeset_register_all.

In addition, performing the initialization too early will get in the way
of the load() hook removal, because the connector interface cannot be
published prior to registering the minors.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
CC: Dave Airlie <airlied@redhat.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
CC: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/qxl/qxl_display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 659c77742649..416ade8566b7 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1077,7 +1077,6 @@ static int qdev_output_init(struct drm_device *dev, int num_output)
 				   dev->mode_config.suggested_x_property, 0);
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.suggested_y_property, 0);
-	drm_connector_register(connector);
 	return 0;
 }
 
-- 
2.11.0

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

[PATCH 2/3] drm: qxl: Open code probing sequence for qxl

[Gabriel Krisman Bertazi]

This avoids using the deprecated drm_get_pci_dev() and load() hook
interfaces in the qxl driver.

The only tricky part is to ensure TTM debugfs initialization
happens after the debugfs root node is created, which is done by moving
that code into the debufs_init() hook.  Since the hook is called 3 times
for each minor function, we make sure it is only executed for the
primary minor.

Tested on qemu with igt and running a WM on top of X.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
CC: Dave Airlie <airlied@redhat.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
CC: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/qxl/qxl_debugfs.c | 12 ++++++++
 drivers/gpu/drm/qxl/qxl_drv.c     | 58 +++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/qxl/qxl_drv.h     |  7 ++++-
 drivers/gpu/drm/qxl/qxl_kms.c     | 40 ++-------------------------
 drivers/gpu/drm/qxl/qxl_ttm.c     |  8 +-----
 5 files changed, 77 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_debugfs.c b/drivers/gpu/drm/qxl/qxl_debugfs.c
index 241af9131dc8..417b538b3ed8 100644
--- a/drivers/gpu/drm/qxl/qxl_debugfs.c
+++ b/drivers/gpu/drm/qxl/qxl_debugfs.c
@@ -84,8 +84,20 @@ int
 qxl_debugfs_init(struct drm_minor *minor)
 {
 #if defined(CONFIG_DEBUG_FS)
+	int r;
+	struct qxl_device *dev =
+		(struct qxl_device *) minor->dev->dev_private;
+
 	drm_debugfs_create_files(qxl_debugfs_list, QXL_DEBUGFS_ENTRIES,
 				 minor->debugfs_root, minor);
+
+	if (minor->type == DRM_MINOR_PRIMARY) {
+		r = qxl_ttm_debugfs_init(dev);
+		if (r) {
+			DRM_ERROR("Failed to init TTM debugfs\n");
+			return r;
+		}
+	}
 #endif
 	return 0;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 460bbceae297..7cc29027a612 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -62,12 +62,67 @@ static struct pci_driver qxl_pci_driver;
 static int
 qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct drm_device *drm;
+	struct qxl_device *qdev;
+	int ret;
+
 	if (pdev->revision < 4) {
 		DRM_ERROR("qxl too old, doesn't support client_monitors_config,"
 			  " use xf86-video-qxl in user mode");
 		return -EINVAL; /* TODO: ENODEV ? */
 	}
-	return drm_get_pci_dev(pdev, ent, &qxl_driver);
+
+	drm = drm_dev_alloc(&qxl_driver, &pdev->dev);
+	if (IS_ERR(drm))
+		return -ENOMEM;
+
+	qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
+	if (!qdev) {
+		ret = -ENOMEM;
+		goto free_drm_device;
+	}
+
+	ret = pci_enable_device(pdev);
+	if (ret)
+		goto free_drm_device;
+
+	drm->pdev = pdev;
+	pci_set_drvdata(pdev, drm);
+	drm->dev_private = qdev;
+
+	ret = qxl_device_init(qdev, drm, pdev, ent->driver_data);
+	if (ret)
+		goto disable_pci;
+
+	ret = drm_vblank_init(drm, 1);
+	if (ret)
+		goto unload;
+
+	ret = qxl_modeset_init(qdev);
+	if (ret)
+		goto vblank_cleanup;
+
+	drm_kms_helper_poll_init(qdev->ddev);
+
+	/* Complete initialization. */
+	ret = drm_dev_register(drm, ent->driver_data);
+	if (ret)
+		goto modeset_cleanup;
+
+	return 0;
+
+modeset_cleanup:
+	qxl_modeset_fini(qdev);
+vblank_cleanup:
+	drm_vblank_cleanup(drm);
+unload:
+	qxl_device_fini(qdev);
+disable_pci:
+	pci_disable_device(pdev);
+free_drm_device:
+	kfree(qdev);
+	kfree(drm);
+	return ret;
 }
 
 static void
@@ -230,7 +285,6 @@ static struct pci_driver qxl_pci_driver = {
 static struct drm_driver qxl_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
 			   DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
-	.load = qxl_driver_load,
 	.unload = qxl_driver_unload,
 	.get_vblank_counter = qxl_noop_get_vblank_counter,
 	.enable_vblank = qxl_noop_enable_vblank,
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 883d8639c04e..d0aed9ed12d2 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -336,7 +336,10 @@ __printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
 
-int qxl_driver_load(struct drm_device *dev, unsigned long flags);
+int qxl_device_init(struct qxl_device *qdev, struct drm_device *ddev,
+		    struct pci_dev *pdev,  unsigned long flags);
+void qxl_device_fini(struct qxl_device *qdev);
+
 void qxl_driver_unload(struct drm_device *dev);
 
 int qxl_modeset_init(struct qxl_device *qdev);
@@ -345,6 +348,8 @@ void qxl_modeset_fini(struct qxl_device *qdev);
 int qxl_bo_init(struct qxl_device *qdev);
 void qxl_bo_fini(struct qxl_device *qdev);
 
+int qxl_ttm_debugfs_init(struct qxl_device *qdev);
+
 void qxl_reinit_memslots(struct qxl_device *qdev);
 int qxl_surf_evict(struct qxl_device *qdev);
 int qxl_vram_evict(struct qxl_device *qdev);
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index b2491407b616..6848057d0917 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -115,7 +115,7 @@ static void qxl_gc_work(struct work_struct *work)
 	qxl_garbage_collect(qdev);
 }
 
-static int qxl_device_init(struct qxl_device *qdev,
+int qxl_device_init(struct qxl_device *qdev,
 		    struct drm_device *ddev,
 		    struct pci_dev *pdev,
 		    unsigned long flags)
@@ -263,7 +263,7 @@ static int qxl_device_init(struct qxl_device *qdev,
 	return 0;
 }
 
-static void qxl_device_fini(struct qxl_device *qdev)
+void qxl_device_fini(struct qxl_device *qdev)
 {
 	if (qdev->current_release_bo[0])
 		qxl_bo_unref(&qdev->current_release_bo[0]);
@@ -300,39 +300,3 @@ void qxl_driver_unload(struct drm_device *dev)
 	kfree(qdev);
 	dev->dev_private = NULL;
 }
-
-int qxl_driver_load(struct drm_device *dev, unsigned long flags)
-{
-	struct qxl_device *qdev;
-	int r;
-
-	qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
-	if (qdev == NULL)
-		return -ENOMEM;
-
-	dev->dev_private = qdev;
-
-	r = qxl_device_init(qdev, dev, dev->pdev, flags);
-	if (r)
-		goto out;
-
-	r = drm_vblank_init(dev, 1);
-	if (r)
-		goto unload;
-
-	r = qxl_modeset_init(qdev);
-	if (r)
-		goto unload;
-
-	drm_kms_helper_poll_init(qdev->ddev);
-
-	return 0;
-unload:
-	qxl_driver_unload(dev);
-
-out:
-	kfree(qdev);
-	return r;
-}
-
-
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 1b096c5252ad..bc1c896bc5e1 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -35,7 +35,6 @@
 #include "qxl_object.h"
 
 #include <linux/delay.h>
-static int qxl_ttm_debugfs_init(struct qxl_device *qdev);
 
 static struct qxl_device *qxl_get_qdev(struct ttm_bo_device *bdev)
 {
@@ -436,11 +435,6 @@ int qxl_ttm_init(struct qxl_device *qdev)
 		 ((unsigned)num_io_pages * PAGE_SIZE) / (1024 * 1024));
 	DRM_INFO("qxl: %uM of Surface memory size\n",
 		 (unsigned)qdev->surfaceram_size / (1024 * 1024));
-	r = qxl_ttm_debugfs_init(qdev);
-	if (r) {
-		DRM_ERROR("Failed to init debugfs\n");
-		return r;
-	}
 	return 0;
 }
 
@@ -473,7 +467,7 @@ static int qxl_mm_dump_table(struct seq_file *m, void *data)
 }
 #endif
 
-static int qxl_ttm_debugfs_init(struct qxl_device *qdev)
+int qxl_ttm_debugfs_init(struct qxl_device *qdev)
 {
 #if defined(CONFIG_DEBUG_FS)
 	static struct drm_info_list qxl_mem_types_list[QXL_DEBUGFS_MEM_TYPES];
-- 
2.11.0

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

[PATCH 3/3] drm: qxl: Open code teardown function for qxl

[Gabriel Krisman Bertazi]

This avoids using the deprecated drm_put_dev() and unload() hook
interfaces in the qxl driver.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
CC: Dave Airlie <airlied@redhat.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
CC: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/qxl/qxl_drv.c | 11 +++++++++--
 drivers/gpu/drm/qxl/qxl_drv.h |  2 --
 drivers/gpu/drm/qxl/qxl_kms.c | 16 ----------------
 3 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 7cc29027a612..6e0f8a2d8ac9 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -129,8 +129,16 @@ static void
 qxl_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct qxl_device *qdev = dev->dev_private;
+
+	drm_dev_unregister(dev);
+
+	qxl_modeset_fini(qdev);
+	qxl_device_fini(qdev);
 
-	drm_put_dev(dev);
+	dev->dev_private = NULL;
+	kfree(qdev);
+	drm_dev_unref(dev);
 }
 
 static const struct file_operations qxl_fops = {
@@ -285,7 +293,6 @@ static struct pci_driver qxl_pci_driver = {
 static struct drm_driver qxl_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
 			   DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
-	.unload = qxl_driver_unload,
 	.get_vblank_counter = qxl_noop_get_vblank_counter,
 	.enable_vblank = qxl_noop_enable_vblank,
 	.disable_vblank = qxl_noop_disable_vblank,
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index d0aed9ed12d2..458501534313 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -340,8 +340,6 @@ int qxl_device_init(struct qxl_device *qdev, struct drm_device *ddev,
 		    struct pci_dev *pdev,  unsigned long flags);
 void qxl_device_fini(struct qxl_device *qdev);
 
-void qxl_driver_unload(struct drm_device *dev);
-
 int qxl_modeset_init(struct qxl_device *qdev);
 void qxl_modeset_fini(struct qxl_device *qdev);
 
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 6848057d0917..d0666f5dccd6 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -284,19 +284,3 @@ void qxl_device_fini(struct qxl_device *qdev)
 	qdev->mode_info.num_modes = 0;
 	qxl_debugfs_remove_files(qdev);
 }
-
-void qxl_driver_unload(struct drm_device *dev)
-{
-	struct qxl_device *qdev = dev->dev_private;
-
-	if (qdev == NULL)
-		return;
-
-	drm_vblank_cleanup(dev);
-
-	qxl_modeset_fini(qdev);
-	qxl_device_fini(qdev);
-
-	kfree(qdev);
-	dev->dev_private = NULL;
-}
-- 
2.11.0

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

Re: [PATCH 2/3] drm: qxl: Open code probing sequence for qxl

[Gustavo Padovan]

Hi Gabriel,

2017-01-12 Gabriel Krisman Bertazi <krisman@collabora.co.uk>:

> This avoids using the deprecated drm_get_pci_dev() and load() hook
> interfaces in the qxl driver.
> 
> The only tricky part is to ensure TTM debugfs initialization
> happens after the debugfs root node is created, which is done by moving
> that code into the debufs_init() hook.  Since the hook is called 3 times
> for each minor function, we make sure it is only executed for the
> primary minor.
> 
> Tested on qemu with igt and running a WM on top of X.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> CC: Dave Airlie <airlied@redhat.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> CC: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/qxl/qxl_debugfs.c | 12 ++++++++
>  drivers/gpu/drm/qxl/qxl_drv.c     | 58 +++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/qxl/qxl_drv.h     |  7 ++++-
>  drivers/gpu/drm/qxl/qxl_kms.c     | 40 ++-------------------------
>  drivers/gpu/drm/qxl/qxl_ttm.c     |  8 +-----
>  5 files changed, 77 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_debugfs.c b/drivers/gpu/drm/qxl/qxl_debugfs.c
> index 241af9131dc8..417b538b3ed8 100644
> --- a/drivers/gpu/drm/qxl/qxl_debugfs.c
> +++ b/drivers/gpu/drm/qxl/qxl_debugfs.c
> @@ -84,8 +84,20 @@ int
>  qxl_debugfs_init(struct drm_minor *minor)
>  {
>  #if defined(CONFIG_DEBUG_FS)
> +	int r;
> +	struct qxl_device *dev =
> +		(struct qxl_device *) minor->dev->dev_private;
> +
>  	drm_debugfs_create_files(qxl_debugfs_list, QXL_DEBUGFS_ENTRIES,
>  				 minor->debugfs_root, minor);
> +
> +	if (minor->type == DRM_MINOR_PRIMARY) {

I'm not confortable with exposing minor->type here and making qxl
the first driver to use it outside of drm core. Don't we have any other
way. 

I see that inside qxl_debugfs_add_files() we have a check for already
registered files. Wouldn't that or some modification around work for
this?

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

Re: [PATCH 1/3] drm: qxl: Let DRM core handle connector registering

[Gustavo Padovan]

Hi Gabriel,

2017-01-12 Gabriel Krisman Bertazi <krisman@collabora.co.uk>:

> Registering the connector explicitly right after creation is not
> necessary for modesetting drivers, because drm_dev_register already takes
> care of this on the core side, by calling drm_modeset_register_all.
> 
> In addition, performing the initialization too early will get in the way
> of the load() hook removal, because the connector interface cannot be
> published prior to registering the minors.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> CC: Dave Airlie <airlied@redhat.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> CC: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 1 -
>  1 file changed, 1 deletion(-)

Applied to drm-misc-next. Thanks for the patch.

Gustavo

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

Re: [PATCH 2/3] drm: qxl: Open code probing sequence for qxl

[Gabriel Krisman Bertazi]

Gustavo Padovan <gustavo@padovan.org> writes:

> I'm not confortable with exposing minor->type here and making qxl
> the first driver to use it outside of drm core. Don't we have any other
> way. 
>
> I see that inside qxl_debugfs_add_files() we have a check for already
> registered files. Wouldn't that or some modification around work for
> this?

Thanks for your review, Gustavo.

As I mentioned on IRC, I verified the check is not needed at all,
since qxl only implements one minor.  I'll resend without this bit.

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