[PATCH] drm: fix drm_framebuffer cleanup.

[Inki Dae]

2012/11/9 Ville Syrjälä <ville.syrjala at linux.intel.com>

> On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
> > 2012/11/9 Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > > 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:
...
}




> 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.


>
> > > 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?
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.

Thanks,
Inki Dae


> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121109/41ab379a/attachment.html>