Hi
2012/12/5 Prathyush K <prathy...@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....@samsung.com> wrote:
>
>>
>>
>> 2012/12/5 Inki Dae <inki....@samsung.com>
>>
>>>
>>>
>>> 2012/12/4 Prathyush K <prathy...@chromium.org>
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Dec 4, 2012 at 5:44 PM, Inki Dae <inki....@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....@samsung.com>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.p...@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@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@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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
