Bug 236436

Summary: [GTK][WPE] Improve device detection in the GbmDevice
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: ANGLEAssignee: 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 Flags
Patch
none
Patch for landing none

Description Alejandro G. Castro 2022-02-10 03:39:41 PST
Currently we are using the filename directly, we need to use drm to detect the devices in a more flexible way.
Comment 1 Alejandro G. Castro 2022-02-10 03:57:11 PST
Created attachment 451514 [details]
Patch
Comment 2 Chris Lord 2022-02-10 04:24:46 PST
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?)
Comment 3 Alejandro G. Castro 2022-02-10 06:15:39 PST
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 4 Chris Lord 2022-02-10 07:26:40 PST
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.
Comment 5 Alejandro G. Castro 2022-02-10 10:48:42 PST
Created attachment 451569 [details]
Patch for landing
Comment 6 EWS 2022-02-10 14:18:52 PST
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 7 Radar WebKit Bug Importer 2022-02-10 14:19:34 PST
<rdar://problem/88777827>
Comment 8 Zan Dobersek 2022-02-14 03:32:07 PST
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.
Comment 9 Alejandro G. Castro 2022-02-14 04:57:01 PST
(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! :-)