Hi, On 27 April 2016 at 03:03, Dave Airlie <airlied@xxxxxxxxx> wrote: > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 66ca313..29b7835 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -456,6 +456,9 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) > * between them is henceforth no longer available. > */ > connector->dpms = DRM_MODE_DPMS_OFF; > + > + /* we keep a reference while the encoder is bound */ > + drm_connector_unreference(connector); > } > } > > @@ -635,9 +638,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > mode_changed = true; > /* If the encoder is reused for another connector, then > * the appropriate crtc will be set later. > + * take a reference only if we haven't had an encoder before. > */ > if (connector->encoder) > connector->encoder->crtc = NULL; > + else > + drm_connector_reference(connector); > connector->encoder = new_encoder; > } > } This new ref leaks in the if (fail) goto fail path below, and I can't quite convince myself that it's correct in the case where they share an encoder. How about this: - unconditionally ref every connector in set->connectors before doing anything which can fail - on successful exit, unconditionally unref every connector in save_connectors - in the fail label, unconditionally unref every connector in set->connectors Not quite as efficient as trying to do the only-ref-on-change dance, but much easier to prove correct, as well as matching the atomic state approach, if you imagine save_connectors as the current state, and set->connectors as the new/req state. Plus, refcounting really isn't the expensive part of this operation. ;) Cheers, Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
