2012/11/9 Prathyush K <prathy...@chromium.org>
>
> On Fri, Nov 9, 2012 at 1:09 PM, Inki Dae <inki....@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....@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.p...@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@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
