PrimeFD leak in DmabufFramebuffer?

[matthoosier-garmin]

I'm not sure whether DmabufFramebuffer is meant to be taking over ownership of the fd's passed down to its constructor or not.

If so, then then it's leaking them. If not, then it probably shouldn't hold onto the numeric values of the fd's in m_planes[].prime_fd.

I can fix this up either way, but what's the intent here?

[tomba]

Good question.

DmabufFramebuffer needs to store the fd, so that it can mmap.

But, e.g. DumbFramebuffer owns its fd, and closes it at destructor. So if you pass the DumbFb's prime_fd to DmabufFramebuffer, DmabufFramebuffer should not free it.

I admit there's no clear intent here. The kms++ resource management has kind of evolved depending on what has been needed, but there has never been proper work to really define how it goes. Another example is Card and everything you get from the Card. You can easily create a Card instance, get a Crtc pointer from it, free Card, and you've got Crtc without a Card.

So, I think, if there's any rule here, it's been that the coder needs to take care that the resources are kept alive until no one needs them anymore.

In DmabufFramebuffer's case, I think it means that the code is currently as it should be. That's not to say it cannot be changed, but if it is changed, it needs proper thought and probably other parts of kms++ need to be changed too.

[matthoosier-garmin]

Right; the dumb framebuffer allocates its own dmabuf, it naturally owns the lifetime of the fd.

For the dmabuf framebuffer, how about we just do a dup() on the incoming fd's so that the contained ones are wholly managed by the object, and it can safely close() them. The caller can continue to keep the original fd's alive (or not) according to their preference.

[tomba]

That behavior would be slightly different than with ExtFramebuffer and ExtCPUFramebuffer, as neither of those take the ownership of the passed buffer, and expect the caller to keep it alive.

Did you encounter a problem with this? I kind of would like to keep the model the same for all framebuffers, but on the other hand, if you have an actual issue that adding dup() would solve neatly, I'm ok with it.

[matthoosier-garmin]

Yeah, I quickly ran into a an exhaustion of fd's when I ran a render loop that wrapped buffers (submitted by a Wayland client) in kmsxx::DmabufFramebuffer() objects that got discarded at the end of the loop.

[tomba]

Hmm, so what components are involved in your use case? You can't close the fd in your code?

I don't know... I feel a bit uneasy changing the behavior, especially as it would be different than the other FBs. Then again, the resources for different type of FBs are somewhat different, and behave differently...

[matthoosier-garmin]

Sure, I can (and do for now) track and close the FD in the enclosing code.

My suggestion is that usage of the DmabufFramebuffer is harder to understand because of the mixed treatment of the FD (it sort-of owns them in the sense that it continues to use the same numeric FD's given to the constructor when returning values from prime_fd(), but also sort-of doesn't own them because it expects the parent to do the cleanup.

My take on the situation is that because Prime FD's map many-to-one against Prime handles, it's warranted to have the DmabufFramebuffer take private ownership of the FD's. Closing its copy of the FD does not invalidate the prime handle from which the FD was generated.

[matthoosier-garmin]

I've pretty much found that I can accomplish my use-case with ExtFramebuffer rather than using DmabufFramebuffer (the main advantage of DmabufFramebuffer is that it provides an implementation of map(), but I don't strictly speaking need that). So I don't have a burning case-in-point to motivate this issue.

Would it make sense just to make a small PR that documents how DmabufFramebuffer treats its fd's:

• The caller owns them
• The caller must not close() them until the DmabufFramebuffer is destroyed
• They are the same fd's returned (without duping) from prime_fd()

for posterity?

[tomba]

So isn't the owner model the same for Ext and Dmabuf fbs? In both cases the caller owns the buffers, and must not close them until they're no longer used. If so, why did switching to ExtFb help?

But yes, PR that adds clarifications on these kind of questions are always welcome.

[matthoosier-garmin]

Yeah, the ownership lifetimes are the same. But that's less shocking for handles because the GEM handle to a renderbuffer is by definition long-lived.

Prime FD's can be manufactured on-demand from the GEM handles, so it's more conventional in compositors to just make a new one (and hand ownership of it off) each time somebody wants one.