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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
