DrmOutputManager

[cmeissl]

highly wip of the ideas described here: pop-os/cosmic-comp#969 (comment)

it implements most of the stuff, except:

• the capabilities stuff
• the actual format selection
• some things are stubs and need to be implemented properly

not extensively tested, but anvil seems to still be able to launch.
returning an error from DrmCompositor::new also successfully triggers the format selection
logic

TODO

A lot...

• Verify that this does not mess up the render logic with multiple commits triggering vblanks
  If this turns out to be problematic we can disable the event for commit_frame, but we need to make sure
  to kick off rendering in this case somehow..

[Paraworker]

If i understand correctly, RefCell is !Sync, which would make CompositorList both !Sync and !Send, making Arc<RwLock<>> pointless.

[cmeissl]

Indeed, sigh. !Sync is intended/expected, it should be Send , I guess this is blocked on the PixmanRenderer not being Send :(

[Paraworker]

Sorry I'm a little confused. Arc<T> is Sync or Send requires T to be both Sync and Send, but RefCell<T> is unconditional !Sync. I think RefCell<T> should be removed here.

[Drakulix]

RwLock or Mutex add the Sync bound, so our T is just required to be Send, which is indeed blocked on pixman-rs

[cmeissl]

Looking at it again we indeed need to use Mutex instead of the Refcell because RwLock requires Sync for read access.

This is the easy part, unfortunately DrmCompositor is not Send atm. Currently looking into this.

[cmeissl]

Okay, DrmCompositor is now Send as long as the allocator buffer and the framebuffer are Send + Sync. This allows DrmOutputManager to be Send and DrmOutput to implement Send and Sync.

Notes

• This currently needs unsafe impl Sync for GbmBuffer which should be removed when this lands
• The unsafe Sync impl on PixmanRenderer should be fine atm, but we need to address this in pixman-rs

[Drakulix]

gbm 0.16.1 published

[Drakulix]

I don't want to make this even more complicated, but in the future I would really like to have the option to direct scanout a fullscreen surface in cosmic-comp and then composite the rest onto an overlay plane.

[cmeissl]

tbh I don't think this will fit in the current DrmCompositor API. This will require allowing custom assign algorithms. What I have in mind is that we make something similar to the FrameState public and actually externalize the whole assigning algorithm.

[Drakulix]

Sounds good. (And is also not a very important problem atm.)

[Drakulix]

Wouldn't we want to first disable overlay planes and then fallback to no-modifer instead of doing both at the same time?

The former happens a lot more frequently on recent gpus.

[cmeissl]

Sure, this is more to provide a PoC

[Paraworker]

Hey guys I have an idea about getting rid of the Mutex in CompositorList.

We access DrmCompositor through two ways, under RwLock::write() or RwLock::read(), let me discuss separately:

RwLock::write()

In this case, we can be sure that access is unique, Mutex is redundant here.

RwLock::read()

This is the case we really need to care about. From the code I can see RwLock::read() only happens in DrmOutput::with_compositor(). If we can mark the DrmOutput with !Sync or mark DrmOutput::with_compositor() with mut, then no race condition exists since DrmOutput only access it's own DrmCompositor under RwLock::read().

While the Mutex is not need, we do still need interior mutability. A kind of cell can be introduced, which depends on the caller to do proper unique access.

struct MutexCell<T> {
    inner: UnsafeCell<T>,
}

impl<T> MutexCell<T> {
    const fn new(value: T) -> Self {
        Self { inner: UnsafeCell::new(value) }
    }

    /// SAFETY: make sure unique access
    unsafe fn get(&self) -> &mut T {
        &mut *self.inner.get()
    }
}

unsafe impl<T: Send> Sync for MutexCell<T> {}
unsafe impl<T: Send> Send for MutexCell<T> {}

Then the CompositorList would be like:

type CompositorList<A, F, U, G> = Arc<RwLock<HashMap<crtc::Handle, MutexCell<DrmCompositor<A, F, U, G>>>>>;

And the DrmOutput::with_compositor() could be like:

pub fn with_compositor<T, R>(&mut self, f: T) -> R
where
    T: FnOnce(&mut DrmCompositor<A, F, U, G>) -> R,
{
    let reader = self.compositor.read().unwrap();

    // SAFETY: unique access to our `DrmCompositor`
    f(unsafe { reader.get(&self.crtc).unwrap().get() })
}

All these are just theoretical and may be too tricky😮‍💨. I don't know how you guys think about it.