On Sun, 19 Aug 2012 21:13:08 +0200
Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> ... using the pipe masks from the previous patch.
>
> Well, not quite:
> - We still need to call the disable_unused_functions helper, until
> we've moved the call to commit_output_state further down and
> adjusted intel_crtc_disable a bit. The next patch will do that.
> - Because we don't support (yet) mode changes on more than one crtc at
> a time, some of the modeset_pipes checks are a bit hackish - but
> that only needs fixing once we incorporate global modeset support.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_display.c | 99 +++++++++++++++++++++++-------------
> 1 file changed, 63 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4334400..3d99522 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6791,6 +6791,12 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
> *prepare_pipes &= ~(*disable_pipes);
> }
>
> +#define for_each_intel_crtc_masked(dev, mask, intel_crtc) \
> + list_for_each_entry((intel_crtc), \
> + &(dev)->mode_config.crtc_list, \
> + base.head) \
> + if (mask & (1 <<(intel_crtc)->pipe)) \
> +
> bool intel_set_mode(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> int x, int y, struct drm_framebuffer *fb)
> @@ -6800,73 +6806,92 @@ bool intel_set_mode(struct drm_crtc *crtc,
> struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode;
> struct drm_encoder_helper_funcs *encoder_funcs;
> struct drm_encoder *encoder;
> - unsigned disable_pipe, prepare_pipes, modeset_pipes;
> + struct intel_crtc *intel_crtc;
> + unsigned disable_pipes, prepare_pipes, modeset_pipes;
> bool ret = true;
>
> intel_modeset_affected_pipes(crtc, &modeset_pipes,
> - &prepare_pipes, &disable_pipe);
> + &prepare_pipes, &disable_pipes);
> +
> + DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> + modeset_pipes, prepare_pipes, disable_pipes);
>
> intel_modeset_commit_output_state(dev);
>
> crtc->enabled = drm_helper_crtc_in_use(crtc);
> - if (!crtc->enabled) {
> - drm_helper_disable_unused_functions(dev);
> - return true;
> - }
>
> + for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> + drm_helper_disable_unused_functions(dev);
>
> saved_hwmode = crtc->hwmode;
> saved_mode = crtc->mode;
>
> - adjusted_mode = intel_modeset_adjusted_mode(crtc, mode);
> - if (IS_ERR(adjusted_mode)) {
> - return false;
> - }
> + /* Hack: Because we don't (yet) support global modeset on multiple
> + * crtcs, we don't keep track of the new mode for more than one crtc.
> + * Hence simply check whether any bit is set in modeset_pipes in all the
> + * pieces of code that are not yet converted to deal with mutliple crtcs
> + * changing their mode at the same time. */
> + adjusted_mode = NULL;
> + if (modeset_pipes) {
> + adjusted_mode = intel_modeset_adjusted_mode(crtc, mode);
> + if (IS_ERR(adjusted_mode)) {
> + return false;
> + }
>
> - intel_crtc_prepare_encoders(dev);
> + intel_crtc_prepare_encoders(dev);
> + }
>
> - dev_priv->display.crtc_disable(crtc);
> + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> + dev_priv->display.crtc_disable(&intel_crtc->base);
>
> - crtc->mode = *mode;
> + if (modeset_pipes) {
> + crtc->mode = *mode;
> + crtc->x = x;
> + crtc->y = y;
> + }
>
> /* Set up the DPLL and any encoders state that needs to adjust or depend
> * on the DPLL.
> */
> - ret = !intel_crtc_mode_set(crtc, mode, adjusted_mode, x, y, fb);
> - if (!ret)
> - goto done;
> + for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> + ret = !intel_crtc_mode_set(&intel_crtc->base,
> + mode, adjusted_mode,
> + x, y, fb);
> + if (!ret)
> + goto done;
>
> - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
>
> - if (encoder->crtc != crtc)
> - continue;
> + if (encoder->crtc != &intel_crtc->base)
> + continue;
>
> - DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> - encoder->base.id, drm_get_encoder_name(encoder),
> - mode->base.id, mode->name);
> - encoder_funcs = encoder->helper_private;
> - encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> + DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> + encoder->base.id, drm_get_encoder_name(encoder),
> + mode->base.id, mode->name);
> + encoder_funcs = encoder->helper_private;
> + encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> + }
> }
>
> - crtc->x = x;
> - crtc->y = y;
> -
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> - dev_priv->display.crtc_enable(crtc);
> + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> + dev_priv->display.crtc_enable(&intel_crtc->base);
>
> - /* Store real post-adjustment hardware mode. */
> - crtc->hwmode = *adjusted_mode;
> + if (modeset_pipes) {
> + /* Store real post-adjustment hardware mode. */
> + crtc->hwmode = *adjusted_mode;
>
> - /* Calculate and store various constants which
> - * are later needed by vblank and swap-completion
> - * timestamping. They are derived from true hwmode.
> - */
> - drm_calc_timestamping_constants(crtc);
> + /* Calculate and store various constants which
> + * are later needed by vblank and swap-completion
> + * timestamping. They are derived from true hwmode.
> + */
> + drm_calc_timestamping_constants(crtc);
> + }
>
> /* FIXME: add subpixel order */
> done:
> drm_mode_destroy(dev, adjusted_mode);
> - if (!ret) {
> + if (!ret && crtc->enabled) {
> crtc->hwmode = saved_hwmode;
> crtc->mode = saved_mode;
> }
> @@ -6874,6 +6899,8 @@ done:
> return ret;
> }
>
> +#undef for_each_intel_crtc_masked
> +
> static void intel_set_config_free(struct intel_set_config *config)
> {
> if (config) {
An ugly intermediate step... also did you check whether moving the
crtc->x/y assignment up is safe? We're passing it around, but some
places might check for crtc->x/y looking for old values (or did that
already change in the previous patch... too many patches).
Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
