[PATCH xf86-video-amdgpu 00/19] Removing UMS remnants and assorted patches

[PATCH xf86-video-amdgpu 00/19] Removing UMS remnants and assorted patches

[Emil Velikov]

Hi all,

Here is some work that I had for ages - was planning to do a similar
series for the radeon driver. Since ETIME for the latter I'm getting
this out.

This series has two goals, although since the split it a bit hard I've
left it one big bunch:
 - remove most of the UMS remnants
 - simplify the {pci,platform}_probe duplication

To top it up there's a TODO as 'Patch' 19. It mostly seeks to gather
feedback on the items listed.

As always, input is greatly appreciated.

Thanks
Emil

Emil Velikov (19):
  Move amdgpu_bus_id/amgpu_kernel_mode within amdgpu_kernel_open_fd
  Guard ODEV_ATTRIB_FD usage with the correct ifdef
  Use ODEV_ATTRIB_PATH where possible for the device node.
  Remove drmCheckModesettingSupported and kernel module loading
  Kill off drmOpen/Close/drmSetInterfaceVersion in favour of drmDevices
  Introduce amdgpu_device_setup helper
  Factor out common code to amdgpu_probe()
  Simplify fd_ref handling in amdgpu_probe()
  Remove error handling on xnfcalloc()
  Remove unneeded xf86GetEntityInfo() call
  Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails
  Remove ancient "pointer" macro
  configure: check for XORG_DRIVER_CHECK_EXT prior to using it
  Do not export gAMDGPUEntityIndex
  Do not export the DriverRec AMDGPU
  Remove set but unused amdgpu_dri2::pKernelDRMVersion
  Store device_name as AMDGPUEntRec::master_node
  Keep track of who owns the fd in AMDGPUEnt
  TODO

 configure.ac                 |   6 +
 src/amdgpu_dri2.c            |  12 +-
 src/amdgpu_dri2.h            |   2 -
 src/amdgpu_dri3.c            |   3 +-
 src/amdgpu_glamor.c          |   2 +-
 src/amdgpu_glamor_wrappers.c |   4 +-
 src/amdgpu_kms.c             |  25 +---
 src/amdgpu_misc.c            |   2 +-
 src/amdgpu_probe.c           | 304 ++++++++++++++++---------------------------
 src/amdgpu_probe.h           |   3 +-
 src/compat-api.h             |   4 +-
 src/drmmode_display.c        |   2 +-
 todo                         |   9 ++
 13 files changed, 146 insertions(+), 232 deletions(-)
 create mode 100644 todo

-- 
2.16.0

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

[PATCH xf86-video-amdgpu 01/19] Move amdgpu_bus_id/amgpu_kernel_mode within amdgpu_kernel_open_fd

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Small step towards unifying the code paths and removing a handful of
duplication.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 0217060..075e5c1 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -112,9 +112,12 @@ static Bool amdgpu_kernel_mode_enabled(ScrnInfoPtr pScrn, char *busIdString)
 	return TRUE;
 }
 
-static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char *busid,
+static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
+				 struct pci_device *pci_dev,
 				 struct xf86_platform_device *platform_dev)
 {
+	struct pci_device *dev;
+	char *busid;
 	int fd;
 
 #ifdef XF86_PDEV_SERVER_FD
@@ -126,11 +129,26 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char *busid,
 	}
 #endif
 
+	if (platform_dev)
+		dev = platform_dev->pdev;
+	else
+		dev = pci_dev;
+
+	busid = amdgpu_bus_id(pScrn, dev);
+	if (!busid)
+		return -1;
+
+	if (!amdgpu_kernel_mode_enabled(pScrn, busid)) {
+		free(busid);
+		return -1;
+	}
+
 	fd = drmOpen(NULL, busid);
 	if (fd == -1)
 		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
 			   "[drm] Failed to open DRM device for %s: %s\n",
 			   busid, strerror(errno));
+	free(busid);
 	return fd;
 }
 
@@ -145,12 +163,12 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 }
 
 static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
-				   char *busid)
+				   struct pci_device *pci_dev)
 {
 	drmSetVersion sv;
 	int err;
 
-	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, NULL);
+	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
 	if (pAMDGPUEnt->fd == -1)
 		return FALSE;
 
@@ -176,7 +194,6 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
 static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 {
 	ScrnInfoPtr pScrn = NULL;
-	char *busid;
 	EntityInfoPtr pEnt = NULL;
 	DevUnion *pPriv;
 	AMDGPUEntPtr pAMDGPUEnt;
@@ -187,10 +204,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 	if (!pScrn)
 		return FALSE;
 
-	busid = amdgpu_bus_id(pScrn, pci_dev);
-	if (!amdgpu_kernel_mode_enabled(pScrn, busid))
-		goto error;
-
 	pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
 	pScrn->driverName = AMDGPU_DRIVER_NAME;
 	pScrn->name = AMDGPU_NAME;
@@ -226,7 +239,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 			goto error;
 
 		pAMDGPUEnt = pPriv->ptr;
-		if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt, busid))
+		if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt, pci_dev))
 			goto error;
 
 		pAMDGPUEnt->fd_ref = 1;
@@ -249,7 +262,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 								 index)
 				       - 1);
 	free(pEnt);
-	free(busid);
 
 	return TRUE;
 
@@ -257,7 +269,6 @@ error_amdgpu:
 	amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
 	free(pEnt);
-	free(busid);
 	return FALSE;
 }
 
@@ -294,7 +305,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 {
 	ScrnInfoPtr pScrn;
 	int scr_flags = 0;
-	char *busid;
 	EntityInfoPtr pEnt = NULL;
 	DevUnion *pPriv;
 	AMDGPUEntPtr pAMDGPUEnt;
@@ -310,13 +320,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 		xf86SetEntityShared(entity_num);
 	xf86AddEntityToScreen(pScrn, entity_num);
 
-	busid = amdgpu_bus_id(pScrn, dev->pdev);
-	if (!busid)
-		return FALSE;
-
-	if (!amdgpu_kernel_mode_enabled(pScrn, busid))
-		goto error;
-
 	pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
 	pScrn->driverName = AMDGPU_DRIVER_NAME;
 	pScrn->name = AMDGPU_NAME;
@@ -349,7 +352,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
 		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
 		pAMDGPUEnt = pPriv->ptr;
 		pAMDGPUEnt->platform_dev = dev;
-		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
+		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, NULL, dev);
 		if (pAMDGPUEnt->fd < 0)
 			goto error;
 
@@ -373,7 +376,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 								 index)
 				       - 1);
 	free(pEnt);
-	free(busid);
 
 	return TRUE;
 
@@ -381,7 +383,6 @@ error_amdgpu:
 	amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
 	free(pEnt);
-	free(busid);
 	return FALSE;
 }
 #endif
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 075e5c1..e65c83b 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 	char *busid;
 	int fd;
 
-#ifdef XF86_PDEV_SERVER_FD
+#ifdef ODEV_ATTRIB_FD
 	if (platform_dev) {
 		fd = xf86_get_platform_device_int_attrib(platform_dev,
 							 ODEV_ATTRIB_FD, -1);
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index e65c83b..78cc005 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -33,6 +33,8 @@
 #include <errno.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /*
  * Authors:
@@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 				 struct xf86_platform_device *platform_dev)
 {
 	struct pci_device *dev;
+	const char *path;
 	char *busid;
 	int fd;
 
-#ifdef ODEV_ATTRIB_FD
 	if (platform_dev) {
+#ifdef ODEV_ATTRIB_FD
 		fd = xf86_get_platform_device_int_attrib(platform_dev,
 							 ODEV_ATTRIB_FD, -1);
 		if (fd != -1)
 			return fd;
-	}
 #endif
 
+#ifdef ODEV_ATTRIB_PATH
+		path = xf86_get_platform_device_attrib(platform_dev,
+						       ODEV_ATTRIB_PATH);
+
+		fd = open(path, O_RDWR | O_CLOEXEC);
+		if (fd != -1)
+			return fd;
+#endif
+	}
+
 	if (platform_dev)
 		dev = platform_dev->pdev;
 	else
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The former of these is a UMS artefact which gives incorrect and
misleading promise whether "KMS" is supported. Not to mention that
AMDGPU is a only KMS driver.

In a similar fashion xf86LoadKernelModule() is a relic of the times,
where platforms had no scheme of detecting and loading the appropriate
kernel module.

Cc: Robert Millan <rmh@freebsd.org>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Robert, off the top of my head this should work with FreeBSD. Admittedly
I'm not an expert on the platform. Please give it a test.
---
 src/amdgpu_probe.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 78cc005..d8d8383 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -52,10 +52,6 @@
 #include "xf86drmMode.h"
 #include "dri.h"
 
-#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-#include <xf86_OSproc.h>
-#endif
-
 #ifdef XSERVER_PLATFORM_BUS
 #include <xf86platformBus.h>
 #endif
@@ -93,27 +89,6 @@ static char *amdgpu_bus_id(ScrnInfoPtr pScrn, struct pci_device *dev)
 	return busid;
 }
 
-static Bool amdgpu_kernel_mode_enabled(ScrnInfoPtr pScrn, char *busIdString)
-{
-	int ret = drmCheckModesettingSupported(busIdString);
-
-#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-	if (ret) {
-		if (xf86LoadKernelModule("amdgpukms"))
-			ret = drmCheckModesettingSupported(busIdString);
-	}
-#endif
-	if (ret) {
-		xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, 0,
-			       "[KMS] drm report modesetting isn't supported.\n");
-		return FALSE;
-	}
-
-	xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, 0,
-		       "[KMS] Kernel modesetting enabled.\n");
-	return TRUE;
-}
-
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 				 struct pci_device *pci_dev,
 				 struct xf86_platform_device *platform_dev)
@@ -150,11 +125,6 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 	if (!busid)
 		return -1;
 
-	if (!amdgpu_kernel_mode_enabled(pScrn, busid)) {
-		free(busid);
-		return -1;
-	}
-
 	fd = drmOpen(NULL, busid);
 	if (fd == -1)
 		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 05/19] Kill off drmOpen/Close/drmSetInterfaceVersion in favour of drmDevices

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The former has very subtle semantics (see the implementation in libdrm
for details) which were required in the UMS days.

With drmDevices around, we have enough information to build our
heuristics and avoid drmOpen all together.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 93 +++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 50 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index d8d8383..ec132e0 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -75,28 +75,24 @@ static void AMDGPUIdentify(int flags)
 	xf86PrintChipsets(AMDGPU_NAME, "Driver for AMD Radeon", AMDGPUAny);
 }
 
-static char *amdgpu_bus_id(ScrnInfoPtr pScrn, struct pci_device *dev)
+static Bool amdgpu_device_matches(const drmDevicePtr device,
+				  const struct pci_device *dev)
 {
-	char *busid;
-
-	XNFasprintf(&busid, "pci:%04x:%02x:%02x.%d",
-		    dev->domain, dev->bus, dev->dev, dev->func);
-
-	if (!busid)
-		xf86DrvMsgVerb(pScrn->scrnIndex, X_ERROR, 0,
-			       "AMDGPU: Failed to generate bus ID string\n");
-
-	return busid;
+	return (device->bustype == DRM_BUS_PCI &&
+		device->businfo.pci->domain == dev->domain &&
+		device->businfo.pci->bus == dev->bus &&
+		device->businfo.pci->dev == dev->dev &&
+		device->businfo.pci->func == dev->func);
 }
 
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 				 struct pci_device *pci_dev,
 				 struct xf86_platform_device *platform_dev)
 {
+	drmDevicePtr *devices;
 	struct pci_device *dev;
 	const char *path;
-	char *busid;
-	int fd;
+	int fd, i, ret;
 
 	if (platform_dev) {
 #ifdef ODEV_ATTRIB_FD
@@ -121,16 +117,41 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 	else
 		dev = pci_dev;
 
-	busid = amdgpu_bus_id(pScrn, dev);
-	if (!busid)
+	ret = drmGetDevices2(0, NULL, 0);
+	if (ret <= 0) {
+		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+			   "[drm] Failed to retrieve number of DRM devices: %d\n",
+			   ret);
 		return -1;
+	}
+
+	devices = xnfcalloc(sizeof(drmDevicePtr), ret);
+
+	ret = drmGetDevices2(0, devices, ret);
+	if (fd == -1) {
+		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+			   "[drm] Failed to retrieve DRM devices information: %d\n",
+			   ret);
+		free(devices);
+		return -1;
+	}
+
+	for (i = 0; i < ret; i++) {
+		if (amdgpu_device_matches(devices[i], dev) &&
+		    devices[i]->available_nodes & (1 << DRM_NODE_PRIMARY)) {
+			path = devices[i]->nodes[DRM_NODE_PRIMARY];
+			fd = open(path, O_RDWR | O_CLOEXEC);
+			break;
+		}
+	}
+	drmFreeDevices(devices, ret);
+	free(devices);
 
-	fd = drmOpen(NULL, busid);
 	if (fd == -1)
 		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-			   "[drm] Failed to open DRM device for %s: %s\n",
-			   busid, strerror(errno));
-	free(busid);
+			   "[drm] Failed to open DRM device for "
+			   "pci:%04x:%02x:%02x.%1u\n",
+			   dev->domain, dev->bus, dev->dev, dev->func);
 	return fd;
 }
 
@@ -140,39 +161,10 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 	if (!(pAMDGPUEnt->platform_dev &&
 	      pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
 #endif
-		drmClose(pAMDGPUEnt->fd);
+		close(pAMDGPUEnt->fd);
 	pAMDGPUEnt->fd = -1;
 }
 
-static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
-				   struct pci_device *pci_dev)
-{
-	drmSetVersion sv;
-	int err;
-
-	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
-	if (pAMDGPUEnt->fd == -1)
-		return FALSE;
-
-	/* Check that what we opened was a master or a master-capable FD,
-	 * by setting the version of the interface we'll use to talk to it.
-	 * (see DRIOpenDRMMaster() in DRI1)
-	 */
-	sv.drm_di_major = 1;
-	sv.drm_di_minor = 1;
-	sv.drm_dd_major = -1;
-	sv.drm_dd_minor = -1;
-	err = drmSetInterfaceVersion(pAMDGPUEnt->fd, &sv);
-	if (err != 0) {
-		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-			   "[drm] failed to set drm interface version.\n");
-		amdgpu_kernel_close_fd(pAMDGPUEnt);
-		return FALSE;
-	}
-
-	return TRUE;
-}
-
 static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 {
 	ScrnInfoPtr pScrn = NULL;
@@ -221,7 +213,8 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 			goto error;
 
 		pAMDGPUEnt = pPriv->ptr;
-		if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt, pci_dev))
+		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
+		if (pAMDGPUEnt->fd < 0)
 			goto error;
 
 		pAMDGPUEnt->fd_ref = 1;
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 06/19] Introduce amdgpu_device_setup helper

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

It folds the device specifics (open fd, device init) into a single
place.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 66 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index ec132e0..afc8d4c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -165,6 +165,35 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 	pAMDGPUEnt->fd = -1;
 }
 
+static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
+				struct pci_device *pci_dev,
+				struct xf86_platform_device *platform_dev,
+				AMDGPUEntPtr pAMDGPUEnt)
+{
+	uint32_t major_version;
+	uint32_t minor_version;
+
+	pAMDGPUEnt->platform_dev = platform_dev;
+	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev);
+	if (pAMDGPUEnt->fd < 0)
+		return FALSE;
+
+	if (amdgpu_device_initialize(pAMDGPUEnt->fd,
+				     &major_version,
+				     &minor_version,
+				     &pAMDGPUEnt->pDev)) {
+		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+			   "amdgpu_device_initialize failed\n");
+		goto error_amdgpu;
+	}
+
+	return TRUE;
+
+error_amdgpu:
+	amdgpu_kernel_close_fd(pAMDGPUEnt);
+	return FALSE;
+}
+
 static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 {
 	ScrnInfoPtr pScrn = NULL;
@@ -205,28 +234,16 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 	pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
 
 	if (!pPriv->ptr) {
-		uint32_t major_version;
-		uint32_t minor_version;
-
 		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
 		if (!pPriv->ptr)
 			goto error;
 
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
-		if (pAMDGPUEnt->fd < 0)
+		if (!amdgpu_device_setup(pScrn, pci_dev, NULL, pPriv->ptr))
 			goto error;
 
+		pAMDGPUEnt = pPriv->ptr;
 		pAMDGPUEnt->fd_ref = 1;
 
-		if (amdgpu_device_initialize(pAMDGPUEnt->fd,
-					     &major_version,
-					     &minor_version,
-					     &pAMDGPUEnt->pDev)) {
-			xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-				   "amdgpu_device_initialize failed\n");
-			goto error_amdgpu;
-		}
 	} else {
 		pAMDGPUEnt = pPriv->ptr;
 		pAMDGPUEnt->fd_ref++;
@@ -240,8 +257,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 
 	return TRUE;
 
-error_amdgpu:
-	amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
 	free(pEnt);
 	return FALSE;
@@ -321,26 +336,15 @@ amdgpu_platform_probe(DriverPtr pDriver,
 	pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
 
 	if (!pPriv->ptr) {
-		uint32_t major_version;
-		uint32_t minor_version;
 
 		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->platform_dev = dev;
-		pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, NULL, dev);
-		if (pAMDGPUEnt->fd < 0)
+
+		if (!amdgpu_device_setup(pScrn, NULL, dev, pPriv->ptr))
 			goto error;
 
+		pAMDGPUEnt = pPriv->ptr;
 		pAMDGPUEnt->fd_ref = 1;
 
-		if (amdgpu_device_initialize(pAMDGPUEnt->fd,
-					     &major_version,
-					     &minor_version,
-					     &pAMDGPUEnt->pDev)) {
-			xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-				   "amdgpu_device_initialize failed\n");
-			goto error_amdgpu;
-		}
 	} else {
 		pAMDGPUEnt = pPriv->ptr;
 		pAMDGPUEnt->fd_ref++;
@@ -354,8 +358,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 
 	return TRUE;
 
-error_amdgpu:
-	amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
 	free(pEnt);
 	return FALSE;
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 07/19] Factor out common code to amdgpu_probe()

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Keep the distinct pci/platform screen management in the separate probe
entry point and fold the rest into a single function.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 71 +++++++-----------------------------------------------
 1 file changed, 9 insertions(+), 62 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index afc8d4c..d1ad13f 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -194,16 +194,14 @@ error_amdgpu:
 	return FALSE;
 }
 
-static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
+static Bool
+amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
+	     struct pci_device *pci_dev, struct xf86_platform_device *dev)
 {
-	ScrnInfoPtr pScrn = NULL;
 	EntityInfoPtr pEnt = NULL;
 	DevUnion *pPriv;
 	AMDGPUEntPtr pAMDGPUEnt;
 
-	pScrn = xf86ConfigPciEntity(pScrn, 0, entity_num, NULL,
-				    NULL, NULL, NULL, NULL, NULL);
-
 	if (!pScrn)
 		return FALSE;
 
@@ -211,7 +209,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 	pScrn->driverName = AMDGPU_DRIVER_NAME;
 	pScrn->name = AMDGPU_NAME;
 	pScrn->Probe = NULL;
-
 	pScrn->PreInit = AMDGPUPreInit_KMS;
 	pScrn->ScreenInit = AMDGPUScreenInit_KMS;
 	pScrn->SwitchMode = AMDGPUSwitchMode_KMS;
@@ -238,7 +235,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 		if (!pPriv->ptr)
 			goto error;
 
-		if (!amdgpu_device_setup(pScrn, pci_dev, NULL, pPriv->ptr))
+		if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
 			goto error;
 
 		pAMDGPUEnt = pPriv->ptr;
@@ -266,7 +263,10 @@ static Bool
 amdgpu_pci_probe(DriverPtr pDriver,
 		 int entity_num, struct pci_device *device, intptr_t match_data)
 {
-	return amdgpu_get_scrninfo(entity_num, device);
+	ScrnInfoPtr pScrn = xf86ConfigPciEntity(NULL, 0, entity_num, NULL,
+						NULL, NULL, NULL, NULL, NULL);
+
+	return amdgpu_probe(pScrn, entity_num, device, NULL);
 }
 
 static Bool AMDGPUDriverFunc(ScrnInfoPtr scrn, xorgDriverFuncOp op, void *data)
@@ -295,9 +295,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 {
 	ScrnInfoPtr pScrn;
 	int scr_flags = 0;
-	EntityInfoPtr pEnt = NULL;
-	DevUnion *pPriv;
-	AMDGPUEntPtr pAMDGPUEnt;
 
 	if (!dev->pdev)
 		return FALSE;
@@ -310,57 +307,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
 		xf86SetEntityShared(entity_num);
 	xf86AddEntityToScreen(pScrn, entity_num);
 
-	pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
-	pScrn->driverName = AMDGPU_DRIVER_NAME;
-	pScrn->name = AMDGPU_NAME;
-	pScrn->Probe = NULL;
-	pScrn->PreInit = AMDGPUPreInit_KMS;
-	pScrn->ScreenInit = AMDGPUScreenInit_KMS;
-	pScrn->SwitchMode = AMDGPUSwitchMode_KMS;
-	pScrn->AdjustFrame = AMDGPUAdjustFrame_KMS;
-	pScrn->EnterVT = AMDGPUEnterVT_KMS;
-	pScrn->LeaveVT = AMDGPULeaveVT_KMS;
-	pScrn->FreeScreen = AMDGPUFreeScreen_KMS;
-	pScrn->ValidMode = AMDGPUValidMode;
-
-	pEnt = xf86GetEntityInfo(entity_num);
-
-	/* Create a AMDGPUEntity for all chips, even with old single head
-	 * Radeon, need to use pAMDGPUEnt for new monitor detection routines.
-	 */
-	xf86SetEntitySharable(entity_num);
-
-	if (gAMDGPUEntityIndex == -1)
-		gAMDGPUEntityIndex = xf86AllocateEntityPrivateIndex();
-
-	pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
-
-	if (!pPriv->ptr) {
-
-		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-
-		if (!amdgpu_device_setup(pScrn, NULL, dev, pPriv->ptr))
-			goto error;
-
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->fd_ref = 1;
-
-	} else {
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->fd_ref++;
-	}
-
-	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
-				       xf86GetNumEntityInstances(pEnt->
-								 index)
-				       - 1);
-	free(pEnt);
-
-	return TRUE;
-
-error:
-	free(pEnt);
-	return FALSE;
+	return amdgpu_probe(pScrn, entity_num, NULL, dev);
 }
 #endif
 
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 08/19] Simplify fd_ref handling in amdgpu_probe()

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index d1ad13f..8a16060 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -237,14 +237,9 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 
 		if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
 			goto error;
-
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->fd_ref = 1;
-
-	} else {
-		pAMDGPUEnt = pPriv->ptr;
-		pAMDGPUEnt->fd_ref++;
 	}
+	pAMDGPUEnt = pPriv->ptr;
+	pAMDGPUEnt->fd_ref++;
 
 	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
 				       xf86GetNumEntityInstances(pEnt->
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 09/19] Remove error handling on xnfcalloc()

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The function "cannot fail", thus we'll never hit the error path.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 8a16060..8842868 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -232,8 +232,6 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 
 	if (!pPriv->ptr) {
 		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-		if (!pPriv->ptr)
-			goto error;
 
 		if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
 			goto error;
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 10/19] Remove unneeded xf86GetEntityInfo() call

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

We only use ent->index, which is the same as the argument fed into the
function.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Note: I'm not 100% sure if the X API guarantees that, yet it seems to be
the case for over 3 years.
---
 src/amdgpu_probe.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 8842868..537d44c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -198,7 +198,6 @@ static Bool
 amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 	     struct pci_device *pci_dev, struct xf86_platform_device *dev)
 {
-	EntityInfoPtr pEnt = NULL;
 	DevUnion *pPriv;
 	AMDGPUEntPtr pAMDGPUEnt;
 
@@ -218,8 +217,6 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 	pScrn->FreeScreen = AMDGPUFreeScreen_KMS;
 	pScrn->ValidMode = AMDGPUValidMode;
 
-	pEnt = xf86GetEntityInfo(entity_num);
-
 	/* Create a AMDGPUEntity for all chips, even with old single head
 	 * Radeon, need to use pAMDGPUEnt for new monitor detection routines.
 	 */
@@ -228,7 +225,7 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 	if (gAMDGPUEntityIndex == -1)
 		gAMDGPUEntityIndex = xf86AllocateEntityPrivateIndex();
 
-	pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
+	pPriv = xf86GetEntityPrivate(entity_num, gAMDGPUEntityIndex);
 
 	if (!pPriv->ptr) {
 		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
@@ -239,16 +236,13 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 	pAMDGPUEnt = pPriv->ptr;
 	pAMDGPUEnt->fd_ref++;
 
-	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
-				       xf86GetNumEntityInstances(pEnt->
-								 index)
+	xf86SetEntityInstanceForScreen(pScrn, entity_num,
+				       xf86GetNumEntityInstances(entity_num)
 				       - 1);
-	free(pEnt);
 
 	return TRUE;
 
 error:
-	free(pEnt);
 	return FALSE;
 }
 
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Seems like we've been leaking this for years. It became more obvious
with the recent refactoring.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 537d44c..588891c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 	return TRUE;
 
 error:
+	free(pPriv->ptr);
+	pPriv->ptr = NULL;
 	return FALSE;
 }
 
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 12/19] Remove ancient "pointer" macro

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Use "void *" over the macro.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Worth splitting this into separate patches and/or keeping compat-api.h
as-is?
---
 src/amdgpu_dri2.c            | 4 ++--
 src/amdgpu_glamor.c          | 2 +-
 src/amdgpu_glamor_wrappers.c | 4 ++--
 src/amdgpu_kms.c             | 6 +++---
 src/amdgpu_misc.c            | 2 +-
 src/compat-api.h             | 4 ++--
 src/drmmode_display.c        | 2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
index 4a0f8bf..62964ab 100644
--- a/src/amdgpu_dri2.c
+++ b/src/amdgpu_dri2.c
@@ -341,7 +341,7 @@ static void amdgpu_dri2_unref_buffer(BufferPtr buffer)
 
 static void
 amdgpu_dri2_client_state_changed(CallbackListPtr * ClientStateCallback,
-				 pointer data, pointer calldata)
+				 void *data, void *calldata)
 {
 	NewClientInfoRec *clientinfo = calldata;
 	ClientPtr pClient = clientinfo->client;
@@ -846,7 +846,7 @@ static int amdgpu_dri2_get_msc(DrawablePtr draw, CARD64 * ust, CARD64 * msc)
 }
 
 static
-CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
+CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, void *data)
 {
 	DRI2FrameEventPtr event_info = (DRI2FrameEventPtr) data;
 	xf86CrtcPtr crtc = event_info->crtc;
diff --git a/src/amdgpu_glamor.c b/src/amdgpu_glamor.c
index 6efc372..9b3a4e6 100644
--- a/src/amdgpu_glamor.c
+++ b/src/amdgpu_glamor.c
@@ -78,7 +78,7 @@ Bool amdgpu_glamor_create_screen_resources(ScreenPtr screen)
 Bool amdgpu_glamor_pre_init(ScrnInfoPtr scrn)
 {
 	AMDGPUInfoPtr info = AMDGPUPTR(scrn);
-	pointer glamor_module;
+	void *glamor_module;
 	CARD32 version;
 
 	if (scrn->depth < 24) {
diff --git a/src/amdgpu_glamor_wrappers.c b/src/amdgpu_glamor_wrappers.c
index fab8e5a..cf3b6c0 100644
--- a/src/amdgpu_glamor_wrappers.c
+++ b/src/amdgpu_glamor_wrappers.c
@@ -434,7 +434,7 @@ amdgpu_glamor_poly_fill_rect(DrawablePtr pDrawable, GCPtr pGC,
 static void
 amdgpu_glamor_image_glyph_blt(DrawablePtr pDrawable, GCPtr pGC,
 			      int x, int y, unsigned int nglyph,
-			      CharInfoPtr *ppci, pointer pglyphBase)
+			      CharInfoPtr *ppci, void *pglyphBase)
 {
 	ScrnInfoPtr scrn = xf86ScreenToScrn(pDrawable->pScreen);
 	PixmapPtr pixmap = get_drawable_pixmap(pDrawable);
@@ -453,7 +453,7 @@ amdgpu_glamor_image_glyph_blt(DrawablePtr pDrawable, GCPtr pGC,
 static void
 amdgpu_glamor_poly_glyph_blt(DrawablePtr pDrawable, GCPtr pGC,
 			     int x, int y, unsigned int nglyph,
-			     CharInfoPtr *ppci, pointer pglyphBase)
+			     CharInfoPtr *ppci, void *pglyphBase)
 {
 	ScrnInfoPtr scrn = xf86ScreenToScrn(pDrawable->pScreen);
 	PixmapPtr pixmap = get_drawable_pixmap(pDrawable);
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 7ec610f..2ee7007 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -183,7 +183,7 @@ callback_needs_flush(AMDGPUInfoPtr info, struct amdgpu_client_priv *client_priv)
 
 static void
 amdgpu_event_callback(CallbackListPtr *list,
-		      pointer user_data, pointer call_data)
+		      void *user_data, void *call_data)
 {
 	EventInfoRec *eventinfo = call_data;
 	ScrnInfoPtr pScrn = user_data;
@@ -218,7 +218,7 @@ amdgpu_event_callback(CallbackListPtr *list,
 
 static void
 amdgpu_flush_callback(CallbackListPtr *list,
-		      pointer user_data, pointer call_data)
+		      void *user_data, void *call_data)
 {
 	ScrnInfoPtr pScrn = user_data;
 	ScreenPtr pScreen = pScrn->pScreen;
@@ -1644,7 +1644,7 @@ static void amdgpu_drop_drm_master(ScrnInfoPtr pScrn)
 
 
 static
-CARD32 cleanup_black_fb(OsTimerPtr timer, CARD32 now, pointer data)
+CARD32 cleanup_black_fb(OsTimerPtr timer, CARD32 now, void *data)
 {
 	ScreenPtr screen = data;
 	ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
diff --git a/src/amdgpu_misc.c b/src/amdgpu_misc.c
index 560a877..1e5b7cb 100644
--- a/src/amdgpu_misc.c
+++ b/src/amdgpu_misc.c
@@ -50,7 +50,7 @@ static XF86ModuleVersionInfo AMDGPUVersionRec = {
  * This function is called every time the module is loaded.
  */
 static pointer
-AMDGPUSetup(pointer Module, pointer Options, int *ErrorMajor, int *ErrorMinor)
+AMDGPUSetup(void *Module, void *Options, int *ErrorMajor, int *ErrorMinor)
 {
 	static Bool Inited = FALSE;
 
diff --git a/src/compat-api.h b/src/compat-api.h
index a703e5c..cc48b21 100644
--- a/src/compat-api.h
+++ b/src/compat-api.h
@@ -31,10 +31,10 @@
 #endif
 
 #if ABI_VIDEODRV_VERSION >= SET_ABI_VERSION(23, 0)
-#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScreen, pointer pTimeout
+#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScreen, void *pTimeout
 #define BLOCKHANDLER_ARGS pScreen, pTimeout
 #else
-#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScreen, pointer pTimeout, pointer pReadmask
+#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScreen, void *pTimeout, void *pReadmask
 #define BLOCKHANDLER_ARGS pScreen, pTimeout, pReadmask
 #endif
 
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 85970d1..92ddd4b 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2341,7 +2341,7 @@ static void drmmode_notify_fd(int fd, int notify, void *data)
 	drmHandleEvent(fd, &drmmode->event_context);
 }
 #else
-static void drm_wakeup_handler(pointer data, int err, pointer p)
+static void drm_wakeup_handler(void *data, int err, void *p)
 {
 	drmmode_ptr drmmode = data;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(drmmode->scrn);
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 13/19] configure: check for XORG_DRIVER_CHECK_EXT prior to using it

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Noticed while skimming through the intel driver - haven't came across
this while building the amdgpu driver.
---
 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index 7b7a4b1..91fbb7b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -43,6 +43,12 @@ m4_ifndef([XORG_MACROS_VERSION],
 XORG_MACROS_VERSION(1.8)
 XORG_DEFAULT_OPTIONS
 
+# Require X.Org server macros (i.e. XORG_DRIVER_CHECK_EXT) to check for required modules
+m4_ifndef([XORG_DRIVER_CHECK_EXT],
+          [m4_fatal([must install xorg-server macros before running autoconf/autogen.
+  Hint: either install from source, git://anongit.freedesktop.org/xorg/xserver or,
+  depending on your distribution, try package 'xserver-xorg-dev' or 'xorg-x11-server-devel'])])
+
 # Initialize libtool
 AC_DISABLE_STATIC
 AC_PROG_LIBTOOL
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 14/19] Do not export gAMDGPUEntityIndex

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The modules should not export anything but a single *ModuleData symbol.
Seemingly a copy/paste mistake from the radeon driver.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
I'm aroung 70% sure on this one.

Yet again, perhaps multi GPU setups do - somehow pulling the _X_EXPORT
variable over the local one? I seriously doubt it though.
---
 src/amdgpu_kms.c   | 2 +-
 src/amdgpu_probe.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 2ee7007..5d85bd7 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -87,7 +87,7 @@ const OptionInfoRec *AMDGPUOptionsWeak(void)
 	return AMDGPUOptions_KMS;
 }
 
-extern _X_EXPORT int gAMDGPUEntityIndex;
+extern int gAMDGPUEntityIndex;
 
 static int getAMDGPUEntityIndex(void)
 {
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 588891c..0a46cf1 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -56,7 +56,7 @@
 #include <xf86platformBus.h>
 #endif
 
-_X_EXPORT int gAMDGPUEntityIndex = -1;
+int gAMDGPUEntityIndex = -1;
 
 /* Return the options for supported chipset 'n'; NULL otherwise */
 static const OptionInfoRec *AMDGPUAvailableOptions(int chipid, int busid)
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 15/19] Do not export the DriverRec AMDGPU

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Analogous to previous commit - unused externally and should not be
exported.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 0a46cf1..4959bd6 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -305,7 +305,7 @@ static const struct pci_id_match amdgpu_device_match[] = {
     {0, 0, 0},
 };
 
-_X_EXPORT DriverRec AMDGPU = {
+DriverRec AMDGPU = {
 	AMDGPU_VERSION_CURRENT,
 	AMDGPU_DRIVER_NAME,
 	AMDGPUIdentify,
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 16/19] Remove set but unused amdgpu_dri2::pKernelDRMVersion

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_dri2.h | 1 -
 src/amdgpu_kms.c  | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/src/amdgpu_dri2.h b/src/amdgpu_dri2.h
index c6a2ab6..a345e6b 100644
--- a/src/amdgpu_dri2.h
+++ b/src/amdgpu_dri2.h
@@ -30,7 +30,6 @@
 #include <xorg-server.h>
 
 struct amdgpu_dri2 {
-	drmVersionPtr pKernelDRMVersion;
 	Bool available;
 	Bool enabled;
 	char *device_name;
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 5d85bd7..c70cfe5 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1397,12 +1397,6 @@ Bool AMDGPUPreInit_KMS(ScrnInfoPtr pScrn, int flags)
 
 	info->dri2.available = FALSE;
 	info->dri2.enabled = FALSE;
-	info->dri2.pKernelDRMVersion = drmGetVersion(pAMDGPUEnt->fd);
-	if (info->dri2.pKernelDRMVersion == NULL) {
-		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-			   "AMDGPUDRIGetVersion failed to get the DRM version\n");
-		return FALSE;
-	}
 
 	/* Get ScreenInit function */
 	if (!xf86LoadSubModule(pScrn, "fb"))
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Rename the variable to reflect what it is. Plus move it out of the dri2
section - it's used in dri2 and dri3.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_dri2.c  | 8 +-------
 src/amdgpu_dri2.h  | 1 -
 src/amdgpu_dri3.c  | 3 +--
 src/amdgpu_kms.c   | 1 +
 src/amdgpu_probe.c | 5 +++++
 src/amdgpu_probe.h | 1 +
 6 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
index 62964ab..082520a 100644
--- a/src/amdgpu_dri2.c
+++ b/src/amdgpu_dri2.c
@@ -1282,11 +1282,9 @@ Bool amdgpu_dri2_screen_init(ScreenPtr pScreen)
 	if (!info->dri2.available)
 		return FALSE;
 
-	info->dri2.device_name = drmGetDeviceNameFromFd(pAMDGPUEnt->fd);
-
 	dri2_info.driverName = SI_DRIVER_NAME;
 	dri2_info.fd = pAMDGPUEnt->fd;
-	dri2_info.deviceName = info->dri2.device_name;
+	dri2_info.deviceName = pAMDGPUEnt->master_node;
 
 	if (info->drmmode.count_crtcs > 2) {
 		uint64_t cap_value;
@@ -1340,15 +1338,11 @@ Bool amdgpu_dri2_screen_init(ScreenPtr pScreen)
 
 void amdgpu_dri2_close_screen(ScreenPtr pScreen)
 {
-	ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
-	AMDGPUInfoPtr info = AMDGPUPTR(pScrn);
-
 	if (--DRI2InfoCnt == 0)
 		DeleteCallback(&ClientStateCallback,
 			       amdgpu_dri2_client_state_changed, 0);
 
 	DRI2CloseScreen(pScreen);
-	drmFree(info->dri2.device_name);
 }
 
 #endif /* DRI2 */
diff --git a/src/amdgpu_dri2.h b/src/amdgpu_dri2.h
index a345e6b..a9411b2 100644
--- a/src/amdgpu_dri2.h
+++ b/src/amdgpu_dri2.h
@@ -32,7 +32,6 @@
 struct amdgpu_dri2 {
 	Bool available;
 	Bool enabled;
-	char *device_name;
 };
 
 #ifdef DRI2
diff --git a/src/amdgpu_dri3.c b/src/amdgpu_dri3.c
index 87e3d85..2c06b74 100644
--- a/src/amdgpu_dri3.c
+++ b/src/amdgpu_dri3.c
@@ -44,11 +44,10 @@ static int open_master_node(ScreenPtr screen, int *out)
 {
 	ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
-	AMDGPUInfoPtr info = AMDGPUPTR(scrn);
 	drm_magic_t magic;
 	int fd;
 
-	fd = open(info->dri2.device_name, O_RDWR | O_CLOEXEC);
+	fd = open(pAMDGPUEnt->master_node, O_RDWR | O_CLOEXEC);
 	if (fd < 0)
 		return BadAlloc;
 
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index c70cfe5..d27f71d 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -147,6 +147,7 @@ static void AMDGPUFreeRec(ScrnInfoPtr pScrn)
 		pAMDGPUEnt->fd_ref--;
 		if (!pAMDGPUEnt->fd_ref) {
 			amdgpu_device_deinitialize(pAMDGPUEnt->pDev);
+			free(pAMDGPUEnt->master_node);
 			amdgpu_kernel_close_fd(pAMDGPUEnt);
 			free(pPriv->ptr);
 			pPriv->ptr = NULL;
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 4959bd6..e9afe42 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
 	if (pAMDGPUEnt->fd < 0)
 		return FALSE;
 
+	pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
+	if (pAMDGPUEnt->master_node)
+	       goto error_amdgpu;
+
 	if (amdgpu_device_initialize(pAMDGPUEnt->fd,
 				     &major_version,
 				     &minor_version,
@@ -190,6 +194,7 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
 	return TRUE;
 
 error_amdgpu:
+	free(pAMDGPUEnt->master_node);
 	amdgpu_kernel_close_fd(pAMDGPUEnt);
 	return FALSE;
 }
diff --git a/src/amdgpu_probe.h b/src/amdgpu_probe.h
index 5f61aab..94f204f 100644
--- a/src/amdgpu_probe.h
+++ b/src/amdgpu_probe.h
@@ -62,6 +62,7 @@ typedef struct {
 
 	int fd;			/* for sharing across zaphod heads   */
 	int fd_ref;
+	char *master_node;
 	unsigned long fd_wakeup_registered;	/* server generation for which fd has been registered for wakeup handling */
 	int fd_wakeup_ref;
 	unsigned int assigned_crtcs;
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 18/19] Keep track of who owns the fd in AMDGPUEnt

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Currently we're having xf86_platform_device pointer embedded, alongside
a bunch of ifdef compiler guards.

Swap that with a simple is_server_fd bool ;-)

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 src/amdgpu_kms.c   | 10 ++--------
 src/amdgpu_probe.c | 15 +++++++--------
 src/amdgpu_probe.h |  2 +-
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index d27f71d..d625f56 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1611,11 +1611,8 @@ static Bool amdgpu_set_drm_master(ScrnInfoPtr pScrn)
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
 	int err;
 
-#ifdef XF86_PDEV_SERVER_FD
-	if (pAMDGPUEnt->platform_dev &&
-	    (pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
+	if (pAMDGPUEnt->is_server_fd)
 		return TRUE;
-#endif
 
 	err = drmSetMaster(pAMDGPUEnt->fd);
 	if (err)
@@ -1628,11 +1625,8 @@ static void amdgpu_drop_drm_master(ScrnInfoPtr pScrn)
 {
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
 
-#ifdef XF86_PDEV_SERVER_FD
-	if (pAMDGPUEnt->platform_dev &&
-	    (pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
+	if (pAMDGPUEnt->is_server_fd)
 		return;
-#endif
 
 	drmDropMaster(pAMDGPUEnt->fd);
 }
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index e9afe42..5d4890b 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -87,7 +87,8 @@ static Bool amdgpu_device_matches(const drmDevicePtr device,
 
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 				 struct pci_device *pci_dev,
-				 struct xf86_platform_device *platform_dev)
+				 struct xf86_platform_device *platform_dev,
+				 AMDGPUEntPtr pAMDGPUEnt)
 {
 	drmDevicePtr *devices;
 	struct pci_device *dev;
@@ -98,8 +99,10 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 #ifdef ODEV_ATTRIB_FD
 		fd = xf86_get_platform_device_int_attrib(platform_dev,
 							 ODEV_ATTRIB_FD, -1);
-		if (fd != -1)
+		if (fd != -1) {
+			pAMDGPUEnt->is_server_fd = TRUE;
 			return fd;
+		}
 #endif
 
 #ifdef ODEV_ATTRIB_PATH
@@ -157,10 +160,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 
 void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 {
-#ifdef XF86_PDEV_SERVER_FD
-	if (!(pAMDGPUEnt->platform_dev &&
-	      pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
-#endif
+	if (!pAMDGPUEnt->is_server_fd)
 		close(pAMDGPUEnt->fd);
 	pAMDGPUEnt->fd = -1;
 }
@@ -173,8 +173,7 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
 	uint32_t major_version;
 	uint32_t minor_version;
 
-	pAMDGPUEnt->platform_dev = platform_dev;
-	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev);
+	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev, pAMDGPUEnt);
 	if (pAMDGPUEnt->fd < 0)
 		return FALSE;
 
diff --git a/src/amdgpu_probe.h b/src/amdgpu_probe.h
index 94f204f..81fe4a1 100644
--- a/src/amdgpu_probe.h
+++ b/src/amdgpu_probe.h
@@ -65,10 +65,10 @@ typedef struct {
 	char *master_node;
 	unsigned long fd_wakeup_registered;	/* server generation for which fd has been registered for wakeup handling */
 	int fd_wakeup_ref;
+	Bool is_server_fd;
 	unsigned int assigned_crtcs;
 	ScrnInfoPtr primary_scrn;
 	ScrnInfoPtr secondary_scrn;
-	struct xf86_platform_device *platform_dev;
 	char *render_node;
 } AMDGPUEntRec, *AMDGPUEntPtr;
 
-- 
2.16.0

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

[PATCH xf86-video-amdgpu 19/19] TODO

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 todo | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 todo

diff --git a/todo b/todo
new file mode 100644
index 0000000..10c1ad5
--- /dev/null
+++ b/todo
@@ -0,0 +1,9 @@
+ - on amdgpu_probe failure, the pScrn entry is leaked - missing api/examples?
+ - introduce xf86ConfigEntity and use it
+ - remove embedded AMDGPUInfoRec::pEnt
+ - consistently use gAMDGPUEntityIndex or getAMDGPUEntityIndex
+ - consistently use of pEnt/entity_num -> pScrn->list[], AMDPRIV
+ - kill off DRI_1_ DRICreatePCIBusID - demote again to DRI1 only in X codebase
+ - compose bus string early & strcmp instead of device_match?
+ - remove embedded AMDGPUInfoRec::PciInfo - reuse EntityInfoRec::chipset, GDevRec::chiIDi, amdgpu_gpu_info::asic_id or ...
+ - use odev to fetch render_node?
-- 
2.16.0

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

Re: [PATCH xf86-video-amdgpu 00/19] Removing UMS remnants and assorted patches

[Michel Dänzer]

Hi Emil,


thanks for the patches and sorry for the late follow-up, I was on
vacation and backlogged.


> Here is some work that I had for ages - was planning to do a similar
> series for the radeon driver. Since ETIME for the latter I'm getting
> this out.

FWIW, it would be nice to bring the radeon probe code more in line with
amdgpu, which should make porting these changes easy.


> This series has two goals, although since the split it a bit hard I've
> left it one big bunch:
>  - remove most of the UMS remnants
>  - simplify the {pci,platform}_probe duplication
> 
> To top it up there's a TODO as 'Patch' 19. It mostly seeks to gather
> feedback on the items listed.

Patches 1, 15 & 16 are

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


I'm going to follow up with some comments to individual patches. Some of
them should be dropped, the rest modified or rebased.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> The former of these is a UMS artefact which gives incorrect and
> misleading promise whether "KMS" is supported. Not to mention that
> AMDGPU is a only KMS driver.
> 
> In a similar fashion xf86LoadKernelModule() is a relic of the times,
> where platforms had no scheme of detecting and loading the appropriate
> kernel module.
> 
> Cc: Robert Millan <rmh@freebsd.org>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Robert, off the top of my head this should work with FreeBSD. Admittedly
> I'm not an expert on the platform. Please give it a test.

I want to get confirmation from Robert that this will work on FreeBSD
now, since he explicitly restored the kernel module loading code in
https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  src/amdgpu_probe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 075e5c1..e65c83b 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>  	char *busid;
>  	int fd;
>  
> -#ifdef XF86_PDEV_SERVER_FD
> +#ifdef ODEV_ATTRIB_FD
>  	if (platform_dev) {
>  		fd = xf86_get_platform_device_int_attrib(platform_dev,
>  							 ODEV_ATTRIB_FD, -1);
> 

ODEV_ATTRIB_FD doesn't seem obviously more "correct" than
XF86_PDEV_SERVER_FD, since both were added in the same xserver commit,
and the latter might be helpful for understanding this is related to the
other code guarded by XF86_PDEV_SERVER_FD.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  src/amdgpu_probe.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index e65c83b..78cc005 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -33,6 +33,8 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <stdlib.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
>  
>  /*
>   * Authors:
> @@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>  				 struct xf86_platform_device *platform_dev)
>  {
>  	struct pci_device *dev;
> +	const char *path;
>  	char *busid;
>  	int fd;
>  
> -#ifdef ODEV_ATTRIB_FD
>  	if (platform_dev) {
> +#ifdef ODEV_ATTRIB_FD
>  		fd = xf86_get_platform_device_int_attrib(platform_dev,
>  							 ODEV_ATTRIB_FD, -1);
>  		if (fd != -1)
>  			return fd;
> -	}
>  #endif
>  
> +#ifdef ODEV_ATTRIB_PATH

This guard is superfluous: ODEV_ATTRIB_PATH was added in xserver 1.13,
and we require >= 1.13.


> +		path = xf86_get_platform_device_attrib(platform_dev,
> +						       ODEV_ATTRIB_PATH);
> +
> +		fd = open(path, O_RDWR | O_CLOEXEC);
> +		if (fd != -1)
> +			return fd;
> +#endif
> +	}
> +
>  	if (platform_dev)
>  		dev = platform_dev->pdev;
>  	else
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Seems like we've been leaking this for years. It became more obvious
> with the recent refactoring.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  src/amdgpu_probe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 537d44c..588891c 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>  	return TRUE;
>  
>  error:
> +	free(pPriv->ptr);
> +	pPriv->ptr = NULL;
>  	return FALSE;
>  }
>  
> 

valgrind doesn't report a leak if I force this error path; presumably
Xorg frees the private after returning FALSE here.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 13/19] configure: check for XORG_DRIVER_CHECK_EXT prior to using it

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Noticed while skimming through the intel driver - haven't came across
> this while building the amdgpu driver.

Neither have I, and it seems pretty unlikely that somebody would end up
having xorg-server.pc but lacking xorg-server.m4. I'm inclined to pass
on this one until there's actual evidence of it being useful.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Rename the variable to reflect what it is. Plus move it out of the dri2
> section - it's used in dri2 and dri3.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

[...]

> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 4959bd6..e9afe42 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
>  	if (pAMDGPUEnt->fd < 0)
>  		return FALSE;
>  
> +	pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
> +	if (pAMDGPUEnt->master_node)
> +	       goto error_amdgpu;

This should be

	if (!pAMDGPUEnt->master_node)

shouldn't it?


... Which raises the question: How did you test these patches? :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 19/19] TODO

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  todo | 9 +++++++++
>  1 file changed, 9 insertions(+)
>  create mode 100644 todo
> 
> diff --git a/todo b/todo
> new file mode 100644
> index 0000000..10c1ad5
> --- /dev/null
> +++ b/todo
> @@ -0,0 +1,9 @@
> + - on amdgpu_probe failure, the pScrn entry is leaked - missing api/examples?

Might be similar to patch 11; does valgrind actually report a leak if
you force this?


> + - introduce xf86ConfigEntity and use it
> + - remove embedded AMDGPUInfoRec::pEnt
> + - consistently use gAMDGPUEntityIndex or getAMDGPUEntityIndex
> + - consistently use of pEnt/entity_num -> pScrn->list[], AMDPRIV
> + - kill off DRI_1_ DRICreatePCIBusID - demote again to DRI1 only in X codebase
> + - compose bus string early & strcmp instead of device_match?
> + - remove embedded AMDGPUInfoRec::PciInfo - reuse EntityInfoRec::chipset, GDevRec::chiIDi, amdgpu_gpu_info::asic_id or ...
> + - use odev to fetch render_node?

I'm afraid I don't really see these as important enough to be tracked
like this.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

On 10 April 2018 at 09:26, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> The former of these is a UMS artefact which gives incorrect and
>> misleading promise whether "KMS" is supported. Not to mention that
>> AMDGPU is a only KMS driver.
>>
>> In a similar fashion xf86LoadKernelModule() is a relic of the times,
>> where platforms had no scheme of detecting and loading the appropriate
>> kernel module.
>>
>> Cc: Robert Millan <rmh@freebsd.org>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>> Robert, off the top of my head this should work with FreeBSD. Admittedly
>> I'm not an expert on the platform. Please give it a test.
>
> I want to get confirmation from Robert that this will work on FreeBSD
> now, since he explicitly restored the kernel module loading code in
> https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
> .
>
Fully agreed!. That's why I added him to the CC list.

Throwing some ideas:
 - If it's still needed can we keep it !Linux only?
 - Wayland does not have a kernel module loading mechanism.
 - To prevent fan noise and/or card overheating, one really wants to
load the kernel module early. Not when X starts ;-)

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Emil Velikov]

On 10 April 2018 at 09:27, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 075e5c1..e65c83b 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>       char *busid;
>>       int fd;
>>
>> -#ifdef XF86_PDEV_SERVER_FD
>> +#ifdef ODEV_ATTRIB_FD
>>       if (platform_dev) {
>>               fd = xf86_get_platform_device_int_attrib(platform_dev,
>>                                                        ODEV_ATTRIB_FD, -1);
>>
>
> ODEV_ATTRIB_FD doesn't seem obviously more "correct" than
> XF86_PDEV_SERVER_FD, since both were added in the same xserver commit,
> and the latter might be helpful for understanding this is related to the
> other code guarded by XF86_PDEV_SERVER_FD.
>
All the XF86_PDEV_SERVER_FD code is dropped with a later commit ;-)
I could move this patch just after said commit, or you prefer to keep
the original guard?

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Michel Dänzer]

On 2018-04-10 11:20 AM, Emil Velikov wrote:
> On 10 April 2018 at 09:26, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> The former of these is a UMS artefact which gives incorrect and
>>> misleading promise whether "KMS" is supported. Not to mention that
>>> AMDGPU is a only KMS driver.
>>>
>>> In a similar fashion xf86LoadKernelModule() is a relic of the times,
>>> where platforms had no scheme of detecting and loading the appropriate
>>> kernel module.
>>>
>>> Cc: Robert Millan <rmh@freebsd.org>
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>> Robert, off the top of my head this should work with FreeBSD. Admittedly
>>> I'm not an expert on the platform. Please give it a test.
>>
>> I want to get confirmation from Robert that this will work on FreeBSD
>> now, since he explicitly restored the kernel module loading code in
>> https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
>> .
>>
> Fully agreed!. That's why I added him to the CC list.
> 
> Throwing some ideas:
>  - If it's still needed can we keep it !Linux only?

The first drmCheckModesettingSupported call? Fine with me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Michel Dänzer]

On 2018-04-10 11:24 AM, Emil Velikov wrote:
> On 10 April 2018 at 09:27, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>>  src/amdgpu_probe.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>> index 075e5c1..e65c83b 100644
>>> --- a/src/amdgpu_probe.c
>>> +++ b/src/amdgpu_probe.c
>>> @@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>>       char *busid;
>>>       int fd;
>>>
>>> -#ifdef XF86_PDEV_SERVER_FD
>>> +#ifdef ODEV_ATTRIB_FD
>>>       if (platform_dev) {
>>>               fd = xf86_get_platform_device_int_attrib(platform_dev,
>>>                                                        ODEV_ATTRIB_FD, -1);
>>>
>>
>> ODEV_ATTRIB_FD doesn't seem obviously more "correct" than
>> XF86_PDEV_SERVER_FD, since both were added in the same xserver commit,
>> and the latter might be helpful for understanding this is related to the
>> other code guarded by XF86_PDEV_SERVER_FD.
>>
> All the XF86_PDEV_SERVER_FD code is dropped with a later commit ;-)
> I could move this patch just after said commit, or you prefer to keep
> the original guard?

The latter, less churn. :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 09:28, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Seems like we've been leaking this for years. It became more obvious
>> with the recent refactoring.
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 537d44c..588891c 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>       return TRUE;
>>
>>  error:
>> +     free(pPriv->ptr);
>> +     pPriv->ptr = NULL;
>>       return FALSE;
>>  }
>>
>>
>
> valgrind doesn't report a leak if I force this error path; presumably
> Xorg frees the private after returning FALSE here.
>
Just double-checked and Xorg does not know anything about ptr. The
only one who clears it up is AMDGPUFreeScreen_KMS.

The magic (for this and the other 'leak') seems to be happening in
xf86platformAddDevice. Namely:
 - ::platformProbe is called via doPlatformProbe
 - the driver explicitly calls xf86AllocateScreen, yet fails later on
 - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
 - ::PreInit fails, ::configured is false
 - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
AMDGPUFreeScreen_KMS)

Eventually, I could unwrap all that although it makes sense to keep
things simpler. As effectively done by the patch.

I believe you'll agree?
-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Emil Velikov]

On 10 April 2018 at 09:29, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Rename the variable to reflect what it is. Plus move it out of the dri2
>> section - it's used in dri2 and dri3.
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>
> [...]
>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 4959bd6..e9afe42 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
>>       if (pAMDGPUEnt->fd < 0)
>>               return FALSE;
>>
>> +     pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
>> +     if (pAMDGPUEnt->master_node)
>> +            goto error_amdgpu;
>
> This should be
>
>         if (!pAMDGPUEnt->master_node)
>
> shouldn't it?
>
>
> ... Which raises the question: How did you test these patches? :)
>
I mentioned it in the cover letter, but seems to have dropped it -
they are untested.
There's a r600 card close-by I could test with, but no amdgpu one :-\

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Michel Dänzer]

On 2018-04-10 11:47 AM, Emil Velikov wrote:
> On 10 April 2018 at 09:28, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> Seems like we've been leaking this for years. It became more obvious
>>> with the recent refactoring.
>>>
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>>  src/amdgpu_probe.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>> index 537d44c..588891c 100644
>>> --- a/src/amdgpu_probe.c
>>> +++ b/src/amdgpu_probe.c
>>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>>       return TRUE;
>>>
>>>  error:
>>> +     free(pPriv->ptr);
>>> +     pPriv->ptr = NULL;
>>>       return FALSE;
>>>  }
>>>
>>>
>>
>> valgrind doesn't report a leak if I force this error path; presumably
>> Xorg frees the private after returning FALSE here.
>>
> Just double-checked and Xorg does not know anything about ptr. The
> only one who clears it up is AMDGPUFreeScreen_KMS.
> 
> The magic (for this and the other 'leak') seems to be happening in
> xf86platformAddDevice. Namely:
>  - ::platformProbe is called via doPlatformProbe
>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>  - ::PreInit fails, ::configured is false
>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
> AMDGPUFreeScreen_KMS)
> 
> Eventually, I could unwrap all that although it makes sense to keep
> things simpler. As effectively done by the patch.
> 
> I believe you'll agree?

I'm afraid not. There's no leak because it's getting cleaned up as
designed, so there's no need for this change.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 19/19] TODO

[Emil Velikov]

On 10 April 2018 at 09:30, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>>  todo | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>  create mode 100644 todo
>>
>> diff --git a/todo b/todo
>> new file mode 100644
>> index 0000000..10c1ad5
>> --- /dev/null
>> +++ b/todo
>> @@ -0,0 +1,9 @@
>> + - on amdgpu_probe failure, the pScrn entry is leaked - missing api/examples?
>
> Might be similar to patch 11; does valgrind actually report a leak if
> you force this?
>
>
>> + - introduce xf86ConfigEntity and use it
>> + - remove embedded AMDGPUInfoRec::pEnt
>> + - consistently use gAMDGPUEntityIndex or getAMDGPUEntityIndex
>> + - consistently use of pEnt/entity_num -> pScrn->list[], AMDPRIV
>> + - kill off DRI_1_ DRICreatePCIBusID - demote again to DRI1 only in X codebase
>> + - compose bus string early & strcmp instead of device_match?
>> + - remove embedded AMDGPUInfoRec::PciInfo - reuse EntityInfoRec::chipset, GDevRec::chiIDi, amdgpu_gpu_info::asic_id or ...
>> + - use odev to fetch render_node?
>
> I'm afraid I don't really see these as important enough to be tracked
> like this.
>
Agreed - no reason to keep these in-tree.

Idea was to gather feedback on the topics. One example:
Do we need the getAMDGPUEntityIndex helper, considering ~half of the
existing codebase uses it. Yet other half references
gAMDGPUEntityIndex directly.

Most of the above, seem to be a copy/paste from the radeon driver,
which in turn is a copy from (?) and the original commit lacks any
information :-\

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Michel Dänzer]

On 2018-04-10 11:51 AM, Emil Velikov wrote:
> On 10 April 2018 at 09:29, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> Rename the variable to reflect what it is. Plus move it out of the dri2
>>> section - it's used in dri2 and dri3.
>>>
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>
>> [...]
>>
>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>> index 4959bd6..e9afe42 100644
>>> --- a/src/amdgpu_probe.c
>>> +++ b/src/amdgpu_probe.c
>>> @@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
>>>       if (pAMDGPUEnt->fd < 0)
>>>               return FALSE;
>>>
>>> +     pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
>>> +     if (pAMDGPUEnt->master_node)
>>> +            goto error_amdgpu;
>>
>> This should be
>>
>>         if (!pAMDGPUEnt->master_node)
>>
>> shouldn't it?
>>
>>
>> ... Which raises the question: How did you test these patches? :)
>>
> I mentioned it in the cover letter, but seems to have dropped it -
> they are untested.
> There's a r600 card close-by I could test with, but no amdgpu one :-\

Okay. I can probably test this series, but in general it's preferable
for patches to be tested before sending them out for review.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 10:58, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-10 11:47 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:28, Michel Dänzer <michel@daenzer.net> wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>>
>>>> Seems like we've been leaking this for years. It became more obvious
>>>> with the recent refactoring.
>>>>
>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>> ---
>>>>  src/amdgpu_probe.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>>> index 537d44c..588891c 100644
>>>> --- a/src/amdgpu_probe.c
>>>> +++ b/src/amdgpu_probe.c
>>>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>>>       return TRUE;
>>>>
>>>>  error:
>>>> +     free(pPriv->ptr);
>>>> +     pPriv->ptr = NULL;
>>>>       return FALSE;
>>>>  }
>>>>
>>>>
>>>
>>> valgrind doesn't report a leak if I force this error path; presumably
>>> Xorg frees the private after returning FALSE here.
>>>
>> Just double-checked and Xorg does not know anything about ptr. The
>> only one who clears it up is AMDGPUFreeScreen_KMS.
>>
>> The magic (for this and the other 'leak') seems to be happening in
>> xf86platformAddDevice. Namely:
>>  - ::platformProbe is called via doPlatformProbe
>>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>>  - ::PreInit fails, ::configured is false
>>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
>> AMDGPUFreeScreen_KMS)
>>
>> Eventually, I could unwrap all that although it makes sense to keep
>> things simpler. As effectively done by the patch.
>>
>> I believe you'll agree?
>
> I'm afraid not. There's no leak because it's getting cleaned up as
> designed, so there's no need for this change.
>
Fair enough. I'll swap the commit with a comment one for v2.
This way, the next person will be less tempted to send the same patch.

Something like:

pPriv->ptr is freed in our ::FreeScreen callback. Latter of which gets
called by xf86DeleteScreen() as the driver ::*Probe call fails.

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

On 10 April 2018 at 10:27, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-10 11:20 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:26, Michel Dänzer <michel@daenzer.net> wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>>
>>>> The former of these is a UMS artefact which gives incorrect and
>>>> misleading promise whether "KMS" is supported. Not to mention that
>>>> AMDGPU is a only KMS driver.
>>>>
>>>> In a similar fashion xf86LoadKernelModule() is a relic of the times,
>>>> where platforms had no scheme of detecting and loading the appropriate
>>>> kernel module.
>>>>
>>>> Cc: Robert Millan <rmh@freebsd.org>
>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>> ---
>>>> Robert, off the top of my head this should work with FreeBSD. Admittedly
>>>> I'm not an expert on the platform. Please give it a test.
>>>
>>> I want to get confirmation from Robert that this will work on FreeBSD
>>> now, since he explicitly restored the kernel module loading code in
>>> https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
>>> .
>>>
>> Fully agreed!. That's why I added him to the CC list.
>>
>> Throwing some ideas:
>>  - If it's still needed can we keep it !Linux only?
>
> The first drmCheckModesettingSupported call? Fine with me.
>
Since ifndef out the drmCheckModesettingSupported call makes
amdgpu_kernel_mode_enabled function a no-op, I was thinking about
ifndef the whole thing.
Yet again, it would be so much better to actually nuke it (if possible).

Robert, does one still need xf86LoadKernelModule and/or
drmCheckModesettingSupported for FreeBSD?

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

Re: [PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Emil Velikov]

On 10 April 2018 at 09:27, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index e65c83b..78cc005 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -33,6 +33,8 @@
>>  #include <errno.h>
>>  #include <string.h>
>>  #include <stdlib.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>>
>>  /*
>>   * Authors:
>> @@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>                                struct xf86_platform_device *platform_dev)
>>  {
>>       struct pci_device *dev;
>> +     const char *path;
>>       char *busid;
>>       int fd;
>>
>> -#ifdef ODEV_ATTRIB_FD
>>       if (platform_dev) {
>> +#ifdef ODEV_ATTRIB_FD
>>               fd = xf86_get_platform_device_int_attrib(platform_dev,
>>                                                        ODEV_ATTRIB_FD, -1);
>>               if (fd != -1)
>>                       return fd;
>> -     }
>>  #endif
>>
>> +#ifdef ODEV_ATTRIB_PATH
>
> This guard is superfluous: ODEV_ATTRIB_PATH was added in xserver 1.13,
> and we require >= 1.13.
>
Was respinning the patches and noticed that the guard is needed. Namely:

The ODEV_ATTRIB_FD macro is set in xf86platformBus.h which is included
only as XSERVER_PLATFORM_BUS is set.
We can use either macro as a guard, yet the former seems more natural/obvious.

What do you think?

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Michel Dänzer]

On 2018-04-24 08:13 PM, Emil Velikov wrote:
> On 10 April 2018 at 09:27, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>>  src/amdgpu_probe.c | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>> index e65c83b..78cc005 100644
>>> --- a/src/amdgpu_probe.c
>>> +++ b/src/amdgpu_probe.c
>>> @@ -33,6 +33,8 @@
>>>  #include <errno.h>
>>>  #include <string.h>
>>>  #include <stdlib.h>
>>> +#include <sys/stat.h>
>>> +#include <fcntl.h>
>>>
>>>  /*
>>>   * Authors:
>>> @@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>>                                struct xf86_platform_device *platform_dev)
>>>  {
>>>       struct pci_device *dev;
>>> +     const char *path;
>>>       char *busid;
>>>       int fd;
>>>
>>> -#ifdef ODEV_ATTRIB_FD
>>>       if (platform_dev) {
>>> +#ifdef ODEV_ATTRIB_FD
>>>               fd = xf86_get_platform_device_int_attrib(platform_dev,
>>>                                                        ODEV_ATTRIB_FD, -1);
>>>               if (fd != -1)
>>>                       return fd;
>>> -     }
>>>  #endif
>>>
>>> +#ifdef ODEV_ATTRIB_PATH
>>
>> This guard is superfluous: ODEV_ATTRIB_PATH was added in xserver 1.13,
>> and we require >= 1.13.
>>
> Was respinning the patches and noticed that the guard is needed. Namely:
> 
> The ODEV_ATTRIB_FD macro is set in xf86platformBus.h which is included
> only as XSERVER_PLATFORM_BUS is set.
> We can use either macro as a guard, yet the former seems more natural/obvious.

I see, yeah makes sense.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx