On Fri, Nov 9, 2012 at 10:50 AM, Inki Dae inki.dae@samsung.com wrote:
I don't think so. let's see it again. below is my idea AND the reason I posted this patch. Original codes, gem alloc(gem0); -> gem0 refcount = 1 gem0 mmap -> gem0 refcount = 2 gem alloc(gem1); -> gem1 refcount =1 gem1 mmap -> gem1 refcount =2 addfb(fb0, gem0); -> gem0 refcount=3 addfb(fb1,gem1); -> gem1 refcount = 3 setcrtc(fb0, crtc0) -> crtc0.fb = fb0 pageflip(crtc0, fb1); -> crtc0.fb = fb1. and pageflip is repeated
close(gem0) -> gem0 refcount = 2 close(gem1) ->gem1 refcount = 2 munmap(gem0) ->gem0 refcount = 1 munmap(gem1) ->gem1 refcount = 1
close(drm)
- fb release
-> check if crtc->fb is same as fb0 but current crtc is pointing to fb1 2. so free fb0 without disabling current crtc. -> gem0 refcount = 0 so released. At this time, dma access invalid memory region unfortunately if the dma is accessing gem0.
I think you should either hold an extra ref to the fb until dma stops, or block when the fb is destroyed until dma stops.
See the 'drm: refcnt drm_framebuffer' patch. This lets me solve a similar issue in omapdrm. (Well, you could solve the same issue by holding a ref to the GEM objects somewhere else until the scanout stops, but it is easier for the crtc just to hold a ref to the fb.)
BR, -R
With my patch, ... setcrtc(fb0, crtc0) -> crtc0.fb = fb0, fb0.crtc = crtc0 pageflip(crtc0, fb1); -> crtc0.fb = fb1, fb1.crtc = crtc0. and pageflip is repeated
close(gem0) -> gem0 refcount = 2 close(gem1) ->gem1 refcount = 2 munmap(gem0) ->gem0 refcount = 1 munmap(gem1) ->gem1 refcount = 1 close(drm)
- fb release
-> fb0->crtc is same as crtc so disable crtc (dma stop) 2. and free fb0. -> gem0 refcount = 0 so released. We can avoid invalid memory access because the dma was stopped. In addition, one thing you pointed should be considered like below,
- When rmfb is called, if fb is last one then it sets fb->crtc to NULL.
- the fb is cleaned up with disabling crtc.