On Sun, 19 Aug 2012 21:12:19 +0200
Daniel Vetter <daniel.vetter at 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 at 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 at virtuousgeek.org>
