Re: [PATCH 4/9] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, ville.syrjala@intel.com,
	shuang.he@linux.intel.com
Subject: Re: [PATCH 4/9] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs
Date: Tue, 04 Nov 2014 08:57:56 +0200	[thread overview]
Message-ID: <545878F4.6090002@intel.com> (raw)
In-Reply-To: <20141103135642.GG26941@phenom.ffwll.local>

On 11/03/2014 03:56 PM, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 02:51:27PM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 11:32:33AM +0200, Ander Conselvan de Oliveira wrote:
>>> It is possible for a mode set to fail if there aren't shared DPLLS that
>>> match the new configuration requirement or other errors in clock
>>> computation. If that step is executed after disabling crtcs, in the
>>> failure case the hardware configuration is changed and needs to be
>>> restored. Doing those things early will allow the mode set to fail
>>> before actually touching the hardware.
>>>
>>> Follow up patches will convert different platforms to use the new
>>> infrastructure.
>>>
>>> v2: Keep pll->new_config valid only during mode set (Ville)
>>>      Use kmemdup() in i915_shared_dpll_start_config() (Ville)
>>>      Restore old pll config if something fails before commit (Ville)
>>>      Don't set compute_clock hooks since dev_priv is kzalloc()'d (Ville)
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> Ran into a blocking question with this one, merged thus far.
>>
>>> @@ -7395,6 +7456,9 @@ static int ironlake_crtc_mode_set(struct intel_crtc *crtc,
>>>   		else
>>>   			crtc->new_config->dpll_hw_state.fp1 = fp;
>>>
>>> +		if (intel_crtc_to_shared_dpll(crtc))
>>> +			intel_put_shared_dpll(crtc);
>>
>> Don't we need the same fixup in intel_ddi_pll_select?
>
> Ok, I think I've figured it out - hsw does an unconditional put since a
> ddi pll might not be needed (for e.g. DP).

Yep, intel_ddi_pll_select() already had an unconditional call to 
intel_put_shared_dpll(), while the ironlake code relied on them being 
released in intel_get_shared_dpll(). After this patch the shared DPLLs 
should be release by the caller before getting a new one, but a later 
patch removes all those intel_put_shared_dpll() calls anyway.

Ander
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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