Re: [PATCH 1/2] drm/i915: Remove crtc->config usage from intel_modeset_readout_hw_state()

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

On Tue, Dec 27, 2016 at 03:56:54PM +0100, Daniel Vetter wrote:
> On Thu, Dec 22, 2016 at 04:04:41PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > crtc->config is on its way out. Let's reduce our dependence on it a
> > little bit by removing it from intel_modeset_readout_hw_state().
> > 
> > Also replace crtc->acttive checks with crtc_state->base.active checks.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed both patches to dinq. Thanks for the reviews.

> 
> Idling musing: I wonder whether we should convert the readout code to use
> an entirely free-standing atomic static tracked in drm_atomic_commit. And
> then apply it with a directly call to swap_state. That way we could reuse
> all the neat iterators.
> 
> Not sure that's a good idea though.

Hmm. Maybe.

At some point I was toying with the idea of making the old LVDS/DVO
intel_crtc_mode_get() thing use the normal readout paths, but I was
slightly worried that the lack of the top level atomic state could have
been a problem. I don't think it actually was for the platforms where
that codepath can be taken, but I did have to think about it a bit at
least.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d8effd4da034..091c192c6c5c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -17032,7 +17032,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  	dev_priv->active_crtcs = 0;
> >  
> >  	for_each_intel_crtc(dev, crtc) {
> > -		struct intel_crtc_state *crtc_state = crtc->config;
> > +		struct intel_crtc_state *crtc_state =
> > +			to_intel_crtc_state(crtc->base.state);
> >  
> >  		__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
> >  		memset(crtc_state, 0, sizeof(*crtc_state));
> > @@ -17051,7 +17052,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  
> >  		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
> >  			      crtc->base.base.id, crtc->base.name,
> > -			      enableddisabled(crtc->active));
> > +			      enableddisabled(crtc_state->base.active));
> >  	}
> >  
> >  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> > @@ -17061,7 +17062,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  						  &pll->config.hw_state);
> >  		pll->config.crtc_mask = 0;
> >  		for_each_intel_crtc(dev, crtc) {
> > -			if (crtc->active && crtc->config->shared_dpll == pll)
> > +			struct intel_crtc_state *crtc_state =
> > +				to_intel_crtc_state(crtc->base.state);
> > +
> > +			if (crtc_state->base.active &&
> > +			    crtc_state->shared_dpll == pll)
> >  				pll->config.crtc_mask |= 1 << crtc->pipe;
> >  		}
> >  		pll->active_mask = pll->config.crtc_mask;
> > @@ -17074,11 +17079,14 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  		pipe = 0;
> >  
> >  		if (encoder->get_hw_state(encoder, &pipe)) {
> > +			struct intel_crtc_state *crtc_state;
> > +
> >  			crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > +			crtc_state = to_intel_crtc_state(crtc->base.state);
> >  
> >  			encoder->base.crtc = &crtc->base;
> > -			crtc->config->output_types |= 1 << encoder->type;
> > -			encoder->get_config(encoder, crtc->config);
> > +			crtc_state->output_types |= 1 << encoder->type;
> > +			encoder->get_config(encoder, crtc_state);
> >  		} else {
> >  			encoder->base.crtc = NULL;
> >  		}
> > @@ -17119,14 +17127,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  	}
> >  
> >  	for_each_intel_crtc(dev, crtc) {
> > +		struct intel_crtc_state *crtc_state =
> > +			to_intel_crtc_state(crtc->base.state);
> >  		int pixclk = 0;
> >  
> > -		crtc->base.hwmode = crtc->config->base.adjusted_mode;
> > +		crtc->base.hwmode = crtc_state->base.adjusted_mode;
> >  
> >  		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> > -		if (crtc->base.state->active) {
> > -			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
> > -			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
> > +		if (crtc_state->base.active) {
> > +			intel_mode_from_pipe_config(&crtc->base.mode, crtc_state);
> > +			intel_mode_from_pipe_config(&crtc_state->base.adjusted_mode, crtc_state);
> >  			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
> >  
> >  			/*
> > @@ -17146,17 +17156,17 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  			 * mode change, which means it's safe to do a full
> >  			 * recalculation.
> >  			 */
> > -			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
> > +			crtc_state->base.mode.private_flags = I915_MODE_FLAG_INHERITED;
> >  
> >  			if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -				pixclk = ilk_pipe_pixel_rate(crtc->config);
> > +				pixclk = ilk_pipe_pixel_rate(crtc_state);
> >  			else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -				pixclk = crtc->config->base.adjusted_mode.crtc_clock;
> > +				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> >  			else
> >  				WARN_ON(dev_priv->display.modeset_calc_cdclk);
> >  
> >  			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > -			if (IS_BROADWELL(dev_priv) && crtc->config->ips_enabled)
> > +			if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> >  				pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> >  
> >  			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
> > @@ -17165,7 +17175,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  
> >  		dev_priv->min_pixclk[crtc->pipe] = pixclk;
> >  
> > -		intel_pipe_config_sanity_check(dev_priv, crtc->config);
> > +		intel_pipe_config_sanity_check(dev_priv, crtc_state);
> >  	}
> >  }
> >  
> > -- 
> > 2.10.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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