<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 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>
</div></div>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></blockquote><div> </div><div>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.</div><div> </div><div>int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv) {<br>
        ....<br>        fb->crtc = NULL;<br>        fb->funcs->destroy(fb);<br>out:<br>        ...<br>} </div><div> </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">

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>
<div class="im"><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 disable<br>
> > some crtc.<br>
> ><br>
> ><br>
> It looks like that you missed my previous comment. Plaese, see the 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 really<br>
> wonder that with this patch, drm framwork could be faced with any problem.<br>
<br>
</div>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></blockquote><div> </div><div>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.</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="im"><br>
> > 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 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 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>
</div>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>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div> </div><div>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?</div>
<div>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.</div><div> </div><div>Thanks,</div><div>Inki Dae</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>