[PATCH v2] drm/exynos: release fb pended by page flip
Prathyush K
prathyush at chromium.org
Tue Dec 4 22:14:48 PST 2012
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 at 4:1280x720.
Regards,
Prathyush
On Wed, Dec 5, 2012 at 9:16 AM, Inki Dae <inki.dae at samsung.com> wrote:
>
>
> 2012/12/5 Inki Dae <inki.dae at samsung.com>
>
>>
>>
>> 2012/12/4 Prathyush K <prathyush at chromium.org>
>>
>>>
>>>
>>>
>>> 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.
>>>
>>>
>> 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 at 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)
>> 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
>>>> 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
>>>>
>>>
>>>
>>> _______________________________________________
>>> 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/20121205/fc6374b9/attachment-0001.html>
More information about the dri-devel
mailing list