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

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 01/15/2015 08:50 PM, Matt Roper wrote:
> 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.

Daniel was anticipating we would still need ->config and ->new_config. 
For async updates, we would commit drm_crtc->state before it is written 
to the hardware. In that case, the hardware current state would still be 
in intel_crtc->config and drm_crtc->state would actually match 
->new_config. We'll have to sort this out later, so I just avoided doing 
too many changes.

Ander

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx