[Intel-gfx] [PATCH] drm/i915: Hook the force-restore into the highlevel modeset

[Daniel Vetter]

On Fri, Jun 7, 2013 at 2:47 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On random machines, the BIOS changes the hardware state of the display
> when it detects a lid event. In return, we have to then do a forced
> restore of the users requested configuration as soon as we handle the
> lid event. Currently, this is done by calling the lowlevel CRTC
> configuration functions directly with the old state. This results in
> a flash on all connected displays. However, since we probe the state of
> the hardware left by the BIOS, we can instead hook into the higher level
> modeset code which evaluates the minimum changes required to setup the
> requested state. This converts the modeset back into a no-op on sane
> machines, and eliminates the screen blanking.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65486
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

Hm, my Grand Plan was to actually move into the other direction, i.e.
shovel all the modeset compare logic into the lower levels (that's
what the different pipe masks have been for) and in addition switch
from comparing a few important things like the requested
mode/fb->pixel format to comparing computed pipe configs. That way we
can fully subsume fastboot into any other modeset (and also subsume
such "fixup bios damage" like this one).

Essentially my plan is to pimp intel_modeset_affected_pipes to not
just look at the output routing, but also compare the pipe config.
That means we need to move it past the pipe config compute stage and
also have a crtc->new_config pointer for each crtc (pointing to the
old one for all other crtcs). Then we could drop the HACK at the end
of intel_modeset_affected_pipes and instead just compare pipe configs.

Problem is that we still lack a few essential pieces to not miss a
modeset update. To paper over that we could use the recently added
pipe_config->quirk field and set the PIPE_CONFIG_QUIRK_NO_FASTBOOT
flag in intel_set_mode. The lid updater here only calls
__intel_set_mode so would get the good treatment already. And
intel_pipe_config_compare would also need to grow some smarts (and
maybe a flag parameter to control a few things betweent the fast
modeset compare mode and the check state compare mode.

Anyway, just wanted to dump my ideas about this, your patch here looks
simple enough as a short-term stopgap measure.
-Daniel


> ---
>  drivers/gpu/drm/i915/intel_display.c |   26 ++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |    5 +++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba7e311..5b727a3b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9632,11 +9632,33 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                  * checking (bogus) intermediate states.
>                  */
>                 for_each_pipe(pipe) {
> +                       struct drm_mode_set set;
> +                       struct drm_connector *connector, *connectors[16];
>                         struct drm_crtc *crtc =
>                                 dev_priv->pipe_to_crtc_mapping[pipe];
>
> -                       __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> -                                        crtc->fb);
> +                       set.crtc = crtc;
> +                       set.x = crtc->x;
> +                       set.y = crtc->y;
> +                       set.mode = crtc->fb ? &crtc->mode : NULL;
> +                       set.fb = crtc->fb;
> +                       set.connectors = connectors;
> +                       set.num_connectors = 0;
> +
> +                       list_for_each_entry(connector,
> +                                           &dev->mode_config.connector_list,
> +                                           head) {
> +                               if (&intel_attached_crtc(connector)->base != crtc)
> +                                       continue;
> +
> +                               connectors[set.num_connectors++] = connector;
> +                               if (set.num_connectors == ARRAY_SIZE(connectors))
> +                                       break;
> +                       }
> +
> +                       if (intel_crtc_set_config(&set))
> +                               __intel_set_mode(crtc, &crtc->mode,
> +                                                crtc->x, crtc->y, crtc->fb);
>                 }
>                 list_for_each_entry(plane, &dev->mode_config.plane_list, head)
>                         intel_plane_restore(plane);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1f04b7f..ef3f809 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -648,6 +648,11 @@ static inline struct intel_encoder *intel_attached_encoder(struct drm_connector
>         return to_intel_connector(connector)->encoder;
>  }
>
> +static inline struct intel_crtc *intel_attached_crtc(struct drm_connector *connector)
> +{
> +       return to_intel_crtc(to_intel_connector(connector)->encoder->base.crtc);
> +}
> +
>  static inline struct intel_digital_port *
>  enc_to_dig_port(struct drm_encoder *encoder)
>  {
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch