On Mon, Feb 17, 2025 at 06:26:17PM +0100, Simona Vetter wrote: > On Mon, Feb 17, 2025 at 12:08:08PM +0200, Pekka Paalanen wrote: > > Hi Arun, > > > > this whole series seems to be missing all the UAPI docs for the DRM > > ReST files, e.g. drm-kms.rst. The UAPI header doc comments are not a > > replacement for them, I would assume both are a requirement. > > > > Without the ReST docs it is really difficult to see how this new UAPI > > should be used. > > Seconded. But really only wanted to comment on the userspace address in > drm blobs. > > > > +/** > > > + * struct drm_histogram_config > > > + * > > > + * @hist_mode_data: address to the histogram mode specific data if any > > > > Do I understand correctly that the KMS blob will contain a userspace > > virtual memory address (a user pointer)? How does that work? What are > > the lifetime requirements for that memory? > > > > I do not remember any precedent of this, and I suspect it's not a good > > design. I believe all the data should be contained in the blobs, e.g. > > how IN_FORMATS does it. I'm not sure what would be the best UAPI here > > for returning histogram data to userspace, but at least all the data > > sent to the kernel should be contained in the blob itself since it > > seems to be quite small. Variable length is ok for blobs. > > So yeah this doesn't work for a few reasons: > > - It's very restrictive what you're allowed to do during an atomic kms > commit, and a userspace page fault due to copy_from/to_user is > definitely not ok. Which means you need to unconditionally copy before > the atomic commit in the synchronous prep phase for the user->kernel > direction, and somewhere after the entire thing has finished for the > other direction. So this is worse than just more blobs, because with > drm blobs you can at least avoid copying if nothing has changed. > > - Due to the above you also cannot synchronize with userspace for the > kernel->userspace copy. And you can't fix that with a sync_file out > fence, because the underlying dma_fence rules are what prevents you from > doing userspace page faults in atomic commit, and the same rules apply > for any other sync_file fence too. > > - More fundamentally, both drm blobs and userspace virtual address spaces > (as represented by struct mm_struct) are refconted objects, with > entirely decoupled lifetimes. You'll have UAF issues here, and if you > fix them by grabbing references you'll break the world. > > tldr; this does not work > > Alternative A: drm blob > ----------------------- > > This would work for the userspace->kernel direction, but there's some > downsides: > > - You still copy, although less often than with a userspace pointer. > > - The kernel->userspace direction doesn't work, because blob objects are > immutable. We have mutable blob properties, but mutability is achieved > by exchanging the entire blob object. There's two options to address > that: > > a) Fundamentally immutable objects is really nice api designs, so I > prefer to not change that. But in theory making blob objects mutable > would work, and probably break the world. > > b) A more benign trick would be to split the blob object id allocation > from creating the object itself. We could then allocate and return > the blob ID of the new histogram to userspace synchronously from the > atomic ioctl, while creating the object for real only in the atomic > commit. > > As long as we preallocate any memory this doesn't break and dma_fence > signalling rules. Which also means we could use the existing atomic > out-fence (or a new one for histograms) to signal to userspace when > the data is ready, so this is at least somewhat useful for > compositors without fundamental issues. > > You still suffer from additional copies here. Another detail I've forgotten: If you queue an atomic commit and then immmediately do a compositor swithc, then the new compositor would end up with the very confusing situation of having a blob property pointing at a blob which does not yet exist. And since it wont get the drm_event nor has a dma_fence out-fence, it also cannot reliably wait. So this would be awkward at best, and might actually be a cross-compositor attack vector. So yeah delayed blob object creation also don't look great, and mutable blob objects probably break compositors even harder and we'd need to make this all opt-in. We need an opt-in for all of these I think, but the more I think about it the more this alternative looks like the worst. > Alternative B: gem_bo > --------------------- > > One alternative which naturally has mutable data would be gem_bo, maybe > wrapped in a drm_fb. The issue with that is that for small histograms you > really want cpu access both in userspace and the kernel, while most > display hardware wants uncached. And all the display-only kms drivers we > have do not have a concept of cached gem_bo, unlike many of the drm > drivers with render/accel support. Which means we're adding gem_bo which > cannot be used for display, on display-only drivers, and I'd expect this > will result in compositors blowing up in funny ways to no end. > > So not a good idea either, at least not if your histograms are small and > the display hw doesn't dma them in/out already anyway. > > This also means that we'll probably need 2 interfaces here, one supporting > gem_bo for big histograms and hw that can dma in/out of them, and a 2nd > one optimized for the cpu access case. > > Alternative C: memfd > -------------------- > > I think a new drm property type that accepts memfd would fit the bill > quit well: > > - memfd can be mmap(), so you avoid copies. > > - their distinct from gem_bo, so no chaos in apis everywhere with imposter > gem_bo that cannot ever be used for display. > > - memfd can be sealed, so we can validate that they have the right size > > - thanks to umdabuf there's already core mm code to properly pin them, so > painful to implement this all. > > For a driver interface I think the memfd should be pinned as long as it's > in a drm_crtc/plane/whatever_state structure, with a kernel vmap void * > pointer already set up. That way drivers can't get this wrong. > > The uapi has a few options: > > - Allow memfd to back drm_framebuffer. This won't result in api chaos > since the compositor creates these, and these memfd should never show up > in any property that would have a real fb backed by gem_bo. This still > feels horrible to me personally, but it would allow to support > histograms that need gem_bo in the same api. Personally I think we > should just do two flavors, they're too distinct. > > - A new memfd kms object like blob objects, which you can create and > destroy and which are refcounted. Creation would also pin the memfd and > check it has a sealed size (and whatever else we want sealed). This > avoids pin/unpin every time you change the memfd property, but no idea > whether that's a real use-case. > > - memfd properties just get the file descriptor (like in/out fences do) > and the drm atomic ioctl layer transparently pins/unpins as needed. > > Personally I think option C is neat, A doable, B really only for hw that > can dma in/out of histograms and where it's big enough that doing so is a > functional requirement. Also for all these we'd need to make these new properties opt-in and hide them from compositors who cannot cope. Just defensive programming best practices. -Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
