GNOME Bugzilla – Bug 782774
kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries
Last modified: 2017-07-07 19:50:45 UTC
.
Created attachment 352077 [details] [review] kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries kmssink keeps a reference on the last rendered buffer. This reference should be released on DRAIN and ALLOCATION queries so all upstream buffers can be returned to the pool if needed.
Talking with Nicolas about the patch, this issue seems to be trickier: We have two cases: 1\ the buffer is an internal kms buffer and 2\ it is an imported dmabuf In the case 1\ as is an internal buffer, the kmssink, AFAIK, doesn't have to reclaim its own buffers, so it is not oblige to attend the DRAIN and ALLOCATION queries. In the case of 2\ as is a buffer for upstream, so kmssink has to attend those events. But not just forget about the last buffer, since it might keep being rendered by DRM. To avoid a possible glitch an option would be to copy the dmabuf onto a dumb buffer, render it, and keep it as the last buffer.
I have the problem in the case 2. But I wonder whether copy it is necessary. Just find a way to increase the reference of drm gem object or fd is enough.
In omxvideodec case, this won't be sufficient to renegotiate.
Created attachment 354965 [details] [review] kmssink: Factor out copying to dump buffer
Created attachment 354966 [details] [review] kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries kmssink keeps a reference on the last rendered buffer. If this buffer refers to an upstream buffer, it should be should be released on DRAIN and ALLOCATION queries so all upstream buffers can be returned to the pool if needed. As the buffr may be used for scanout, we copy this buffer into a dumb buffer prior to let it go.
This has been tested to not cause regressions, but I still need to write a proper test. Guillaume, could you also test this on your platform ?
Review of attachment 354966 [details] [review]: typo in commit message: "buffr". This patch doesn't seem to actually copy anything?
Review of attachment 354966 [details] [review]: Oops, missing a git add before attaching.
Created attachment 355014 [details] [review] kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries kmssink keeps a reference on the last rendered buffer. If this buffer refers to an upstream buffer, it should be should be released on DRAIN and ALLOCATION queries so all upstream buffers can be returned to the pool if needed. As the buffr may be used for scanout, we copy this buffer into a dumb buffer prior to let it go.
Created attachment 355015 [details] [review] kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries kmssink keeps a reference on the last rendered buffer. If this buffer refers to an upstream buffer, it should be should be released on DRAIN and ALLOCATION queries so all upstream buffers can be returned to the pool if needed. As the buffer may be used for scanout, we copy this buffer into a dumb buffer prior to let it go. Based on patch from Guillaume Desmottes <guillaume.desmottes@collabora.com>
Sorry, forgot the typo. If you test resolution change, you'll notice that with previous drain, you can observe a render glitch, and it's now clean again as we don't clear that last_buffer when it's owned by kmssink. I could not test the DMABuf case yet.
Review of attachment 354965 [details] [review]: Looks good to me. As nitpick I would add, in the commit log, that this a previous step for the drain logic.
Review of attachment 355015 [details] [review]: it looks good to me.
Review of attachment 355015 [details] [review]: I resurrected some ancient resolution change test for v4l2src (which I know is pretty picky about drain implementation), unfortunately it raised issues, apparently the video info is out of sync, as a side effect, we can't map last_buffer anymore. That makes the copy to fail, the buffer to be kept and finally v4l2src return EBUSY -> not-negotiated.
Review of attachment 354965 [details] [review]: I'll add a description explaining why we factor this out.
Created attachment 355048 [details] [review] kmssink: Factor out copying to dump buffer This will be used to copy any upstream memory in order to return it on resolution change, allocation query or drain query.
Created attachment 355049 [details] [review] kmssink: Don't leak GEM primed from DMABuf This otherwise breaks DMABuf reclaiming. This is not visible from userspace, but inside the kernel, the DRM driver will hold a ref to the DMABuf object. With a V4L2 driver allocating those DMABuf, it then prevent changing the resolution and re-allocation new buffers.
Created attachment 355050 [details] [review] kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries kmssink keeps a reference on the last rendered buffer. If this buffer refers to an upstream buffer, it should be should be released on DRAIN and ALLOCATION queries so all upstream buffers can be returned to the pool if needed. As the buffer may be used for scanout, we copy this buffer into a dumb buffer prior to let it go. Based on patch from Guillaume Desmottes <guillaume.desmottes@collabora.com>
New patchset, I update the commit on the "kmssink: Factor out copying to dump buffer". I also had to add a bug fix, as the GEM handle were leaked. These GEM are holding ref to the DMABuf object (kernel side). For the main patch, I now properly use show_frame() to update the image, I just realized that there was some state that would not get updated otherwise. Yet, with v4l2src ! kmssink with Intel DRM it still does not work (get ebusy from the v4l2 driver). This is caused by the DRM driver holding a ref for an arbitrary amount of time after all userspace reference being dropped and despite a non-dmabuf backed FB being set for scanout. I could of course work around with sleeps, but we'll need to figure-out something better. I have been told that Atomic DRM API would make this easier, I haven't read that yet.
Review of attachment 355049 [details] [review]: this one looks worthy to backport to 1.12 and 1.10
Created attachment 355107 [details] [review] kmssink: Move kmsmem cache code higher This will be needed as this API will be used elsewhere to clear the cache.
Created attachment 355108 [details] [review] kmssink: Track cached kmsmem and clear them on drain In this patch we keep track of the cached kmsmem in a way that we can clear the cache during the drain process. This release the framebuffer before waiting for the next vblank, hence add support for DRM driver (like Intel one) that release the associated DMABuf reference asynchronously.
Attachment 355048 [details] pushed as 6e3fe4e - kmssink: Factor out copying to dump buffer Attachment 355049 [details] pushed as cfadd5a - kmssink: Don't leak GEM primed from DMABuf Attachment 355050 [details] pushed as b8c5a4c - kmssink: drop last rendered buffer on ALLOCATION and DRAIN queries Attachment 355107 [details] pushed as 98b91d0 - kmssink: Move kmsmem cache code higher Attachment 355108 [details] pushed as 0a0bc8a - kmssink: Track cached kmsmem and clear them on drain
Backported the GEM leak fix to 1.12. commit e1c02e097d671c454e5e3afdad3c96b1eedebaf5 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Thu Jul 6 17:20:56 2017 -0400 kmssink: Don't leak GEM primed from DMABuf