Re: [PATCH v8 01/14] drm: Define histogram structures exposed to user

["Murthy, Arun R" <arun.r.murthy@intel.com>]

On 19-02-2025 19:01, Simona Vetter wrote:
> 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.
Thanks for the suggestions, having a new IPC memfd in drm is a great 
idea and helps not only for this but should also help others like 
writeback etc. I will explore more on the memfd.

Thanks and Regards,
Arun R Murthy
--------------------

> -Sima