[PATCH 1/2] Add device enumeration interface (v2)

[PATCH 1/2] Add device enumeration interface (v2)

[Jammy Zhou]

drmGetDevices interface is added to enumernate GPU devices on the system

v2: rebase the code and some improvement for the coding style

Signed-off-by: Frank Min <Frank.Min@amd.com>
Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com> (v2)
---
 Makefile.am |  3 ++-
 xf86drm.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 xf86drm.h   | 18 ++++++++++++++++++
 3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 42d3d7f..8236ed8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -89,7 +89,8 @@ SUBDIRS = \
 libdrm_la_LTLIBRARIES = libdrm.la
 libdrm_ladir = $(libdir)
 libdrm_la_LDFLAGS = -version-number 2:4:0 -no-undefined
-libdrm_la_LIBADD = @CLOCK_LIB@
+libdrm_la_LIBADD = @CLOCK_LIB@ \
+		@PCIACCESS_LIBS@
 
 libdrm_la_CPPFLAGS = -I$(top_srcdir)/include/drm
 AM_CFLAGS = \
diff --git a/xf86drm.c b/xf86drm.c
index ffc53b8..4d67861 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -63,6 +63,7 @@
 
 #include "xf86drm.h"
 #include "libdrm.h"
+#include <pciaccess.h>
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
 #define DRM_MAJOR 145
@@ -2817,3 +2818,50 @@ char *drmGetRenderDeviceNameFromFd(int fd)
 {
 	return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
 }
+
+/**
+ * Enumerate the GPU devices on the system
+ *
+ * \param devs device array set to return the device information
+ * (if NULL, the number of device is returned)
+ * \param vendor the vendor ID for GPU devices to list
+ * (optional, if not specified, all GPU devices are returned)
+ *
+ * \return the number of GPU devices
+ */
+int drmGetDevices(drmDevicePtr devs, uint16_t vendor)
+{
+	struct pci_device_iterator * iter;
+	struct pci_device * dev;
+	uint32_t count = 0;
+
+	if (pci_system_init())
+		return -EINVAL;
+
+	iter = pci_slot_match_iterator_create(NULL);
+	if (!iter)
+		return -EINVAL;
+
+	while ((dev = pci_device_next(iter))) {
+		if (((dev->device_class == 0x30000) ||
+			(dev->device_class == 0x38000)) &&
+			((vendor == 0) || (dev->vendor_id == vendor))){
+			if (devs) {
+				devs[count].domain = dev->domain;
+				devs[count].bus = dev->bus;
+				devs[count].dev = dev->dev;
+				devs[count].func = dev->func;
+				devs[count].vendor_id = dev->vendor_id;
+				devs[count].device_id = dev->device_id;
+				devs[count].subvendor_id = dev->subvendor_id;
+				devs[count].subdevice_id = dev->subdevice_id;
+				devs[count].revision_id = dev->revision;
+			}
+			count++;
+		}
+	}
+
+	pci_iterator_destroy(iter);
+	pci_system_cleanup();
+	return count;
+}
diff --git a/xf86drm.h b/xf86drm.h
index 40c55c9..6150e71 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -752,6 +752,24 @@ extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
 extern char *drmGetPrimaryDeviceNameFromFd(int fd);
 extern char *drmGetRenderDeviceNameFromFd(int fd);
 
+/**
+ * Structure for a general pcie gpu device
+ */
+typedef struct _drmDevice {
+	uint16_t domain;
+	uint8_t bus;
+	uint8_t dev;
+	uint8_t func;
+	uint8_t revision_id;
+	uint16_t vendor_id;
+	uint16_t device_id;
+	uint16_t subvendor_id;
+	uint16_t subdevice_id;
+} drmDevice, *drmDevicePtr;
+
+
+extern int drmGetDevices(drmDevicePtr devs, uint16_t vendor);
+
 #if defined(__cplusplus) || defined(c_plusplus)
 }
 #endif
-- 
1.9.1

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

[PATCH 2/2] Fix one warning

[Jammy Zhou]

xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
  ^

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index 4d67861..fbda2aa 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
     }
 
     if (drm_server_info) {
-	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
+	group = serv_group;
 	chown_check_return(buf, user, group);
 	chmod(buf, devmode);
     }
-- 
1.9.1

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

Re: [PATCH 2/2] Fix one warning

[Jan Vesely]

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

On Fri, 2015-04-24 at 11:44 +0800, Jammy Zhou wrote:
> xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
>   group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
>   ^
> 
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>  xf86drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 4d67861..fbda2aa 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
>      }
>  
>      if (drm_server_info) {
> -	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> +	group = serv_group;

I don't think this change is correct. get_perms can return errors as
negative values. I found that xserver does, see [0] for my take on
fixing this, as well as Emil's response [1].

I think changing the condition to:
((int)serv_group >= 0)

should be ok(I don't think there are systems with GID_MAX greater than
2^31-1), but I never bothered sending v2.

jan


[0]http://lists.freedesktop.org/archives/dri-devel/2015-February/077276.html
[1]http://lists.freedesktop.org/archives/dri-devel/2015-February/078171.html



>  	chown_check_return(buf, user, group);
>  	chmod(buf, devmode);
>      }


-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

RE: [PATCH 2/2] Fix one warning

[Zhou, Jammy]

Thanks for sharing the background. For [0], you mentioned that get_perms may return -1 in some cases for the group, can you help indicate which case it is?

Since the drmSetServerInfo is seldom used, maybe we can just do the 'int' cast at this moment. I will send v2 for this.

Regards,
Jammy

-----Original Message-----
From: Jan Vesely [mailto:jv356@scarletmail.rutgers.edu] On Behalf Of Jan Vesely
Sent: Friday, April 24, 2015 10:30 PM
To: Zhou, Jammy
Cc: dri-devel@lists.freedesktop.org; Min, Frank
Subject: Re: [PATCH 2/2] Fix one warning

On Fri, 2015-04-24 at 11:44 +0800, Jammy Zhou wrote:
> xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
>   group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
>   ^
> 
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>  xf86drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 4d67861..fbda2aa 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
>      }
>  
>      if (drm_server_info) {
> -	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> +	group = serv_group;

I don't think this change is correct. get_perms can return errors as negative values. I found that xserver does, see [0] for my take on fixing this, as well as Emil's response [1].

I think changing the condition to:
((int)serv_group >= 0)

should be ok(I don't think there are systems with GID_MAX greater than 2^31-1), but I never bothered sending v2.

jan


[0]http://lists.freedesktop.org/archives/dri-devel/2015-February/077276.html
[1]http://lists.freedesktop.org/archives/dri-devel/2015-February/078171.html



>  	chown_check_return(buf, user, group);
>  	chmod(buf, devmode);
>      }


--
Jan Vesely <jan.vesely@rutgers.edu>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Add device enumeration interface (v2)

[Emil Velikov]

Hi Jammy, Frank

As far as I can see you're trying to get a different version of
drmGetBusid(). With the DRM_IOCTL_{G,S}ET_UNIQUE ioctl being lovely as
it is I do see your point, but I'm not sure that the current design will
be too useful.

Do we have any upcoming users for this new function, can you share a bit
about the usecase ?

On 24/04/15 03:44, Jammy Zhou wrote:
> drmGetDevices interface is added to enumernate GPU devices on the system
> 
> v2: rebase the code and some improvement for the coding style
> 
> Signed-off-by: Frank Min <Frank.Min@amd.com>
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com> (v2)
> ---
>  Makefile.am |  3 ++-
>  xf86drm.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  xf86drm.h   | 18 ++++++++++++++++++
>  3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 42d3d7f..8236ed8 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -89,7 +89,8 @@ SUBDIRS = \
>  libdrm_la_LTLIBRARIES = libdrm.la
>  libdrm_ladir = $(libdir)
>  libdrm_la_LDFLAGS = -version-number 2:4:0 -no-undefined
> -libdrm_la_LIBADD = @CLOCK_LIB@
> +libdrm_la_LIBADD = @CLOCK_LIB@ \
> +		@PCIACCESS_LIBS@
>  
>  libdrm_la_CPPFLAGS = -I$(top_srcdir)/include/drm
>  AM_CFLAGS = \
> diff --git a/xf86drm.c b/xf86drm.c
> index ffc53b8..4d67861 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -63,6 +63,7 @@
>  
>  #include "xf86drm.h"
>  #include "libdrm.h"
> +#include <pciaccess.h>
>  
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
>  #define DRM_MAJOR 145
> @@ -2817,3 +2818,50 @@ char *drmGetRenderDeviceNameFromFd(int fd)
>  {
>  	return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
>  }
> +
> +/**
> + * Enumerate the GPU devices on the system
> + *
> + * \param devs device array set to return the device information
> + * (if NULL, the number of device is returned)
> + * \param vendor the vendor ID for GPU devices to list
> + * (optional, if not specified, all GPU devices are returned)
> + *
> + * \return the number of GPU devices
> + */
> +int drmGetDevices(drmDevicePtr devs, uint16_t vendor)
> +{
> +	struct pci_device_iterator * iter;
> +	struct pci_device * dev;
> +	uint32_t count = 0;
> +
> +	if (pci_system_init())
> +		return -EINVAL;
> +
> +	iter = pci_slot_match_iterator_create(NULL);
> +	if (!iter)
> +		return -EINVAL;
> +
> +	while ((dev = pci_device_next(iter))) {
> +		if (((dev->device_class == 0x30000) ||
> +			(dev->device_class == 0x38000)) &&
Any particular reason why "3D controller" (0x32000) is omitted ?

> +			((vendor == 0) || (dev->vendor_id == vendor))){
> +			if (devs) {
> +				devs[count].domain = dev->domain;
> +				devs[count].bus = dev->bus;
> +				devs[count].dev = dev->dev;
> +				devs[count].func = dev->func;
> +				devs[count].vendor_id = dev->vendor_id;
> +				devs[count].device_id = dev->device_id;
> +				devs[count].subvendor_id = dev->subvendor_id;
> +				devs[count].subdevice_id = dev->subdevice_id;
> +				devs[count].revision_id = dev->revision;
> +			}
> +			count++;
> +		}
> +	}
> +
> +	pci_iterator_destroy(iter);
> +	pci_system_cleanup();
Using libpciaccess, will give you the number of PCI devices available on
the system rather than the ones accessible - think about platform
devices and/or devices without a drm driver.

Another solution will be to get the information based on the
primary/control/render nodes available. This will also allow one to know
what the device can be used for - be that via a separate parameter set
by the function or having different functions altogether.

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

Re: [PATCH 2/2] Fix one warning

[Jan Vesely]

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

On Mon, 2015-04-27 at 01:40 +0000, Zhou, Jammy wrote:
> Thanks for sharing the background. For [0], you mentioned that
> get_perms may return -1 in some cases for the group, can you help
> indicate which case it is?

in xserver:
function dr_drm_get_perms (hw/xfree86/dri/dri.c:759) copies previously
initialized values of xf86ConfigDRI.

Those values are set in configDRI (hw/xfree86/common/xf86Config.c:2166),
which is called from xf86HandleConfigFile
(hw/xfree86/common/xf86Config.c:2479)
and provided values parsed in
xf86readConfigFile(hw/xfree86/parser/read.c:89), group is only set in
dri section.

I did not really look into details. to me it looks like -1 is stored in
group unless there is a valid DRI section with group assigned name or
gid.

I did not look for other users of libdrm that might also use neg values
to indicate errors. You might want to ask someone who is more familiar
with the design than just reading the code (like I did)


regards,
jan


> 
> Since the drmSetServerInfo is seldom used, maybe we can just do the 'int' cast at this moment. I will send v2 for this.
> 
> Regards,
> Jammy
> 
> -----Original Message-----
> From: Jan Vesely [mailto:jv356@scarletmail.rutgers.edu] On Behalf Of Jan Vesely
> Sent: Friday, April 24, 2015 10:30 PM
> To: Zhou, Jammy
> Cc: dri-devel@lists.freedesktop.org; Min, Frank
> Subject: Re: [PATCH 2/2] Fix one warning
> 
> On Fri, 2015-04-24 at 11:44 +0800, Jammy Zhou wrote:
> > xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
> >   group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> >   ^
> > 
> > Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> > ---
> >  xf86drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 4d67861..fbda2aa 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
> >      }
> >  
> >      if (drm_server_info) {
> > -	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> > +	group = serv_group;
> 
> I don't think this change is correct. get_perms can return errors as negative values. I found that xserver does, see [0] for my take on fixing this, as well as Emil's response [1].
> 
> I think changing the condition to:
> ((int)serv_group >= 0)
> 
> should be ok(I don't think there are systems with GID_MAX greater than 2^31-1), but I never bothered sending v2.
> 
> jan
> 
> 
> [0]http://lists.freedesktop.org/archives/dri-devel/2015-February/077276.html
> [1]http://lists.freedesktop.org/archives/dri-devel/2015-February/078171.html
> 
> 
> 
> >  	chown_check_return(buf, user, group);
> >  	chmod(buf, devmode);
> >      }
> 
> 
> --
> Jan Vesely <jan.vesely@rutgers.edu>

-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

RE: [PATCH 1/2] Add device enumeration interface (v2)

[Zhou, Jammy]

Hi Emil,

This interface is intended for multiple GPU support. For example, we need to know how many GPU devices are available on the system (and for some specific vendor) in the client drivers, and then we can select proper devices for rendering/compute/etc. For current mesa and Xserver implementations, the device enumeration is done separately. I think it will be helpful if we can have such kind of function in libdrm core, which can also be leveraged by other new APIs requiring multi GPU support.

> Any particular reason why "3D controller" (0x32000) is omitted ?
No. For AMD cards, we currently have 0x30000 and 0x38000. Is 0x32000 used by Nvidia cards? If so, I think we should add it as well.

> Using libpciaccess, will give you the number of PCI devices available on the system rather than the ones accessible - think about platform devices and/or devices without a drm driver.
This interface is just to enumerate the PCIE GPU devices on the system. With regard to which ones are accessible, we can use drmOpen/drmOpenWithType to check, and I don't want to have duplicated functionalities for these interfaces. And for those non-PCIE platform devices (mostly on ARM platforms), this interface shouldn't be used, and instead the client drivers should handle it by themselves.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Tuesday, April 28, 2015 3:07 AM
To: Zhou, Jammy; dri-devel@lists.freedesktop.org
Cc: emil.l.velikov@gmail.com; Min, Frank
Subject: Re: [PATCH 1/2] Add device enumeration interface (v2)

Hi Jammy, Frank

As far as I can see you're trying to get a different version of drmGetBusid(). With the DRM_IOCTL_{G,S}ET_UNIQUE ioctl being lovely as it is I do see your point, but I'm not sure that the current design will be too useful.

Do we have any upcoming users for this new function, can you share a bit about the usecase ?

On 24/04/15 03:44, Jammy Zhou wrote:
> drmGetDevices interface is added to enumernate GPU devices on the 
> system
> 
> v2: rebase the code and some improvement for the coding style
> 
> Signed-off-by: Frank Min <Frank.Min@amd.com>
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com> (v2)
> ---
>  Makefile.am |  3 ++-
>  xf86drm.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  xf86drm.h   | 18 ++++++++++++++++++
>  3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.am b/Makefile.am index 42d3d7f..8236ed8 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -89,7 +89,8 @@ SUBDIRS = \
>  libdrm_la_LTLIBRARIES = libdrm.la
>  libdrm_ladir = $(libdir)
>  libdrm_la_LDFLAGS = -version-number 2:4:0 -no-undefined 
> -libdrm_la_LIBADD = @CLOCK_LIB@
> +libdrm_la_LIBADD = @CLOCK_LIB@ \
> +		@PCIACCESS_LIBS@
>  
>  libdrm_la_CPPFLAGS = -I$(top_srcdir)/include/drm  AM_CFLAGS = \ diff 
> --git a/xf86drm.c b/xf86drm.c index ffc53b8..4d67861 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -63,6 +63,7 @@
>  
>  #include "xf86drm.h"
>  #include "libdrm.h"
> +#include <pciaccess.h>
>  
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || 
> defined(__DragonFly__)  #define DRM_MAJOR 145 @@ -2817,3 +2818,50 @@ 
> char *drmGetRenderDeviceNameFromFd(int fd)  {
>  	return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);  }
> +
> +/**
> + * Enumerate the GPU devices on the system
> + *
> + * \param devs device array set to return the device information
> + * (if NULL, the number of device is returned)
> + * \param vendor the vendor ID for GPU devices to list
> + * (optional, if not specified, all GPU devices are returned)
> + *
> + * \return the number of GPU devices
> + */
> +int drmGetDevices(drmDevicePtr devs, uint16_t vendor) {
> +	struct pci_device_iterator * iter;
> +	struct pci_device * dev;
> +	uint32_t count = 0;
> +
> +	if (pci_system_init())
> +		return -EINVAL;
> +
> +	iter = pci_slot_match_iterator_create(NULL);
> +	if (!iter)
> +		return -EINVAL;
> +
> +	while ((dev = pci_device_next(iter))) {
> +		if (((dev->device_class == 0x30000) ||
> +			(dev->device_class == 0x38000)) &&
Any particular reason why "3D controller" (0x32000) is omitted ?

> +			((vendor == 0) || (dev->vendor_id == vendor))){
> +			if (devs) {
> +				devs[count].domain = dev->domain;
> +				devs[count].bus = dev->bus;
> +				devs[count].dev = dev->dev;
> +				devs[count].func = dev->func;
> +				devs[count].vendor_id = dev->vendor_id;
> +				devs[count].device_id = dev->device_id;
> +				devs[count].subvendor_id = dev->subvendor_id;
> +				devs[count].subdevice_id = dev->subdevice_id;
> +				devs[count].revision_id = dev->revision;
> +			}
> +			count++;
> +		}
> +	}
> +
> +	pci_iterator_destroy(iter);
> +	pci_system_cleanup();
Using libpciaccess, will give you the number of PCI devices available on the system rather than the ones accessible - think about platform devices and/or devices without a drm driver.

Another solution will be to get the information based on the primary/control/render nodes available. This will also allow one to know what the device can be used for - be that via a separate parameter set by the function or having different functions altogether.

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

Re: [PATCH 2/2] Fix one warning

[Jan Vesely]

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

On Mon, 2015-04-27 at 01:40 +0000, Zhou, Jammy wrote:
> Thanks for sharing the background. For [0], you mentioned that
> get_perms may return -1 in some cases for the group, can you help
> indicate which case it is?

The one i found is in xserver:
dri_drm_get_perms (hw/xfree86/dri/dri.c:759) copies values from
xf86ConfigDRI.

xf86configDRI is initialized in
configDRI(hw/xfree86/common/xf86Config.c:2166).
However, the default value if the DRI section is not present or does not
contain group setting is -1.

it looks like it relies on libdrm to fall back to default in that case,
and it looks like that path currently broken

I don't claim to fully understand what that old code is doing/supposed
to do, but scanning through it suggests that negative values are legal
way to report errors/undefined values.

there might be other users as well

jan

> 
> Since the drmSetServerInfo is seldom used, maybe we can just do the
> 'int' cast at this moment. I will send v2 for this.
> 
> Regards,
> Jammy
> 
> -----Original Message-----
> From: Jan Vesely [mailto:jv356@scarletmail.rutgers.edu] On Behalf Of Jan Vesely
> Sent: Friday, April 24, 2015 10:30 PM
> To: Zhou, Jammy
> Cc: dri-devel@lists.freedesktop.org; Min, Frank
> Subject: Re: [PATCH 2/2] Fix one warning
> 
> On Fri, 2015-04-24 at 11:44 +0800, Jammy Zhou wrote:
> > xf86drm.c:356:2: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
> >   group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> >   ^
> > 
> > Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> > ---
> >  xf86drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 4d67861..fbda2aa 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -353,7 +353,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
> >      }
> >  
> >      if (drm_server_info) {
> > -	group = (serv_group >= 0) ? serv_group : DRM_DEV_GID;
> > +	group = serv_group;
> 
> I don't think this change is correct. get_perms can return errors as negative values. I found that xserver does, see [0] for my take on fixing this, as well as Emil's response [1].
> 
> I think changing the condition to:
> ((int)serv_group >= 0)
> 
> should be ok(I don't think there are systems with GID_MAX greater than 2^31-1), but I never bothered sending v2.
> 
> jan
> 
> 
> [0]http://lists.freedesktop.org/archives/dri-devel/2015-February/077276.html
> [1]http://lists.freedesktop.org/archives/dri-devel/2015-February/078171.html
> 
> 
> 
> >  	chown_check_return(buf, user, group);
> >  	chmod(buf, devmode);
> >      }
> 
> 
> --
> Jan Vesely <jan.vesely@rutgers.edu>

-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] Add device enumeration interface (v2)

[Emil Velikov]

On 28 April 2015 at 04:26, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Hi Emil,
>
> This interface is intended for multiple GPU support. For example, we need to know how many GPU devices are available on the system (and for some specific vendor) in the client drivers, and then we can select proper devices for rendering/compute/etc. For current mesa and Xserver implementations, the device enumeration is done separately. I think it will be helpful if we can have such kind of function in libdrm core, which can also be leveraged by other new APIs requiring multi GPU support.
>
Hmm I'm not sure how the proposed interface will ease either mesa or
xserver's implementation. The former is used only for clover(opencl)
and already handles platform devices. While for the server I believe
(haven't checked) that it just "throws" the PCI device information to
the ddx and lets the latter do its thing.

>> Any particular reason why "3D controller" (0x32000) is omitted ?
> No. For AMD cards, we currently have 0x30000 and 0x38000. Is 0x32000 used by Nvidia cards? If so, I think we should add it as well.
>
What I am thinking is that using heuristics such as these will either
1) work for vendor A but not for B or 2) will be OK for B but will
produce false positives for A.

>> Using libpciaccess, will give you the number of PCI devices available on the system rather than the ones accessible - think about platform devices and/or devices without a drm driver.
> This interface is just to enumerate the PCIE GPU devices on the system. With regard to which ones are accessible, we can use drmOpen/drmOpenWithType to check, and I don't want to have duplicated functionalities for these interfaces. And for those non-PCIE platform devices (mostly on ARM platforms), this interface shouldn't be used, and instead the client drivers should handle it by themselves.
>
I am against duplication, to the point that I may have alienated a
person or two :-\ Although this function as-is won't bring much
benefit to mesa/xserver afaict. Plus it would be nice to keep an open
mind for platform world, so that things will just work when AMD
decides to go that road. Not to mention that iterating through all the
devices in drmOpen* just to find that the device at pci:X:Y provides
only a primary/render node seems a bit wasteful.


A trivial lookup in sysfs will be able to provide all the required
information, won't you agree ?


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

RE: [PATCH 1/2] Add device enumeration interface (v2)

[Zhou, Jammy]

> 1) work for vendor A but not for B or 2) will be OK for B but will produce false positives for A.
For such kind of cases, IMHO we probably can have vendor specific implementations.

> A trivial lookup in sysfs will be able to provide all the required information, won't you agree ?
Yes, I agree. I think we can use the udev interfaces for such kind of enumeration by doing the match on the drm subsystem (the assumption is that the drm driver is loaded for the graphics device already). We will do some prototyping with udev for this.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Wednesday, April 29, 2015 12:18 AM
To: Zhou, Jammy
Cc: dri-devel@lists.freedesktop.org; Min, Frank
Subject: Re: [PATCH 1/2] Add device enumeration interface (v2)

On 28 April 2015 at 04:26, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Hi Emil,
>
> This interface is intended for multiple GPU support. For example, we need to know how many GPU devices are available on the system (and for some specific vendor) in the client drivers, and then we can select proper devices for rendering/compute/etc. For current mesa and Xserver implementations, the device enumeration is done separately. I think it will be helpful if we can have such kind of function in libdrm core, which can also be leveraged by other new APIs requiring multi GPU support.
>
Hmm I'm not sure how the proposed interface will ease either mesa or xserver's implementation. The former is used only for clover(opencl) and already handles platform devices. While for the server I believe (haven't checked) that it just "throws" the PCI device information to the ddx and lets the latter do its thing.

>> Any particular reason why "3D controller" (0x32000) is omitted ?
> No. For AMD cards, we currently have 0x30000 and 0x38000. Is 0x32000 used by Nvidia cards? If so, I think we should add it as well.
>
What I am thinking is that using heuristics such as these will either
1) work for vendor A but not for B or 2) will be OK for B but will produce false positives for A.

>> Using libpciaccess, will give you the number of PCI devices available on the system rather than the ones accessible - think about platform devices and/or devices without a drm driver.
> This interface is just to enumerate the PCIE GPU devices on the system. With regard to which ones are accessible, we can use drmOpen/drmOpenWithType to check, and I don't want to have duplicated functionalities for these interfaces. And for those non-PCIE platform devices (mostly on ARM platforms), this interface shouldn't be used, and instead the client drivers should handle it by themselves.
>
I am against duplication, to the point that I may have alienated a person or two :-\ Although this function as-is won't bring much benefit to mesa/xserver afaict. Plus it would be nice to keep an open mind for platform world, so that things will just work when AMD decides to go that road. Not to mention that iterating through all the devices in drmOpen* just to find that the device at pci:X:Y provides only a primary/render node seems a bit wasteful.


A trivial lookup in sysfs will be able to provide all the required information, won't you agree ?


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