Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 27 additions & 49 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,4 @@ lto = "fat"
cosmic-protocols = { git = "https://github.com/pop-os//cosmic-protocols", branch = "main" }

[patch.crates-io]
smithay = { git = "https://github.com/smithay/smithay.git", rev = "cdc03f7" }
smithay = { git = "https://github.com/smithay/smithay.git", rev = "347b2b3" }
12 changes: 9 additions & 3 deletions src/backend/kms/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use smithay::{
output::{DrmOutputManager, LockedDrmOutputManager},
},
egl::{EGLContext, EGLDevice, EGLDisplay, context::ContextPriority},
renderer::glow::GlowRenderer,
renderer::{Renderer, glow::GlowRenderer},
session::{Session, libseat::LibSeatSession},
},
desktop::utils::OutputPresentationFeedback,
Expand All @@ -45,7 +45,7 @@ use smithay::{
drm_syncobj::supports_syncobj_eventfd,
},
};
use tracing::{error, info, warn};
use tracing::{debug, error, info, warn};
use wayland_backend::server::ClientId;

use std::{
Expand Down Expand Up @@ -322,7 +322,7 @@ impl State {
.find_map(|(crtc, surface)| (surface.connector == conn).then_some(crtc))
.cloned()
{
device.inner.surfaces.remove(&crtc).unwrap();
device.inner.surfaces.remove(&crtc).unwrap().drop_and_join();
}

if !changes.added.iter().any(|(c, _)| c == &conn) {
Expand Down Expand Up @@ -929,6 +929,12 @@ impl LockedDevice<'_> {
return Err(err.into());
}
}

// This renderer draws only infrequently; drop the imports it just
// cached so they don't pin client buffers in VRAM until its next draw.
if let Err(err) = renderer.invalidate_caches() {
debug!(?err, "Failed to invalidate main-thread renderer caches");
}
}

Ok(())
Expand Down
42 changes: 42 additions & 0 deletions src/backend/kms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ pub struct KmsState {

pub syncobj_state: Option<DrmSyncobjState>,
pub dmabuf_global: Option<DmabufGlobal>,
pending_renderer_cleanup: bool,
}

pub struct KmsGuard<'a> {
Expand Down Expand Up @@ -137,6 +138,7 @@ pub fn init_backend(

syncobj_state: None,
dmabuf_global: None,
pending_renderer_cleanup: false,
});

// manually add already present gpus
Expand Down Expand Up @@ -700,6 +702,27 @@ impl KmsState {
Ok(node)
}

/// Request a drain of the main-thread renderers' destruction queues on the next refresh.
///
/// Destroying a surface or buffer drops the textures these renderers imported from it,
/// which only *queues* the GL deletions on their contexts; the queues are flushed by
/// rendering or an explicit drain. Those renderers may not draw again for a long time,
/// so until the drain runs the dead client's buffers stay pinned in VRAM.
pub fn schedule_renderer_cleanup(&mut self) {
self.pending_renderer_cleanup = true;
}

/// Drain the GL destruction queues of the main-thread renderers, if scheduled.
pub fn run_scheduled_renderer_cleanup(&mut self) {
if !self.pending_renderer_cleanup || !self.session.is_active() {
return;
}
self.pending_renderer_cleanup = false;
if let Err(err) = self.api.cleanup_texture_cache() {
debug!(?err, "Failed to drain main-thread renderer cleanup queue");
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a new periodic cleanup function, should we be able to simply do a cleanup (or schedule a cleanup with loop_handle.insert_idle after output (re-)configuration?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that'd fix the issue as the queue is populated at drop time, not import time — surfaces imported during (re-)configuration only become garbage later, when the window closes or the client exits. A cleanup right after configuration would run before that garbage exists and then never again, so e.g. a client exiting an hour after the last hotplug would stay pinned in VRAM until the next mode change. Since the resources are queued at arbitrary points (window close, client exit, cache eviction inside smithay), there's no single event that covers them all — hence the throttled drain (a make_current + empty try_iter at most every 2s).

We could make this event-based with a smithay change (e.g. a notifier/ping when a cleanup queue gets work), but for this PR I decided to keep the change contained to cosmic-comp.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I think ideally we would simply have a method on the GraphicsApi/MultiRenderer and GlesRenderer to invalidate all caches. We know we don't need them and cleaning them up periodically still seems quite unnecessary to me.

Would you consider making a smithay PR to add such methods?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make the smithay change and I agree it's a better solution. I'm away on holiday, so when I'm back next week I'll complete my testing and submit the smithay PR. Once that's approved, I'll update this PR.

pub fn schedule_render(&mut self, output: &Output) {
for surface in self
.drm_devices
Expand Down Expand Up @@ -1285,6 +1308,25 @@ impl KmsGuard<'_> {
}
}

self.invalidate_renderer_caches();

Ok(())
}

/// Drop all cached imports held by the main-thread renderers, along with the
/// framebuffers cached for copying between a render and a target node.
///
/// Unlike the per-output render threads (which draw every frame), these renderers
/// only draw during output (re-)configuration. Between those infrequent draws their
/// import caches provide no benefit yet keep live clients' buffers pinned in VRAM;
/// the next render re-imports what it needs. Imports belonging to clients that have
/// already exited are released by the destruction-scheduled drain.
fn invalidate_renderer_caches(&mut self) {
if !self.session.is_active() {
return;
}
if let Err(err) = self.api.invalidate_caches() {
debug!(?err, "Failed to invalidate main-thread renderer caches");
}
}
}
7 changes: 3 additions & 4 deletions src/backend/kms/surface/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ use smithay::{
GlesRenderbuffer, GlesRenderer, GlesTexture, Uniform, element::TextureShaderElement,
},
glow::GlowRenderer,
multigpu::{ApiDevice, Error as MultiError, GpuManager},
multigpu::{Error as MultiError, GpuManager},
sync::SyncPoint,
utils::with_renderer_surface_state,
},
Expand Down Expand Up @@ -739,6 +739,7 @@ impl SurfaceThreadState {

fn node_removed(&mut self, node: DrmNode) {
self.api.as_mut().remove_node(&node);
self.postprocess_textures.remove(&node);
// force enumeration
let _ = self.api.devices();
}
Expand Down Expand Up @@ -1408,9 +1409,7 @@ impl SurfaceThreadState {
}
}

for device in self.api.devices_mut()? {
device.renderer_mut().cleanup_texture_cache()?;
}
self.api.cleanup_texture_cache()?;

Ok(())
}
Expand Down
7 changes: 7 additions & 0 deletions src/backend/render/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ pub struct CursorStateInner {
cursors: HashMap<CursorIcon, Cursor>,
current_image: Option<Image>,
image_cache: Vec<(Image, MemoryRenderBuffer)>,
last_cursor_icon: Option<CursorIcon>,

hidden: bool,
idle_timer: Option<RegistrationToken>,
Expand Down Expand Up @@ -320,6 +321,7 @@ impl Default for CursorStateInner {
cursors: HashMap::new(),
current_image: None,
image_cache: Vec::new(),
last_cursor_icon: None,

hidden: false,
idle_timer: None,
Expand Down Expand Up @@ -363,6 +365,11 @@ pub fn draw_cursor<R>(
return;
}

if state.last_cursor_icon != Some(current_cursor) {
state.image_cache.clear();
state.last_cursor_icon = Some(current_cursor);
}

let integer_scale = (scale.x.max(scale.y) * buffer_scale).ceil() as u32;
let frame = state
.get_named_cursor(current_cursor)
Expand Down
Loading