Re: [PATCH 5/8] drm/i915: Don't access to crtc->new_config from intel_mode_max_pixclk()

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Dec 11, 2014 at 02:38:08PM +0200, Ander Conselvan de Oliveira wrote:
> So that we can get rid of the new_config pointer later.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index da5af23..a9f3034 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4897,27 +4897,39 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
>  }
>  
>  /* compute the max pixel clock for new configuration */
> -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv,
> +				 struct intel_crtc *mode_set_crtc,

I'm not sure if you need to pass this.  drm_crtc_state has a backpointer
to the corresponding drm_crtc now, so if we ensure that's set by our
driver when creating a new pipe_config, we could probably get away with
just passing the state here.

> +				 struct intel_crtc_state *crtc_new_state)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	struct intel_crtc *intel_crtc;
>  	int max_pixclk = 0;
> +	int pixclk;
>  
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->new_enabled)
> -			max_pixclk = max(max_pixclk,
> -					 intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> +		if (!intel_crtc->new_enabled)
> +			continue;

Not really related to this patch specifically, but I'm a little fuzzy on
the distinction between drm_crtc->enabled and drm_crtc_state->enable
(and then by extension intel_crtc->new_enabled).  It's not immediately
clear to me why we can't just have a single 'enabled' in the base state
structure and then when we have a new config that we're passing around,
the enabled flag in that structure would serve the purpose that
intel_crtc->new_enabled does for i915 today.  I'm probably
misunderstanding something.


Matt

> +
> +		if (intel_crtc == mode_set_crtc)
> +			pixclk = crtc_new_state->base.adjusted_mode.crtc_clock;
> +		else
> +			pixclk = intel_crtc->config->base.adjusted_mode.crtc_clock;
> +
> +		max_pixclk = max(max_pixclk, pixclk);
>  	}
>  
>  	return max_pixclk;
>  }
>  
>  static void valleyview_modeset_global_pipes(struct drm_device *dev,
> -					    unsigned *prepare_pipes)
> +					    unsigned *prepare_pipes,
> +					    struct intel_crtc *mode_set_crtc,
> +					    struct intel_crtc_state *crtc_new_state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int max_pixclk = intel_mode_max_pixclk(dev_priv);
> +	int max_pixclk =
> +		intel_mode_max_pixclk(dev_priv, mode_set_crtc, crtc_new_state);
>  
>  	if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
>  	    dev_priv->vlv_cdclk_freq)
> @@ -4932,7 +4944,7 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
>  static void valleyview_modeset_global_resources(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int max_pixclk = intel_mode_max_pixclk(dev_priv);
> +	int max_pixclk = intel_mode_max_pixclk(dev_priv, NULL, NULL);
>  	int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
>  
>  	if (req_cdclk != dev_priv->vlv_cdclk_freq) {
> @@ -10956,7 +10968,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
>  	if (IS_VALLEYVIEW(dev)) {
> -		valleyview_modeset_global_pipes(dev, &prepare_pipes);
> +		valleyview_modeset_global_pipes(dev, &prepare_pipes,
> +						to_intel_crtc(crtc),
> +						pipe_config);
>  
>  		/* may have added more to prepare_pipes than we should */
>  		prepare_pipes &= ~disable_pipes;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx