Re: [PATCH 02/58] drm/i915: rip out crtc prepare/commit indirection

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Sun, 19 Aug 2012 21:12:19 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Just impendance matching with the the crtc helper stuff.
> 
> ... and somehow the design of this all ended up in this commit here,
> too ;-)
> 
> The big plan is that this new set of crtc display_funcs take full
> responsibility of modeset operations for the entire display output
> pipeline (by calling down into object-specific callbacks and
> functions). The platform-specific callbacks simply know best what the
> proper order is.
> 
> This has the drawback that we can't do minimal change-overs any more
> if a modeset just disables one encoder in a cloned configuration
> (because we will only expose a disable/enable action that takes
> down/sets up the entire crtc including all encoders). Imo that's the
> only sane way to do it though:
> - The use-case for this is pretty minimal, even when presenting (at
>   least sane people) should use a dual-screen output so that you can
>   see your notes on your panel. Clone mode is imo BS.
> - With all the clone mode constrains, shared resources, and special
>   ordering requirements (which differ even on the same platform
>   sometimes for different outputs) there's no way we'd get this right
>   for all cases. Especially since this is a under-used feature.
> - And to top it off: On haswell even dp link re-training requires us
>   to take down the entire display pipe - otherwise the chip dies.
> 
> So the only sane way is to do a full modeset on every crtc where the
> output config changes in any way.
> 
> To support global modeset (i.e. set the configuration for all crtcs at
> once) we'd then add one more function to allocate global and shared
> objects in the best ways (e.g. fdi links, pch plls, ...). The crtc
> functions would then simply use the pre-allocated stuff (and shouldn't
> be able to fail, ever). We could even do all the object pinning in
> there (and maybe try to defragment the global gtt if we fail)!
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 37
> ++---------------------------------- 1 file changed, 2 insertions(+),
> 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index adc9868..04bec4b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3500,34 +3500,6 @@ static void intel_crtc_disable(struct drm_crtc
> *crtc) }
>  }
>  
> -/* Prepare for a mode set.
> - *
> - * Note we could be a lot smarter here.  We need to figure out which
> outputs
> - * will be enabled, which disabled (in short, how the config will
> changes)
> - * and perform the minimum necessary steps to accomplish that, e.g.
> updating
> - * watermarks, FBC configuration, making sure PLLs are programmed
> correctly,
> - * panel fitting is in the proper state, etc.
> - */
> -static void i9xx_crtc_prepare(struct drm_crtc *crtc)
> -{
> -	i9xx_crtc_disable(crtc);
> -}
> -
> -static void i9xx_crtc_commit(struct drm_crtc *crtc)
> -{
> -	i9xx_crtc_enable(crtc);
> -}
> -
> -static void ironlake_crtc_prepare(struct drm_crtc *crtc)
> -{
> -	ironlake_crtc_disable(crtc);
> -}
> -
> -static void ironlake_crtc_commit(struct drm_crtc *crtc)
> -{
> -	ironlake_crtc_enable(crtc);
> -}
> -
>  void intel_encoder_prepare(struct drm_encoder *encoder)
>  {
>  	struct drm_encoder_helper_funcs *encoder_funcs =
> encoder->helper_private; @@ -6626,13 +6598,8 @@ static void
> intel_crtc_init(struct drm_device *dev, int pipe) intel_crtc->active
> = true; /* force the pipe off on setup_init_config */ intel_crtc->bpp
> = 24; /* default for pre-Ironlake */ 
> -	if (HAS_PCH_SPLIT(dev)) {
> -		intel_helper_funcs.prepare = ironlake_crtc_prepare;
> -		intel_helper_funcs.commit = ironlake_crtc_commit;
> -	} else {
> -		intel_helper_funcs.prepare = i9xx_crtc_prepare;
> -		intel_helper_funcs.commit = i9xx_crtc_commit;
> -	}
> +	intel_helper_funcs.prepare = dev_priv->display.crtc_disable;
> +	intel_helper_funcs.commit = dev_priv->display.crtc_enable;
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  }


Looks fine.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>