| Message ID | 1462249706-4759-5-git-send-email-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 03, 2016 at 02:28:25PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > Take a reference when setting a crtc on a connecter, > also take one when duplicating if a crtc is set, > and drop one on destroy if a crtc is set. > > v2: take Daniel Stone's advice and simplify the > ref/unref dances, also take care of NULL as connector > to state reset. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > --- > drivers/gpu/drm/drm_atomic.c | 4 ++++ > drivers/gpu/drm/drm_atomic_helper.c | 5 +++++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 9d5e3c8..91cae88 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1160,6 +1160,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > { > struct drm_crtc_state *crtc_state; > > + if (crtc) > + drm_connector_reference(conn_state->connector); > if (conn_state->crtc && conn_state->crtc != crtc) { > crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state, > conn_state->crtc); > @@ -1177,6 +1179,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > 1 << drm_connector_index(conn_state->connector); > } > > + if (conn_state->crtc) > + drm_connector_unreference(conn_state->connector); > conn_state->crtc = crtc; > > if (crtc) > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index d25abce..f3e2642 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > struct drm_connector_state *state) > { > memcpy(state, connector->state, sizeof(*state)); > + if (state->crtc) > + drm_connector_reference(connector); > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); > > @@ -2889,6 +2891,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, > * state will automatically do the right thing if code is ever added > * to this function. > */ > + if (connector && state->crtc) { > + drm_connector_unreference(state->connector); > + } Bikeshed: no need for {} and you don't need to check that connector is NULL either. Tbh all the destroy_state helpers don't really need their object argument, only the state. Cleaning that up is somewhere on my todo. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); > > -- > 2.5.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> */ >> + if (connector && state->crtc) { >> + drm_connector_unreference(state->connector); >> + } > > Bikeshed: no need for {} and you don't need to check that connector is > NULL either. Tbh all the destroy_state helpers don't really need their > object argument, only the state. Cleaning that up is somewhere on my todo. Yes you really do need the connector, lost a boot to an oops, drm_atomic_state_default_clear does some funky stuff that can possibly go away after this, but I'm not sure. Dave.
On Tue, May 3, 2016 at 12:09 PM, Dave Airlie <airlied@gmail.com> wrote: >>> */ >>> + if (connector && state->crtc) { >>> + drm_connector_unreference(state->connector); >>> + } >> >> Bikeshed: no need for {} and you don't need to check that connector is >> NULL either. Tbh all the destroy_state helpers don't really need their >> object argument, only the state. Cleaning that up is somewhere on my todo. > > Yes you really do need the connector, lost a boot to an oops, > > drm_atomic_state_default_clear does some funky stuff that can possibly go away > after this, but I'm not sure. Surprising, didn't expect that. Do you still have the backtrace somewhere? I really wonder what goes boom with that one ... What we might need is check for state->connector (instead of connector), but that being NULL for a valid state is also a bit a bug. -Daniel
Hi, On 3 May 2016 at 05:28, Dave Airlie <airlied@gmail.com> wrote: > From: Dave Airlie <airlied@redhat.com> > > Take a reference when setting a crtc on a connecter, > also take one when duplicating if a crtc is set, > and drop one on destroy if a crtc is set. > > v2: take Daniel Stone's advice and simplify the > ref/unref dances, also take care of NULL as connector > to state reset. > > Signed-off-by: Dave Airlie <airlied@redhat.com> I expect we'll still end up with all kinds of comedy races here, but on the grounds that it's unambiguously an improvement on what came before, and I can't see anything obviously wrong, for patches 1-5: Reviewed-by: Daniel Stone <daniels@collabora.com> Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9d5e3c8..91cae88 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1160,6 +1160,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, { struct drm_crtc_state *crtc_state; + if (crtc) + drm_connector_reference(conn_state->connector); if (conn_state->crtc && conn_state->crtc != crtc) { crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state, conn_state->crtc); @@ -1177,6 +1179,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, 1 << drm_connector_index(conn_state->connector); } + if (conn_state->crtc) + drm_connector_unreference(conn_state->connector); conn_state->crtc = crtc; if (crtc) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d25abce..f3e2642 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2762,6 +2762,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, struct drm_connector_state *state) { memcpy(state, connector->state, sizeof(*state)); + if (state->crtc) + drm_connector_reference(connector); } EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); @@ -2889,6 +2891,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, * state will automatically do the right thing if code is ever added * to this function. */ + if (connector && state->crtc) { + drm_connector_unreference(state->connector); + } } EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);