[PATCH v2] drm/exynos: release fb pended by page flip
Prathyush K
prathyush at chromium.org
Tue Dec 4 04:58:06 PST 2012
On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae <inki.dae at 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 at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at 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);
> +
>
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 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;
> --
> 1.7.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121204/5d0cc13a/attachment-0001.html>
More information about the dri-devel
mailing list