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/cae8647f/attachment-0001.html>
