[PATCH 13/15] drm: take references to connectors used in a modeset.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:44PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> As suggested by Daniel, if we are actively using the connector in a modeset
> we don't want it to disappear from underneath us. This takes a reference
> to the connector in the atomic paths when we are setting the state up,
> and in the non-atomic paths when binding the encoder.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  drivers/gpu/drm/drm_atomic.c      | 11 ++++++++++-
>  drivers/gpu/drm/drm_crtc_helper.c |  6 ++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9d5e3c8..d899dac 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1159,7 +1159,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  				  struct drm_crtc *crtc)
>  {
>  	struct drm_crtc_state *crtc_state;
> -
> +	bool had_crtc = conn_state->crtc ? true : false;
>  	if (conn_state->crtc && conn_state->crtc != crtc) {
>  		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
>  								conn_state->crtc);
> @@ -1179,6 +1179,15 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  
>  	conn_state->crtc = crtc;
>  
> +	/* If we had no crtc then got one, add a reference,
> +	 * if we had a crtc and are going to none, drop a reference,
> +	 * otherwise just keep the reference we have.
> +	 */
> +	if (!had_crtc && crtc)
> +		drm_connector_reference(conn_state->connector);
> +	else if (!crtc && had_crtc)
> +		 drm_connector_unreference(conn_state->connector);
> +
>  	if (crtc)
>  		DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
>  				 conn_state, crtc->base.id, crtc->name);

I'm super paranoid about the refcounting for modesets, since at least with
fbs that was where we had endless amounts of pain and random bugs.

- I think we should split this patch into atomic and legacy parts, since
  the code-paths are completely different.

- I'm not sure on the atomic version. I think conceptually it would be
  even cleaner for atomic to say that drm_connector_state holds a ref on
  the connector iff the crtc pointer is set. This would mean we grab the
  reference also in duplicate_state and drop it in destroy_state (and ofc
  update it in set_crtc_for_connector). It's a bit funny semantics, but by
  attaching the refcounting to the lifetime of the state struct we fully
  align with how refcounting is done for everything else in atomic. And
  avoiding special cases in refcounting is good imo.

  Also since we know that the state structures are tracked correctly I
  wouldn't have to think about correctness at all, makes the review easier
  ;-)

Cheers, Daniel

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 79555d2..71b6c72 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;
>  		}
>  	}
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch