[Mesa-dev] [PATCH v3 3/3] egl/android: Add DRM node probing and filtering

[Robert Foss]

Hey Tomasz,

Thanks for the quick feedback.

On 2018-06-14 08:30, Tomasz Figa wrote:
> Hi Rob,
> 
> Thanks for sending v3. Please see few more comments inline.
> 
> On Sun, Jun 10, 2018 at 2:28 AM Robert Foss <robert.foss at collabora.com> wrote:
>>
>> This patch both adds support for probing & filtering DRM nodes
>> and switches away from using the GRALLOC_MODULE_PERFORM_GET_DRM_FD
>> gralloc call.
>>
>> Currently the filtering is based just on the driver name,
>> and the desired name is supplied using the "drm.gpu.vendor_name"
>> Android property.
>>
>> Signed-off-by: Robert Foss <robert.foss at collabora.com>
>> ---
>>
>> Changes since v2:
>>   - Switch from drmGetDevices2 to manual renderD node iteration
>>   - Add probe_res enum to communicate probing results better
>>   - Avoid using _eglError() in internal static functions
>>   - Avoid actually loading the driver while probing, just verify
>>     that it exists.
>>   - Replace strlen call with the assumed length PROPERTY_VALUE_MAX
> 
> [snip]
> 
>> +static probe_ret_t
>> +droid_probe_device(_EGLDisplay *disp, int fd, char *vendor)
>> +{
>> +   int ret;
>> +
>> +   drmVersionPtr ver = drmGetVersion(fd);
>> +   if (!ver)
>> +      return probe_error;
>> +
>> +   if (vendor != NULL && ver->name != NULL &&
>> +       strncmp(vendor, ver->name, PROPERTY_VALUE_MAX) != 0) {
> 
> Shouldn't we fail the match if vendor != NULL, but ver->name == NULL? i.e.
> 
> if (vendor && (!ver->name || strcmp(...)) { ...
> 

Yeah, overall that if-case is too much. I'll split out the NULL check separate
check that return an error.

>> +          ret = probe_filtered_out;
>> +          goto cleanup;
>> +       }
>> +
>> +
>> +   if (!droid_probe_driver(fd)) {
>> +      ret = probe_no_driver;
>> +      goto cleanup;
>> +   }
>> +
>> +   ret = probe_success;
>> +
>> +cleanup:
>> +   drmFreeVersion(ver);
>> +   return ret;
>> +}
>> +
>> +static int
>> +droid_open_device(_EGLDisplay *disp)
>> +{
>> +   const int MAX_DRM_DEVICES = 32;
>> +   int prop_set, num_devices;
>> +   int fd = -1, fallback_fd = -1;
>> +
>> +   char *vendor_name = NULL;
>> +   char vendor_buf[PROPERTY_VALUE_MAX];
>> +   if (property_get("drm.gpu.vendor_name", vendor_buf, NULL) > 0);
>> +      vendor_name = vendor_buf;
>> +
>> +   const char *drm_dir_name = "/dev/dri";
>> +   DIR *sysdir = opendir(drm_dir_name);
>> +   if (!sysdir)
>> +       return -errno;
>> +
>> +   struct dirent *dent;
>> +   while ((dent = readdir(sysdir))) {
>> +      char dev_path[128];
>> +      char *render_dev_prefix = "renderD";
>> +      size_t prefix_len = strlen(render_dev_prefix);
> 
> If a const array like below is used instead
> 
> const char render_dev_prefix[] = "renderD";
> 
> you can just use sizeof(render_dev_prefix), without the need for strlen().
> 

Ack.

>> +
>> +      if (strncmp(render_dev_prefix, dent->d_name, prefix_len) != 0)
>> +         continue;
>> +
>> +      sprintf(dev_path, "%s/%s", drm_dir_name, dent->d_name);
> 
> snprintf(dev_path, sizeof(dev_path), ...);
> 

Ack.

>> +      fd = loader_open_device(dev_path);
>> +      if (fd == -1) {
> 
> nit: Perhaps fd < 0, just to be safe?
> 

Ack.

>> +         _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s",
>> +                 __func__, dev_path);
>> +         continue;
>> +      }
>> +
>> +      int ret = droid_probe_device(disp, fd, vendor_name);
>> +      switch (ret) {
>> +      case probe_success:
>> +         goto success;
>> +      case probe_filtered_out:
>> +         goto allow_fallback;
> 
> The 2 lines of code at the "allow_fallback" label could be just moved here.
> 

Ack.

>> +      case probe_error:
>> +      case probe_no_driver:
> 
> Do we need 2 separate cases for these? Just one "probe_fail" should be enough.
> 

Ack.

>> +         goto next;
> 
> If we move the fallback handling to "probe_filtered_out" case, we
> could remove the "next" label too and simply "break" here.
> 

Ack.

>> +      }
>> +
>> +allow_fallback:
>> +      if (fallback_fd == -1)
>> +         fallback_fd = fd;
>> +next:
>> +      if (fallback_fd != fd)
>> +         close(fd);
>> +      fd = -1;
>> +      continue;
>> +   }
>> +
>> +success:
>> +   closedir(sysdir);
>> +
>> +   if (fallback_fd < 0 && fd < 0) {
>> +      _eglLog(_EGL_WARNING, "Failed to open any DRM device");
>> +      return -1;
>> +   }
>> +
>> +   if (fd < 0) {
>> +      _eglLog(_EGL_WARNING, "Failed to open desired DRM device, using fallback");
>> +      return fallback_fd;
>> +   }
>> +
>> +   close(fallback_fd);
>> +   return fd;
>> +}
> 
> [Leaving context for readability.]
> 
> Best regards,
> Tomasz
>