Commit 94b79589 authored by Kristian H. Kristensen's avatar Kristian H. Kristensen Committed by Commit Bot

Reland "exo: Set the gfx::NativePixmapHandle modifier"

The original CL didn't account for most clients passing modifier 0 to
mean "don't know modifier".  The 0 modifier is DRM_FORMAT_MOD_LINEAR,
while DRM_FORMAT_MOD_INVALID is to be used to indicate that the client
doesn't know the modifier and triggers the legacy implicit layout
discovery:

https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/master/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml#L133

Specifically, what broke was Intel GPUs using tiled buffers and
importing those as EGLImages without specifying a modifier.  Without a
modifier, the GLES driver will get the tiling information from the
kernel.  One we started forwarding the DRM_FORMAT_MOD_LINEAR modifier
coming from the clients, that would take precedence and the driver
would view the buffer as linear, even though it was tiled.

We'll have to fix the various clients, but in the interim, we can add
support for non-linear modifiers by treating 0 as
DRM_FORMAT_MOD_INVALID like before.  This does the right thing for
linear buffers for all GPUs and keeps the mismatch in the Intel case
working.  At the same time it allows modifier-aware clients to pass in
explicit modifiers so that we can enable more efficient buffer
layouts.

BUG=b:145579089, b:79682290, b:162704898, chromium:1112017
TEST=wayland_rects_client --use-drm=msm and verify modifiers get passed

Change-Id: Ib00d4d6fe96e957c1f66205f2f5f515c09f3aabd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340164
Commit-Queue: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: default avatarDaniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795280}
parent c71f4c7c
...@@ -51,6 +51,7 @@ struct LinuxBufferParams { ...@@ -51,6 +51,7 @@ struct LinuxBufferParams {
base::ScopedFD fd; base::ScopedFD fd;
uint32_t stride; uint32_t stride;
uint32_t offset; uint32_t offset;
uint64_t modifier;
}; };
explicit LinuxBufferParams(Display* display) : display(display) {} explicit LinuxBufferParams(Display* display) : display(display) {}
...@@ -74,7 +75,8 @@ void linux_buffer_params_add(wl_client* client, ...@@ -74,7 +75,8 @@ void linux_buffer_params_add(wl_client* client,
LinuxBufferParams* linux_buffer_params = LinuxBufferParams* linux_buffer_params =
GetUserDataAs<LinuxBufferParams>(resource); GetUserDataAs<LinuxBufferParams>(resource);
LinuxBufferParams::Plane plane{base::ScopedFD(fd), stride, offset}; const uint64_t modifier = (static_cast<uint64_t>(modifier_hi) << 32) | modifier_lo;
LinuxBufferParams::Plane plane{base::ScopedFD(fd), stride, offset, modifier};
const auto& inserted = linux_buffer_params->planes.insert( const auto& inserted = linux_buffer_params->planes.insert(
std::pair<uint32_t, LinuxBufferParams::Plane>(plane_idx, std::pair<uint32_t, LinuxBufferParams::Plane>(plane_idx,
...@@ -106,8 +108,16 @@ bool ValidateLinuxBufferParams(wl_resource* resource, ...@@ -106,8 +108,16 @@ bool ValidateLinuxBufferParams(wl_resource* resource,
LinuxBufferParams* linux_buffer_params = LinuxBufferParams* linux_buffer_params =
GetUserDataAs<LinuxBufferParams>(resource); GetUserDataAs<LinuxBufferParams>(resource);
size_t num_planes = gfx::NumberOfPlanesForLinearBufferFormat(format);
size_t num_planes = linux_buffer_params->planes.size();
if (num_planes == 0) {
wl_resource_post_error(resource,
ZWP_LINUX_BUFFER_PARAMS_V1_ERROR_INCOMPLETE,
"no planes given");
return false;
}
// Validate that we have planes 0..num_planes-1
for (uint32_t i = 0; i < num_planes; ++i) { for (uint32_t i = 0; i < num_planes; ++i) {
auto plane_it = linux_buffer_params->planes.find(i); auto plane_it = linux_buffer_params->planes.find(i);
if (plane_it == linux_buffer_params->planes.end()) { if (plane_it == linux_buffer_params->planes.end()) {
...@@ -118,10 +128,15 @@ bool ValidateLinuxBufferParams(wl_resource* resource, ...@@ -118,10 +128,15 @@ bool ValidateLinuxBufferParams(wl_resource* resource,
} }
} }
if (linux_buffer_params->planes.size() != num_planes) { // All planes must have the same modifier.
wl_resource_post_error(resource, ZWP_LINUX_BUFFER_PARAMS_V1_ERROR_PLANE_IDX, uint64_t modifier = linux_buffer_params->planes[0].modifier;
"plane idx out of bounds"); for (uint32_t i = 1; i < num_planes; ++i) {
return false; if (linux_buffer_params->planes[i].modifier != modifier) {
wl_resource_post_error(resource,
ZWP_LINUX_BUFFER_PARAMS_V1_ERROR_INVALID_FORMAT,
"all planes must have same modifier");
return false;
}
} }
return true; return true;
...@@ -153,14 +168,25 @@ wl_resource* create_buffer(wl_client* client, ...@@ -153,14 +168,25 @@ wl_resource* create_buffer(wl_client* client,
LinuxBufferParams* linux_buffer_params = LinuxBufferParams* linux_buffer_params =
GetUserDataAs<LinuxBufferParams>(resource); GetUserDataAs<LinuxBufferParams>(resource);
size_t num_planes =
gfx::NumberOfPlanesForLinearBufferFormat(supported_format->buffer_format);
gfx::NativePixmapHandle handle; gfx::NativePixmapHandle handle;
for (uint32_t i = 0; i < num_planes; ++i) { // A lot of clients (arc++, arcvm, sommelier etc) pass 0
auto plane_it = linux_buffer_params->planes.find(i); // (DRM_FORMAT_MOD_LINEAR) when they don't know the format modifier.
LinuxBufferParams::Plane& plane = plane_it->second; // They're supposed to pass DRM_FORMAT_MOD_INVALID, which triggers
// EGL import without an explicit modifier and lets the driver pick
// up the buffer layout from out-of-band channels like kernel ioctls.
//
// We can't fix all the clients in one go, but we can preserve the
// behaviour that 0 means implicit modifier, but only setting the
// handle modifier if we get a non-0 modifier.
//
// TODO(hoegsberg): Once we've fixed all relevant clients, we should
// remove this so as to catch future misuse.
if (linux_buffer_params->planes[0].modifier != 0)
handle.modifier = linux_buffer_params->planes[0].modifier;
for (uint32_t i = 0; i < linux_buffer_params->planes.size(); ++i) {
auto& plane = linux_buffer_params->planes[i];
handle.planes.emplace_back(plane.stride, plane.offset, 0, handle.planes.emplace_back(plane.stride, plane.offset, 0,
std::move(plane.fd)); std::move(plane.fd));
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment