Commit 94352cf9 authored by Daniel Vetter's avatar Daniel Vetter

drm/i915: push crtc->fb update into pipe_set_base

Passing in the old fb, having overwritten the current fb, leads to
some neatly convoluted code. It's much simpler if we defer the
crtc->fb update to the place that updates the hw, in pipe_set_base.
This way we also don't need to restore anything in case something
fails - we only update crtc->fb once things have succeeded.

The real reason for this change is that now we keep the old fb
assigned to crtc->fb, which allows us to finally move the crtc disable
case into the common low-level set_mode function in the next patch.

Also don't clobber crtc->x and crtc->y, we neatly pass these down the
callchain already. Unfortunately we can't do the same with crtc->mode,
because that one is being used in the mode_set callbacks.

v2: Don't restore the drm_crtc object any more on failed modesets,
since we've lose an fb reference otherwise. Also (and this is the
reason this has been found), this totally confused the modeset state
tracking, since it clobbers crtc->enabled. Issue reported by Paulo
Zanoni.

v3: Rip out the entire crtc saving into struct intel_set_config, not
just the restoring part.
Reviewed-by: default avatarJesse Barnes <jbarnes@virtuousgeek.org>
Signed-Off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent 9a935856
...@@ -2201,16 +2201,17 @@ intel_finish_fb(struct drm_framebuffer *old_fb) ...@@ -2201,16 +2201,17 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
static int static int
intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
struct drm_framebuffer *old_fb) struct drm_framebuffer *fb)
{ {
struct drm_device *dev = crtc->dev; struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_master_private *master_priv; struct drm_i915_master_private *master_priv;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct drm_framebuffer *old_fb;
int ret; int ret;
/* no fb bound */ /* no fb bound */
if (!crtc->fb) { if (!fb) {
DRM_ERROR("No FB bound\n"); DRM_ERROR("No FB bound\n");
return 0; return 0;
} }
...@@ -2224,7 +2225,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, ...@@ -2224,7 +2225,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
mutex_lock(&dev->struct_mutex); mutex_lock(&dev->struct_mutex);
ret = intel_pin_and_fence_fb_obj(dev, ret = intel_pin_and_fence_fb_obj(dev,
to_intel_framebuffer(crtc->fb)->obj, to_intel_framebuffer(fb)->obj,
NULL); NULL);
if (ret != 0) { if (ret != 0) {
mutex_unlock(&dev->struct_mutex); mutex_unlock(&dev->struct_mutex);
...@@ -2232,17 +2233,20 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, ...@@ -2232,17 +2233,20 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
return ret; return ret;
} }
if (old_fb) if (crtc->fb)
intel_finish_fb(old_fb); intel_finish_fb(crtc->fb);
ret = dev_priv->display.update_plane(crtc, crtc->fb, x, y); ret = dev_priv->display.update_plane(crtc, fb, x, y);
if (ret) { if (ret) {
intel_unpin_fb_obj(to_intel_framebuffer(crtc->fb)->obj); intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
mutex_unlock(&dev->struct_mutex); mutex_unlock(&dev->struct_mutex);
DRM_ERROR("failed to update base address\n"); DRM_ERROR("failed to update base address\n");
return ret; return ret;
} }
old_fb = crtc->fb;
crtc->fb = fb;
if (old_fb) { if (old_fb) {
intel_wait_for_vblank(dev, intel_crtc->pipe); intel_wait_for_vblank(dev, intel_crtc->pipe);
intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj); intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
...@@ -3777,6 +3781,7 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) ...@@ -3777,6 +3781,7 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
* true if they don't match). * true if they don't match).
*/ */
static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
unsigned int *pipe_bpp, unsigned int *pipe_bpp,
struct drm_display_mode *mode) struct drm_display_mode *mode)
{ {
...@@ -3846,7 +3851,7 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, ...@@ -3846,7 +3851,7 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
* also stays within the max display bpc discovered above. * also stays within the max display bpc discovered above.
*/ */
switch (crtc->fb->depth) { switch (fb->depth) {
case 8: case 8:
bpc = 8; /* since we go through a colormap */ bpc = 8; /* since we go through a colormap */
break; break;
...@@ -4265,7 +4270,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ...@@ -4265,7 +4270,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
struct drm_display_mode *mode, struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode, struct drm_display_mode *adjusted_mode,
int x, int y, int x, int y,
struct drm_framebuffer *old_fb) struct drm_framebuffer *fb)
{ {
struct drm_device *dev = crtc->dev; struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_private *dev_priv = dev->dev_private;
...@@ -4455,7 +4460,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ...@@ -4455,7 +4460,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
I915_WRITE(DSPCNTR(plane), dspcntr); I915_WRITE(DSPCNTR(plane), dspcntr);
POSTING_READ(DSPCNTR(plane)); POSTING_READ(DSPCNTR(plane));
ret = intel_pipe_set_base(crtc, x, y, old_fb); ret = intel_pipe_set_base(crtc, x, y, fb);
intel_update_watermarks(dev); intel_update_watermarks(dev);
...@@ -4613,7 +4618,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ...@@ -4613,7 +4618,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
struct drm_display_mode *mode, struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode, struct drm_display_mode *adjusted_mode,
int x, int y, int x, int y,
struct drm_framebuffer *old_fb) struct drm_framebuffer *fb)
{ {
struct drm_device *dev = crtc->dev; struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_private *dev_priv = dev->dev_private;
...@@ -4733,7 +4738,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ...@@ -4733,7 +4738,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
/* determine panel color depth */ /* determine panel color depth */
temp = I915_READ(PIPECONF(pipe)); temp = I915_READ(PIPECONF(pipe));
temp &= ~PIPE_BPC_MASK; temp &= ~PIPE_BPC_MASK;
dither = intel_choose_pipe_bpp_dither(crtc, &pipe_bpp, mode); dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
switch (pipe_bpp) { switch (pipe_bpp) {
case 18: case 18:
temp |= PIPE_6BPC; temp |= PIPE_6BPC;
...@@ -5002,7 +5007,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ...@@ -5002,7 +5007,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
I915_WRITE(DSPCNTR(plane), dspcntr); I915_WRITE(DSPCNTR(plane), dspcntr);
POSTING_READ(DSPCNTR(plane)); POSTING_READ(DSPCNTR(plane));
ret = intel_pipe_set_base(crtc, x, y, old_fb); ret = intel_pipe_set_base(crtc, x, y, fb);
intel_update_watermarks(dev); intel_update_watermarks(dev);
...@@ -5015,7 +5020,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, ...@@ -5015,7 +5020,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
struct drm_display_mode *mode, struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode, struct drm_display_mode *adjusted_mode,
int x, int y, int x, int y,
struct drm_framebuffer *old_fb) struct drm_framebuffer *fb)
{ {
struct drm_device *dev = crtc->dev; struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_private *dev_priv = dev->dev_private;
...@@ -5026,7 +5031,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, ...@@ -5026,7 +5031,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
drm_vblank_pre_modeset(dev, pipe); drm_vblank_pre_modeset(dev, pipe);
ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode, ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
x, y, old_fb); x, y, fb);
drm_vblank_post_modeset(dev, pipe); drm_vblank_post_modeset(dev, pipe);
return ret; return ret;
...@@ -5718,7 +5723,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, ...@@ -5718,7 +5723,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
struct drm_encoder *encoder = &intel_encoder->base; struct drm_encoder *encoder = &intel_encoder->base;
struct drm_crtc *crtc = NULL; struct drm_crtc *crtc = NULL;
struct drm_device *dev = encoder->dev; struct drm_device *dev = encoder->dev;
struct drm_framebuffer *old_fb; struct drm_framebuffer *fb;
int i = -1; int i = -1;
DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
...@@ -5779,8 +5784,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, ...@@ -5779,8 +5784,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
if (!mode) if (!mode)
mode = &load_detect_mode; mode = &load_detect_mode;
old_fb = crtc->fb;
/* We need a framebuffer large enough to accommodate all accesses /* We need a framebuffer large enough to accommodate all accesses
* that the plane may generate whilst we perform load detection. * that the plane may generate whilst we perform load detection.
* We can not rely on the fbcon either being present (we get called * We can not rely on the fbcon either being present (we get called
...@@ -5788,19 +5791,19 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, ...@@ -5788,19 +5791,19 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
* not even exist) or that it is large enough to satisfy the * not even exist) or that it is large enough to satisfy the
* requested mode. * requested mode.
*/ */
crtc->fb = mode_fits_in_fbdev(dev, mode); fb = mode_fits_in_fbdev(dev, mode);
if (crtc->fb == NULL) { if (fb == NULL) {
DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
crtc->fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32); fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
old->release_fb = crtc->fb; old->release_fb = fb;
} else } else
DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n"); DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
if (IS_ERR(crtc->fb)) { if (IS_ERR(fb)) {
DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n"); DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
goto fail; goto fail;
} }
if (!intel_set_mode(crtc, mode, 0, 0, old_fb)) { if (!intel_set_mode(crtc, mode, 0, 0, fb)) {
DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
if (old->release_fb) if (old->release_fb)
old->release_fb->funcs->destroy(old->release_fb); old->release_fb->funcs->destroy(old->release_fb);
...@@ -5814,7 +5817,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, ...@@ -5814,7 +5817,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
fail: fail:
connector->encoder = NULL; connector->encoder = NULL;
encoder->crtc = NULL; encoder->crtc = NULL;
crtc->fb = old_fb;
return false; return false;
} }
...@@ -6666,13 +6668,12 @@ static void intel_modeset_commit_output_state(struct drm_device *dev) ...@@ -6666,13 +6668,12 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
bool intel_set_mode(struct drm_crtc *crtc, bool intel_set_mode(struct drm_crtc *crtc,
struct drm_display_mode *mode, struct drm_display_mode *mode,
int x, int y, struct drm_framebuffer *old_fb) int x, int y, struct drm_framebuffer *fb)
{ {
struct drm_device *dev = crtc->dev; struct drm_device *dev = crtc->dev;
drm_i915_private_t *dev_priv = dev->dev_private; drm_i915_private_t *dev_priv = dev->dev_private;
struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode;
struct drm_encoder_helper_funcs *encoder_funcs; struct drm_encoder_helper_funcs *encoder_funcs;
int saved_x, saved_y;
struct drm_encoder *encoder; struct drm_encoder *encoder;
bool ret = true; bool ret = true;
...@@ -6686,15 +6687,11 @@ bool intel_set_mode(struct drm_crtc *crtc, ...@@ -6686,15 +6687,11 @@ bool intel_set_mode(struct drm_crtc *crtc,
saved_hwmode = crtc->hwmode; saved_hwmode = crtc->hwmode;
saved_mode = crtc->mode; saved_mode = crtc->mode;
saved_x = crtc->x;
saved_y = crtc->y;
/* Update crtc values up front so the driver can rely on them for mode /* Update crtc values up front so the driver can rely on them for mode
* setting. * setting.
*/ */
crtc->mode = *mode; crtc->mode = *mode;
crtc->x = x;
crtc->y = y;
/* Pass our mode to the connectors and the CRTC to give them a chance to /* Pass our mode to the connectors and the CRTC to give them a chance to
* adjust it according to limitations or connector properties, and also * adjust it according to limitations or connector properties, and also
...@@ -6725,7 +6722,7 @@ bool intel_set_mode(struct drm_crtc *crtc, ...@@ -6725,7 +6722,7 @@ bool intel_set_mode(struct drm_crtc *crtc,
/* Set up the DPLL and any encoders state that needs to adjust or depend /* Set up the DPLL and any encoders state that needs to adjust or depend
* on the DPLL. * on the DPLL.
*/ */
ret = !intel_crtc_mode_set(crtc, mode, adjusted_mode, x, y, old_fb); ret = !intel_crtc_mode_set(crtc, mode, adjusted_mode, x, y, fb);
if (!ret) if (!ret)
goto done; goto done;
...@@ -6741,6 +6738,9 @@ bool intel_set_mode(struct drm_crtc *crtc, ...@@ -6741,6 +6738,9 @@ bool intel_set_mode(struct drm_crtc *crtc,
encoder_funcs->mode_set(encoder, mode, adjusted_mode); 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. */ /* Now enable the clocks, plane, pipe, and connectors that we set up. */
dev_priv->display.crtc_enable(crtc); dev_priv->display.crtc_enable(crtc);
...@@ -6759,8 +6759,6 @@ bool intel_set_mode(struct drm_crtc *crtc, ...@@ -6759,8 +6759,6 @@ bool intel_set_mode(struct drm_crtc *crtc,
if (!ret) { if (!ret) {
crtc->hwmode = saved_hwmode; crtc->hwmode = saved_hwmode;
crtc->mode = saved_mode; crtc->mode = saved_mode;
crtc->x = saved_x;
crtc->y = saved_y;
} }
return ret; return ret;
...@@ -6773,25 +6771,16 @@ static void intel_set_config_free(struct intel_set_config *config) ...@@ -6773,25 +6771,16 @@ static void intel_set_config_free(struct intel_set_config *config)
kfree(config->save_connector_encoders); kfree(config->save_connector_encoders);
kfree(config->save_encoder_crtcs); kfree(config->save_encoder_crtcs);
kfree(config->save_crtcs);
kfree(config); kfree(config);
} }
static int intel_set_config_save_state(struct drm_device *dev, static int intel_set_config_save_state(struct drm_device *dev,
struct intel_set_config *config) struct intel_set_config *config)
{ {
struct drm_crtc *crtc;
struct drm_encoder *encoder; struct drm_encoder *encoder;
struct drm_connector *connector; struct drm_connector *connector;
int count; int count;
/* Allocate space for the backup of all (non-pointer) crtc, encoder and
* connector data. */
config->save_crtcs = kcalloc(dev->mode_config.num_crtc,
sizeof(struct drm_crtc), GFP_KERNEL);
if (!config->save_crtcs)
return -ENOMEM;
config->save_encoder_crtcs = config->save_encoder_crtcs =
kcalloc(dev->mode_config.num_encoder, kcalloc(dev->mode_config.num_encoder,
sizeof(struct drm_crtc *), GFP_KERNEL); sizeof(struct drm_crtc *), GFP_KERNEL);
...@@ -6808,11 +6797,6 @@ static int intel_set_config_save_state(struct drm_device *dev, ...@@ -6808,11 +6797,6 @@ static int intel_set_config_save_state(struct drm_device *dev,
* Should anything bad happen only the expected state is * Should anything bad happen only the expected state is
* restored, not the drivers personal bookkeeping. * restored, not the drivers personal bookkeeping.
*/ */
count = 0;
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
config->save_crtcs[count++] = *crtc;
}
count = 0; count = 0;
list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
config->save_encoder_crtcs[count++] = encoder->crtc; config->save_encoder_crtcs[count++] = encoder->crtc;
...@@ -6829,16 +6813,10 @@ static int intel_set_config_save_state(struct drm_device *dev, ...@@ -6829,16 +6813,10 @@ static int intel_set_config_save_state(struct drm_device *dev,
static void intel_set_config_restore_state(struct drm_device *dev, static void intel_set_config_restore_state(struct drm_device *dev,
struct intel_set_config *config) struct intel_set_config *config)
{ {
struct drm_crtc *crtc;
struct intel_encoder *encoder; struct intel_encoder *encoder;
struct intel_connector *connector; struct intel_connector *connector;
int count; int count;
count = 0;
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
*crtc = config->save_crtcs[count++];
}
count = 0; count = 0;
list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
encoder->new_crtc = encoder->new_crtc =
...@@ -6994,7 +6972,6 @@ intel_modeset_stage_output_state(struct drm_device *dev, ...@@ -6994,7 +6972,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
static int intel_crtc_set_config(struct drm_mode_set *set) static int intel_crtc_set_config(struct drm_mode_set *set)
{ {
struct drm_device *dev; struct drm_device *dev;
struct drm_framebuffer *old_fb = NULL;
struct drm_mode_set save_set; struct drm_mode_set save_set;
struct intel_set_config *config; struct intel_set_config *config;
int ret; int ret;
...@@ -7057,13 +7034,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set) ...@@ -7057,13 +7034,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
DRM_DEBUG_KMS("attempting to set mode from" DRM_DEBUG_KMS("attempting to set mode from"
" userspace\n"); " userspace\n");
drm_mode_debug_printmodeline(set->mode); drm_mode_debug_printmodeline(set->mode);
old_fb = set->crtc->fb;
set->crtc->fb = set->fb;
if (!intel_set_mode(set->crtc, set->mode, if (!intel_set_mode(set->crtc, set->mode,
set->x, set->y, old_fb)) { set->x, set->y, set->fb)) {
DRM_ERROR("failed to set mode on [CRTC:%d]\n", DRM_ERROR("failed to set mode on [CRTC:%d]\n",
set->crtc->base.id); set->crtc->base.id);
set->crtc->fb = old_fb;
ret = -EINVAL; ret = -EINVAL;
goto fail; goto fail;
} }
...@@ -7076,18 +7050,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set) ...@@ -7076,18 +7050,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
} }
drm_helper_disable_unused_functions(dev); drm_helper_disable_unused_functions(dev);
} else if (config->fb_changed) { } else if (config->fb_changed) {
set->crtc->x = set->x;
set->crtc->y = set->y;
old_fb = set->crtc->fb;
if (set->crtc->fb != set->fb)
set->crtc->fb = set->fb;
ret = intel_pipe_set_base(set->crtc, ret = intel_pipe_set_base(set->crtc,
set->x, set->y, old_fb); set->x, set->y, set->fb);
if (ret != 0) {
set->crtc->fb = old_fb;
goto fail;
}
} }
intel_set_config_free(config); intel_set_config_free(config);
......
...@@ -442,7 +442,6 @@ extern enum drm_connector_status intel_panel_detect(struct drm_device *dev); ...@@ -442,7 +442,6 @@ extern enum drm_connector_status intel_panel_detect(struct drm_device *dev);
struct intel_set_config { struct intel_set_config {
struct drm_encoder **save_connector_encoders; struct drm_encoder **save_connector_encoders;
struct drm_crtc **save_encoder_crtcs; struct drm_crtc **save_encoder_crtcs;
struct drm_crtc *save_crtcs;
bool fb_changed; bool fb_changed;
bool mode_changed; bool mode_changed;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment