Yeah, I seem to recall noticing this while looking at the code in the past too, and I agree it should be fixed. The DRM backend may have additional similar issues when using overlays or direct scanout, which actually causes re-doing the work every output frame, not just every input frame.

I always forget what the EGL image extension specs say about content guarantees, but obviously we should avoid unneeded work when possible. So, we should verify if we can keep the EGLImage around while the client re-uses it as long as the wl_buffer exists. If that is guaranteed to work, we probably need some renderer-specific state attached to weston_buffer to implement the fix. (Note: mind the renderer hand-over.)

By "frame" you do mean every frame the client attach+commits that actually gets into output repaint, right? Not every frame the compositor paints?

(In reply to comment #1)
> Yeah, I seem to recall noticing this while looking at the code in the past
> too, and I agree it should be fixed. The DRM backend may have additional
> similar issues when using overlays or direct scanout, which actually causes
> re-doing the work every output frame, not just every input frame.
> 
> I always forget what the EGL image extension specs say about content
> guarantees, but obviously we should avoid unneeded work when possible. So,
> we should verify if we can keep the EGLImage around while the client re-uses
> it as long as the wl_buffer exists. If that is guaranteed to work, we
> probably need some renderer-specific state attached to weston_buffer to
> implement the fix. (Note: mind the renderer hand-over.)
> 
> By "frame" you do mean every frame the client attach+commits that actually
> gets into output repaint, right? Not every frame the compositor paints?

thanks for your quick response. the "frame" I mentioned is attach+commits as you described.

No, this can't actually be fixed. Holding on to EGLImages means that the EGL implementation can _never_ change its buffering strategy except on resize. Doing this would actually preclude promotion to overlays in some case: imagine you have one foreground app and one background app, both of a constant size. Switch apps so the second is now in the foreground and can be promoted to an overlay, if allocated from the correct backing store.

The foreground app would need to be able to release its buffers, and the background app would need to be able to reallocate, but this is not possible if Weston is still holding a reference to the EGLImage.

Creating and destroying images should be a very lightweight operation of altering refcounts and adding/removing EGL resource identifiers. The buffers themselves will _not_ be reallocated or destroyed unless the client-side EGL (or whatever) implementation has also released its last reference to the buffer.

If your stack is spending too long in CreateImage/DestroyImage (Vivante?), then I'd strongly recommending your stack.

Hi Daniel
sorry for the slow response.
I think it is quite expensive to allocate memory every frame, especially large memory. I have worked for several graphics companies that are all avoid to do such things.
when eglCreateImageKHR is called, my driver have to get the memory handle from the dma buf fd, and during my test, the call stack is always pending on the ioctl from user mode to kernel mode to wrap the dma buf fd.
if client side doesn't release the image when eglDestroyImageKHR is called, when should the image to be released? from EGL's point of view, there is no chance to release it

(In reply to Sunny from comment #4)
> I think it is quite expensive to allocate memory every frame, especially
> large memory. I have worked for several graphics companies that are all
> avoid to do such things.

I agree, allocation is expensive. eglCreateImageKHR + glEGLImageTargetTexture2DOES do _not_ require memory allocation - they must only take a _reference_ to existing storage. There is no allocation, deallocation, or copying required.

> when eglCreateImageKHR is called, my driver have to get the memory handle
> from the dma buf fd, and during my test, the call stack is always pending on
> the ioctl from user mode to kernel mode to wrap the dma buf fd.

I agree this is quite unfortunate. It would be possible to work around this by adding additional protocol inside your EGL implementation to retain a buffer cache - have you tried adding something like that?

In the long term, I would like to see this resolved via weak references in dmabuf, which would enable buffers to be kept alive in GPUs / display controllers / etc, and avoid the overhead of MMU control and such. But this will not be ready for a couple of kernel releases.

> if client side doesn't release the image when eglDestroyImageKHR is called,
> when should the image to be released? from EGL's point of view, there is no
> chance to release it

Huh?

The client allocates buffers through some platform-specific mechanism. It then transfers a reference to this buffer (say, dmabuf) to the compositor, and the compositor creates an EGLImage from that. The buffer will be destroyed when the last of the client references (platform-specific) and compositor references (EGLImage + GLES texture units) is destroyed, if your kernel implements correct reference counting and buffer lifetime management.

Normally this will be when the client calls eglDestroyWindowSurface and destroys the matching wl_surface. At this point, the client will drop all its references to the buffers it internally allocated for the surface, and the compositor will drop the EGLImages. The kernel can now deallocate the buffers.

Hi Daniel
Thanks for your reply, I know what you mean.
It may be an issue of my driver implementations, I will try to solve it in the driver side.