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 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.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/drm_crtc.c | 7 ++++--- include/drm/drm_crtc.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..5c04bd4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb == fb) { + if (fb->crtc == crtc) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); set.crtc = crtc; @@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.mode = mode; set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors; + fb->crtc = crtc; set.fb = fb; ret = crtc->funcs->set_config(&set);
@@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); } - } - + } else + fb->crtc = crtc; out: mutex_unlock(&dev->mode_config.mutex); return ret; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base; + struct drm_crtc *crtc; const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];
On Fri, Nov 9, 2012 at 1:09 PM, Inki Dae inki.dae@samsung.com 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.
I am confused regarding this. If crtc points to second frame buffer, then the dma is accessing the memory region of the second framebuffer. Why can't we free the first framebuffer and its gem buffer?
With this patch, there is no way to free a framebuffer (which has been set to a crtc), without disabling the crtc.
Consider this example: I have three framebuffers (0, 1, 2) and I set them to a crtc A one by one. So with your patch, fb[0]->crtc = A fb[1]->crtc = A fb[2]->crtc = A
After this, I am using only framebuffer 0 and 1 i.e. 0 and 1 are being page flipped. Now, I want to remove framebuffer 2. fb[2]->crtc = A .. so while removing the framebuffer, we will end up disabling the crtc which is not correct.
I think, there should be an additional interface to unset a fb to a crtc. That way, we can reset the crtc inside a framebuffer so that it can be freed if not in use. i.e. fb[2]->crtc = NULL;
This patch adds drm_framebuffer to drm_crtc structure as member 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.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/drm_crtc.c | 7 ++++--- include/drm/drm_crtc.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..5c04bd4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
if (crtc->fb == fb) {
if (fb->crtc == crtc) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); set.crtc = crtc;@@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.mode = mode; set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors;
fb->crtc = crtc; set.fb = fb; ret = crtc->funcs->set_config(&set);@@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); }
}
} elsefb->crtc = crtc;out: mutex_unlock(&dev->mode_config.mutex); return ret; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc; const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/11/9 Prathyush K prathyush@chromium.org
On Fri, Nov 9, 2012 at 1:09 PM, Inki Dae inki.dae@samsung.com 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.
I am confused regarding this. If crtc points to second frame buffer, then the dma is accessing the memory region of the second framebuffer. Why can't we free the first framebuffer and its gem buffer?
With this patch, there is no way to free a framebuffer (which has been set to a crtc), without disabling the crtc.
Consider this example: I have three framebuffers (0, 1, 2) and I set them to a crtc A one by one. So with your patch, fb[0]->crtc = A fb[1]->crtc = A fb[2]->crtc = A
After this, I am using only framebuffer 0 and 1 i.e. 0 and 1 are being page flipped. Now, I want to remove framebuffer 2. fb[2]->crtc = A .. so while removing the framebuffer, we will end up disabling the crtc which is not correct.
I think, there should be an additional interface to unset a fb to a crtc. That way, we can reset the crtc inside a framebuffer so that it can be freed if not in use. i.e. fb[2]->crtc = NULL;
Right, thank you for comments. There is my missing point. how about adding fb->crtc = NULL like below then?
int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv) { .... fb->crtc = NULL; fb->funcs->destroy(fb); out: ... }
With this, when user requested rmfb to remove fb[2], fb[2]'s crtc becomes NULL so the fb would be freed without disabling crtc.
Thanks, Inki Dae
This patch adds drm_framebuffer to drm_crtc structure as member 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.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/drm_crtc.c | 7 ++++--- include/drm/drm_crtc.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..5c04bd4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
if (crtc->fb == fb) {
if (fb->crtc == crtc) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); set.crtc = crtc;@@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.mode = mode; set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors;
fb->crtc = crtc; set.fb = fb; ret = crtc->funcs->set_config(&set);@@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); }
}
} elsefb->crtc = crtc;out: mutex_unlock(&dev->mode_config.mutex); return ret; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc; const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
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.
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.
2012/11/9 Ville Syrjälä ville.syrjala@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.
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.
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.
Thanks, Inki Dae
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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).
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.
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.
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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.
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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) 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 9, 2012 at 10:50 AM, Inki Dae inki.dae@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)
- 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)
- 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,
- When rmfb is called, if fb is last one then it sets fb->crtc to NULL.
- the fb is cleaned up with disabling crtc.
On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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)
- 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)
- 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.
- 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,
- 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
2012/11/10 Ville Syrjälä ville.syrjala@linux.intel.com
On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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)
- 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)
- 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.
Do you think this is wrong that when fb0 is destroyed, the crtc also is disabled and crtc is pointing to fb1? And in case of Exynos driver, disabling the crtc would disable hardware overlay so other overlays would be holded on as is. This is just Exynos case so I don't know how other SoCs are operated. Could you explain how Desktop side is operated? Maybe there is my missing point here.
Thanks, Inki Dae
- 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,
- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Nov 10, 2012 at 10:09:02AM +0900, Inki Dae wrote:
2012/11/10 Ville Syrjälä ville.syrjala@linux.intel.com
On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote: > 2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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)
- 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)
- 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.
Do you think this is wrong that when fb0 is destroyed, the crtc also is disabled and crtc is pointing to fb1?
Yes. You must disable the crtc if and only if you destroy the current fb. Current fb means the fb specified by the latest setcrtc or pageflip ioctl. Destroying any other fb must not affect the crtc. And this is exactly what the current code does.
And in case of Exynos driver, disabling the crtc would disable hardware overlay so other overlays would be holded on as is. This is just Exynos case so I don't know how other SoCs are operated. Could you explain how Desktop side is operated? Maybe there is my missing point here.
I'm not sure what you're asking.
Are you asking how other drivers know know when it's safe to free the memory? Well, the usual method is to track the page flips via some suitable interrupts. A reference is kept to the memory until it's safe to get rid of it.
Or are you asking what should happen when a crtc is disabled? Then my answer is that when a crtc is disabled, there must be no output signal whatsoever.
It's an unfortunate design bug that we even have the fb->crtc link. Ideally it would not exist, and all scanout duties would be handled by planes. Then destroying fbs would never disable the crtc completely, just the planes scanning from those fbs.
2012/11/12 Ville Syrjälä ville.syrjala@linux.intel.com
On Sat, Nov 10, 2012 at 10:09:02AM +0900, Inki Dae wrote:
2012/11/10 Ville Syrjälä ville.syrjala@linux.intel.com
On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
2012/11/9 Ville Syrjälä ville.syrjala@linux.intel.com
> On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote: > > 2012/11/9 Ville Syrjälä ville.syrjala@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: ... }
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)
- fb release
-> check if crtc->fb is same as fb0 but current crtc is pointing to
fb1
- 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)
- 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.
Do you think this is wrong that when fb0 is destroyed, the crtc also is disabled and crtc is pointing to fb1?
Yes. You must disable the crtc if and only if you destroy the current fb. Current fb means the fb specified by the latest setcrtc or pageflip ioctl. Destroying any other fb must not affect the crtc. And this is exactly what the current code does.
Right, I realized that there was my missing point though your comment. I thought that current fb would be set to crtc->fb only if setmode request. But page flip request also did it(by specific driver's page flip function). Now Exynos driver has one problem. that is to set current fb to crtc->fb after drm_vblank_get call . This could induce invalid memory access like below,
crtc0's current fb is fb0
page flip request(crtc0, fb1) 1. drm_vblank_get call 2. vsync occurs and the dma access memory region to fb0 yet. 3. crtc0->fb = fb1 3. drm is released 4. crtc's current fb is fb1 but the dma is accessing memory region to fb0 yet because vsync doesn't occur so fb0 doesn't disable crtc and releses its own gem buffer. This would induce the problem and definitely is a bug.
For this, I will fix it as soon as possible after testing.
And in case of Exynos driver, disabling the crtc would disable hardware overlay so other overlays would be holded on as is. This is just Exynos case so I don't know how other
SoCs
are operated. Could you explain how Desktop side is operated? Maybe there is my missing point here.
I'm not sure what you're asking.
Are you asking how other drivers know know when it's safe to free the memory? Well, the usual method is to track the page flips via some suitable interrupts. A reference is kept to the memory until it's safe to get rid of it.
Or are you asking what should happen when a crtc is disabled? Then my answer is that when a crtc is disabled, there must be no output signal whatsoever.
It's an unfortunate design bug that we even have the fb->crtc link. Ideally it would not exist, and all scanout duties would be handled by planes. Then destroying fbs would never disable the crtc completely, just the planes scanning from those fbs.
Understood. And I'd like to say thank you again. Your comment is very helpful to me.
Thanks, Inki Dae
-- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae inki.dae@samsung.com 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.
btw, this should instead be done by holding a ref to the GEM object(s).. or these days you can increment the reference count on the fb and let the fb hold ref's to the GEM object(s) (which makes it a bit easier to deal with multi-planar formats)
BR, -R
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 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.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/drm_crtc.c | 7 ++++--- include/drm/drm_crtc.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..5c04bd4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
if (crtc->fb == fb) {
if (fb->crtc == crtc) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); set.crtc = crtc;@@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.mode = mode; set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors;
fb->crtc = crtc; set.fb = fb; ret = crtc->funcs->set_config(&set);@@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); }
}
} elsefb->crtc = crtc;out: mutex_unlock(&dev->mode_config.mutex); return ret; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc; const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/11/9 Rob Clark robdclark@gmail.com
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae inki.dae@samsung.com 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.
btw, this should instead be done by holding a ref to the GEM object(s).. or these days you can increment the reference count on the fb and let the fb hold ref's to the GEM object(s) (which makes it a bit easier to deal with multi-planar formats)
Rob, let's discuss that again after you read my latest comment. Please, see my latest comment.
Thanks, Inki Dae
BR, -R
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 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.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/drm_crtc.c | 7 ++++--- include/drm/drm_crtc.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ef1b221..5c04bd4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer
*fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
if (crtc->fb == fb) {
if (fb->crtc == crtc) { /* should turn off the crtc */ memset(&set, 0, sizeof(struct drm_mode_set)); set.crtc = crtc;@@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void
*data,
set.mode = mode; set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors;
fb->crtc = crtc; set.fb = fb; ret = crtc->funcs->set_config(&set);@@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device
*dev,
spin_unlock_irqrestore(&dev->event_lock, flags); kfree(e); }
}
} elsefb->crtc = crtc;out: mutex_unlock(&dev->mode_config.mutex); return ret; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc; const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 9, 2012 at 10:56 AM, Inki Dae inki.dae@samsung.com wrote:
2012/11/9 Rob Clark robdclark@gmail.com
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae inki.dae@samsung.com 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.
btw, this should instead be done by holding a ref to the GEM object(s).. or these days you can increment the reference count on the fb and let the fb hold ref's to the GEM object(s) (which makes it a bit easier to deal with multi-planar formats)
Rob, let's discuss that again after you read my latest comment. Please, see my latest comment.
oh, I think our emails crossed paths. But I think the crtc (or plane) holding a ref to the fb until scanout stops should solve the issue for you.
BR, -R
Thanks, Inki Dae
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae inki.dae@samsung.com wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc;
note that one fb can be attached to multiple crtc's (and/or planes).. so this will never work.
Anyways, I think the right answer is somehow reference-counting. You should *somewhere* be holding a ref to bo's until there is no more dma access, either directly or indirectly by holding a ref to the fb (which holds a ref to the bo(s))
BR, -R
const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012. 11. 10. 오전 2:10 Rob Clark robdclark@gmail.com 작성:
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae inki.dae@samsung.com wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3fa18b7..92889be 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -256,6 +256,7 @@ struct drm_framebuffer { struct kref refcount; struct list_head head; struct drm_mode_object base;
struct drm_crtc *crtc;note that one fb can be attached to multiple crtc's (and/or planes).. so this will never work.
I know that. drm_framebuffer'crtc means that crtc'dma is accessing this fb. So when pageflip and setcrtc are called, this crtc will be updated again.
Anyways, I think the right answer is somehow reference-counting. You should *somewhere* be holding a ref to bo's until there is no more dma access, either directly or indirectly by holding a ref to the fb (which holds a ref to the bo(s))
Understood. Yes, we could resolve this issue using reference-counting also. I'd just like to make this issue to be solved more generically. In fact, I couldn't understand why my patch has some problem yet. :(
Thanks for comments, Inki Dae
BR, -R
const struct drm_framebuffer_funcs *funcs; unsigned int pitches[4]; unsigned int offsets[4];-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Inki Dae -
Inki Dae -
Prathyush K -
Rob Clark -
Ville Syrjälä