2012/11/9 Rob Clark <robdclark@xxxxxxxxx>
On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:btw, this should instead be done by holding a ref to the GEM
> 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.
object(s).. or these days you can increment the reference count on
the fb and let the fb hold ref's to the GEM object(s) (which makes it
a bit easier to deal with multi-planar formats)
Rob, let's discuss that again after you read my latest comment. Please, see my latest comment.
Thanks,
Inki Dae
BR,
-R
> 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.
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
