linux_drm_syncobj_v1: Part 4: Add DrmIoctlWrapper [chromium/src : main]
0 views
Skip to first unread message
Orko Garai (Gerrit)
Oct 10, 2024, 5:33:30 PM10/10/24
to Chromium LUCI CQ, Nick Yamane, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge and Nick Yamane
Orko Garai voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Kramer Ge
- Nick Yamane
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Yamane (Gerrit)
Oct 15, 2024, 10:16:28 AM10/15/24
to Orko Garai, Maksim Sisov, Chromium LUCI CQ, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge, Maksim Sisov and Orko Garai
Nick Yamane added 4 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 13 (Latest):
Nick Yamane . resolved
+msisov@ could you help on reviewing this? thanks
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.cc
Line 12, Patchset 13 (Latest):
// TODO remove workaround after libdrm in sysroot is updatedNick Yamane . unresolved
I assume this is blocking this CL from landing?
Is there any CL under review addressing it? If not, should we add a TODO+crbug to track it?
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
Line 178, Patchset 13 (Latest):
DrmIoctlWrapper* drm() const { return drm_.get(); }Nick Yamane . unresolved
Lets avoid using const for functions that return pointers to non-const.
File ui/ozone/platform/wayland/test/mock_drm_ioctl_wrapper.h
Line 49, Patchset 13 (Latest):
static bool fail_on_syncobj_handle_to_fd;Nick Yamane . unresolved
please use methods, unless the style guide mentions exceptions for mock/test classes?
Related details
Attention is currently required from:
- Kramer Ge
- Maksim Sisov
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Sisov (Gerrit)
Oct 16, 2024, 4:20:37 AM10/16/24
to Orko Garai, Chromium LUCI CQ, Nick Yamane, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge, Nick Yamane and Orko Garai
Maksim Sisov added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Nick Yamane . resolved
+msisov@ could you help on reviewing this? thanks
Attention is currently required from:
- Kramer Ge
- Nick Yamane
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Sisov (Gerrit)
Oct 16, 2024, 10:33:52 AM10/16/24
to Orko Garai, Chromium LUCI CQ, Nick Yamane, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge, Nick Yamane and Orko Garai
Maksim Sisov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Sisov
| Code-Review | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 14 (Latest):
Maksim Sisov . resolved
lgtm given Nick's comments are addressed
Related details
Attention is currently required from:
- Kramer Ge
- Nick Yamane
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Yamane (Gerrit)
Oct 16, 2024, 11:02:08 AM10/16/24
to Orko Garai, Maksim Sisov, Chromium LUCI CQ, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge and Orko Garai
Nick Yamane added 1 comment![]( "Open in Gerrit")
File ui/ozone/platform/wayland/test/mock_drm_ioctl_wrapper.h
Line 49, Patchset 13:
static bool fail_on_syncobj_handle_to_fd;Nick Yamane . unresolved
please use methods, unless the style guide mentions exceptions for mock/test classes?
Nick Yamane
Per offline discussion and after looking at how these are used in the subsequent CL, I believe it complies with the coding style. So, please, just add a comment above them advice about their usage/reset requirements or typical usage. thanks
Related details
Attention is currently required from:
- Kramer Ge
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Oct 16, 2024, 12:59:12 PM10/16/24
to Maksim Sisov, Chromium LUCI CQ, Nick Yamane, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge and Nick Yamane
Orko Garai added 3 comments![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.cc
Line 12, Patchset 13:
// TODO remove workaround after libdrm in sysroot is updatedNick Yamane . unresolved
I assume this is blocking this CL from landing?
Is there any CL under review addressing it? If not, should we add a TODO+crbug to track it?
Orko Garai
I think it's not a blocker as this just adds the API here if the `DRM_IOCTL_SYNCOBJ_EVENTFD` macro is not defined in libdrm version being used.
I added a crbug here.
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
Lets avoid using const for functions that return pointers to non-const.
Orko Garai
Done.
File ui/ozone/platform/wayland/test/mock_drm_ioctl_wrapper.h
Line 49, Patchset 13:
static bool fail_on_syncobj_handle_to_fd;Nick Yamane . resolved
Nick Yamaneplease use methods, unless the style guide mentions exceptions for mock/test classes?
Per offline discussion and after looking at how these are used in the subsequent CL, I believe it complies with the coding style. So, please, just add a comment above them advice about their usage/reset requirements or typical usage. thanks
Attention is currently required from:
- Kramer Ge
- Nick Yamane
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Yamane (Gerrit)
Oct 16, 2024, 2:57:39 PM10/16/24
to Orko Garai, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Kramer Ge, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge and Orko Garai
Nick Yamane voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nick Yamane
| Code-Review | +1 |
1 comment
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.cc
Line 12, Patchset 13:
// TODO remove workaround after libdrm in sysroot is updatedNick Yamane . resolved
Orko GaraiI assume this is blocking this CL from landing?
Is there any CL under review addressing it? If not, should we add a TODO+crbug to track it?
I think it's not a blocker as this just adds the API here if the `DRM_IOCTL_SYNCOBJ_EVENTFD` macro is not defined in libdrm version being used.
I added a crbug here.
Nick Yamane
Ack.
+thomasanderson@ who might help with the sysroot update I think.
Related details
Attention is currently required from:
- Kramer Ge
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Oct 16, 2024, 3:53:27 PM10/16/24
to Orko Garai, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Orko Garai
Kramer Ge added 1 comment![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.h
Line 15, Patchset 14:
class DrmIoctlWrapper {Kramer Ge . unresolved
Can this be named to specify for syncobj ioctl? I do not foresee that we do other drm ioctl's here.
Related details
Attention is currently required from:
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Oct 16, 2024, 5:31:51 PM10/16/24
to Orko Garai, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Orko Garai
Kramer Ge added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 16 (Latest):
Kramer Ge . resolved
Structurally drm syncobj related stuff is a gpu process responsibility, I think it's possible that we can keep drm node in the gpu process and transport the syncobj for the buffer from ozone/wayland/gpu to ozone/wayland/host, in a future possibility that shared_image maybe exporting timeline to syncobj.
But I understand it's an over-optimization so we don't want to do it now, at least we right now there's no syncobj equivalent of GpuFenceHandle.
Kramer Ge (Gerrit)
Oct 16, 2024, 5:35:11 PM10/16/24
to Orko Garai, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Orko Garai
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
1 comment
Patchset-level comments
Kramer Ge . resolved
% the question about naming
Orko Garai (Gerrit)
Oct 16, 2024, 6:13:36 PM10/16/24
to Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge
Orko Garai added 1 comment![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.h
Line 15, Patchset 14:
class DrmIoctlWrapper {Kramer Ge . unresolved
Can this be named to specify for syncobj ioctl? I do not foresee that we do other drm ioctl's here.
Attention is currently required from:
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Oct 16, 2024, 6:21:46 PM10/16/24
to Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge
Orko Garai added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Kramer Ge . resolved
Structurally drm syncobj related stuff is a gpu process responsibility, I think it's possible that we can keep drm node in the gpu process and transport the syncobj for the buffer from ozone/wayland/gpu to ozone/wayland/host, in a future possibility that shared_image maybe exporting timeline to syncobj.
But I understand it's an over-optimization so we don't want to do it now, at least we right now there's no syncobj equivalent of GpuFenceHandle.
Orko Garai
That sounds good. Yeah I agree in future syncobj support could be added to chromium's GPU fence (or equivalent), and we could do the wait using that in the GPU process.
Orko Garai (Gerrit)
Oct 18, 2024, 3:28:36 PM10/18/24
to Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge, Maksim Sisov and Nick Yamane
Orko Garai added 1 comment![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/drm_ioctl_wrapper.h
Line 15, Patchset 14:
class DrmIoctlWrapper {Kramer Ge . resolved
Orko GaraiCan this be named to specify for syncobj ioctl? I do not foresee that we do other drm ioctl's here.
Acknowledged
Attention is currently required from:
- Kramer Ge
- Maksim Sisov
- Nick Yamane
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Oct 21, 2024, 7:31:19 PM10/21/24
to Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge, Maksim Sisov and Nick Yamane
Orko Garai added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 18 (Latest):
Orko Garai . resolved
@fang...@chromium.org The +1 got removed due to the renaming. Please see if all is good now.
Related details
Attention is currently required from:
- Kramer Ge
- Maksim Sisov
- Nick Yamane
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Oct 22, 2024, 10:10:45 AM10/22/24
to Orko Garai, Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Maksim Sisov, Nick Yamane and Orko Garai
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Maksim Sisov
- Nick Yamane
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Oct 22, 2024, 4:40:21 PM10/22/24
to Nick Yamane, Thomas Anderson, Maksim Sisov, Chromium LUCI CQ, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Maksim Sisov and Nick Yamane
Orko Garai voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Maksim Sisov
- Nick Yamane
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Oct 22, 2024, 5:53:48 PM10/22/24
to Orko Garai, Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
linux_drm_syncobj_v1: Part 4: Add DrmIoctlWrapper
This is a change in preparation for the linux_drm_syncobj_v1
implementation.
Summary of changes:
- Add DrmIoctlWrapper class which wraps libDrm ioctl APIs.
- Set the drm wrapper in WaylandBufferManagerHost from
OzonePlatformWayland::InitializeUI().
- Add mock for testing.
Bug: 367623923
Change-Id: I47250871ea64bef9824372f4a6307fb458287c96
Commit-Queue: Orko Garai <or...@igalia.com>
Reviewed-by: Kramer Ge <fang...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1372353}
Files:
- M ui/ozone/platform/wayland/BUILD.gn
- A ui/ozone/platform/wayland/host/drm_syncobj_ioctl_wrapper.cc
- A ui/ozone/platform/wayland/host/drm_syncobj_ioctl_wrapper.h
- M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
- M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
- M ui/ozone/platform/wayland/ozone_platform_wayland.cc
- A ui/ozone/platform/wayland/test/mock_drm_syncobj_ioctl_wrapper.cc
- A ui/ozone/platform/wayland/test/mock_drm_syncobj_ioctl_wrapper.h
- A ui/ozone/platform/wayland/test/test_fd_factory.cc
- A ui/ozone/platform/wayland/test/test_fd_factory.h
Change size: L
Delta: 10 files changed, 314 insertions(+), 2 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Kramer Ge
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alberto Martínez Olivares (Gerrit)
Oct 23, 2024, 12:37:18 PM10/23/24
to Chromium LUCI CQ, Orko Garai, Nick Yamane, Thomas Anderson, Maksim Sisov, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Alberto Martínez Olivares added 1 comment![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brett Howell (Gerrit)
Dec 10, 2025, 5:49:43 AM (12 days ago) Dec 10
to Orko Garai, Chromium LUCI CQ, Alberto Martínez Olivares, Nick Yamane, Thomas Anderson, Maksim Sisov, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Brett Howell added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Brett Howell . resolved
Good job thanks for all the hard work
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brett Howell (Gerrit)
Dec 10, 2025, 5:50:37 AM (12 days ago) Dec 10
to Orko Garai, Chromium LUCI CQ, Alberto Martínez Olivares, Kramer Ge, Nick Yamane, Thomas Anderson, Maksim Sisov, Tricium, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Alberto Martínez Olivares, Brett Howell, Kramer Ge, Maksim Sisov, Nick Yamane, Orko Garai and Thomas Anderson
Brett Howell added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Brett Howell . resolved
Thanks again
Related details
Attention is currently required from:
- Alberto Martínez Olivares
- Brett Howell
- Kramer Ge
- Maksim Sisov
- Nick Yamane
- Orko Garai
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages