On Wed, Aug 30, 2017 at 05:10:43PM +0300, Laurent Pinchart wrote:
> Hi Maarten,
>
> Thank you for the patch.
>
> On Wednesday, 30 August 2017 15:17:51 EEST Maarten Lankhorst wrote:
> > Currently we neatly track the crtc state, but forget to look at
> > plane/connector state.
> >
> > When doing a nonblocking modeset, immediately followed by a setprop
> > before the modeset completes, the setprop will see the modesets new
> > state as the old state and free it.
> >
> > This has to be solved by waiting for hw_done on the connector, even
> > if it's not assigned to a crtc. When a connector is unbound we take
> > the last crtc commit, and when it stays unbound we create a new
> > fake crtc commit for that gets signaled on hw_done for all the
> > planes/connectors.
> >
> > We wait for it the same way as we do for crtc's, which will make
> > sure we never run into a use-after-free situation.
> >
> > Changes since v1:
> > - Only create a single disable commit. (danvet)
> > - Fix leak in intel_legacy_cursor_update.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 4 +
> > drivers/gpu/drm/drm_atomic_helper.c | 156 ++++++++++++++++++++++++++++++--
> > drivers/gpu/drm/i915/intel_display.c | 2 +
> > include/drm/drm_atomic.h | 12 +++
> > include/drm/drm_connector.h | 7 ++
> > include/drm/drm_plane.h | 7 ++
> > 6 files changed, 182 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 2cce48f203e0..75f5f74de9bf 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct
> > drm_atomic_state *state) }
> > state->num_private_objs = 0;
> >
> > + if (state->fake_commit) {
> > + drm_crtc_commit_put(state->fake_commit);
> > + state->fake_commit = NULL;
> > + }
> > }
> > EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index 8ccb8b6536c0..034f563fb130
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1644,6 +1644,40 @@ static void release_crtc_commit(struct completion
> > *completion) drm_crtc_commit_put(commit);
> > }
> >
> > +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc
> > *crtc)
> > +{
>
> You could allocate the commit in this function too, the kzalloc() is currently
> duplicated. The function should probably be called alloc_commit() then.
>
> > + init_completion(&commit->flip_done);
> > + init_completion(&commit->hw_done);
> > + init_completion(&commit->cleanup_done);
> > + INIT_LIST_HEAD(&commit->commit_entry);
> > + kref_init(&commit->ref);
> > + commit->crtc = crtc;
> > +}
> > +
> > +static struct drm_crtc_commit *
> > +fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> > +{
> > + struct drm_crtc_commit *commit;
> > +
> > + if (crtc) {
> > + struct drm_crtc_state *new_crtc_state;
> > +
> > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > +
> > + commit = new_crtc_state->commit;
> > + } else if (!state->fake_commit) {
> > + state->fake_commit = commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> > + if (!commit)
> > + return NULL;
> > +
> > + init_commit(commit, NULL);
> > + } else
> > + commit = state->fake_commit;
> > +
> > + drm_crtc_commit_get(commit);
>
> I believe the reference counting is right. The double reference in the second
> case (kref_init() when initializing the commit and drm_crtc_commit_get())
> should not cause a leak. The kref_init() takes a reference to store the commit
> in state->fake_commit, released in drm_atomic_state_default_clear(), and the
> drm_crtc_commit_get() takes a reference returned by the function, stored in
> new_*_state->commit by the caller.
>
> This being said, I think the reference counting is confusing, as proved by
> Daniel thinking there was a leak here (or by me thinking there's no leak while
> there's one :-)). To make the implementation clearer, I propose turning the
> definition of drm_crtc_commit_get() to
>
> static inline struct drm_crtc_commit *
> drm_crtc_commit_get(struct drm_crtc_commit *commit)
> {
> kref_get(&commit->ref);
> return commit;
> }
>
> and writing this function as
>
> /* Return a new reference to the commit object */
> static struct drm_crtc_commit *
> fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> {
> struct drm_crtc_commit *commit;
>
> if (crtc) {
> struct drm_crtc_state *new_crtc_state;
>
> new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> commit = new_crtc_state->commit;
> } else {
> if (!state->fake_commit)
> state->fake_commit = alloc_commit(NULL);
>
> commit = state->fake_commit;
> }
>
> return drm_crtc_commit_get(commit);
> }
+1 on something like this, the compressed layout with assigning both
commit and ->fake_commit is what tricked me into seeing a leak.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
