On Fri, Apr 10, 2015 at 11:38:36AM +0300, Ander Conselvan de Oliveira wrote:
> Set the mode_changed field on the crtc_states and use that instead.
>
> Note that even though this patch doesn't completely replace the logic in
> intel_modeset_affected_pipes(), that logic was never fully used to its
> full extent. Since the commit mentioned below, modeset_pipes and
> prepare_pipes would only contain at most the pipe for which the set_crtc
> ioctl was called. We can grow back that logic when the time comes.
>
> commit b6c5164d7bf624f3e1b750787ddb983150c5117c
> Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Date: Fri Apr 12 18:48:43 2013 +0200
>
> drm/i915: Fixup Oops in the pipe config computation
Bikeshed: Instead of switching to pipe_config I'd just switch to the
overall state for these functions. Or is there something preventing this?
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 +
> drivers/gpu/drm/i915/intel_display.c | 287 ++++++++++++++---------------------
> 2 files changed, 126 insertions(+), 169 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e12bc5a..08ea6d0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -272,6 +272,14 @@ enum hpd_pin {
> (__i)++) \
> if (connector)
>
> +#define for_each_intel_crtc_in_state(state, crtc, crtc_state, __i) \
> + for ((__i) = 0; \
> + (crtc) = to_intel_crtc((state)->crtcs[__i]), \
> + (crtc_state) = to_intel_crtc_state((state)->crtc_states[__i]), \
> + (__i) < (state)->dev->mode_config.num_crtc; \
> + (__i)++) \
> + if (crtc_state)
> +
> struct drm_i915_private;
> struct i915_mm_struct;
> struct i915_mmu_object;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 232ef56..a255b24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5179,12 +5179,13 @@ static int intel_mode_max_pixclk(struct drm_atomic_state *state)
> return max_pixclk;
> }
>
> -static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
> - unsigned *prepare_pipes)
> +static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_crtc *intel_crtc;
> + struct intel_crtc_state *crtc_state;
> int max_pixclk = intel_mode_max_pixclk(state);
> + int i;
>
> if (max_pixclk < 0)
> return max_pixclk;
> @@ -5193,10 +5194,20 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
> dev_priv->vlv_cdclk_freq)
> return 0;
>
> + /* add all active pipes to the state */
> + for_each_intel_crtc(state->dev, intel_crtc) {
> + if (!intel_crtc->base.state->enable)
> + continue;
> +
> + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> + }
> +
> /* disable/enable all currently active pipes while we change cdclk */
> - for_each_intel_crtc(state->dev, intel_crtc)
> - if (intel_crtc->base.state->enable)
> - *prepare_pipes |= (1 << intel_crtc->pipe);
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> + if (crtc_state->base.enable)
> + crtc_state->base.mode_changed = true;
>
> return 0;
> }
> @@ -10879,94 +10890,6 @@ fail:
> return ERR_PTR(ret);
> }
>
> -/* Computes which crtcs are affected and sets the relevant bits in the mask. For
> - * simplicity we use the crtc's pipe number (because it's easier to obtain). */
> -static void
> -intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
> - unsigned *prepare_pipes, unsigned *disable_pipes)
> -{
> - struct intel_crtc *intel_crtc;
> - struct drm_device *dev = crtc->dev;
> - struct intel_encoder *encoder;
> - struct intel_connector *connector;
> - struct drm_crtc *tmp_crtc;
> -
> - *disable_pipes = *modeset_pipes = *prepare_pipes = 0;
> -
> - /* Check which crtcs have changed outputs connected to them, these need
> - * to be part of the prepare_pipes mask. We don't (yet) support global
> - * modeset across multiple crtcs, so modeset_pipes will only have one
> - * bit set at most. */
> - for_each_intel_connector(dev, connector) {
> - if (connector->base.encoder == &connector->new_encoder->base)
> - continue;
> -
> - if (connector->base.encoder) {
> - tmp_crtc = connector->base.encoder->crtc;
> -
> - *prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> - }
> -
> - if (connector->new_encoder)
> - *prepare_pipes |=
> - 1 << connector->new_encoder->new_crtc->pipe;
> - }
> -
> - for_each_intel_encoder(dev, encoder) {
> - if (encoder->base.crtc == &encoder->new_crtc->base)
> - continue;
> -
> - if (encoder->base.crtc) {
> - tmp_crtc = encoder->base.crtc;
> -
> - *prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe;
> - }
> -
> - if (encoder->new_crtc)
> - *prepare_pipes |= 1 << encoder->new_crtc->pipe;
> - }
> -
> - /* Check for pipes that will be enabled/disabled ... */
> - for_each_intel_crtc(dev, intel_crtc) {
> - if (intel_crtc->base.state->enable == intel_crtc->new_enabled)
> - continue;
> -
> - if (!intel_crtc->new_enabled)
> - *disable_pipes |= 1 << intel_crtc->pipe;
> - else
> - *prepare_pipes |= 1 << intel_crtc->pipe;
> - }
> -
> -
> - /* set_mode is also used to update properties on life display pipes. */
> - intel_crtc = to_intel_crtc(crtc);
> - if (intel_crtc->new_enabled)
> - *prepare_pipes |= 1 << intel_crtc->pipe;
> -
> - /*
> - * For simplicity do a full modeset on any pipe where the output routing
> - * changed. We could be more clever, but that would require us to be
> - * more careful with calling the relevant encoder->mode_set functions.
> - */
> - if (*prepare_pipes)
> - *modeset_pipes = *prepare_pipes;
> -
> - /* ... and mask these out. */
> - *modeset_pipes &= ~(*disable_pipes);
> - *prepare_pipes &= ~(*disable_pipes);
> -
> - /*
> - * HACK: We don't (yet) fully support global modesets. intel_set_config
> - * obies this rule, but the modeset restore mode of
> - * intel_modeset_setup_hw_state does not.
> - */
> - *modeset_pipes &= 1 << intel_crtc->pipe;
> - *prepare_pipes &= 1 << intel_crtc->pipe;
> -
> - DRM_DEBUG_KMS("set mode pipe masks: modeset: %x, prepare: %x, disable: %x\n",
> - *modeset_pipes, *prepare_pipes, *disable_pipes);
> -}
> -
> static bool intel_crtc_in_use(struct drm_crtc *crtc)
> {
> struct drm_encoder *encoder;
> @@ -10979,13 +10902,22 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
> return false;
> }
>
> +static bool
> +needs_modeset(struct intel_crtc_state *state)
> +{
> + return state->base.mode_changed || state->base.active_changed;
> +}
> +
> static void
> -intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> +intel_modeset_update_state(struct drm_atomic_state *state)
> {
> + struct drm_device *dev = state->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_encoder *intel_encoder;
> struct intel_crtc *intel_crtc;
> + struct intel_crtc_state *crtc_state;
> struct drm_connector *connector;
> + int i;
>
> intel_shared_dpll_commit(dev_priv);
>
> @@ -10993,9 +10925,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> if (!intel_encoder->base.crtc)
> continue;
>
> - intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> + if (&intel_crtc->base == intel_encoder->base.crtc)
> + break;
> +
> + if (&intel_crtc->base != intel_encoder->base.crtc)
> + continue;
>
> - if (prepare_pipes & (1 << intel_crtc->pipe))
> + if (crtc_state->base.enable && needs_modeset(crtc_state))
> intel_encoder->connectors_active = false;
> }
>
> @@ -11010,9 +10947,14 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> if (!connector->encoder || !connector->encoder->crtc)
> continue;
>
> - intel_crtc = to_intel_crtc(connector->encoder->crtc);
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i)
> + if (&intel_crtc->base == connector->encoder->crtc)
> + break;
> +
> + if (&intel_crtc->base != connector->encoder->crtc)
> + continue;
>
> - if (prepare_pipes & (1 << intel_crtc->pipe)) {
> + if (crtc_state->base.enable && needs_modeset(crtc_state)) {
> struct drm_property *dpms_property =
> dev->mode_config.dpms_property;
>
> @@ -11547,26 +11489,41 @@ static void update_scanline_offset(struct intel_crtc *crtc)
> crtc->scanline_offset = 1;
> }
>
> +static void
> +intel_atomic_modeset_compute_changed_flags(struct drm_atomic_state *state,
> + struct drm_crtc *modeset_crtc)
> +{
> + struct intel_crtc_state *crtc_state;
> + struct intel_crtc *crtc;
> + int i;
> +
> + for_each_intel_crtc_in_state(state, crtc, crtc_state, i) {
> + if (crtc_state->base.enable != crtc->base.state->enable)
> + crtc_state->base.mode_changed = true;
> +
> + /* FIXME: move comment about changing properties in live pipes
> + * from affectec_pipes() here */
> + if (&crtc->base == modeset_crtc && crtc_state->base.enable)
> + crtc_state->base.mode_changed = true;
> + }
> +}
> +
> static struct intel_crtc_state *
> intel_modeset_compute_config(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> struct drm_framebuffer *fb,
> - struct drm_atomic_state *state,
> - unsigned *modeset_pipes,
> - unsigned *prepare_pipes,
> - unsigned *disable_pipes)
> + struct drm_atomic_state *state)
> {
> - struct drm_device *dev = crtc->dev;
> - struct intel_crtc_state *pipe_config = NULL;
> + struct intel_crtc_state *crtc_state;
> struct intel_crtc *intel_crtc;
> int ret = 0;
> + int i;
>
> ret = drm_atomic_add_affected_connectors(state, crtc);
> if (ret)
> return ERR_PTR(ret);
>
> - intel_modeset_affected_pipes(crtc, modeset_pipes,
> - prepare_pipes, disable_pipes);
> + intel_atomic_modeset_compute_changed_flags(state, crtc);
>
> /*
> * Note this needs changes when we start tracking multiple modes
> @@ -11574,46 +11531,49 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
> * (i.e. one pipe_config for each crtc) rather than just the one
> * for this crtc.
> */
> - for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
> - /* FIXME: For now we still expect modeset_pipes has at most
> - * one bit set. */
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> + if (!crtc_state->base.enable || !needs_modeset(crtc_state))
> + continue;
> +
> + /* FIXME: For now we still expect modeset on only one pipe. */
> if (WARN_ON(&intel_crtc->base != crtc))
> continue;
>
> - pipe_config = intel_modeset_pipe_config(crtc, fb, mode, state);
> - if (IS_ERR(pipe_config))
> - return pipe_config;
> + crtc_state = intel_modeset_pipe_config(crtc, fb, mode, state);
> + if (IS_ERR(crtc_state))
> + return crtc_state;
>
> - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> + intel_dump_pipe_config(to_intel_crtc(crtc), crtc_state,
> "[modeset]");
> }
>
> return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));;
> }
>
> -static int __intel_set_mode_setup_plls(struct drm_atomic_state *state,
> - unsigned modeset_pipes,
> - unsigned disable_pipes)
> +static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
> {
> struct drm_device *dev = state->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - unsigned clear_pipes = modeset_pipes | disable_pipes;
> + unsigned clear_pipes = 0;
> struct intel_crtc *intel_crtc;
> + struct intel_crtc_state *crtc_state;
> int ret = 0;
> + int i;
>
> if (!dev_priv->display.crtc_compute_clock)
> return 0;
>
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> + if (needs_modeset(crtc_state))
> + clear_pipes |= 1 << intel_crtc->pipe;
> + }
> +
> ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
> if (ret)
> goto done;
>
> - for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> - struct intel_crtc_state *crtc_state =
> - intel_atomic_get_crtc_state(state, intel_crtc);
> -
> - /* Modeset pipes should have a new state by now */
> - if (WARN_ON(IS_ERR(crtc_state)))
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> + if (!crtc_state->base.enable)
> continue;
>
> ret = dev_priv->display.crtc_compute_clock(intel_crtc,
> @@ -11631,10 +11591,7 @@ done:
> static int __intel_set_mode(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> int x, int y, struct drm_framebuffer *fb,
> - struct intel_crtc_state *pipe_config,
> - unsigned modeset_pipes,
> - unsigned prepare_pipes,
> - unsigned disable_pipes)
> + struct intel_crtc_state *pipe_config)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -11642,7 +11599,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> struct drm_atomic_state *state = pipe_config->base.state;
> struct intel_crtc_state *crtc_state_copy = NULL;
> struct intel_crtc *intel_crtc;
> + struct intel_crtc_state *crtc_state;
> int ret = 0;
> + int i;
>
> saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
> if (!saved_mode)
> @@ -11664,23 +11623,22 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> * adjusted_mode bits in the crtc directly.
> */
> if (IS_VALLEYVIEW(dev)) {
> - ret = valleyview_modeset_global_pipes(state, &prepare_pipes);
> + ret = valleyview_modeset_global_pipes(state);
> if (ret)
> goto done;
> -
> - /* may have added more to prepare_pipes than we should */
> - prepare_pipes &= ~disable_pipes;
> }
>
> - ret = __intel_set_mode_setup_plls(state, modeset_pipes, disable_pipes);
> + ret = __intel_set_mode_setup_plls(state);
> if (ret)
> goto done;
>
> - for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> - intel_crtc_disable(&intel_crtc->base);
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> + if (!needs_modeset(crtc_state))
> + continue;
>
> - for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> - if (intel_crtc->base.state->enable)
> + if (!crtc_state->base.enable)
> + intel_crtc_disable(&intel_crtc->base);
> + else if (intel_crtc->base.state->enable)
> dev_priv->display.crtc_disable(&intel_crtc->base);
> }
>
> @@ -11691,7 +11649,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> * pipes; here we assume a single modeset_pipe and only track the
> * single crtc and mode.
> */
> - if (modeset_pipes) {
> + if (pipe_config->base.enable && needs_modeset(pipe_config)) {
> crtc->mode = *mode;
> /* mode_set/enable/disable functions rely on a correct pipe
> * config. */
> @@ -11708,16 +11666,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>
> /* Only after disabling all output pipelines that will be changed can we
> * update the the output configuration. */
> - intel_modeset_update_state(dev, prepare_pipes);
> + intel_modeset_update_state(state);
>
> modeset_update_crtc_power_domains(state);
>
> - for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> - struct drm_plane *primary = intel_crtc->base.primary;
> + if (pipe_config->base.enable && needs_modeset(pipe_config)) {
> + struct drm_plane *primary;
> int vdisplay, hdisplay;
>
> + intel_crtc = to_intel_crtc(crtc);
> + primary = intel_crtc->base.primary;
> +
> drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> - ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> +
> + ret = primary->funcs->update_plane(primary, crtc,
> fb, 0, 0,
> hdisplay, vdisplay,
> x << 16, y << 16,
> @@ -11725,7 +11687,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> }
>
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> - for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> + for_each_intel_crtc_in_state(state, intel_crtc, crtc_state, i) {
> + if (!crtc_state->base.enable)
> + continue;
> +
> update_scanline_offset(intel_crtc);
>
> dev_priv->display.crtc_enable(&intel_crtc->base);
> @@ -11753,18 +11718,14 @@ done:
> return ret;
> }
>
> -static int intel_set_mode_pipes(struct drm_crtc *crtc,
> - struct drm_display_mode *mode,
> - int x, int y, struct drm_framebuffer *fb,
> - struct intel_crtc_state *pipe_config,
> - unsigned modeset_pipes,
> - unsigned prepare_pipes,
> - unsigned disable_pipes)
> +static int intel_set_mode_with_config(struct drm_crtc *crtc,
> + struct drm_display_mode *mode,
> + int x, int y, struct drm_framebuffer *fb,
> + struct intel_crtc_state *pipe_config)
> {
> int ret;
>
> - ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config, modeset_pipes,
> - prepare_pipes, disable_pipes);
> + ret = __intel_set_mode(crtc, mode, x, y, fb, pipe_config);
>
> if (ret == 0)
> intel_modeset_check_state(crtc->dev);
> @@ -11778,22 +11739,15 @@ static int intel_set_mode(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> struct intel_crtc_state *pipe_config;
> - unsigned modeset_pipes, prepare_pipes, disable_pipes;
> int ret = 0;
>
> - pipe_config = intel_modeset_compute_config(crtc, mode, fb, state,
> - &modeset_pipes,
> - &prepare_pipes,
> - &disable_pipes);
> -
> + pipe_config = intel_modeset_compute_config(crtc, mode, fb, state);
> if (IS_ERR(pipe_config)) {
> ret = PTR_ERR(pipe_config);
> goto out;
> }
>
> - ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> - modeset_pipes, prepare_pipes,
> - disable_pipes);
> + ret = intel_set_mode_with_config(crtc, mode, x, y, fb, pipe_config);
> if (ret)
> goto out;
>
> @@ -12217,7 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> struct drm_atomic_state *state = NULL;
> struct intel_set_config *config;
> struct intel_crtc_state *pipe_config;
> - unsigned modeset_pipes, prepare_pipes, disable_pipes;
> int ret;
>
> BUG_ON(!set);
> @@ -12272,10 +12225,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> intel_set_config_compute_mode_changes(set, config);
>
> pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
> - set->fb, state,
> - &modeset_pipes,
> - &prepare_pipes,
> - &disable_pipes);
> + set->fb, state);
> if (IS_ERR(pipe_config)) {
> ret = PTR_ERR(pipe_config);
> goto fail;
> @@ -12295,10 +12245,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> intel_update_pipe_size(to_intel_crtc(set->crtc));
>
> if (config->mode_changed) {
> - ret = intel_set_mode_pipes(set->crtc, set->mode,
> - set->x, set->y, set->fb, pipe_config,
> - modeset_pipes, prepare_pipes,
> - disable_pipes);
> + ret = intel_set_mode_with_config(set->crtc, set->mode,
> + set->x, set->y, set->fb,
> + pipe_config);
> } else if (config->fb_changed) {
> struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> struct drm_plane *primary = set->crtc->primary;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
