Re: [PATCH 17/17] drm/atomic: Refcounting for plane_state->fb

On Sun, Nov 02, 2014 at 02:19:30PM +0100, Daniel Vetter wrote:

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.

s/refernces/references/

• 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

s/cant'/can't/

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.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++ 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, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index c6db8a48cad6..af34321b675d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -366,6 +366,33 @@ 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 crtc for a plane requires us to grab the lock and state
• • for the new crtc, as needed. 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 d0ca681d6326..a5de60faedff 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel