[Mesa-dev] [RFC] egl/android: Add DRM node probing and filtering
Robert Foss
robert.foss at collabora.com
Fri Apr 20 07:17:49 UTC 2018
On 04/20/2018 06:41 AM, Tomasz Figa wrote:
> Hi Rob,
>
> On Thu, Apr 19, 2018 at 1:03 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.
>
>> The filtering itself is done using the newly introduced
>> libdrm drmHandleMatch() call.
>
>> Signed-off-by: Robert Foss <robert.foss at collabora.com>
>> ---
>
>> This patch is based on[1], which contains a new libdrm function,
>> called drmHandleMatch(), which allows for matching an opened
>> drm node handle against some desired properties.
>
>> A choice that was made for this patch was to add support for
>> falling back to to DRM nodes that have failed the filtering,
>> if no node passes the filter.
>> If this wouldn't be useful to anyone, I would suggest ripping it
>> out since it is a little bit ugly&complex.
>
> This is actually not only useful, but quite of a requirement for Chrome OS,
> where we use the same Android image for devices with different GPU vendors.
> (Technically we could hack it up by injecting some per-board properties,
> but it would be painful from maintenance point of view.)
Ack, I'll remove the fallback.
>
> But actually, the way it is implemented in this patch will not work. The
> code assumes that the first DRM node that could be opened
> (loader_open_device()) is fine, but if you look at what we do in our
> Chromium patch [2], it is actually necessary to check if Mesa includes a
> driver for it (loader_get_driver_for_fd()).
>
> [2]
> https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/780797
Ack, I'll add a check for a driver existing.
>
>
>
>> [1] https://www.spinics.net/lists/dri-devel/msg172497.html
>
>
>> src/egl/drivers/dri2/platform_android.c | 72
> ++++++++++++++++++++++++++++-----
>> 1 file changed, 62 insertions(+), 10 deletions(-)
>
>> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
>> index 7f1a496ea24..0b082fe5dcc 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 <cutils/properties.h>
>> #include <errno.h>
>> #include <dlfcn.h>
>> #include <fcntl.h>
>> @@ -1117,18 +1118,69 @@ droid_add_configs_for_visuals(_EGLDriver *drv,
> _EGLDisplay *dpy)
>> 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;
>> + int prop_set, num_devices, ret;
>> + const int node_type = DRM_NODE_RENDER;
>> + int fd = -1, fallback_fd = -1;
>> +
>> + const int MAX_DRM_DEVICES = 32;
>> + char vendor_name[PROPERTY_VALUE_MAX];
>> + prop_set = property_get("drm.gpu.vendor_name", vendor_name, NULL);
>> + drmVersion ver_filter;
>> + ver_filter.name = vendor_name;
>> +
>> + drmDevicePtr devices[MAX_DRM_DEVICES];
>> + num_devices = drmGetDevices2(0, devices, MAX_DRM_DEVICES);
>> + if (num_devices < 0) {
>> + _eglLog(_EGL_WARNING, "Failed to find any DRM devices");
>> + return -1;
>> + }
>> +
>> + for (int i = 0; i < num_devices; i++) {
>> + /* Filter out DRM_NODE_ types we aren't interested in */
>> + if (!(devices[i]->available_nodes & (1 << node_type))) {
>> + continue;
>> + }
>> +
>> + /* Open DRM node FD */
>> + fd = loader_open_device(devices[i]->nodes[node_type]);
>> + if (fd == -1 && errno == EINVAL) {
>> + _eglLog(_EGL_WARNING, "%s() node #%d failed to open",
> __func__, i);
>> + continue;
>> + }
>
> What should happen if (fd == -1 && errno != EINVAL)? I don't think we
> should run the code below in such case.
>
>> +
>> + /* See if FD matches the driver vendor we want */
>> + if (prop_set && !drmHandleMatch(fd, &ver_filter, NULL)){
>> + _eglLog(_EGL_WARNING, "%s() node #%d FD=%d does not match
> filter", __func__, i , fd);
>
> How about printing the actual node (i.e. devices[i]->nodes[node_type]),
> rather than index in devices array?
>
> Also, should it be _EGL_DEBUG? I'd expect it to be a normal event, which
> should not concern the user.
>
>> + goto next;
>> + }
>> +
>> + /* Successfully found matching FD */
>> + close(fallback_fd);
>> + fallback_fd = -1;
>> + break;
>> +
>> +next:
>> + if (fallback_fd == -1) {
>> + fallback_fd = fd;
>> + fd = -1;
>> + } else {
>> + close(fd);
>> + fd = -1;
>> + }
>> + continue;
>> + }
>
> How about simplifying the code a bit:
>
> + for (int i = 0; i < num_devices; i++) {
> + /* Filter out DRM_NODE_ types we aren't interested in */
> + if (!(devices[i]->available_nodes & (1 << node_type))) {
> + continue;
> + }
> +
> + /* Open DRM node FD */
> + fd = loader_open_device(devices[i]->nodes[node_type]);
> + if (fd < 0) {
> + _eglLog(_EGL_WARNING, "%s() node #%d failed to open", __func__,
> i);
> + continue;
> + }
> +
> + if (fallback_fd < 0)
> + fallback_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> +
> + /* See if FD matches the driver vendor we want */
> + if (!prop_set || drmHandleMatch(fd, &ver_filter, NULL))
> + /* Successfully found matching FD */
> + break;
> +
> + _eglLog(_EGL_WARNING, "%s() node #%d FD=%d does not match filter",
> __func__, i , fd);
> + close(fd);
> + fd = -1;
> + }
> +
> + if (fd >= 0) {
> + close(fallback_fd);
> + fallback_fd = -1;
> + } else {
> + _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
> fallback");
> + fd = fallback_fd;
> + }
>> +
>> + if (fd < 0) {
>> + _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
> fallback");
>> + return fallback_fd;
>> }
>
> Best regards,
> Tomasz
>
More information about the mesa-dev
mailing list