[PATCH] drm: fix drm_framebuffer cleanup.

[Rob Clark]

On Fri, Nov 9, 2012 at 10:50 AM, Inki Dae <inki.dae at 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)
> 1. 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)
> 1. 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,
> 1. When rmfb is called, if fb is last one then it sets fb->crtc to NULL.
> - the fb is cleaned up with disabling crtc.
>