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

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

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