Summary: | [GTK][WPE] Improve device detection in the GbmDevice | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alejandro G. Castro <alex> | ||||||
Component: | ANGLE | Assignee: | Alejandro G. Castro <alex> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, clord, cmarcelo, dino, ews-watchlist, gyuyoung.kim, kbr, kkinnunen, kondapallykalyan, luiz, ryuan.choi, sergio, webkit-bug-importer, zan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 237649 | ||||||||
Attachments: |
|
Description
Alejandro G. Castro
2022-02-10 03:39:41 PST
Created attachment 451514 [details]
Patch
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?) 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! 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. Created attachment 451569 [details]
Patch for landing
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]. 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. (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! :-) |