Re: [PATCH 1/3] drm/i915: Do not acquire crtc state to check clock during modeset, v2.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 27, 2015 at 02:51:16PM +0100, Maarten Lankhorst wrote:
> Op 27-10-15 om 14:49 schreef Ville Syrjälä:
> > On Tue, Oct 27, 2015 at 02:41:16PM +0100, Maarten Lankhorst wrote:
> >> Op 27-10-15 om 14:29 schreef Ville Syrjälä:
> >>> On Tue, Oct 27, 2015 at 02:26:31PM +0100, 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.
> >>>>
> >>>> Changes since v1:
> >>>> - Add dev_priv->active_crtcs for tracking which crtcs are active.
> >>>> - Rename min_cdclk to min_pixclk and move to dev_priv.
> >>>> - Add a active_crtcs mask which is updated atomically.
> >>>> - Add intel_atomic_state->modeset which is set on modesets.
> >>>> - Commit new pixclk/active_crtcs right after state swap.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/i915_drv.h      |   5 ++
> >>>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
> >>>>  drivers/gpu/drm/i915/intel_display.c | 160 +++++++++++++++++++++++++----------
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |   7 +-
> >>>>  4 files changed, 125 insertions(+), 49 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>> index 59c94929f25b..37eef86e4a0a 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>> @@ -1837,8 +1837,13 @@ struct drm_i915_private {
> >>>>  	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
> >>>>  #endif
> >>>>  
> >>>> +	/* dpll and cdclk state is protected by connection_mutex */
> >>>>  	int num_shared_dpll;
> >>>>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
> >>>> +
> >>>> +	unsigned int active_crtcs;
> >>>> +	unsigned int min_pixclk[I915_MAX_PIPES];
> >>>> +
> >>>>  	int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
> >>>>  
> >>>>  	struct i915_workarounds workarounds;
> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>>> index 25a891aa3824..cb8fcf4fe7f2 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>>> @@ -305,5 +305,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
> >>>>  {
> >>>>  	struct intel_atomic_state *state = to_intel_atomic_state(s);
> >>>>  	drm_atomic_state_default_clear(&state->base);
> >>>> -	state->dpll_set = false;
> >>>> +	state->dpll_set = state->modeset = false;
> >>>>  }
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 022e628b8520..a0508d5e9e13 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,18 @@ 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;
> >>>> +
> >>>> +		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))
> >>>> @@ -5938,23 +5944,32 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> >>>>  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;
> >>>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>> +	struct drm_crtc *crtc;
> >>>> +	struct drm_crtc_state *crtc_state;
> >>>> +	unsigned max_pixel_rate = 0, i;
> >>>> +	enum pipe pipe;
> >>>>  
> >>>> -	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);
> >>>> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> >>>> +	       sizeof(intel_state->min_pixclk));
> >>>>  
> >>>> -		if (!crtc_state->base.enable)
> >>>> -			continue;
> >>>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >>>> +		int pixclk = 0;
> >>>>  
> >>>> -		max_pixclk = max(max_pixclk,
> >>>> -				 crtc_state->base.adjusted_mode.crtc_clock);
> >>>> +		if (crtc_state->enable)
> >>>> +			pixclk = crtc_state->adjusted_mode.crtc_clock;
> >>>> +
> >>>> +		intel_state->min_pixclk[i] = pixclk;
> >>>>  	}
> >>>>  
> >>>> -	return max_pixclk;
> >>>> +	if (!intel_state->active_crtcs)
> >>>> +		return 0;
> >>>> +
> >>>> +	for_each_pipe(dev_priv, pipe)
> >>>> +		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
> >>>> +
> >>>> +	return max_pixel_rate;
> >>>>  }
> >>>>  
> >>>>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >>>> @@ -6255,6 +6270,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> >>>>  	for_each_power_domain(domain, domains)
> >>>>  		intel_display_power_put(dev_priv, domain);
> >>>>  	intel_crtc->enabled_power_domains = 0;
> >>>> +
> >>>> +	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> >>>> +	dev_priv->min_pixclk[intel_crtc->pipe] = 0;
> >>>>  }
> >>>>  
> >>>>  /*
> >>>> @@ -9490,29 +9508,39 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >>>>  /* compute the max rate for new configuration */
> >>>>  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;
> >>>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >>>> +	struct drm_i915_private *dev_priv = state->dev->dev_private;
> >>>> +	struct drm_crtc *crtc;
> >>>> +	struct drm_crtc_state *crtc_state;
> >>>> +	unsigned max_pixel_rate = 0, i;
> >>>> +	enum pipe pipe;
> >>>>  
> >>>> -	for_each_intel_crtc(state->dev, intel_crtc) {
> >>>> -		int pixel_rate;
> >>>> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> >>>> +	       sizeof(intel_state->min_pixclk));
> >>>>  
> >>>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> >>>> -		if (IS_ERR(crtc_state))
> >>>> -			return PTR_ERR(crtc_state);
> >>>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >>>> +		int pixclk = 0;
> >>>>  
> >>>> -		if (!crtc_state->base.enable)
> >>>> -			continue;
> >>>> +		if (crtc_state->enable) {
> >>>> +			struct intel_crtc_state *intel_crtc_state =
> >>>> +				to_intel_crtc_state(crtc_state);
> >>>>  
> >>>> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> >>>> +			pixclk = ilk_pipe_pixel_rate(intel_crtc_state);
> >>> Could you avoid the reindent and renames please?
> >>>
> >> The pixclk has to be set even when enabled = false. This way the names for ilk_max_pixel_rate and intel_mode_max_pixclk are identical.
> > I don't follow.
> The old code was a noop when crtc_state->enable = false, new code needs to set min_pixclk[pipe] to 0 in that case.

That doesn't explain the rename or reindent.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx