Merge pull request #8137 from bunnei/improve-nvflinger-2

Follow-up fixes for NVFlinger rewrite (Part 2)

src/core/hle/service/nvflinger/buffer_queue_consumer.cpp

@@ -18,8 +18,7 @@ BufferQueueConsumer::BufferQueueConsumer(std::shared_ptr<BufferQueueCore> core_)
 BufferQueueConsumer::~BufferQueueConsumer() = default;
 
 Status BufferQueueConsumer::AcquireBuffer(BufferItem* out_buffer,
-                                          std::chrono::nanoseconds expected_present,
+                                          std::chrono::nanoseconds expected_present) {
-                                          u64 max_frame_number) {
     std::scoped_lock lock(core->mutex);
 
     // Check that the consumer doesn't currently have the maximum number of buffers acquired.
@@ -50,12 +49,6 @@ Status BufferQueueConsumer::AcquireBuffer(BufferItem* out_buffer,
         while (core->queue.size() > 1 && !core->queue[0].is_auto_timestamp) {
             const auto& buffer_item{core->queue[1]};
 
-            // If dropping entry[0] would leave us with a buffer that the consumer is not yet ready
-            // for, don't drop it.
-            if (max_frame_number && buffer_item.frame_number > max_frame_number) {
-                break;
-            }
-
             // If entry[1] is timely, drop entry[0] (and repeat).
             const auto desired_present = buffer_item.timestamp;
             if (desired_present < expected_present.count() - MAX_REASONABLE_NSEC ||
@@ -200,4 +193,39 @@ Status BufferQueueConsumer::Connect(std::shared_ptr<IConsumerListener> consumer_
     return Status::NoError;
 }
 
+Status BufferQueueConsumer::GetReleasedBuffers(u64* out_slot_mask) {
+    if (out_slot_mask == nullptr) {
+        LOG_ERROR(Service_NVFlinger, "out_slot_mask may not be nullptr");
+        return Status::BadValue;
+    }
+
+    std::scoped_lock lock(core->mutex);
+
+    if (core->is_abandoned) {
+        LOG_ERROR(Service_NVFlinger, "BufferQueue has been abandoned");
+        return Status::NoInit;
+    }
+
+    u64 mask = 0;
+    for (int s = 0; s < BufferQueueDefs::NUM_BUFFER_SLOTS; ++s) {
+        if (!slots[s].acquire_called) {
+            mask |= (1ULL << s);
+        }
+    }
+
+    // Remove from the mask queued buffers for which acquire has been called, since the consumer
+    // will not receive their buffer addresses and so must retain their cached information
+    auto current(core->queue.begin());
+    while (current != core->queue.end()) {
+        if (current->acquire_called) {
+            mask &= ~(1ULL << current->slot);
+        }
+        ++current;
+    }
+
+    LOG_DEBUG(Service_NVFlinger, "returning mask {}", mask);
+    *out_slot_mask = mask;
+    return Status::NoError;
+}
+
 } // namespace Service::android

src/core/hle/service/nvflinger/buffer_queue_consumer.h

@@ -24,10 +24,10 @@ public:
     explicit BufferQueueConsumer(std::shared_ptr<BufferQueueCore> core_);
     ~BufferQueueConsumer();
 
-    Status AcquireBuffer(BufferItem* out_buffer, std::chrono::nanoseconds expected_present,
+    Status AcquireBuffer(BufferItem* out_buffer, std::chrono::nanoseconds expected_present);
-                         u64 max_frame_number = 0);
     Status ReleaseBuffer(s32 slot, u64 frame_number, const Fence& release_fence);
     Status Connect(std::shared_ptr<IConsumerListener> consumer_listener, bool controlled_by_app);
+    Status GetReleasedBuffers(u64* out_slot_mask);
 
 private:
     std::shared_ptr<BufferQueueCore> core;

src/core/hle/service/nvflinger/buffer_queue_core.cpp

@@ -95,7 +95,6 @@ void BufferQueueCore::FreeBufferLocked(s32 slot) {
 }
 
 void BufferQueueCore::FreeAllBuffersLocked() {
-    queue.clear();
     buffer_has_been_queued = false;
 
     for (s32 slot = 0; slot < BufferQueueDefs::NUM_BUFFER_SLOTS; ++slot) {

src/core/hle/service/nvflinger/buffer_queue_core.h

@@ -73,8 +73,6 @@ private:
     u32 transform_hint{};
     bool is_allocating{};
     mutable std::condition_variable_any is_allocating_condition;
-    bool allow_allocation{true};
-    u64 buffer_age{};
     bool is_shutting_down{};
 };
 

src/core/hle/service/nvflinger/buffer_queue_producer.cpp

@@ -62,11 +62,12 @@ Status BufferQueueProducer::RequestBuffer(s32 slot, std::shared_ptr<GraphicBuffe
 
 Status BufferQueueProducer::SetBufferCount(s32 buffer_count) {
     LOG_DEBUG(Service_NVFlinger, "count = {}", buffer_count);
-    std::shared_ptr<IConsumerListener> listener;
 
+    std::shared_ptr<IConsumerListener> listener;
     {
         std::scoped_lock lock(core->mutex);
         core->WaitWhileAllocatingLocked();
+
         if (core->is_abandoned) {
             LOG_ERROR(Service_NVFlinger, "BufferQueue has been abandoned");
             return Status::NoInit;
@@ -120,7 +121,7 @@ Status BufferQueueProducer::SetBufferCount(s32 buffer_count) {
 }
 
 Status BufferQueueProducer::WaitForFreeSlotThenRelock(bool async, s32* found,
-                                                      Status* returnFlags) const {
+                                                      Status* return_flags) const {
     bool try_again = true;
 
     while (try_again) {
@@ -142,10 +143,12 @@ Status BufferQueueProducer::WaitForFreeSlotThenRelock(bool async, s32* found,
             ASSERT(slots[s].buffer_state == BufferState::Free);
             if (slots[s].graphic_buffer != nullptr) {
                 core->FreeBufferLocked(s);
-                *returnFlags |= Status::ReleaseAllBuffers;
+                *return_flags |= Status::ReleaseAllBuffers;
             }
         }
 
+        // Look for a free buffer to give to the client
+        *found = BufferQueueCore::INVALID_BUFFER_SLOT;
         s32 dequeued_count{};
         s32 acquired_count{};
         for (s32 s{}; s < max_buffer_count; ++s) {
@@ -235,20 +238,15 @@ Status BufferQueueProducer::DequeueBuffer(s32* out_slot, Fence* out_fence, bool
     {
         std::scoped_lock lock(core->mutex);
         core->WaitWhileAllocatingLocked();
+
         if (format == PixelFormat::NoFormat) {
             format = core->default_buffer_format;
         }
 
         // Enable the usage bits the consumer requested
         usage |= core->consumer_usage_bit;
-        const bool use_default_size = !width && !height;
-        if (use_default_size) {
-            width = core->default_width;
-            height = core->default_height;
-        }
 
-        s32 found = BufferItem::INVALID_BUFFER_SLOT;
+        s32 found{};
-        while (found == BufferItem::INVALID_BUFFER_SLOT) {
         Status status = WaitForFreeSlotThenRelock(async, &found, &return_flags);
         if (status != Status::NoError) {
             return status;
@@ -256,47 +254,34 @@ Status BufferQueueProducer::DequeueBuffer(s32* out_slot, Fence* out_fence, bool
 
         // This should not happen
         if (found == BufferQueueCore::INVALID_BUFFER_SLOT) {
-                LOG_DEBUG(Service_NVFlinger, "no available buffer slots");
+            LOG_ERROR(Service_NVFlinger, "no available buffer slots");
             return Status::Busy;
         }
 
-            const std::shared_ptr<GraphicBuffer>& buffer(slots[found].graphic_buffer);
-
-            // If we are not allowed to allocate new buffers, WaitForFreeSlotThenRelock must have
-            // returned a slot containing a buffer. If this buffer would require reallocation to
-            // meet the requested attributes, we free it and attempt to get another one.
-            if (!core->allow_allocation) {
-                if (buffer->NeedsReallocation(width, height, format, usage)) {
-                    core->FreeBufferLocked(found);
-                    found = BufferItem::INVALID_BUFFER_SLOT;
-                    continue;
-                }
-            }
-        }
-
         *out_slot = found;
+
         attached_by_consumer = slots[found].attached_by_consumer;
+
+        const bool use_default_size = !width && !height;
+        if (use_default_size) {
+            width = core->default_width;
+            height = core->default_height;
+        }
+
         slots[found].buffer_state = BufferState::Dequeued;
 
         const std::shared_ptr<GraphicBuffer>& buffer(slots[found].graphic_buffer);
-
+        if ((buffer == nullptr) || (buffer->Width() != width) || (buffer->Height() != height) ||
-        if ((buffer == nullptr) || buffer->NeedsReallocation(width, height, format, usage)) {
+            (buffer->Format() != format) || ((buffer->Usage() & usage) != usage)) {
             slots[found].acquire_called = false;
             slots[found].graphic_buffer = nullptr;
             slots[found].request_buffer_called = false;
             slots[found].fence = Fence::NoFence();
-            core->buffer_age = 0;
+
             return_flags |= Status::BufferNeedsReallocation;
-        } else {
-            // We add 1 because that will be the frame number when this buffer
-            // is queued
-            core->buffer_age = core->frame_counter + 1 - slots[found].frame_number;
         }
 
-        LOG_DEBUG(Service_NVFlinger, "setting buffer age to {}", core->buffer_age);
-
         *out_fence = slots[found].fence;
-
         slots[found].fence = Fence::NoFence();
     }
 
@@ -311,6 +296,7 @@ Status BufferQueueProducer::DequeueBuffer(s32* out_slot, Fence* out_fence, bool
 
         {
             std::scoped_lock lock(core->mutex);
+
             if (core->is_abandoned) {
                 LOG_ERROR(Service_NVFlinger, "BufferQueue has been abandoned");
                 return Status::NoInit;
@@ -327,6 +313,7 @@ Status BufferQueueProducer::DequeueBuffer(s32* out_slot, Fence* out_fence, bool
 
     LOG_DEBUG(Service_NVFlinger, "returning slot={} frame={}, flags={}", *out_slot,
               slots[*out_slot].frame_number, return_flags);
+
     return return_flags;
 }
 
@@ -334,6 +321,7 @@ Status BufferQueueProducer::DetachBuffer(s32 slot) {
     LOG_DEBUG(Service_NVFlinger, "slot {}", slot);
 
     std::scoped_lock lock(core->mutex);
+
     if (core->is_abandoned) {
         LOG_ERROR(Service_NVFlinger, "BufferQueue has been abandoned");
         return Status::NoInit;
@@ -369,7 +357,6 @@ Status BufferQueueProducer::DetachNextBuffer(std::shared_ptr<GraphicBuffer>* out
     }
 
     std::scoped_lock lock(core->mutex);
-
     core->WaitWhileAllocatingLocked();
 
     if (core->is_abandoned) {
@@ -423,6 +410,7 @@ Status BufferQueueProducer::AttachBuffer(s32* out_slot,
         return status;
     }
 
+    // This should not happen
     if (found == BufferQueueCore::INVALID_BUFFER_SLOT) {
         LOG_ERROR(Service_NVFlinger, "No available buffer slots");
         return Status::Busy;
@@ -466,8 +454,8 @@ Status BufferQueueProducer::QueueBuffer(s32 slot, const QueueBufferInput& input,
         return Status::BadValue;
     }
 
-    std::shared_ptr<IConsumerListener> frameAvailableListener;
+    std::shared_ptr<IConsumerListener> frame_available_listener;
-    std::shared_ptr<IConsumerListener> frameReplacedListener;
+    std::shared_ptr<IConsumerListener> frame_replaced_listener;
     s32 callback_ticket{};
     BufferItem item;
 
@@ -541,12 +529,13 @@ Status BufferQueueProducer::QueueBuffer(s32 slot, const QueueBufferInput& input,
         item.fence = fence;
         item.is_droppable = core->dequeue_buffer_cannot_block || async;
         item.swap_interval = swap_interval;
+
         sticky_transform = sticky_transform_;
 
         if (core->queue.empty()) {
             // When the queue is empty, we can simply queue this buffer
             core->queue.push_back(item);
-            frameAvailableListener = core->consumer_listener;
+            frame_available_listener = core->consumer_listener;
         } else {
             // When the queue is not empty, we need to look at the front buffer
             // state to see if we need to replace it
@@ -563,10 +552,10 @@ Status BufferQueueProducer::QueueBuffer(s32 slot, const QueueBufferInput& input,
                 }
                 // Overwrite the droppable buffer with the incoming one
                 *front = item;
-                frameReplacedListener = core->consumer_listener;
+                frame_replaced_listener = core->consumer_listener;
             } else {
                 core->queue.push_back(item);
-                frameAvailableListener = core->consumer_listener;
+                frame_available_listener = core->consumer_listener;
             }
         }
 
@@ -592,10 +581,10 @@ Status BufferQueueProducer::QueueBuffer(s32 slot, const QueueBufferInput& input,
             callback_condition.wait(callback_mutex);
         }
 
-        if (frameAvailableListener != nullptr) {
+        if (frame_available_listener != nullptr) {
-            frameAvailableListener->OnFrameAvailable(item);
+            frame_available_listener->OnFrameAvailable(item);
-        } else if (frameReplacedListener != nullptr) {
+        } else if (frame_replaced_listener != nullptr) {
-            frameReplacedListener->OnFrameReplaced(item);
+            frame_replaced_listener->OnFrameReplaced(item);
         }
 
         ++current_callback_ticket;
@@ -669,13 +658,6 @@ Status BufferQueueProducer::Query(NativeWindow what, s32* out_value) {
     case NativeWindow::ConsumerUsageBits:
         value = core->consumer_usage_bit;
         break;
-    case NativeWindow::BufferAge:
-        if (core->buffer_age > INT32_MAX) {
-            value = 0;
-        } else {
-            value = static_cast<u32>(core->buffer_age);
-        }
-        break;
     default:
         UNREACHABLE();
         return Status::BadValue;
@@ -737,7 +719,6 @@ Status BufferQueueProducer::Connect(const std::shared_ptr<IProducerListener>& li
     core->buffer_has_been_queued = false;
     core->dequeue_buffer_cannot_block =
         core->consumer_controlled_by_app && producer_controlled_by_app;
-    core->allow_allocation = true;
 
     return status;
 }
@@ -770,7 +751,7 @@ Status BufferQueueProducer::Disconnect(NativeWindowApi api) {
                 core->SignalDequeueCondition();
                 buffer_wait_event->GetWritableEvent().Signal();
                 listener = core->consumer_listener;
-            } else if (core->connected_api != NativeWindowApi::NoConnectedApi) {
+            } else {
                 LOG_ERROR(Service_NVFlinger, "still connected to another api (cur = {} req = {})",
                           core->connected_api, api);
                 status = Status::BadValue;

src/core/hle/service/nvflinger/buffer_queue_producer.h

@@ -66,7 +66,7 @@ public:
 private:
     BufferQueueProducer(const BufferQueueProducer&) = delete;
 
-    Status WaitForFreeSlotThenRelock(bool async, s32* found, Status* returnFlags) const;
+    Status WaitForFreeSlotThenRelock(bool async, s32* found, Status* return_flags) const;
 
     Kernel::KEvent* buffer_wait_event{};
     Service::KernelHelpers::ServiceContext& service_context;

src/core/hle/service/nvflinger/consumer_base.cpp

@@ -36,38 +36,41 @@ void ConsumerBase::FreeBufferLocked(s32 slot_index) {
 }
 
 void ConsumerBase::OnFrameAvailable(const BufferItem& item) {
-    std::scoped_lock lock(mutex);
     LOG_DEBUG(Service_NVFlinger, "called");
 }
 
 void ConsumerBase::OnFrameReplaced(const BufferItem& item) {
-    std::scoped_lock lock(mutex);
     LOG_DEBUG(Service_NVFlinger, "called");
 }
 
 void ConsumerBase::OnBuffersReleased() {
     std::scoped_lock lock(mutex);
+
     LOG_DEBUG(Service_NVFlinger, "called");
+
+    if (is_abandoned) {
+        // Nothing to do if we're already abandoned.
+        return;
+    }
+
+    u64 mask = 0;
+    consumer->GetReleasedBuffers(&mask);
+    for (int i = 0; i < BufferQueueDefs::NUM_BUFFER_SLOTS; i++) {
+        if (mask & (1ULL << i)) {
+            FreeBufferLocked(i);
+        }
+    }
 }
 
 void ConsumerBase::OnSidebandStreamChanged() {}
 
-Status ConsumerBase::AcquireBufferLocked(BufferItem* item, std::chrono::nanoseconds present_when,
+Status ConsumerBase::AcquireBufferLocked(BufferItem* item, std::chrono::nanoseconds present_when) {
-                                         u64 max_frame_number) {
+    Status err = consumer->AcquireBuffer(item, present_when);
-    if (is_abandoned) {
-        LOG_ERROR(Service_NVFlinger, "consumer is abandoned!");
-        return Status::NoInit;
-    }
-
-    Status err = consumer->AcquireBuffer(item, present_when, max_frame_number);
     if (err != Status::NoError) {
         return err;
     }
 
     if (item->graphic_buffer != nullptr) {
-        if (slots[item->slot].graphic_buffer != nullptr) {
-            FreeBufferLocked(item->slot);
-        }
         slots[item->slot].graphic_buffer = item->graphic_buffer;
     }
 

src/core/hle/service/nvflinger/consumer_base.h

@@ -35,8 +35,7 @@ protected:
     virtual void OnSidebandStreamChanged() override;
 
     void FreeBufferLocked(s32 slot_index);
-    Status AcquireBufferLocked(BufferItem* item, std::chrono::nanoseconds present_when,
+    Status AcquireBufferLocked(BufferItem* item, std::chrono::nanoseconds present_when);
-                               u64 max_frame_number = 0);
     Status ReleaseBufferLocked(s32 slot, const std::shared_ptr<GraphicBuffer> graphic_buffer);
     bool StillTracking(s32 slot, const std::shared_ptr<GraphicBuffer> graphic_buffer) const;
     Status AddReleaseFenceLocked(s32 slot, const std::shared_ptr<GraphicBuffer> graphic_buffer,

src/core/hle/service/nvflinger/nvflinger.cpp

@@ -104,7 +104,7 @@ void NVFlinger::SetNVDrvInstance(std::shared_ptr<Nvidia::Module> instance) {
 std::optional<u64> NVFlinger::OpenDisplay(std::string_view name) {
     const auto lock_guard = Lock();
 
-    LOG_DEBUG(Service, "Opening \"{}\" display", name);
+    LOG_DEBUG(Service_NVFlinger, "Opening \"{}\" display", name);
 
     const auto itr =
         std::find_if(displays.begin(), displays.end(),
@@ -219,7 +219,7 @@ VI::Layer* NVFlinger::FindOrCreateLayer(u64 display_id, u64 layer_id) {
     auto* layer = display->FindLayer(layer_id);
 
     if (layer == nullptr) {
-        LOG_DEBUG(Service, "Layer at id {} not found. Trying to create it.", layer_id);
+        LOG_DEBUG(Service_NVFlinger, "Layer at id {} not found. Trying to create it.", layer_id);
         CreateLayerAtId(*display, layer_id);
         return display->FindLayer(layer_id);
     }