[PATCH] drm: fix drm_framebuffer cleanup.

[Prathyush K]

On Fri, Nov 9, 2012 at 1:09 PM, Inki Dae <inki.dae at samsung.com> wrote:

> This patch fixes access issue to invalid memory region.
>
> crtc had only one drm_framebuffer object so when framebuffer
> cleanup was requested after page flip, it'd try to disable
> hardware overlay to current crtc.
> But if current crtc points to another drm_framebuffer,
> This may induce invalid memory access.
>
> Assume that some application are doing page flip with two
> drm_framebuffer objects. At this time, if second drm_framebuffer
> is set to current crtc and the cleanup to first drm_framebuffer
> is requested once drm release is requested, then first
> drm_framebuffer would be cleaned up without disabling
> hardware overlay because current crtc points to second
> drm_framebuffer. After that, gem buffer to first drm_framebuffer
> would be released and this makes dma access invalid memory region.
>


I am confused regarding this. If crtc points to second frame buffer, then
the dma is accessing the memory region
of the second framebuffer. Why can't we free the first framebuffer and its
gem buffer?

With this patch, there is no way to free a framebuffer (which has been set
to a crtc), without disabling
the crtc.

Consider this example:
I have three framebuffers (0, 1, 2) and I set them to a crtc A one by one.
So with your patch,
fb[0]->crtc = A
fb[1]->crtc = A
fb[2]->crtc = A

After this, I am using only framebuffer 0 and 1 i.e. 0 and 1 are being page
flipped.
Now, I want to remove framebuffer 2.
fb[2]->crtc = A .. so while removing the framebuffer, we will end up
disabling the crtc
which is not correct.

I think, there should be an additional interface to unset a fb to a crtc.
That way, we can
reset the crtc inside a framebuffer so that it can be freed if not in use.
i.e. fb[2]->crtc = NULL;






> This patch adds drm_framebuffer to drm_crtc structure as member
> and makes drm_framebuffer_cleanup function check if fb->crtc is
> same as desired fb. And also when setcrtc and pageflip are
> requested, it makes each drm_framebuffer point to current crtc.
>
> Signed-off-by: Inki Dae <inki.dae at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    7 ++++---
>  include/drm/drm_crtc.h     |    1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ef1b221..5c04bd4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>
>         /* remove from any CRTC */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -               if (crtc->fb == fb) {
> +               if (fb->crtc == crtc) {
>                         /* should turn off the crtc */
>                         memset(&set, 0, sizeof(struct drm_mode_set));
>                         set.crtc = crtc;
> @@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void
> *data,
>         set.mode = mode;
>         set.connectors = connector_set;
>         set.num_connectors = crtc_req->count_connectors;
> +       fb->crtc = crtc;
>         set.fb = fb;
>         ret = crtc->funcs->set_config(&set);
>
> @@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>                         spin_unlock_irqrestore(&dev->event_lock, flags);
>                         kfree(e);
>                 }
> -       }
> -
> +       } else
> +               fb->crtc = crtc;
>  out:
>         mutex_unlock(&dev->mode_config.mutex);
>         return ret;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3fa18b7..92889be 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -256,6 +256,7 @@ struct drm_framebuffer {
>         struct kref refcount;
>         struct list_head head;
>         struct drm_mode_object base;
> +       struct drm_crtc *crtc;
>         const struct drm_framebuffer_funcs *funcs;
>         unsigned int pitches[4];
>         unsigned int offsets[4];
> --
> 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/20121109/2a0513a6/attachment-0001.html>