[libdrm,4/5] xf86drm: introduce drmGetDevice[s]2

Commit Message

Emil Velikov Nov. 30, 2016, 8:35 p.m. UTC

From: Emil Velikov <[e-mail]>

Relative to the original version, here one can provide a flags bitmask.
Currently only DRM_DEVICE_IGNORE_PCI_REVISION is supported.

Implementation detail:
If it's set, we will only parse the separate sysfs files and we won't
touch the config one. The latter awakes the device (causing delays)
which is the core reason why this API was introduced.

Cc: Michel Dänzer <[e-mail]>
Cc: Nicolai Hähnle <[e-mail]>
Cc: Mauro Santos <[e-mail]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
Signed-off-by: Emil Velikov <[e-mail]>
---
Michel, Nicolai any naming suggestions or input in general will be
appreciated.
---
 xf86drm.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 xf86drm.h |  4 ++++
 2 files changed, 60 insertions(+), 8 deletions(-)

Comments

Michel Dänzer Dec. 1, 2016, 3:09 a.m. UTC | #1

On 01/12/16 05:35 AM, Emil Velikov wrote:
> From: Emil Velikov <[e-mail]>
> 
> Relative to the original version, here one can provide a flags bitmask.
> Currently only DRM_DEVICE_IGNORE_PCI_REVISION is supported.
> 
> Implementation detail:
> If it's set, we will only parse the separate sysfs files and we won't
> touch the config one. The latter awakes the device (causing delays)
> which is the core reason why this API was introduced.
> 
> Cc: Michel Dänzer <[e-mail]>
> Cc: Nicolai Hähnle <[e-mail]>
> Cc: Mauro Santos <[e-mail]>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <[e-mail]>
> ---
> Michel, Nicolai any naming suggestions or input in general will be
> appreciated.

It all looks good to me in general, thanks for doing this! I just have
a couple of minor suggestions for this patch which might make the code
clearer, feel free to take them or leave them. Either way, patches 3-5
are

Reviewed-by: Michel Dänzer <[e-mail]>


> @@ -2963,7 +2964,7 @@ static int parse_separate_sysfs_files(int maj, int min,
>      FILE *fp;
>      int ret;
>  
> -    for (unsigned i = 0; i < ARRAY_SIZE(attrs); i++) {
> +    for (unsigned i = (0 + !!ignore_revision); i < ARRAY_SIZE(attrs); i++) {

    for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {


> diff --git a/xf86drm.h b/xf86drm.h
> index 4da6bd3..1c6ed36 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -801,6 +801,10 @@ extern void drmFreeDevice(drmDevicePtr *device);
>  extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
>  extern void drmFreeDevices(drmDevicePtr devices[], int count);
>  
> +#define DRM_DEVICE_IGNORE_PCI_REVISION 0x0001

#define DRM_DEVICE_IGNORE_PCI_REVISION (1 << 0)

to make it clearer that flags will be separate bits, not enumeration values.

Michel Dänzer Dec. 1, 2016, 3:56 a.m. UTC | #2

On 01/12/16 12:09 PM, Michel Dänzer wrote:
> On 01/12/16 05:35 AM, Emil Velikov wrote:
>> From: Emil Velikov <[e-mail]>
>>
>> Relative to the original version, here one can provide a flags bitmask.
>> Currently only DRM_DEVICE_IGNORE_PCI_REVISION is supported.
>>
>> Implementation detail:
>> If it's set, we will only parse the separate sysfs files and we won't
>> touch the config one. The latter awakes the device (causing delays)
>> which is the core reason why this API was introduced.
>>
>> Cc: Michel Dänzer <[e-mail]>
>> Cc: Nicolai Hähnle <[e-mail]>
>> Cc: Mauro Santos <[e-mail]>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>> Signed-off-by: Emil Velikov <[e-mail]>
>> ---
>> Michel, Nicolai any naming suggestions or input in general will be
>> appreciated.
> 
> It all looks good to me in general, thanks for doing this! I just have
> a couple of minor suggestions for this patch which might make the code
> clearer, feel free to take them or leave them. Either way, patches 3-5
> are
> 
> Reviewed-by: Michel Dänzer <[e-mail]>

On further thought, I wonder if maybe drmGetDevice[s]2 should default to
not retrieving the PCI revision, unless a flag is set, say
DRM_DEVICE_GET_PCI_REVISION? That would avoid unnecessarily using the
config file if the caller forgets to set the flag even though it doesn't
need the revision.

Though in that case, maybe the revision_id field should also be set to
0xff without the flag, to avoid callers forgetting to set the flag and
getting an incorrect but plausible(?) 0 for the revision.

Emil Velikov Dec. 1, 2016, 1:35 p.m. UTC | #3

On 1 December 2016 at 03:56, Michel Dänzer <[e-mail]> wrote:
> On 01/12/16 12:09 PM, Michel Dänzer wrote:
>> On 01/12/16 05:35 AM, Emil Velikov wrote:
>>> From: Emil Velikov <[e-mail]>
>>>
>>> Relative to the original version, here one can provide a flags bitmask.
>>> Currently only DRM_DEVICE_IGNORE_PCI_REVISION is supported.
>>>
>>> Implementation detail:
>>> If it's set, we will only parse the separate sysfs files and we won't
>>> touch the config one. The latter awakes the device (causing delays)
>>> which is the core reason why this API was introduced.
>>>
>>> Cc: Michel Dänzer <[e-mail]>
>>> Cc: Nicolai Hähnle <[e-mail]>
>>> Cc: Mauro Santos <[e-mail]>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>> Signed-off-by: Emil Velikov <[e-mail]>
>>> ---
>>> Michel, Nicolai any naming suggestions or input in general will be
>>> appreciated.
>>
>> It all looks good to me in general, thanks for doing this! I just have
>> a couple of minor suggestions for this patch which might make the code
>> clearer, feel free to take them or leave them. Either way, patches 3-5
>> are
>>
>> Reviewed-by: Michel Dänzer <[e-mail]>
>
> On further thought, I wonder if maybe drmGetDevice[s]2 should default to
> not retrieving the PCI revision, unless a flag is set, say
> DRM_DEVICE_GET_PCI_REVISION? That would avoid unnecessarily using the
> config file if the caller forgets to set the flag even though it doesn't
> need the revision.
>
Not 100% sold on the reasoning (if someone forgets...) yet making the
revision opt-in (as opposed to opt-out) makes sense.

> Though in that case, maybe the revision_id field should also be set to
> 0xff without the flag, to avoid callers forgetting to set the flag and
> getting an incorrect but plausible(?) 0 for the revision.
>
All suggestions sound amazing, thank you !

Barring any objections, I'll re-spin the series and send one for Mesa
later on today.

Thanks again,
Emil

Patch

diff --git a/xf86drm.c b/xf86drm.c
index 701cf29..f117716 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2948,7 +2948,8 @@  static int drmGetMaxNodeName(void)
 
 #ifdef __linux__
 static int parse_separate_sysfs_files(int maj, int min,
-                                      drmPciDeviceInfoPtr device)
+                                      drmPciDeviceInfoPtr device,
+                                      bool ignore_revision)
 {
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
     static const char *attrs[] = {
@@ -2963,7 +2964,7 @@  static int parse_separate_sysfs_files(int maj, int min,
     FILE *fp;
     int ret;
 
-    for (unsigned i = 0; i < ARRAY_SIZE(attrs); i++) {
+    for (unsigned i = (0 + !!ignore_revision); i < ARRAY_SIZE(attrs); i++) {
         snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
                  attrs[i]);
         fp = fopen(path, "r");
@@ -2977,7 +2978,7 @@  static int parse_separate_sysfs_files(int maj, int min,
 
     }
 
-    device->revision_id = data[0] & 0xff;
+    device->revision_id = ignore_revision ? 0 : data[0] & 0xff;
     device->vendor_id = data[1] & 0xffff;
     device->device_id = data[2] & 0xffff;
     device->subvendor_id = data[3] & 0xffff;
@@ -3018,7 +3019,10 @@  static int drmParsePciDeviceInfo(int maj, int min,
                                  uint32_t flags)
 {
 #ifdef __linux__
-    if (parse_separate_sysfs_files(maj, min, device))
+    if (flags & DRM_DEVICE_IGNORE_PCI_REVISION)
+        return parse_separate_sysfs_files(maj, min, device, true);
+
+    if (parse_separate_sysfs_files(maj, min, device, false))
         return parse_config_sysfs_file(maj, min, device);
 
     return 0;
@@ -3125,16 +3129,24 @@  static void drmFoldDuplicatedDevices(drmDevicePtr local_devices[], int count)
     }
 }
 
+/* Check that the given flags are valid returning 0 on success */
+static int
+drm_device_validate_flags(uint32_t flags)
+{
+        return (flags & ~DRM_DEVICE_IGNORE_PCI_REVISION);
+}
+
 /**
  * Get information about the opened drm device
  *
  * \param fd file descriptor of the drm device
+ * \param flags feature/behaviour bitmask
  * \param device the address of a drmDevicePtr where the information
  *               will be allocated in stored
  *
  * \return zero on success, negative error code otherwise.
  */
-int drmGetDevice(int fd, drmDevicePtr *device)
+int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 {
     drmDevicePtr *local_devices;
     drmDevicePtr d;
@@ -3147,7 +3159,9 @@  int drmGetDevice(int fd, drmDevicePtr *device)
     int ret, i, node_count;
     int max_count = 16;
     dev_t find_rdev;
-    uint32_t flags = 0;
+
+    if (drm_device_validate_flags(flags))
+        return -EINVAL;
 
     if (fd == -1 || device == NULL)
         return -EINVAL;
@@ -3246,8 +3260,23 @@  free_locals:
 }
 
 /**
+ * Get information about the opened drm device
+ *
+ * \param fd file descriptor of the drm device
+ * \param device the address of a drmDevicePtr where the information
+ *               will be allocated in stored
+ *
+ * \return zero on success, negative error code otherwise.
+ */
+int drmGetDevice(int fd, drmDevicePtr *device)
+{
+    return drmGetDevice2(fd, 0, device);
+}
+
+/**
  * Get drm devices on the system
  *
+ * \param flags feature/behaviour bitmask
  * \param devices the array of devices with drmDevicePtr elements
  *                can be NULL to get the device number first
  * \param max_devices the maximum number of devices for the array
@@ -3257,7 +3286,7 @@  free_locals:
  *         alternatively the number of devices stored in devices[], which is
  *         capped by the max_devices.
  */
-int drmGetDevices(drmDevicePtr devices[], int max_devices)
+int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 {
     drmDevicePtr *local_devices;
     drmDevicePtr device;
@@ -3269,7 +3298,9 @@  int drmGetDevices(drmDevicePtr devices[], int max_devices)
     int maj, min;
     int ret, i, node_count, device_count;
     int max_count = 16;
-    uint32_t flags = 0;
+
+    if (drm_device_validate_flags(flags))
+        return -EINVAL;
 
     local_devices = calloc(max_count, sizeof(drmDevicePtr));
     if (local_devices == NULL)
@@ -3357,6 +3388,23 @@  free_locals:
     return ret;
 }
 
+/**
+ * Get drm devices on the system
+ *
+ * \param devices the array of devices with drmDevicePtr elements
+ *                can be NULL to get the device number first
+ * \param max_devices the maximum number of devices for the array
+ *
+ * \return on error - negative error code,
+ *         if devices is NULL - total number of devices available on the system,
+ *         alternatively the number of devices stored in devices[], which is
+ *         capped by the max_devices.
+ */
+int drmGetDevices(drmDevicePtr devices[], int max_devices)
+{
+    return drmGetDevices2(0, devices, max_devices);
+}
+
 char *drmGetDeviceNameFromFd2(int fd)
 {
 #ifdef __linux__
diff --git a/xf86drm.h b/xf86drm.h
index 4da6bd3..1c6ed36 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -801,6 +801,10 @@  extern void drmFreeDevice(drmDevicePtr *device);
 extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
 extern void drmFreeDevices(drmDevicePtr devices[], int count);
 
+#define DRM_DEVICE_IGNORE_PCI_REVISION 0x0001
+extern int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device);
+extern int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices);
+
 #if defined(__cplusplus)
 }
 #endif