Bug 236436 – [GTK][WPE] Improve device detection in the GbmDevice

• Component: ANGLE
• Status: RESOLVED FIXED

[Alejandro G. Castro]

Currently we are using the filename directly, we need to use drm to detect the devices in a more flexible way.

[Alejandro G. Castro]

Created attachment 451514 [details]
Patch

[Chris Lord]

Comment on attachment 451514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451514&action=review

Much better! However, this currently includes an invalid memory write error - that needs to be addressed before commit and I think the other comment should also be addressed to avoid possible errors in future. You can assume my r+ with those fixed (but feel free to ping me for review if you aren't in a rush :))

> Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74
> +    memset(devices, 0, sizeof(drmDevicePtr) * 64);

This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own.

> Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:76
> +    int numDevices = drmGetDevices2(0, devices, 64);

Rather than putting 64 here, it would be good to use an array length macro (something of the form `#define ARRAY_LENGTH(x) (sizeof(x) / sizeof((x)[0]))` - I expect we have something like this already in WTF?)

[Alejandro G. Castro]

Thanks for the feedbak!

(In reply to Chris Lord from comment #2)
> Comment on attachment 451514 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451514&action=review
> 
> Much better! However, this currently includes an invalid memory write error
> - that needs to be addressed before commit and I think the other comment
> should also be addressed to avoid possible errors in future. You can assume
> my r+ with those fixed (but feel free to ping me for review if you aren't in
> a rush :))
> 
> > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74
> > +    memset(devices, 0, sizeof(drmDevicePtr) * 64);
> 
> This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own.
> 

Oops, yep :-).

> > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:76
> > +    int numDevices = drmGetDevices2(0, devices, 64);
> 
> Rather than putting 64 here, it would be good to use an array length macro
> (something of the form `#define ARRAY_LENGTH(x) (sizeof(x) /
> sizeof((x)[0]))` - I expect we have something like this already in WTF?)

Ok, I'll try to find something

Thanks for the review!

[Chris Lord]

Comment on attachment 451514 [details]
Patch

> > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:74
> > +    memset(devices, 0, sizeof(drmDevicePtr) * 64);
> 
> This * 64 is incorrect, sizeof(drmDevicePtr) should be correct on its own.

Whoops, we just spoke about this and I wanted to add here - I misread this and what's there is fine, but `sizeof(devices)` along with the other suggested change would make this patch more robust. Changing my r- to an r+ to reflect that mistake.

[Alejandro G. Castro]

Created attachment 451569 [details]
Patch for landing

[EWS]

Committed r289575 (247092@main): <https://commits.webkit.org/247092@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451569 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88777827>

[Zan Dobersek]

Comment on attachment 451569 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=451569&action=review

> Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:58
> +    int fd = getDeviceFd();

This file descriptor, when valid, gets leaked. gbm_device doesn't own it.

> Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:71
> +int GBMDevice::getDeviceFd()

This should be a static method. Better yet, if GBMDevice is kept as thread-specific, this file descriptor could instead be global, and the drmGetDevices2 lookup would then only be done once, not for every initialization.

[Alejandro G. Castro]

(In reply to Zan Dobersek from comment #8)
> Comment on attachment 451569 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451569&action=review
> 
> > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:58
> > +    int fd = getDeviceFd();
> 
> This file descriptor, when valid, gets leaked. gbm_device doesn't own it.
> 
> > Source/WebCore/platform/graphics/gbm/GBMDevice.cpp:71
> > +int GBMDevice::getDeviceFd()
> 
> This should be a static method. Better yet, if GBMDevice is kept as
> thread-specific, this file descriptor could instead be global, and the
> drmGetDevices2 lookup would then only be done once, not for every
> initialization.

Thanks for the comments! I'll address this and some other proposals about the drm device to choose in a following patch! :-)