[Fuchsia] Stop setting image constraints in OutputPresenterFuchsia

OutputPresenterFuchsia was setting image constraints for allocated
buffers. As result the images were always allocated with
pixel_format=R8G8B8A8 without any flags set in format_modifiers. As
result, device-specific optimizations were not enabled for these
buffers, so GPU was using more memory bandwidth than would be possible
otherwise. This change updates OutputPresenterFuchsia to avoid setting
any constraints for the buffer collection. Instead buffer collection
constraints are set only through Vulkan. This ensures that the buffers
are allocated with all possible optimizations supported by the Vulkan
driver.

Bug: 1128740
Change-Id: I7ec8f605ace86a33062d5296a1b6b2e1f647b3c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414757Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Auto-Submit: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808101}

components/viz/service/display_embedder/output_presenter_fuchsia.cc

@@ -226,7 +226,7 @@ OutputPresenterFuchsia::AllocateImages(gfx::ColorSpace color_space,
 
   // Create buffer collection with 2 extra tokens: one for Vulkan and one for
   // the ImagePipe.
-  fuchsia::sysmem::BufferCollectionTokenPtr collection_token;
+  fuchsia::sysmem::BufferCollectionTokenSyncPtr collection_token;
   sysmem_allocator_->AllocateSharedCollection(collection_token.NewRequest());
 
   fidl::InterfaceHandle<fuchsia::sysmem::BufferCollectionToken>
@@ -234,16 +234,7 @@ OutputPresenterFuchsia::AllocateImages(gfx::ColorSpace color_space,
   collection_token->Duplicate(ZX_RIGHT_SAME_RIGHTS,
                               token_for_scenic.NewRequest());
 
-  fidl::InterfaceHandle<fuchsia::sysmem::BufferCollectionToken>
-      token_for_vulkan;
-  collection_token->Duplicate(ZX_RIGHT_SAME_RIGHTS,
-                              token_for_vulkan.NewRequest());
-
-  fuchsia::sysmem::BufferCollectionSyncPtr collection;
-  sysmem_allocator_->BindSharedCollection(std::move(collection_token),
-                                          collection.NewRequest());
-
-  zx_status_t status = collection->Sync();
+  zx_status_t status = collection_token->Sync();
   if (status != ZX_OK) {
     ZX_DLOG(ERROR, status) << "fuchsia.sysmem.BufferCollection.Sync()";
     return {};
@@ -252,21 +243,6 @@ OutputPresenterFuchsia::AllocateImages(gfx::ColorSpace color_space,
   auto* vulkan =
       dependency_->GetVulkanContextProvider()->GetVulkanImplementation();
 
-  // Set constraints for the new collection.
-  fuchsia::sysmem::BufferCollectionConstraints constraints;
-  constraints.min_buffer_count = num_images;
-  constraints.usage.none = fuchsia::sysmem::noneUsage;
-  constraints.image_format_constraints_count = 1;
-  constraints.image_format_constraints[0].pixel_format.type =
-      fuchsia::sysmem::PixelFormatType::R8G8B8A8;
-  constraints.image_format_constraints[0].min_coded_width = frame_size_.width();
-  constraints.image_format_constraints[0].min_coded_height =
-      frame_size_.height();
-  constraints.image_format_constraints[0].color_spaces_count = 1;
-  constraints.image_format_constraints[0].color_space[0].type =
-      fuchsia::sysmem::ColorSpaceType::SRGB;
-  collection->SetConstraints(true, constraints);
-
   // Register the new buffer collection with the ImagePipe.
   last_buffer_collection_id_++;
   image_pipe_->AddBufferCollection(last_buffer_collection_id_,
@@ -280,31 +256,14 @@ OutputPresenterFuchsia::AllocateImages(gfx::ColorSpace color_space,
                            ->GetDeviceQueue()
                            ->GetVulkanDevice();
   buffer_collection_ = vulkan->RegisterSysmemBufferCollection(
-      vk_device, buffer_collection_id, token_for_vulkan.TakeChannel(),
-      buffer_format_, gfx::BufferUsage::SCANOUT);
+      vk_device, buffer_collection_id, collection_token.Unbind().TakeChannel(),
+      buffer_format_, gfx::BufferUsage::SCANOUT, frame_size_, num_images);
 
-  // Wait for the images to be allocated.
-  zx_status_t wait_status;
-  fuchsia::sysmem::BufferCollectionInfo_2 buffers_info;
-  status = collection->WaitForBuffersAllocated(&wait_status, &buffers_info);
-  if (status != ZX_OK) {
-    ZX_DLOG(ERROR, status) << "fuchsia.sysmem.BufferCollection failed";
+  if (!buffer_collection_) {
+    ZX_DLOG(ERROR, status) << "Failed to allocate sysmem buffer collection";
     return {};
   }
 
-  if (wait_status != ZX_OK) {
-    ZX_DLOG(ERROR, wait_status)
-        << "Sysmem buffer collection allocation failed.";
-    return {};
-  }
-
-  DCHECK_GE(buffers_info.buffer_count, num_images);
-
-  // We no longer need the BufferCollection connection. Close it to ensure
-  // ImagePipe can still use the collection after BufferCollection connection
-  // is dropped below.
-  collection->Close();
-
   // Create PresenterImageFuchsia for each buffer in the collection.
   uint32_t image_usage =
       gpu::SHARED_IMAGE_USAGE_RASTER | gpu::SHARED_IMAGE_USAGE_SCANOUT;

gpu/command_buffer/service/shared_image_factory.cc

@@ -362,9 +362,10 @@ bool SharedImageFactory::RegisterSysmemBufferCollection(
   VkDevice device =
       vulkan_context_provider_->GetDeviceQueue()->GetVulkanDevice();
   DCHECK(device != VK_NULL_HANDLE);
-  it->second = vulkan_context_provider_->GetVulkanImplementation()
-                   ->RegisterSysmemBufferCollection(
-                       device, id, std::move(token), format, usage);
+  it->second =
+      vulkan_context_provider_->GetVulkanImplementation()
+          ->RegisterSysmemBufferCollection(device, id, std::move(token), format,
+                                           usage, gfx::Size(), 0);
 
   return true;
 }

gpu/vulkan/vulkan_implementation.h

@@ -136,7 +136,9 @@ class COMPONENT_EXPORT(VULKAN) VulkanImplementation {
                                  gfx::SysmemBufferCollectionId id,
                                  zx::channel token,
                                  gfx::BufferFormat format,
-                                 gfx::BufferUsage usage) = 0;
+                                 gfx::BufferUsage usage,
+                                 gfx::Size size,
+                                 size_t min_buffer_count) = 0;
 #endif  // defined(OS_FUCHSIA)
 
   bool use_swiftshader() const { return use_swiftshader_; }

ui/ozone/platform/scenic/sysmem_buffer_collection.cc

@@ -129,11 +129,13 @@ SysmemBufferCollection::SysmemBufferCollection(gfx::SysmemBufferCollectionId id)
 
 bool SysmemBufferCollection::Initialize(
     fuchsia::sysmem::Allocator_Sync* allocator,
+    zx::channel token_handle,
     gfx::Size size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
     VkDevice vk_device,
-    size_t num_buffers) {
+    size_t min_buffer_count,
+    bool force_protected) {
   DCHECK(IsNativePixmapConfigSupported(format, usage));
   DCHECK(!collection_);
   DCHECK(!vk_buffer_collection_);
@@ -143,51 +145,41 @@ bool SysmemBufferCollection::Initialize(
   if (vk_device == VK_NULL_HANDLE)
     return false;
 
-  min_size_ = size;
+  if (size.IsEmpty()) {
+    // Buffer collection that doesn't have explicit size is expected to be
+    // shared with other participants, who will determine the actual image size.
+    DCHECK(token_handle);
+
+    // Set nominal size of 1x1, which will be used only for
+    // vkSetBufferCollectionConstraintsFUCHSIA(). The actual size of the
+    // allocated buffers is determined by constraints set by other sysmem
+    // clients for the same collection. Size of the Vulkan image is determined
+    // by the values passed to CreateVkImage().
+    min_size_ = gfx::Size(1, 1);
+  } else {
+    min_size_ = size;
+  }
+
   format_ = format;
   usage_ = usage;
   vk_device_ = vk_device;
+  is_protected_ = force_protected;
 
   fuchsia::sysmem::BufferCollectionTokenSyncPtr collection_token;
-  zx_status_t status =
-      allocator->AllocateSharedCollection(collection_token.NewRequest());
-  if (status != ZX_OK) {
-    ZX_DLOG(ERROR, status)
-        << "fuchsia.sysmem.Allocator.AllocateSharedCollection()";
-    return false;
+  if (token_handle) {
+    collection_token.Bind(std::move(token_handle));
+  } else {
+    zx_status_t status =
+        allocator->AllocateSharedCollection(collection_token.NewRequest());
+    if (status != ZX_OK) {
+      ZX_DLOG(ERROR, status)
+          << "fuchsia.sysmem.Allocator.AllocateSharedCollection()";
+      return false;
+    }
   }
 
   return InitializeInternal(allocator, std::move(collection_token),
-                            num_buffers);
-}
-
-bool SysmemBufferCollection::Initialize(
-    fuchsia::sysmem::Allocator_Sync* allocator,
-    VkDevice vk_device,
-    zx::channel token_handle,
-    gfx::BufferFormat format,
-    gfx::BufferUsage usage,
-    bool force_protected) {
-  DCHECK(!collection_);
-  DCHECK(!vk_buffer_collection_);
-
-  // Set nominal size of 1x1, which will be used only for
-  // vkSetBufferCollectionConstraintsFUCHSIA(). The actual size of the allocated
-  // buffers is determined by constraints set by other sysmem clients for the
-  // same collection. Size of the Vulkan image is determined by the valus passed
-  // to CreateVkImage().
-  min_size_ = gfx::Size(1, 1);
-
-  vk_device_ = vk_device;
-  format_ = format;
-  usage_ = usage;
-  is_protected_ = force_protected;
-
-  fuchsia::sysmem::BufferCollectionTokenSyncPtr token;
-  token.Bind(std::move(token_handle));
-
-  return InitializeInternal(allocator, std::move(token),
-                            /*buffers_for_camping=*/0);
+                            min_buffer_count);
 }
 
 scoped_refptr<gfx::NativePixmap> SysmemBufferCollection::CreateNativePixmap(

ui/ozone/platform/scenic/sysmem_buffer_collection.h

@@ -39,18 +39,18 @@ class SysmemBufferCollection
   SysmemBufferCollection();
   explicit SysmemBufferCollection(gfx::SysmemBufferCollectionId id);
 
+  // Initializes the buffer collection and registers it with Vulkan using the
+  // specified |vk_device|. If |token_handle| is null then a new collection
+  // collection is created. |size| may be empty. In that case |token_handle|
+  // must not be null and the image size is determined by the other sysmem
+  // participants.
   bool Initialize(fuchsia::sysmem::Allocator_Sync* allocator,
+                  zx::channel token_handle,
                   gfx::Size size,
                   gfx::BufferFormat format,
                   gfx::BufferUsage usage,
                   VkDevice vk_device,
-                  size_t num_buffers);
-
-  bool Initialize(fuchsia::sysmem::Allocator_Sync* allocator,
-                  VkDevice vk_device,
-                  zx::channel token,
-                  gfx::BufferFormat format,
-                  gfx::BufferUsage usage,
+                  size_t min_buffer_count,
                   bool force_protected);
 
   // Must not be called more than once.

ui/ozone/platform/scenic/sysmem_buffer_manager.cc

@@ -37,10 +37,11 @@ scoped_refptr<SysmemBufferCollection> SysmemBufferManager::CreateCollection(
     gfx::Size size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
-    size_t num_buffers) {
+    size_t min_buffer_count) {
   auto result = base::MakeRefCounted<SysmemBufferCollection>();
-  if (!result->Initialize(allocator_.get(), size, format, usage, vk_device,
-                          num_buffers)) {
+  if (!result->Initialize(allocator_.get(), /*token_channel=*/zx::channel(),
+                          size, format, usage, vk_device, min_buffer_count,
+                          /*force_protected=*/false)) {
     return nullptr;
   }
   RegisterCollection(result.get());
@@ -52,12 +53,15 @@ SysmemBufferManager::ImportSysmemBufferCollection(
     VkDevice vk_device,
     gfx::SysmemBufferCollectionId id,
     zx::channel token,
+    gfx::Size size,
     gfx::BufferFormat format,
     gfx::BufferUsage usage,
+    size_t min_buffer_count,
     bool force_protected) {
   auto result = base::MakeRefCounted<SysmemBufferCollection>(id);
-  if (!result->Initialize(allocator_.get(), vk_device, std::move(token), format,
-                          usage, force_protected)) {
+  if (!result->Initialize(allocator_.get(), std::move(token), size, format,
+                          usage, vk_device, min_buffer_count,
+                          force_protected)) {
     return nullptr;
   }
   RegisterCollection(result.get());

ui/ozone/platform/scenic/sysmem_buffer_manager.h

@@ -47,8 +47,10 @@ class SysmemBufferManager {
       VkDevice vk_device,
       gfx::SysmemBufferCollectionId id,
       zx::channel token,
+      gfx::Size size,
       gfx::BufferFormat format,
       gfx::BufferUsage usage,
+      size_t min_buffer_count,
       bool force_protected);
 
   scoped_refptr<SysmemBufferCollection> GetCollectionById(

ui/ozone/platform/scenic/vulkan_implementation_scenic.cc

@@ -300,14 +300,21 @@ VulkanImplementationScenic::RegisterSysmemBufferCollection(
     gfx::SysmemBufferCollectionId id,
     zx::channel token,
     gfx::BufferFormat format,
-    gfx::BufferUsage usage) {
+    gfx::BufferUsage usage,
+    gfx::Size size,
+    size_t min_buffer_count) {
   // SCANOUT images must be protected in protected mode.
   bool force_protected =
       usage == gfx::BufferUsage::SCANOUT && enforce_protected_memory();
 
+  auto buffer_collection = sysmem_buffer_manager_->ImportSysmemBufferCollection(
+      device, id, std::move(token), size, format, usage, min_buffer_count,
+      force_protected);
+  if (!buffer_collection)
+    return nullptr;
+
   return std::make_unique<SysmemBufferCollectionImpl>(
-      sysmem_buffer_manager_->ImportSysmemBufferCollection(
-          device, id, std::move(token), format, usage, force_protected));
+      std::move(buffer_collection));
 }
 
 }  // namespace ui

ui/ozone/platform/scenic/vulkan_implementation_scenic.h

@@ -58,7 +58,9 @@ class VulkanImplementationScenic : public gpu::VulkanImplementation {
       gfx::SysmemBufferCollectionId id,
       zx::channel token,
       gfx::BufferFormat format,
-      gfx::BufferUsage usage) override;
+      gfx::BufferUsage usage,
+      gfx::Size size,
+      size_t min_buffer_count) override;
 
  private:
   ScenicSurfaceFactory* const scenic_surface_factory_;