Re: [Mesa-dev] [PATCH 3/3] egl/android: Add DRM node probing and filtering
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
> 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 t
Re: [Mesa-dev] [PATCH 3/3] egl/android: Add DRM node probing and filtering
Hi Rob,
Finally got to review this. Please see my comments inline.
On Fri, May 11, 2018 at 10:48 PM Robert Foss
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()?
> +}
> +
> +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.)
> +
> + /* 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.
> + 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?
> + dri2_dpy->driver_name = NULL;
> + return true;
To match the change above:
return 1;
> +}
> +
> +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.
> +
> + 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.
Also nit: The label could be named in a more meaningful way, e.g.
err_free_version.
> +
> + 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;
> +}
> +
> +static int
> +droid_open_device(_EGLDisplay *disp)
> +{
> + const int MAX_DRM_DEVICES = 32;
> + int prop_set, num_devices, ret;
> + int fd = -1, fallbac
[Mesa-dev] [PATCH 3/3] egl/android: Add DRM node probing and filtering
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.
The filtering itself is done using the newly introduced
libdrm drmHandleMatch() call.
Signed-off-by: Robert Foss
---
Changes since v1:
- Do not rely on libdrm for probing
- Distinguish between errors and when no drm devices are found
Changes since RFC:
- Rebased on newer libdrm drmHandleMatch patch
- Added support for driver probing
src/egl/drivers/dri2/platform_android.c | 202 +---
1 file changed, 149 insertions(+), 53 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_android.c
b/src/egl/drivers/dri2/platform_android.c
index 4ba96aad90..76e5474e48 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -27,6 +27,7 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include
#include
#include
#include
@@ -1130,31 +1131,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv,
_EGLDisplay *dpy)
return (config_count != 0);
}
-enum {
-/* perform(const struct gralloc_module_t *mod,
- * int op,
- * int *fd);
- */
-GRALLOC_MODULE_PERFORM_GET_DRM_FD = 0x4002,
-};
-
-static int
-droid_open_device(struct dri2_egl_display *dri2_dpy)
-{
- int fd = -1, err = -EINVAL;
-
- if (dri2_dpy->gralloc->perform)
- err = dri2_dpy->gralloc->perform(dri2_dpy->gralloc,
- GRALLOC_MODULE_PERFORM_GET_DRM_FD,
- &fd);
- if (err || fd < 0) {
- _eglLog(_EGL_WARNING, "fail to get drm fd");
- fd = -1;
- }
-
- return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
-}
-
static const struct dri2_egl_display_vtbl droid_display_vtbl = {
.authenticate = NULL,
.create_window_surface = droid_create_window_surface,
@@ -1215,6 +1191,151 @@ static const __DRIextension
*droid_image_loader_extensions[] = {
NULL,
};
+EGLBoolean
+droid_load_driver(_EGLDisplay *disp)
+{
+ 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;
+ }
+
+ 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);
+}
+
+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;
+
+ /* Since this probe can succeed, but another filter may fail,
+ 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);
+ dri2_dpy->driver_name = NULL;
+ return true;
+}
+
+static int
+droid_probe_device(_EGLDisplay *disp, int fd, drmDevicePtr dev, char *vendor)
+{
+ drmVersionPtr ver = drmGetVersion(fd);
+ if (!ver)
+ goto fail;
+
+ size_t vendor_len = strlen(vendor);
+ if (vendor_len != 0 && strncmp(vendor, ver->name, vendor_len))
+ goto fail;
+
+ if (!droid_probe_driver(disp, fd))
+ goto fail;
+
+ drmFreeVersion(ver);
+ return true;
+
+fail:
+ drmFreeVersion(ver);
+ return false;
+}
+
+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);
+
+ 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
