Skip to content

Commit 321ebf0

Browse files
danvetdanvet
committed
drm/atomic: Refcounting for plane_state->fb
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 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 can't 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. v3: Fix spelling in the commit message (Sean). Reviewed-by: Sean Paul <seanpaul@chromium.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
1 parent 3150c7d commit 321ebf0

File tree

5 files changed

+60
-16
lines changed

5 files changed

+60
-16
lines changed

‎drivers/gpu/drm/drm_atomic.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,34 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
367367
}
368368
EXPORT_SYMBOL(drm_atomic_set_crtc_for_plane);
369369

370+
/**
371+
* drm_atomic_set_fb_for_plane - set crtc for plane
372+
* @plane_state: atomic state object for the plane
373+
* @fb: fb to use for the plane
374+
*
375+
* Changing the assigned framebuffer for a plane requires us to grab a reference
376+
* to the new fb and drop the reference to the old fb, if there is one. This
377+
* function takes care of all these details besides updating the pointer in the
378+
* state object itself.
379+
*/
380+
void
381+
drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
382+
struct drm_framebuffer *fb)
383+
{
384+
if (plane_state->fb)
385+
drm_framebuffer_unreference(plane_state->fb);
386+
if (fb)
387+
drm_framebuffer_reference(fb);
388+
plane_state->fb = fb;
389+
390+
if (fb)
391+
DRM_DEBUG_KMS("Set [FB:%d] for plane state %p\n",
392+
fb->base.id, plane_state);
393+
else
394+
DRM_DEBUG_KMS("Set [NOFB] for plane state %p\n", plane_state);
395+
}
396+
EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
397+
370398
/**
371399
* drm_atomic_set_crtc_for_connector - set crtc for connector
372400
* @conn_state: atomic state object for the connector

‎drivers/gpu/drm/drm_atomic_helper.c

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,7 +1182,7 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
11821182
ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
11831183
if (ret != 0)
11841184
goto fail;
1185-
plane_state->fb = fb;
1185+
drm_atomic_set_fb_for_plane(plane_state, fb);
11861186
plane_state->crtc_x = crtc_x;
11871187
plane_state->crtc_y = crtc_y;
11881188
plane_state->crtc_h = crtc_h;
@@ -1250,7 +1250,7 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane)
12501250
ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
12511251
if (ret != 0)
12521252
goto fail;
1253-
plane_state->fb = NULL;
1253+
drm_atomic_set_fb_for_plane(plane_state, NULL);
12541254
plane_state->crtc_x = 0;
12551255
plane_state->crtc_y = 0;
12561256
plane_state->crtc_h = 0;
@@ -1422,7 +1422,7 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
14221422
ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
14231423
if (ret != 0)
14241424
goto fail;
1425-
primary_state->fb = set->fb;
1425+
drm_atomic_set_fb_for_plane(primary_state, set->fb);
14261426
primary_state->crtc_x = 0;
14271427
primary_state->crtc_y = 0;
14281428
primary_state->crtc_h = set->mode->vdisplay;
@@ -1694,7 +1694,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
16941694
ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
16951695
if (ret != 0)
16961696
goto fail;
1697-
plane_state->fb = fb;
1697+
drm_atomic_set_fb_for_plane(plane_state, fb);
16981698

16991699
ret = drm_atomic_async_commit(state);
17001700
if (ret != 0)
@@ -1808,6 +1808,9 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
18081808
*/
18091809
void drm_atomic_helper_plane_reset(struct drm_plane *plane)
18101810
{
1811+
if (plane->state && plane->state->fb)
1812+
drm_framebuffer_unreference(plane->state->fb);
1813+
18111814
kfree(plane->state);
18121815
plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
18131816
}
@@ -1823,10 +1826,17 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
18231826
struct drm_plane_state *
18241827
drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane)
18251828
{
1829+
struct drm_plane_state *state;
1830+
18261831
if (WARN_ON(!plane->state))
18271832
return NULL;
18281833

1829-
return kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL);
1834+
state = kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL);
1835+
1836+
if (state && state->fb)
1837+
drm_framebuffer_reference(state->fb);
1838+
1839+
return state;
18301840
}
18311841
EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
18321842

@@ -1839,8 +1849,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
18391849
* subclassed plane state structure.
18401850
*/
18411851
void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
1842-
struct drm_plane_state *state)
1852+
struct drm_plane_state *state)
18431853
{
1854+
if (state->fb)
1855+
drm_framebuffer_unreference(state->fb);
1856+
18441857
kfree(state);
18451858
}
18461859
EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);

‎drivers/gpu/drm/drm_crtc_helper.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,13 @@
3434
#include <linux/moduleparam.h>
3535

3636
#include <drm/drmP.h>
37+
#include <drm/drm_atomic.h>
3738
#include <drm/drm_crtc.h>
3839
#include <drm/drm_fourcc.h>
3940
#include <drm/drm_crtc_helper.h>
4041
#include <drm/drm_fb_helper.h>
4142
#include <drm/drm_plane_helper.h>
43+
#include <drm/drm_atomic_helper.h>
4244
#include <drm/drm_edid.h>
4345

4446
/**
@@ -998,15 +1000,14 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
9981000
if (plane->funcs->atomic_duplicate_state)
9991001
plane_state = plane->funcs->atomic_duplicate_state(plane);
10001002
else if (plane->state)
1001-
plane_state = kmemdup(plane->state, sizeof(*plane_state),
1002-
GFP_KERNEL);
1003+
plane_state = drm_atomic_helper_plane_duplicate_state(plane);
10031004
else
10041005
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
10051006
if (!plane_state)
10061007
return -ENOMEM;
10071008

10081009
plane_state->crtc = crtc;
1009-
plane_state->fb = crtc->primary->fb;
1010+
drm_atomic_set_fb_for_plane(plane_state, crtc->primary->fb);
10101011
plane_state->crtc_x = 0;
10111012
plane_state->crtc_y = 0;
10121013
plane_state->crtc_h = crtc->mode.vdisplay;

‎drivers/gpu/drm/drm_plane_helper.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@
2727
#include <drm/drmP.h>
2828
#include <drm/drm_plane_helper.h>
2929
#include <drm/drm_rect.h>
30+
#include <drm/drm_atomic.h>
3031
#include <drm/drm_crtc_helper.h>
32+
#include <drm/drm_atomic_helper.h>
3133

3234
#define SUBPIXEL_MASK 0xffff
3335

@@ -464,7 +466,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
464466
if (plane->funcs->atomic_destroy_state)
465467
plane->funcs->atomic_destroy_state(plane, plane_state);
466468
else
467-
kfree(plane_state);
469+
drm_atomic_helper_plane_destroy_state(plane, plane_state);
468470
}
469471

470472
return ret;
@@ -505,15 +507,14 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
505507
if (plane->funcs->atomic_duplicate_state)
506508
plane_state = plane->funcs->atomic_duplicate_state(plane);
507509
else if (plane->state)
508-
plane_state = kmemdup(plane->state, sizeof(*plane_state),
509-
GFP_KERNEL);
510+
plane_state = drm_atomic_helper_plane_duplicate_state(plane);
510511
else
511512
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
512513
if (!plane_state)
513514
return -ENOMEM;
514515

515516
plane_state->crtc = crtc;
516-
plane_state->fb = fb;
517+
drm_atomic_set_fb_for_plane(plane_state, fb);
517518
plane_state->crtc_x = crtc_x;
518519
plane_state->crtc_y = crtc_y;
519520
plane_state->crtc_h = crtc_h;
@@ -552,15 +553,14 @@ int drm_plane_helper_disable(struct drm_plane *plane)
552553
if (plane->funcs->atomic_duplicate_state)
553554
plane_state = plane->funcs->atomic_duplicate_state(plane);
554555
else if (plane->state)
555-
plane_state = kmemdup(plane->state, sizeof(*plane_state),
556-
GFP_KERNEL);
556+
plane_state = drm_atomic_helper_plane_duplicate_state(plane);
557557
else
558558
plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
559559
if (!plane_state)
560560
return -ENOMEM;
561561

562562
plane_state->crtc = NULL;
563-
plane_state->fb = NULL;
563+
drm_atomic_set_fb_for_plane(plane_state, NULL);
564564

565565
return drm_plane_helper_commit(plane, plane_state, plane->fb);
566566
}

‎include/drm/drm_atomic.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
4646
int __must_check
4747
drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
4848
struct drm_crtc *crtc);
49+
void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
50+
struct drm_framebuffer *fb);
4951
int __must_check
5052
drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
5153
struct drm_crtc *crtc);

0 commit comments

Comments
 (0)