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>

I don't see a need to keep patch #7 separate from the changes you're
making here; I'd go ahead and just squash it into this patch since it's
a trivial change.

Although 6+7 look okay, I do question whether we really want to keep
intel_crtc->config now that our intel crtc state is a subclass of the
DRM state object and can always be accessed through our base pointer.
For planes, we just grab the base state through the
object's base pointer, then cast that to the i915-specific state object;
i.e.,

        foo = to_intel_plane_state(intel_plane->base.state);

My inclination would be to handle crtc's the same way for consistency
and so that we don't have two pointers per object (one base, one
i915-specific) that are essentially pointing at the same thing and have
to be kept in sync.

Anyway, you can consider this entire series

    Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

whether or not you decide to make further updates to 6+7.  The use of
Coccinelle to auto-generate most of these patches was great and made the
series very easy to review.


Matt


> ---
>  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);
>  	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