On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote: > 2012/11/9 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote: > > > 2012/11/9 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote: > > > > > 2012/11/9 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > > > > On Fri, Nov 09, 2012 at 04:39:30PM +0900, Inki Dae wrote: > > > > > > > This patch fixes access issue to invalid memory region. > > > > > > > > > > > > > > crtc had only one drm_framebuffer object so when framebuffer > > > > > > > cleanup was requested after page flip, it'd try to disable > > > > > > > hardware overlay to current crtc. > > > > > > > But if current crtc points to another drm_framebuffer, > > > > > > > This may induce invalid memory access. > > > > > > > > > > > > > > Assume that some application are doing page flip with two > > > > > > > drm_framebuffer objects. At this time, if second drm_framebuffer > > > > > > > is set to current crtc and the cleanup to first drm_framebuffer > > > > > > > is requested once drm release is requested, then first > > > > > > > drm_framebuffer would be cleaned up without disabling > > > > > > > hardware overlay because current crtc points to second > > > > > > > drm_framebuffer. After that, gem buffer to first drm_framebuffer > > > > > > > would be released and this makes dma access invalid memory > > region. > > > > > > > > > > > > > > This patch adds drm_framebuffer to drm_crtc structure as member > > > > > > > > > > > > That is exactly the reverse of what you're doing in the patch. > > > > > > We already have crtc.fb, and you're adding fb.crtc. > > > > > > > > > > > > > and makes drm_framebuffer_cleanup function check if fb->crtc is > > > > > > > same as desired fb. And also when setcrtc and pageflip are > > > > > > > requested, it makes each drm_framebuffer point to current crtc. > > > > > > > > > > > > Looks like you're just setting the crtc.fb and fb.crtc pointers to > > > > > > exactly mirror each other in the page flip ioctl. That can't fix > > > > > > anything. > > > > > > > > > > > > > > > > > At least, when drm is released, the crtc to framebuffer that was > > recently > > > > > used for scanout can be disabled correctly. > > > > > > > > Let's take this scenario: > > > > > > > > setcrtc(crtc0, fb0) > > > > -> crtc0.fb = fb0, fb0.crtc = crtc0 > > > > page_flip(crtc0, fb1) > > > > -> crtc0.fb = fb1, fb1.crtc = crtc0 > > > > rmfb(fb0) > > > > -> ? > > > > > > > > The rmfb() will now disable crtc0 because fb0.crtc == crtc0, but > > > > let's consider this just a bug and ignore it for now. So let's assume > > > > that crtc0 won't be disabled when we call rmfb(fb0). > > > > > > > > > > > Ok, Please assume that my patch includes the below codes. when we call > > > rmfb(fb0), fb0->crtc is NULL. so fb0 will be freed without disabling > > crtc. > > > > > > int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file > > > *file_priv) { > > > .... > > > fb->crtc = NULL; > > > fb->funcs->destroy(fb); > > > out: > > > ... > > > } > > > > As stated above, I already assumed that. > > > > > > The real question is, how would your patch help? You can't free fb0 > > > > until the page flip has occured, and your patch doesn't track page > > > > flips, so how can it do anything useful? > > > > > > > > > > First of all multiple crtcs can scan out from the same fb, so a > > single > > > > > > fb.crtc pointer is clearly not enough to represent the relationship > > > > > > between fbs and crtcs. > > > > > > > > > > > > Also you're not clearing the fb.crtc pointer anywhere, so now > > > > > > destroying any framebuffer that was once used for scanout, would > > > > disable > > > > > > some crtc. > > > > > > > > > > > > > > > > > It looks like that you missed my previous comment. Plaese, see the > > > > previous > > > > > comment. And that was already pointed out by Prathyush. fb.crtc will > > be > > > > > cleared at drm_mode_rmfb(). With this, is there my missing point? I > > > > really > > > > > wonder that with this patch, drm framwork could be faced with any > > > > problem. > > > > > > > > If you clear the fb.crtc pointer before destroying the fb, then you > > > > never disable any crtcs in response to removing a framebuffer. > > > > That's not what we want when the fb is the current fb for the crtc. > > > > > > > > > > Right, there is my missing point. How about this then? Couldn't we check > > > this fb is last one or not? And if last one, it makes fb->crtc keep as > > is. > > > > That won't help, It would just make the behaviour of your code > > identical to the behaviour of the current code. > > > > > > > > So it looks like you're making things worse, not better. > > > > > > > > > > Anyway I'll just nack the whole idea. What you need to do is track > the > > > > > pending page flips, and make sure the old buffer is not freed too > > > early. > > > > > Just grab a reference to the old gem object when issuing the page > flip, > > > > > and unref it when you're sure the flip has occured. Or you could use > > > the > > > > > new drm_framebuffer refcount, but personally I don't see much point > in > > > > > that when you already have the gem object refcount at your disposal. > > > > > > > > > > > > > > > > > > > > > > > And it seems like that your saying is that each specific drivers > > > > should resolve this issue(access to invalid memory region) tracking > the > > > > pending page flip. But I think that this issue may be a missing point > drm > > > > framework missed so specific drivers is processing this instead. And > with > > > > this patch, couldn't some codes you mentioned be removed from specific > > > > drivers? because it doesn't need to track the pending page flips and > > > > relevant things anymore. > > > > > > > > There may be my missing point. I'd welcome to any comments and > advices. > > > > > > If you don't track the page flips somehow, you can't safely free the > > > previous buffer. It's that simple. You can of course make some of > > > that code generic enough to be shared between drivers. That is > > > actually what the drm_flip mechanism that I wrote for atomic page > > > flips is; a reasonably generic helper for tracking page flips. > > > > > > > > Thanks for comments. Right, now Exynos driver doesn't consider tracking > > page flips yet. This is my TODO. Anyway couldn't we improve this patch so > > that drm framework considers such thing? > > > No, because it depends on hardware specific information. The drm core > > code doesn't have a crystall ball, so it can't just magically know when > > a page flip completes. > > > > > I think with this patch, we could avoid invald memory access issue > > without > > > tracking page flips. In this case, pended page flips would be just > > ignored. > > > > I still don't understand how the patch is supposed to help. If you make > > the proposed change to rmfb() so that it doesn't disable the crtc when > > you remove a non-current fb, then this would happen: > > > > setcrtc(crtc0, fb0) > > -> crtc0.fb = fb0, fb0.crtc = crtc0 > > pageflip(crtc0, fb1); > > -> crtc0.fb = fb1, fb1.crtc = crtc0 > > rmfb(fb0); > > -> fb0.crtc = NULL; > > destroy(fb0); > > > > That is exactly the same behaviour we have today, so you still get > > the invalid memory access to fb0 if the page flip hasn't occured yet. > > And that means the fb.crtc pointer is entirely pointless. > > > > > 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. > > > > 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) No, that's wrong. The current fb is fb1, so destroying fb0 must not disable the crtc. > 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. > > That's all and the issue I faced with. > > I'd happy to give me any comments, opinions. > > Thanks, > Inki Dae -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel