modesetting crtc that doesn't support modifiers may reuse buffer that has modifiers |
||
Issue description
I noticed this when investigating a test failure that occurred after some refactoring (of RejectBufferWithIncompatibleModifiers).
This test does not work properly. It's trying to verify that non-matching modifiers will cause screen manager to reject the current buffer for modesetting, but currently we do accept the buffer. The only reason the test passes is because the EXPECT_NE() checks that the framebuffer id doesn't match the current framebuffer, when it should check the opaque framebuffer id.
This may cause issues when modesetting a buffer that has modifiers onto a crtc that doesn't support them - this case can be erroneously accepted.
Do we need to do something like this ?
diff --git a/ui/ozone/platform/drm/gpu/screen_manager.cc b/ui/ozone/platform/drm/gpu/screen_manager.cc
index 3cd0ad0e6304..9483ee64a7c4 100644
--- a/ui/ozone/platform/drm/gpu/screen_manager.cc
+++ b/ui/ozone/platform/drm/gpu/screen_manager.cc
@@ -377,7 +377,8 @@ DrmOverlayPlane ScreenManager::GetModesetBuffer(
// modifier either and we can reuse the buffer. Otherwise, check
// to see if the controller supports the buffers format
// modifier.
- if (modifiers.empty())
+ if (modifiers.empty() &&
+ primary->buffer->format_modifier() == DRM_FORMAT_MOD_NONE)
return primary->Clone();
for (const uint64_t modifier : modifiers) {
if (modifier == primary->buffer->format_modifier())
Here's a patch that fixes the test:
commit 9ccc75d9e9ea2c4cd67c3f9bd751420b78b53fd5
Author: Michael Spang <spang@chromium.org>
Date: Thu Jul 26 12:39:46 2018 -0400
diff --git a/ui/ozone/platform/drm/gpu/screen_manager_unittest.cc b/ui/ozone/platform/drm/gpu/screen_manager_
index ebb9a52dd1d4..a20b5f048961 100644
--- a/ui/ozone/platform/drm/gpu/screen_manager_unittest.cc
+++ b/ui/ozone/platform/drm/gpu/screen_manager_unittest.cc
@@ -524,7 +524,7 @@ TEST_F(ScreenManagerTest, EnableControllerWhenWindowHasBuffer) {
window->Shutdown();
}
-TEST_F(ScreenManagerTest, RejectBufferWithIncompatibleModifiers) {
+TEST_F(ScreenManagerTest, DISABLED_RejectBufferWithIncompatibleModifiers) {
std::unique_ptr<ui::DrmWindow> window(
new ui::DrmWindow(1, device_manager_.get(), screen_manager_.get()));
window->Initialize(buffer_generator_.get());
@@ -550,6 +550,7 @@ TEST_F(ScreenManagerTest, RejectBufferWithIncompatibleModifiers) {
// I915_FORMAT_MOD_X_TILED modifier we created above and the two
// framebuffer IDs should be different.
EXPECT_NE(buffer->GetFramebufferId(), drm_->current_framebuffer());
+ EXPECT_NE(buffer->GetOpaqueFramebufferId(), drm_->current_framebuffer());
window = screen_manager_->RemoveWindow(1);
window->Shutdown();
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/321935a26f98665327315641fded25d48cd30ae1 commit 321935a26f98665327315641fded25d48cd30ae1 Author: Michael Spang <spang@chromium.org> Date: Tue Jul 31 21:27:29 2018 ozone: drm: Fix RejectBufferWithIncompatibleModifiers and disable it This test is broken - it's trying to verify that non-matching modifiers will cause screen manager to reject the current buffer for modesetting, but currently we do accept the buffer. The only reason the test passes is because the EXPECT_NE() checks that the framebuffer id doesn't match the current framebuffer, when it should check the opaque framebuffer id. Bug: 868010 Change-Id: I1f2312e6bc698f68f3806afd79ac07c19c3027cb Reviewed-on: https://chromium-review.googlesource.com/1155914 Commit-Queue: Michael Spang <spang@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> Cr-Commit-Position: refs/heads/master@{#579573} [modify] https://crrev.com/321935a26f98665327315641fded25d48cd30ae1/ui/ozone/platform/drm/gpu/screen_manager_unittest.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e5f38f60551dca0fd978bd26469f3d2905d7132c commit e5f38f60551dca0fd978bd26469f3d2905d7132c Author: Michael Spang <spang@chromium.org> Date: Tue Jul 31 22:12:10 2018 ozone: drm: Move cursor buffer management to HardwareDisplayController Since HardwareDisplayControllers are never moved between DRM devices, it's much simpler to manage the cursor buffers there - we never need to reallocate them due to a device change. This will also allow us to remove the refcounting. Bug: 868010 Change-Id: Ie7e3615b4c653ea6250a4a4a45510c84badabe9c Reviewed-on: https://chromium-review.googlesource.com/1155915 Commit-Queue: Michael Spang <spang@chromium.org> Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> Cr-Commit-Position: refs/heads/master@{#579593} [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/crtc_controller.cc [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/crtc_controller.h [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/drm_window.cc [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/drm_window.h [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/drm_window_unittest.cc [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/hardware_display_controller.cc [modify] https://crrev.com/e5f38f60551dca0fd978bd26469f3d2905d7132c/ui/ozone/platform/drm/gpu/hardware_display_controller.h
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de0cbe8b267bdf4daa57617971b8e14bc430f5d9 commit de0cbe8b267bdf4daa57617971b8e14bc430f5d9 Author: Michael Spang <spang@chromium.org> Date: Tue Jul 31 22:47:46 2018 ozone: drm: Concretize DrmFramebuffer and compose it into {Gbm,Drm}Buffer Change DrmFramebuffer from a virtual interface to a concrete wrapper for a DRM framebuffer. This is what used to be ScanoutBuffer and is all that is needed for modesetting. In particular, we don't need access to gbm_bo, skia surfaces backed by dumb buffer mmaps, or other bits from {Gbm,Drm}Buffer in the modesetting internals. DrmFramebuffer is composed into GbmBuffer and DrmBuffer so that we can still access framebuffers for those types when presenting a buffer. GbmBuffer and DrmBuffer are no longer refcounted after this change. Bug: 868010 Change-Id: I6d31b000b35bc4a6caa6280bee4156ca9b271bb9 Reviewed-on: https://chromium-review.googlesource.com/1155916 Commit-Queue: Michael Spang <spang@chromium.org> Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> Cr-Commit-Position: refs/heads/master@{#579605} [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/BUILD.gn [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/crtc_controller.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_buffer.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_buffer.h [add] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_framebuffer.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_framebuffer.h [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_overlay_plane.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_overlay_validator.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_thread.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_thread.h [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_thread_proxy.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_thread_proxy.h [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/drm_window.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/gbm_buffer.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/gbm_buffer.h [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/gbm_overlay_surface.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/gbm_surface_factory.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/gbm_surfaceless.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_controller.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_controller.h [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_plane_manager.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_plane_manager_legacy.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/hardware_display_plane_manager_unittest.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/mock_drm_framebuffer_generator.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/mock_dumb_buffer_generator.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/screen_manager.cc [modify] https://crrev.com/de0cbe8b267bdf4daa57617971b8e14bc430f5d9/ui/ozone/platform/drm/gpu/screen_manager_unittest.cc
,
Jul 31
oops, the last 2 CLs were for bug 869206
,
Jan 11
This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this. |
||
►
Sign in to add a comment |
||
Comment 1 by dcasta...@chromium.org
, Jul 26