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

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

On 10/22/2014 11:33 AM, Ville Syrjälä wrote:
> On Tue, Oct 21, 2014 at 04:02:04PM +0300, 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.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |   3 +
>>   drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>>   drivers/gpu/drm/i915/intel_display.c | 125 +++++++++++++++++++++++++++--------
>>   3 files changed, 102 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index d7412d9..5b2464f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -233,6 +233,8 @@ 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 */
>>   	const char *name;
>> @@ -480,6 +482,7 @@ struct drm_i915_display_funcs {
>>   				struct intel_crtc_config *);
>>   	void (*get_plane_config)(struct intel_crtc *,
>>   				 struct intel_plane_config *);
>> +	int (*crtc_compute_clock)(struct intel_crtc *crtc);
>>   	int (*crtc_mode_set)(struct intel_crtc *crtc,
>>   			     int x, int y,
>>   			     struct drm_framebuffer *old_fb);
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 5915a07..7b8c4b8 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1375,6 +1375,8 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>>   	dev_priv->num_shared_dpll = 2;
>>
>>   	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		dev_priv->shared_dplls[i].new_config =
>> +			&dev_priv->shared_dplls[i].config;
>>   		dev_priv->shared_dplls[i].id = i;
>>   		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
>>   		dev_priv->shared_dplls[i].disable = hsw_ddi_pll_disable;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index cdaf8ef..f2f7e8f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3851,15 +3851,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>>   struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   {
>>   	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> +	struct intel_shared_dpll *pll;
>>   	enum intel_dpll_id i;
>>
>> -	if (pll) {
>> -		DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
>> -			      crtc->base.base.id, pll->name);
>> -		intel_put_shared_dpll(crtc);
>> -	}
>> -
>>   	if (HAS_PCH_IBX(dev_priv->dev)) {
>>   		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>   		i = (enum intel_dpll_id) crtc->pipe;
>> @@ -3868,7 +3862,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>   			      crtc->base.base.id, pll->name);
>>
>> -		WARN_ON(pll->config.crtc_mask);
>> +		WARN_ON(pll->new_config->crtc_mask);
>>
>>   		goto found;
>>   	}
>> @@ -3877,17 +3871,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   		pll = &dev_priv->shared_dplls[i];
>>
>>   		/* Only want to check enabled timings first */
>> -		if (pll->config.crtc_mask == 0)
>> +		if (pll->new_config->crtc_mask == 0)
>>   			continue;
>>
>> -		if (memcmp(&crtc->config.dpll_hw_state,
>> -			   &pll->config.hw_state,
>> -			   sizeof(pll->config.hw_state)) == 0) {
>> -			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
>> -				      "(crtc_mask 0x%08x, active %d)\n",
>> +		if (memcmp(&crtc->new_config->dpll_hw_state,
>> +			   &pll->new_config->hw_state,
>> +			   sizeof(pll->new_config->hw_state)) == 0) {
>> +			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>>   				      crtc->base.base.id, pll->name,
>> -				      pll->config.crtc_mask, pll->active);
>> -
>> +				      pll->new_config->crtc_mask,
>> +				      pll->active);
>>   			goto found;
>>   		}
>>   	}
>> @@ -3895,7 +3888,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   	/* Ok no matching timings, maybe there's a free one? */
>>   	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>   		pll = &dev_priv->shared_dplls[i];
>> -		if (pll->config.crtc_mask == 0) {
>> +		if (pll->new_config->crtc_mask == 0) {
>>   			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>>   				      crtc->base.base.id, pll->name);
>>   			goto found;
>> @@ -3905,18 +3898,64 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   	return NULL;
>>
>>   found:
>> -	if (pll->config.crtc_mask == 0)
>> -		pll->config.hw_state = crtc->config.dpll_hw_state;
>> +	if (pll->new_config->crtc_mask == 0)
>> +		pll->new_config->hw_state = crtc->new_config->dpll_hw_state;
>>
>> -	crtc->config.shared_dpll = i;
>> +	crtc->new_config->shared_dpll = i;
>>   	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>>   			 pipe_name(crtc->pipe));
>>
>> -	pll->config.crtc_mask |= 1 << crtc->pipe;
>> +	pll->new_config->crtc_mask |= 1 << crtc->pipe;
>>
>>   	return pll;
>>   }
>>
>> +/**
>> + * intel_shared_dpll_start_config - start a new PLL staged config
>> + * @dev_priv: DRM device
>> + * @clear_pipes: mask of pipes that will have their PLLs freed
>> + *
>> + * Starts a new PLL staged config, copying the current config but
>> + * releasing the references of pipes specified in clear_pipes.
>> + */
>> +static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
>> +					  unsigned clear_pipes)
>> +{
>> +	struct intel_shared_dpll *pll;
>> +	enum intel_dpll_id i;
>> +
>> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		pll = &dev_priv->shared_dplls[i];
>> +
>> +		pll->new_config = kmalloc(sizeof(*pll->new_config),
>> +					  GFP_KERNEL);
>> +		if (!pll->new_config)
>> +			return -ENOMEM;
>
> Need to restore new_config = &config? For the crtc config we have
> new_config==NULL outside modeset operations to catch abusers, but you
> seem to have new_config==&config for the plls normally. I had the crtc
> config that way too initially, but Daniel wanted to changed it. Not sure
> how hard it would be to do the same thing for plls?

That makes sense. It shouldn't be too difficult to do that, since the 
only function that needs to work on both old and new config is 
intel_get_shared_dpll(), and that is only called during mode set.

> My original idea for the new_config==&config approach in crtcs was that
> we might be able to share some functions that get called both during
> and after modesets and they would automagically consult the correct
> pipe config. But the NULL approach certainly seems to work so far and
> it would result in a neat explosion if it didn't.
>
> Also what happens with the already allocated pll configs if we fail
> midway through the loop here? Or if we fail later in the
> .compute_clock() or some other place before the pll config is commited?

Yeah, oops, we end up with corrupted state. Will fix.

> Could also use kmemdup() here I suppose.
>
>> +
>> +		pll->new_config->crtc_mask =
>> +			pll->config.crtc_mask & ~clear_pipes;
>> +		pll->new_config->hw_state = pll->config.hw_state;
>> +	}
>> +
>> +	return 0;
>> +}
>> +

[...]

>> @@ -12586,6 +12651,7 @@ static void intel_init_display(struct drm_device *dev)
>>   	if (HAS_DDI(dev)) {
>>   		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>>   		dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> +		dev_priv->display.crtc_compute_clock = NULL;
>
> dev_priv is kzalloc()ed so no need for NULL initialization.

Ok. I'll resend with the above fixed.


Thanks,
Ander