Re: [PATCH 6/7] drm/i915: Improve how the memory for crtc state is allocated

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote:
> The previous patch changed the config field in intel_crtc to a pointer,
> but to keep the mechanical changes (done with spatch) separate from the
> new code, the pointer was made to point to a new _config field with type
> struct intel_crtc_state added to that struct. This patch improves that
> code by getting rid of that field, allocating a state struct in
> intel_crtc_init() a keeping it properly updated when a mode set
> happens.
> 
> v2: Manual changes split from previous patch. (Matt)
>     Don't leak the current state when the crtc is destroyed (Matt)
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index acdaed2..002e5a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8926,6 +8926,13 @@ out:
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +static void intel_crtc_set_state(struct intel_crtc *crtc,
> +				 struct intel_crtc_state *crtc_state)
> +{
> +	kfree(crtc->config);
> +	crtc->config = crtc_state;
> +}
> +
>  static void intel_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  
>  	drm_crtc_cleanup(crtc);
>  
> +	intel_crtc_set_state(intel_crtc, NULL);

Actually I just looked at this again and I think this is going to cause
a non-fatal warning on unload.  Given your update of the base state in
patch 7, the drm_crtc_cleanup() here is going to see crtc->state as non-NULL and
try to clean it up itself.  But since you haven't implemented
crtc->funcs->atomic_destroy_state(), it will just throw a warning and
continue on.

So I think you want to do one of the following:
 * Move the intel_crtc_set_state() call above drm_crtc_cleanup() so that
   the core won't see a non-NULL state and think it needs to clean up.
 * Completely drop the intel_crtc_set_state() call and instead implement
   crtc->funcs->atomic_destroy_state() so that the core can handle the
   cleanup for us.


Matt

>  	kfree(intel_crtc);
>  }
>  
> @@ -10995,8 +11003,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		crtc->mode = *mode;
>  		/* mode_set/enable/disable functions rely on a correct pipe
>  		 * config. */
> -		(*(to_intel_crtc(crtc)->config)) = *pipe_config;
> -		to_intel_crtc(crtc)->new_config = to_intel_crtc(crtc)->config;
> +		intel_crtc_set_state(to_intel_crtc(crtc), pipe_config);
>  
>  		/*
>  		 * Calculate and store various constants which
> @@ -11040,7 +11047,6 @@ done:
>  	if (ret && crtc->enabled)
>  		crtc->mode = *saved_mode;
>  
> -	kfree(pipe_config);
>  	kfree(saved_mode);
>  	return ret;
>  }
> @@ -12187,6 +12193,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *crtc_state = NULL;
>  	struct drm_plane *primary = NULL;
>  	struct drm_plane *cursor = NULL;
>  	int i, ret;
> @@ -12195,6 +12202,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	if (intel_crtc == NULL)
>  		return;
>  
> +	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> +	if (!crtc_state)
> +		goto fail;
> +	intel_crtc_set_state(intel_crtc, crtc_state);
> +
>  	primary = intel_primary_plane_create(dev, pipe);
>  	if (!primary)
>  		goto fail;
> @@ -12240,7 +12252,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> -	intel_crtc->config = &intel_crtc->_config;
>  	return;
>  
>  fail:
> @@ -12248,6 +12259,7 @@ fail:
>  		drm_plane_cleanup(primary);
>  	if (cursor)
>  		drm_plane_cleanup(cursor);
> +	kfree(crtc_state);
>  	kfree(intel_crtc);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0b59a93..c8c0b7f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -469,7 +469,6 @@ struct intel_crtc {
>  	uint32_t cursor_base;
>  
>  	struct intel_plane_config plane_config;
> -	struct intel_crtc_state _config;
>  	struct intel_crtc_state *config;
>  	struct intel_crtc_state *new_config;
>  	bool new_enabled;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx