Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Use our newly created encoder_mask to iterate over the encoders.

As someone who was not paying attention to the discussion of the
previous patches related to this, I think it would be really good if
your commit message could tell me why we should use the newly created
encoder_mask instead of the current patch. What's bad about the current
version? Please sell me your patch. If you think the answer is trivial,
remember that it's not trivial to many people, and that random people
may find this patch through git-bisect and have to judge its
importance. Also, an explanation really helps the reviewers :)

The patch looks correct, so if you improve the commit message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++-----
> -------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 30d4db0d776f..b479ba6238d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5301,31 +5301,37 @@ intel_display_port_aux_power_domain(struct
> intel_encoder *intel_encoder)
>  	}
>  }
>  
> -static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> +static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
> +					    struct intel_crtc_state
> *crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct intel_encoder *intel_encoder;
> +	struct drm_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	unsigned long mask;
> -	enum transcoder transcoder = intel_crtc->config-
> >cpu_transcoder;
> +	enum transcoder transcoder = crtc_state->cpu_transcoder;
>  
> -	if (!crtc->state->active)
> +	if (!crtc_state->base.active)
>  		return 0;
>  
>  	mask = BIT(POWER_DOMAIN_PIPE(pipe));
>  	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> -	if (intel_crtc->config->pch_pfit.enabled ||
> -	    intel_crtc->config->pch_pfit.force_thru)
> +	if (crtc_state->pch_pfit.enabled ||
> +	    crtc_state->pch_pfit.force_thru)
>  		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
>  
> -	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> +	drm_for_each_encoder_mask(encoder, dev, crtc_state-
> >base.encoder_mask) {
> +		struct intel_encoder *intel_encoder =
> to_intel_encoder(encoder);
> +
>  		mask |=
> BIT(intel_display_port_power_domain(intel_encoder));
> +	}
>  
>  	return mask;
>  }
>  
> -static unsigned long modeset_get_crtc_power_domains(struct drm_crtc
> *crtc)
> +static unsigned long
> +modeset_get_crtc_power_domains(struct drm_crtc *crtc,
> +			       struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5333,7 +5339,8 @@ static unsigned long
> modeset_get_crtc_power_domains(struct drm_crtc *crtc)
>  	unsigned long domains, new_domains, old_domains;
>  
>  	old_domains = intel_crtc->enabled_power_domains;
> -	intel_crtc->enabled_power_domains = new_domains =
> get_crtc_power_domains(crtc);
> +	intel_crtc->enabled_power_domains = new_domains =
> +		get_crtc_power_domains(crtc, crtc_state);
>  
>  	domains = new_domains & ~old_domains;
>  
> @@ -5365,7 +5372,8 @@ static void
> modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>  	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)
> ;
> +				modeset_get_crtc_power_domains(crtc,
> +					to_intel_crtc_state(crtc-
> >state));
>  	}
>  
>  	if (dev_priv->display.modeset_commit_cdclk &&
> @@ -13486,7 +13494,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		}
>  
>  		if (update_pipe) {
> -			put_domains =
> modeset_get_crtc_power_domains(crtc);
> +			put_domains =
> modeset_get_crtc_power_domains(crtc,
> +					      to_intel_crtc_state(cr
> tc->state));
>  
>  			/* make sure intel_modeset_check_state runs
> */
>  			hw_check = true;
> @@ -15830,7 +15839,7 @@ intel_modeset_setup_hw_state(struct
> drm_device *dev)
>  	for_each_intel_crtc(dev, crtc) {
>  		unsigned long put_domains;
>  
> -		put_domains = modeset_get_crtc_power_domains(&crtc-
> >base);
> +		put_domains = modeset_get_crtc_power_domains(&crtc-
> >base, crtc->config);
>  		if (WARN_ON(put_domains))
>  			modeset_put_power_domains(dev_priv,
> put_domains);
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx