Re: [PATCH v2 11/17] drm/i915: Use global atomic state for staged pll config

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Ander Conselvan de Oliveira
	<ander.conselvan.de.oliveira@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 11/17] drm/i915: Use global atomic state for staged pll config
Date: Mon, 18 May 2015 18:27:51 +0200	[thread overview]
Message-ID: <555A1307.2000004@linux.intel.com> (raw)
In-Reply-To: <20150518154513.GY15256@phenom.ffwll.local>

Op 18-05-15 om 17:45 schreef Daniel Vetter:
> On Wed, May 13, 2015 at 10:23:41PM +0200, Maarten Lankhorst wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> Now that we can subclass drm_atomic_state we can also use it to keep
>> track of all the pll settings. atomic_state is a better place to hold
>> all shared state than keeping pll->new_config everywhere.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   1 -
>>  drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
>>  drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
>>  4 files changed, 95 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a0e4868653f2..0e200018c9aa 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
>>  
>>  struct intel_shared_dpll {
>>  	struct intel_shared_dpll_config config;
>> -	struct intel_shared_dpll_config *new_config;
>>  
>>  	int active; /* count of number of active CRTCs (i.e. DPMS on) */
>>  	bool on; /* is the PLL actually active? Disabled during modeset */
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 7ed8033aae60..aff87054406c 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>>  
>>  	return 0;
>>  }
>> +
>> +static void intel_atomic_duplicate_dpll_state(struct drm_device *dev,
>> +					      struct intel_atomic_state *state)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_shared_dpll *pll;
>> +	enum intel_dpll_id i;
>> +
>> +	state->dpll_set = true;
> The ww mutex locking dance here is missing. For simplicity I think we
> could just repurpose the dev->mode_config.connection_mutex. We need that
> anyway full modesets, and dpll changes should only be required in those.
> Which means this wont introduce any unecessary parallelism.
>
> It means though that we need to wire up a proper error code to all callers
> of duplicate/get_dpll_state, like with all the other atomic states. Might
> be best to follow the design for connector/crtc/planes an pass around
> pointers with error codes explicitly (instead of returning
> state->shared_dpll below since that can only cope with NULL and not with
> -EDEADLK).
>
> Sorry that I didn't spot this earlier.
>
The dpll state should only ever be done during a modeset in which case the connection_mutex is guaranteed to be held.
Instead of making this return a value can we add a lockdep_assert_held ?

~Maarten

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