<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae <span dir="ltr"><<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Changelog v2:<br>
fix page fault issue.<br>
- defer to unreference old fb to avoid page fault issue.<br>
So with this fixup, new fb would be updated to hardware<br>
prior to old fb unreferencing. And it removes unnecessary<br>
patch, "drm/exynos: Unreference fb in exynos_disable_plane()"<br>
<br>
Changelog v1:<br>
<div class="im">This patch releases the fb pended by page flip after fbdev is<br>
restored propely. And fixes invalid memory access when drm is<br>
released while doing pageflip.<br>
<br>
This patch makes fb's refcount to be increased when setcrtc and<br>
pageflip are requested. In other words, it increases fb's refcount<br>
only if dma is going to access memory region to fb and decreases<br>
old fb's because the old fb isn't accessed by dma anymore.<br>
<br>
This could guarantee releasing gem buffer to the fb after dma<br>
access to the gem buffer has been completed.<br>
<br>
Signed-off-by: Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>><br>
Signed-off-by: Kyungmin Park <<a href="mailto:kyungmin.park@samsung.com">kyungmin.park@samsung.com</a>><br>
---<br>
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 42 ++++++++++++++++++++++++++++-<br>
drivers/gpu/drm/exynos/exynos_drm_fb.c | 23 ++++++++++++++++<br>
drivers/gpu/drm/exynos/exynos_drm_fb.h | 6 ++++<br>
drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 9 ++++++<br>
</div> drivers/gpu/drm/exynos/exynos_drm_plane.c | 28 ++++++++++++++++++-<br>
5 files changed, 106 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c<br>
index 2efa4b0..b9c37eb 100644<br>
<div class="im">--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c<br>
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c<br>
@@ -32,6 +32,7 @@<br>
#include "exynos_drm_drv.h"<br>
#include "exynos_drm_encoder.h"<br>
#include "exynos_drm_plane.h"<br>
+#include "exynos_drm_fb.h"<br>
<br>
#define to_exynos_crtc(x) container_of(x, struct exynos_drm_crtc,\<br>
drm_crtc)<br>
</div>@@ -139,8 +140,25 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,<br>
plane->crtc = crtc;<br>
<div class="im"> plane->fb = crtc->fb;<br>
<br>
+ /*<br>
+ * Take a reference to new fb.<br>
+ *<br>
+ * Taking a reference means that this plane's dma is going to access<br>
+ * memory region to the new fb.<br>
+ */<br>
+ drm_framebuffer_reference(plane->fb);<br>
+<br></div></blockquote><div><br></div><div>Hi Mr. Dae,</div><div><br></div><div>There is an issue with this approach.</div><div><br></div><div>Take this simple use case with just one crtc. (fbdev = fb0)</div><div><br></div>
<div>First, set fb1<br></div><div><br></div><div>we reference fb1 and unreference fb0.</div><div><br></div><div>Second, remove fb1<br></div><div><br></div><div>In this case, we are removing the current fb of the crtc </div>
<div>We hit the function 'drm_helper_disable_unused_functions'.<br></div><div>Here, we try to disable the crtc and then we set crtc->fb = NULL.<br></div><div>So the value of crtc->fb is lost.<br></div><div><br>
</div><div>After drm release, we restore fbdev mode.</div><div><br></div><div>Now we reference fb0 - but we fail to unreference fb1. (old_fb is NULL)</div><div><br></div><div>So fb1 never gets freed thus causing a memory leak.</div>
<div><br></div><div>I tested this with modetest and each time the fb/gem memory never gets freed.</div><div><br></div><div>Also, another issue:<br></div><div><br></div><div>If a page flip is pending, you set the 'pending' flag and do not actually unreference the fb.</div>
<div>And you are freeing that fb after fbdev is restored.</div><div><br></div><div>In a normal setup, we release DRM only during system shutdown i.e. we open the drm</div><div>device during boot up and do not release drm till the end. But we keep page flipping and removing</div>
<div>framebuffers all the time.</div><div><br></div><div>In this case, the pending fb memory does not get freed till we actually release drm at the</div><div>very end.</div><div><br></div><div>I am not sure why this approach is required.</div>
<div>We are anyway waiting for vblank before removing a framebuffer so we can be sure that</div><div>the dma has stopped accessing the fb. Right?</div><div><br></div><div>Regards,</div><div>Prathyush</div><div><br></div><div>
<br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);<br>
<br>
+ /*<br>
+ * If old_fb exists, unreference the fb.<br>
+ *<br>
+ * This means that memory region to the fb isn't accessed by the dma<br>
+ * of this plane anymore.<br>
+ */<br>
+ if (old_fb)<br>
+ drm_framebuffer_unreference(old_fb);<br>
+<br>
</div> return 0;<br>
}<br>
<br>
@@ -169,8 +187,27 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,<br>
if (ret)<br>
return ret;<br>
<br>
<div class="im">+ plane->fb = crtc->fb;<br>
+<br>
+ /*<br>
+ * Take a reference to new fb.<br>
+ *<br>
+ * Taking a reference means that this plane's dma is going to access<br>
+ * memory region to the new fb.<br>
+ */<br>
+ drm_framebuffer_reference(plane->fb);<br>
</div>+<br>
exynos_drm_crtc_commit(crtc);<br>
<div class="im"><br>
+ /*<br>
+ * If old_fb exists, unreference the fb.<br>
+ *<br>
+ * This means that memory region to the fb isn't accessed by the dma<br>
+ * of this plane anymore.<br>
+ */<br>
+ if (old_fb)<br>
+ drm_framebuffer_unreference(old_fb);<br>
+<br>
</div> return 0;<br>
<div class="im"> }<br>
<br>
@@ -243,7 +280,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,<br>
<br>
crtc->fb = fb;<br>
ret = exynos_drm_crtc_mode_set_base(crtc, crtc->x, crtc->y,<br>
- NULL);<br>
+ old_fb);<br>
if (ret) {<br>
crtc->fb = old_fb;<br>
<br>
@@ -254,6 +291,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,<br>
<br>
goto out;<br>
}<br>
+<br>
+ exynos_drm_fb_set_pending(old_fb, false);<br>
+ exynos_drm_fb_set_pending(fb, true);<br>
}<br>
out:<br>
mutex_unlock(&dev->struct_mutex);<br>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c<br>
</div>index 7190b64..7ed5507 100644<br>
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c<br>
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c<br>
@@ -45,11 +45,15 @@<br>
<div class="im"> * @fb: drm framebuffer obejct.<br>
* @buf_cnt: a buffer count to drm framebuffer.<br>
* @exynos_gem_obj: array of exynos specific gem object containing a gem object.<br>
+ * @pending: indicate whehter a fb was pended by page flip.<br>
+ * if true, the fb should be released after fbdev is restored to avoid<br>
+ * accessing invalid memory region.<br>
*/<br>
struct exynos_drm_fb {<br>
struct drm_framebuffer fb;<br>
unsigned int buf_cnt;<br>
struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER];<br>
+ bool pending;<br>
};<br>
<br>
</div> static int check_fb_gem_memory_type(struct drm_device *drm_dev,<br>
@@ -278,6 +282,25 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,<br>
<div><div class="h5"> return fb;<br>
}<br>
<br>
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending)<br>
+{<br>
+ struct exynos_drm_fb *exynos_fb;<br>
+<br>
+ exynos_fb = to_exynos_fb(fb);<br>
+<br>
+ exynos_fb->pending = pending;<br>
+}<br>
+<br>
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb)<br>
+{<br>
+ struct exynos_drm_fb *exynos_fb;<br>
+<br>
+ exynos_fb = to_exynos_fb(fb);<br>
+<br>
+ if (exynos_fb->pending)<br>
+ drm_framebuffer_unreference(fb);<br>
+}<br>
+<br>
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb,<br>
int index)<br>
{<br>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h<br>
index 96262e5..6b63bb1 100644<br>
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.h<br>
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h<br>
@@ -33,6 +33,12 @@ exynos_drm_framebuffer_init(struct drm_device *dev,<br>
struct drm_mode_fb_cmd2 *mode_cmd,<br>
struct drm_gem_object *obj);<br>
<br>
+/* set fb->pending variable to desired value. */<br>
+void exynos_drm_fb_set_pending(struct drm_framebuffer *fb, bool pending);<br>
+<br>
+/* release a fb pended by page flip. */<br>
+void exynos_drm_release_pended_fb(struct drm_framebuffer *fb);<br>
+<br>
/* get memory information of a drm framebuffer */<br>
struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb,<br>
int index);<br>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c<br>
index e7466c4..62f3b9e 100644<br>
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c<br>
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c<br>
@@ -314,9 +314,18 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)<br>
void exynos_drm_fbdev_restore_mode(struct drm_device *dev)<br>
{<br>
struct exynos_drm_private *private = dev->dev_private;<br>
+ struct drm_framebuffer *fb, *fbt;<br>
<br>
if (!private || !private->fb_helper)<br>
return;<br>
<br>
drm_fb_helper_restore_fbdev_mode(private->fb_helper);<br>
+<br>
+ mutex_lock(&dev->mode_config.mutex);<br>
+<br>
+ /* Release fb pended by page flip. */<br>
+ list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head)<br>
+ exynos_drm_release_pended_fb(fb);<br>
+<br>
+ mutex_unlock(&dev->mode_config.mutex);<br>
}<br>
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c<br>
</div></div>index 862ca1e..81d7323 100644<br>
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c<br>
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c<br>
@@ -203,11 +203,29 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,<br>
<div class="im"> if (ret < 0)<br>
return ret;<br>
<br>
- plane->crtc = crtc;<br>
+ /*<br>
</div><div class="im">+ * Take a reference to new fb.<br>
+ *<br>
+ * Taking a reference means that this plane's dma is going to access<br>
+ * memory region to the new fb.<br>
+ */<br>
+ drm_framebuffer_reference(fb);<br>
<br>
+ plane->crtc = crtc;<br>
exynos_plane_commit(plane);<br>
exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);<br>
<br>
</div>+ /*<br>
+ * If plane->fb is existed, unreference the fb.<br>
<div class="im">+ *<br>
+ * This means that memory region to the fb isn't accessed by the dma<br>
+ * of this plane anymore.<br>
+ */<br>
+ if (plane->fb)<br>
+ drm_framebuffer_unreference(plane->fb);<br>
+<br>
</div>+ plane->fb = fb;<br>
+<br>
return 0;<br>
}<br>
<br>
@@ -215,6 +233,14 @@ static int exynos_disable_plane(struct drm_plane *plane)<br>
{<br>
DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);<br>
<br>
+ /*<br>
+ * Unreference the (current)fb if plane->fb is existed.<br>
+ * In exynos_update_plane(), the new fb reference count can be bigger<br>
+ * than 1. So it can't be removed for that reference count.<br>
<div class="im">+ */<br>
+ if (plane->fb)<br>
+ drm_framebuffer_unreference(plane->fb);<br>
+<br>
</div> exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF);<br>
<br>
return 0;<br>
<div class=""><div class="h5">--<br>
1.7.4.1<br>
<br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br></div>