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