plane->fb will be set to new fb after update_plane callback is called by drm_mode_set_plane()
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_plane.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 60b877a..862ca1e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -204,7 +204,6 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, return ret;
plane->crtc = crtc; - plane->fb = crtc->fb;
exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);
This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 18 +++++++++++- 5 files changed, 96 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..2a52b7e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -137,8 +138,26 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, return ret;
plane->crtc = crtc; + + /* + * If old_fb exists, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (old_fb) + drm_framebuffer_unreference(old_fb); + plane->fb = crtc->fb;
+ /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(plane->fb); + exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
return 0; @@ -168,6 +187,24 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, x, y, crtc_w, crtc_h); if (ret) return ret; + /* + * If old_fb exists, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (old_fb) + drm_framebuffer_unreference(old_fb); + + plane->fb = crtc->fb; + + /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(plane->fb);
exynos_drm_crtc_commit(crtc);
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x, crtc->y, - NULL); + old_fb); if (ret) { crtc->fb = old_fb;
@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; } + + exynos_drm_fb_set_pending(old_fb, false); + exynos_drm_fb_set_pending(fb, true); } out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 4ef4cd3..f100035 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -43,11 +43,15 @@ * @fb: drm framebuffer obejct. * @buf_cnt: a buffer count to drm framebuffer. * @exynos_gem_obj: array of exynos specific gem object containing a gem object. + * @pending: indicate whehter a fb was pended by page flip. + * if true, the fb should be released after fbdev is restored to avoid + * accessing invalid memory region. */ struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER]; + bool pending; };
static void exynos_drm_fb_destroy(struct drm_framebuffer *fb) @@ -228,6 +232,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{ + struct exynos_drm_fb *exynos_fb; + + exynos_fb = to_exynos_fb(fb); + + exynos_fb->pending = pending; +} + +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{ + struct exynos_drm_fb *exynos_fb; + + exynos_fb = to_exynos_fb(fb); + + if (exynos_fb->pending) + drm_framebuffer_unreference(fb); +} + struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending); + +/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb); + /* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private; + struct drm_framebuffer *fb, *fbt;
if (!private || !private->fb_helper) return;
drm_fb_helper_restore_fbdev_mode(private->fb_helper); + + mutex_lock(&dev->mode_config.mutex); + + /* Release fb pended by page flip. */ + list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) + exynos_drm_release_pended_fb(fb); + + mutex_unlock(&dev->mode_config.mutex); } diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..37a2f68 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,8 +203,24 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
- plane->crtc = crtc; + /* + * If this plane already has a fb, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (plane->fb) + drm_framebuffer_unreference(plane->fb); + + /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(fb);
+ plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);
Changelog v2: fix page fault issue. - defer to unreference old fb to avoid page fault issue. So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
+ /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(plane->fb); + exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
+ /* + * If old_fb exists, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (old_fb) + drm_framebuffer_unreference(old_fb); + return 0; }
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
+ plane->fb = crtc->fb; + + /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(plane->fb); + exynos_drm_crtc_commit(crtc);
+ /* + * If old_fb exists, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (old_fb) + drm_framebuffer_unreference(old_fb); + return 0; }
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x, crtc->y, - NULL); + old_fb); if (ret) { crtc->fb = old_fb;
@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; } + + exynos_drm_fb_set_pending(old_fb, false); + exynos_drm_fb_set_pending(fb, true); } out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@ * @fb: drm framebuffer obejct. * @buf_cnt: a buffer count to drm framebuffer. * @exynos_gem_obj: array of exynos specific gem object containing a gem object. + * @pending: indicate whehter a fb was pended by page flip. + * if true, the fb should be released after fbdev is restored to avoid + * accessing invalid memory region. */ struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER]; + bool pending; };
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{ + struct exynos_drm_fb *exynos_fb; + + exynos_fb = to_exynos_fb(fb); + + exynos_fb->pending = pending; +} + +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{ + struct exynos_drm_fb *exynos_fb; + + exynos_fb = to_exynos_fb(fb); + + if (exynos_fb->pending) + drm_framebuffer_unreference(fb); +} + struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending); + +/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb); + /* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private; + struct drm_framebuffer *fb, *fbt;
if (!private || !private->fb_helper) return;
drm_fb_helper_restore_fbdev_mode(private->fb_helper); + + mutex_lock(&dev->mode_config.mutex); + + /* Release fb pended by page flip. */ + list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) + exynos_drm_release_pended_fb(fb); + + mutex_unlock(&dev->mode_config.mutex); } diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
- plane->crtc = crtc; + /* + * Take a reference to new fb. + * + * Taking a reference means that this plane's dma is going to access + * memory region to the new fb. + */ + drm_framebuffer_reference(fb);
+ plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);
+ /* + * If plane->fb is existed, unreference the fb. + * + * This means that memory region to the fb isn't accessed by the dma + * of this plane anymore. + */ + if (plane->fb) + drm_framebuffer_unreference(plane->fb); + + plane->fb = fb; + return 0; }
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
+ /* + * Unreference the (current)fb if plane->fb is existed. + * In exynos_update_plane(), the new fb reference count can be bigger + * than 1. So it can't be removed for that reference count. + */ + if (plane->fb) + drm_framebuffer_unreference(plane->fb); + exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF);
return 0;
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);
Hi Mr. Dae,
There is an issue with this approach.
Take this simple use case with just one crtc. (fbdev = fb0)
First, set fb1
we reference fb1 and unreference fb0.
Second, remove fb1
In this case, we are removing the current fb of the crtc We hit the function 'drm_helper_disable_unused_functions'. Here, we try to disable the crtc and then we set crtc->fb = NULL. So the value of crtc->fb is lost.
After drm release, we restore fbdev mode.
Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)
So fb1 never gets freed thus causing a memory leak.
I tested this with modetest and each time the fb/gem memory never gets freed.
Also, another issue:
If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb. And you are freeing that fb after fbdev is restored.
In a normal setup, we release DRM only during system shutdown i.e. we open the drm device during boot up and do not release drm till the end. But we keep page flipping and removing framebuffers all the time.
In this case, the pending fb memory does not get freed till we actually release drm at the very end.
I am not sure why this approach is required. We are anyway waiting for vblank before removing a framebuffer so we can be sure that the dma has stopped accessing the fb. Right?
Regards, Prathyush
exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
plane->fb = crtc->fb;/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);exynos_drm_crtc_commit(crtc);/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x, crtc->y,
NULL);
old_fb); if (ret) { crtc->fb = old_fb;@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; }
exynos_drm_fb_set_pending(old_fb, false);exynos_drm_fb_set_pending(fb, true); }out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@
- @fb: drm framebuffer obejct.
- @buf_cnt: a buffer count to drm framebuffer.
- @exynos_gem_obj: array of exynos specific gem object containing a gem
object.
- @pending: indicate whehter a fb was pended by page flip.
if true, the fb should be released after fbdev is restored to avoid*/
accessing invalid memory region.struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];
bool pending;};
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);exynos_fb->pending = pending;+}
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);if (exynos_fb->pending)drm_framebuffer_unreference(fb);+}
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);
+/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);
/* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private;
struct drm_framebuffer *fb, *fbt; if (!private || !private->fb_helper) return; drm_fb_helper_restore_fbdev_mode(private->fb_helper);mutex_lock(&dev->mode_config.mutex);/* Release fb pended by page flip. */list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head)exynos_drm_release_pended_fb(fb);mutex_unlock(&dev->mode_config.mutex);} diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
plane->crtc = crtc;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(fb);plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);/** If plane->fb is existed, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);plane->fb = fb;return 0;}
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
/** Unreference the (current)fb if plane->fb is existed.* In exynos_update_plane(), the new fb reference count can bebigger
* than 1. So it can't be removed for that reference count.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF); return 0;-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/12/4 Prathyush K prathyush@chromium.org
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);Hi Mr. Dae,
There is an issue with this approach.
Take this simple use case with just one crtc. (fbdev = fb0)
First, set fb1
we reference fb1 and unreference fb0.
Second, remove fb1
In this case, we are removing the current fb of the crtc We hit the function 'drm_helper_disable_unused_functions'. Here, we try to disable the crtc and then we set crtc->fb = NULL. So the value of crtc->fb is lost.
After drm release, we restore fbdev mode.
Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)
So fb1 never gets freed thus causing a memory leak.
No, please see drm_framebuffer_remove funtion. At this function, drm_framebuffer_unreference(fb1) is called so the fb1 is released correctly after the crtc to current fb1 is disabled like below,
drm_framebuffer_remove(...) { disable the crtc to current fb1 disable all planes to current fb1 ... drm_framebuffer_unreference(fb1); <- here }
So unreferencing to fb1 is igonored because of NULL at fbdev restore.
I tested this with modetest and each time the fb/gem memory never gets freed.
Really? is there any case that drm_framebuffer_unreference(fb1) isn't called. I couldn't find any memory leak. Anyway, I will check it again.
Also, another issue:
If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb. And you are freeing that fb after fbdev is restored.
Ok, I had mentioned about this before but leave it below again and also refer to the below email threads, http://www.spinics.net/lists/dri-devel/msg29880.html
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 4. drm is released 5. 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. At this time, page fault would be induced because dma is still accessing the fb0 but the fb0 had already been released.
So this patch defers fb releasing until fbdev is restored by drm release to avoid the page fault issue.
In a normal setup, we release DRM only during system shutdown i.e. we open the drm device during boot up and do not release drm till the end. But we keep page flipping and removing framebuffers all the time.
In this case, the pending fb memory does not get freed till we actually release drm at the very end.
For this, I mentioned above.(to defer fb releasing)
I am not sure why this approach is required. We are anyway waiting for vblank before removing a framebuffer so we can be sure that the dma has stopped accessing the fb. Right?
No, the fb to be released could be different from current fb like below,
dma is accessing fb0 current fb is fb1 try to release fb0 so crtc isn't disabled because the fb0 is not current fb and then the fb0 is released. (page fault!!!)
Thanks, Inki Dae
Regards, Prathyush
exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
plane->fb = crtc->fb;/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);exynos_drm_crtc_commit(crtc);/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x,crtc->y,
NULL);
old_fb); if (ret) { crtc->fb = old_fb;@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; }
exynos_drm_fb_set_pending(old_fb, false);exynos_drm_fb_set_pending(fb, true); }out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@
- @fb: drm framebuffer obejct.
- @buf_cnt: a buffer count to drm framebuffer.
- @exynos_gem_obj: array of exynos specific gem object containing a gem
object.
- @pending: indicate whehter a fb was pended by page flip.
if true, the fb should be released after fbdev is restored toavoid
*/
accessing invalid memory region.struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];
bool pending;};
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);exynos_fb->pending = pending;+}
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);if (exynos_fb->pending)drm_framebuffer_unreference(fb);+}
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);
+/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);
/* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private;
struct drm_framebuffer *fb, *fbt; if (!private || !private->fb_helper) return; drm_fb_helper_restore_fbdev_mode(private->fb_helper);mutex_lock(&dev->mode_config.mutex);/* Release fb pended by page flip. */list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head)exynos_drm_release_pended_fb(fb);mutex_unlock(&dev->mode_config.mutex);} diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
plane->crtc = crtc;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(fb);plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);/** If plane->fb is existed, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);plane->fb = fb;return 0;}
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
/** Unreference the (current)fb if plane->fb is existed.* In exynos_update_plane(), the new fb reference count can bebigger
* than 1. So it can't be removed for that reference count.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF); return 0;-- 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
2012/12/5 Inki Dae inki.dae@samsung.com
2012/12/4 Prathyush K prathyush@chromium.org
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);Hi Mr. Dae,
There is an issue with this approach.
Take this simple use case with just one crtc. (fbdev = fb0)
First, set fb1
we reference fb1 and unreference fb0.
Second, remove fb1
In this case, we are removing the current fb of the crtc We hit the function 'drm_helper_disable_unused_functions'. Here, we try to disable the crtc and then we set crtc->fb = NULL. So the value of crtc->fb is lost.
After drm release, we restore fbdev mode.
Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)
So fb1 never gets freed thus causing a memory leak.
No, please see drm_framebuffer_remove funtion. At this function, drm_framebuffer_unreference(fb1) is called so the fb1 is released correctly after the crtc to current fb1 is disabled like below,
drm_framebuffer_remove(...) { disable the crtc to current fb1 disable all planes to current fb1 ... drm_framebuffer_unreference(fb1); <- here }
So unreferencing to fb1 is igonored because of NULL at fbdev restore.
I tested this with modetest and each time the fb/gem memory never gets freed.
Really? is there any case that drm_framebuffer_unreference(fb1) isn't called. I couldn't find any memory leak. Anyway, I will check it again.
I have tested modetest 10 times and below is the result,
Before modetest is tested: MemTotal: 1025260 kB MemFree: 977584 kB Buffers: 7740 kB Cached: 5780 kB
After modetest is tested 10 times(modetest -v -s 17@4:1280x720): MemTotal: 1025260 kB MemFree: 979652 kB Buffers: 7748 kB Cached: 5908 kB
Thus, we can't find any memory leak. So I think you missed something.
Also, another issue:
If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb. And you are freeing that fb after fbdev is restored.
Ok, I had mentioned about this before but leave it below again and also refer to the below email threads, http://www.spinics.net/lists/dri-devel/msg29880.html
crtc0's current fb is fb0 page flip request(crtc0, fb1)
- drm_vblank_get call
- vsync occurs and the dma access memory region to fb0 yet.
- crtc0->fb = fb1
- drm is released
- 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. At this time, page fault would be induced because dma is still accessing the fb0 but the fb0 had already been released.
So this patch defers fb releasing until fbdev is restored by drm release to avoid the page fault issue.
In a normal setup, we release DRM only during system shutdown i.e. we open the drm device during boot up and do not release drm till the end. But we keep page flipping and removing framebuffers all the time.
In this case, the pending fb memory does not get freed till we actually release drm at the very end.
For this, I mentioned above.(to defer fb releasing)
I am not sure why this approach is required. We are anyway waiting for vblank before removing a framebuffer so we can be sure that the dma has stopped accessing the fb. Right?
No, the fb to be released could be different from current fb like below,
dma is accessing fb0 current fb is fb1 try to release fb0 so crtc isn't disabled because the fb0 is not current fb and then the fb0 is released. (page fault!!!)
Thanks, Inki Dae
Regards, Prathyush
exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
plane->fb = crtc->fb;/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);exynos_drm_crtc_commit(crtc);/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x,crtc->y,
NULL);
old_fb); if (ret) { crtc->fb = old_fb;@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; }
exynos_drm_fb_set_pending(old_fb, false);exynos_drm_fb_set_pending(fb, true); }out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@
- @fb: drm framebuffer obejct.
- @buf_cnt: a buffer count to drm framebuffer.
- @exynos_gem_obj: array of exynos specific gem object containing a
gem object.
- @pending: indicate whehter a fb was pended by page flip.
if true, the fb should be released after fbdev is restored toavoid
*/
accessing invalid memory region.struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];
bool pending;};
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);exynos_fb->pending = pending;+}
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);if (exynos_fb->pending)drm_framebuffer_unreference(fb);+}
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);
+/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);
/* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private;
struct drm_framebuffer *fb, *fbt; if (!private || !private->fb_helper) return; drm_fb_helper_restore_fbdev_mode(private->fb_helper);mutex_lock(&dev->mode_config.mutex);/* Release fb pended by page flip. */list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list,head)
exynos_drm_release_pended_fb(fb);mutex_unlock(&dev->mode_config.mutex);} diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
plane->crtc = crtc;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(fb);plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);/** If plane->fb is existed, unreference the fb.** This means that memory region to the fb isn't accessed by thedma
* of this plane anymore.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);plane->fb = fb;return 0;}
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
/** Unreference the (current)fb if plane->fb is existed.* In exynos_update_plane(), the new fb reference count can bebigger
* than 1. So it can't be removed for that reference count.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF); return 0;-- 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
Hi,
Please check the reference counts for framebuffers: This is for one modetest (without flipping)
Bootup: [ 2.310000] KP: drm_framebuffer_init edb86900 - fb0 refcount 1 [ 2.310000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2
Modetest: [ 26.560000] KP: drm_framebuffer_init ed43c900 - fb1 refcount 1 (done in addFB) [ 26.560000] KP: drm_framebuffer_reference ed43c900 - fb1 refcount 2 (done in setCRTC for new fb) [ 26.570000] KP: drm_framebuffer_unreference edb86900 - fb0 refcount 1 (done in setCRTC for old fb)
End of modetest and drm release: [ 39.080000] KP: drm_framebuffer_unreference ed43c900 - fb1 refcount 1 (this is done by the unref in drm_framebuffer_remove) [ 39.100000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2 (this is done when we restore fbdev)
At the end of modetest, you can see that fb1 refcount is still 1 and the memory does not get freed.
You can put a print in the low level buffer allocate and deallocate and you can see that deallocate does not get called for fb1.
And also, I see a new dma-address getting created each time - e.g. 20400000, 20800000, 20C00000.
Please check modetest without enabling flipping. modetest -s 17@4:1280x720.
Regards, Prathyush
On Wed, Dec 5, 2012 at 9:16 AM, Inki Dae inki.dae@samsung.com wrote:
2012/12/5 Inki Dae inki.dae@samsung.com
2012/12/4 Prathyush K prathyush@chromium.org
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);Hi Mr. Dae,
There is an issue with this approach.
Take this simple use case with just one crtc. (fbdev = fb0)
First, set fb1
we reference fb1 and unreference fb0.
Second, remove fb1
In this case, we are removing the current fb of the crtc We hit the function 'drm_helper_disable_unused_functions'. Here, we try to disable the crtc and then we set crtc->fb = NULL. So the value of crtc->fb is lost.
After drm release, we restore fbdev mode.
Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)
So fb1 never gets freed thus causing a memory leak.
No, please see drm_framebuffer_remove funtion. At this function, drm_framebuffer_unreference(fb1) is called so the fb1 is released correctly after the crtc to current fb1 is disabled like below,
drm_framebuffer_remove(...) { disable the crtc to current fb1 disable all planes to current fb1 ... drm_framebuffer_unreference(fb1); <- here }
So unreferencing to fb1 is igonored because of NULL at fbdev restore.
I tested this with modetest and each time the fb/gem memory never gets freed.
Really? is there any case that drm_framebuffer_unreference(fb1) isn't called. I couldn't find any memory leak. Anyway, I will check it again.
I have tested modetest 10 times and below is the result,
Before modetest is tested: MemTotal: 1025260 kB MemFree: 977584 kB Buffers: 7740 kB Cached: 5780 kB
After modetest is tested 10 times(modetest -v -s 17@4:1280x720): MemTotal: 1025260 kB MemFree: 979652 kB Buffers: 7748 kB Cached: 5908 kB
Thus, we can't find any memory leak. So I think you missed something.
Also, another issue:
If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb. And you are freeing that fb after fbdev is restored.
Ok, I had mentioned about this before but leave it below again and also refer to the below email threads, http://www.spinics.net/lists/dri-devel/msg29880.html
crtc0's current fb is fb0 page flip request(crtc0, fb1)
- drm_vblank_get call
- vsync occurs and the dma access memory region to fb0 yet.
- crtc0->fb = fb1
- drm is released
- 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. At this time, page fault would be induced because dma is still accessing the fb0 but the fb0 had already been released.
So this patch defers fb releasing until fbdev is restored by drm release to avoid the page fault issue.
In a normal setup, we release DRM only during system shutdown i.e. we open the drm device during boot up and do not release drm till the end. But we keep page flipping and removing framebuffers all the time.
In this case, the pending fb memory does not get freed till we actually release drm at the very end.
For this, I mentioned above.(to defer fb releasing)
I am not sure why this approach is required. We are anyway waiting for vblank before removing a framebuffer so we can be sure that the dma has stopped accessing the fb. Right?
No, the fb to be released could be different from current fb like below,
dma is accessing fb0 current fb is fb1 try to release fb0 so crtc isn't disabled because the fb0 is not current fb and then the fb0 is released. (page fault!!!)
Thanks, Inki Dae
Regards, Prathyush
exynos_drm_fn_encoder(crtc, &pipe,exynos_drm_encoder_crtc_pipe);
/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
plane->fb = crtc->fb;/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);exynos_drm_crtc_commit(crtc);/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x,crtc->y,
NULL);
old_fb); if (ret) { crtc->fb = old_fb;@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; }
exynos_drm_fb_set_pending(old_fb, false);exynos_drm_fb_set_pending(fb, true); }out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@
- @fb: drm framebuffer obejct.
- @buf_cnt: a buffer count to drm framebuffer.
- @exynos_gem_obj: array of exynos specific gem object containing a
gem object.
- @pending: indicate whehter a fb was pended by page flip.
if true, the fb should be released after fbdev is restored toavoid
*/
accessing invalid memory region.struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];
bool pending;};
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);exynos_fb->pending = pending;+}
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);if (exynos_fb->pending)drm_framebuffer_unreference(fb);+}
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);
+/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);
/* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private;
struct drm_framebuffer *fb, *fbt; if (!private || !private->fb_helper) return; drm_fb_helper_restore_fbdev_mode(private->fb_helper);mutex_lock(&dev->mode_config.mutex);/* Release fb pended by page flip. */list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list,head)
exynos_drm_release_pended_fb(fb);mutex_unlock(&dev->mode_config.mutex);} diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
plane->crtc = crtc;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(fb);plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);/** If plane->fb is existed, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);plane->fb = fb;return 0;}
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
/** Unreference the (current)fb if plane->fb is existed.* In exynos_update_plane(), the new fb reference count can bebigger
* than 1. So it can't be removed for that reference count.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF); return 0;-- 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
Hi
2012/12/5 Prathyush K prathyush@chromium.org
Hi,
Please check the reference counts for framebuffers: This is for one modetest (without flipping)
Bootup: [ 2.310000] KP: drm_framebuffer_init edb86900 - fb0 refcount 1 [ 2.310000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2
Modetest: [ 26.560000] KP: drm_framebuffer_init ed43c900 - fb1 refcount 1 (done in addFB) [ 26.560000] KP: drm_framebuffer_reference ed43c900 - fb1 refcount 2 (done in setCRTC for new fb) [ 26.570000] KP: drm_framebuffer_unreference edb86900 - fb0 refcount 1 (done in setCRTC for old fb)
End of modetest and drm release: [ 39.080000] KP: drm_framebuffer_unreference ed43c900 - fb1 refcount 1 (this is done by the unref in drm_framebuffer_remove) [ 39.100000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2 (this is done when we restore fbdev)
At the end of modetest, you can see that fb1 refcount is still 1 and the memory does not get freed.
You can put a print in the low level buffer allocate and deallocate and you can see that deallocate does not get called for fb1.
And also, I see a new dma-address getting created each time - e.g. 20400000, 20800000, 20C00000.
Please check modetest without enabling flipping. modetest -s 17@4 :1280x720.
We missed the test without vblank. Right tested and confirmed.
Thanks, Inki Dae
Regards, Prathyush
On Wed, Dec 5, 2012 at 9:16 AM, Inki Dae inki.dae@samsung.com wrote:
2012/12/5 Inki Dae inki.dae@samsung.com
2012/12/4 Prathyush K prathyush@chromium.org
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++ drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++- 5 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2efa4b0..b9c37eb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -32,6 +32,7 @@ #include "exynos_drm_drv.h" #include "exynos_drm_encoder.h" #include "exynos_drm_plane.h" +#include "exynos_drm_fb.h"
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\ drm_crtc) @@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, plane->crtc = crtc; plane->fb = crtc->fb;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);Hi Mr. Dae,
There is an issue with this approach.
Take this simple use case with just one crtc. (fbdev = fb0)
First, set fb1
we reference fb1 and unreference fb0.
Second, remove fb1
In this case, we are removing the current fb of the crtc We hit the function 'drm_helper_disable_unused_functions'. Here, we try to disable the crtc and then we set crtc->fb = NULL. So the value of crtc->fb is lost.
After drm release, we restore fbdev mode.
Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)
So fb1 never gets freed thus causing a memory leak.
No, please see drm_framebuffer_remove funtion. At this function, drm_framebuffer_unreference(fb1) is called so the fb1 is released correctly after the crtc to current fb1 is disabled like below,
drm_framebuffer_remove(...) { disable the crtc to current fb1 disable all planes to current fb1 ... drm_framebuffer_unreference(fb1); <- here }
So unreferencing to fb1 is igonored because of NULL at fbdev restore.
I tested this with modetest and each time the fb/gem memory never gets freed.
Really? is there any case that drm_framebuffer_unreference(fb1) isn't called. I couldn't find any memory leak. Anyway, I will check it again.
I have tested modetest 10 times and below is the result,
Before modetest is tested: MemTotal: 1025260 kB MemFree: 977584 kB Buffers: 7740 kB Cached: 5780 kB
After modetest is tested 10 times(modetest -v -s 17@4:1280x720): MemTotal: 1025260 kB MemFree: 979652 kB Buffers: 7748 kB Cached: 5908 kB
Thus, we can't find any memory leak. So I think you missed something.
Also, another issue:
If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb. And you are freeing that fb after fbdev is restored.
Ok, I had mentioned about this before but leave it below again and also refer to the below email threads, http://www.spinics.net/lists/dri-devel/msg29880.html
crtc0's current fb is fb0 page flip request(crtc0, fb1)
- drm_vblank_get call
- vsync occurs and the dma access memory region to fb0 yet.
- crtc0->fb = fb1
- drm is released
- 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. At this time, page fault would be induced because dma is still accessing the fb0 but the fb0 had already been released.
So this patch defers fb releasing until fbdev is restored by drm release to avoid the page fault issue.
In a normal setup, we release DRM only during system shutdown i.e. we open the drm device during boot up and do not release drm till the end. But we keep page flipping and removing framebuffers all the time.
In this case, the pending fb memory does not get freed till we actually release drm at the very end.
For this, I mentioned above.(to defer fb releasing)
I am not sure why this approach is required. We are anyway waiting for vblank before removing a framebuffer so we can be sure that the dma has stopped accessing the fb. Right?
No, the fb to be released could be different from current fb like below,
dma is accessing fb0 current fb is fb1 try to release fb0 so crtc isn't disabled because the fb0 is not current fb and then the fb0 is released. (page fault!!!)
Thanks, Inki Dae
Regards, Prathyush
exynos_drm_fn_encoder(crtc, &pipe,exynos_drm_encoder_crtc_pipe);
/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (ret) return ret;
plane->fb = crtc->fb;/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(plane->fb);exynos_drm_crtc_commit(crtc);/** If old_fb exists, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (old_fb)drm_framebuffer_unreference(old_fb);return 0;}
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
crtc->fb = fb; ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x,crtc->y,
NULL);
old_fb); if (ret) { crtc->fb = old_fb;@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
goto out; }
exynos_drm_fb_set_pending(old_fb, false);exynos_drm_fb_set_pending(fb, true); }out: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 7190b64..7ed5507 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -45,11 +45,15 @@
- @fb: drm framebuffer obejct.
- @buf_cnt: a buffer count to drm framebuffer.
- @exynos_gem_obj: array of exynos specific gem object containing a
gem object.
- @pending: indicate whehter a fb was pended by page flip.
if true, the fb should be released after fbdev is restored toavoid
*/
accessing invalid memory region.struct exynos_drm_fb { struct drm_framebuffer fb; unsigned int buf_cnt; struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];
bool pending;};
static int check_fb_gem_memory_type(struct drm_device *drm_dev, @@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return fb; }
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);exynos_fb->pending = pending;+}
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb) +{
struct exynos_drm_fb *exynos_fb;exynos_fb = to_exynos_fb(fb);if (exynos_fb->pending)drm_framebuffer_unreference(fb);+}
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h index 96262e5..6b63bb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h @@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj);
+/* set fb->pending variable to desired value. */ +void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);
+/* release a fb pended by page flip. */ +void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);
/* get memory information of a drm framebuffer */ struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb, int index); diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index e7466c4..62f3b9e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev) void exynos_drm_fbdev_restore_mode(struct drm_device *dev) { struct exynos_drm_private *private = dev->dev_private;
struct drm_framebuffer *fb, *fbt; if (!private || !private->fb_helper) return; drm_fb_helper_restore_fbdev_mode(private->fb_helper);mutex_lock(&dev->mode_config.mutex);/* Release fb pended by page flip. */list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list,head)
exynos_drm_release_pended_fb(fb);mutex_unlock(&dev->mode_config.mutex);} diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 862ca1e..81d7323 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret < 0) return ret;
plane->crtc = crtc;
/** Take a reference to new fb.** Taking a reference means that this plane's dma is going toaccess
* memory region to the new fb.*/drm_framebuffer_reference(fb);plane->crtc = crtc; exynos_plane_commit(plane); exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);/** If plane->fb is existed, unreference the fb.** This means that memory region to the fb isn't accessed bythe dma
* of this plane anymore.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);plane->fb = fb;return 0;}
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane) { DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
/** Unreference the (current)fb if plane->fb is existed.* In exynos_update_plane(), the new fb reference count can bebigger
* than 1. So it can't be removed for that reference count.*/if (plane->fb)drm_framebuffer_unreference(plane->fb);exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF); return 0;-- 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 mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Dec 5, 2012 at 12:55 AM, Inki Dae inki.dae@samsung.com wrote:
Hi
2012/12/5 Prathyush K prathyush@chromium.org
Hi,
Please check the reference counts for framebuffers: This is for one modetest (without flipping)
Bootup: [ 2.310000] KP: drm_framebuffer_init edb86900 - fb0 refcount 1 [ 2.310000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2
Modetest: [ 26.560000] KP: drm_framebuffer_init ed43c900 - fb1 refcount 1 (done in addFB) [ 26.560000] KP: drm_framebuffer_reference ed43c900 - fb1 refcount 2 (done in setCRTC for new fb) [ 26.570000] KP: drm_framebuffer_unreference edb86900 - fb0 refcount 1 (done in setCRTC for old fb)
End of modetest and drm release: [ 39.080000] KP: drm_framebuffer_unreference ed43c900 - fb1 refcount 1 (this is done by the unref in drm_framebuffer_remove) [ 39.100000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2 (this is done when we restore fbdev)
At the end of modetest, you can see that fb1 refcount is still 1 and the memory does not get freed.
You can put a print in the low level buffer allocate and deallocate and you can see that deallocate does not get called for fb1.
And also, I see a new dma-address getting created each time - e.g. 20400000, 20800000, 20C00000.
Please check modetest without enabling flipping. modetest -s 17@4:1280x720.
We missed the test without vblank. Right tested and confirmed.
just fwiw, I've found debugfs file giving a dump of all the fb's and gem objects in omapdrm has been quite useful in testing for and debugging memory leaks
BR, -R
2012/12/5 Rob Clark robdclark@gmail.com
On Wed, Dec 5, 2012 at 12:55 AM, Inki Dae inki.dae@samsung.com wrote:
Hi
2012/12/5 Prathyush K prathyush@chromium.org
Hi,
Please check the reference counts for framebuffers: This is for one modetest (without flipping)
Bootup: [ 2.310000] KP: drm_framebuffer_init edb86900 - fb0 refcount 1 [ 2.310000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2
Modetest: [ 26.560000] KP: drm_framebuffer_init ed43c900 - fb1 refcount 1 (done in addFB) [ 26.560000] KP: drm_framebuffer_reference ed43c900 - fb1 refcount 2 (done in setCRTC for new fb) [ 26.570000] KP: drm_framebuffer_unreference edb86900 - fb0 refcount 1 (done in setCRTC for old fb)
End of modetest and drm release: [ 39.080000] KP: drm_framebuffer_unreference ed43c900 - fb1 refcount 1 (this is done by the unref in drm_framebuffer_remove) [ 39.100000] KP: drm_framebuffer_reference edb86900 - fb0 refcount 2 (this is done when we restore fbdev)
At the end of modetest, you can see that fb1 refcount is still 1 and the memory does not get freed.
You can put a print in the low level buffer allocate and deallocate and you can see that deallocate does not get called for fb1.
And also, I see a new dma-address getting created each time - e.g. 20400000, 20800000, 20C00000.
Please check modetest without enabling flipping. modetest -s 17@4:1280x720.
We missed the test without vblank. Right tested and confirmed.
just fwiw, I've found debugfs file giving a dump of all the fb's and gem objects in omapdrm has been quite useful in testing for and debugging memory leaks
Thanks for information. I looked into your driver and it seems useful to us. Actually we have been using similar way and that includes other memory relevant things also.
Thanks, Inki Dae
BR,
-R _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 4, 2012 at 1:14 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Just an fyi, I'm looking into revamping the locking/refcounting in this area in the drm core right now. No patches yet for public consumption, but I hope that I can send out and rfc at the end of this week. Might fix your problem, too, and certainly addresses the issue Prathyush noticed, since the refcounting is done in the drm core and should be able to catch all the cases where we need to reference/unreference an fb. -Daniel
2012/12/5 Daniel Vetter daniel@ffwll.ch
On Tue, Dec 4, 2012 at 1:14 PM, Inki Dae inki.dae@samsung.com wrote:
Changelog v2: fix page fault issue.
- defer to unreference old fb to avoid page fault issue.
So with this fixup, new fb would be updated to hardware prior to old fb unreferencing. And it removes unnecessary patch, "drm/exynos: Unreference fb in exynos_disable_plane()"
Changelog v1: This patch releases the fb pended by page flip after fbdev is restored propely. And fixes invalid memory access when drm is released while doing pageflip.
This patch makes fb's refcount to be increased when setcrtc and pageflip are requested. In other words, it increases fb's refcount only if dma is going to access memory region to fb and decreases old fb's because the old fb isn't accessed by dma anymore.
This could guarantee releasing gem buffer to the fb after dma access to the gem buffer has been completed.
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Just an fyi, I'm looking into revamping the locking/refcounting in this area in the drm core right now. No patches yet for public consumption, but I hope that I can send out and rfc at the end of this week. Might fix your problem, too, and certainly addresses the issue Prathyush noticed, since the refcounting is done in the drm core and should be able to catch all the cases where we need to reference/unreference an fb.
Right, referencing/unreferencing an fb should be done in drm core and I think this is generic way.
Thanks for your effort, Inki Dae
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Inki Dae -
Prathyush K -
Rob Clark