Use SIZE_HINTS property for cursor buffer size instead of probing [chromium/src : main]
Jiahe Zhang (Gerrit)
Jiahe Zhang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang added 2 comments![]( "Open in Gerrit")
Hi Peter, the SIZE_HINTS kernel patches have been backported to several branches (5.4, 6.1, 6.6). So I update the probing part to use SIZE_HINTS instead. PTAL.
// TODO(jiahe.zhang): Remove this once libdrm is updated.The definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Related details
- Peter McNeeley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter McNeeley (Gerrit)
Peter McNeeley added 3 comments![]( "Open in Gerrit")
base::flat_map<std::string, std::vector<std::unique_ptr<DrmDumbBuffer>>>We should really avoid using string. We should map to our types.
plane->CanUseForCrtcId(crtc_controllers_[0]->crtc())) {This is for 0? This this going to matter? What about the other controllers
// TODO(jiahe.zhang): Remove this once libdrm is updated.The definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
@sean...@chromium.org Might know the status of things here.
Related details
- Jiahe Zhang
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jiahe Zhang
| Commit-Queue | +1 |
3 comments
Thanks for the review. Addressed the comments. PTAL.
base::flat_map<std::string, std::vector<std::unique_ptr<DrmDumbBuffer>>>We should really avoid using string. We should map to our types.
Done.
Note:
Directly using `base::flat_map<gfx::Size, ...>` here results in the following compile error, so `gfx::Rect` is used instead (or we will need to add `operator<` to `gfx::Size`.
```
../../base/containers/flat_map.h:283:31: error: no matching function for call to object of type 'key_compare' (aka 'std::less<void>')
283 | if (found == tree::end() || tree::key_comp()(key, found->first))
| ^~~~~~~~~~~~~~~~
../../ui/ozone/platform/drm/gpu/hardware_display_controller.cc:551:25: note: in instantiation of member function 'base::flat_map<gfx::Size, std::vector<std::unique_ptr<ui::DrmDumbBuffer>>>::operator[]' requested here
551 | cursor_buffer_map_[size].push_back(
```
plane->CanUseForCrtcId(crtc_controllers_[0]->crtc())) {This is for 0? This this going to matter? What about the other controllers
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter McNeeley (Gerrit)
Peter McNeeley added 3 comments![]( "Open in Gerrit")
base::flat_map<std::string, std::vector<std::unique_ptr<DrmDumbBuffer>>>Jiahe ZhangWe should really avoid using string. We should map to our types.
Done.
Note:
Directly using `base::flat_map<gfx::Size, ...>` here results in the following compile error, so `gfx::Rect` is used instead (or we will need to add `operator<` to `gfx::Size`.
```
../../base/containers/flat_map.h:283:31: error: no matching function for call to object of type 'key_compare' (aka 'std::less<void>')
283 | if (found == tree::end() || tree::key_comp()(key, found->first))
| ^~~~~~~~~~~~~~~~
../../ui/ozone/platform/drm/gpu/hardware_display_controller.cc:551:25: note: in instantiation of member function 'base::flat_map<gfx::Size, std::vector<std::unique_ptr<ui::DrmDumbBuffer>>>::operator[]' requested here
551 | cursor_buffer_map_[size].push_back(```
You create a custom comparator for this type at the site of the flat_map
gfx::Size(std::min(max_cursor_size_supported.width(), 256),You can name this magic number and resuse it elsewhere. This gives it semantic meaning.
std::vector<gfx::Size>* supported_cursor_sizes) {Nit:
Usually it is better to do this as a return value if you want to replace the list. Again this depends on if you are replacing vs adding.
Related details
- Jiahe Zhang
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter McNeeley (Gerrit)
Peter McNeeley added 1 comment![]( "Open in Gerrit")
// If there are multiple CRTCs they should all have the same supported
// cursor sizes.I think this is going to be an intel assumption
Jiahe Zhang (Gerrit)
Jiahe Zhang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jiahe Zhang
| Commit-Queue | +1 |
4 comments
base::flat_map<std::string, std::vector<std::unique_ptr<DrmDumbBuffer>>>Jiahe ZhangWe should really avoid using string. We should map to our types.
Peter McNeeleyDone.
Note:
Directly using `base::flat_map<gfx::Size, ...>` here results in the following compile error, so `gfx::Rect` is used instead (or we will need to add `operator<` to `gfx::Size`.
```
../../base/containers/flat_map.h:283:31: error: no matching function for call to object of type 'key_compare' (aka 'std::less<void>')
283 | if (found == tree::end() || tree::key_comp()(key, found->first))
| ^~~~~~~~~~~~~~~~
../../ui/ozone/platform/drm/gpu/hardware_display_controller.cc:551:25: note: in instantiation of member function 'base::flat_map<gfx::Size, std::vector<std::unique_ptr<ui::DrmDumbBuffer>>>::operator[]' requested here
551 | cursor_buffer_map_[size].push_back(```
You create a custom comparator for this type at the site of the flat_map
Done
// If there are multiple CRTCs they should all have the same supported
// cursor sizes.I think this is going to be an intel assumption
Yes, this is true on Intel while other platforms don't have SIZE_HINTS values in the kernel.
As this is already guarded by `*driver == "i915"`, should we keep the current code or, to be rigorous， only take the common elements from all CRTCs? WDYT?
gfx::Size(std::min(max_cursor_size_supported.width(), 256),You can name this magic number and resuse it elsewhere. This gives it semantic meaning.
Done
Nit:
Usually it is better to do this as a return value if you want to replace the list. Again this depends on if you are replacing vs adding.
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Paul (Gerrit)
Sean Paul added 1 comment![]( "Open in Gerrit")
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
@sean...@chromium.org Might know the status of things here.
I'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Related details
- Jiahe Zhang
- Peter McNeeley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel added 10 comments![]( "Open in Gerrit")
struct SizeCompare {
bool operator()(const gfx::Size& a, const gfx::Size& b) const {
if (a.GetArea() == b.GetArea()) {
if (a.width() == b.width()) {
return a.height() < b.height();
} else {
return a.width() < b.width();
}
} else {
return a.GetArea() < b.GetArea();
}
}
};You're doing a size comparison in the source file as well.
You should move this to `ui/ozone/platform/drm/common/drm_util.h` [[1]](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/common/drm_util.h) and call it something like `CursorSizeComparator`, and then use it both here and in `InitSupportedCursorSizes()`.
// |cursor_buffer_map_| stores active buffers for each
// |supported_cursor_sizes_|.nit: This should be right above `cursor_buffer_map_`
DCHECK(!crtc_controllers_.empty());Sorry if I'm missing something, but why is this `DCHECK` needed here? is `crtc_controllers_` used below somehow?
const auto& planes = GetDrmDevice()->plane_manager()->planes();If the types are simple enough, please use them over `auto`. Here, below, and anywhere else.
// If there are multiple CRTCs they should all have the same supported
// cursor sizes.Jiahe ZhangI think this is going to be an intel assumption
Yes, this is true on Intel while other platforms don't have SIZE_HINTS values in the kernel.
As this is already guarded by `*driver == "i915"`, should we keep the current code or, to be rigorous， only take the common elements from all CRTCs? WDYT?
I think there should be no assumptions at all and the cursor sizes inferred per CRTC. If this is already being done, then the comment is unnecessary.
for (const auto& plane : planes) {
// Currently on Intel, if there are multiple CRTCs they should all have
// the same supported cursor sizes.
if (plane->type() == DRM_PLANE_TYPE_CURSOR) {
const auto& supported_cursor_sizes = plane->supported_cursor_sizes();
supported_cursor_sizes_.assign(supported_cursor_sizes.begin(),
supported_cursor_sizes.end());
break;
}
}
}Please cover this case by a test. Test should be added to `ui/ozone/platform/drm/gpu/hardware_display_controller_unittest.cc`
gfx::Size max_cursor_size_supported = GetMaximumCursorSize(*GetDrmDevice());const
// Using a moderate size e.g. 256 for the cursor is enough in most cases.
constexpr int kMaxCursorBufferSize = 256;Please move this to `ui/ozone/platform/drm/common/drm_util.h` [[1]](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/common/drm_util.h)
[](const gfx::Size& a, const gfx::Size& b) {
return a.GetArea() < b.GetArea();See my comment on reusing your cursor comparator.
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
I'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
- Jiahe Zhang
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel added 2 comments![]( "Open in Gerrit")
gfx::Size buffer_size = supported_cursor_sizes_.back();Is this guaranteed to be populated? This feels risky to me. we should not make any assumptions here.
if (type_ == DRM_PLANE_TYPE_CURSOR && properties_.size_hints.id) {
ScopedDrmPropertyBlobPtr size_hints_blob(
drm->GetPropertyBlob(properties_.size_hints.value));
if (size_hints_blob) {
supported_cursor_sizes_ =
ParseSupportedCursorSizes(size_hints_blob.get());
}
}Needs test overage as well.
Peter McNeeley (Gerrit)
Peter McNeeley added 1 comment![]( "Open in Gerrit")
// If there are multiple CRTCs they should all have the same supported
// cursor sizes.Jiahe ZhangI think this is going to be an intel assumption
Yes, this is true on Intel while other platforms don't have SIZE_HINTS values in the kernel.
As this is already guarded by `*driver == "i915"`, should we keep the current code or, to be rigorous， only take the common elements from all CRTCs? WDYT?
I think it is fine. You might want to add that this is an intel assumption in the comments.
Related details
- Gil Dekel
- Jiahe Zhang
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang added 13 comments![]( "Open in Gerrit")
struct SizeCompare {
bool operator()(const gfx::Size& a, const gfx::Size& b) const {
if (a.GetArea() == b.GetArea()) {
if (a.width() == b.width()) {
return a.height() < b.height();
} else {
return a.width() < b.width();
}
} else {
return a.GetArea() < b.GetArea();
}
}
};You're doing a size comparison in the source file as well.
You should move this to `ui/ozone/platform/drm/common/drm_util.h` [[1]](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/common/drm_util.h) and call it something like `CursorSizeComparator`, and then use it both here and in `InitSupportedCursorSizes()`.
Done
// |cursor_buffer_map_| stores active buffers for each
// |supported_cursor_sizes_|.nit: This should be right above `cursor_buffer_map_`
Done
gfx::Size buffer_size = supported_cursor_sizes_.back();Is this guaranteed to be populated? This feels risky to me. we should not make any assumptions here.
`InitSupportedCursorSizes()` will make sure `supported_cursor_sizes_` has either 3 (the current value from SIZE_HINTS on Intel) elements, or 1 element (from the legacy `GetMaximumCursorSize()`) if reading SIZE_HINTS fails. We also have a DCHECK at the end of `InitSupportedCursorSizes()`.
Sorry if I'm missing something, but why is this `DCHECK` needed here? is `crtc_controllers_` used below somehow?
Done
`crtc_controllers_[0]` was used in a previous patchset and I forgot to remove this. We don't need it anymore now. Sorry for the confusing.
const auto& planes = GetDrmDevice()->plane_manager()->planes();If the types are simple enough, please use them over `auto`. Here, below, and anywhere else.
Done
// If there are multiple CRTCs they should all have the same supported
// cursor sizes.Jiahe ZhangI think this is going to be an intel assumption
Peter McNeeleyYes, this is true on Intel while other platforms don't have SIZE_HINTS values in the kernel.
As this is already guarded by `*driver == "i915"`, should we keep the current code or, to be rigorous， only take the common elements from all CRTCs? WDYT?
I think it is fine. You might want to add that this is an intel assumption in the comments.
Acknowledged
for (const auto& plane : planes) {
// Currently on Intel, if there are multiple CRTCs they should all have
// the same supported cursor sizes.
if (plane->type() == DRM_PLANE_TYPE_CURSOR) {
const auto& supported_cursor_sizes = plane->supported_cursor_sizes();
supported_cursor_sizes_.assign(supported_cursor_sizes.begin(),
supported_cursor_sizes.end());
break;
}
}
}Please cover this case by a test. Test should be added to `ui/ozone/platform/drm/gpu/hardware_display_controller_unittest.cc`
Done
gfx::Size max_cursor_size_supported = GetMaximumCursorSize(*GetDrmDevice());Jiahe Zhangconst
Done
// Using a moderate size e.g. 256 for the cursor is enough in most cases.
constexpr int kMaxCursorBufferSize = 256;Please move this to `ui/ozone/platform/drm/common/drm_util.h` [[1]](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/common/drm_util.h)
Done
[](const gfx::Size& a, const gfx::Size& b) {
return a.GetArea() < b.GetArea();See my comment on reusing your cursor comparator.
Done
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
+1
Let's prioritize updating libdrm first.
Ping @i...@chromium.org .
if (type_ == DRM_PLANE_TYPE_CURSOR && properties_.size_hints.id) {
ScopedDrmPropertyBlobPtr size_hints_blob(
drm->GetPropertyBlob(properties_.size_hints.value));
if (size_hints_blob) {
supported_cursor_sizes_ =
ParseSupportedCursorSizes(size_hints_blob.get());
}
}Needs test overage as well.
Done
This should be covered by `HardwareDisplayControllerTest.DynamicCursorSize` now.
Related details
- Gil Dekel
- Ilja Friedel
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang added 1 comment![]( "Open in Gerrit")
Ping @gild...@chromium.org / @peterm...@chromium.org, any further comments on this patch?
And ping @i...@chromium.org, please help on the libdrm updating (see the comments in hardware_display_plane.cc).
Thanks a lot!
Gil Dekel (Gerrit)
Gil Dekel added 6 comments![]( "Open in Gerrit")
I'll try to get ihf@'s attention on this
{kSizeHintsPropId, "SIZE_HINTS"},nit: Is this a required property name...? IIUC this is currently an i915 only property, and more generally, [a property](https://drmdb.emersion.fr/properties/4008636142/SIZE_HINTS) that is attached to `planes that support a very limited set of sizes.`
std::vector<drm_format_modifier> supported_format_modifiers,
std::vector<gfx::Size> plane_size_hints) {I don't think these should be a part of the signature of this API, especially since `plane_size_hints` is currently an intel-only property. They make it increasingly more difficult to write tests if the caller always have to pass default or empty parameters here when they're not needed ([as seen here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/gpu/fake_drm_device.h;l=286-295;drc=c1b9831c8c232ab9470645977a18527cfa7bb993;bpv=0;bpt=1)).
I think we need a different mechanism of setting these props to planes for testing.
CursorSizeComparator>Thinking about this further - is there a reason for this map to be sorted...? Especially since `cursor_buffer_map_` is sorted when it's populated in `InitSupportedCursorSizes()`? It doesn't seem like you're trying to iterate over the keys.
If that's the case, we should either leave the vector sorted and not worry about the map, or sort the map and not have the vector `supported_cursor_sizes_`.
return current_cursor_;Is returning it as a raw ptr safe in terms of resource life-time management..?
gfx::Size buffer_size = supported_cursor_sizes_.back();Jiahe ZhangIs this guaranteed to be populated? This feels risky to me. we should not make any assumptions here.
`InitSupportedCursorSizes()` will make sure `supported_cursor_sizes_` has either 3 (the current value from SIZE_HINTS on Intel) elements, or 1 element (from the legacy `GetMaximumCursorSize()`) if reading SIZE_HINTS fails. We also have a DCHECK at the end of `InitSupportedCursorSizes()`.
- Ilja Friedel
- Jiahe Zhang
- Peter McNeeley
- Sean Paul
Gil Dekel (Gerrit)
Gil Dekel added 1 comment![]( "Open in Gerrit")
CursorSizeComparator>Thinking about this further - is there a reason for this map to be sorted...? Especially since `cursor_buffer_map_` is sorted when it's populated in `InitSupportedCursorSizes()`? It doesn't seem like you're trying to iterate over the keys.
If that's the case, we should either leave the vector sorted and not worry about the map, or sort the map and not have the vector `supported_cursor_sizes_`.
Correction:
Especially since `supported_cursor_sizes_` is sorted when it's populated in InitSupportedCursorSizes()?
Gil Dekel (Gerrit)
Gil Dekel added 1 comment![]( "Open in Gerrit")
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Ping @i...@chromium.org .
libdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
Jiahe Zhang (Gerrit)
Jiahe Zhang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jiahe Zhang
| Commit-Queue | +1 |
6 comments
nit: Is this a required property name...? IIUC this is currently an i915 only property, and more generally, [a property](https://drmdb.emersion.fr/properties/4008636142/SIZE_HINTS) that is attached to `planes that support a very limited set of sizes.`
Done. Moved this to the `kPlaneOptionalPropertyNames`.
std::vector<drm_format_modifier> supported_format_modifiers,
std::vector<gfx::Size> plane_size_hints) {I don't think these should be a part of the signature of this API, especially since `plane_size_hints` is currently an intel-only property. They make it increasingly more difficult to write tests if the caller always have to pass default or empty parameters here when they're not needed ([as seen here](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/gpu/fake_drm_device.h;l=286-295;drc=c1b9831c8c232ab9470645977a18527cfa7bb993;bpv=0;bpt=1)).
I think we need a different mechanism of setting these props to planes for testing.
Done
CursorSizeComparator>Gil DekelThinking about this further - is there a reason for this map to be sorted...? Especially since `cursor_buffer_map_` is sorted when it's populated in `InitSupportedCursorSizes()`? It doesn't seem like you're trying to iterate over the keys.
If that's the case, we should either leave the vector sorted and not worry about the map, or sort the map and not have the vector `supported_cursor_sizes_`.
Correction:
Especially since `supported_cursor_sizes_` is sorted when it's populated in InitSupportedCursorSizes()?
The only reason we need a comparator here is either `base::flat_map` or `std::map` requires it's type of key (aka `gfx::Size` here) can be compared, otherwise it cannot compile. So we either need a custom comparator like the current CL or add `operator<` to gfx::Size. Here's a previous discussion on this: https://chromium-review.googlesource.com/c/chromium/src/+/5587916/comment/88eb1626_93c88416/
Is returning it as a raw ptr safe in terms of resource life-time management..?
Done
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Gil DekelPing @i...@chromium.org .
libdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
- Gil Dekel
- Peter McNeeley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel added 4 comments![]( "Open in Gerrit")
size_t num_supported_cursor_sizes_for_testing() const {
return supported_cursor_sizes_.size();
}
gfx::Size current_cursor_size_for_testing() const {
return current_cursor_ ? current_cursor_->GetSize() : gfx::Size();
}nit: these are no longer inline (one line). Please move implementation to the .cc file.
CursorSizeComparator>Gil DekelThinking about this further - is there a reason for this map to be sorted...? Especially since `cursor_buffer_map_` is sorted when it's populated in `InitSupportedCursorSizes()`? It doesn't seem like you're trying to iterate over the keys.
If that's the case, we should either leave the vector sorted and not worry about the map, or sort the map and not have the vector `supported_cursor_sizes_`.
Jiahe ZhangCorrection:
Especially since `supported_cursor_sizes_` is sorted when it's populated in InitSupportedCursorSizes()?
The only reason we need a comparator here is either `base::flat_map` or `std::map` requires it's type of key (aka `gfx::Size` here) can be compared, otherwise it cannot compile. So we either need a custom comparator like the current CL or add `operator<` to gfx::Size. Here's a previous discussion on this: https://chromium-review.googlesource.com/c/chromium/src/+/5587916/comment/88eb1626_93c88416/
Acknowledged
std::optional<std::string> driver = GetDrmDevice()->GetDriverName();Do we still need this guard now that sizes are provided by a connector prop? Can't we instead assume whatever cursor sizes the planes report is valid? If not, can we make sure the planes are initialized with default cursor sizes? (either max size or `kMaxCursorBufferSize`)?
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Gil DekelPing @i...@chromium.org .
Jiahe Zhanglibdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
Thanks a lot! I will update this CL once that gets merged.
Heads up: Once libdrm is upreved, you'll have to add this to `ui/ozone/platform/drm/common/scoped_drm_types.h` as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
Related details
- Jiahe Zhang
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel added 1 comment![]( "Open in Gerrit")
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Gil DekelPing @i...@chromium.org .
Jiahe Zhanglibdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
Gil DekelThanks a lot! I will update this CL once that gets merged.
Heads up: Once libdrm is upreved, you'll have to add this to `ui/ozone/platform/drm/common/scoped_drm_types.h` as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
Also it seems that the libdrm 2.4.122 is now live on CrOS R129-15985.0.0
Jiahe Zhang (Gerrit)
Jiahe Zhang added 3 comments![]( "Open in Gerrit")
size_t num_supported_cursor_sizes_for_testing() const {
return supported_cursor_sizes_.size();
}
gfx::Size current_cursor_size_for_testing() const {
return current_cursor_ ? current_cursor_->GetSize() : gfx::Size();
}nit: these are no longer inline (one line). Please move implementation to the .cc file.
Done
std::optional<std::string> driver = GetDrmDevice()->GetDriverName();Do we still need this guard now that sizes are provided by a connector prop? Can't we instead assume whatever cursor sizes the planes report is valid? If not, can we make sure the planes are initialized with default cursor sizes? (either max size or `kMaxCursorBufferSize`)?
The only concern is that we have an Intel specific assumption below that all CRTCs have the same supported cursor sizes. As currently only i915 implements the SIZE_HINTS, I'm not sure if other drivers will follow the same assumption in the future.
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Gil DekelPing @i...@chromium.org .
Jiahe Zhanglibdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
Gil DekelThanks a lot! I will update this CL once that gets merged.
Gil DekelHeads up: Once libdrm is upreved, you'll have to add this to `ui/ozone/platform/drm/common/scoped_drm_types.h` as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
Also it seems that the libdrm 2.4.122 is now live on CrOS R129-15985.0.0
Heads up: Once libdrm is upreved, you'll have to add this to ui/ozone/platform/drm/common/scoped_drm_types.h as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
We don't actually instantiate a `drm_plane_size_hint` here, but just cast something from `blob->data`, whose lifecycle should be managed properly by `ScopedDrmPropertyBlobPtr` (like the `ParseSupportedFormatsAndModifiers` above). So it seems we don't need a deleter here, WDYT?
Related details
- Gil Dekel
- Peter McNeeley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel added 2 comments![]( "Open in Gerrit")
std::optional<std::string> driver = GetDrmDevice()->GetDriverName();Jiahe ZhangDo we still need this guard now that sizes are provided by a connector prop? Can't we instead assume whatever cursor sizes the planes report is valid? If not, can we make sure the planes are initialized with default cursor sizes? (either max size or `kMaxCursorBufferSize`)?
The only concern is that we have an Intel specific assumption below that all CRTCs have the same supported cursor sizes. As currently only i915 implements the SIZE_HINTS, I'm not sure if other drivers will follow the same assumption in the future.
Ok. Perhaps not for this CL, but can we modify the code so that it doesn't make this assumption and that each CRTC is checked against the cursor sizes in a follow-up CL?
I would rather keep Ozone/DRM agnostic to drivers when possible, and since those are properties available via DRM resources, I believe it can be done.
// TODO(jiahe.zhang): Remove this once libdrm is updated.Peter McNeeleyThe definition of `drm_plane_size_hint` is already in the upstream libdrm, but I don't know how to update libdrm in Chromium/ChromiumOS. Any hints?
Sean Paul@sean...@chromium.org Might know the status of things here.
Gil DekelI'm not a big fan of this. I think we should update libdrm instead. @i...@chromium.org, do you have a pointer to how we would do this?
Jiahe Zhang+1
Let's prioritize updating libdrm first.
Gil DekelPing @i...@chromium.org .
Jiahe Zhanglibdrm is upreved to 2.4.122 in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5760406. Assuming no major complications, this should take about a week. Hopefully less.
Thanks Ilja
Gil DekelThanks a lot! I will update this CL once that gets merged.
Gil DekelHeads up: Once libdrm is upreved, you'll have to add this to `ui/ozone/platform/drm/common/scoped_drm_types.h` as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
Jiahe ZhangAlso it seems that the libdrm 2.4.122 is now live on CrOS R129-15985.0.0
Heads up: Once libdrm is upreved, you'll have to add this to ui/ozone/platform/drm/common/scoped_drm_types.h as a scoped type with a proper deleter and instantiate it in a similar way to how other libdrm types are instantiated in Chrome.
We don't actually instantiate a `drm_plane_size_hint` here, but just cast something from `blob->data`, whose lifecycle should be managed properly by `ScopedDrmPropertyBlobPtr` (like the `ParseSupportedFormatsAndModifiers` above). So it seems we don't need a deleter here, WDYT?
Ok, if that's the case then it's probably fine to skip. I'll take a look at the code once you update it to use libdrm instead.
Related details
- Jiahe Zhang
- Peter McNeeley
- Sean Paul
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang added 2 comments![]( "Open in Gerrit")
Code has already been updated using the libdrm. PTAL.
std::optional<std::string> driver = GetDrmDevice()->GetDriverName();Jiahe ZhangDo we still need this guard now that sizes are provided by a connector prop? Can't we instead assume whatever cursor sizes the planes report is valid? If not, can we make sure the planes are initialized with default cursor sizes? (either max size or `kMaxCursorBufferSize`)?
Gil DekelThe only concern is that we have an Intel specific assumption below that all CRTCs have the same supported cursor sizes. As currently only i915 implements the SIZE_HINTS, I'm not sure if other drivers will follow the same assumption in the future.
Ok. Perhaps not for this CL, but can we modify the code so that it doesn't make this assumption and that each CRTC is checked against the cursor sizes in a follow-up CL?
I would rather keep Ozone/DRM agnostic to drivers when possible, and since those are properties available via DRM resources, I believe it can be done.
- Gil Dekel
- Peter McNeeley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gil Dekel (Gerrit)
Gil Dekel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gil Dekel
| Code-Review | +1 |
1 comment
- Jiahe Zhang
- Peter McNeeley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter McNeeley (Gerrit)
Peter McNeeley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter McNeeley
| Code-Review | +1 |
1 comment
(tagging along)
Related details
- Jiahe Zhang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiahe Zhang (Gerrit)
Jiahe Zhang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jiahe Zhang
| Commit-Queue | +2 |
1 comment
Thanks!
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Use SIZE_HINTS property for cursor buffer size instead of probing
The previous probing of supported cursor sizes causes issues on some
devices. This CL replaces that with SIZE_HINTS property of cursor
plane and uses the max size if SIZE_HINTS is not available. This
feature is currently behind a flag UseDynamicCursorSize.
- M ui/ozone/platform/drm/common/drm_util.h
- M ui/ozone/platform/drm/gpu/crtc_controller.cc
- M ui/ozone/platform/drm/gpu/crtc_controller.h
- M ui/ozone/platform/drm/gpu/fake_drm_device.cc
- M ui/ozone/platform/drm/gpu/fake_drm_device.h
- M ui/ozone/platform/drm/gpu/hardware_display_controller.cc
- M ui/ozone/platform/drm/gpu/hardware_display_controller.h
- M ui/ozone/platform/drm/gpu/hardware_display_controller_unittest.cc
- M ui/ozone/platform/drm/gpu/hardware_display_plane.cc
- M ui/ozone/platform/drm/gpu/hardware_display_plane.h
Code-Review: +1 by Peter McNeeley, +1 by Gil Dekel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |