mirror of
https://github.com/LadybirdBrowser/ladybird.git
synced 2024-09-11 13:36:13 +03:00
Kernel: Improve context state keeping in the VirtIOGPU3DDevice class
This is done mainly by implementing safe locking on the data structure keeping the pointers to the PerContextState objects. Therefore, this now eliminates the need for using LockRefPtr, as SpinlockProtected is enough for the whole list. The usage of HashMap in this class was questionable, and according to Sahan Fernando (the original contributor to the VirGL work also known as ccapitalK) there was no deep research on which data structure to use for keeping all pointers to PerContextState objects. Therefore, this structure is changed to IntrusiveList as the main reason and advantage to use it is that handling OOM conditions is much more simple, because if we succeeded to create a PerContextState object, we can be sure now that inserting it to the list will not cause OOM error condition.
This commit is contained in:
parent
8289759f1d
commit
7b6cea9ef4
Notes:
sideshowbarker
2024-07-17 07:43:44 +09:00
Author: https://github.com/supercomputer7 Commit: https://github.com/SerenityOS/serenity/commit/7b6cea9ef4
@ -15,9 +15,10 @@
|
||||
|
||||
namespace Kernel {
|
||||
|
||||
VirtIOGPU3DDevice::PerContextState::PerContextState(Graphics::VirtIOGPU::ContextID context_id, OwnPtr<Memory::Region> transfer_buffer_region)
|
||||
VirtIOGPU3DDevice::PerContextState::PerContextState(OpenFileDescription& description, Graphics::VirtIOGPU::ContextID context_id, OwnPtr<Memory::Region> transfer_buffer_region)
|
||||
: m_context_id(context_id)
|
||||
, m_transfer_buffer_region(move(transfer_buffer_region))
|
||||
, m_attached_file_description(description)
|
||||
{
|
||||
}
|
||||
|
||||
@ -43,34 +44,45 @@ VirtIOGPU3DDevice::VirtIOGPU3DDevice(VirtIOGraphicsAdapter const& graphics_adapt
|
||||
|
||||
void VirtIOGPU3DDevice::detach(OpenFileDescription& description)
|
||||
{
|
||||
m_context_state_lookup.remove(&description);
|
||||
m_context_state_list.with([&description](auto& list) {
|
||||
for (auto& context : list) {
|
||||
if (&context.description() == &description) {
|
||||
context.m_list_node.remove();
|
||||
// NOTE: We break here because there shouldn't be another context
|
||||
// being attached to this OpenFileDescription.
|
||||
break;
|
||||
}
|
||||
}
|
||||
});
|
||||
CharacterDevice::detach(description);
|
||||
}
|
||||
|
||||
ErrorOr<LockRefPtr<VirtIOGPU3DDevice::PerContextState>> VirtIOGPU3DDevice::get_context_for_description(OpenFileDescription& description)
|
||||
{
|
||||
auto res = m_context_state_lookup.get(&description);
|
||||
if (!res.has_value())
|
||||
return EBADF;
|
||||
return res.value();
|
||||
}
|
||||
|
||||
ErrorOr<void> VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigned request, Userspace<void*> arg)
|
||||
{
|
||||
|
||||
auto get_context_for_description = [](ContextList& list, OpenFileDescription& description) -> ErrorOr<NonnullRefPtr<PerContextState>> {
|
||||
for (auto& context : list) {
|
||||
if (&context.description() == &description)
|
||||
return context;
|
||||
}
|
||||
return EBADF;
|
||||
};
|
||||
|
||||
// TODO: We really should have ioctls for destroying resources as well
|
||||
switch (request) {
|
||||
case VIRGL_IOCTL_CREATE_CONTEXT: {
|
||||
if (m_context_state_lookup.contains(&description))
|
||||
return EEXIST;
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
// TODO: Delete the context if it fails to be set in m_context_state_lookup
|
||||
auto context_id = TRY(m_graphics_adapter->create_context());
|
||||
LockRefPtr<PerContextState> per_context_state = TRY(PerContextState::try_create(context_id));
|
||||
TRY(m_context_state_lookup.try_set(&description, per_context_state));
|
||||
return {};
|
||||
return m_context_state_list.with([get_context_for_description, &description, this](auto& list) -> ErrorOr<void> {
|
||||
if (auto result = get_context_for_description(list, description); !result.is_error())
|
||||
return EEXIST;
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
// TODO: Delete the context if it fails to be set in m_context_state_lookup
|
||||
auto context_id = TRY(m_graphics_adapter->create_context());
|
||||
auto per_context_state = TRY(PerContextState::try_create(description, context_id));
|
||||
list.append(per_context_state);
|
||||
return {};
|
||||
});
|
||||
}
|
||||
case VIRGL_IOCTL_TRANSFER_DATA: {
|
||||
auto& transfer_buffer_region = TRY(get_context_for_description(description))->transfer_buffer_region();
|
||||
auto user_transfer_descriptor = static_ptr_cast<VirGLTransferDescriptor const*>(arg);
|
||||
auto transfer_descriptor = TRY(copy_typed_from_user(user_transfer_descriptor));
|
||||
if (Checked<size_t>::addition_would_overflow(transfer_descriptor.offset_in_region, transfer_descriptor.num_bytes)) {
|
||||
@ -79,34 +91,40 @@ ErrorOr<void> VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigne
|
||||
if (transfer_descriptor.offset_in_region + transfer_descriptor.num_bytes > NUM_TRANSFER_REGION_PAGES * PAGE_SIZE) {
|
||||
return EOVERFLOW;
|
||||
}
|
||||
if (transfer_descriptor.direction == VIRGL_DATA_DIR_GUEST_TO_HOST) {
|
||||
auto target = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr();
|
||||
return copy_from_user(target, transfer_descriptor.data, transfer_descriptor.num_bytes);
|
||||
} else if (transfer_descriptor.direction == VIRGL_DATA_DIR_HOST_TO_GUEST) {
|
||||
auto source = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr();
|
||||
return copy_to_user(transfer_descriptor.data, source, transfer_descriptor.num_bytes);
|
||||
} else {
|
||||
return EINVAL;
|
||||
}
|
||||
|
||||
return m_context_state_list.with([get_context_for_description, &description, &transfer_descriptor](auto& list) -> ErrorOr<void> {
|
||||
auto context = TRY(get_context_for_description(list, description));
|
||||
auto& transfer_buffer_region = context->transfer_buffer_region();
|
||||
if (transfer_descriptor.direction == VIRGL_DATA_DIR_GUEST_TO_HOST) {
|
||||
auto target = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr();
|
||||
return copy_from_user(target, transfer_descriptor.data, transfer_descriptor.num_bytes);
|
||||
} else if (transfer_descriptor.direction == VIRGL_DATA_DIR_HOST_TO_GUEST) {
|
||||
auto source = transfer_buffer_region.vaddr().offset(transfer_descriptor.offset_in_region).as_ptr();
|
||||
return copy_to_user(transfer_descriptor.data, source, transfer_descriptor.num_bytes);
|
||||
} else {
|
||||
return EINVAL;
|
||||
}
|
||||
});
|
||||
}
|
||||
case VIRGL_IOCTL_SUBMIT_CMD: {
|
||||
auto context_id = TRY(get_context_for_description(description))->context_id();
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
auto user_command_buffer = static_ptr_cast<VirGLCommandBuffer const*>(arg);
|
||||
auto command_buffer = TRY(copy_typed_from_user(user_command_buffer));
|
||||
TRY(m_graphics_adapter->submit_command_buffer(context_id, [&](Bytes buffer) {
|
||||
auto num_bytes = command_buffer.num_elems * sizeof(u32);
|
||||
VERIFY(num_bytes <= buffer.size());
|
||||
MUST(copy_from_user(buffer.data(), command_buffer.data, num_bytes));
|
||||
return num_bytes;
|
||||
}));
|
||||
return {};
|
||||
return m_context_state_list.with([get_context_for_description, &description, &command_buffer, this](auto& list) -> ErrorOr<void> {
|
||||
auto context = TRY(get_context_for_description(list, description));
|
||||
auto context_id = context->context_id();
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
TRY(m_graphics_adapter->submit_command_buffer(context_id, [&](Bytes buffer) {
|
||||
auto num_bytes = command_buffer.num_elems * sizeof(u32);
|
||||
VERIFY(num_bytes <= buffer.size());
|
||||
MUST(copy_from_user(buffer.data(), command_buffer.data, num_bytes));
|
||||
return num_bytes;
|
||||
}));
|
||||
return {};
|
||||
});
|
||||
}
|
||||
case VIRGL_IOCTL_CREATE_RESOURCE: {
|
||||
auto per_context_state = TRY(get_context_for_description(description));
|
||||
auto user_spec = static_ptr_cast<VirGL3DResourceSpec const*>(arg);
|
||||
VirGL3DResourceSpec spec = TRY(copy_typed_from_user(user_spec));
|
||||
|
||||
Graphics::VirtIOGPU::Protocol::Resource3DSpecification const resource_spec = {
|
||||
.target = static_cast<Graphics::VirtIOGPU::Protocol::Gallium::PipeTextureTarget>(spec.target),
|
||||
.format = spec.format,
|
||||
@ -120,15 +138,18 @@ ErrorOr<void> VirtIOGPU3DDevice::ioctl(OpenFileDescription& description, unsigne
|
||||
.flags = spec.flags,
|
||||
.padding = 0,
|
||||
};
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
// FIXME: What would be an appropriate resource free-ing mechanism to use in case anything
|
||||
// after this fails?
|
||||
auto resource_id = TRY(m_graphics_adapter->create_3d_resource(resource_spec));
|
||||
TRY(m_graphics_adapter->attach_resource_to_context(resource_id, per_context_state->context_id()));
|
||||
TRY(m_graphics_adapter->ensure_backing_storage(resource_id, per_context_state->transfer_buffer_region(), 0, NUM_TRANSFER_REGION_PAGES * PAGE_SIZE));
|
||||
spec.created_resource_id = resource_id.value();
|
||||
// FIXME: We should delete the resource we just created if we fail to copy the resource id out
|
||||
return copy_to_user(static_ptr_cast<VirGL3DResourceSpec*>(arg), &spec);
|
||||
return m_context_state_list.with([get_context_for_description, &description, &resource_spec, &spec, &arg, this](auto& list) -> ErrorOr<void> {
|
||||
auto context = TRY(get_context_for_description(list, description));
|
||||
SpinlockLocker locker(m_graphics_adapter->operation_lock());
|
||||
// FIXME: What would be an appropriate resource free-ing mechanism to use in case anything
|
||||
// after this fails?
|
||||
auto resource_id = TRY(m_graphics_adapter->create_3d_resource(resource_spec));
|
||||
TRY(m_graphics_adapter->attach_resource_to_context(resource_id, context->context_id()));
|
||||
TRY(m_graphics_adapter->ensure_backing_storage(resource_id, context->transfer_buffer_region(), 0, NUM_TRANSFER_REGION_PAGES * PAGE_SIZE));
|
||||
spec.created_resource_id = resource_id.value();
|
||||
// FIXME: We should delete the resource we just created if we fail to copy the resource id out
|
||||
return copy_to_user(static_ptr_cast<VirGL3DResourceSpec*>(arg), &spec);
|
||||
});
|
||||
}
|
||||
}
|
||||
return EINVAL;
|
||||
|
@ -7,9 +7,11 @@
|
||||
#pragma once
|
||||
|
||||
#include <AK/DistinctNumeric.h>
|
||||
#include <AK/IntrusiveList.h>
|
||||
#include <Kernel/Devices/CharacterDevice.h>
|
||||
#include <Kernel/Devices/DeviceManagement.h>
|
||||
#include <Kernel/Graphics/VirtIOGPU/Protocol.h>
|
||||
#include <Kernel/Locking/SpinlockProtected.h>
|
||||
|
||||
namespace Kernel::Graphics::VirtIOGPU {
|
||||
|
||||
@ -101,24 +103,35 @@ private:
|
||||
VirtIOGPU3DDevice(VirtIOGraphicsAdapter const& graphics_adapter, NonnullOwnPtr<Memory::Region> transfer_buffer_region, Graphics::VirtIOGPU::ContextID kernel_context_id);
|
||||
|
||||
class PerContextState final : public AtomicRefCounted<PerContextState> {
|
||||
friend class VirtIOGPU3DDevice;
|
||||
|
||||
public:
|
||||
static ErrorOr<LockRefPtr<PerContextState>> try_create(Graphics::VirtIOGPU::ContextID context_id)
|
||||
static ErrorOr<NonnullRefPtr<PerContextState>> try_create(OpenFileDescription& description, Graphics::VirtIOGPU::ContextID context_id)
|
||||
{
|
||||
auto region_result = TRY(MM.allocate_kernel_region(
|
||||
NUM_TRANSFER_REGION_PAGES * PAGE_SIZE,
|
||||
"VIRGL3D userspace upload buffer"sv,
|
||||
Memory::Region::Access::ReadWrite,
|
||||
AllocationStrategy::AllocateNow));
|
||||
return TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) PerContextState(context_id, move(region_result))));
|
||||
return TRY(adopt_nonnull_ref_or_enomem(new (nothrow) PerContextState(description, context_id, move(region_result))));
|
||||
}
|
||||
Graphics::VirtIOGPU::ContextID context_id() { return m_context_id; }
|
||||
Memory::Region& transfer_buffer_region() { return *m_transfer_buffer_region; }
|
||||
|
||||
OpenFileDescription& description() { return m_attached_file_description; }
|
||||
|
||||
private:
|
||||
PerContextState() = delete;
|
||||
explicit PerContextState(Graphics::VirtIOGPU::ContextID context_id, OwnPtr<Memory::Region> transfer_buffer_region);
|
||||
PerContextState(OpenFileDescription&, Graphics::VirtIOGPU::ContextID context_id, OwnPtr<Memory::Region> transfer_buffer_region);
|
||||
Graphics::VirtIOGPU::ContextID m_context_id;
|
||||
OwnPtr<Memory::Region> m_transfer_buffer_region;
|
||||
|
||||
// NOTE: We clean this whole object when the file description is closed, therefore we need to hold
|
||||
// a raw reference here instead of a strong reference pointer (e.g. RefPtr, which will make it
|
||||
// possible to leak the attached OpenFileDescription for a context in this device).
|
||||
OpenFileDescription& m_attached_file_description;
|
||||
|
||||
IntrusiveListNode<PerContextState, NonnullRefPtr<PerContextState>> m_list_node;
|
||||
};
|
||||
|
||||
virtual bool can_read(OpenFileDescription const&, u64) const override { return true; }
|
||||
@ -130,13 +143,13 @@ private:
|
||||
virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override;
|
||||
virtual void detach(OpenFileDescription&) override;
|
||||
|
||||
private:
|
||||
ErrorOr<LockRefPtr<PerContextState>> get_context_for_description(OpenFileDescription&);
|
||||
using ContextList = IntrusiveListRelaxedConst<&PerContextState::m_list_node>;
|
||||
|
||||
private:
|
||||
NonnullLockRefPtr<VirtIOGraphicsAdapter> m_graphics_adapter;
|
||||
// Context used for kernel operations (e.g. flushing resources to scanout)
|
||||
Graphics::VirtIOGPU::ContextID m_kernel_context_id;
|
||||
HashMap<OpenFileDescription*, LockRefPtr<PerContextState>> m_context_state_lookup;
|
||||
SpinlockProtected<ContextList, LockRank::None> m_context_state_list;
|
||||
// Memory management for backing buffers
|
||||
NonnullOwnPtr<Memory::Region> m_transfer_buffer_region;
|
||||
constexpr static size_t NUM_TRANSFER_REGION_PAGES = 1024;
|
||||
|
Loading…
Reference in New Issue
Block a user