[PATCH] drm: fix drm_framebuffer cleanup.

[Inki Dae]

2012/11/9 Prathyush K <prathyush at chromium.org>

>
> 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;
>
>
>
Right, thank you for comments. There is my missing point. how about adding
fb->crtc = NULL like below then?

int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file
*file_priv) {
        ....
        fb->crtc = NULL;
        fb->funcs->destroy(fb);
out:
        ...
}

With this, when user requested rmfb to remove fb[2], fb[2]'s crtc becomes
NULL so the fb would be freed without disabling crtc.

Thanks,
Inki Dae



>
>
>
>
>> 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
>>
>
>
> _______________________________________________
> 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/a7a753a7/attachment.html>