[PATCH 01/11] drm: Move master pointer from drm_minor to drm_device

[PATCH 01/11] drm: Move master pointer from drm_minor to drm_device

[Daniel Vetter]

There can only be one current master, and it's for the overall device.
Render/control minors don't support master-based auth at all.

This simplifies the master logic a lot, at least in my eyes: All these
additional pointer chases are just confusing.

While doing the conversion I spotted some locking fail:
- drm_lock/drm_auth check dev->master without holding the
  master_mutex. This is fallout from

  commit c996fd0b956450563454e7ccc97a82ca31f9d043
  Author: Thomas Hellstrom <thellstrom@vmware.com>
  Date:   Tue Feb 25 19:57:44 2014 +0100

      drm: Protect the master management with a drm_device::master_mutex v3

  but I honestly don't care one bit about those old legacy drivers
  using this.

- debugfs name info should just grab master_mutex.

- And the fbdev helper looked at it to figure out whether someone is
  using KMS. We just need a consistent value, so READ_ONCE. Aside: We
  should probably check if anyone has opened a control node too, but I
  guess current userspace doesn't really do that yet.

v2: Balance locking, reported by Julia.

v3: Rebase on top of Chris' oops fixes.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> (v2)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c      | 26 +++++++++++++-------------
 drivers/gpu/drm/drm_bufs.c      |  8 ++++----
 drivers/gpu/drm/drm_fb_helper.c |  2 +-
 drivers/gpu/drm/drm_info.c      |  9 ++++++++-
 drivers/gpu/drm/drm_lock.c      |  2 +-
 drivers/gpu/drm/sis/sis_mm.c    |  2 +-
 drivers/gpu/drm/via/via_mm.c    |  2 +-
 include/drm/drmP.h              |  9 +++++----
 8 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index d858e69d337b..6bba6b9e9dcc 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 	lockdep_assert_held_once(&dev->master_mutex);
 
 	/* create a new master */
-	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
-	if (!fpriv->minor->master)
+	dev->master = drm_master_create(dev);
+	if (!dev->master)
 		return -ENOMEM;
 
 	/* take another reference for the copy in the local file priv */
 	old_master = fpriv->master;
-	fpriv->master = drm_master_get(fpriv->minor->master);
+	fpriv->master = drm_master_get(dev->master);
 
 	if (dev->driver->master_create) {
 		ret = dev->driver->master_create(dev, fpriv->master);
@@ -157,7 +157,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 
 out_err:
 	/* drop both references and restore old master on failure */
-	drm_master_put(&fpriv->minor->master);
+	drm_master_put(&dev->master);
 	drm_master_put(&fpriv->master);
 	fpriv->master = old_master;
 
@@ -173,7 +173,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 	if (file_priv->is_master)
 		goto out_unlock;
 
-	if (file_priv->minor->master) {
+	if (dev->master) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -188,13 +188,13 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	file_priv->minor->master = drm_master_get(file_priv->master);
+	dev->master = drm_master_get(file_priv->master);
 	file_priv->is_master = 1;
 	if (dev->driver->master_set) {
 		ret = dev->driver->master_set(dev, file_priv, false);
 		if (unlikely(ret != 0)) {
 			file_priv->is_master = 0;
-			drm_master_put(&file_priv->minor->master);
+			drm_master_put(&dev->master);
 		}
 	}
 
@@ -212,13 +212,13 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	if (!file_priv->is_master)
 		goto out_unlock;
 
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		goto out_unlock;
 
 	ret = 0;
 	if (dev->driver->master_drop)
 		dev->driver->master_drop(dev, file_priv, false);
-	drm_master_put(&file_priv->minor->master);
+	drm_master_put(&dev->master);
 	file_priv->is_master = 0;
 
 out_unlock:
@@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
 	/* if there is no current master make this fd it, but do not create
 	 * any master object for render clients */
 	mutex_lock(&dev->master_mutex);
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		ret = drm_new_set_master(dev, file_priv);
 	else
-		file_priv->master = drm_master_get(file_priv->minor->master);
+		file_priv->master = drm_master_get(dev->master);
 	mutex_unlock(&dev->master_mutex);
 
 	return ret;
@@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (file_priv->minor->master == file_priv->master) {
+	if (dev->master == file_priv->master) {
 		/* drop the reference held my the minor */
 		if (dev->driver->master_drop)
 			dev->driver->master_drop(dev, file_priv, true);
-		drm_master_put(&file_priv->minor->master);
+		drm_master_put(&dev->master);
 	}
 out:
 	/* drop the master reference held by the file priv */
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 9b34158c0f77..c3a12cd8bd0d 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -51,7 +51,7 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev,
 		 */
 		if (!entry->map ||
 		    map->type != entry->map->type ||
-		    entry->master != dev->primary->master)
+		    entry->master != dev->master)
 			continue;
 		switch (map->type) {
 		case _DRM_SHM:
@@ -245,12 +245,12 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 		map->offset = (unsigned long)map->handle;
 		if (map->flags & _DRM_CONTAINS_LOCK) {
 			/* Prevent a 2nd X Server from creating a 2nd lock */
-			if (dev->primary->master->lock.hw_lock != NULL) {
+			if (dev->master->lock.hw_lock != NULL) {
 				vfree(map->handle);
 				kfree(map);
 				return -EBUSY;
 			}
-			dev->sigdata.lock = dev->primary->master->lock.hw_lock = map->handle;	/* Pointer to lock */
+			dev->sigdata.lock = dev->master->lock.hw_lock = map->handle;	/* Pointer to lock */
 		}
 		break;
 	case _DRM_AGP: {
@@ -356,7 +356,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 	mutex_unlock(&dev->struct_mutex);
 
 	if (!(map->flags & _DRM_DRIVER))
-		list->master = dev->primary->master;
+		list->master = dev->master;
 	*maplist = list;
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0bac5246e5a7..0a06f9120b5a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 
 	/* Sometimes user space wants everything disabled, so don't steal the
 	 * display if there's a master. */
-	if (dev->primary->master)
+	if (READ_ONCE(dev->master))
 		return false;
 
 	drm_for_each_crtc(crtc, dev) {
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index e2d2543d5bd0..d6cfd5340d52 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -50,7 +50,12 @@ int drm_name_info(struct seq_file *m, void *data)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_minor *minor = node->minor;
 	struct drm_device *dev = minor->dev;
-	struct drm_master *master = minor->master;
+	struct drm_master *master;
+
+	mutex_lock(&dev->master_mutex);
+	master = dev->master;
+	if (!master)
+		goto out_unlock;
 
 	seq_printf(m, "%s", dev->driver->name);
 	if (dev->dev)
@@ -60,6 +65,8 @@ int drm_name_info(struct seq_file *m, void *data)
 	if (dev->unique)
 		seq_printf(m, " unique=%s", dev->unique);
 	seq_printf(m, "\n");
+out_unlock:
+	mutex_unlock(&dev->master_mutex);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 0fb8f9e73486..ae0a4d39deec 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -334,7 +334,7 @@ void drm_legacy_lock_release(struct drm_device *dev, struct file *filp)
 	struct drm_file *file_priv = filp->private_data;
 
 	/* if the master has gone away we can't do anything with the lock */
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		return;
 
 	if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
diff --git a/drivers/gpu/drm/sis/sis_mm.c b/drivers/gpu/drm/sis/sis_mm.c
index 93ad8a5704d1..03defda77766 100644
--- a/drivers/gpu/drm/sis/sis_mm.c
+++ b/drivers/gpu/drm/sis/sis_mm.c
@@ -316,7 +316,7 @@ void sis_reclaim_buffers_locked(struct drm_device *dev,
 	struct sis_file_private *file_priv = file->driver_priv;
 	struct sis_memblock *entry, *next;
 
-	if (!(file->minor->master && file->master->lock.hw_lock))
+	if (!(dev->master && file->master->lock.hw_lock))
 		return;
 
 	drm_legacy_idlelock_take(&file->master->lock);
diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c
index 4f20742e7788..a04ef1c992d9 100644
--- a/drivers/gpu/drm/via/via_mm.c
+++ b/drivers/gpu/drm/via/via_mm.c
@@ -208,7 +208,7 @@ void via_reclaim_buffers_locked(struct drm_device *dev,
 	struct via_file_private *file_priv = file->driver_priv;
 	struct via_memblock *entry, *next;
 
-	if (!(file->minor->master && file->master->lock.hw_lock))
+	if (!(dev->master && file->master->lock.hw_lock))
 		return;
 
 	drm_legacy_idlelock_take(&file->master->lock);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3b183c36801e..88f7b7e055f1 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -336,7 +336,7 @@ struct drm_file {
 	void *driver_priv;
 
 	struct drm_master *master; /* master this node is currently associated with
-				      N.B. not always minor->master */
+				      N.B. not always dev->master */
 	/**
 	 * fbs - List of framebuffers associated with this file.
 	 *
@@ -708,9 +708,6 @@ struct drm_minor {
 
 	struct list_head debugfs_list;
 	struct mutex debugfs_lock; /* Protects debugfs_list. */
-
-	/* currently active master for this node. Protected by master_mutex */
-	struct drm_master *master;
 };
 
 
@@ -759,6 +756,10 @@ struct drm_device {
 	struct drm_minor *control;		/**< Control node */
 	struct drm_minor *primary;		/**< Primary node */
 	struct drm_minor *render;		/**< Render node */
+
+	/* currently active master for this device. Protected by master_mutex */
+	struct drm_master *master;
+
 	atomic_t unplugged;			/**< Flag whether dev is dead */
 	struct inode *anon_inode;		/**< inode for private address-space */
 	char *unique;				/**< unique name of the device */
-- 
2.8.1

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

[PATCH 02/11] drm: Clean up drm_crtc.h

[Daniel Vetter]

- Group declarations for separate files (drm_bridge.c, drm_edid.c)
- Move declarations only used within drm.ko to drm_crtc_internal.h
- drm_property_type_valid to drm_crtc.c, its only callsite

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c          |   7 ++
 drivers/gpu/drm/drm_crtc_internal.h |  86 ++++++++++++++++-
 drivers/gpu/drm/drm_drv.c           |   1 +
 drivers/gpu/drm/drm_fops.c          |   1 +
 include/drm/drm_crtc.h              | 188 +++++++++++-------------------------
 5 files changed, 150 insertions(+), 133 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7c862bd2f19..9fa9b04cd5a9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3685,6 +3685,13 @@ void drm_fb_release(struct drm_file *priv)
 	}
 }
 
+static bool drm_property_type_valid(struct drm_property *property)
+{
+	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
+		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
+	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
+}
+
 /**
  * drm_property_create - create a new property type
  * @dev: drm device
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index a78c138282ea..8186c0e05c42 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -31,14 +31,98 @@
  * and are not exported to drivers.
  */
 
+
+/* drm_crtc.c */
+void drm_connector_ida_init(void);
+void drm_connector_ida_destroy(void);
 int drm_mode_object_get(struct drm_device *dev,
 			struct drm_mode_object *obj, uint32_t obj_type);
 void drm_mode_object_unregister(struct drm_device *dev,
 				struct drm_mode_object *object);
+bool drm_property_change_valid_get(struct drm_property *property,
+				   uint64_t value,
+				   struct drm_mode_object **ref);
+void drm_property_change_valid_put(struct drm_property *property,
+				   struct drm_mode_object *ref);
+
+int drm_plane_check_pixel_format(const struct drm_plane *plane,
+				 u32 format);
+int drm_crtc_check_viewport(const struct drm_crtc *crtc,
+			    int x, int y,
+			    const struct drm_display_mode *mode,
+			    const struct drm_framebuffer *fb);
+
+void drm_fb_release(struct drm_file *file_priv);
+void drm_property_destroy_user_blobs(struct drm_device *dev,
+				     struct drm_file *file_priv);
+
+/* dumb buffer support IOCTLs */
+int drm_mode_create_dumb_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
+				void *data, struct drm_file *file_priv);
+
+/* framebuffer IOCTLs */
+extern int drm_mode_addfb(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+extern int drm_mode_addfb2(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_rmfb(struct drm_device *dev,
+			 void *data, struct drm_file *file_priv);
+int drm_mode_getfb(struct drm_device *dev,
+		   void *data, struct drm_file *file_priv);
+int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+
+/* IOCTLs */
+int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
+				      struct drm_file *file_priv);
+int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
+				    struct drm_file *file_priv);
+
+int drm_mode_getresources(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_getplane_res(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv);
+int drm_mode_getcrtc(struct drm_device *dev,
+		     void *data, struct drm_file *file_priv);
+int drm_mode_getconnector(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_setcrtc(struct drm_device *dev,
+		     void *data, struct drm_file *file_priv);
+int drm_mode_getplane(struct drm_device *dev,
+		      void *data, struct drm_file *file_priv);
+int drm_mode_setplane(struct drm_device *dev,
+		      void *data, struct drm_file *file_priv);
+int drm_mode_cursor_ioctl(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_cursor2_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_getproperty_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_getblob_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_createblob_ioctl(struct drm_device *dev,
+			      void *data, struct drm_file *file_priv);
+int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
+					  void *data, struct drm_file *file_priv);
+int drm_mode_getencoder(struct drm_device *dev,
+			void *data, struct drm_file *file_priv);
+int drm_mode_gamma_get_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+int drm_mode_gamma_set_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+
+int drm_mode_page_flip_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
 
 /* drm_atomic.c */
 int drm_atomic_get_property(struct drm_mode_object *obj,
-			   struct drm_property *property, uint64_t *val);
+			    struct drm_property *property, uint64_t *val);
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c7101c06b02e..61d0c0c1ce6d 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -36,6 +36,7 @@
 #include <drm/drm_core.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 /*
  * drm_debug: Enable debug output.
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index f6dfdfcd018b..323c238fcac7 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -40,6 +40,7 @@
 #include <linux/module.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 /* from BKL pushdown */
 DEFINE_MUTEX(drm_global_mutex);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c2734979f164..efef0614703a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -44,6 +44,7 @@ struct drm_file;
 struct drm_clip_rect;
 struct device_node;
 struct fence;
+struct edid;
 
 struct drm_mode_object {
 	uint32_t id;
@@ -2496,12 +2497,10 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
 	return 1 << drm_crtc_index(crtc);
 }
 
-extern void drm_connector_ida_init(void);
-extern void drm_connector_ida_destroy(void);
-extern int drm_connector_init(struct drm_device *dev,
-			      struct drm_connector *connector,
-			      const struct drm_connector_funcs *funcs,
-			      int connector_type);
+int drm_connector_init(struct drm_device *dev,
+		       struct drm_connector *connector,
+		       const struct drm_connector_funcs *funcs,
+		       int connector_type);
 int drm_connector_register(struct drm_connector *connector);
 void drm_connector_unregister(struct drm_connector *connector);
 
@@ -2515,22 +2514,6 @@ static inline unsigned drm_connector_index(struct drm_connector *connector)
 extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
 
-extern int drm_bridge_add(struct drm_bridge *bridge);
-extern void drm_bridge_remove(struct drm_bridge *bridge);
-extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
-extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
-
-bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
-			const struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
-void drm_bridge_disable(struct drm_bridge *bridge);
-void drm_bridge_post_disable(struct drm_bridge *bridge);
-void drm_bridge_mode_set(struct drm_bridge *bridge,
-			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
-void drm_bridge_pre_enable(struct drm_bridge *bridge);
-void drm_bridge_enable(struct drm_bridge *bridge);
-
 extern __printf(5, 6)
 int drm_encoder_init(struct drm_device *dev,
 		     struct drm_encoder *encoder,
@@ -2592,14 +2575,8 @@ static inline unsigned int drm_plane_index(struct drm_plane *plane)
 }
 extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx);
 extern void drm_plane_force_disable(struct drm_plane *plane);
-extern int drm_plane_check_pixel_format(const struct drm_plane *plane,
-					u32 format);
 extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 				   int *hdisplay, int *vdisplay);
-extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
-				   int x, int y,
-				   const struct drm_display_mode *mode,
-				   const struct drm_framebuffer *fb);
 
 extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
@@ -2610,16 +2587,6 @@ extern const char *drm_get_dvi_i_subconnector_name(int val);
 extern const char *drm_get_dvi_i_select_name(int val);
 extern const char *drm_get_tv_subconnector_name(int val);
 extern const char *drm_get_tv_select_name(int val);
-extern void drm_fb_release(struct drm_file *file_priv);
-extern void drm_property_destroy_user_blobs(struct drm_device *dev,
-                                            struct drm_file *file_priv);
-extern bool drm_probe_ddc(struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
-					    struct i2c_adapter *adapter);
-extern struct edid *drm_edid_duplicate(const struct edid *edid);
-extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_config_init(struct drm_device *dev);
 extern void drm_mode_config_reset(struct drm_device *dev);
 extern void drm_mode_config_cleanup(struct drm_device *dev);
@@ -2643,13 +2610,6 @@ static inline bool drm_property_type_is(struct drm_property *property,
 	return property->flags & type;
 }
 
-static inline bool drm_property_type_valid(struct drm_property *property)
-{
-	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
-		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
-	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
-}
-
 extern int drm_object_property_set_value(struct drm_mode_object *obj,
 					 struct drm_property *property,
 					 uint64_t val);
@@ -2707,86 +2667,15 @@ extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
 extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
-extern bool drm_property_change_valid_get(struct drm_property *property,
-					 uint64_t value, struct drm_mode_object **ref);
-extern void drm_property_change_valid_put(struct drm_property *property,
-		struct drm_mode_object *ref);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
 extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
-extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
-		uint32_t id, uint32_t type);
-void drm_mode_object_reference(struct drm_mode_object *obj);
-void drm_mode_object_unreference(struct drm_mode_object *obj);
 
-/* IOCTLs */
-extern int drm_mode_getresources(struct drm_device *dev,
-				 void *data, struct drm_file *file_priv);
-extern int drm_mode_getplane_res(struct drm_device *dev, void *data,
-				   struct drm_file *file_priv);
-extern int drm_mode_getcrtc(struct drm_device *dev,
-			    void *data, struct drm_file *file_priv);
-extern int drm_mode_getconnector(struct drm_device *dev,
-			      void *data, struct drm_file *file_priv);
 extern int drm_mode_set_config_internal(struct drm_mode_set *set);
-extern int drm_mode_setcrtc(struct drm_device *dev,
-			    void *data, struct drm_file *file_priv);
-extern int drm_mode_getplane(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_setplane(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_cursor_ioctl(struct drm_device *dev,
-				void *data, struct drm_file *file_priv);
-extern int drm_mode_cursor2_ioctl(struct drm_device *dev,
-				void *data, struct drm_file *file_priv);
-extern int drm_mode_addfb(struct drm_device *dev,
-			  void *data, struct drm_file *file_priv);
-extern int drm_mode_addfb2(struct drm_device *dev,
-			   void *data, struct drm_file *file_priv);
+
 extern uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
-extern int drm_mode_rmfb(struct drm_device *dev,
-			 void *data, struct drm_file *file_priv);
-extern int drm_mode_getfb(struct drm_device *dev,
-			  void *data, struct drm_file *file_priv);
-extern int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
-				  void *data, struct drm_file *file_priv);
-
-extern int drm_mode_getproperty_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_getblob_ioctl(struct drm_device *dev,
-				  void *data, struct drm_file *file_priv);
-extern int drm_mode_createblob_ioctl(struct drm_device *dev,
-				     void *data, struct drm_file *file_priv);
-extern int drm_mode_destroyblob_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
-					      void *data, struct drm_file *file_priv);
-extern int drm_mode_getencoder(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
-extern enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
-extern bool drm_detect_hdmi_monitor(struct edid *edid);
-extern bool drm_detect_monitor_audio(struct edid *edid);
-extern bool drm_rgb_quant_range_selectable(struct edid *edid);
-extern int drm_mode_page_flip_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_add_modes_noedid(struct drm_connector *connector,
-				int hdisplay, int vdisplay);
-extern void drm_set_preferred_mode(struct drm_connector *connector,
-				   int hpref, int vpref);
-
-extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
-				 bool *edid_corrupt);
-extern bool drm_edid_is_valid(struct edid *edid);
-extern void drm_edid_get_monitor_name(struct edid *edid, char *name,
-				      int buflen);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
 							 char topology[8]);
@@ -2794,25 +2683,10 @@ extern struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
 					       char topology[8]);
 extern void drm_mode_put_tile_group(struct drm_device *dev,
 				   struct drm_tile_group *tg);
-struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
-					   int hsize, int vsize, int fresh,
-					   bool rb);
 
-extern int drm_mode_create_dumb_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
-					     struct drm_file *file_priv);
-extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
-					   struct drm_file *file_priv);
 extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 				       struct drm_property *property,
 				       uint64_t value);
-extern int drm_mode_atomic_ioctl(struct drm_device *dev,
-				 void *data, struct drm_file *file_priv);
 
 extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 							      unsigned int supported_rotations);
@@ -2823,6 +2697,10 @@ extern void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				       bool has_ctm,
 				       uint gamma_lut_size);
 /* Helpers */
+struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
+					     uint32_t id, uint32_t type);
+void drm_mode_object_reference(struct drm_mode_object *obj);
+void drm_mode_object_unreference(struct drm_mode_object *obj);
 
 static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 		uint32_t id)
@@ -2988,4 +2866,50 @@ assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
 	     &fb->head != (&(dev)->mode_config.fb_list);			\
 	     fb = list_next_entry(fb, head))
 
+/* drm_edid.c */
+bool drm_probe_ddc(struct i2c_adapter *adapter);
+struct edid *drm_get_edid(struct drm_connector *connector,
+			  struct i2c_adapter *adapter);
+struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+				     struct i2c_adapter *adapter);
+struct edid *drm_edid_duplicate(const struct edid *edid);
+int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
+
+u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
+enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
+bool drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_monitor_audio(struct edid *edid);
+bool drm_rgb_quant_range_selectable(struct edid *edid);
+int drm_add_modes_noedid(struct drm_connector *connector,
+			 int hdisplay, int vdisplay);
+void drm_set_preferred_mode(struct drm_connector *connector,
+			    int hpref, int vpref);
+
+int drm_edid_header_is_valid(const u8 *raw_edid);
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+			  bool *edid_corrupt);
+bool drm_edid_is_valid(struct edid *edid);
+void drm_edid_get_monitor_name(struct edid *edid, char *name,
+			       int buflen);
+struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
+					   int hsize, int vsize, int fresh,
+					   bool rb);
+
+/* drm_bridge.c */
+extern int drm_bridge_add(struct drm_bridge *bridge);
+extern void drm_bridge_remove(struct drm_bridge *bridge);
+extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
+extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
+
+bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
+			const struct drm_display_mode *mode,
+			struct drm_display_mode *adjusted_mode);
+void drm_bridge_disable(struct drm_bridge *bridge);
+void drm_bridge_post_disable(struct drm_bridge *bridge);
+void drm_bridge_mode_set(struct drm_bridge *bridge,
+			struct drm_display_mode *mode,
+			struct drm_display_mode *adjusted_mode);
+void drm_bridge_pre_enable(struct drm_bridge *bridge);
+void drm_bridge_enable(struct drm_bridge *bridge);
+
 #endif /* __DRM_CRTC_H__ */
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/11] drm: Use dev->name as fallback for dev->unique

[Daniel Vetter]

Lots of arm drivers get this wrong and for most arm boards this is the
right thing actually. And anyway with most loaders you want to chase
sysfs links anyway to figure out which dri device you want.

This will fix dmesg noise for rockchip and sti.

Also add a fallback to driver->name for entirely virtual drivers like
vgem.

v2: Rebase on top of

commit e112e593b215c394c0303dbf0534db0928e87967
Author: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Date:   Fri Dec 11 11:20:28 2015 +0100

    drm: use dev_name as default unique name in drm_dev_alloc()

and simplify a bit. Plus add a comment.

v3: WARN_ON(!dev->unique) as discussed with Emil.

Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Reported-by: Ilia Mirkin <imirkin@alum.mit.edu>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c   | 10 +++++-----
 drivers/gpu/drm/drm_ioctl.c |  7 +------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 61d0c0c1ce6d..063ba6820411 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -524,11 +524,11 @@ int drm_dev_init(struct drm_device *dev,
 		}
 	}
 
-	if (parent) {
-		ret = drm_dev_set_unique(dev, dev_name(parent));
-		if (ret)
-			goto err_setunique;
-	}
+	/* Use the parent device name as DRM device unique identifier, but fall
+	 * back to the driver name for virtual devices like vgem. */
+	ret = drm_dev_set_unique(dev, parent ? dev_name(parent) : driver->name);
+	if (ret)
+		goto err_setunique;
 
 	return 0;
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 11eda9050215..b7f7d968e4cd 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -135,12 +135,7 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
 			return ret;
 		}
 	} else {
-		if (WARN(dev->unique == NULL,
-			 "No drm_driver.set_busid() implementation provided by "
-			 "%ps. Use drm_dev_set_unique() to set the unique "
-			 "name explicitly.", dev->driver))
-			return -EINVAL;
-
+		WARN_ON(!dev->unique);
 		master->unique = kstrdup(dev->unique, GFP_KERNEL);
 		if (master->unique)
 			master->unique_len = strlen(dev->unique);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/11] drm/vgem: Stop calling drm_drv_set_unique

[Daniel Vetter]

With the previous patch this is now redudant, the core always
sets a reasonable dev->unique string.

Cc: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 1b4cc8b27080..35ea5d02a827 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -260,8 +260,6 @@ static int __init vgem_init(void)
 		goto out;
 	}
 
-	drm_dev_set_unique(vgem_device, "vgem");
-
 	ret  = drm_dev_register(vgem_device, 0);
 
 	if (ret)
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/11] drm: Don't call drm_dev_set_unique from platform drivers

[Daniel Vetter]

Since

commit e112e593b215c394c0303dbf0534db0928e87967
Author: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Date:   Fri Dec 11 11:20:28 2015 +0100

    drm: use dev_name as default unique name in drm_dev_alloc()

we're using a reasonable default which should work for everyone. Only
mtk, rcar-du and sun4i are affected, and as kms-only drivers without
any rendering support no one should ever care about the unique name

v2: Rebase on top of mediatek.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c              | 35 +++++++++++-----------------------
 drivers/gpu/drm/mediatek/mtk_drm_drv.c |  2 --
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  2 --
 drivers/gpu/drm/sun4i/sun4i_drv.c      |  4 ----
 include/drm/drmP.h                     |  1 -
 5 files changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 063ba6820411..12aea538d52f 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -298,10 +298,9 @@ void drm_minor_release(struct drm_minor *minor)
  * callbacks implemented by the driver. The driver then needs to initialize all
  * the various subsystems for the drm device like memory management, vblank
  * handling, modesetting support and intial output configuration plus obviously
- * initialize all the corresponding hardware bits. An important part of this is
- * also calling drm_dev_set_unique() to set the userspace-visible unique name of
- * this device instance. Finally when everything is up and running and ready for
- * userspace the device instance can be published using drm_dev_register().
+ * initialize all the corresponding hardware bits. Finally when everything is up
+ * and running and ready for userspace the device instance can be published
+ * using drm_dev_register().
  *
  * There is also deprecated support for initalizing device instances using
  * bus-specific helpers and the ->load() callback. But due to
@@ -323,6 +322,14 @@ void drm_minor_release(struct drm_minor *minor)
  * dev_priv field of &drm_device.
  */
 
+static int drm_dev_set_unique(struct drm_device *dev, const char *name)
+{
+	kfree(dev->unique);
+	dev->unique = kstrdup(name, GFP_KERNEL);
+
+	return dev->unique ? 0 : -ENOMEM;
+}
+
 /**
  * drm_put_dev - Unregister and release a DRM device
  * @dev: DRM device
@@ -741,26 +748,6 @@ void drm_dev_unregister(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_dev_unregister);
 
-/**
- * drm_dev_set_unique - Set the unique name of a DRM device
- * @dev: device of which to set the unique name
- * @name: unique name
- *
- * Sets the unique name of a DRM device using the specified string. Drivers
- * can use this at driver probe time if the unique name of the devices they
- * drive is static.
- *
- * Return: 0 on success or a negative error code on failure.
- */
-int drm_dev_set_unique(struct drm_device *dev, const char *name)
-{
-	kfree(dev->unique);
-	dev->unique = kstrdup(name, GFP_KERNEL);
-
-	return dev->unique ? 0 : -ENOMEM;
-}
-EXPORT_SYMBOL(drm_dev_set_unique);
-
 /*
  * DRM Core
  * The DRM core module initializes all global DRM objects and makes them
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index c33bf98c5d5e..04e901a80234 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -280,8 +280,6 @@ static int mtk_drm_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	drm_dev_set_unique(drm, dev_name(dev));
-
 	drm->dev_private = private;
 	private->drm = drm;
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 48ec4b6e8b26..8784208d8eed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -320,8 +320,6 @@ static int rcar_du_probe(struct platform_device *pdev)
 	if (!ddev)
 		return -ENOMEM;
 
-	drm_dev_set_unique(ddev, dev_name(&pdev->dev));
-
 	rcdu->ddev = ddev;
 	ddev->dev_private = rcdu;
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 68e9d85085fb..5c4b4ad17ad3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -135,10 +135,6 @@ static int sun4i_drv_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(drm, dev_name(drm->dev));
-	if (ret)
-		goto free_drm;
-
 	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
 	if (!drv) {
 		ret = -ENOMEM;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 88f7b7e055f1..dd07dc552f28 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,7 +1076,6 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *name);
 
 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
-- 
2.8.1

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

[PATCH 06/11] drm: Nuke SET_UNIQUE ioctl

[Daniel Vetter]

Ever since

commit 2e1868b560315a8b20d688e646c489a5ad93eeae
Author: Eric Anholt <anholt@freebsd.org>
Date:   Wed Jun 16 09:25:21 2004 +0000

    DRI trunk-20040613 import

the X server supports drm 1.1, thus doesn't call call libdrm's
drmSetBusid - the sole user of this ioctl. When reviewing this note
that for hilarity both the kernel-internal functions (set_busid) and
the libdrm wrapper (drmSetBusid) have names not matching this ioctl
(SET_UNIQUE).

v2: Polish commit message (Emil).

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_internal.h |  3 ---
 drivers/gpu/drm/drm_ioctl.c    | 47 +-------------------------------------
 drivers/gpu/drm/drm_pci.c      | 51 ------------------------------------------
 3 files changed, 1 insertion(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 38401d406532..b86dc9b921a5 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -29,9 +29,6 @@ extern struct mutex drm_global_mutex;
 void drm_lastclose(struct drm_device *dev);
 
 /* drm_pci.c */
-int drm_pci_set_unique(struct drm_device *dev,
-		       struct drm_master *master,
-		       struct drm_unique *u);
 int drm_irq_by_busid(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index b7f7d968e4cd..1fa7619face3 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -75,51 +75,6 @@ drm_unset_busid(struct drm_device *dev,
 	master->unique_len = 0;
 }
 
-/*
- * Set the bus id.
- *
- * \param inode device inode.
- * \param file_priv DRM file private.
- * \param cmd command.
- * \param arg user argument, pointing to a drm_unique structure.
- * \return zero on success or a negative number on failure.
- *
- * Copies the bus id from userspace into drm_device::unique, and verifies that
- * it matches the device this DRM is attached to (EINVAL otherwise).  Deprecated
- * in interface version 1.1 and will return EBUSY when setversion has requested
- * version 1.1 or greater. Also note that KMS is all version 1.1 and later and
- * UMS was only ever supported on pci devices.
- */
-static int drm_setunique(struct drm_device *dev, void *data,
-		  struct drm_file *file_priv)
-{
-	struct drm_unique *u = data;
-	struct drm_master *master = file_priv->master;
-	int ret;
-
-	if (master->unique_len || master->unique)
-		return -EBUSY;
-
-	if (!u->unique_len || u->unique_len > 1024)
-		return -EINVAL;
-
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		return 0;
-
-	if (WARN_ON(!dev->pdev))
-		return -EINVAL;
-
-	ret = drm_pci_set_unique(dev, master, u);
-	if (ret)
-		goto err;
-
-	return 0;
-
-err:
-	drm_unset_busid(dev, master);
-	return ret;
-}
-
 static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
 {
 	struct drm_master *master = file_priv->master;
@@ -508,7 +463,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
 
-	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_setunique, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_AUTH|DRM_UNLOCKED|DRM_MASTER),
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 29d5a548d07a..b2f8f1062d5f 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -144,50 +144,6 @@ int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
 }
 EXPORT_SYMBOL(drm_pci_set_busid);
 
-int drm_pci_set_unique(struct drm_device *dev,
-		       struct drm_master *master,
-		       struct drm_unique *u)
-{
-	int domain, bus, slot, func, ret;
-
-	master->unique_len = u->unique_len;
-	master->unique = kmalloc(master->unique_len + 1, GFP_KERNEL);
-	if (!master->unique) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	if (copy_from_user(master->unique, u->unique, master->unique_len)) {
-		ret = -EFAULT;
-		goto err;
-	}
-
-	master->unique[master->unique_len] = '\0';
-
-	/* Return error if the busid submitted doesn't match the device's actual
-	 * busid.
-	 */
-	ret = sscanf(master->unique, "PCI:%d:%d:%d", &bus, &slot, &func);
-	if (ret != 3) {
-		ret = -EINVAL;
-		goto err;
-	}
-
-	domain = bus >> 8;
-	bus &= 0xff;
-
-	if ((domain != drm_get_pci_domain(dev)) ||
-	    (bus != dev->pdev->bus->number) ||
-	    (slot != PCI_SLOT(dev->pdev->devfn)) ||
-	    (func != PCI_FUNC(dev->pdev->devfn))) {
-		ret = -EINVAL;
-		goto err;
-	}
-	return 0;
-err:
-	return ret;
-}
-
 static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p)
 {
 	if ((p->busnum >> 8) != drm_get_pci_domain(dev) ||
@@ -444,13 +400,6 @@ int drm_irq_by_busid(struct drm_device *dev, void *data,
 {
 	return -EINVAL;
 }
-
-int drm_pci_set_unique(struct drm_device *dev,
-		       struct drm_master *master,
-		       struct drm_unique *u)
-{
-	return -EINVAL;
-}
 #endif
 
 EXPORT_SYMBOL(drm_pci_init);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/11] drm: Lobotomize set_busid nonsense for !pci drivers

[Daniel Vetter]

We already have a fallback in place to fill out the unique from
dev->unique, which is set to something reasonable in drm_dev_alloc.

Which means we only need to have a special set_busid for pci devices,
to be able to care the backwards compat code for drm 1.1 around, which
libdrm still needs.

While developing and testing this patch things blew up in really
interesting ways, and the code is rather confusing in naming things
between the kernel code, ioctl #defines and libdrm. For the next brave
dragon slayer, document all this madness properly in the userspace
interface section of gpu.tmpl.

v2: Make drm_dev_set_unique static and update kerneldoc.

v3: Entire rewrite, plus document what's going on for posterity in the
gpu docbook uapi section.

v4: Drop accidental amdgpu hunk (Emil).

Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Tested-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> (virt_gpu)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/DocBook/gpu.tmpl                  |  4 ++
 drivers/gpu/drm/armada/armada_drv.c             |  1 -
 drivers/gpu/drm/drm_ioctl.c                     | 58 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_platform.c                  | 18 --------
 drivers/gpu/drm/etnaviv/etnaviv_drv.c           |  1 -
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |  1 -
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  1 -
 drivers/gpu/drm/imx/imx-drm-core.c              |  1 -
 drivers/gpu/drm/msm/msm_drv.c                   |  1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c           |  1 -
 drivers/gpu/drm/omapdrm/omap_drv.c              |  2 -
 drivers/gpu/drm/shmobile/shmob_drm_drv.c        |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c             |  1 -
 drivers/gpu/drm/virtio/virtgpu_drm_bus.c        | 10 -----
 drivers/gpu/drm/virtio/virtgpu_drv.c            |  1 -
 drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
 include/drm/drmP.h                              |  1 -
 17 files changed, 62 insertions(+), 42 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index a836a8bcd289..94c6bdee8080 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3099,6 +3099,10 @@ int num_ioctls;</synopsis>
   <!-- External: render nodes -->
 
     <sect1>
+      <title>libdrm Device Lookup</title>
+!Pdrivers/gpu/drm/drm_vma_manager.c getunique and setversion story
+    </sect1>
+    <sect1>
       <title>Render nodes</title>
       <para>
         DRM core provides multiple character-devices for user-space to use.
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index cb21c0b6374a..f5ebdd681445 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -189,7 +189,6 @@ static struct drm_driver armada_drm_driver = {
 	.load			= armada_drm_load,
 	.lastclose		= armada_drm_lastclose,
 	.unload			= armada_drm_unload,
-	.set_busid		= drm_platform_set_busid,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= armada_drm_enable_vblank,
 	.disable_vblank		= armada_drm_disable_vblank,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 1fa7619face3..063775f7946e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -37,6 +37,64 @@
 #include <linux/pci.h>
 #include <linux/export.h>
 
+/**
+ * DOC: getunique and setversion story
+ *
+ * BEWARE THE DRAGONS! MIND THE TRAPDOORS!
+ *
+ * In and attempt to warn anyone else who's trying to figure out what's going
+ * on here, I'll try to summarize the story. First things first, let's clear up
+ * the names, because the kernel internals, libdrm and the ioctls are all named
+ * differently:
+ *
+ *  - GET_UNIQUE ioctl, implemented by drm_getunique is wrapped up in libdrm
+ *    through the drmGetBusid function.
+ *  - The libdrm drmSetBusid function is backed by the SET_UNIQUE ioctl. All
+ *    that code is nerved in the kernel with drm_invalid_op().
+ *  - The internal set_busid kernel functions and driver callbacks are
+ *    exclusively use by the SET_VERSION ioctl, because only drm 1.0 (which is
+ *    nerved) allowed userspace to set the busid through the above ioctl.
+ *  - Other ioctls and functions involved are named consistently.
+ *
+ * For anyone wondering what's the difference between drm 1.1 and 1.4: Correctly
+ * handling pci domains in the busid on ppc. Doing this correctly was only
+ * implemented in libdrm in 2010, hence can't be nerved yet. No one knows what's
+ * special with drm 1.2 and 1.3.
+ *
+ * Now the actual horror story of how device lookup in drm works. At large,
+ * there's 2 different ways, either by busid, or by device driver name.
+ *
+ * Opening by busid is fairly simple:
+ *
+ * 1. First call SET_VERSION to make sure pci domains are handled properly. As a
+ *    side-effect this fills out the unique name in the master structure.
+ * 2. Call GET_UNIQUE to read out the unique name from the master structure,
+ *    which matches the busid thanks to step 1. If it doesn't, proceed to try
+ *    the next device node.
+ *
+ * Opening by name is slightly different:
+ *
+ * 1. Directly call VERSION to get the version and to match against the driver
+ *    name returned by that ioctl. Note that SET_VERSION is not called, which
+ *    means the the unique name for the master node just opening is _not_ filled
+ *    out. This despite that with current drm device nodes are always bound to
+ *    one device, and can't be runtime assigned like with drm 1.0.
+ * 2. Match driver name. If it mismatches, proceed to the next device node.
+ * 3. Call GET_UNIQUE, and check whether the unique name has length zero (by
+ *    checking that the first byte in the string is 0). If that's not the case
+ *    libdrm skips and proceeds to the next device node. Probably this is just
+ *    copypasta from drm 1.0 times where a set unique name meant that the driver
+ *    was in use already, but that's just conjecture.
+ *
+ * Long story short: To keep the open by name logic working, GET_UNIQUE must
+ * _not_ return a unique string when SET_VERSION hasn't been called yet,
+ * otherwise libdrm breaks. Even when that unique string can't ever change, and
+ * is totally irrelevant for actually opening the device because runtime
+ * assignable device instances were only support in drm 1.0, which is long dead.
+ * But the libdrm code in drmOpenByName somehow survived, hence this can't be
+ * broken.
+ */
+
 static int drm_version(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 644169e1a029..2c819ef90090 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -68,24 +68,6 @@ err_free:
 	return ret;
 }
 
-int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
-{
-	int id;
-
-	id = dev->platformdev->id;
-	if (id < 0)
-		id = 0;
-
-	master->unique = kasprintf(GFP_KERNEL, "platform:%s:%02d",
-						dev->platformdev->name, id);
-	if (!master->unique)
-		return -ENOMEM;
-
-	master->unique_len = strlen(master->unique);
-	return 0;
-}
-EXPORT_SYMBOL(drm_platform_set_busid);
-
 /**
  * drm_platform_init - Register a platform device with the DRM subsystem
  * @driver: DRM device driver
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 3d4f56df8359..340d390306d8 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -496,7 +496,6 @@ static struct drm_driver etnaviv_drm_driver = {
 				DRIVER_RENDER,
 	.open               = etnaviv_open,
 	.preclose           = etnaviv_preclose,
-	.set_busid          = drm_platform_set_busid,
 	.gem_free_object_unlocked = etnaviv_gem_free_object,
 	.gem_vm_ops         = &vm_ops,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 4a679fb9bb02..13d28d4229e2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -407,7 +407,6 @@ static struct drm_driver exynos_drm_driver = {
 	.preclose		= exynos_drm_preclose,
 	.lastclose		= exynos_drm_lastclose,
 	.postclose		= exynos_drm_postclose,
-	.set_busid		= drm_platform_set_busid,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= exynos_drm_crtc_enable_vblank,
 	.disable_vblank		= exynos_drm_crtc_disable_vblank,
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 193657259ee9..418a87cc6ac9 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -171,7 +171,6 @@ static struct drm_driver kirin_drm_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
 				  DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
 	.fops			= &kirin_drm_fops,
-	.set_busid		= drm_platform_set_busid,
 
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 82656654fb21..7746418a4c08 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -407,7 +407,6 @@ static struct drm_driver imx_drm_driver = {
 	.load			= imx_drm_driver_load,
 	.unload			= imx_drm_driver_unload,
 	.lastclose		= imx_drm_driver_lastclose,
-	.set_busid		= drm_platform_set_busid,
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
 	.dumb_create		= drm_gem_cma_dumb_create,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9c654092ef78..798822bfb5c3 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -730,7 +730,6 @@ static struct drm_driver msm_driver = {
 	.open               = msm_open,
 	.preclose           = msm_preclose,
 	.lastclose          = msm_lastclose,
-	.set_busid          = drm_platform_set_busid,
 	.irq_handler        = msm_irq,
 	.irq_preinstall     = msm_irq_preinstall,
 	.irq_postinstall    = msm_irq_postinstall,
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index c00ff6e786a3..295e7621cc68 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1070,7 +1070,6 @@ nouveau_drm_init(void)
 	driver_pci = driver_stub;
 	driver_pci.set_busid = drm_pci_set_busid;
 	driver_platform = driver_stub;
-	driver_platform.set_busid = drm_platform_set_busid;
 
 	nouveau_display_options();
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 6b97011154bf..0e6d5a8ff5fd 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -801,8 +801,6 @@ static struct drm_driver omap_drm_driver = {
 	.unload = dev_unload,
 	.open = dev_open,
 	.lastclose = dev_lastclose,
-	.set_busid = drm_platform_set_busid,
-	.get_vblank_counter = drm_vblank_no_hw_counter,
 	.enable_vblank = omap_irq_enable_vblank,
 	.disable_vblank = omap_irq_disable_vblank,
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index ee79264b5b6a..f0492603ea88 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -259,7 +259,6 @@ static struct drm_driver shmob_drm_driver = {
 				| DRIVER_PRIME,
 	.load			= shmob_drm_load,
 	.unload			= shmob_drm_unload,
-	.set_busid		= drm_platform_set_busid,
 	.irq_handler		= shmob_drm_irq,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= shmob_drm_enable_vblank,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 308e197908fc..d27809372d54 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -541,7 +541,6 @@ static struct drm_driver tilcdc_driver = {
 	.load               = tilcdc_load,
 	.unload             = tilcdc_unload,
 	.lastclose          = tilcdc_lastclose,
-	.set_busid          = drm_platform_set_busid,
 	.irq_handler        = tilcdc_irq,
 	.irq_preinstall     = tilcdc_irq_preinstall,
 	.irq_postinstall    = tilcdc_irq_postinstall,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
index 88a39165edd5..7f0e93f87a55 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
@@ -27,16 +27,6 @@
 
 #include "virtgpu_drv.h"
 
-int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master)
-{
-	struct pci_dev *pdev = dev->pdev;
-
-	if (pdev) {
-		return drm_pci_set_busid(dev, master);
-	}
-	return 0;
-}
-
 static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev)
 {
 	struct apertures_struct *ap;
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 5820b7020ae5..c13f70cfc461 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -117,7 +117,6 @@ static const struct file_operations virtio_gpu_driver_fops = {
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
-	.set_busid = drm_virtio_set_busid,
 	.load = virtio_gpu_driver_load,
 	.unload = virtio_gpu_driver_unload,
 	.open = virtio_gpu_driver_open,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index acf556a35cb2..b18ef3111f0c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -49,7 +49,6 @@
 #define DRIVER_PATCHLEVEL 1
 
 /* virtgpu_drm_bus.c */
-int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master);
 int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev);
 
 struct virtio_gpu_object {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index dd07dc552f28..179bdb228ecb 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1128,7 +1128,6 @@ extern int drm_pcie_get_max_link_width(struct drm_device *dev, u32 *mlw);
 
 /* platform section */
 extern int drm_platform_init(struct drm_driver *driver, struct platform_device *platform_device);
-extern int drm_platform_set_busid(struct drm_device *d, struct drm_master *m);
 
 /* returns true if currently okay to sleep */
 static __inline__ bool drm_can_sleep(void)
-- 
2.8.1

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

[PATCH 08/11] drm: Refactor drop/set master code a bit

[Daniel Vetter]

File open/set_maseter ioctl and file close/drop_master ioctl share the
same master handling code. Extract it.

Note that vmwgfx's master_set callback needs to know whether the
master is a new one or has been used already, so thread this through.
On the close/drop side a similar parameter existed, but wasnt used.
Drop it to simplify the flow.

v2: Try to make it not leak so much (Emil).

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c          | 80 +++++++++++++++++++------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  3 +-
 include/drm/drmP.h                  |  3 +-
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 6bba6b9e9dcc..2794a4f3a105 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -110,6 +110,24 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
 	return master;
 }
 
+static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
+			  bool new_master)
+{
+	int ret = 0;
+
+	dev->master = drm_master_get(fpriv->master);
+	fpriv->is_master = 1;
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, new_master);
+		if (unlikely(ret != 0)) {
+			fpriv->is_master = 0;
+			drm_master_put(&dev->master);
+		}
+	}
+
+	return ret;
+}
+
 /*
  * drm_new_set_master - Allocate a new master object and become master for the
  * associated master realm.
@@ -127,37 +145,32 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 
 	lockdep_assert_held_once(&dev->master_mutex);
 
-	/* create a new master */
-	dev->master = drm_master_create(dev);
-	if (!dev->master)
-		return -ENOMEM;
-
-	/* take another reference for the copy in the local file priv */
 	old_master = fpriv->master;
-	fpriv->master = drm_master_get(dev->master);
+	fpriv->master = drm_master_create(dev);
+	if (!fpriv->master) {
+		fpriv->master = old_master;
+		return -ENOMEM;
+	}
 
 	if (dev->driver->master_create) {
 		ret = dev->driver->master_create(dev, fpriv->master);
 		if (ret)
 			goto out_err;
 	}
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, fpriv, true);
-		if (ret)
-			goto out_err;
-	}
-
-	fpriv->is_master = 1;
 	fpriv->allowed_master = 1;
 	fpriv->authenticated = 1;
+
+	ret = drm_set_master(dev, fpriv, true);
+	if (ret)
+		goto out_err;
+
 	if (old_master)
 		drm_master_put(&old_master);
 
 	return 0;
 
 out_err:
-	/* drop both references and restore old master on failure */
-	drm_master_put(&dev->master);
+	/* drop references and restore old master on failure */
 	drm_master_put(&fpriv->master);
 	fpriv->master = old_master;
 
@@ -188,21 +201,21 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	dev->master = drm_master_get(file_priv->master);
-	file_priv->is_master = 1;
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, file_priv, false);
-		if (unlikely(ret != 0)) {
-			file_priv->is_master = 0;
-			drm_master_put(&dev->master);
-		}
-	}
-
+	ret = drm_set_master(dev, file_priv, false);
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
 	return ret;
 }
 
+static void drm_drop_master(struct drm_device *dev,
+			    struct drm_file *fpriv)
+{
+	if (dev->driver->master_drop)
+		dev->driver->master_drop(dev, fpriv);
+	drm_master_put(&dev->master);
+	fpriv->is_master = 0;
+}
+
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -216,11 +229,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 
 	ret = 0;
-	if (dev->driver->master_drop)
-		dev->driver->master_drop(dev, file_priv, false);
-	drm_master_put(&dev->master);
-	file_priv->is_master = 0;
-
+	drm_drop_master(dev, file_priv);
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
 	return ret;
@@ -271,17 +280,12 @@ void drm_master_release(struct drm_file *file_priv)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (dev->master == file_priv->master) {
-		/* drop the reference held my the minor */
-		if (dev->driver->master_drop)
-			dev->driver->master_drop(dev, file_priv, true);
-		drm_master_put(&dev->master);
-	}
+	if (dev->master == file_priv->master)
+		drm_drop_master(dev, file_priv);
 out:
 	/* drop the master reference held by the file priv */
 	if (file_priv->master)
 		drm_master_put(&file_priv->master);
-	file_priv->is_master = 0;
 	mutex_unlock(&dev->master_mutex);
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 9fcd8200d485..35eedc9d44fa 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1228,8 +1228,7 @@ static int vmw_master_set(struct drm_device *dev,
 }
 
 static void vmw_master_drop(struct drm_device *dev,
-			    struct drm_file *file_priv,
-			    bool from_release)
+			    struct drm_file *file_priv)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
 	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 179bdb228ecb..0f9046de0418 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -573,8 +573,7 @@ struct drm_driver {
 
 	int (*master_set)(struct drm_device *dev, struct drm_file *file_priv,
 			  bool from_open);
-	void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv,
-			    bool from_release);
+	void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv);
 
 	int (*debugfs_init)(struct drm_minor *minor);
 	void (*debugfs_cleanup)(struct drm_minor *minor);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/11] drm: Extract drm_is_current_master

[Daniel Vetter]

Just rolling out a bit of abstraction to be able to clean
up the master logic in the next step.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_auth.c                 | 12 +++++++++---
 drivers/gpu/drm/drm_crtc.c                 |  2 +-
 drivers/gpu/drm/drm_info.c                 |  2 +-
 drivers/gpu/drm/drm_ioctl.c                |  3 ++-
 drivers/gpu/drm/drm_lock.c                 |  2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c        |  2 +-
 include/drm/drmP.h                         | 15 ++++++++-------
 8 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 2794a4f3a105..dc33387519cb 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -183,7 +183,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 	int ret = 0;
 
 	mutex_lock(&dev->master_mutex);
-	if (file_priv->is_master)
+	if (drm_is_current_master(file_priv))
 		goto out_unlock;
 
 	if (dev->master) {
@@ -222,7 +222,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	int ret = -EINVAL;
 
 	mutex_lock(&dev->master_mutex);
-	if (!file_priv->is_master)
+	if (!drm_is_current_master(file_priv))
 		goto out_unlock;
 
 	if (!dev->master)
@@ -261,7 +261,7 @@ void drm_master_release(struct drm_file *file_priv)
 	if (file_priv->magic)
 		idr_remove(&file_priv->master->magic_map, file_priv->magic);
 
-	if (!file_priv->is_master)
+	if (!drm_is_current_master(file_priv))
 		goto out;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
@@ -289,6 +289,12 @@ out:
 	mutex_unlock(&dev->master_mutex);
 }
 
+bool drm_is_current_master(struct drm_file *fpriv)
+{
+	return fpriv->is_master;
+}
+EXPORT_SYMBOL(drm_is_current_master);
+
 struct drm_master *drm_master_get(struct drm_master *master)
 {
 	kref_get(&master->refcount);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9fa9b04cd5a9..2a4c5a408bbe 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3528,7 +3528,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->bpp = fb->bits_per_pixel;
 	r->pitch = fb->pitches[0];
 	if (fb->funcs->create_handle) {
-		if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
+		if (drm_is_current_master(file_priv) || capable(CAP_SYS_ADMIN) ||
 		    drm_is_control_client(file_priv)) {
 			ret = fb->funcs->create_handle(fb, file_priv,
 						       &r->handle);
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index d6cfd5340d52..9ae353f4dd06 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -102,7 +102,7 @@ int drm_clients_info(struct seq_file *m, void *data)
 			   task ? task->comm : "<unknown>",
 			   pid_vnr(priv->pid),
 			   priv->minor->index,
-			   priv->is_master ? 'y' : 'n',
+			   drm_is_current_master(priv) ? 'y' : 'n',
 			   priv->authenticated ? 'y' : 'n',
 			   from_kuid_munged(seq_user_ns(m), priv->uid),
 			   priv->magic);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 063775f7946e..8e921670020e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -481,7 +481,8 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 		return -EACCES;
 
 	/* MASTER is only for master or control clients */
-	if (unlikely((flags & DRM_MASTER) && !file_priv->is_master &&
+	if (unlikely((flags & DRM_MASTER) && 
+		     !drm_is_current_master(file_priv) &&
 		     !drm_is_control_client(file_priv)))
 		return -EACCES;
 
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index ae0a4d39deec..48ac0ebbd663 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -219,7 +219,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
 	/* don't set the block all signals on the master process for now 
 	 * really probably not the correct answer but lets us debug xkb
  	 * xserver for now */
-	if (!file_priv->is_master) {
+	if (!drm_is_current_master(file_priv)) {
 		dev->sigdata.context = lock->context;
 		dev->sigdata.lock = master->lock.hw_lock;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 8097698b9622..7941f1fe9cd2 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1446,7 +1446,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	dispatch_flags = 0;
 	if (args->flags & I915_EXEC_SECURE) {
-		if (!file->is_master || !capable(CAP_SYS_ADMIN))
+		if (!drm_is_current_master(file) || !capable(CAP_SYS_ADMIN))
 		    return -EPERM;
 
 		dispatch_flags |= I915_DISPATCH_SECURE;
@@ -1553,7 +1553,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 							     batch_obj,
 							     args->batch_start_offset,
 							     args->batch_len,
-							     file->is_master);
+							     drm_is_current_master(file));
 		if (IS_ERR(parsed_batch_obj)) {
 			ret = PTR_ERR(parsed_batch_obj);
 			goto err;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 35eedc9d44fa..60646644bef3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1049,7 +1049,7 @@ static struct vmw_master *vmw_master_check(struct drm_device *dev,
 	if (unlikely(ret != 0))
 		return ERR_PTR(-ERESTARTSYS);
 
-	if (file_priv->is_master) {
+	if (drm_is_current_master(file_priv)) {
 		mutex_unlock(&dev->master_mutex);
 		return NULL;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0f9046de0418..5f811edefaf1 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1010,14 +1010,15 @@ static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc
 	return &crtc->dev->vblank[drm_crtc_index(crtc)].queue;
 }
 
-				/* Stub support (drm_stub.h) */
-extern struct drm_master *drm_master_get(struct drm_master *master);
-extern void drm_master_put(struct drm_master **master);
-
-extern void drm_put_dev(struct drm_device *dev);
-extern void drm_unplug_dev(struct drm_device *dev);
+/* drm_auth.c */
+struct drm_master *drm_master_get(struct drm_master *master);
+void drm_master_put(struct drm_master **master);
+bool drm_is_current_master(struct drm_file *fpriv);
+
+/* drm_drv.c */
+void drm_put_dev(struct drm_device *dev);
+void drm_unplug_dev(struct drm_device *dev);
 extern unsigned int drm_debug;
-extern bool drm_atomic;
 
 				/* Debugfs support */
 #if defined(CONFIG_DEBUG_FS)
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/11] drm: Clear up master tracking booleans

[Daniel Vetter]

- is_master can be removed, we can compute this by checking allowed_master
  (which really just tracks whether a master struct has been allocated
  for this fpriv in either open or set_master), and whether the fpriv is
  the current master on the device.

- that frees up is_master as a good replacement name for allowed_master.
  With that it's clear that it tracks whether the fpriv is a master (with
  possibly clients attached to it and authenticated against it), and that
  one of those fprivs with is_master set is the current master.

v2: Fix kerneldoc for is_master (Emil).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_auth.c | 9 +++------
 include/drm/drmP.h         | 6 ++----
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index dc33387519cb..0a23b32e627c 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -116,11 +116,9 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
 	int ret = 0;
 
 	dev->master = drm_master_get(fpriv->master);
-	fpriv->is_master = 1;
 	if (dev->driver->master_set) {
 		ret = dev->driver->master_set(dev, fpriv, new_master);
 		if (unlikely(ret != 0)) {
-			fpriv->is_master = 0;
 			drm_master_put(&dev->master);
 		}
 	}
@@ -157,7 +155,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 		if (ret)
 			goto out_err;
 	}
-	fpriv->allowed_master = 1;
+	fpriv->is_master = 1;
 	fpriv->authenticated = 1;
 
 	ret = drm_set_master(dev, fpriv, true);
@@ -196,7 +194,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	if (!file_priv->allowed_master) {
+	if (!file_priv->is_master) {
 		ret = drm_new_set_master(dev, file_priv);
 		goto out_unlock;
 	}
@@ -213,7 +211,6 @@ static void drm_drop_master(struct drm_device *dev,
 	if (dev->driver->master_drop)
 		dev->driver->master_drop(dev, fpriv);
 	drm_master_put(&dev->master);
-	fpriv->is_master = 0;
 }
 
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
@@ -291,7 +288,7 @@ out:
 
 bool drm_is_current_master(struct drm_file *fpriv)
 {
-	return fpriv->is_master;
+	return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
 }
 EXPORT_SYMBOL(drm_is_current_master);
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 5f811edefaf1..128a03bfdebd 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -303,8 +303,6 @@ struct drm_prime_file_private {
 /** File private data */
 struct drm_file {
 	unsigned authenticated :1;
-	/* Whether we're master for a minor. Protected by master_mutex */
-	unsigned is_master :1;
 	/* true when the client has asked us to expose stereo 3D mode flags */
 	unsigned stereo_allowed :1;
 	/*
@@ -315,10 +313,10 @@ struct drm_file {
 	/* true if client understands atomic properties */
 	unsigned atomic:1;
 	/*
-	 * This client is allowed to gain master privileges for @master.
+	 * This client is the creator of @master.
 	 * Protected by struct drm_device::master_mutex.
 	 */
-	unsigned allowed_master:1;
+	unsigned is_master:1;
 
 	struct pid *pid;
 	kuid_t uid;
-- 
2.8.1

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

[PATCH 11/11] drm: document drm_auth.c

[Daniel Vetter]

Also extract drm_auth.h for nicer grouping.

v2: Nuke the other comments since they don't really explain a lot, and
within the drm core we generally only document functions exported to
drivers: The main audience for these docs are driver writers.

v3: Limit the exposure of drm_master internals by only including
drm_auth.h where it is neede (Chris).

v4: Spelling polish (Emil).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/DocBook/gpu.tmpl      |  6 ++++
 drivers/gpu/drm/drm_auth.c          | 69 +++++++++++++++++++++----------------
 drivers/gpu/drm/drm_crtc.c          |  1 +
 drivers/gpu/drm/drm_ioctl.c         |  1 +
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h |  1 +
 include/drm/drmP.h                  | 30 +---------------
 include/drm/drm_auth.h              | 59 +++++++++++++++++++++++++++++++
 include/drm/drm_legacy.h            |  2 ++
 9 files changed, 112 insertions(+), 58 deletions(-)
 create mode 100644 include/drm/drm_auth.h

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 94c6bdee8080..b7f6316b7bee 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3103,6 +3103,12 @@ int num_ioctls;</synopsis>
 !Pdrivers/gpu/drm/drm_vma_manager.c getunique and setversion story
     </sect1>
     <sect1>
+      <title>Primary Nodes, DRM Master and Authentication</title>
+!Pdrivers/gpu/drm_auth.c master and authentication
+!Edrivers/gpu/drm_auth.c
+!Einclude/drm/drm_auth.h
+    </sect1>
+    <sect1>
       <title>Render nodes</title>
       <para>
         DRM core provides multiple character-devices for user-space to use.
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 0a23b32e627c..c07112b3c3c0 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -32,18 +32,27 @@
 #include "drm_internal.h"
 #include "drm_legacy.h"
 
-/**
- * drm_getmagic - Get unique magic of a client
- * @dev: DRM device to operate on
- * @data: ioctl data containing the drm_auth object
- * @file_priv: DRM file that performs the operation
+/** DOC: master and authentication
+ *
+ * struct &drm_master is used to track groups of clients with open
+ * primary/legacy device nodes. For every struct &drm_file which has had at
+ * least once successfully became the device master (either through the
+ * SET_MASTER IOCTL, or implicitly through opening the primary device node when
+ * no one else is the current master that time) there exists one &drm_master.
+ * This is noted in the is_master member of &drm_file. All other clients have
+ * just a pointer to the &drm_master they are associated with.
  *
- * This looks up the unique magic of the passed client and returns it. If the
- * client did not have a magic assigned, yet, a new one is registered. The magic
- * is stored in the passed drm_auth object.
+ * In addition only one &drm_master can be the current master for a &drm_device.
+ * It can be switched through the DROP_MASTER and SET_MASTER IOCTL, or
+ * implicitly through closing/openeing the primary device node. See also
+ * drm_is_current_master().
  *
- * Returns: 0 on success, negative error code on failure.
+ * Clients can authenticate against the current master (if it matches their own)
+ * using the GETMAGIC and AUTHMAGIC IOCTLs. Together with exchanging masters,
+ * this allows controlled access to the device for an entire group of mutually
+ * trusted clients.
  */
+
 int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
 	struct drm_auth *auth = data;
@@ -64,16 +73,6 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	return ret < 0 ? ret : 0;
 }
 
-/**
- * drm_authmagic - Authenticate client with a magic
- * @dev: DRM device to operate on
- * @data: ioctl data containing the drm_auth object
- * @file_priv: DRM file that performs the operation
- *
- * This looks up a DRM client by the passed magic and authenticates it.
- *
- * Returns: 0 on success, negative error code on failure.
- */
 int drm_authmagic(struct drm_device *dev, void *data,
 		  struct drm_file *file_priv)
 {
@@ -126,16 +125,6 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
 	return ret;
 }
 
-/*
- * drm_new_set_master - Allocate a new master object and become master for the
- * associated master realm.
- *
- * @dev: The associated device.
- * @fpriv: File private identifying the client.
- *
- * This function must be called with dev::master_mutex held.
- * Returns negative error code on failure. Zero on success.
- */
 static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 {
 	struct drm_master *old_master;
@@ -286,12 +275,28 @@ out:
 	mutex_unlock(&dev->master_mutex);
 }
 
+/**
+ * drm_is_current_master - checks whether @priv is the current master
+ * @fpriv: DRM file private
+ *
+ * Checks whether @fpriv is current master on its device. This decides whether a
+ * client is allowed to run DRM_MASTER IOCTLs.
+ *
+ * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
+ * - the current master is assumed to own the non-shareable display hardware.
+ */
 bool drm_is_current_master(struct drm_file *fpriv)
 {
 	return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
 }
 EXPORT_SYMBOL(drm_is_current_master);
 
+/**
+ * drm_master_get - reference a master pointer
+ * @master: struct &drm_master
+ *
+ * Increments the reference count of @master and returns a pointer to @master.
+ */
 struct drm_master *drm_master_get(struct drm_master *master)
 {
 	kref_get(&master->refcount);
@@ -314,6 +319,12 @@ static void drm_master_destroy(struct kref *kref)
 	kfree(master);
 }
 
+/**
+ * drm_master_put - unreference and clear a master pointer
+ * @master: pointer to a pointer of struct &drm_master
+ *
+ * This decrements the &drm_master behind @master and sets it to NULL.
+ */
 void drm_master_put(struct drm_master **master)
 {
 	kref_put(&(*master)->refcount, drm_master_destroy);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2a4c5a408bbe..db93487a3cc4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -39,6 +39,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_auth.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8e921670020e..699a89d22f43 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -30,6 +30,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_core.h>
+#include <drm/drm_auth.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
 #include "drm_crtc_internal.h"
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 48928227bdcc..811ee198a561 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -47,6 +47,7 @@
 #include <drm/intel-gtt.h>
 #include <drm/drm_legacy.h> /* for struct drm_dma_handle */
 #include <drm/drm_gem.h>
+#include <drm/drm_auth.h>
 
 #include "i915_params.h"
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 1980e2a28265..9a90f824814e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -32,6 +32,7 @@
 #include <drm/drmP.h>
 #include <drm/vmwgfx_drm.h>
 #include <drm/drm_hashtab.h>
+#include <drm/drm_auth.h>
 #include <linux/suspend.h>
 #include <drm/ttm/ttm_bo_driver.h>
 #include <drm/ttm/ttm_object.h>
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 128a03bfdebd..28d341afbf6c 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -86,6 +86,7 @@ struct drm_local_map;
 struct drm_device_dma;
 struct drm_dma_handle;
 struct drm_gem_object;
+struct drm_master;
 
 struct device_node;
 struct videomode;
@@ -373,30 +374,6 @@ struct drm_lock_data {
 	int idle_has_lock;
 };
 
-/**
- * struct drm_master - drm master structure
- *
- * @refcount: Refcount for this master object.
- * @dev: Link back to the DRM device
- * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
- * @unique_len: Length of unique field. Protected by drm_global_mutex.
- * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
- * @lock: DRI lock information.
- * @driver_priv: Pointer to driver-private information.
- *
- * Note that master structures are only relevant for the legacy/primary device
- * nodes, hence there can only be one per device, not one per drm_minor.
- */
-struct drm_master {
-	struct kref refcount;
-	struct drm_device *dev;
-	char *unique;
-	int unique_len;
-	struct idr magic_map;
-	struct drm_lock_data lock;
-	void *driver_priv;
-};
-
 /* Flags and return codes for get_vblank_timestamp() driver function. */
 #define DRM_CALLED_FROM_VBLIRQ 1
 #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0)
@@ -1008,11 +985,6 @@ static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc
 	return &crtc->dev->vblank[drm_crtc_index(crtc)].queue;
 }
 
-/* drm_auth.c */
-struct drm_master *drm_master_get(struct drm_master *master);
-void drm_master_put(struct drm_master **master);
-bool drm_is_current_master(struct drm_file *fpriv);
-
 /* drm_drv.c */
 void drm_put_dev(struct drm_device *dev);
 void drm_unplug_dev(struct drm_device *dev);
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
new file mode 100644
index 000000000000..610223b0481b
--- /dev/null
+++ b/include/drm/drm_auth.h
@@ -0,0 +1,59 @@
+/*
+ * Internal Header for the Direct Rendering Manager
+ *
+ * Copyright 2016 Intel Corporation
+ *
+ * Author: Daniel Vetter <daniel.vetter@ffwll.ch>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef _DRM_AUTH_H_
+#define _DRM_AUTH_H_
+
+/**
+ * struct drm_master - drm master structure
+ *
+ * @refcount: Refcount for this master object.
+ * @dev: Link back to the DRM device
+ * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
+ * @unique_len: Length of unique field. Protected by drm_global_mutex.
+ * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
+ * @lock: DRI lock information.
+ * @driver_priv: Pointer to driver-private information.
+ *
+ * Note that master structures are only relevant for the legacy/primary device
+ * nodes, hence there can only be one per device, not one per drm_minor.
+ */
+struct drm_master {
+	struct kref refcount;
+	struct drm_device *dev;
+	char *unique;
+	int unique_len;
+	struct idr magic_map;
+	struct drm_lock_data lock;
+	void *driver_priv;
+};
+
+struct drm_master *drm_master_get(struct drm_master *master);
+void drm_master_put(struct drm_master **master);
+bool drm_is_current_master(struct drm_file *fpriv);
+
+#endif
diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
index a5ef2c7e40f8..cf0e7d89bcdf 100644
--- a/include/drm/drm_legacy.h
+++ b/include/drm/drm_legacy.h
@@ -1,6 +1,8 @@
 #ifndef __DRM_DRM_LEGACY_H__
 #define __DRM_DRM_LEGACY_H__
 
+#include <drm/drm_auth.h>
+
 /*
  * Legacy driver interfaces for the Direct Rendering Manager
  *
-- 
2.8.1

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

[PATCH] drm: Lobotomize set_busid nonsense for !pci drivers

[Daniel Vetter]

We already have a fallback in place to fill out the unique from
dev->unique, which is set to something reasonable in drm_dev_alloc.

Which means we only need to have a special set_busid for pci devices,
to be able to care the backwards compat code for drm 1.1 around, which
libdrm still needs.

While developing and testing this patch things blew up in really
interesting ways, and the code is rather confusing in naming things
between the kernel code, ioctl #defines and libdrm. For the next brave
dragon slayer, document all this madness properly in the userspace
interface section of gpu.tmpl.

v2: Make drm_dev_set_unique static and update kerneldoc.

v3: Entire rewrite, plus document what's going on for posterity in the
gpu docbook uapi section.

v4: Drop accidental amdgpu hunk (Emil).

v5: Drop accidental omapdrm vblank counter change (Emil).

Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Tested-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> (virt_gpu)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/DocBook/gpu.tmpl                  |  4 ++
 drivers/gpu/drm/armada/armada_drv.c             |  1 -
 drivers/gpu/drm/drm_ioctl.c                     | 58 +++++++++++++++++++++++++
 drivers/gpu/drm/drm_platform.c                  | 18 --------
 drivers/gpu/drm/etnaviv/etnaviv_drv.c           |  1 -
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |  1 -
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  1 -
 drivers/gpu/drm/imx/imx-drm-core.c              |  1 -
 drivers/gpu/drm/msm/msm_drv.c                   |  1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c           |  1 -
 drivers/gpu/drm/omapdrm/omap_drv.c              |  1 -
 drivers/gpu/drm/shmobile/shmob_drm_drv.c        |  1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c             |  1 -
 drivers/gpu/drm/virtio/virtgpu_drm_bus.c        | 10 -----
 drivers/gpu/drm/virtio/virtgpu_drv.c            |  1 -
 drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
 include/drm/drmP.h                              |  1 -
 17 files changed, 62 insertions(+), 41 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index a836a8bcd289..94c6bdee8080 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3099,6 +3099,10 @@ int num_ioctls;</synopsis>
   <!-- External: render nodes -->
 
     <sect1>
+      <title>libdrm Device Lookup</title>
+!Pdrivers/gpu/drm/drm_vma_manager.c getunique and setversion story
+    </sect1>
+    <sect1>
       <title>Render nodes</title>
       <para>
         DRM core provides multiple character-devices for user-space to use.
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index cb21c0b6374a..f5ebdd681445 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -189,7 +189,6 @@ static struct drm_driver armada_drm_driver = {
 	.load			= armada_drm_load,
 	.lastclose		= armada_drm_lastclose,
 	.unload			= armada_drm_unload,
-	.set_busid		= drm_platform_set_busid,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= armada_drm_enable_vblank,
 	.disable_vblank		= armada_drm_disable_vblank,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 1fa7619face3..063775f7946e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -37,6 +37,64 @@
 #include <linux/pci.h>
 #include <linux/export.h>
 
+/**
+ * DOC: getunique and setversion story
+ *
+ * BEWARE THE DRAGONS! MIND THE TRAPDOORS!
+ *
+ * In and attempt to warn anyone else who's trying to figure out what's going
+ * on here, I'll try to summarize the story. First things first, let's clear up
+ * the names, because the kernel internals, libdrm and the ioctls are all named
+ * differently:
+ *
+ *  - GET_UNIQUE ioctl, implemented by drm_getunique is wrapped up in libdrm
+ *    through the drmGetBusid function.
+ *  - The libdrm drmSetBusid function is backed by the SET_UNIQUE ioctl. All
+ *    that code is nerved in the kernel with drm_invalid_op().
+ *  - The internal set_busid kernel functions and driver callbacks are
+ *    exclusively use by the SET_VERSION ioctl, because only drm 1.0 (which is
+ *    nerved) allowed userspace to set the busid through the above ioctl.
+ *  - Other ioctls and functions involved are named consistently.
+ *
+ * For anyone wondering what's the difference between drm 1.1 and 1.4: Correctly
+ * handling pci domains in the busid on ppc. Doing this correctly was only
+ * implemented in libdrm in 2010, hence can't be nerved yet. No one knows what's
+ * special with drm 1.2 and 1.3.
+ *
+ * Now the actual horror story of how device lookup in drm works. At large,
+ * there's 2 different ways, either by busid, or by device driver name.
+ *
+ * Opening by busid is fairly simple:
+ *
+ * 1. First call SET_VERSION to make sure pci domains are handled properly. As a
+ *    side-effect this fills out the unique name in the master structure.
+ * 2. Call GET_UNIQUE to read out the unique name from the master structure,
+ *    which matches the busid thanks to step 1. If it doesn't, proceed to try
+ *    the next device node.
+ *
+ * Opening by name is slightly different:
+ *
+ * 1. Directly call VERSION to get the version and to match against the driver
+ *    name returned by that ioctl. Note that SET_VERSION is not called, which
+ *    means the the unique name for the master node just opening is _not_ filled
+ *    out. This despite that with current drm device nodes are always bound to
+ *    one device, and can't be runtime assigned like with drm 1.0.
+ * 2. Match driver name. If it mismatches, proceed to the next device node.
+ * 3. Call GET_UNIQUE, and check whether the unique name has length zero (by
+ *    checking that the first byte in the string is 0). If that's not the case
+ *    libdrm skips and proceeds to the next device node. Probably this is just
+ *    copypasta from drm 1.0 times where a set unique name meant that the driver
+ *    was in use already, but that's just conjecture.
+ *
+ * Long story short: To keep the open by name logic working, GET_UNIQUE must
+ * _not_ return a unique string when SET_VERSION hasn't been called yet,
+ * otherwise libdrm breaks. Even when that unique string can't ever change, and
+ * is totally irrelevant for actually opening the device because runtime
+ * assignable device instances were only support in drm 1.0, which is long dead.
+ * But the libdrm code in drmOpenByName somehow survived, hence this can't be
+ * broken.
+ */
+
 static int drm_version(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 644169e1a029..2c819ef90090 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -68,24 +68,6 @@ err_free:
 	return ret;
 }
 
-int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
-{
-	int id;
-
-	id = dev->platformdev->id;
-	if (id < 0)
-		id = 0;
-
-	master->unique = kasprintf(GFP_KERNEL, "platform:%s:%02d",
-						dev->platformdev->name, id);
-	if (!master->unique)
-		return -ENOMEM;
-
-	master->unique_len = strlen(master->unique);
-	return 0;
-}
-EXPORT_SYMBOL(drm_platform_set_busid);
-
 /**
  * drm_platform_init - Register a platform device with the DRM subsystem
  * @driver: DRM device driver
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 3d4f56df8359..340d390306d8 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -496,7 +496,6 @@ static struct drm_driver etnaviv_drm_driver = {
 				DRIVER_RENDER,
 	.open               = etnaviv_open,
 	.preclose           = etnaviv_preclose,
-	.set_busid          = drm_platform_set_busid,
 	.gem_free_object_unlocked = etnaviv_gem_free_object,
 	.gem_vm_ops         = &vm_ops,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 4a679fb9bb02..13d28d4229e2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -407,7 +407,6 @@ static struct drm_driver exynos_drm_driver = {
 	.preclose		= exynos_drm_preclose,
 	.lastclose		= exynos_drm_lastclose,
 	.postclose		= exynos_drm_postclose,
-	.set_busid		= drm_platform_set_busid,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= exynos_drm_crtc_enable_vblank,
 	.disable_vblank		= exynos_drm_crtc_disable_vblank,
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index ef2c32ec1616..1edd9bc80294 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -171,7 +171,6 @@ static struct drm_driver kirin_drm_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
 				  DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
 	.fops			= &kirin_drm_fops,
-	.set_busid		= drm_platform_set_busid,
 
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 82656654fb21..7746418a4c08 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -407,7 +407,6 @@ static struct drm_driver imx_drm_driver = {
 	.load			= imx_drm_driver_load,
 	.unload			= imx_drm_driver_unload,
 	.lastclose		= imx_drm_driver_lastclose,
-	.set_busid		= drm_platform_set_busid,
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
 	.dumb_create		= drm_gem_cma_dumb_create,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 568fcc328f27..a02dc2b27739 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -722,7 +722,6 @@ static struct drm_driver msm_driver = {
 	.open               = msm_open,
 	.preclose           = msm_preclose,
 	.lastclose          = msm_lastclose,
-	.set_busid          = drm_platform_set_busid,
 	.irq_handler        = msm_irq,
 	.irq_preinstall     = msm_irq_preinstall,
 	.irq_postinstall    = msm_irq_postinstall,
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index c00ff6e786a3..295e7621cc68 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1070,7 +1070,6 @@ nouveau_drm_init(void)
 	driver_pci = driver_stub;
 	driver_pci.set_busid = drm_pci_set_busid;
 	driver_platform = driver_stub;
-	driver_platform.set_busid = drm_platform_set_busid;
 
 	nouveau_display_options();
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 6b97011154bf..26c6134eb744 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -801,7 +801,6 @@ static struct drm_driver omap_drm_driver = {
 	.unload = dev_unload,
 	.open = dev_open,
 	.lastclose = dev_lastclose,
-	.set_busid = drm_platform_set_busid,
 	.get_vblank_counter = drm_vblank_no_hw_counter,
 	.enable_vblank = omap_irq_enable_vblank,
 	.disable_vblank = omap_irq_disable_vblank,
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index ee79264b5b6a..f0492603ea88 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -259,7 +259,6 @@ static struct drm_driver shmob_drm_driver = {
 				| DRIVER_PRIME,
 	.load			= shmob_drm_load,
 	.unload			= shmob_drm_unload,
-	.set_busid		= drm_platform_set_busid,
 	.irq_handler		= shmob_drm_irq,
 	.get_vblank_counter	= drm_vblank_no_hw_counter,
 	.enable_vblank		= shmob_drm_enable_vblank,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 308e197908fc..d27809372d54 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -541,7 +541,6 @@ static struct drm_driver tilcdc_driver = {
 	.load               = tilcdc_load,
 	.unload             = tilcdc_unload,
 	.lastclose          = tilcdc_lastclose,
-	.set_busid          = drm_platform_set_busid,
 	.irq_handler        = tilcdc_irq,
 	.irq_preinstall     = tilcdc_irq_preinstall,
 	.irq_postinstall    = tilcdc_irq_postinstall,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
index 88a39165edd5..7f0e93f87a55 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
@@ -27,16 +27,6 @@
 
 #include "virtgpu_drv.h"
 
-int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master)
-{
-	struct pci_dev *pdev = dev->pdev;
-
-	if (pdev) {
-		return drm_pci_set_busid(dev, master);
-	}
-	return 0;
-}
-
 static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev)
 {
 	struct apertures_struct *ap;
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 5820b7020ae5..c13f70cfc461 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -117,7 +117,6 @@ static const struct file_operations virtio_gpu_driver_fops = {
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
-	.set_busid = drm_virtio_set_busid,
 	.load = virtio_gpu_driver_load,
 	.unload = virtio_gpu_driver_unload,
 	.open = virtio_gpu_driver_open,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index acf556a35cb2..b18ef3111f0c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -49,7 +49,6 @@
 #define DRIVER_PATCHLEVEL 1
 
 /* virtgpu_drm_bus.c */
-int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master);
 int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev);
 
 struct virtio_gpu_object {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index dd07dc552f28..179bdb228ecb 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1128,7 +1128,6 @@ extern int drm_pcie_get_max_link_width(struct drm_device *dev, u32 *mlw);
 
 /* platform section */
 extern int drm_platform_init(struct drm_driver *driver, struct platform_device *platform_device);
-extern int drm_platform_set_busid(struct drm_device *d, struct drm_master *m);
 
 /* returns true if currently okay to sleep */
 static __inline__ bool drm_can_sleep(void)
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Refactor drop/set master code a bit

[Daniel Vetter]

File open/set_maseter ioctl and file close/drop_master ioctl share the
same master handling code. Extract it.

Note that vmwgfx's master_set callback needs to know whether the
master is a new one or has been used already, so thread this through.
On the close/drop side a similar parameter existed, but wasnt used.
Drop it to simplify the flow.

v2: Try to make it not leak so much (Emil).

v3: Send out the right version ...

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c          | 80 +++++++++++++++++++------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  3 +-
 include/drm/drmP.h                  |  3 +-
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 6bba6b9e9dcc..2794a4f3a105 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -110,6 +110,24 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
 	return master;
 }
 
+static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
+			  bool new_master)
+{
+	int ret = 0;
+
+	dev->master = drm_master_get(fpriv->master);
+	fpriv->is_master = 1;
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, new_master);
+		if (unlikely(ret != 0)) {
+			fpriv->is_master = 0;
+			drm_master_put(&dev->master);
+		}
+	}
+
+	return ret;
+}
+
 /*
  * drm_new_set_master - Allocate a new master object and become master for the
  * associated master realm.
@@ -127,37 +145,32 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 
 	lockdep_assert_held_once(&dev->master_mutex);
 
-	/* create a new master */
-	dev->master = drm_master_create(dev);
-	if (!dev->master)
-		return -ENOMEM;
-
-	/* take another reference for the copy in the local file priv */
 	old_master = fpriv->master;
-	fpriv->master = drm_master_get(dev->master);
+	fpriv->master = drm_master_create(dev);
+	if (!fpriv->master) {
+		fpriv->master = old_master;
+		return -ENOMEM;
+	}
 
 	if (dev->driver->master_create) {
 		ret = dev->driver->master_create(dev, fpriv->master);
 		if (ret)
 			goto out_err;
 	}
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, fpriv, true);
-		if (ret)
-			goto out_err;
-	}
-
-	fpriv->is_master = 1;
 	fpriv->allowed_master = 1;
 	fpriv->authenticated = 1;
+
+	ret = drm_set_master(dev, fpriv, true);
+	if (ret)
+		goto out_err;
+
 	if (old_master)
 		drm_master_put(&old_master);
 
 	return 0;
 
 out_err:
-	/* drop both references and restore old master on failure */
-	drm_master_put(&dev->master);
+	/* drop references and restore old master on failure */
 	drm_master_put(&fpriv->master);
 	fpriv->master = old_master;
 
@@ -188,21 +201,21 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	dev->master = drm_master_get(file_priv->master);
-	file_priv->is_master = 1;
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, file_priv, false);
-		if (unlikely(ret != 0)) {
-			file_priv->is_master = 0;
-			drm_master_put(&dev->master);
-		}
-	}
-
+	ret = drm_set_master(dev, file_priv, false);
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
 	return ret;
 }
 
+static void drm_drop_master(struct drm_device *dev,
+			    struct drm_file *fpriv)
+{
+	if (dev->driver->master_drop)
+		dev->driver->master_drop(dev, fpriv);
+	drm_master_put(&dev->master);
+	fpriv->is_master = 0;
+}
+
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -216,11 +229,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 
 	ret = 0;
-	if (dev->driver->master_drop)
-		dev->driver->master_drop(dev, file_priv, false);
-	drm_master_put(&dev->master);
-	file_priv->is_master = 0;
-
+	drm_drop_master(dev, file_priv);
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
 	return ret;
@@ -271,17 +280,12 @@ void drm_master_release(struct drm_file *file_priv)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (dev->master == file_priv->master) {
-		/* drop the reference held my the minor */
-		if (dev->driver->master_drop)
-			dev->driver->master_drop(dev, file_priv, true);
-		drm_master_put(&dev->master);
-	}
+	if (dev->master == file_priv->master)
+		drm_drop_master(dev, file_priv);
 out:
 	/* drop the master reference held by the file priv */
 	if (file_priv->master)
 		drm_master_put(&file_priv->master);
-	file_priv->is_master = 0;
 	mutex_unlock(&dev->master_mutex);
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 9fcd8200d485..35eedc9d44fa 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1228,8 +1228,7 @@ static int vmw_master_set(struct drm_device *dev,
 }
 
 static void vmw_master_drop(struct drm_device *dev,
-			    struct drm_file *file_priv,
-			    bool from_release)
+			    struct drm_file *file_priv)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
 	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 179bdb228ecb..0f9046de0418 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -573,8 +573,7 @@ struct drm_driver {
 
 	int (*master_set)(struct drm_device *dev, struct drm_file *file_priv,
 			  bool from_open);
-	void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv,
-			    bool from_release);
+	void (*master_drop)(struct drm_device *dev, struct drm_file *file_priv);
 
 	int (*debugfs_init)(struct drm_minor *minor);
 	void (*debugfs_cleanup)(struct drm_minor *minor);
-- 
2.8.1

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

✓ Ro.CI.BAT: success for drm: Lobotomize set_busid nonsense for !pci drivers

[Patchwork]

== Series Details ==

Series: drm: Lobotomize set_busid nonsense for !pci drivers
URL   : https://patchwork.freedesktop.org/series/8989/
State : success

== Summary ==

Series 8989v1 drm: Lobotomize set_busid nonsense for !pci drivers
http://patchwork.freedesktop.org/api/1.0/series/8989/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:225  pass:192  dwarn:0   dfail:0   fail:2   skip:31 
fi-skl-i5-6260u  total:225  pass:200  dwarn:0   dfail:0   fail:2   skip:23 
fi-skl-i7-6700k  total:225  pass:186  dwarn:0   dfail:0   fail:2   skip:37 
fi-snb-i7-2600   total:225  pass:172  dwarn:0   dfail:0   fail:2   skip:51 
ro-bdw-i5-5250u  total:225  pass:197  dwarn:1   dfail:0   fail:0   skip:27 
ro-bdw-i7-5600u  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
ro-byt-n2820     total:225  pass:173  dwarn:0   dfail:0   fail:3   skip:49 
ro-hsw-i3-4010u  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
ro-hsw-i7-4770r  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
ro-ilk-i7-620lm  total:225  pass:150  dwarn:0   dfail:0   fail:1   skip:74 
ro-ilk1-i5-650   total:220  pass:150  dwarn:0   dfail:0   fail:1   skip:69 
ro-ivb-i7-3770   total:225  pass:181  dwarn:0   dfail:0   fail:0   skip:44 
ro-ivb2-i7-3770  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
ro-skl3-i5-6260u total:225  pass:201  dwarn:1   dfail:0   fail:0   skip:23 
ro-snb-i7-2620M  total:225  pass:174  dwarn:0   dfail:0   fail:1   skip:50 
fi-bxt-q6he failed to connect after reboot
fi-kbl-qkkr failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1254/

2cdff39 drm-intel-nightly: 2016y-06m-21d-11h-54m-56s UTC integration manifest
0f182b1 drm: Lobotomize set_busid nonsense for !pci drivers

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for drm: Refactor drop/set master code a bit

[Patchwork]

== Series Details ==

Series: drm: Refactor drop/set master code a bit
URL   : https://patchwork.freedesktop.org/series/8993/
State : failure

== Summary ==

Applying: drm: Refactor drop/set master code a bit
fatal: sha1 information is lacking or useless (include/drm/drmP.h).
error: could not build fake ancestor
Patch failed at 0001 drm: Refactor drop/set master code a bit
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

Hello Daniel,

On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
> We already have a fallback in place to fill out the unique from
> dev->unique, which is set to something reasonable in drm_dev_alloc.
> 
> Which means we only need to have a special set_busid for pci devices,
> to be able to care the backwards compat code for drm 1.1 around, which
> libdrm still needs.
> 
> While developing and testing this patch things blew up in really
> interesting ways, and the code is rather confusing in naming things
> between the kernel code, ioctl #defines and libdrm. For the next brave
> dragon slayer, document all this madness properly in the userspace
> interface section of gpu.tmpl.
> 
> v2: Make drm_dev_set_unique static and update kerneldoc.
> 
> v3: Entire rewrite, plus document what's going on for posterity in the
> gpu docbook uapi section.
> 
> v4: Drop accidental amdgpu hunk (Emil).
> 
> v5: Drop accidental omapdrm vblank counter change (Emil).
> 
> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Cc: Emil Velikov <emil.l.velikov at gmail.com>
> Tested-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk> (virt_gpu)
> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  Documentation/DocBook/gpu.tmpl                  |  4 ++
>  drivers/gpu/drm/armada/armada_drv.c             |  1 -
>  drivers/gpu/drm/drm_ioctl.c                     | 58 +++++++++++++++++++++++++
>  drivers/gpu/drm/drm_platform.c                  | 18 --------
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c           |  1 -
>  drivers/gpu/drm/exynos/exynos_drm_drv.c         |  1 -
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  1 -
>  drivers/gpu/drm/imx/imx-drm-core.c              |  1 -
>  drivers/gpu/drm/msm/msm_drv.c                   |  1 -
>  drivers/gpu/drm/nouveau/nouveau_drm.c           |  1 -
>  drivers/gpu/drm/omapdrm/omap_drv.c              |  1 -
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c        |  1 -
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c             |  1 -
>  drivers/gpu/drm/virtio/virtgpu_drm_bus.c        | 10 -----
>  drivers/gpu/drm/virtio/virtgpu_drv.c            |  1 -
>  drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
>  include/drm/drmP.h                              |  1 -
>  17 files changed, 62 insertions(+), 41 deletions(-)

This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
device. Please see

  https://bugzilla.redhat.com/show_bug.cgi?id=1366842

complete with a bisection log under

  drivers/gpu/drm/virtio/

(comment 20).

Copying Thorsten so he can include this report in his next v4.8-rc8
regression report, if he chooses so. (Commit a325725633c2 is part of
v4.8-rc1, but we only managed to identify it now.) The last such report
I know of is archived e.g. at
<http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1239220.html>.

Reported-by: Joachim Frieben <jfrieben@hotmail.com>

Thanks
Laszlo

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Hans de Goede]

Hi,

On 30-09-16 05:09, Laszlo Ersek wrote:
> Hello Daniel,
>
> On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
>> We already have a fallback in place to fill out the unique from
>> dev->unique, which is set to something reasonable in drm_dev_alloc.
>>
>> Which means we only need to have a special set_busid for pci devices,
>> to be able to care the backwards compat code for drm 1.1 around, which
>> libdrm still needs.
>>
>> While developing and testing this patch things blew up in really
>> interesting ways, and the code is rather confusing in naming things
>> between the kernel code, ioctl #defines and libdrm. For the next brave
>> dragon slayer, document all this madness properly in the userspace
>> interface section of gpu.tmpl.
>>
>> v2: Make drm_dev_set_unique static and update kerneldoc.
>>
>> v3: Entire rewrite, plus document what's going on for posterity in the
>> gpu docbook uapi section.
>>
>> v4: Drop accidental amdgpu hunk (Emil).
>>
>> v5: Drop accidental omapdrm vblank counter change (Emil).
>>
>> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>> Tested-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk> (virt_gpu)
>> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> ---
>>  Documentation/DocBook/gpu.tmpl                  |  4 ++
>>  drivers/gpu/drm/armada/armada_drv.c             |  1 -
>>  drivers/gpu/drm/drm_ioctl.c                     | 58 +++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_platform.c                  | 18 --------
>>  drivers/gpu/drm/etnaviv/etnaviv_drv.c           |  1 -
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c         |  1 -
>>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  1 -
>>  drivers/gpu/drm/imx/imx-drm-core.c              |  1 -
>>  drivers/gpu/drm/msm/msm_drv.c                   |  1 -
>>  drivers/gpu/drm/nouveau/nouveau_drm.c           |  1 -
>>  drivers/gpu/drm/omapdrm/omap_drv.c              |  1 -
>>  drivers/gpu/drm/shmobile/shmob_drm_drv.c        |  1 -
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c             |  1 -
>>  drivers/gpu/drm/virtio/virtgpu_drm_bus.c        | 10 -----
>>  drivers/gpu/drm/virtio/virtgpu_drv.c            |  1 -
>>  drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
>>  include/drm/drmP.h                              |  1 -
>>  17 files changed, 62 insertions(+), 41 deletions(-)
>
> This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
> device. Please see
>
>   https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>
> complete with a bisection log under
>
>   drivers/gpu/drm/virtio/
>
> (comment 20).
>
> Copying Thorsten so he can include this report in his next v4.8-rc8
> regression report, if he chooses so. (Commit a325725633c2 is part of
> v4.8-rc1, but we only managed to identify it now.) The last such report
> I know of is archived e.g. at
> <http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1239220.html>.
>
> Reported-by: Joachim Frieben <jfrieben@hotmail.com>

First of all Joachim thanks for bisecting this. I was thinking about this
bug / issue, while doing my laps in the swimming pool.

I wanted to add a comment to the bug to tell you that this is likely
a Xorg xserver issue and not a kernel issue and that there is no need to
bisect, but it is too late for that now.

Xorg when running without a Xorg.conf searches for what it considers
a "primary" gpu / video-card, basically it attempts to bring up the
right card in setups where there are multiple cards and if it does not
find one exits with an error.

The xserver has a 2 step process for finding the primary card:

1) It searches for is a card which has a vga-bios mapped,
as we've already determined in the mentioned Red Hat bug that works for
the classic qemu emulated video-cards, but not for qemu's virtio-vga.

2) If that does not work Xorg will fallback to any video class device
on pci-bus 1.

This fallback actually has been broken in the Xorg xserver for quite a
while now and only 2 days ago a patch from Laszlo was merged to fix this.

Only for things to break again due to this kernel patch.

Since the whole step 2) thingie is very much tied to x86 machines
where pci-bus 0 used to be the main bus and pci-bus 1 the agp,
which is sorta an obsolete assumption now a days and  since relying
on bus numbers / enumeration order is a bad idea in general I'm not
entirely sure if this counts as a regression.

I've discussed the problem of the xserver exiting with an error when
no primary device can be found with some people (ajax) at XDC last week
since there are other use-cases where the pci-bus 1 fallback does not
work.

As such I've been working on a xserver patch-set to make the xserver
try harder (pick the first available device) when both steps described
above fail to find one, which should make things work even with the
newest (broken / regressed) kernels.

Given this mail thread, I guess I'm working after all today (I had
planned a day off) and I'll try to wrap up this patch-set and reply
to this mail with the server patches attached for Joachim and/or
Laszlo to test.

Regards,

Hans



p.s.

It would be interesting to do a lspci on both a working and a
non-working kernel to see what exactly is going on here.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

Hi all

On 30 September 2016 at 04:09, Laszlo Ersek <lersek@redhat.com> wrote:
> Hello Daniel,
>
> On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
>> We already have a fallback in place to fill out the unique from
>> dev->unique, which is set to something reasonable in drm_dev_alloc.
>>
>> Which means we only need to have a special set_busid for pci devices,
>> to be able to care the backwards compat code for drm 1.1 around, which
>> libdrm still needs.
>>
>> While developing and testing this patch things blew up in really
>> interesting ways, and the code is rather confusing in naming things
>> between the kernel code, ioctl #defines and libdrm. For the next brave
>> dragon slayer, document all this madness properly in the userspace
>> interface section of gpu.tmpl.
>>
>> v2: Make drm_dev_set_unique static and update kerneldoc.
>>
>> v3: Entire rewrite, plus document what's going on for posterity in the
>> gpu docbook uapi section.
>>
>> v4: Drop accidental amdgpu hunk (Emil).
>>
>> v5: Drop accidental omapdrm vblank counter change (Emil).
>>

> This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
> device. Please see
>
>   https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>
> complete with a bisection log under
>
>   drivers/gpu/drm/virtio/
>
Having gone through the xserver code as this patch was written, I'm
not entrirely sure how this can happen.

Since there is a very nice cleanup series that this patch is build
upon, I'd suggest trying to understand the issue, rather than a blind
revert.

Here we want to check the xserver/libdrm codepath before/after the
kernel patch, in particular the following drmOpen, drmAvailable,
drmSetInterfaceVersion and drmGetBusid. If the xserver is using open()
with drmSetInterfaceVersion then that is _not_ something it should be
doing and I'm against working around it in the kernel (fwiw).

To ease things one can set LIBGL_DEBUG=verbose (yay fun name) and
libdrm will print out some debug output to stderr

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 09/30/16 10:28, Hans de Goede wrote:
> Hi,
> 
> On 30-09-16 05:09, Laszlo Ersek wrote:
>> Hello Daniel,
>>
>> On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
>>> We already have a fallback in place to fill out the unique from
>>> dev->unique, which is set to something reasonable in drm_dev_alloc.
>>>
>>> Which means we only need to have a special set_busid for pci devices,
>>> to be able to care the backwards compat code for drm 1.1 around, which
>>> libdrm still needs.
>>>
>>> While developing and testing this patch things blew up in really
>>> interesting ways, and the code is rather confusing in naming things
>>> between the kernel code, ioctl #defines and libdrm. For the next brave
>>> dragon slayer, document all this madness properly in the userspace
>>> interface section of gpu.tmpl.
>>>
>>> v2: Make drm_dev_set_unique static and update kerneldoc.
>>>
>>> v3: Entire rewrite, plus document what's going on for posterity in the
>>> gpu docbook uapi section.
>>>
>>> v4: Drop accidental amdgpu hunk (Emil).
>>>
>>> v5: Drop accidental omapdrm vblank counter change (Emil).
>>>
>>> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>> Tested-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>>> (virt_gpu)
>>> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>> ---
>>>  Documentation/DocBook/gpu.tmpl                  |  4 ++
>>>  drivers/gpu/drm/armada/armada_drv.c             |  1 -
>>>  drivers/gpu/drm/drm_ioctl.c                     | 58
>>> +++++++++++++++++++++++++
>>>  drivers/gpu/drm/drm_platform.c                  | 18 --------
>>>  drivers/gpu/drm/etnaviv/etnaviv_drv.c           |  1 -
>>>  drivers/gpu/drm/exynos/exynos_drm_drv.c         |  1 -
>>>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  1 -
>>>  drivers/gpu/drm/imx/imx-drm-core.c              |  1 -
>>>  drivers/gpu/drm/msm/msm_drv.c                   |  1 -
>>>  drivers/gpu/drm/nouveau/nouveau_drm.c           |  1 -
>>>  drivers/gpu/drm/omapdrm/omap_drv.c              |  1 -
>>>  drivers/gpu/drm/shmobile/shmob_drm_drv.c        |  1 -
>>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c             |  1 -
>>>  drivers/gpu/drm/virtio/virtgpu_drm_bus.c        | 10 -----
>>>  drivers/gpu/drm/virtio/virtgpu_drv.c            |  1 -
>>>  drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
>>>  include/drm/drmP.h                              |  1 -
>>>  17 files changed, 62 insertions(+), 41 deletions(-)
>>
>> This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
>> device. Please see
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>>
>> complete with a bisection log under
>>
>>   drivers/gpu/drm/virtio/
>>
>> (comment 20).
>>
>> Copying Thorsten so he can include this report in his next v4.8-rc8
>> regression report, if he chooses so. (Commit a325725633c2 is part of
>> v4.8-rc1, but we only managed to identify it now.) The last such report
>> I know of is archived e.g. at
>> <http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1239220.html>.
>>
>>
>> Reported-by: Joachim Frieben <jfrieben@hotmail.com>
> 
> First of all Joachim thanks for bisecting this.

(Small correction: while Joachim reported the BZ, the bisection was done
by yours truly. The bisection was painful enough that I'd want to take
"credit" for it -- using the stock Fedora kernel config for the
bisection, I think my laptop must have burned through enough electricity
to power a small town from Christmas to New Year's Eve. I *literally*
took naps between the test cycles. (And I mean literally literally.) I
know about "localmodconfig" but it has broken on me before, so I opted
for the Fedora config.)

> I was thinking about this
> bug / issue, while doing my laps in the swimming pool.

If you do that, it's easy to lose count of your laps ;)

> I wanted to add a comment to the bug to tell you that this is likely
> a Xorg xserver issue and not a kernel issue and that there is no need to
> bisect, but it is too late for that now.

Ouch. :/

> Xorg when running without a Xorg.conf searches for what it considers
> a "primary" gpu / video-card, basically it attempts to bring up the
> right card in setups where there are multiple cards and if it does not
> find one exits with an error.
> 
> The xserver has a 2 step process for finding the primary card:
> 
> 1) It searches for is a card which has a vga-bios mapped,
> as we've already determined in the mentioned Red Hat bug that works for
> the classic qemu emulated video-cards, but not for qemu's virtio-vga.
> 
> 2) If that does not work Xorg will fallback to any video class device
> on pci-bus 1.
> 
> This fallback actually has been broken in the Xorg xserver for quite a
> while now and only 2 days ago a patch from Laszlo was merged to fix this.
> 
> Only for things to break again due to this kernel patch.
> 
> Since the whole step 2) thingie is very much tied to x86 machines
> where pci-bus 0 used to be the main bus and pci-bus 1 the agp,
> which is sorta an obsolete assumption now a days and  since relying
> on bus numbers / enumeration order is a bad idea in general I'm not
> entirely sure if this counts as a regression.
> 
> I've discussed the problem of the xserver exiting with an error when
> no primary device can be found with some people (ajax) at XDC last week
> since there are other use-cases where the pci-bus 1 fallback does not
> work.
> 
> As such I've been working on a xserver patch-set to make the xserver
> try harder (pick the first available device) when both steps described
> above fail to find one, which should make things work even with the
> newest (broken / regressed) kernels.
> 
> Given this mail thread, I guess I'm working after all today (I had
> planned a day off)

Apologies...

> and I'll try to wrap up this patch-set and reply
> to this mail with the server patches attached for Joachim and/or
> Laszlo to test.

Thank you Hans, that's very kind of you. (And I also greatly appreciate
your description of the primary card selection logic.)

> p.s.
> 
> It would be interesting to do a lspci on both a working and a
> non-working kernel to see what exactly is going on here.

I'll upload the outputs to the RHBZ soon.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Hans de Goede]

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

Hi,

On 30-09-16 10:28, Hans de Goede wrote:
> Hi,
>

<snip>

> Xorg when running without a Xorg.conf searches for what it considers
> a "primary" gpu / video-card, basically it attempts to bring up the
> right card in setups where there are multiple cards and if it does not
> find one exits with an error.
>
> The xserver has a 2 step process for finding the primary card:
>
> 1) It searches for is a card which has a vga-bios mapped,
> as we've already determined in the mentioned Red Hat bug that works for
> the classic qemu emulated video-cards, but not for qemu's virtio-vga.
>
> 2) If that does not work Xorg will fallback to any video class device
> on pci-bus 1.
>
> This fallback actually has been broken in the Xorg xserver for quite a
> while now and only 2 days ago a patch from Laszlo was merged to fix this.
>
> Only for things to break again due to this kernel patch.
>
> Since the whole step 2) thingie is very much tied to x86 machines
> where pci-bus 0 used to be the main bus and pci-bus 1 the agp,
> which is sorta an obsolete assumption now a days and  since relying
> on bus numbers / enumeration order is a bad idea in general I'm not
> entirely sure if this counts as a regression.
>
> I've discussed the problem of the xserver exiting with an error when
> no primary device can be found with some people (ajax) at XDC last week
> since there are other use-cases where the pci-bus 1 fallback does not
> work.
>
> As such I've been working on a xserver patch-set to make the xserver
> try harder (pick the first available device) when both steps described
> above fail to find one, which should make things work even with the
> newest (broken / regressed) kernels.
>
> Given this mail thread, I guess I'm working after all today (I had
> planned a day off) and I'll try to wrap up this patch-set and reply
> to this mail with the server patches attached for Joachim and/or
> Laszlo to test.

Attached are 2 patches against the xserver which should fix this,
please give them a try.

Regards,

Hans

[-- Attachment #2: 0001-xfree86-Make-adding-unclaimed-devices-as-GPU-devices.patch --]
[-- Type: text/x-patch, Size: 2991 bytes --]

>From 8499d06529c3f95cf94a6d4429e18d48052c2689 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 30 Sep 2016 11:59:04 +0200
Subject: [PATCH 1/2] xfree86: Make adding unclaimed devices as GPU devices a
 separate step

This is primarily a preparation patch for fixing the xserver exiting with
a "no screens found" error even though there are supported video cards,
due to the server not recognizing any card as the primary card.

This also fixes the (mostly theoretical) case of a platformBus capable
driver adding a device as GPUscreen before a driver which only supports
the old PCI probe method gets a chance to claim it as a normal screen.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/xfree86/common/xf86Bus.c         |  4 ++++
 hw/xfree86/common/xf86platformBus.c | 15 +++++++++++++++
 hw/xfree86/common/xf86platformBus.h |  6 ++++++
 3 files changed, 25 insertions(+)

diff --git a/hw/xfree86/common/xf86Bus.c b/hw/xfree86/common/xf86Bus.c
index 27c6b1b..a3a9898 100644
--- a/hw/xfree86/common/xf86Bus.c
+++ b/hw/xfree86/common/xf86Bus.c
@@ -125,6 +125,10 @@ xf86BusConfig(void)
         xf86CallDriverProbe(xf86DriverList[i], FALSE);
     }
 
+    for (i = 0; i < xf86NumDrivers; i++) {
+        xf86platformAddGPUDevices(xf86DriverList[i]);
+    }
+
     /* If nothing was detected, return now */
     if (xf86NumScreens == 0) {
         xf86Msg(X_ERROR, "No devices detected.\n");
diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c
index 3e2264f..ec2a1dd 100644
--- a/hw/xfree86/common/xf86platformBus.c
+++ b/hw/xfree86/common/xf86platformBus.c
@@ -476,6 +476,21 @@ xf86platformProbeDev(DriverPtr drvp)
                                         isGPUDevice(devList[i]) ? PLATFORM_PROBE_GPU_SCREEN : 0);
     }
 
+    return foundScreen;
+}
+
+int
+xf86platformAddGPUDevices(DriverPtr drvp)
+{
+    Bool foundScreen = FALSE;
+    GDevPtr *devList;
+    int j;
+
+    if (!drvp->platformProbe)
+        return FALSE;
+
+    xf86MatchDevice(drvp->driverName, &devList);
+
     /* if autoaddgpu devices is enabled then go find any unclaimed platform
      * devices and add them as GPU screens */
     if (xf86Info.autoAddGPU) {
diff --git a/hw/xfree86/common/xf86platformBus.h b/hw/xfree86/common/xf86platformBus.h
index a7335b9..0f5c0ef 100644
--- a/hw/xfree86/common/xf86platformBus.h
+++ b/hw/xfree86/common/xf86platformBus.h
@@ -41,6 +41,7 @@ struct xf86_platform_device {
 #ifdef XSERVER_PLATFORM_BUS
 int xf86platformProbe(void);
 int xf86platformProbeDev(DriverPtr drvp);
+int xf86platformAddGPUDevices(DriverPtr drvp);
 
 extern int xf86_num_platform_devices;
 extern struct xf86_platform_device *xf86_platform_devices;
@@ -156,6 +157,11 @@ xf86PlatformMatchDriver(char *matches[], int nmatches);
 
 extern void xf86platformVTProbe(void);
 extern void xf86platformPrimary(void);
+
+#else
+
+static inline int xf86platformAddGPUDevices(DriverPtr drvp) { return FALSE; }
+
 #endif
 
 #endif
-- 
2.9.3


[-- Attachment #3: 0002-xfree86-Try-harder-to-find-atleast-1-non-GPU-Screen.patch --]
[-- Type: text/x-patch, Size: 4701 bytes --]

>From 3241fcf1636e1b1cb8e0fdd698a2d2e00d1bbcc0 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 30 Sep 2016 12:29:09 +0200
Subject: [PATCH 2/2] xfree86: Try harder to find atleast 1 non GPU Screen

If we did not find any non GPU Screens, try again ignoring the notion
of any video devices being the primary device. This fixes Xorg exiting
with a "no screens found" error when using virtio-vga in a
virtual-machine and when using a device driven by simpledrm.

This is a somewhat ugly solution, but it is the best I can come up with
without major surgery to the bus and probe code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/xfree86/common/xf86.h            |  1 +
 hw/xfree86/common/xf86Bus.c         | 26 +++++++++++++++++++++++---
 hw/xfree86/common/xf86Globals.c     |  1 +
 hw/xfree86/common/xf86pciBus.c      |  4 ++++
 hw/xfree86/common/xf86platformBus.c |  4 ++++
 5 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/hw/xfree86/common/xf86.h b/hw/xfree86/common/xf86.h
index e54c811..f724688 100644
--- a/hw/xfree86/common/xf86.h
+++ b/hw/xfree86/common/xf86.h
@@ -55,6 +55,7 @@
 extern _X_EXPORT int xf86DoConfigure;
 extern _X_EXPORT int xf86DoShowOptions;
 extern _X_EXPORT Bool xf86DoConfigurePass1;
+extern _X_EXPORT Bool xf86ProbeIgnorePrimary;
 extern _X_EXPORT Bool xorgHWAccess;
 
 extern _X_EXPORT DevPrivateKeyRec xf86ScreenKeyRec;
diff --git a/hw/xfree86/common/xf86Bus.c b/hw/xfree86/common/xf86Bus.c
index a3a9898..9836803 100644
--- a/hw/xfree86/common/xf86Bus.c
+++ b/hw/xfree86/common/xf86Bus.c
@@ -117,14 +117,34 @@ xf86BusConfig(void)
     int i, j;
 
     /*
-     * Now call each of the Probe functions.  Each successful probe will
-     * result in an extra entry added to the xf86Screens[] list for each
-     * instance of the hardware found.
+     * 3 step probe to (hopefully) ensure that we always find at least 1
+     * (non GPU) screen:
+     *
+     * 1. Call each drivers probe function normally,
+     *    Each successful probe will result in an extra entry added to the
+     *    xf86Screens[] list for each instance of the hardware found.
      */
     for (i = 0; i < xf86NumDrivers; i++) {
         xf86CallDriverProbe(xf86DriverList[i], FALSE);
     }
 
+    /*
+     * 2. If no Screens were found, call each drivers probe function with
+     *    ignorePrimary = TRUE, to ensure that we do actually get a
+     *    Screen if there is atleast one supported video card.
+     */
+    if (xf86NumScreens == 0) {
+        xf86ProbeIgnorePrimary = TRUE;
+        for (i = 0; i < xf86NumDrivers && xf86NumScreens == 0; i++) {
+            xf86CallDriverProbe(xf86DriverList[i], FALSE);
+        }
+        xf86ProbeIgnorePrimary = FALSE;
+    }
+
+    /*
+     * 3. Call xf86platformAddGPUDevices() to add any additional video cards as
+     *    GPUScreens (GPUScreens are only supported by platformBus drivers).
+     */
     for (i = 0; i < xf86NumDrivers; i++) {
         xf86platformAddGPUDevices(xf86DriverList[i]);
     }
diff --git a/hw/xfree86/common/xf86Globals.c b/hw/xfree86/common/xf86Globals.c
index 072c3fc..0d1e31b 100644
--- a/hw/xfree86/common/xf86Globals.c
+++ b/hw/xfree86/common/xf86Globals.c
@@ -153,6 +153,7 @@ XF86ConfigPtr xf86configptr = NULL;
 Bool xf86Resetting = FALSE;
 Bool xf86Initialising = FALSE;
 Bool xf86DoConfigure = FALSE;
+Bool xf86ProbeIgnorePrimary = FALSE;
 Bool xf86DoShowOptions = FALSE;
 DriverPtr *xf86DriverList = NULL;
 int xf86NumDrivers = 0;
diff --git a/hw/xfree86/common/xf86pciBus.c b/hw/xfree86/common/xf86pciBus.c
index 8158c2b..9adfee5 100644
--- a/hw/xfree86/common/xf86pciBus.c
+++ b/hw/xfree86/common/xf86pciBus.c
@@ -352,6 +352,10 @@ xf86ComparePciBusString(const char *busID, int bus, int device, int func)
 Bool
 xf86IsPrimaryPci(struct pci_device *pPci)
 {
+    /* Add max. 1 screen for the IgnorePrimary fallback path */
+    if (xf86ProbeIgnorePrimary && xf86NumScreens == 0)
+        return TRUE;
+
     if (primaryBus.type == BUS_PCI)
         return pPci == primaryBus.id.pci;
 #ifdef XSERVER_PLATFORM_BUS
diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c
index ec2a1dd..fde6796 100644
--- a/hw/xfree86/common/xf86platformBus.c
+++ b/hw/xfree86/common/xf86platformBus.c
@@ -115,6 +115,10 @@ xf86_find_platform_device_by_devnum(int major, int minor)
 static Bool
 xf86IsPrimaryPlatform(struct xf86_platform_device *plat)
 {
+    /* Add max. 1 screen for the IgnorePrimary fallback path */
+    if (xf86ProbeIgnorePrimary && xf86NumScreens == 0)
+        return TRUE;
+
     if (primaryBus.type == BUS_PLATFORM)
         return plat == primaryBus.id.plat;
 #ifdef XSERVER_LIBPCIACCESS
-- 
2.9.3


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

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

On 30 September 2016 at 10:55, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi all
>
> On 30 September 2016 at 04:09, Laszlo Ersek <lersek@redhat.com> wrote:
>> Hello Daniel,
>>
>> On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
>>> We already have a fallback in place to fill out the unique from
>>> dev->unique, which is set to something reasonable in drm_dev_alloc.
>>>
>>> Which means we only need to have a special set_busid for pci devices,
>>> to be able to care the backwards compat code for drm 1.1 around, which
>>> libdrm still needs.
>>>
>>> While developing and testing this patch things blew up in really
>>> interesting ways, and the code is rather confusing in naming things
>>> between the kernel code, ioctl #defines and libdrm. For the next brave
>>> dragon slayer, document all this madness properly in the userspace
>>> interface section of gpu.tmpl.
>>>
>>> v2: Make drm_dev_set_unique static and update kerneldoc.
>>>
>>> v3: Entire rewrite, plus document what's going on for posterity in the
>>> gpu docbook uapi section.
>>>
>>> v4: Drop accidental amdgpu hunk (Emil).
>>>
>>> v5: Drop accidental omapdrm vblank counter change (Emil).
>>>
>
>> This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
>> device. Please see
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>>
>> complete with a bisection log under
>>
>>   drivers/gpu/drm/virtio/
>>
> Having gone through the xserver code as this patch was written, I'm
> not entrirely sure how this can happen.
>
> Since there is a very nice cleanup series that this patch is build
> upon, I'd suggest trying to understand the issue, rather than a blind
> revert.
>
> Here we want to check the xserver/libdrm codepath before/after the
> kernel patch, in particular the following drmOpen, drmAvailable,
> drmSetInterfaceVersion and drmGetBusid. If the xserver is using open()
> with drmSetInterfaceVersion then that is _not_ something it should be
> doing and I'm against working around it in the kernel (fwiw).
>
Skimming through the xserver code, with the xserver patch/fix the
following comes up - before we would use open(card0) while now we'll
opt for drmOpen alongside the drmSetInterfaceVersion/drmGetBusid.

IMHO the proper way (tm) to fix this is to stop using drmOpen (replace
with open), drmAvailable, drmSetInterfaceVersion (drop these two, they
are legacy UMS code) and drmGetBusid (where needed use the drm*Device
API.

Ideally we'll get platform/usb devices support for drm*Device thus one
will be able to correctly query/enumerate all the available devices,
apply heuristics and open() the {card,render,control} node of the
required device.

On the kernel side I'm haven't looked too closely, although things
should be handled correctly -> drm_dev_set_unique, sets the
unique/busid which is called in virtio via
drm_dev_init/drm_dev_alloc/drm_virtio_init


I'll see if I can find some time to untangle this later on today, if
not I'll look into it tomorrow morning.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

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

On 09/30/16 12:35, Hans de Goede wrote:

> Attached are 2 patches against the xserver which should fix this,
> please give them a try.

Sorry about the delay.

The patches don't seem to fix the issue for me. Please see the Xorg log
attached.

I tested the patches as follows. Given that my bisection had been done
in a Fedora 24 guest, using

  xorg-x11-server-1.18.4-4.fc24
  http://koji.fedoraproject.org/koji/buildinfo?buildID=794494

I now rebuilt the guest kernel exactly at the failing commit (a325725
"drm: Lobotomize set_busid nonsense for !pci drivers"), and first
reproduced the issue with the above X server.

Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
xserver tree), and rebuilt the Fedora package with the backport. For the
backport, I had to cherry-pick the following two patches from master first:

1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
               xf86IsPrimaryPlatform()
2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID

This way your patches applied cleanly. (Cherry pick #1 above is actually
necessary for semantics, while cherry pick #2 is needed for a clean
context only, and has no impact for this test.)

That is, in total, I added the following four patches to the Fedora 24
package:

1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
2 config: fix GPUDevice fail when AutoAddGPU off + BusID
3 xfree86: Make adding unclaimed devices as GPU devices a separate step
4 xfree86: Try harder to find atleast 1 non GPU Screen

You can find the scratch build that I used for testing here:

  xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
  http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087

Another reason I used F24's X server as basis, rather than upstream
HEAD, is that Fedora 24 is pretty young, and it's already on kernel
4.7.4, and I believe it will soon move to kernel 4.8, without
(necessarily) rebasing its X server package to upstream. IOW the kernel
upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
Fedora X maintainers would have to cherry pick those two patches as
dependencies just the same.

To summarize, the patches don't seem to help. I shall nonetheless thank
you for spending your Friday on this!

Laszlo

[-- Attachment #2: Xorg.0.log --]
[-- Type: text/x-log, Size: 4117 bytes --]

[     9.841] 
X.Org X Server 1.18.4
Release Date: 2016-07-19
[     9.842] X Protocol Version 11, Revision 0
[     9.842] Build Operating System:  4.7.2-201.fc24.x86_64 
[     9.842] Current Operating System: Linux ovmf-fedora 4.7.0-rc2+ #7 SMP Fri Sep 30 12:05:35 CEST 2016 x86_64
[     9.842] Kernel command line: BOOT_IMAGE=/vmlinuz-4.7.0-rc2+ root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/swap vconsole.font=latarcyrheb-sun16 rd.lvm.lv=fedora/root rhgb quiet video=640x480 no_console_suspend console=tty console=ttyS0,115200 ignore_loglevel LANG=en_US.UTF-8
[     9.842] Build Date: 30 September 2016  02:16:20PM
[     9.842] Build ID: xorg-x11-server 1.18.4-4.hans_bz1366842_2.fc24 
[     9.842] Current version of pixman: 0.34.0
[     9.842] 	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.
[     9.842] Markers: (--) probed, (**) from config file, (==) default setting,
	(++) from command line, (!!) notice, (II) informational,
	(WW) warning, (EE) error, (NI) not implemented, (??) unknown.
[     9.842] (==) Log file: "/var/log/Xorg.0.log", Time: Fri Sep 30 16:32:21 2016
[     9.842] (==) Using config directory: "/etc/X11/xorg.conf.d"
[     9.842] (==) Using system config directory "/usr/share/X11/xorg.conf.d"
[     9.842] (==) No Layout section.  Using the first Screen section.
[     9.842] (**) |-->Screen "Default Screen" (0)
[     9.842] (**) |   |-->Monitor "Default Monitor"
[     9.842] (**) |   |-->Device "Default Device"
[     9.842] (==) Automatically adding devices
[     9.842] (==) Automatically enabling devices
[     9.842] (==) Automatically adding GPU devices
[     9.842] (==) Max clients allowed: 256, resource mask: 0x1fffff
[     9.842] (==) FontPath set to:
	catalogue:/etc/X11/fontpath.d,
	built-ins
[     9.843] (==) ModulePath set to "/usr/lib64/xorg/modules"
[     9.843] (II) The server relies on udev to provide the list of input devices.
	If no devices become available, reconfigure udev or disable AutoAddDevices.
[     9.843] (II) Loader magic: 0x825dc0
[     9.843] (II) Module ABI versions:
[     9.843] 	X.Org ANSI C Emulation: 0.4
[     9.843] 	X.Org Video Driver: 20.0
[     9.843] 	X.Org XInput driver : 22.1
[     9.843] 	X.Org Server Extension : 9.0
[     9.843] (++) using VT number 1

[     9.843] (II) systemd-logind: logind integration requires -keeptty and -keeptty was not provided, disabling logind integration
[     9.844] (II) xfree86: Adding drm device (/dev/dri/card0)
[     9.848] (--) PCI:*(0:0:2:0) 1af4:1050:1af4:1100 rev 1, Mem @ 0xc0000000/8388608, 0x800000000/16384, 0xc0804000/4096, BIOS @ 0x????????/131072
[     9.849] (II) LoadModule: "glx"
[     9.849] (II) Loading /usr/lib64/xorg/modules/extensions/libglx.so
[     9.851] (II) Module glx: vendor="X.Org Foundation"
[     9.851] 	compiled for 1.18.4, module version = 1.0.0
[     9.851] 	ABI class: X.Org Server Extension, version 9.0
[     9.851] (==) AIGLX enabled
[     9.851] (II) LoadModule: "modesetting"
[     9.851] (II) Loading /usr/lib64/xorg/modules/drivers/modesetting_drv.so
[     9.851] (II) Module modesetting: vendor="X.Org Foundation"
[     9.851] 	compiled for 1.18.4, module version = 1.18.4
[     9.851] 	Module class: X.Org Video Driver
[     9.851] 	ABI class: X.Org Video Driver, version 20.0
[     9.851] (II) modesetting: Driver for Modesetting Kernel Drivers: kms
[     9.851] (WW) Falling back to old probe method for modesetting
[     9.851] (II) modeset(1): using default device
[     9.852] (II) modeset(G0): using drv /dev/dri/card0
[     9.852] (EE) Screen 0 deleted because of no matching config section.
[     9.852] (II) UnloadModule: "modesetting"
[     9.852] (EE) 
Fatal server error:
[     9.852] (EE) Cannot run in framebuffer mode. Please specify busIDs        for all framebuffer devices
[     9.852] (EE) 
[     9.852] (EE) 
Please consult the Fedora Project support 
	 at http://wiki.x.org
 for help. 
[     9.852] (EE) Please also check the log file at "/var/log/Xorg.0.log" for additional information.
[     9.852] (EE) 
[     9.853] (EE) Server terminated with error (1). Closing log file.

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

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Hans de Goede]

Hi,

On 30-09-16 16:51, Laszlo Ersek wrote:
> On 09/30/16 12:35, Hans de Goede wrote:
>
>> Attached are 2 patches against the xserver which should fix this,
>> please give them a try.
>
> Sorry about the delay.
>
> The patches don't seem to fix the issue for me. Please see the Xorg log
> attached.
>
> I tested the patches as follows. Given that my bisection had been done
> in a Fedora 24 guest, using
>
>   xorg-x11-server-1.18.4-4.fc24
>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>
> I now rebuilt the guest kernel exactly at the failing commit (a325725
> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
> reproduced the issue with the above X server.
>
> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
> xserver tree), and rebuilt the Fedora package with the backport. For the
> backport, I had to cherry-pick the following two patches from master first:
>
> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>                xf86IsPrimaryPlatform()
> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>
> This way your patches applied cleanly. (Cherry pick #1 above is actually
> necessary for semantics, while cherry pick #2 is needed for a clean
> context only, and has no impact for this test.)
>
> That is, in total, I added the following four patches to the Fedora 24
> package:
>
> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>
> You can find the scratch build that I used for testing here:
>
>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>
> Another reason I used F24's X server as basis, rather than upstream
> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
> 4.7.4, and I believe it will soon move to kernel 4.8, without
> (necessarily) rebasing its X server package to upstream. IOW the kernel
> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
> Fedora X maintainers would have to cherry pick those two patches as
> dependencies just the same.
>
> To summarize, the patches don't seem to help. I shall nonetheless thank
> you for spending your Friday on this!

Hmm, do you have a xorg.conf file lying around somewhere, the message
about the xserver not being able to find an entry for screen 0 does
not make sense ...

Anyways, Monday is another day we can look at this ...

Regards,

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

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

On 09/30/16 16:59, Hans de Goede wrote:
> Hi,
> 
> On 30-09-16 16:51, Laszlo Ersek wrote:
>> On 09/30/16 12:35, Hans de Goede wrote:
>>
>>> Attached are 2 patches against the xserver which should fix this,
>>> please give them a try.
>>
>> Sorry about the delay.
>>
>> The patches don't seem to fix the issue for me. Please see the Xorg log
>> attached.
>>
>> I tested the patches as follows. Given that my bisection had been done
>> in a Fedora 24 guest, using
>>
>>   xorg-x11-server-1.18.4-4.fc24
>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>
>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>> reproduced the issue with the above X server.
>>
>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>> xserver tree), and rebuilt the Fedora package with the backport. For the
>> backport, I had to cherry-pick the following two patches from master
>> first:
>>
>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>                xf86IsPrimaryPlatform()
>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>
>> This way your patches applied cleanly. (Cherry pick #1 above is actually
>> necessary for semantics, while cherry pick #2 is needed for a clean
>> context only, and has no impact for this test.)
>>
>> That is, in total, I added the following four patches to the Fedora 24
>> package:
>>
>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>
>> You can find the scratch build that I used for testing here:
>>
>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>
>> Another reason I used F24's X server as basis, rather than upstream
>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>> Fedora X maintainers would have to cherry pick those two patches as
>> dependencies just the same.
>>
>> To summarize, the patches don't seem to help. I shall nonetheless thank
>> you for spending your Friday on this!
> 
> Hmm, do you have a xorg.conf file lying around somewhere, the message
> about the xserver not being able to find an entry for screen 0 does
> not make sense ...

Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":

* "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
contents

------------
# Read and parsed by systemd-localed. It's probably wise not to edit
this file
# manually too freely.
Section "InputClass"
        Identifier "system-keyboard"
        MatchIsKeyboard "on"
        Option "XkbLayout" "us"
EndSection
------------

* "01-resolution.conf", which I had created, in order to set the
preferred display resolution:

------------
Section "Screen"
  Identifier "Default Screen"
  Device     "Default Device"
  Monitor    "Default Monitor"
EndSection

Section "Device"
  Identifier "Default Device"
  Driver     "modesetting"
EndSection

Section "Monitor"
  Identifier "Default Monitor"
  Option     "PreferredMode"   "640x480"
# Option     "PreferredMode"   "1440x900"
EndSection
------------

I removed these files now, and repeated the test. Again, the X server
wouldn't start, but I think the log file looks a bit different now.
Attached.

> Anyways, Monday is another day we can look at this ...

Sure; many thanks!
Laszlo


[-- Attachment #2: Xorg.0.log --]
[-- Type: text/x-log, Size: 5691 bytes --]

[     9.853] 
X.Org X Server 1.18.4
Release Date: 2016-07-19
[     9.853] X Protocol Version 11, Revision 0
[     9.853] Build Operating System:  4.7.2-201.fc24.x86_64 
[     9.853] Current Operating System: Linux ovmf-fedora 4.7.0-rc2+ #7 SMP Fri Sep 30 12:05:35 CEST 2016 x86_64
[     9.853] Kernel command line: BOOT_IMAGE=/vmlinuz-4.7.0-rc2+ root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/swap vconsole.font=latarcyrheb-sun16 rd.lvm.lv=fedora/root rhgb quiet video=640x480 no_console_suspend console=tty console=ttyS0,115200 ignore_loglevel LANG=en_US.UTF-8
[     9.853] Build Date: 30 September 2016  02:16:20PM
[     9.853] Build ID: xorg-x11-server 1.18.4-4.hans_bz1366842_2.fc24 
[     9.853] Current version of pixman: 0.34.0
[     9.853] 	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.
[     9.853] Markers: (--) probed, (**) from config file, (==) default setting,
	(++) from command line, (!!) notice, (II) informational,
	(WW) warning, (EE) error, (NI) not implemented, (??) unknown.
[     9.853] (==) Log file: "/var/log/Xorg.0.log", Time: Fri Sep 30 17:25:34 2016
[     9.853] (==) Using system config directory "/usr/share/X11/xorg.conf.d"
[     9.854] (==) No Layout section.  Using the first Screen section.
[     9.854] (==) No screen section available. Using defaults.
[     9.854] (**) |-->Screen "Default Screen Section" (0)
[     9.854] (**) |   |-->Monitor "<default monitor>"
[     9.854] (==) No monitor specified for screen "Default Screen Section".
	Using a default monitor configuration.
[     9.854] (==) Automatically adding devices
[     9.854] (==) Automatically enabling devices
[     9.854] (==) Automatically adding GPU devices
[     9.854] (==) Max clients allowed: 256, resource mask: 0x1fffff
[     9.854] (==) FontPath set to:
	catalogue:/etc/X11/fontpath.d,
	built-ins
[     9.854] (==) ModulePath set to "/usr/lib64/xorg/modules"
[     9.854] (II) The server relies on udev to provide the list of input devices.
	If no devices become available, reconfigure udev or disable AutoAddDevices.
[     9.854] (II) Loader magic: 0x825dc0
[     9.854] (II) Module ABI versions:
[     9.854] 	X.Org ANSI C Emulation: 0.4
[     9.854] 	X.Org Video Driver: 20.0
[     9.854] 	X.Org XInput driver : 22.1
[     9.854] 	X.Org Server Extension : 9.0
[     9.855] (++) using VT number 1

[     9.855] (II) systemd-logind: logind integration requires -keeptty and -keeptty was not provided, disabling logind integration
[     9.856] (II) xfree86: Adding drm device (/dev/dri/card0)
[     9.860] (--) PCI:*(0:0:2:0) 1af4:1050:1af4:1100 rev 1, Mem @ 0xc0000000/8388608, 0x800000000/16384, 0xc0804000/4096, BIOS @ 0x????????/131072
[     9.860] (II) LoadModule: "glx"
[     9.860] (II) Loading /usr/lib64/xorg/modules/extensions/libglx.so
[     9.862] (II) Module glx: vendor="X.Org Foundation"
[     9.862] 	compiled for 1.18.4, module version = 1.0.0
[     9.862] 	ABI class: X.Org Server Extension, version 9.0
[     9.862] (==) AIGLX enabled
[     9.862] (==) Matched modesetting as autoconfigured driver 0
[     9.862] (==) Matched fbdev as autoconfigured driver 1
[     9.862] (==) Matched vesa as autoconfigured driver 2
[     9.862] (==) Assigned the driver to the xf86ConfigLayout
[     9.862] (II) LoadModule: "modesetting"
[     9.862] (II) Loading /usr/lib64/xorg/modules/drivers/modesetting_drv.so
[     9.862] (II) Module modesetting: vendor="X.Org Foundation"
[     9.863] 	compiled for 1.18.4, module version = 1.18.4
[     9.863] 	Module class: X.Org Video Driver
[     9.863] 	ABI class: X.Org Video Driver, version 20.0
[     9.863] (II) LoadModule: "fbdev"
[     9.863] (II) Loading /usr/lib64/xorg/modules/drivers/fbdev_drv.so
[     9.863] (II) Module fbdev: vendor="X.Org Foundation"
[     9.863] 	compiled for 1.18.0, module version = 0.4.3
[     9.863] 	Module class: X.Org Video Driver
[     9.863] 	ABI class: X.Org Video Driver, version 20.0
[     9.863] (II) LoadModule: "vesa"
[     9.863] (II) Loading /usr/lib64/xorg/modules/drivers/vesa_drv.so
[     9.863] (II) Module vesa: vendor="X.Org Foundation"
[     9.863] 	compiled for 1.18.0, module version = 2.3.2
[     9.863] 	Module class: X.Org Video Driver
[     9.863] 	ABI class: X.Org Video Driver, version 20.0
[     9.863] (II) modesetting: Driver for Modesetting Kernel Drivers: kms
[     9.863] (II) FBDEV: driver for framebuffer: fbdev
[     9.863] (II) VESA: driver for VESA chipsets: vesa
[     9.863] (WW) Falling back to old probe method for modesetting
[     9.863] (II) modeset(1): using default device
[     9.863] (II) Loading sub module "fbdevhw"
[     9.863] (II) LoadModule: "fbdevhw"
[     9.863] (II) Loading /usr/lib64/xorg/modules/libfbdevhw.so
[     9.864] (II) Module fbdevhw: vendor="X.Org Foundation"
[     9.864] 	compiled for 1.18.4, module version = 0.0.2
[     9.864] 	ABI class: X.Org Video Driver, version 20.0
[     9.864] (**) FBDEV(2): claimed PCI slot 0@0:2:0
[     9.864] (II) FBDEV(2): using default device
[     9.864] (WW) Falling back to old probe method for vesa
[     9.864] (II) modeset(G0): using drv /dev/dri/card0
[     9.864] (EE) Screen 0 deleted because of no matching config section.
[     9.864] (II) UnloadModule: "modesetting"
[     9.864] (EE) 
Fatal server error:
[     9.864] (EE) Cannot run in framebuffer mode. Please specify busIDs        for all framebuffer devices
[     9.864] (EE) 
[     9.864] (EE) 
Please consult the Fedora Project support 
	 at http://wiki.x.org
 for help. 
[     9.864] (EE) Please also check the log file at "/var/log/Xorg.0.log" for additional information.
[     9.864] (EE) 
[     9.865] (EE) Server terminated with error (1). Closing log file.

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

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Hans de Goede]

Hi,

On 30-09-16 17:33, Laszlo Ersek wrote:
> On 09/30/16 16:59, Hans de Goede wrote:
>> Hi,
>>
>> On 30-09-16 16:51, Laszlo Ersek wrote:
>>> On 09/30/16 12:35, Hans de Goede wrote:
>>>
>>>> Attached are 2 patches against the xserver which should fix this,
>>>> please give them a try.
>>>
>>> Sorry about the delay.
>>>
>>> The patches don't seem to fix the issue for me. Please see the Xorg log
>>> attached.
>>>
>>> I tested the patches as follows. Given that my bisection had been done
>>> in a Fedora 24 guest, using
>>>
>>>   xorg-x11-server-1.18.4-4.fc24
>>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>>
>>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>>> reproduced the issue with the above X server.
>>>
>>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>>> xserver tree), and rebuilt the Fedora package with the backport. For the
>>> backport, I had to cherry-pick the following two patches from master
>>> first:
>>>
>>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>>                xf86IsPrimaryPlatform()
>>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>
>>> This way your patches applied cleanly. (Cherry pick #1 above is actually
>>> necessary for semantics, while cherry pick #2 is needed for a clean
>>> context only, and has no impact for this test.)
>>>
>>> That is, in total, I added the following four patches to the Fedora 24
>>> package:
>>>
>>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>>
>>> You can find the scratch build that I used for testing here:
>>>
>>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>>
>>> Another reason I used F24's X server as basis, rather than upstream
>>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>>> Fedora X maintainers would have to cherry pick those two patches as
>>> dependencies just the same.
>>>
>>> To summarize, the patches don't seem to help. I shall nonetheless thank
>>> you for spending your Friday on this!
>>
>> Hmm, do you have a xorg.conf file lying around somewhere, the message
>> about the xserver not being able to find an entry for screen 0 does
>> not make sense ...
>
> Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":
>
> * "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
> contents
>
> ------------
> # Read and parsed by systemd-localed. It's probably wise not to edit
> this file
> # manually too freely.
> Section "InputClass"
>         Identifier "system-keyboard"
>         MatchIsKeyboard "on"
>         Option "XkbLayout" "us"
> EndSection
> ------------
>
> * "01-resolution.conf", which I had created, in order to set the
> preferred display resolution:
>
> ------------
> Section "Screen"
>   Identifier "Default Screen"
>   Device     "Default Device"
>   Monitor    "Default Monitor"
> EndSection
>
> Section "Device"
>   Identifier "Default Device"
>   Driver     "modesetting"
> EndSection
>
> Section "Monitor"
>   Identifier "Default Monitor"
>   Option     "PreferredMode"   "640x480"
> # Option     "PreferredMode"   "1440x900"
> EndSection
> ------------
>
> I removed these files now, and repeated the test. Again, the X server
> wouldn't start, but I think the log file looks a bit different now.
> Attached.

Ah, ok so it seems that my initial analysis is wrong, the problem
is not a re-occuring of the device getting identified as a GPU screen,
libdrm sorta depends on bus-ids and the lack of one is causing the
server to misbehave. I guess that even with a xorg.conf things
will fail with the troublesome kernel version (might be worth
trying).

Emil's analysis seems to be spot on. This does not seem easily
fixable in userspace / does seem like a real regression as it
even breaks things when specifying the device through xorg.conf
(I or so I believe) which is something which uses to work ...

I made the mistake of thinking the kernel change was re-triggering
the old problem Laszlo fixed, but that does not seem to be the
case.

Regards,

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

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

On 09/30/16 18:38, Hans de Goede wrote:
> Hi,
> 
> On 30-09-16 17:33, Laszlo Ersek wrote:
>> On 09/30/16 16:59, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 30-09-16 16:51, Laszlo Ersek wrote:
>>>> On 09/30/16 12:35, Hans de Goede wrote:
>>>>
>>>>> Attached are 2 patches against the xserver which should fix this,
>>>>> please give them a try.
>>>>
>>>> Sorry about the delay.
>>>>
>>>> The patches don't seem to fix the issue for me. Please see the Xorg log
>>>> attached.
>>>>
>>>> I tested the patches as follows. Given that my bisection had been done
>>>> in a Fedora 24 guest, using
>>>>
>>>>   xorg-x11-server-1.18.4-4.fc24
>>>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>>>
>>>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>>>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>>>> reproduced the issue with the above X server.
>>>>
>>>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>>>> xserver tree), and rebuilt the Fedora package with the backport. For
>>>> the
>>>> backport, I had to cherry-pick the following two patches from master
>>>> first:
>>>>
>>>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>>>                xf86IsPrimaryPlatform()
>>>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>
>>>> This way your patches applied cleanly. (Cherry pick #1 above is
>>>> actually
>>>> necessary for semantics, while cherry pick #2 is needed for a clean
>>>> context only, and has no impact for this test.)
>>>>
>>>> That is, in total, I added the following four patches to the Fedora 24
>>>> package:
>>>>
>>>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>>>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>>>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>>>
>>>> You can find the scratch build that I used for testing here:
>>>>
>>>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>>>
>>>> Another reason I used F24's X server as basis, rather than upstream
>>>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>>>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>>>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>>>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>>>> Fedora X maintainers would have to cherry pick those two patches as
>>>> dependencies just the same.
>>>>
>>>> To summarize, the patches don't seem to help. I shall nonetheless thank
>>>> you for spending your Friday on this!
>>>
>>> Hmm, do you have a xorg.conf file lying around somewhere, the message
>>> about the xserver not being able to find an entry for screen 0 does
>>> not make sense ...
>>
>> Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":
>>
>> * "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
>> contents
>>
>> ------------
>> # Read and parsed by systemd-localed. It's probably wise not to edit
>> this file
>> # manually too freely.
>> Section "InputClass"
>>         Identifier "system-keyboard"
>>         MatchIsKeyboard "on"
>>         Option "XkbLayout" "us"
>> EndSection
>> ------------
>>
>> * "01-resolution.conf", which I had created, in order to set the
>> preferred display resolution:
>>
>> ------------
>> Section "Screen"
>>   Identifier "Default Screen"
>>   Device     "Default Device"
>>   Monitor    "Default Monitor"
>> EndSection
>>
>> Section "Device"
>>   Identifier "Default Device"
>>   Driver     "modesetting"
>> EndSection
>>
>> Section "Monitor"
>>   Identifier "Default Monitor"
>>   Option     "PreferredMode"   "640x480"
>> # Option     "PreferredMode"   "1440x900"
>> EndSection
>> ------------
>>
>> I removed these files now, and repeated the test. Again, the X server
>> wouldn't start, but I think the log file looks a bit different now.
>> Attached.
> 
> Ah, ok so it seems that my initial analysis is wrong, the problem
> is not a re-occuring of the device getting identified as a GPU screen,
> libdrm sorta depends on bus-ids and the lack of one is causing the
> server to misbehave. I guess that even with a xorg.conf things
> will fail with the troublesome kernel version (might be worth
> trying).
> 
> Emil's analysis seems to be spot on. This does not seem easily
> fixable in userspace / does seem like a real regression as it
> even breaks things when specifying the device through xorg.conf
> (I or so I believe) which is something which uses to work ...

In order to check this hypothesis, I did the following:
- I downgraded my xorg-x11-server installation to the most recent
official F24 packages, that is, "1.18.4-4.fc24",
- I kept the kernel that I built exactly at the regressive commit
(a325725633c2)
- I modified "01-resolution.conf" (see it above in the context) like this:

----
Section "Device"
  Identifier "Default Device"
  Driver     "modesetting"
  BusID      "PCI:00:02:0" <------------ new option added
EndSection
----

where BusID matches the B/D/F of the virtio-vga device from "lspci".

This setup (modulo the kernel of course) was known to work, but now the
X server actually segfaults (apparently in the
xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.

(NB: this is unrelated to upstream commit de9ce6757c2e -- which the
pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
it is left at its default "on" value.)

Therefore, you are right. :)

Thanks
Laszlo

> I made the mistake of thinking the kernel change was re-triggering
> the old problem Laszlo fixed, but that does not seem to be the
> case.
> 
> Regards,
> 
> Hans


[-- Attachment #2: Xorg.0.log --]
[-- Type: text/x-log, Size: 4610 bytes --]

[     9.978] 
X.Org X Server 1.18.4
Release Date: 2016-07-19
[     9.978] X Protocol Version 11, Revision 0
[     9.978] Build Operating System:  4.6.4-301.fc24.x86_64 
[     9.978] Current Operating System: Linux ovmf-fedora 4.7.0-rc2+ #7 SMP Fri Sep 30 12:05:35 CEST 2016 x86_64
[     9.978] Kernel command line: BOOT_IMAGE=/vmlinuz-4.7.0-rc2+ root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/swap vconsole.font=latarcyrheb-sun16 rd.lvm.lv=fedora/root rhgb quiet video=640x480 no_console_suspend console=tty console=ttyS0,115200 ignore_loglevel LANG=en_US.UTF-8
[     9.978] Build Date: 25 August 2016  05:27:31PM
[     9.978] Build ID: xorg-x11-server 1.18.4-4.fc24 
[     9.978] Current version of pixman: 0.34.0
[     9.978] 	Before reporting problems, check http://wiki.x.org
	to make sure that you have the latest version.
[     9.978] Markers: (--) probed, (**) from config file, (==) default setting,
	(++) from command line, (!!) notice, (II) informational,
	(WW) warning, (EE) error, (NI) not implemented, (??) unknown.
[     9.978] (==) Log file: "/var/log/Xorg.0.log", Time: Fri Sep 30 19:19:13 2016
[     9.978] (==) Using config directory: "/etc/X11/xorg.conf.d"
[     9.978] (==) Using system config directory "/usr/share/X11/xorg.conf.d"
[     9.979] (==) No Layout section.  Using the first Screen section.
[     9.979] (**) |-->Screen "Default Screen" (0)
[     9.979] (**) |   |-->Monitor "Default Monitor"
[     9.979] (**) |   |-->Device "Default Device"
[     9.979] (==) Automatically adding devices
[     9.979] (==) Automatically enabling devices
[     9.979] (==) Automatically adding GPU devices
[     9.979] (==) Max clients allowed: 256, resource mask: 0x1fffff
[     9.979] (==) FontPath set to:
	catalogue:/etc/X11/fontpath.d,
	built-ins
[     9.979] (==) ModulePath set to "/usr/lib64/xorg/modules"
[     9.979] (II) The server relies on udev to provide the list of input devices.
	If no devices become available, reconfigure udev or disable AutoAddDevices.
[     9.979] (II) Loader magic: 0x824dc0
[     9.979] (II) Module ABI versions:
[     9.979] 	X.Org ANSI C Emulation: 0.4
[     9.979] 	X.Org Video Driver: 20.0
[     9.979] 	X.Org XInput driver : 22.1
[     9.979] 	X.Org Server Extension : 9.0
[     9.980] (++) using VT number 1

[     9.980] (II) systemd-logind: logind integration requires -keeptty and -keeptty was not provided, disabling logind integration
[     9.981] (II) xfree86: Adding drm device (/dev/dri/card0)
[     9.986] (--) PCI:*(0:0:2:0) 1af4:1050:1af4:1100 rev 1, Mem @ 0xc0000000/8388608, 0x800000000/16384, 0xc0804000/4096, BIOS @ 0x????????/131072
[     9.986] (II) LoadModule: "glx"
[     9.986] (II) Loading /usr/lib64/xorg/modules/extensions/libglx.so
[     9.988] (II) Module glx: vendor="X.Org Foundation"
[     9.988] 	compiled for 1.18.4, module version = 1.0.0
[     9.988] 	ABI class: X.Org Server Extension, version 9.0
[     9.988] (==) AIGLX enabled
[     9.989] (II) LoadModule: "modesetting"
[     9.989] (II) Loading /usr/lib64/xorg/modules/drivers/modesetting_drv.so
[     9.989] (II) Module modesetting: vendor="X.Org Foundation"
[     9.989] 	compiled for 1.18.4, module version = 1.18.4
[     9.989] 	Module class: X.Org Video Driver
[     9.989] 	ABI class: X.Org Video Driver, version 20.0
[     9.989] (II) modesetting: Driver for Modesetting Kernel Drivers: kms
[     9.989] (EE) 
[     9.989] (EE) Backtrace:
[     9.989] (EE) 0: /usr/libexec/Xorg (OsLookupColor+0x139) [0x59f679]
[     9.990] (EE) 1: /lib64/libc.so.6 (__restore_rt+0x0) [0x7ffa90b2176f]
[     9.990] (EE) 2: /usr/libexec/Xorg (xf86PlatformDeviceCheckBusID+0xa7) [0x49cb87]
[     9.990] (EE) 3: /usr/libexec/Xorg (xf86PlatformMatchDriver+0x4dd) [0x49754d]
[     9.991] (EE) 4: /usr/libexec/Xorg (xf86CallDriverProbe+0xb0) [0x46e530]
[     9.991] (EE) 5: /usr/libexec/Xorg (xf86BusConfig+0x46) [0x46eea6]
[     9.991] (EE) 6: /usr/libexec/Xorg (InitOutput+0x97a) [0x47d52a]
[     9.991] (EE) 7: /usr/libexec/Xorg (remove_fs_handlers+0x266) [0x43ac66]
[     9.992] (EE) 8: /lib64/libc.so.6 (__libc_start_main+0xf1) [0x7ffa90b0d731]
[     9.992] (EE) 9: /usr/libexec/Xorg (_start+0x29) [0x424d59]
[     9.992] (EE) 10: ? (?+0x29) [0x29]
[     9.993] (EE) 
[     9.993] (EE) Segmentation fault at address 0x0
[     9.993] (EE) 
Fatal server error:
[     9.993] (EE) Caught signal 11 (Segmentation fault). Server aborting
[     9.993] (EE) 
[     9.993] (EE) 
Please consult the Fedora Project support 
	 at http://wiki.x.org
 for help. 
[     9.993] (EE) Please also check the log file at "/var/log/Xorg.0.log" for additional information.
[     9.993] (EE) 

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

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/30/16 18:38, Hans de Goede wrote:
>> Hi,
>>
>> On 30-09-16 17:33, Laszlo Ersek wrote:
>>> On 09/30/16 16:59, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-09-16 16:51, Laszlo Ersek wrote:
>>>>> On 09/30/16 12:35, Hans de Goede wrote:
>>>>>
>>>>>> Attached are 2 patches against the xserver which should fix this,
>>>>>> please give them a try.
>>>>>
>>>>> Sorry about the delay.
>>>>>
>>>>> The patches don't seem to fix the issue for me. Please see the Xorg log
>>>>> attached.
>>>>>
>>>>> I tested the patches as follows. Given that my bisection had been done
>>>>> in a Fedora 24 guest, using
>>>>>
>>>>>   xorg-x11-server-1.18.4-4.fc24
>>>>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>>>>
>>>>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>>>>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>>>>> reproduced the issue with the above X server.
>>>>>
>>>>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>>>>> xserver tree), and rebuilt the Fedora package with the backport. For
>>>>> the
>>>>> backport, I had to cherry-pick the following two patches from master
>>>>> first:
>>>>>
>>>>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>>>>                xf86IsPrimaryPlatform()
>>>>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>>
>>>>> This way your patches applied cleanly. (Cherry pick #1 above is
>>>>> actually
>>>>> necessary for semantics, while cherry pick #2 is needed for a clean
>>>>> context only, and has no impact for this test.)
>>>>>
>>>>> That is, in total, I added the following four patches to the Fedora 24
>>>>> package:
>>>>>
>>>>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>>>>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>>>>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>>>>
>>>>> You can find the scratch build that I used for testing here:
>>>>>
>>>>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>>>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>>>>
>>>>> Another reason I used F24's X server as basis, rather than upstream
>>>>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>>>>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>>>>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>>>>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>>>>> Fedora X maintainers would have to cherry pick those two patches as
>>>>> dependencies just the same.
>>>>>
>>>>> To summarize, the patches don't seem to help. I shall nonetheless thank
>>>>> you for spending your Friday on this!
>>>>
>>>> Hmm, do you have a xorg.conf file lying around somewhere, the message
>>>> about the xserver not being able to find an entry for screen 0 does
>>>> not make sense ...
>>>
>>> Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":
>>>
>>> * "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
>>> contents
>>>
>>> ------------
>>> # Read and parsed by systemd-localed. It's probably wise not to edit
>>> this file
>>> # manually too freely.
>>> Section "InputClass"
>>>         Identifier "system-keyboard"
>>>         MatchIsKeyboard "on"
>>>         Option "XkbLayout" "us"
>>> EndSection
>>> ------------
>>>
>>> * "01-resolution.conf", which I had created, in order to set the
>>> preferred display resolution:
>>>
>>> ------------
>>> Section "Screen"
>>>   Identifier "Default Screen"
>>>   Device     "Default Device"
>>>   Monitor    "Default Monitor"
>>> EndSection
>>>
>>> Section "Device"
>>>   Identifier "Default Device"
>>>   Driver     "modesetting"
>>> EndSection
>>>
>>> Section "Monitor"
>>>   Identifier "Default Monitor"
>>>   Option     "PreferredMode"   "640x480"
>>> # Option     "PreferredMode"   "1440x900"
>>> EndSection
>>> ------------
>>>
>>> I removed these files now, and repeated the test. Again, the X server
>>> wouldn't start, but I think the log file looks a bit different now.
>>> Attached.
>>
>> Ah, ok so it seems that my initial analysis is wrong, the problem
>> is not a re-occuring of the device getting identified as a GPU screen,
>> libdrm sorta depends on bus-ids and the lack of one is causing the
>> server to misbehave. I guess that even with a xorg.conf things
>> will fail with the troublesome kernel version (might be worth
>> trying).
>>
>> Emil's analysis seems to be spot on. This does not seem easily
>> fixable in userspace / does seem like a real regression as it
>> even breaks things when specifying the device through xorg.conf
>> (I or so I believe) which is something which uses to work ...
>
> In order to check this hypothesis, I did the following:
> - I downgraded my xorg-x11-server installation to the most recent
> official F24 packages, that is, "1.18.4-4.fc24",
> - I kept the kernel that I built exactly at the regressive commit
> (a325725633c2)
> - I modified "01-resolution.conf" (see it above in the context) like this:
>
> ----
> Section "Device"
>   Identifier "Default Device"
>   Driver     "modesetting"
>   BusID      "PCI:00:02:0" <------------ new option added
> EndSection
> ----
>
> where BusID matches the B/D/F of the virtio-vga device from "lspci".
>
> This setup (modulo the kernel of course) was known to work, but now the
> X server actually segfaults (apparently in the
> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>
> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
> it is left at its default "on" value.)
>
Where is this upstream commit again - it shows as unknown for the
kernel, xserver and libdrm ?

So my theory was a bit off - SetVersion is the one responsible to set
the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
open is used.

Here's a bit of a brain dump from the other day:

 - The commit mentioned 'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
userspace codepaths.
 - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
not functional for platform devices.
The latter of which seems to be the case for virt-gpu based on the
kernel module.
 - The modesetting driver should/cannot reach the above xserver codepath

That said, it seems that (at least some) userspace expects a PCI
device despite the kernel module 'advertising' itself as platform one
:-\

Going through the xserver layers is a bit inspiring I'm wondering if
we can not get a strace before/after the xserver commit
ca8d88e50310a0d440a127c22a0a383cc149f408 ? It will help us track
things a lot quicker/easier.


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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 13:34, Emil Velikov wrote:
> On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:

>> This setup (modulo the kernel of course) was known to work, but now the
>> X server actually segfaults (apparently in the
>> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>>
>> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
>> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
>> it is left at its default "on" value.)
>>
> Where is this upstream commit again - it shows as unknown for the
> kernel, xserver and libdrm ?

Apologies, that commit hash belongs to a cherry-picked patch on one of
my private branches; the original is

  ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID


> So my theory was a bit off - SetVersion is the one responsible to set
> the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
> open is used.
> 
> Here's a bit of a brain dump from the other day:
> 
>  - The commit mentioned

(Here I assume you mean kernel commit

  a325725633c2 drm: Lobotomize set_busid nonsense for !pci drivers

again.)

>'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
> userspace codepaths.
>  - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
> not functional for platform devices.
> The latter of which seems to be the case for virt-gpu based on the
> kernel module.
>  - The modesetting driver should/cannot reach the above xserver codepath
> 
> That said, it seems that (at least some) userspace expects a PCI
> device despite the kernel module 'advertising' itself as platform one
> :-\
> 
> Going through the xserver layers is a bit inspiring I'm wondering if
> we can not get a strace before/after the xserver commit
> ca8d88e50310a0d440a127c22a0a383cc149f408 ? It will help us track
> things a lot quicker/easier.

This commit:

  ca8d88e50310 xfree86: recognize primary BUS_PCI device in
               xf86IsPrimaryPlatform()

is not relevant when running Xorg on QEMU's "virtio-vga" device, it is
only relevant when the "virtio-gpu-pci" device is used. Aarch64 guests
can't have "virtio-vga", only "virgio-gpu-pci", so that commit was
needed for Aarch64 guests primarily.

In this case however, kernel commit a325725633c2 breaks "virtio-vga" in
x86_64 guests, both with and without xserver commit ca8d88e50310.

Anyway I'll look into capturing a strace and/or following Xorg with gdb.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

On 3 October 2016 at 13:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/03/16 13:34, Emil Velikov wrote:
>> On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>
>>> This setup (modulo the kernel of course) was known to work, but now the
>>> X server actually segfaults (apparently in the
>>> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>>>
>>> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
>>> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
>>> it is left at its default "on" value.)
>>>
>> Where is this upstream commit again - it shows as unknown for the
>> kernel, xserver and libdrm ?
>
> Apologies, that commit hash belongs to a cherry-picked patch on one of
> my private branches; the original is
>
>   ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>
>
>> So my theory was a bit off - SetVersion is the one responsible to set
>> the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
>> open is used.
>>
>> Here's a bit of a brain dump from the other day:
>>
>>  - The commit mentioned
>
> (Here I assume you mean kernel commit
>
>   a325725633c2 drm: Lobotomize set_busid nonsense for !pci drivers
>
> again.)
>
Correct.

>>'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
>> userspace codepaths.
>>  - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
>> not functional for platform devices.
>> The latter of which seems to be the case for virt-gpu based on the
>> kernel module.
>>  - The modesetting driver should/cannot reach the above xserver codepath
>>
>> That said, it seems that (at least some) userspace expects a PCI
>> device despite the kernel module 'advertising' itself as platform one
>> :-\
>>
>> Going through the xserver layers is a bit inspiring I'm wondering if
>> we can not get a strace before/after the xserver commit
>> ca8d88e50310a0d440a127c22a0a383cc149f408 ? It will help us track
>> things a lot quicker/easier.
>
> This commit:
>
>   ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>                xf86IsPrimaryPlatform()
>
> is not relevant when running Xorg on QEMU's "virtio-vga" device, it is
> only relevant when the "virtio-gpu-pci" device is used. Aarch64 guests
> can't have "virtio-vga", only "virgio-gpu-pci", so that commit was
> needed for Aarch64 guests primarily.
>
> In this case however, kernel commit a325725633c2 breaks "virtio-vga" in
> x86_64 guests, both with and without xserver commit ca8d88e50310.
>
> Anyway I'll look into capturing a strace and/or following Xorg with gdb.
>
If the overall case/issue is irrelevant of the xserver commit we can
ignore if for the time being.
In that case can you please get a xserver strace before/after the
offending kernel commit.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 14:46, Emil Velikov wrote:
> On 3 October 2016 at 13:14, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/03/16 13:34, Emil Velikov wrote:
>>> On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> This setup (modulo the kernel of course) was known to work, but now the
>>>> X server actually segfaults (apparently in the
>>>> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>>>>
>>>> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
>>>> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
>>>> it is left at its default "on" value.)
>>>>
>>> Where is this upstream commit again - it shows as unknown for the
>>> kernel, xserver and libdrm ?
>>
>> Apologies, that commit hash belongs to a cherry-picked patch on one of
>> my private branches; the original is
>>
>>   ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>
>>
>>> So my theory was a bit off - SetVersion is the one responsible to set
>>> the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
>>> open is used.
>>>
>>> Here's a bit of a brain dump from the other day:
>>>
>>>  - The commit mentioned
>>
>> (Here I assume you mean kernel commit
>>
>>   a325725633c2 drm: Lobotomize set_busid nonsense for !pci drivers
>>
>> again.)
>>
> Correct.
> 
>>> 'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
>>> userspace codepaths.
>>>  - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
>>> not functional for platform devices.
>>> The latter of which seems to be the case for virt-gpu based on the
>>> kernel module.
>>>  - The modesetting driver should/cannot reach the above xserver codepath
>>>
>>> That said, it seems that (at least some) userspace expects a PCI
>>> device despite the kernel module 'advertising' itself as platform one
>>> :-\
>>>
>>> Going through the xserver layers is a bit inspiring I'm wondering if
>>> we can not get a strace before/after the xserver commit
>>> ca8d88e50310a0d440a127c22a0a383cc149f408 ? It will help us track
>>> things a lot quicker/easier.
>>
>> This commit:
>>
>>   ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>                xf86IsPrimaryPlatform()
>>
>> is not relevant when running Xorg on QEMU's "virtio-vga" device, it is
>> only relevant when the "virtio-gpu-pci" device is used. Aarch64 guests
>> can't have "virtio-vga", only "virgio-gpu-pci", so that commit was
>> needed for Aarch64 guests primarily.
>>
>> In this case however, kernel commit a325725633c2 breaks "virtio-vga" in
>> x86_64 guests, both with and without xserver commit ca8d88e50310.
>>
>> Anyway I'll look into capturing a strace and/or following Xorg with gdb.
>>
> If the overall case/issue is irrelevant of the xserver commit we can
> ignore if for the time being.
> In that case can you please get a xserver strace before/after the
> offending kernel commit.

Please find them at <http://people.redhat.com/~lersek/set_busid/>.

Thanks!
Laszlo

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 13:34, Emil Velikov wrote:
> On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/30/16 18:38, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 30-09-16 17:33, Laszlo Ersek wrote:
>>>> On 09/30/16 16:59, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 30-09-16 16:51, Laszlo Ersek wrote:
>>>>>> On 09/30/16 12:35, Hans de Goede wrote:
>>>>>>
>>>>>>> Attached are 2 patches against the xserver which should fix this,
>>>>>>> please give them a try.
>>>>>>
>>>>>> Sorry about the delay.
>>>>>>
>>>>>> The patches don't seem to fix the issue for me. Please see the Xorg log
>>>>>> attached.
>>>>>>
>>>>>> I tested the patches as follows. Given that my bisection had been done
>>>>>> in a Fedora 24 guest, using
>>>>>>
>>>>>>   xorg-x11-server-1.18.4-4.fc24
>>>>>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>>>>>
>>>>>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>>>>>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>>>>>> reproduced the issue with the above X server.
>>>>>>
>>>>>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>>>>>> xserver tree), and rebuilt the Fedora package with the backport. For
>>>>>> the
>>>>>> backport, I had to cherry-pick the following two patches from master
>>>>>> first:
>>>>>>
>>>>>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>>>>>                xf86IsPrimaryPlatform()
>>>>>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>>>
>>>>>> This way your patches applied cleanly. (Cherry pick #1 above is
>>>>>> actually
>>>>>> necessary for semantics, while cherry pick #2 is needed for a clean
>>>>>> context only, and has no impact for this test.)
>>>>>>
>>>>>> That is, in total, I added the following four patches to the Fedora 24
>>>>>> package:
>>>>>>
>>>>>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>>>>>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>>>>>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>>>>>
>>>>>> You can find the scratch build that I used for testing here:
>>>>>>
>>>>>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>>>>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>>>>>
>>>>>> Another reason I used F24's X server as basis, rather than upstream
>>>>>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>>>>>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>>>>>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>>>>>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>>>>>> Fedora X maintainers would have to cherry pick those two patches as
>>>>>> dependencies just the same.
>>>>>>
>>>>>> To summarize, the patches don't seem to help. I shall nonetheless thank
>>>>>> you for spending your Friday on this!
>>>>>
>>>>> Hmm, do you have a xorg.conf file lying around somewhere, the message
>>>>> about the xserver not being able to find an entry for screen 0 does
>>>>> not make sense ...
>>>>
>>>> Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":
>>>>
>>>> * "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
>>>> contents
>>>>
>>>> ------------
>>>> # Read and parsed by systemd-localed. It's probably wise not to edit
>>>> this file
>>>> # manually too freely.
>>>> Section "InputClass"
>>>>         Identifier "system-keyboard"
>>>>         MatchIsKeyboard "on"
>>>>         Option "XkbLayout" "us"
>>>> EndSection
>>>> ------------
>>>>
>>>> * "01-resolution.conf", which I had created, in order to set the
>>>> preferred display resolution:
>>>>
>>>> ------------
>>>> Section "Screen"
>>>>   Identifier "Default Screen"
>>>>   Device     "Default Device"
>>>>   Monitor    "Default Monitor"
>>>> EndSection
>>>>
>>>> Section "Device"
>>>>   Identifier "Default Device"
>>>>   Driver     "modesetting"
>>>> EndSection
>>>>
>>>> Section "Monitor"
>>>>   Identifier "Default Monitor"
>>>>   Option     "PreferredMode"   "640x480"
>>>> # Option     "PreferredMode"   "1440x900"
>>>> EndSection
>>>> ------------
>>>>
>>>> I removed these files now, and repeated the test. Again, the X server
>>>> wouldn't start, but I think the log file looks a bit different now.
>>>> Attached.
>>>
>>> Ah, ok so it seems that my initial analysis is wrong, the problem
>>> is not a re-occuring of the device getting identified as a GPU screen,
>>> libdrm sorta depends on bus-ids and the lack of one is causing the
>>> server to misbehave. I guess that even with a xorg.conf things
>>> will fail with the troublesome kernel version (might be worth
>>> trying).
>>>
>>> Emil's analysis seems to be spot on. This does not seem easily
>>> fixable in userspace / does seem like a real regression as it
>>> even breaks things when specifying the device through xorg.conf
>>> (I or so I believe) which is something which uses to work ...
>>
>> In order to check this hypothesis, I did the following:
>> - I downgraded my xorg-x11-server installation to the most recent
>> official F24 packages, that is, "1.18.4-4.fc24",
>> - I kept the kernel that I built exactly at the regressive commit
>> (a325725633c2)
>> - I modified "01-resolution.conf" (see it above in the context) like this:
>>
>> ----
>> Section "Device"
>>   Identifier "Default Device"
>>   Driver     "modesetting"
>>   BusID      "PCI:00:02:0" <------------ new option added
>> EndSection
>> ----
>>
>> where BusID matches the B/D/F of the virtio-vga device from "lspci".
>>
>> This setup (modulo the kernel of course) was known to work, but now the
>> X server actually segfaults (apparently in the
>> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>>
>> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
>> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
>> it is left at its default "on" value.)
>>
> Where is this upstream commit again - it shows as unknown for the
> kernel, xserver and libdrm ?
> 
> So my theory was a bit off - SetVersion is the one responsible to set
> the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
> open is used.
> 
> Here's a bit of a brain dump from the other day:
> 
>  - The commit mentioned 'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
> userspace codepaths.
>  - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
> not functional for platform devices.
> The latter of which seems to be the case for virt-gpu based on the
> kernel module.
>  - The modesetting driver should/cannot reach the above xserver codepath
> 
> That said, it seems that (at least some) userspace expects a PCI
> device despite the kernel module 'advertising' itself as platform one
> :-\

With kernel commit a325725633c2 applied, the drmGetBusid() call in
get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
"virtio0".

Without kernel commit a325725633c2 in place, the same function call
produces "pci:0000:00:02.0".

I think that the idea of kernel commit a325725633c2:

    We already have a fallback in place to fill out the unique from
    dev->unique, which is set to something reasonable in drm_dev_alloc.

is inappropriate for virtio devices.

While it is true that "virtioN" is unique across the guest system, those
identifiers are not stable.

# ls -1d /sys/devices/pci*/*/virtio*
/sys/devices/pci0000:00/0000:00:02.0/virtio0
/sys/devices/pci0000:00/0000:00:03.0/virtio1
/sys/devices/pci0000:00/0000:00:05.0/virtio2
/sys/devices/pci0000:00/0000:00:06.0/virtio3
/sys/devices/pci0000:00/0000:00:09.0/virtio4

If you tweak the PCI addresses of other virtio devices on the QEMU
command line, while keeping the PCI address of the virtio-vga device
intact, the "virtioN" identifier of the virtio-vga device may change in
an unspecified / unexpected manner.

From the above listing, it seems like higher PCI Device numbers happen
to end up with higher "virtioN" identifiers. While this is unspecified /
non-guaranteed behavior, in practice it means the following:

Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
preserving the PCI address of virtio-vga as 00:02.0, will bump
virtio-vga to "virtio1" from "virtio0" (and sink that other device from
"virtio1" to "virtio0").

I think that unstable identifiers don't qualify for BusID use,
regardless of the actual format of the IDs in question.

Searching the patch quickly, drm_virtio_set_busid() is the only
implementation of the "drm_driver.set_busid" callback that delegates the
task to drm_pci_set_busid(). All other implementations of this callback
were provided by drm_platform_set_busid().

drm_platform_set_busid() would ultimately format
"dev->platformdev->name" and "dev->platformdev->id" into the bus
identifier. Replacing that common logic with the drm_dev_alloc()
fallback that is already in place is probably justified: judging from
"virtio0", the fallback likely captures the exact same information, just
with a different format.

However, drm_virtio_set_busid() used to implement a different logic
(encoding different information). It intentionally used stable PCI
addresses rather than (platformdev->name, platformdev->id).

So, I think that removing drm_virtio_set_busid() was an actual
regression. I propose that the related hunks of a325725633c2 be
reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
and inappropraite BusID pattern.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 16:15, Laszlo Ersek wrote:
> On 10/03/16 13:34, Emil Velikov wrote:
>> On 30 September 2016 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 09/30/16 18:38, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-09-16 17:33, Laszlo Ersek wrote:
>>>>> On 09/30/16 16:59, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 30-09-16 16:51, Laszlo Ersek wrote:
>>>>>>> On 09/30/16 12:35, Hans de Goede wrote:
>>>>>>>
>>>>>>>> Attached are 2 patches against the xserver which should fix this,
>>>>>>>> please give them a try.
>>>>>>>
>>>>>>> Sorry about the delay.
>>>>>>>
>>>>>>> The patches don't seem to fix the issue for me. Please see the Xorg log
>>>>>>> attached.
>>>>>>>
>>>>>>> I tested the patches as follows. Given that my bisection had been done
>>>>>>> in a Fedora 24 guest, using
>>>>>>>
>>>>>>>   xorg-x11-server-1.18.4-4.fc24
>>>>>>>   http://koji.fedoraproject.org/koji/buildinfo?buildID=794494
>>>>>>>
>>>>>>> I now rebuilt the guest kernel exactly at the failing commit (a325725
>>>>>>> "drm: Lobotomize set_busid nonsense for !pci drivers"), and first
>>>>>>> reproduced the issue with the above X server.
>>>>>>>
>>>>>>> Then, I ported your patches to "xorg-server-1.18.4" (using the upstream
>>>>>>> xserver tree), and rebuilt the Fedora package with the backport. For
>>>>>>> the
>>>>>>> backport, I had to cherry-pick the following two patches from master
>>>>>>> first:
>>>>>>>
>>>>>>> 1 ca8d88e50310 xfree86: recognize primary BUS_PCI device in
>>>>>>>                xf86IsPrimaryPlatform()
>>>>>>> 2 ea91db4b8331 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>>>>
>>>>>>> This way your patches applied cleanly. (Cherry pick #1 above is
>>>>>>> actually
>>>>>>> necessary for semantics, while cherry pick #2 is needed for a clean
>>>>>>> context only, and has no impact for this test.)
>>>>>>>
>>>>>>> That is, in total, I added the following four patches to the Fedora 24
>>>>>>> package:
>>>>>>>
>>>>>>> 1 xfree86: recognize primary BUS_PCI device in xf86IsPrimaryPlatform()
>>>>>>> 2 config: fix GPUDevice fail when AutoAddGPU off + BusID
>>>>>>> 3 xfree86: Make adding unclaimed devices as GPU devices a separate step
>>>>>>> 4 xfree86: Try harder to find atleast 1 non GPU Screen
>>>>>>>
>>>>>>> You can find the scratch build that I used for testing here:
>>>>>>>
>>>>>>>   xorg-x11-server-1.18.4-4.hans_bz1366842_2.fc24
>>>>>>>   http://koji.fedoraproject.org/koji/taskinfo?taskID=15875087
>>>>>>>
>>>>>>> Another reason I used F24's X server as basis, rather than upstream
>>>>>>> HEAD, is that Fedora 24 is pretty young, and it's already on kernel
>>>>>>> 4.7.4, and I believe it will soon move to kernel 4.8, without
>>>>>>> (necessarily) rebasing its X server package to upstream. IOW the kernel
>>>>>>> upgrade to 4.8 will break X in Fedora 24 too, and then I expect the
>>>>>>> Fedora X maintainers would have to cherry pick those two patches as
>>>>>>> dependencies just the same.
>>>>>>>
>>>>>>> To summarize, the patches don't seem to help. I shall nonetheless thank
>>>>>>> you for spending your Friday on this!
>>>>>>
>>>>>> Hmm, do you have a xorg.conf file lying around somewhere, the message
>>>>>> about the xserver not being able to find an entry for screen 0 does
>>>>>> not make sense ...
>>>>>
>>>>> Good catch, I actually had two files under "/etc/X11/xorg.conf.d/":
>>>>>
>>>>> * "00-keyboard.conf", from package "systemd-229-13.fc24.x86_64", with
>>>>> contents
>>>>>
>>>>> ------------
>>>>> # Read and parsed by systemd-localed. It's probably wise not to edit
>>>>> this file
>>>>> # manually too freely.
>>>>> Section "InputClass"
>>>>>         Identifier "system-keyboard"
>>>>>         MatchIsKeyboard "on"
>>>>>         Option "XkbLayout" "us"
>>>>> EndSection
>>>>> ------------
>>>>>
>>>>> * "01-resolution.conf", which I had created, in order to set the
>>>>> preferred display resolution:
>>>>>
>>>>> ------------
>>>>> Section "Screen"
>>>>>   Identifier "Default Screen"
>>>>>   Device     "Default Device"
>>>>>   Monitor    "Default Monitor"
>>>>> EndSection
>>>>>
>>>>> Section "Device"
>>>>>   Identifier "Default Device"
>>>>>   Driver     "modesetting"
>>>>> EndSection
>>>>>
>>>>> Section "Monitor"
>>>>>   Identifier "Default Monitor"
>>>>>   Option     "PreferredMode"   "640x480"
>>>>> # Option     "PreferredMode"   "1440x900"
>>>>> EndSection
>>>>> ------------
>>>>>
>>>>> I removed these files now, and repeated the test. Again, the X server
>>>>> wouldn't start, but I think the log file looks a bit different now.
>>>>> Attached.
>>>>
>>>> Ah, ok so it seems that my initial analysis is wrong, the problem
>>>> is not a re-occuring of the device getting identified as a GPU screen,
>>>> libdrm sorta depends on bus-ids and the lack of one is causing the
>>>> server to misbehave. I guess that even with a xorg.conf things
>>>> will fail with the troublesome kernel version (might be worth
>>>> trying).
>>>>
>>>> Emil's analysis seems to be spot on. This does not seem easily
>>>> fixable in userspace / does seem like a real regression as it
>>>> even breaks things when specifying the device through xorg.conf
>>>> (I or so I believe) which is something which uses to work ...
>>>
>>> In order to check this hypothesis, I did the following:
>>> - I downgraded my xorg-x11-server installation to the most recent
>>> official F24 packages, that is, "1.18.4-4.fc24",
>>> - I kept the kernel that I built exactly at the regressive commit
>>> (a325725633c2)
>>> - I modified "01-resolution.conf" (see it above in the context) like this:
>>>
>>> ----
>>> Section "Device"
>>>   Identifier "Default Device"
>>>   Driver     "modesetting"
>>>   BusID      "PCI:00:02:0" <------------ new option added
>>> EndSection
>>> ----
>>>
>>> where BusID matches the B/D/F of the virtio-vga device from "lspci".
>>>
>>> This setup (modulo the kernel of course) was known to work, but now the
>>> X server actually segfaults (apparently in the
>>> xf86PlatformDeviceCheckBusID() function). Please find the logfile attached.
>>>
>>> (NB: this is unrelated to upstream commit de9ce6757c2e -- which the
>>> pristine FC24 build lacks -- because I don't set AutoAddGPU to "off" --
>>> it is left at its default "on" value.)
>>>
>> Where is this upstream commit again - it shows as unknown for the
>> kernel, xserver and libdrm ?
>>
>> So my theory was a bit off - SetVersion is the one responsible to set
>> the "BusID", as retrieved by drmGetBusID, regardless if drmOpen or
>> open is used.
>>
>> Here's a bit of a brain dump from the other day:
>>
>>  - The commit mentioned 'affects' the drmSetBusid/DRM_IOCTL_SET_UNIQUE
>> userspace codepaths.
>>  - The latter itself is dri1/legacy (xserver hw/xfree86/dri/) which is
>> not functional for platform devices.
>> The latter of which seems to be the case for virt-gpu based on the
>> kernel module.
>>  - The modesetting driver should/cannot reach the above xserver codepath
>>
>> That said, it seems that (at least some) userspace expects a PCI
>> device despite the kernel module 'advertising' itself as platform one
>> :-\
> 
> With kernel commit a325725633c2 applied, the drmGetBusid() call in
> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
> "virtio0".
> 
> Without kernel commit a325725633c2 in place, the same function call
> produces "pci:0000:00:02.0".
> 
> I think that the idea of kernel commit a325725633c2:
> 
>     We already have a fallback in place to fill out the unique from
>     dev->unique, which is set to something reasonable in drm_dev_alloc.
> 
> is inappropriate for virtio devices.
> 
> While it is true that "virtioN" is unique across the guest system, those
> identifiers are not stable.
> 
> # ls -1d /sys/devices/pci*/*/virtio*
> /sys/devices/pci0000:00/0000:00:02.0/virtio0
> /sys/devices/pci0000:00/0000:00:03.0/virtio1
> /sys/devices/pci0000:00/0000:00:05.0/virtio2
> /sys/devices/pci0000:00/0000:00:06.0/virtio3
> /sys/devices/pci0000:00/0000:00:09.0/virtio4
> 
> If you tweak the PCI addresses of other virtio devices on the QEMU
> command line, while keeping the PCI address of the virtio-vga device
> intact, the "virtioN" identifier of the virtio-vga device may change in
> an unspecified / unexpected manner.
> 
> From the above listing, it seems like higher PCI Device numbers happen
> to end up with higher "virtioN" identifiers. While this is unspecified /
> non-guaranteed behavior, in practice it means the following:
> 
> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
> preserving the PCI address of virtio-vga as 00:02.0, will bump
> virtio-vga to "virtio1" from "virtio0" (and sink that other device from
> "virtio1" to "virtio0").
> 
> I think that unstable identifiers don't qualify for BusID use,
> regardless of the actual format of the IDs in question.
> 
> Searching the patch quickly, drm_virtio_set_busid() is the only
> implementation of the "drm_driver.set_busid" callback that delegates the
> task to drm_pci_set_busid(). All other implementations of this callback
> were provided by drm_platform_set_busid().
> 
> drm_platform_set_busid() would ultimately format
> "dev->platformdev->name" and "dev->platformdev->id" into the bus
> identifier. Replacing that common logic with the drm_dev_alloc()
> fallback that is already in place is probably justified: judging from
> "virtio0", the fallback likely captures the exact same information, just
> with a different format.
> 
> However, drm_virtio_set_busid() used to implement a different logic
> (encoding different information). It intentionally used stable PCI
> addresses rather than (platformdev->name, platformdev->id).
> 
> So, I think that removing drm_virtio_set_busid() was an actual
> regression. I propose that the related hunks of a325725633c2 be
> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
> and inappropraite BusID pattern.

In support of my argument, please see the following remark in commit
message:

    v4: Drop accidental amdgpu hunk (Emil).

Now, if you look up the v3 posting for that amdgpu hunk:

  https://lists.freedesktop.org/archives/dri-devel/2016-June/111200.html
  https://lists.freedesktop.org/archives/dri-devel/2016-June/111486.html

the v3 patch accidentally removed the similarly customized set_busid
callback for amdgpu -- drm_pci_set_busid(). Emil caught that error in
review, hence the v4 patch wouldn't contain the same error.

My argument is exactly the same -- just as the amdgpu hunk shouldn't be
in the patch, the drm_virtio_set_busid() hunk shouldn't be there either.
Both amdgpu and virtio-gpu implement a set_busid callback -- by
delegating to drm_pci_set_busid() -- that *cannot* be replaced with the
fallback in drm_dev_alloc().

Please revert the drm_virtio_set_busid() part of kernel commit
a325725633c2; at this point I'm 95% sure it was an oversight that didn't
get caught in review.

The rest of kernel commit a325725633c2 looks sane to me; while it
changes the formatting of the busid for the affected platform devices,
relying on the fallback in those cases actually preserves the same
information content. (This is not true for amdgpu and virtio-gpu.)

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

On 3 October 2016 at 15:27, Laszlo Ersek <lersek@redhat.com> wrote:
[snip]
> the v3 patch accidentally removed the similarly customized set_busid
> callback for amdgpu -- drm_pci_set_busid(). Emil caught that error in
> review, hence the v4 patch wouldn't contain the same error.
>
You're spot on - virtio-gpu doesn't use drm_*platform*_set_busid,
despite that I've misread it as such for over a dozen times. So yes,
that hunk should not have been removed.

Lacking a git checkout on this system, so if anyone beats me to it and
sends a patch that'll be great.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 17:00, Emil Velikov wrote:
> On 3 October 2016 at 15:27, Laszlo Ersek <lersek@redhat.com> wrote:
> [snip]
>> the v3 patch accidentally removed the similarly customized set_busid
>> callback for amdgpu -- drm_pci_set_busid(). Emil caught that error in
>> review, hence the v4 patch wouldn't contain the same error.
>>
> You're spot on - virtio-gpu doesn't use drm_*platform*_set_busid,
> despite that I've misread it as such for over a dozen times. So yes,
> that hunk should not have been removed.
> 
> Lacking a git checkout on this system, so if anyone beats me to it and
> sends a patch that'll be great.

I'll send a patch ASAP.

Thank you
Laszlo

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Daniel Vetter]

On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> With kernel commit a325725633c2 applied, the drmGetBusid() call in
> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
> "virtio0".
>
> Without kernel commit a325725633c2 in place, the same function call
> produces "pci:0000:00:02.0".
>
> I think that the idea of kernel commit a325725633c2:
>
>     We already have a fallback in place to fill out the unique from
>     dev->unique, which is set to something reasonable in drm_dev_alloc.
>
> is inappropriate for virtio devices.
>
> While it is true that "virtioN" is unique across the guest system, those
> identifiers are not stable.
>
> # ls -1d /sys/devices/pci*/*/virtio*
> /sys/devices/pci0000:00/0000:00:02.0/virtio0
> /sys/devices/pci0000:00/0000:00:03.0/virtio1
> /sys/devices/pci0000:00/0000:00:05.0/virtio2
> /sys/devices/pci0000:00/0000:00:06.0/virtio3
> /sys/devices/pci0000:00/0000:00:09.0/virtio4
>
> If you tweak the PCI addresses of other virtio devices on the QEMU
> command line, while keeping the PCI address of the virtio-vga device
> intact, the "virtioN" identifier of the virtio-vga device may change in
> an unspecified / unexpected manner.
>
> From the above listing, it seems like higher PCI Device numbers happen
> to end up with higher "virtioN" identifiers. While this is unspecified /
> non-guaranteed behavior, in practice it means the following:
>
> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
> preserving the PCI address of virtio-vga as 00:02.0, will bump
> virtio-vga to "virtio1" from "virtio0" (and sink that other device from
> "virtio1" to "virtio0").
>
> I think that unstable identifiers don't qualify for BusID use,
> regardless of the actual format of the IDs in question.
>
> Searching the patch quickly, drm_virtio_set_busid() is the only
> implementation of the "drm_driver.set_busid" callback that delegates the
> task to drm_pci_set_busid(). All other implementations of this callback
> were provided by drm_platform_set_busid().
>
> drm_platform_set_busid() would ultimately format
> "dev->platformdev->name" and "dev->platformdev->id" into the bus
> identifier. Replacing that common logic with the drm_dev_alloc()
> fallback that is already in place is probably justified: judging from
> "virtio0", the fallback likely captures the exact same information, just
> with a different format.
>
> However, drm_virtio_set_busid() used to implement a different logic
> (encoding different information). It intentionally used stable PCI
> addresses rather than (platformdev->name, platformdev->id).
>
> So, I think that removing drm_virtio_set_busid() was an actual
> regression. I propose that the related hunks of a325725633c2 be
> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
> and inappropraite BusID pattern.

Jumping in a bit late here, and maybe I'm not coherent (flu and all
that), but I'd still like to nuke the set_busid callback for virtio.
The trouble here seems to be that virtio has a bit a confusion about
what it considers to be the parent device it wants to expose to
userspace. And userspace is less than clueful about walking up the
device hierarchy to figure this out on its own.

Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have
a special case for when we're on a PCI bus. I think the better way to
fix this issue would be to:
- again export drm_drv_set_unique to drivers
- overwrite drm's choice of unique identifier with a call to

drm_dev_set_unique(dev, dev->pdev);

in that if block. With a very big comment that this only exists for
backwards compat with userspace's expectations.

Since virtio is newer than drm1.1 we don't need to care about the
backwards compat cruft in the pci set_busid function, and it would be
great to restrict that as much as possible imo. Something like the
below essentially (entirely untested):

diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
index a59d0e309bfc..1fcf739bf509 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
@@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
struct virtio_device *vdev)
                dev->pdev = pdev;
                if (vga)
                        virtio_pci_kick_out_firmware_fb(pdev);
+
+               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
+               if (ret)
+                       goto err_free;
        }

        ret = drm_dev_register(dev, 0);

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 21:12, Daniel Vetter wrote:
> On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> With kernel commit a325725633c2 applied, the drmGetBusid() call in
>> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
>> "virtio0".
>>
>> Without kernel commit a325725633c2 in place, the same function call
>> produces "pci:0000:00:02.0".
>>
>> I think that the idea of kernel commit a325725633c2:
>>
>>     We already have a fallback in place to fill out the unique from
>>     dev->unique, which is set to something reasonable in drm_dev_alloc.
>>
>> is inappropriate for virtio devices.
>>
>> While it is true that "virtioN" is unique across the guest system, those
>> identifiers are not stable.
>>
>> # ls -1d /sys/devices/pci*/*/virtio*
>> /sys/devices/pci0000:00/0000:00:02.0/virtio0
>> /sys/devices/pci0000:00/0000:00:03.0/virtio1
>> /sys/devices/pci0000:00/0000:00:05.0/virtio2
>> /sys/devices/pci0000:00/0000:00:06.0/virtio3
>> /sys/devices/pci0000:00/0000:00:09.0/virtio4
>>
>> If you tweak the PCI addresses of other virtio devices on the QEMU
>> command line, while keeping the PCI address of the virtio-vga device
>> intact, the "virtioN" identifier of the virtio-vga device may change in
>> an unspecified / unexpected manner.
>>
>> From the above listing, it seems like higher PCI Device numbers happen
>> to end up with higher "virtioN" identifiers. While this is unspecified /
>> non-guaranteed behavior, in practice it means the following:
>>
>> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
>> preserving the PCI address of virtio-vga as 00:02.0, will bump
>> virtio-vga to "virtio1" from "virtio0" (and sink that other device from
>> "virtio1" to "virtio0").
>>
>> I think that unstable identifiers don't qualify for BusID use,
>> regardless of the actual format of the IDs in question.
>>
>> Searching the patch quickly, drm_virtio_set_busid() is the only
>> implementation of the "drm_driver.set_busid" callback that delegates the
>> task to drm_pci_set_busid(). All other implementations of this callback
>> were provided by drm_platform_set_busid().
>>
>> drm_platform_set_busid() would ultimately format
>> "dev->platformdev->name" and "dev->platformdev->id" into the bus
>> identifier. Replacing that common logic with the drm_dev_alloc()
>> fallback that is already in place is probably justified: judging from
>> "virtio0", the fallback likely captures the exact same information, just
>> with a different format.
>>
>> However, drm_virtio_set_busid() used to implement a different logic
>> (encoding different information). It intentionally used stable PCI
>> addresses rather than (platformdev->name, platformdev->id).
>>
>> So, I think that removing drm_virtio_set_busid() was an actual
>> regression. I propose that the related hunks of a325725633c2 be
>> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
>> and inappropraite BusID pattern.
> 
> Jumping in a bit late here, and maybe I'm not coherent (flu and all
> that), but I'd still like to nuke the set_busid callback for virtio.
> The trouble here seems to be that virtio has a bit a confusion about
> what it considers to be the parent device it wants to expose to
> userspace. And userspace is less than clueful about walking up the
> device hierarchy to figure this out on its own.
> 
> Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have
> a special case for when we're on a PCI bus. I think the better way to
> fix this issue would be to:
> - again export drm_drv_set_unique to drivers
> - overwrite drm's choice of unique identifier with a call to
> 
> drm_dev_set_unique(dev, dev->pdev);
> 
> in that if block. With a very big comment that this only exists for
> backwards compat with userspace's expectations.
> 
> Since virtio is newer than drm1.1 we don't need to care about the
> backwards compat cruft in the pci set_busid function, and it would be
> great to restrict that as much as possible imo. Something like the
> below essentially (entirely untested):
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> index a59d0e309bfc..1fcf739bf509 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> struct virtio_device *vdev)
>                 dev->pdev = pdev;
>                 if (vga)
>                         virtio_pci_kick_out_firmware_fb(pdev);
> +
> +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> +               if (ret)
> +                       goto err_free;
>         }
> 
>         ret = drm_dev_register(dev, 0);
> 
> Cheers, Daniel


On the flu front: thank you for taking the time to think about this and
to respond despite being ill. Get better!

On the patch front: normally I wouldn't oppose experimentation and
shooting untested patches to users for testing -- that's okay in my
opinion. However, in this case the original patch regressed userspace
*first*; we know the culprit hunks, and those hunks can be very easily
reverted without dropping the entire patch.

Considering Emil's feedback, he wouldn't have let the patch
(a325725633c2) through review if he had noticed that hunk -- similarly
to the amdgpu hunk, which he did notice, in v3.

I think that experimentation, and asking users to test on (virtual)
hardware that is not easily available to the patch submitter, are all
fine -- as long as the public tree is in working shape. Currently, it is
not; with the above in mind, I strongly prefer the (partial) revert.

If you'd like to slay the remaining drm_pci_set_busid() calls, which
affect both virtio-gpu and amdgpu, that could be an entirely justified
change, a 100% improvement, and I'll be more than happy to assist with
testing that (the virtio-gpu case at least) -- *after* the tree is
returned to working shape (including stable), now that we're able to do
that.

Personally, I'm not a DRM "expert" by any means -- this is actually the
first thread on dri-devel that I've even read --, and this is about the
time I can squeeze out right now for this issue. I should help with the
testing as stated above, but we also should have a more convenient
schedule for that testing.

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Daniel Vetter]

On Mon, Oct 3, 2016 at 9:36 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/03/16 21:12, Daniel Vetter wrote:
>> On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> With kernel commit a325725633c2 applied, the drmGetBusid() call in
>>> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
>>> "virtio0".
>>>
>>> Without kernel commit a325725633c2 in place, the same function call
>>> produces "pci:0000:00:02.0".
>>>
>>> I think that the idea of kernel commit a325725633c2:
>>>
>>>     We already have a fallback in place to fill out the unique from
>>>     dev->unique, which is set to something reasonable in drm_dev_alloc.
>>>
>>> is inappropriate for virtio devices.
>>>
>>> While it is true that "virtioN" is unique across the guest system, those
>>> identifiers are not stable.
>>>
>>> # ls -1d /sys/devices/pci*/*/virtio*
>>> /sys/devices/pci0000:00/0000:00:02.0/virtio0
>>> /sys/devices/pci0000:00/0000:00:03.0/virtio1
>>> /sys/devices/pci0000:00/0000:00:05.0/virtio2
>>> /sys/devices/pci0000:00/0000:00:06.0/virtio3
>>> /sys/devices/pci0000:00/0000:00:09.0/virtio4
>>>
>>> If you tweak the PCI addresses of other virtio devices on the QEMU
>>> command line, while keeping the PCI address of the virtio-vga device
>>> intact, the "virtioN" identifier of the virtio-vga device may change in
>>> an unspecified / unexpected manner.
>>>
>>> From the above listing, it seems like higher PCI Device numbers happen
>>> to end up with higher "virtioN" identifiers. While this is unspecified /
>>> non-guaranteed behavior, in practice it means the following:
>>>
>>> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
>>> preserving the PCI address of virtio-vga as 00:02.0, will bump
>>> virtio-vga to "virtio1" from "virtio0" (and sink that other device from
>>> "virtio1" to "virtio0").
>>>
>>> I think that unstable identifiers don't qualify for BusID use,
>>> regardless of the actual format of the IDs in question.
>>>
>>> Searching the patch quickly, drm_virtio_set_busid() is the only
>>> implementation of the "drm_driver.set_busid" callback that delegates the
>>> task to drm_pci_set_busid(). All other implementations of this callback
>>> were provided by drm_platform_set_busid().
>>>
>>> drm_platform_set_busid() would ultimately format
>>> "dev->platformdev->name" and "dev->platformdev->id" into the bus
>>> identifier. Replacing that common logic with the drm_dev_alloc()
>>> fallback that is already in place is probably justified: judging from
>>> "virtio0", the fallback likely captures the exact same information, just
>>> with a different format.
>>>
>>> However, drm_virtio_set_busid() used to implement a different logic
>>> (encoding different information). It intentionally used stable PCI
>>> addresses rather than (platformdev->name, platformdev->id).
>>>
>>> So, I think that removing drm_virtio_set_busid() was an actual
>>> regression. I propose that the related hunks of a325725633c2 be
>>> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
>>> and inappropraite BusID pattern.
>>
>> Jumping in a bit late here, and maybe I'm not coherent (flu and all
>> that), but I'd still like to nuke the set_busid callback for virtio.
>> The trouble here seems to be that virtio has a bit a confusion about
>> what it considers to be the parent device it wants to expose to
>> userspace. And userspace is less than clueful about walking up the
>> device hierarchy to figure this out on its own.
>>
>> Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have
>> a special case for when we're on a PCI bus. I think the better way to
>> fix this issue would be to:
>> - again export drm_drv_set_unique to drivers
>> - overwrite drm's choice of unique identifier with a call to
>>
>> drm_dev_set_unique(dev, dev->pdev);
>>
>> in that if block. With a very big comment that this only exists for
>> backwards compat with userspace's expectations.
>>
>> Since virtio is newer than drm1.1 we don't need to care about the
>> backwards compat cruft in the pci set_busid function, and it would be
>> great to restrict that as much as possible imo. Something like the
>> below essentially (entirely untested):
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>> index a59d0e309bfc..1fcf739bf509 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
>> struct virtio_device *vdev)
>>                 dev->pdev = pdev;
>>                 if (vga)
>>                         virtio_pci_kick_out_firmware_fb(pdev);
>> +
>> +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
>> +               if (ret)
>> +                       goto err_free;
>>         }
>>
>>         ret = drm_dev_register(dev, 0);
>>
>> Cheers, Daniel
>
>
> On the flu front: thank you for taking the time to think about this and
> to respond despite being ill. Get better!
>
> On the patch front: normally I wouldn't oppose experimentation and
> shooting untested patches to users for testing -- that's okay in my
> opinion. However, in this case the original patch regressed userspace
> *first*; we know the culprit hunks, and those hunks can be very easily
> reverted without dropping the entire patch.
>
> Considering Emil's feedback, he wouldn't have let the patch
> (a325725633c2) through review if he had noticed that hunk -- similarly
> to the amdgpu hunk, which he did notice, in v3.
>
> I think that experimentation, and asking users to test on (virtual)
> hardware that is not easily available to the patch submitter, are all
> fine -- as long as the public tree is in working shape. Currently, it is
> not; with the above in mind, I strongly prefer the (partial) revert.
>
> If you'd like to slay the remaining drm_pci_set_busid() calls, which
> affect both virtio-gpu and amdgpu, that could be an entirely justified
> change, a 100% improvement, and I'll be more than happy to assist with
> testing that (the virtio-gpu case at least) -- *after* the tree is
> returned to working shape (including stable), now that we're able to do
> that.
>
> Personally, I'm not a DRM "expert" by any means -- this is actually the
> first thread on dri-devel that I've even read --, and this is about the
> time I can squeeze out right now for this issue. I should help with the
> testing as stated above, but we also should have a more convenient
> schedule for that testing.

I didn't say I'm blocking your fix for this, nor did I want to imply
that. It's just been my experience that bug reporters tend to
disappear fast. So if you have time, please test the above snippet
(without your patch of course). If you don't, no harm done, since I've
been trying to untangle the busid mess since years, so a few more
won't matter ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Laszlo Ersek]

On 10/03/16 22:34, Daniel Vetter wrote:
> On Mon, Oct 3, 2016 at 9:36 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/03/16 21:12, Daniel Vetter wrote:
>>> On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> With kernel commit a325725633c2 applied, the drmGetBusid() call in
>>>> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns
>>>> "virtio0".
>>>>
>>>> Without kernel commit a325725633c2 in place, the same function call
>>>> produces "pci:0000:00:02.0".
>>>>
>>>> I think that the idea of kernel commit a325725633c2:
>>>>
>>>>     We already have a fallback in place to fill out the unique from
>>>>     dev->unique, which is set to something reasonable in drm_dev_alloc.
>>>>
>>>> is inappropriate for virtio devices.
>>>>
>>>> While it is true that "virtioN" is unique across the guest system, those
>>>> identifiers are not stable.
>>>>
>>>> # ls -1d /sys/devices/pci*/*/virtio*
>>>> /sys/devices/pci0000:00/0000:00:02.0/virtio0
>>>> /sys/devices/pci0000:00/0000:00:03.0/virtio1
>>>> /sys/devices/pci0000:00/0000:00:05.0/virtio2
>>>> /sys/devices/pci0000:00/0000:00:06.0/virtio3
>>>> /sys/devices/pci0000:00/0000:00:09.0/virtio4
>>>>
>>>> If you tweak the PCI addresses of other virtio devices on the QEMU
>>>> command line, while keeping the PCI address of the virtio-vga device
>>>> intact, the "virtioN" identifier of the virtio-vga device may change in
>>>> an unspecified / unexpected manner.
>>>>
>>>> From the above listing, it seems like higher PCI Device numbers happen
>>>> to end up with higher "virtioN" identifiers. While this is unspecified /
>>>> non-guaranteed behavior, in practice it means the following:
>>>>
>>>> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while
>>>> preserving the PCI address of virtio-vga as 00:02.0, will bump
>>>> virtio-vga to "virtio1" from "virtio0" (and sink that other device from
>>>> "virtio1" to "virtio0").
>>>>
>>>> I think that unstable identifiers don't qualify for BusID use,
>>>> regardless of the actual format of the IDs in question.
>>>>
>>>> Searching the patch quickly, drm_virtio_set_busid() is the only
>>>> implementation of the "drm_driver.set_busid" callback that delegates the
>>>> task to drm_pci_set_busid(). All other implementations of this callback
>>>> were provided by drm_platform_set_busid().
>>>>
>>>> drm_platform_set_busid() would ultimately format
>>>> "dev->platformdev->name" and "dev->platformdev->id" into the bus
>>>> identifier. Replacing that common logic with the drm_dev_alloc()
>>>> fallback that is already in place is probably justified: judging from
>>>> "virtio0", the fallback likely captures the exact same information, just
>>>> with a different format.
>>>>
>>>> However, drm_virtio_set_busid() used to implement a different logic
>>>> (encoding different information). It intentionally used stable PCI
>>>> addresses rather than (platformdev->name, platformdev->id).
>>>>
>>>> So, I think that removing drm_virtio_set_busid() was an actual
>>>> regression. I propose that the related hunks of a325725633c2 be
>>>> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable
>>>> and inappropraite BusID pattern.
>>>
>>> Jumping in a bit late here, and maybe I'm not coherent (flu and all
>>> that), but I'd still like to nuke the set_busid callback for virtio.
>>> The trouble here seems to be that virtio has a bit a confusion about
>>> what it considers to be the parent device it wants to expose to
>>> userspace. And userspace is less than clueful about walking up the
>>> device hierarchy to figure this out on its own.
>>>
>>> Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have
>>> a special case for when we're on a PCI bus. I think the better way to
>>> fix this issue would be to:
>>> - again export drm_drv_set_unique to drivers
>>> - overwrite drm's choice of unique identifier with a call to
>>>
>>> drm_dev_set_unique(dev, dev->pdev);
>>>
>>> in that if block. With a very big comment that this only exists for
>>> backwards compat with userspace's expectations.
>>>
>>> Since virtio is newer than drm1.1 we don't need to care about the
>>> backwards compat cruft in the pci set_busid function, and it would be
>>> great to restrict that as much as possible imo. Something like the
>>> below essentially (entirely untested):
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>>> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>>> index a59d0e309bfc..1fcf739bf509 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
>>> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
>>> struct virtio_device *vdev)
>>>                 dev->pdev = pdev;
>>>                 if (vga)
>>>                         virtio_pci_kick_out_firmware_fb(pdev);
>>> +
>>> +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
>>> +               if (ret)
>>> +                       goto err_free;
>>>         }
>>>
>>>         ret = drm_dev_register(dev, 0);
>>>
>>> Cheers, Daniel
>>
>>
>> On the flu front: thank you for taking the time to think about this and
>> to respond despite being ill. Get better!
>>
>> On the patch front: normally I wouldn't oppose experimentation and
>> shooting untested patches to users for testing -- that's okay in my
>> opinion. However, in this case the original patch regressed userspace
>> *first*; we know the culprit hunks, and those hunks can be very easily
>> reverted without dropping the entire patch.
>>
>> Considering Emil's feedback, he wouldn't have let the patch
>> (a325725633c2) through review if he had noticed that hunk -- similarly
>> to the amdgpu hunk, which he did notice, in v3.
>>
>> I think that experimentation, and asking users to test on (virtual)
>> hardware that is not easily available to the patch submitter, are all
>> fine -- as long as the public tree is in working shape. Currently, it is
>> not; with the above in mind, I strongly prefer the (partial) revert.
>>
>> If you'd like to slay the remaining drm_pci_set_busid() calls, which
>> affect both virtio-gpu and amdgpu, that could be an entirely justified
>> change, a 100% improvement, and I'll be more than happy to assist with
>> testing that (the virtio-gpu case at least) -- *after* the tree is
>> returned to working shape (including stable), now that we're able to do
>> that.
>>
>> Personally, I'm not a DRM "expert" by any means -- this is actually the
>> first thread on dri-devel that I've even read --, and this is about the
>> time I can squeeze out right now for this issue. I should help with the
>> testing as stated above, but we also should have a more convenient
>> schedule for that testing.
> 
> I didn't say I'm blocking your fix for this, nor did I want to imply
> that. It's just been my experience that bug reporters tend to
> disappear fast.

Valid experience. Doesn't apply to me.

> So if you have time, please test the above snippet
> (without your patch of course). If you don't, no harm done, since I've
> been trying to untangle the busid mess since years, so a few more
> won't matter ;-)

Can you please send the patch in a form that I can apply with git-am,
and ensure that it will build? (I assume "entirely untested" implies
"not build tested".) I think a patch attachment on-list, or an inline
patch sent to me privately should be fine (so as not to confuse
subscribers and/or any patch bot / patchwork instance scraping the
list). I guess a threaded message with a subject-prefix different from
[PATCH] might work as well.

(It's not that I'm too lazy to re-code your line-wrapped patch manually
-- instead, you want me to interfere with your patch before testing it
as little as possible. Git-am or even a fetchable remote are best for that.)

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Gerd Hoffmann]

  Hi,

> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> index a59d0e309bfc..1fcf739bf509 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> struct virtio_device *vdev)
>                 dev->pdev = pdev;
>                 if (vga)
>                         virtio_pci_kick_out_firmware_fb(pdev);
> +
> +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> +               if (ret)
> +                       goto err_free;
>         }

Approach looks sensible to me, I'll give it a try ...

cheers,
  Gerd

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Gerd Hoffmann]

On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > index a59d0e309bfc..1fcf739bf509 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > struct virtio_device *vdev)
> >                 dev->pdev = pdev;
> >                 if (vga)
> >                         virtio_pci_kick_out_firmware_fb(pdev);
> > +
> > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > +               if (ret)
> > +                       goto err_free;
> >         }
> 
> Approach looks sensible to me, I'll give it a try ...

Well, dev_name() returns a string without the "pci:" prefix, we have to
add that to make things actually work, like this:

--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
@@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
struct virtio_device *vdev)
 
        if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
                struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
+               const char *pname = dev_name(&pdev->dev);
                bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
+               char unique[20];
 
-               DRM_INFO("pci: %s detected\n",
-                        vga ? "virtio-vga" : "virtio-gpu-pci");
+               DRM_INFO("pci: %s detected at %s\n",
+                        vga ? "virtio-vga" : "virtio-gpu-pci",
+                        pname);
                dev->pdev = pdev;
                if (vga)
                        virtio_pci_kick_out_firmware_fb(pdev);
+
+               snprintf(unique, sizeof(unique), "pci:%s", pname);
+               ret = drm_dev_set_unique(dev, unique);
+               if (ret)
+                       goto err_free;
+
        }
 
        ret = drm_dev_register(dev, 0);

And a partial revert of commit a742946a1ba57e24e8be205ea87224c05b38c380
to re-export drm_dev_set_unique().

Pushed to git://git.kraxel.org/linux drm-qemu

cheers,
  Gerd

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Daniel Vetter]

On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > index a59d0e309bfc..1fcf739bf509 100644
> > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > struct virtio_device *vdev)
> > >                 dev->pdev = pdev;
> > >                 if (vga)
> > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > +
> > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > +               if (ret)
> > > +                       goto err_free;
> > >         }
> > 
> > Approach looks sensible to me, I'll give it a try ...
> 
> Well, dev_name() returns a string without the "pci:" prefix, we have to
> add that to make things actually work, like this:

Hm right, I missed that in digging through the piles of layers of struct
device naming.

> 
> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> struct virtio_device *vdev)
>  
>         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
>                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> +               const char *pname = dev_name(&pdev->dev);
>                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> +               char unique[20];

Iirc we kfree this on unload. Needs to be a kasprintf I think.
>  
> -               DRM_INFO("pci: %s detected\n",
> -                        vga ? "virtio-vga" : "virtio-gpu-pci");
> +               DRM_INFO("pci: %s detected at %s\n",
> +                        vga ? "virtio-vga" : "virtio-gpu-pci",
> +                        pname);
>                 dev->pdev = pdev;
>                 if (vga)
>                         virtio_pci_kick_out_firmware_fb(pdev);
> +
> +               snprintf(unique, sizeof(unique), "pci:%s", pname);
> +               ret = drm_dev_set_unique(dev, unique);
> +               if (ret)
> +                       goto err_free;
> +

I think once we can nuke the pci_setbusid stuff we might want a
drm_dev_pci_set_unique, which takes the struct pci_device and wraps the
above magic. But that can wait, we still have a few years on the uabi
clock for that.

Strictly speaking this is an issue for virtgpu too, but since virtgpu is
newer than the last broken libdrm release I think we'll be fine.

With the kasprintf change above:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>         }
>  
>         ret = drm_dev_register(dev, 0);
> 
> And a partial revert of commit a742946a1ba57e24e8be205ea87224c05b38c380
> to re-export drm_dev_set_unique().
> 
> Pushed to git://git.kraxel.org/linux drm-qemu
> 
> cheers,
>   Gerd
> 

-- 
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: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Gerd Hoffmann]

On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote:
> On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > index a59d0e309bfc..1fcf739bf509 100644
> > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > > struct virtio_device *vdev)
> > > >                 dev->pdev = pdev;
> > > >                 if (vga)
> > > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > > +
> > > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > > +               if (ret)
> > > > +                       goto err_free;
> > > >         }
> > > 
> > > Approach looks sensible to me, I'll give it a try ...
> > 
> > Well, dev_name() returns a string without the "pci:" prefix, we have to
> > add that to make things actually work, like this:
> 
> Hm right, I missed that in digging through the piles of layers of struct
> device naming.
> 
> > 
> > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> > struct virtio_device *vdev)
> >  
> >         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
> >                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> > +               const char *pname = dev_name(&pdev->dev);
> >                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > +               char unique[20];
> 
> Iirc we kfree this on unload. Needs to be a kasprintf I think.

drm_dev_set_unique() does this:

dev->unique = kstrdup(name, GFP_KERNEL);

So it should not matter at all where the caller stores name.
Or do you mean something else?

cheers,
  Gerd

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

Re: Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Daniel Vetter]

On Wed, Oct 05, 2016 at 02:43:37PM +0200, Gerd Hoffmann wrote:
> On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote:
> > On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote:
> > > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > index a59d0e309bfc..1fcf739bf509 100644
> > > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > > > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver,
> > > > > struct virtio_device *vdev)
> > > > >                 dev->pdev = pdev;
> > > > >                 if (vga)
> > > > >                         virtio_pci_kick_out_firmware_fb(pdev);
> > > > > +
> > > > > +               ret = drm_dev_set_unique(dev, dev_name(dev->pdev));
> > > > > +               if (ret)
> > > > > +                       goto err_free;
> > > > >         }
> > > > 
> > > > Approach looks sensible to me, I'll give it a try ...
> > > 
> > > Well, dev_name() returns a string without the "pci:" prefix, we have to
> > > add that to make things actually work, like this:
> > 
> > Hm right, I missed that in digging through the piles of layers of struct
> > device naming.
> > 
> > > 
> > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c
> > > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver,
> > > struct virtio_device *vdev)
> > >  
> > >         if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) {
> > >                 struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
> > > +               const char *pname = dev_name(&pdev->dev);
> > >                 bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > > +               char unique[20];
> > 
> > Iirc we kfree this on unload. Needs to be a kasprintf I think.
> 
> drm_dev_set_unique() does this:
> 
> dev->unique = kstrdup(name, GFP_KERNEL);
> 
> So it should not matter at all where the caller stores name.
> Or do you mean something else?

Oh, missed that. r-b stands as-is.
-Daniel
-- 
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