[Intel-gfx] [PATCH 3/5] drm/crtc: take references to connectors used in a modeset.

[Daniel Stone]

Hi,

On 27 April 2016 at 03:03, Dave Airlie <airlied at gmail.com> 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