Hey Tomasz,
On 2018-05-25 09:27, Tomasz Figa wrote:
> Hi Rob,
>
> Finally got to review this. Please see my comments inline.
>
> On Fri, May 11, 2018 at 10:48 PM Robert Foss <robert.f...@collabora.com>
> wrote:
> [snip]
>> +EGLBoolean
>> +droid_load_driver(_EGLDisplay *disp)
>
> Since this is not EGL-facing, I'd personally use bool.
>
>> +{
>> + struct dri2_egl_display *dri2_dpy = disp->DriverData;
>> + const char *err;
>> +
>> + dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
>> + if (dri2_dpy->driver_name == NULL) {
>> + err = "DRI2: failed to get driver name";
>> + goto error;
>
> It shouldn't be an error if there is no driver for given render node. We
> should just skip it and try next one, which I believe would be achieved by
> just returning false here.
>
>> + }
>> +
>> + dri2_dpy->is_render_node = drmGetNodeTypeFromFd(dri2_dpy->fd) ==
> DRM_NODE_RENDER;
>> +
>> + if (!dri2_dpy->is_render_node) {
>> + #ifdef HAVE_DRM_GRALLOC
>> + /* Handle control nodes using __DRI_DRI2_LOADER extension and GEM
> names
>> + * for backwards compatibility with drm_gralloc. (Do not use on
> new
>> + * systems.) */
>> + dri2_dpy->loader_extensions = droid_dri2_loader_extensions;
>> + if (!dri2_load_driver(disp)) {
>> + err = "DRI2: failed to load driver";
>> + goto error;
>> + }
>> + #else
>> + err = "DRI2: handle is not for a render node";
>> + goto error;
>> + #endif
>> + } else {
>> + dri2_dpy->loader_extensions = droid_image_loader_extensions;
>> + if (!dri2_load_driver_dri3(disp)) {
>> + err = "DRI3: failed to load driver";
>> + goto error;
>> + }
>> + }
>> +
>> + return EGL_TRUE;
>> +
>> +error:
>> + free(dri2_dpy->driver_name);
>> + dri2_dpy->driver_name = NULL;
>> + return _eglError(EGL_NOT_INITIALIZED, err);
>
> Hmm, if we signal EGL error here, we should break the probing loop and just
> bail out. This would suggest that a boolean is not the right type for this
> function to return. Perhaps something like negative error, 0 for skip and 1
> for success would make sense?
>
> Also, how does it play with the _eglError() called from the error path of
> dri2_initialize_android()?
I can't say I put any though into that aspect, but dri2_initialize_android()
would overwrite it. So maybe completely avoiding _eglError() in
droid_load_driver() is the way to go.
>
>> +}
>> +
>> +static int
>> +droid_probe_driver(_EGLDisplay *disp, int fd)
>> +{
>> + struct dri2_egl_display *dri2_dpy = disp->DriverData;
>> + dri2_dpy->fd = fd;
>> +
>> + if (!droid_load_driver(disp))
>> + return false;
>
> Given my other suggestion about distinguishing failure, render node skip
> and success, I think it should be more like this:
>
> int ret = droid_load_driver(disp);
> if (ret <= 0)
> return ret;
>
> Or actually, maybe we don't really need to go as far as loading the driver.
> I'd say it should be enough to just check if we have a driver for the
> device by looking at what loader_get_driver_for_fd() returns. (In that
> case, we can ignore my comment about returning error on
> loader_get_driver_for_fd() failure in droid_load_driver(), since the
> skipping would be handling only here.)
If we don't need to actually load the driver, I think all of the above
comments can be fixed by just removing chunks out of dri related setup.
>
>> +
>> + /* Since this probe can succeed, but another filter may fail,
>
> What another filter could fail? I can see the vendor name being checked
> before calling this function.
>
> The free() below is actually needed, just the comment is off. We need to
> free the name to be able to probe remaining nodes, without leaking the name.
Ack, fixed by the above change.
>
>> + this string needs to be deallocated either way.
>> + Once an FD has been found, this string will be set a second time.
> */
>> + free(dri2_dpy->driver_name);
>
> Don't we also need to unload the driver?
Ack, fixed by the above change.
>
>> + dri2_dpy->driver_name = NULL;
>> + return true;
>
> To match the change above:
>
> return 1;
>
Ack, fixed by the above change.
>> +}
>> +
>> +static int
>> +droid_probe_device(_EGLDisplay *disp, int fd, drmDevicePtr dev, char
> *vendor)
>> +{
>> + drmVersionPtr ver = drmGetVersion(fd);
>> + if (!ver)
>> + goto fail;
>
> Something wrong with indentation here.
Ack.
>
>> +
>> + size_t vendor_len = strlen(vendor);
>> + if (vendor_len != 0 && strncmp(vendor, ver->name, vendor_len))
>> + goto fail;
>
> Maybe it's just me, but I don't see any point in using strncmp() if the
> length argument is obtained by calling strlen() first. Especially if the
> strlen() call is on a string that comes from some external code
> (property_get()).
>
> Perhaps we could just call strncmp() with PROPERTY_VALUE_MAX? This would
> actually play nice with my other comment about using NULL for vendor
> string, if the property is not present.
Ack, that's reasonable. Replacing with PROPRTY_VALUE_MAX.
>
> Also nit: The label could be named in a more meaningful way, e.g.
> err_free_version.
>
Ack.
>> +
>> + if (!droid_probe_driver(disp, fd))
>> + goto fail;
>> +
>> + drmFreeVersion(ver);
>> + return true;
>> +
>> +fail:
>> + drmFreeVersion(ver);
>> + return false;
>
> Given my other suggestion about distinguishing failure, render node skip
> and success, I think it should be more like this:
>
> ret = droid_probe_driver(disp, fd);
> err_free_version:
> drmFreeVersion(ver);
> return ret;
>
Ack, this is fixed by removing all of the conditions that could cause an error
(but not the ones that cause skips) from the driver probing path.
So just using a bool signifying skip/no-skip should be ok.
>> +}
>> +
>> +static int
>> +droid_open_device(_EGLDisplay *disp)
>> +{
>> + const int MAX_DRM_DEVICES = 32;
>> + int prop_set, num_devices, ret;
>> + int fd = -1, fallback_fd = -1;
>> +
>> + char vendor_name[PROPERTY_VALUE_MAX];
>> + property_get("drm.gpu.vendor_name", vendor_name, NULL);
>
> vendor_name[] is uninitialized. property_get() with NULL 3rd argument
> wouldn't touch the 2nd argument if the property is missing.
>
> However, I'd recommend checking the return value of property_get() and pass
> NULL as vendor_name to droid_probe_device() if it's <= 0. The check for
> existence of the filter will be simpler with this.
>
Ack, fixing it.
>> +
>> + drmDevicePtr devices[MAX_DRM_DEVICES];
>> + num_devices = drmGetDevices2(0, devices, MAX_DRM_DEVICES);
>> +
>> + if (num_devices < 0) {
>> + _eglLog(_EGL_WARNING, "Unable to find DRM devices, error %d",
> num_devices);
>> + return -1;
>> + }
>> +
>> + if (num_devices == 0) {
>> + _eglLog(_EGL_WARNING, "Failed to find any DRM devices");
>> + return -1;
>> + }
>> +
>> + for (int i = 0; i < num_devices; i++) {
>> + char *dev_path = devices[i]->nodes[DRM_NODE_RENDER];
>> + fd = loader_open_device(dev_path);
>> + if (fd == -1) {
>> + _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s",
>> + __func__, dev_path);
>> + continue;
>> + }
>> +
>> + if (!droid_probe_device(disp, fd, devices[i], vendor_name))
>> + goto next;
>> +
>> + break;
>
> Hmm.
>
> Why not just
>
> if (droid_probe_device(disp, fd, devices[i], vendor_name))
> break;
>
> ?
>
> But actually, we need to distinguish the cases of failure and simply render
> node not matching any drivers. We shouldn't use such render node as
> fallback.
>
> int ret = droid_probe_device(...);
> if (!ret)
> break; // Found desired render node.
> if (ret < 0)
> continue; // Did not match any drivers.
> // Matched a driver, but not our filter. Consider as fallback.
>
With respect to this feedback I've taken a slightly different path and added a
probe_result enum and cleaned up the goto labels.
>> +
>> +next:
>> + if (fallback_fd == -1) {
>> + fallback_fd = fd;
>> + fd = -1;
>> + } else {
>> + close(fd);
>> + fd = -1;
>> + }
>
> fd = -1; could be just put here, after the if.
>
Ack.
>> + continue;
>
> No need for this continue.
>
> Best regards,
> Tomasz
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
