[RFC PATCH 17/37] DRM: Atomic: Use pointer for mode in CRTC state

[Daniel Vetter]

On Thu, Mar 19, 2015 at 04:33:16AM +0000, Daniel Stone wrote:
> Holding a pointer to the mode, rather than an embed, allows us to get
> towards sharing refcounted modes.
> 
> XXX: atomic_destroy_state does _not_ seem to be optional - so we should
>      remove any fallback paths which compensate for its lack!
>      the crtc_state->mode handling is particularly ugly here :\

duplicate/destroy callbacks are optional in the transitional helpers. And
that's fairly intentional to avoid the need for switching to the full
state scaffolding at once.

For these couldn't we instead just store a pointer to crtc->mode instead?
Lifetimes should be fully in sync.

> @@ -2058,11 +2058,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>   */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>  {
> +	if (crtc->state)
> +		kfree(crtc->state->mode);
> +
>  	kfree(crtc->state);
>  	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
>  
> -	if (crtc->state)
> +	if (crtc->state) {
>  		crtc->state->crtc = crtc;
> +		crtc->state->mode =
> +			kzalloc(sizeof(*crtc->state->mode), GFP_KERNEL);

Allocating an empty mode object seems superflous. Why do we need this?

> +	}
> +
> +	if (crtc->state && !crtc->state->mode) {
> +		kfree(crtc->state);
> +		crtc->state = NULL;
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
>  
> @@ -2088,6 +2099,11 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
>  		state->active_changed = false;
>  		state->planes_changed = false;
>  		state->event = NULL;
> +		state->mode = drm_mode_duplicate(crtc->dev, crtc->state->mode);
> +		if (!state->mode) {
> +			kfree(state);
> +			state = NULL;
> +		}
>  	}
>  
>  	return state;
> @@ -2105,6 +2121,7 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>  void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					  struct drm_crtc_state *state)
>  {
> +	kfree(state->mode);
>  	kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5785336..6023851 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2008,7 +2008,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
>  		if (crtc->state->enable) {
> -			drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> +			drm_crtc_convert_to_umode(&crtc_resp->mode, crtc->state->mode);
>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index c6063ff..8a9a045 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -943,11 +943,32 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	if (crtc->funcs->atomic_duplicate_state)
>  		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
> -	else if (crtc->state)
> +	else if (crtc->state) {
>  		crtc_state = kmemdup(crtc->state, sizeof(*crtc_state),
>  				     GFP_KERNEL);
> -	else
> +		/* XXX: this is unpleasant: we should mandate dup instead */
> +		if (crtc_state) {
> +			crtc_state->mode =
> +				drm_mode_duplicate(crtc->dev,
> +				                   crtc->state->mode);
> +			if (!crtc_state->mode) {
> +				kfree(crtc_state);
> +				crtc_state = NULL;
> +			}
> +		}
> +	}
> +	else {
>  		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> +		if (crtc_state) {
> +			crtc_state->mode = kzalloc(sizeof(*crtc_state->mode),
> +			                           GFP_KERNEL);
> +			/* XXX: as above, but mandate a new_state */
> +			if (!crtc_state->mode) {
> +				kfree(crtc_state);
> +				crtc_state = NULL;
> +			}
> +		}
> +	}

I think for transitional helpers if we just do a crtc_state->mode =
crtc->mode that should be all that's needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch