Re: [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 22-10-15 om 15:08 schreef Daniel Vetter:
> On Thu, Oct 22, 2015 at 01:56:27PM +0200, Maarten Lankhorst wrote:
>> Parallel modesets are still not allowed, but this will allow updating
>> a different crtc during a modeset if the clock is not changed.
>>
>> Additionally when all pipes are DPMS off the cdclk will be lowered
>> to the minimum allowed.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++
>>  2 files changed, 64 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 022e628b8520..051a1e2b1c55 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5237,6 +5237,7 @@ static void modeset_put_power_domains(struct drm_i915_private *dev_priv,
>>  
>>  static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>>  {
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_device *dev = state->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>> @@ -5245,13 +5246,20 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>>  	int i;
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (needs_modeset(crtc->state))
>> -			put_domains[to_intel_crtc(crtc)->pipe] =
>> -				modeset_get_crtc_power_domains(crtc);
>> +		struct intel_crtc *intel_crtc =
>> +			to_intel_crtc(crtc);
>> +		enum pipe pipe = intel_crtc->pipe;
>> +
>> +		if (!needs_modeset(crtc->state))
>> +			continue;
>> +
>> +		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
>> +
>> +		put_domains[pipe] = modeset_get_crtc_power_domains(crtc);
>>  	}
>>  
>>  	if (dev_priv->display.modeset_commit_cdclk) {
>> -		unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
>> +		unsigned int cdclk = intel_state->cdclk;
>>  
>>  		if (cdclk != dev_priv->cdclk_freq &&
>>  		    !WARN_ON(!state->allow_modeset))
>> @@ -5919,7 +5927,6 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
>>  	/*
>>  	 * FIXME:
>>  	 * - remove the guardband, it's not needed on BXT
>> -	 * - set 19.2MHz bypass frequency if there are no active pipes
>>  	 */
>>  	if (max_pixclk > 576000*9/10)
>>  		return 624000;
>> @@ -5929,8 +5936,10 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
>>  		return 384000;
>>  	else if (max_pixclk > 144000*9/10)
>>  		return 288000;
>> -	else
>> +	else if (max_pixclk)
>>  		return 144000;
>> +	else
>> +		return 19200;
>>  }
>>  
>>  /* Compute the max pixel clock for new configuration. Uses atomic state if
>> @@ -5939,22 +5948,31 @@ static int intel_mode_max_pixclk(struct drm_device *dev,
>>  				 struct drm_atomic_state *state)
>>  {
>>  	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>> -	int max_pixclk = 0;
>> +	int max_pixel_rate = 0;
>> +	bool any_active = false;
>>  
>> -	for_each_intel_crtc(dev, intel_crtc) {
>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +	for_each_intel_crtc(state->dev, intel_crtc) {
>> +		struct drm_crtc_state *drm_crtc_state;
>> +		struct drm_crtc *crtc = &intel_crtc->base;
>> +		int pixclk = 0;
>>  
>> -		if (!crtc_state->base.enable)
>> -			continue;
>> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
>> +		if (!drm_crtc_state) {
>> +			any_active |= intel_crtc->active;
>> +			pixclk = intel_crtc->min_cdclk;
>> +		} else if (drm_crtc_state->enable) {
>> +			struct intel_crtc_state *crtc_state =
>> +				to_intel_crtc_state(drm_crtc_state);
>> +
>> +			any_active |= drm_crtc_state->active;
>> +			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
>> +		}
>>  
>> -		max_pixclk = max(max_pixclk,
>> -				 crtc_state->base.adjusted_mode.crtc_clock);
>> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
>> +		max_pixel_rate = max(max_pixel_rate, pixclk);
>>  	}
>>  
>> -	return max_pixclk;
>> +	return any_active ? max_pixel_rate : 0;
>>  }
>>  
>>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>>  static int ilk_max_pixel_rate(struct drm_atomic_state *state)
>>  {
>>  	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>>  	int max_pixel_rate = 0;
>> +	bool any_active = false;
>>  
>>  	for_each_intel_crtc(state->dev, intel_crtc) {
>> -		int pixel_rate;
>> -
>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +		struct drm_crtc_state *drm_crtc_state;
>> +		struct drm_crtc *crtc = &intel_crtc->base;
>> +		int pixel_rate = 0;
>>  
>> -		if (!crtc_state->base.enable)
>> -			continue;
>> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
>> +		if (!drm_crtc_state) {
>> +			any_active |= intel_crtc->active;
>> +			pixel_rate = intel_crtc->min_cdclk;
>> +		} else if (drm_crtc_state->enable) {
>> +			struct intel_crtc_state *crtc_state =
>> +				to_intel_crtc_state(drm_crtc_state);
>>  
>> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
>> +			any_active |= drm_crtc_state->active;
>> +			pixel_rate = ilk_pipe_pixel_rate(crtc_state);
>>  
>> -		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> -		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
>> -			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>> +			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> +			if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
>> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>> +		}
>>  
>> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
>>  		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>>  	}
>>  
>> -	return max_pixel_rate;
>> +	return any_active ? max_pixel_rate : 0;
>>  }
>>  
>>  static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>> @@ -9612,14 +9636,10 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>>  	else
>>  		cdclk = 337500;
>>  
>> -	/*
>> -	 * FIXME move the cdclk caclulation to
>> -	 * compute_config() so we can fail gracegully.
>> -	 */
>>  	if (cdclk > dev_priv->max_cdclk_freq) {
>>  		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
>>  			  cdclk, dev_priv->max_cdclk_freq);
>> -		cdclk = dev_priv->max_cdclk_freq;
>> +		return -EINVAL;
>>  	}
>>  
>>  	to_intel_atomic_state(state)->cdclk = cdclk;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 51722e657b91..54a2c0da2ece 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -250,6 +250,7 @@ struct intel_atomic_state {
>>  	unsigned int cdclk;
>>  	bool dpll_set;
>>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>> +	unsigned int pipe_cdclk[I915_MAX_PIPES];
>>  };
>>  
>>  struct intel_plane_state {
>> @@ -536,8 +537,16 @@ struct intel_crtc {
>>  	 * Whether the crtc and the connected output pipeline is active. Implies
>>  	 * that crtc->enabled is set, i.e. the current mode configuration has
>>  	 * some outputs connected to this crtc.
>> +	 *
>> +	 * Protected by crtc.mutex and connection_mutex
> This needs to be clarified to mean: read-protected by connection_mutex,
> writers additionally must hold crtc->mutex. Same below.
>
> Thinking about this a bit more the usual design we do is put the state
> copies not into the parent object with funky rules, but instead into the
> state of the higher-level objects. So for example the read-only
> plane_active state is in crtc_state->plane_mask.
>
> Similarly in this case here I think it would be cleaner design to have a
> min_cdclk array in intel_state, setting it to 0 if the pipe is totally
> off. That way locking rules are 100% clear: If you need to change it, you
> need to copy the global state and acquire connection_mutex. That
> automatically gives you read-access to all crtc's min_cdclk.
Sorry this is the only case where a dependency on intel_crtc->active is valid.
fbc and watermark users should stop looking at it. (intel_crtc_active)

2 crtc's, #1 requires clock at 100, #2 requires clock at 200.
You do a DPMS mode off on #2, and if you don't track crtc_clock you would set the display clock to 100 and force a modeset on #1 too.
If you immediately do dpms off on #1 afterwards it means that the user would see #1 being disabled, re-enabled and disabled again..

> That way we don't play tricks with get_existing_state, don't add more
> depencies on the legacy-encumbered intel_crtc->active and it's just
> generally more awesome.
The tricks are needed to allow a modeset to run parallel to any page flips.
Though perhaps it would be better to add a struct to dev_priv that contains
min cdclk and a flag to see if the crtc is active or not.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux