So my original plan was that the drm core refcounts framebuffers like
with the legacy ioctls. But that doesn't work for a bunch of reasons:
- State objects might live longer than until the next fb change
happens for a plane. For example delayed cleanup work only happens
_after_ the pageflip ioctl has completed. So this definitely doesn't
work without the plane state holding its own refernces.
- The other issue is transition from legacy to atomic implementations,
where the driver works under a mix of both worlds. Which means
legacy paths might not properly update the ->fb pointer under
plane->state->fb. Which is a bit a problem when then someone comes
around and _does_ try to clean it up when it's long gone.
The second issue is just a bit a transition bug, since drivers should
update plane->state->fb in all the paths that aren't converted yet.
But a bit more robustness for the transition cant' hurt - we pull
similar tricks with cleaning up the old fb in the transitional helpers
already.
The pattern for drivers that transition is
if (plane->state)
drm_atomic_set_fb_for_plane(plane->state, plane->fb);
inserted after the fb update has logically completed at the end of
->set_config (or ->set_base/mode_set if using the crtc helpers),
->page_flip, ->update_plane or any other entry point which updates
plane->fb.
v2: Update kerneldoc - copypasta fail.
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
---
drivers/gpu/drm/drm_atomic.c | 28 ++++++++++++++++++++++++++++
drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++++++------
drivers/gpu/drm/drm_crtc_helper.c | 7 ++++---
drivers/gpu/drm/drm_plane_helper.c | 14 +++++++-------
include/drm/drm_atomic.h | 2 ++
5 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index bf3cdc2133e4..b71e9200b23d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -366,6 +366,34 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
EXPORT_SYMBOL(drm_atomic_set_crtc_for_plane);
/**
+ * drm_atomic_set_fb_for_plane - set crtc for plane
+ * @plane_state: atomic state object for the plane
+ * @fb: fb to use for the plane
+ *
+ * Changing the assigned framebuffer for a plane requires us to grab a reference
+ * to the new fb and drop the reference to the old fb, if there is one. This
+ * function takes care of all these details besides updating the pointer in the
+ * state object itself.
+ */
+void
+drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
+ struct drm_framebuffer *fb)
+{
+ if (plane_state->fb)
+ drm_framebuffer_unreference(plane_state->fb);
+ if (fb)
+ drm_framebuffer_reference(fb);
+ plane_state->fb = fb;
+
+ if (fb)
+ DRM_DEBUG_KMS("Set [FB:%d] for plane state %p\n",
+ fb->base.id, plane_state);
+ else
+ DRM_DEBUG_KMS("Set [NOFB] for plane state %p\n", plane_state);
+}
+EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
+
+/**
* drm_atomic_set_crtc_for_connector - set crtc for connector
* @conn_state: atomic state object for the connector
* @crtc: crtc to use for the connector
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index bbc41c9b3cfc..851cc6c04b22 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1175,7 +1175,7 @@ retry:
}
drm_atomic_set_crtc_for_plane(plane_state, crtc);
- plane_state->fb = fb;
+ drm_atomic_set_fb_for_plane(plane_state, fb);
plane_state->crtc_x = crtc_x;
plane_state->crtc_y = crtc_y;
plane_state->crtc_h = crtc_h;
@@ -1242,7 +1242,7 @@ retry:
}
drm_atomic_set_crtc_for_plane(plane_state, NULL);
- plane_state->fb = NULL;
+ drm_atomic_set_fb_for_plane(plane_state, NULL);
plane_state->crtc_x = 0;
plane_state->crtc_y = 0;
plane_state->crtc_h = 0;
@@ -1402,7 +1402,7 @@ retry:
}
drm_atomic_set_crtc_for_plane(primary_state, crtc);
- primary_state->fb = set->fb;
+ drm_atomic_set_fb_for_plane(primary_state, set->fb);
primary_state->crtc_x = 0;
primary_state->crtc_y = 0;
primary_state->crtc_h = set->mode->vdisplay;
@@ -1695,7 +1695,7 @@ retry:
}
drm_atomic_set_crtc_for_plane(plane_state, crtc);
- plane_state->fb = fb;
+ drm_atomic_set_fb_for_plane(plane_state, fb);
ret = drm_atomic_async_commit(state);
if (ret == -EDEADLK)
@@ -1809,6 +1809,9 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
*/
void drm_atomic_helper_plane_reset(struct drm_plane *plane)
{
+ if (plane->state && plane->state->fb)
+ drm_framebuffer_unreference(plane->state->fb);
+
kfree(plane->state);
plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
}
@@ -1824,10 +1827,17 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
struct drm_plane_state *
drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane)
{
+ struct drm_plane_state *state;
+
if (WARN_ON(!plane->state))
return NULL;
- return kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL);
+ state = kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL);
+
+ if (state && state->fb)
+ drm_framebuffer_reference(state->fb);
+
+ return state;
}
EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
@@ -1840,8 +1850,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
* subclassed plane state structure.
*/
void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
- struct drm_plane_state *state)
+ struct drm_plane_state *state)
{
+ if (state->fb)
+ drm_framebuffer_unreference(state->fb);
+
kfree(state);
}
EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 33195e9adaab..d552708409de 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -34,11 +34,13 @@
#include <linux/moduleparam.h>
#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_crtc.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_crtc_helper.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_edid.h>
/**
@@ -998,15 +1000,14 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
if (plane->funcs->atomic_duplicate_state)
plane_state = plane->funcs->atomic_duplicate_state(plane);
else if (plane->state)
- plane_state = kmemdup(plane->state, sizeof(*plane_state),
- GFP_KERNEL);
+ plane_state = drm_atomic_helper_plane_duplicate_state(plane);
else
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
if (!plane_state)
return -ENOMEM;
plane_state->crtc = crtc;
- plane_state->fb = crtc->primary->fb;
+ drm_atomic_set_fb_for_plane(plane_state, crtc->primary->fb);
plane_state->crtc_x = 0;
plane_state->crtc_y = 0;
plane_state->crtc_h = crtc->mode.vdisplay;
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index df69522b1f0e..497104df112b 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -27,7 +27,9 @@
#include <drm/drmP.h>
#include <drm/drm_plane_helper.h>
#include <drm/drm_rect.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
#define SUBPIXEL_MASK 0xffff
@@ -462,7 +464,7 @@ fail:
if (plane->funcs->atomic_destroy_state)
plane->funcs->atomic_destroy_state(plane, plane_state);
else
- kfree(plane_state);
+ drm_atomic_helper_plane_destroy_state(plane, plane_state);
}
return ret;
@@ -503,15 +505,14 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
if (plane->funcs->atomic_duplicate_state)
plane_state = plane->funcs->atomic_duplicate_state(plane);
else if (plane->state)
- plane_state = kmemdup(plane->state, sizeof(*plane_state),
- GFP_KERNEL);
+ plane_state = drm_atomic_helper_plane_duplicate_state(plane);
else
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
if (!plane_state)
return -ENOMEM;
plane_state->crtc = crtc;
- plane_state->fb = fb;
+ drm_atomic_set_fb_for_plane(plane_state, fb);
plane_state->crtc_x = crtc_x;
plane_state->crtc_y = crtc_y;
plane_state->crtc_h = crtc_h;
@@ -550,15 +551,14 @@ int drm_plane_helper_disable(struct drm_plane *plane)
if (plane->funcs->atomic_duplicate_state)
plane_state = plane->funcs->atomic_duplicate_state(plane);
else if (plane->state)
- plane_state = kmemdup(plane->state, sizeof(*plane_state),
- GFP_KERNEL);
+ plane_state = drm_atomic_helper_plane_duplicate_state(plane);
else
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
if (!plane_state)
return -ENOMEM;
plane_state->crtc = NULL;
- plane_state->fb = NULL;
+ drm_atomic_set_fb_for_plane(plane_state, NULL);
return drm_plane_helper_commit(plane, plane_state, plane->fb);
}
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 753812034e71..52d92981209f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -45,6 +45,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
int drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
struct drm_crtc *crtc);
+void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
+ struct drm_framebuffer *fb);
int drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
struct drm_crtc *crtc);
int
--
2.1.1
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
