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

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

On Mon, Dec 15, 2014 at 11:17:45AM -0800, Matt Roper wrote:
> 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.

Yeah that sounds like where we want to go. No more random bits of
state sprinkled all over the place, and instead have it all neatly
collected in the state struct.

> 
> 
> 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

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