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