drm/omap: Take a fb reference in omap_plane_update()
Subsystems (3)
PATCH 1/1 drm/omap: Take a fb reference in omap_plane_update()
When userspace calls SET_PLANE ioctl, drm core takes a reference of the fb and passes control to the update_plane op defined by the drm driver.
In omapdrm, we have a worker thread which queues framebuffers objects received from update_plane and displays them at the appropriate time.
It is possible that the framebuffer is destoryed by userspace between the time of calling the ioctl and apply-worker being scheduled. If this happens, the apply-worker holds a pointer to a framebuffer which is already destroyed.
Take an extra refernece/unreference of the fb in omap_plane_update() to prevent this from happening. A reference is taken of the fb passed to update_plane(), the previous framebuffer (held by plane->fb) is unreferenced. This will prevent drm from destroying the framebuffer till the time it's unreferenced by the apply-worker.
This is in addition to the exisitng reference/unreference in update_pin(), which is taken for the scanout of the plane's current framebuffer, and an unreference the previous framebuffer.
--- drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/gpu/drm/omapdrm/omap_plane.c +6 -0
@@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, {struct omap_plane *omap_plane =to_omap_plane (plane); omap_plane->enabled = true; + +if (plane->fb) +drm_framebuffer_unreference (plane->fb); + +drm_framebuffer_reference (fb); + return omap_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h,
Signature · 1 line
1.7.10.4
Quoted 16 lines · Archit Taneja
When userspace calls
SET_PLANEioctl, drm core takes a reference of the fb and passes control to theupdate_planeop defined by the drm driver.In omapdrm, we have a worker thread which queues framebuffers objects received from
update_planeand displays them at the appropriate time.It is possible that the framebuffer is destoryed by userspace between the time of calling the ioctl and apply-worker being scheduled. If this happens, the apply-worker holds a pointer to a framebuffer which is already destroyed.
Take an extra refernece/unreference of the fb in
omap_plane_update()to prevent this from happening. A reference is taken of the fb passed toupdate_plane(), the previous framebuffer (held byplane->fb) is unreferenced. This will prevent drm from destroying the framebuffer till the time it's unreferenced by the apply-worker.
Quoted 6 lines
This is in addition to the exisitng reference/unreference in
update_pin(), which is taken for the scanout of the plane's current framebuffer, and an unreference the previous framebuffer.Signed-off-by Archit Taneja <[email protected]>Cc Rob Clark <[email protected]>
Good catch
Quoted 24 lines
--- drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/gpu/drm/omapdrm/omap_plane.c +6
-1@@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, {struct omap_plane *omap_plane =to_omap_plane (plane); omap_plane->enabled = true; + +if (plane->fb) +drm_framebuffer_unreference (plane->fb); + +drm_framebuffer_reference (fb); + return omap_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, --1.7.10.4
Quoted 31 lines · Archit Taneja
When userspace calls
SET_PLANEioctl, drm core takes a reference of the fb and passes control to theupdate_planeop defined by the drm driver.In omapdrm, we have a worker thread which queues framebuffers objects received from
update_planeand displays them at the appropriate time.It is possible that the framebuffer is destoryed by userspace between the time of calling the ioctl and apply-worker being scheduled. If this happens, the apply-worker holds a pointer to a framebuffer which is already destroyed.
Take an extra refernece/unreference of the fb in
omap_plane_update()to prevent this from happening. A reference is taken of the fb passed toupdate_plane(), the previous framebuffer (held byplane->fb) is unreferenced. This will prevent drm from destroying the framebuffer till the time it's unreferenced by the apply-worker.This is in addition to the exisitng reference/unreference in
update_pin(), which is taken for the scanout of the plane's current framebuffer, and an unreference the previous framebuffer.Signed-off-by Archit Taneja <[email protected]>Cc Rob Clark <[email protected]>--- drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/gpu/drm/omapdrm/omap_plane.c +0
-0
drivers/gpu/drm/omapdrm/omap_plane.c static int omap_plane_update(struct drm_plane *plane, · line 252
Quoted 6 lines
drivers/gpu/drm/omapdrm/omap_plane.c +3
-0@@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, {struct omap_plane *omap_plane =to_omap_plane (plane); omap_plane->enabled = true; + +if (plane->fb) +drm_framebuffer_unreference (plane->fb);
Shouldn't the unref only happen once the flip has completed? Otherwise we might free the memory which is still being scanned out and put some other crap there.
Note that I've been too lazy to read the code, but I expected the unref of the old one to happen after the ref-taking for the new one, with a vblank wait/delay in between ... Might be that I'm completely off ;-) -Daniel
Quoted 13 lines
drivers/gpu/drm/omapdrm/omap_plane.c +3
-0+ +drm_framebuffer_reference (fb); + return omap_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h,Signature · 6 lines
1.7.10.4 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 27 lines · Daniel Vetter
Quoted 26 lines · Archit Taneja
When userspace calls
SET_PLANEioctl, drm core takes a reference of the fb and passes control to theupdate_planeop defined by the drm driver.In omapdrm, we have a worker thread which queues framebuffers objects received from
update_planeand displays them at the appropriate time.It is possible that the framebuffer is destoryed by userspace between the time of calling the ioctl and apply-worker being scheduled. If this happens, the apply-worker holds a pointer to a framebuffer which is already destroyed.
Take an extra refernece/unreference of the fb in
omap_plane_update()to prevent this from happening. A reference is taken of the fb passed toupdate_plane(), the previous framebuffer (held byplane->fb) is unreferenced. This will prevent drm from destroying the framebuffer till the time it's unreferenced by the apply-worker.This is in addition to the exisitng reference/unreference in
update_pin(), which is taken for the scanout of the plane's current framebuffer, and an unreference the previous framebuffer.Signed-off-by Archit Taneja <[email protected]>Cc Rob Clark <[email protected]>--- drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++ 1 file changed, 6 insertions(+)
Quoted 15 lines
drivers/gpu/drm/omapdrm/omap_plane.c static int omap_plane_update(struct drm_plane *plane, · line 252
Quoted 11 lines
drivers/gpu/drm/omapdrm/omap_plane.c +3
-0@@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, {struct omap_plane *omap_plane =to_omap_plane (plane); omap_plane->enabled = true; + +if (plane->fb) +drm_framebuffer_unreference (plane->fb);Shouldn't the unref only happen once the flip has completed? Otherwise we might free the memory which is still being scanned out and put some other crap there.
yup, there is a 2nd ref grabbed when we start scanout and dropped when we finish scanout.. so that part was already covered. What wasn't covered before was the time between the ioctl and the worker thread (which was grabbing/dropping the scanout ref)
BR,
-R
Quoted 23 lines
Note that I've been too lazy to read the code, but I expected the unref of the old one to happen after the ref-taking for the new one, with a vblank wait/delay in between ... Might be that I'm completely off ;-) -Daniel
Quoted 9 lines
drivers/gpu/drm/omapdrm/omap_plane.c +3
-1+ +drm_framebuffer_reference (fb); + return omap_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, --1.7.10.4
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 16 lines · Rob Clark
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/omapdrm/omap_plane.c +0
-0@@ -247,6 +247,12 @@ static int omap_plane_update(struct drm_plane *plane, {struct omap_plane *omap_plane =to_omap_plane (plane);Quoted 8 lines
drivers/gpu/drm/omapdrm/omap_plane.c static int omap_plane_update(struct drm_plane *plane, · line 252
Quoted 4 lines
drivers/gpu/drm/omapdrm/omap_plane.c +3
-0omap_plane->enabled = true; + +if (plane->fb) +drm_framebuffer_unreference (plane->fb);Shouldn't the unref only happen once the flip has completed? Otherwise we might free the memory which is still being scanned out and put some other crap there.
Quoted 4 lines
yup, there is a 2nd ref grabbed when we start scanout and dropped when we finish scanout.. so that part was already covered. What wasn't covered before was the time between the ioctl and the worker thread (which was grabbing/dropping the scanout ref)
Ah, I see. And the ordering doesn't seem to matter here since it's all protected by locks (against races with the worker thread) anyway. Thanks for the explanation.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch