On Sun, 19 Aug 2012 21:13:01 +0200 Daniel Vetter <[email protected]> wrote:
> 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. > > Signed-Off-by: Daniel Vetter <[email protected]> > --- > drivers/gpu/drm/i915/intel_display.c | 84 > +++++++++++++++--------------------- > 1 file changed, 34 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e6701b4..cd3606c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2201,16 +2201,17 @@ intel_finish_fb(struct drm_framebuffer *old_fb) > > static int > 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_i915_private *dev_priv = dev->dev_private; > struct drm_i915_master_private *master_priv; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_framebuffer *old_fb; > int ret; > > /* no fb bound */ > - if (!crtc->fb) { > + if (!fb) { > DRM_ERROR("No FB bound\n"); > return 0; > } > @@ -2224,7 +2225,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > mutex_lock(&dev->struct_mutex); > ret = intel_pin_and_fence_fb_obj(dev, > - to_intel_framebuffer(crtc->fb)->obj, > + to_intel_framebuffer(fb)->obj, > NULL); > if (ret != 0) { > mutex_unlock(&dev->struct_mutex); > @@ -2232,17 +2233,20 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int > y, > return ret; > } > > - if (old_fb) > - intel_finish_fb(old_fb); > + if (crtc->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) { > - intel_unpin_fb_obj(to_intel_framebuffer(crtc->fb)->obj); > + intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj); > mutex_unlock(&dev->struct_mutex); > DRM_ERROR("failed to update base address\n"); > return ret; > } > > + old_fb = crtc->fb; > + crtc->fb = fb; > + > if (old_fb) { > intel_wait_for_vblank(dev, intel_crtc->pipe); > 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) > * true if they don't match). > */ > static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > unsigned int *pipe_bpp, > struct drm_display_mode *mode) > { > @@ -3846,7 +3851,7 @@ static bool intel_choose_pipe_bpp_dither(struct > drm_crtc *crtc, > * also stays within the max display bpc discovered above. > */ > > - switch (crtc->fb->depth) { > + switch (fb->depth) { > case 8: > bpc = 8; /* since we go through a colormap */ > break; > @@ -4265,7 +4270,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > int x, int y, > - struct drm_framebuffer *old_fb) > + struct drm_framebuffer *fb) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4455,7 +4460,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > I915_WRITE(DSPCNTR(plane), dspcntr); > 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); > > @@ -4613,7 +4618,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > int x, int y, > - struct drm_framebuffer *old_fb) > + struct drm_framebuffer *fb) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4733,7 +4738,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > /* determine panel color depth */ > temp = I915_READ(PIPECONF(pipe)); > 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) { > case 18: > temp |= PIPE_6BPC; > @@ -5002,7 +5007,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > I915_WRITE(DSPCNTR(plane), dspcntr); > 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); > > @@ -5015,7 +5020,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > int x, int y, > - struct drm_framebuffer *old_fb) > + struct drm_framebuffer *fb) > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -5026,7 +5031,7 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, > drm_vblank_pre_modeset(dev, pipe); > > ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode, > - x, y, old_fb); > + x, y, fb); > drm_vblank_post_modeset(dev, pipe); > > return ret; > @@ -5718,7 +5723,7 @@ bool intel_get_load_detect_pipe(struct drm_connector > *connector, > struct drm_encoder *encoder = &intel_encoder->base; > struct drm_crtc *crtc = NULL; > struct drm_device *dev = encoder->dev; > - struct drm_framebuffer *old_fb; > + struct drm_framebuffer *fb; > int i = -1; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", > @@ -5779,8 +5784,6 @@ bool intel_get_load_detect_pipe(struct drm_connector > *connector, > if (!mode) > mode = &load_detect_mode; > > - old_fb = crtc->fb; > - > /* We need a framebuffer large enough to accommodate all accesses > * that the plane may generate whilst we perform load detection. > * 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, > * not even exist) or that it is large enough to satisfy the > * requested mode. > */ > - crtc->fb = mode_fits_in_fbdev(dev, mode); > - if (crtc->fb == NULL) { > + fb = mode_fits_in_fbdev(dev, mode); > + if (fb == NULL) { > DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); > - crtc->fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32); > - old->release_fb = crtc->fb; > + fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32); > + old->release_fb = fb; > } else > 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"); > 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"); > if (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, > fail: > connector->encoder = NULL; > encoder->crtc = NULL; > - crtc->fb = old_fb; > return false; > } > > @@ -6666,13 +6668,12 @@ static void intel_modeset_commit_output_state(struct > drm_device *dev) > > bool intel_set_mode(struct drm_crtc *crtc, > 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; > drm_i915_private_t *dev_priv = dev->dev_private; > struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; > struct drm_encoder_helper_funcs *encoder_funcs; > - int saved_x, saved_y; > struct drm_encoder *encoder; > bool ret = true; > > @@ -6686,15 +6687,11 @@ bool intel_set_mode(struct drm_crtc *crtc, > > saved_hwmode = crtc->hwmode; > 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 > * setting. > */ > crtc->mode = *mode; > - crtc->x = x; > - crtc->y = y; > > /* 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 > @@ -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 > * 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) > goto done; > > @@ -6741,6 +6738,9 @@ bool intel_set_mode(struct drm_crtc *crtc, > 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); > > @@ -6759,8 +6759,6 @@ done: > if (!ret) { > crtc->hwmode = saved_hwmode; > crtc->mode = saved_mode; > - crtc->x = saved_x; > - crtc->y = saved_y; > } > > return ret; > @@ -6993,7 +6991,6 @@ next_encoder: > static int intel_crtc_set_config(struct drm_mode_set *set) > { > struct drm_device *dev; > - struct drm_framebuffer *old_fb = NULL; > struct drm_mode_set save_set; > struct intel_set_config *config; > int ret; > @@ -7056,13 +7053,10 @@ static int intel_crtc_set_config(struct drm_mode_set > *set) > DRM_DEBUG_KMS("attempting to set mode from" > " userspace\n"); > drm_mode_debug_printmodeline(set->mode); > - old_fb = set->crtc->fb; > - set->crtc->fb = set->fb; > 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", > set->crtc->base.id); > - set->crtc->fb = old_fb; > ret = -EINVAL; > goto fail; > } > @@ -7075,18 +7069,8 @@ static int intel_crtc_set_config(struct drm_mode_set > *set) > } > drm_helper_disable_unused_functions(dev); > } 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, > - set->x, set->y, old_fb); > - if (ret != 0) { > - set->crtc->fb = old_fb; > - goto fail; > - } > + set->x, set->y, set->fb); > } > > intel_set_config_free(config); I never liked how we passed the old ones around and polluted the callees with knowledge of potential cleanup. Reviewed-by: Jesse Barnes <[email protected]> -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
