[PATCH v2] drm/exynos: release fb pended by page flip

Inki Dae inki.dae at samsung.com
Tue Dec 4 19:46:01 PST 2012


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>


More information about the dri-devel mailing list