| Message ID | 56B0A591.1040801@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote: > drm/i915: Use atomic state to obtain load detection crtc, v2. > > Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate > an atomic state before the new state is committed, and commit it the old state > in intel_release_load_detect_pipe. > > Changes since v1: > - Use a real atomic state. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4d8c9f7857db..c7ba8f4a971e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev, > if (obj->base.size < mode->vdisplay * fb->pitches[0]) > return NULL; > > + drm_framebuffer_reference(fb); > return fb; > #else > return NULL; > @@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > struct drm_device *dev = encoder->dev; > struct drm_framebuffer *fb; > struct drm_mode_config *config = &dev->mode_config; > - struct drm_atomic_state *state = NULL; > + struct drm_atomic_state *state = NULL, *restore_state = NULL; > struct drm_connector_state *connector_state; > struct intel_crtc_state *crtc_state; > int ret, i = -1; > @@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > connector->base.id, connector->name, > encoder->base.id, encoder->name); > > + old->restore_state = NULL; > + > retry: > ret = drm_modeset_lock(&config->connection_mutex, ctx); > if (ret) > @@ -10441,24 +10444,15 @@ retry: > */ > > /* See if we already have a CRTC for this connector */ > - if (encoder->crtc) { > - crtc = encoder->crtc; > + if (connector->state->crtc) { > + crtc = connector->state->crtc; > > ret = drm_modeset_lock(&crtc->mutex, ctx); > if (ret) > goto fail; > - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > - if (ret) > - goto fail; > - > - old->dpms_mode = connector->dpms; > - old->load_detect_temp = false; > > /* Make sure the crtc and connector are running */ > - if (connector->dpms != DRM_MODE_DPMS_ON) > - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); > - > - return true; > + goto found; > } > > /* Find an unused one (if possible) */ > @@ -10466,8 +10460,15 @@ retry: > i++; > if (!(encoder->possible_crtcs & (1 << i))) > continue; > - if (possible_crtc->state->enable) > + > + ret = drm_modeset_lock(&possible_crtc->mutex, ctx); > + if (ret) > + goto fail; > + > + if (possible_crtc->state->enable) { > + drm_modeset_unlock(&possible_crtc->mutex); > continue; > + } > > crtc = possible_crtc; > break; > @@ -10481,23 +10482,22 @@ retry: > goto fail; > } > > - ret = drm_modeset_lock(&crtc->mutex, ctx); > - if (ret) > - goto fail; > +found: > + intel_crtc = to_intel_crtc(crtc); > + > ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > if (ret) > goto fail; > > - intel_crtc = to_intel_crtc(crtc); > - old->dpms_mode = connector->dpms; > - old->load_detect_temp = true; > - old->release_fb = NULL; > - > state = drm_atomic_state_alloc(dev); > - if (!state) > - return false; > + restore_state = drm_atomic_state_alloc(dev); > + if (!state || !restore_state) { > + ret = -ENOMEM; > + goto fail; > + } > > state->acquire_ctx = ctx; > + restore_state->acquire_ctx = ctx; > > connector_state = drm_atomic_get_connector_state(state, connector); > if (IS_ERR(connector_state)) { > @@ -10505,7 +10505,9 @@ retry: > goto fail; > } > > - connector_state->crtc = crtc; > + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc); > + if (ret) > + goto fail; > > crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > if (IS_ERR(crtc_state)) { > @@ -10529,7 +10531,6 @@ retry: > if (fb == NULL) { > DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); > 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(fb)) { > @@ -10541,15 +10542,28 @@ retry: > if (ret) > goto fail; > > - drm_mode_copy(&crtc_state->base.mode, mode); > + drm_framebuffer_unreference(fb); > + > + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); > + if (ret) > + goto fail; > + > + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); > + if (!ret) > + ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); > + if (!ret) > + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); > + if (ret) { > + DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); > + goto fail; > + } > > if (drm_atomic_commit(state)) { > DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); > - if (old->release_fb) > - old->release_fb->funcs->destroy(old->release_fb); > goto fail; > } > - crtc->primary->crtc = crtc; > + > + old->restore_state = restore_state; > > /* let the connector get through one full cycle before testing */ > intel_wait_for_vblank(dev, intel_crtc->pipe); > @@ -10557,7 +10571,8 @@ retry: > > fail: > drm_atomic_state_free(state); > - state = NULL; > + drm_atomic_state_free(restore_state); > + restore_state = state = NULL; > > if (ret == -EDEADLK) { > drm_modeset_backoff(ctx); > @@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > struct intel_load_detect_pipe *old, > struct drm_modeset_acquire_ctx *ctx) > { > - struct drm_device *dev = connector->dev; > struct intel_encoder *intel_encoder = > intel_attached_encoder(connector); > struct drm_encoder *encoder = &intel_encoder->base; > - struct drm_crtc *crtc = encoder->crtc; > + struct drm_crtc *crtc = connector->state->crtc; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct drm_atomic_state *state; > - struct drm_connector_state *connector_state; > + struct drm_atomic_state *state = old->restore_state; > struct intel_crtc_state *crtc_state; > int ret; > > @@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > connector->base.id, connector->name, > encoder->base.id, encoder->name); > > - if (old->load_detect_temp) { > - state = drm_atomic_state_alloc(dev); > - if (!state) > - goto fail; > - > - state->acquire_ctx = ctx; > - > - connector_state = drm_atomic_get_connector_state(state, connector); > - if (IS_ERR(connector_state)) > - goto fail; > - > - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > - if (IS_ERR(crtc_state)) > - goto fail; > - > - connector_state->crtc = NULL; > - > - crtc_state->base.enable = crtc_state->base.active = false; > - > - ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, > - 0, 0); > - if (ret) > - goto fail; > - > - ret = drm_atomic_commit(state); > - if (ret) > - goto fail; > - > - if (old->release_fb) { > - drm_framebuffer_unregister_private(old->release_fb); > - drm_framebuffer_unreference(old->release_fb); > - } > - > + if (!state) > return; > - } > > - /* Switch crtc and encoder back off if necessary */ > - if (old->dpms_mode != DRM_MODE_DPMS_ON) > - connector->funcs->dpms(connector, old->dpms_mode); > + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > + crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll; Hmm. What's this shared_dpll stuff? > > - return; > -fail: > - DRM_DEBUG_KMS("Couldn't release load detect pipe.\n"); > - drm_atomic_state_free(state); > + ret = drm_atomic_commit(state); > + if (ret) { > + DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); > + drm_atomic_state_free(state); > + } > } > > static int i9xx_pll_refclk(struct drm_device *dev, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 93ba14a3bb76..7d10ca7797a9 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -912,9 +912,7 @@ struct intel_unpin_work { > }; > > struct intel_load_detect_pipe { > - struct drm_framebuffer *release_fb; > - bool load_detect_temp; > - int dpms_mode; > + struct drm_atomic_state *restore_state; > }; > > static inline struct intel_encoder *
Op 02-02-16 om 18:32 schreef Ville Syrjälä: > On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote: >> drm/i915: Use atomic state to obtain load detection crtc, v2. >> >> Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate >> an atomic state before the new state is committed, and commit it the old state >> in intel_release_load_detect_pipe. >> >> Changes since v1: >> - Use a real atomic state. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 4d8c9f7857db..c7ba8f4a971e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev, >> if (obj->base.size < mode->vdisplay * fb->pitches[0]) >> return NULL; >> >> + drm_framebuffer_reference(fb); >> return fb; >> #else >> return NULL; >> @@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> struct drm_device *dev = encoder->dev; >> struct drm_framebuffer *fb; >> struct drm_mode_config *config = &dev->mode_config; >> - struct drm_atomic_state *state = NULL; >> + struct drm_atomic_state *state = NULL, *restore_state = NULL; >> struct drm_connector_state *connector_state; >> struct intel_crtc_state *crtc_state; >> int ret, i = -1; >> @@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> connector->base.id, connector->name, >> encoder->base.id, encoder->name); >> >> + old->restore_state = NULL; >> + >> retry: >> ret = drm_modeset_lock(&config->connection_mutex, ctx); >> if (ret) >> @@ -10441,24 +10444,15 @@ retry: >> */ >> >> /* See if we already have a CRTC for this connector */ >> - if (encoder->crtc) { >> - crtc = encoder->crtc; >> + if (connector->state->crtc) { >> + crtc = connector->state->crtc; >> >> ret = drm_modeset_lock(&crtc->mutex, ctx); >> if (ret) >> goto fail; >> - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); >> - if (ret) >> - goto fail; >> - >> - old->dpms_mode = connector->dpms; >> - old->load_detect_temp = false; >> >> /* Make sure the crtc and connector are running */ >> - if (connector->dpms != DRM_MODE_DPMS_ON) >> - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); >> - >> - return true; >> + goto found; >> } >> >> /* Find an unused one (if possible) */ >> @@ -10466,8 +10460,15 @@ retry: >> i++; >> if (!(encoder->possible_crtcs & (1 << i))) >> continue; >> - if (possible_crtc->state->enable) >> + >> + ret = drm_modeset_lock(&possible_crtc->mutex, ctx); >> + if (ret) >> + goto fail; >> + >> + if (possible_crtc->state->enable) { >> + drm_modeset_unlock(&possible_crtc->mutex); >> continue; >> + } >> >> crtc = possible_crtc; >> break; >> @@ -10481,23 +10482,22 @@ retry: >> goto fail; >> } >> >> - ret = drm_modeset_lock(&crtc->mutex, ctx); >> - if (ret) >> - goto fail; >> +found: >> + intel_crtc = to_intel_crtc(crtc); >> + >> ret = drm_modeset_lock(&crtc->primary->mutex, ctx); >> if (ret) >> goto fail; >> >> - intel_crtc = to_intel_crtc(crtc); >> - old->dpms_mode = connector->dpms; >> - old->load_detect_temp = true; >> - old->release_fb = NULL; >> - >> state = drm_atomic_state_alloc(dev); >> - if (!state) >> - return false; >> + restore_state = drm_atomic_state_alloc(dev); >> + if (!state || !restore_state) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> >> state->acquire_ctx = ctx; >> + restore_state->acquire_ctx = ctx; >> >> connector_state = drm_atomic_get_connector_state(state, connector); >> if (IS_ERR(connector_state)) { >> @@ -10505,7 +10505,9 @@ retry: >> goto fail; >> } >> >> - connector_state->crtc = crtc; >> + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc); >> + if (ret) >> + goto fail; >> >> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >> if (IS_ERR(crtc_state)) { >> @@ -10529,7 +10531,6 @@ retry: >> if (fb == NULL) { >> DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); >> 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(fb)) { >> @@ -10541,15 +10542,28 @@ retry: >> if (ret) >> goto fail; >> >> - drm_mode_copy(&crtc_state->base.mode, mode); >> + drm_framebuffer_unreference(fb); >> + >> + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); >> + if (ret) >> + goto fail; >> + >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); >> + if (!ret) >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); >> + if (!ret) >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); >> + if (ret) { >> + DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); >> + goto fail; >> + } >> >> if (drm_atomic_commit(state)) { >> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); >> - if (old->release_fb) >> - old->release_fb->funcs->destroy(old->release_fb); >> goto fail; >> } >> - crtc->primary->crtc = crtc; >> + >> + old->restore_state = restore_state; >> >> /* let the connector get through one full cycle before testing */ >> intel_wait_for_vblank(dev, intel_crtc->pipe); >> @@ -10557,7 +10571,8 @@ retry: >> >> fail: >> drm_atomic_state_free(state); >> - state = NULL; >> + drm_atomic_state_free(restore_state); >> + restore_state = state = NULL; >> >> if (ret == -EDEADLK) { >> drm_modeset_backoff(ctx); >> @@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >> struct intel_load_detect_pipe *old, >> struct drm_modeset_acquire_ctx *ctx) >> { >> - struct drm_device *dev = connector->dev; >> struct intel_encoder *intel_encoder = >> intel_attached_encoder(connector); >> struct drm_encoder *encoder = &intel_encoder->base; >> - struct drm_crtc *crtc = encoder->crtc; >> + struct drm_crtc *crtc = connector->state->crtc; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - struct drm_atomic_state *state; >> - struct drm_connector_state *connector_state; >> + struct drm_atomic_state *state = old->restore_state; >> struct intel_crtc_state *crtc_state; >> int ret; >> >> @@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >> connector->base.id, connector->name, >> encoder->base.id, encoder->name); >> >> - if (old->load_detect_temp) { >> - state = drm_atomic_state_alloc(dev); >> - if (!state) >> - goto fail; >> - >> - state->acquire_ctx = ctx; >> - >> - connector_state = drm_atomic_get_connector_state(state, connector); >> - if (IS_ERR(connector_state)) >> - goto fail; >> - >> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >> - if (IS_ERR(crtc_state)) >> - goto fail; >> - >> - connector_state->crtc = NULL; >> - >> - crtc_state->base.enable = crtc_state->base.active = false; >> - >> - ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, >> - 0, 0); >> - if (ret) >> - goto fail; >> - >> - ret = drm_atomic_commit(state); >> - if (ret) >> - goto fail; >> - >> - if (old->release_fb) { >> - drm_framebuffer_unregister_private(old->release_fb); >> - drm_framebuffer_unreference(old->release_fb); >> - } >> - >> + if (!state) >> return; >> - } >> >> - /* Switch crtc and encoder back off if necessary */ >> - if (old->dpms_mode != DRM_MODE_DPMS_ON) >> - connector->funcs->dpms(connector, old->dpms_mode); >> + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >> + crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll; > Hmm. What's this shared_dpll stuff? It's for intel_modeset_clear_plls, normally we duplicate the new state and it's handled correctly, but when we duplicate the old state the pll may be different and unexpected behavior could occur otherwise. ~Maarten
On Wed, Feb 03, 2016 at 09:57:55AM +0100, Maarten Lankhorst wrote: > Op 02-02-16 om 18:32 schreef Ville Syrjälä: > > On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote: > >> drm/i915: Use atomic state to obtain load detection crtc, v2. > >> > >> Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate > >> an atomic state before the new state is committed, and commit it the old state > >> in intel_release_load_detect_pipe. > >> > >> Changes since v1: > >> - Use a real atomic state. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 4d8c9f7857db..c7ba8f4a971e 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev, > >> if (obj->base.size < mode->vdisplay * fb->pitches[0]) > >> return NULL; > >> > >> + drm_framebuffer_reference(fb); > >> return fb; > >> #else > >> return NULL; > >> @@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > >> struct drm_device *dev = encoder->dev; > >> struct drm_framebuffer *fb; > >> struct drm_mode_config *config = &dev->mode_config; > >> - struct drm_atomic_state *state = NULL; > >> + struct drm_atomic_state *state = NULL, *restore_state = NULL; > >> struct drm_connector_state *connector_state; > >> struct intel_crtc_state *crtc_state; > >> int ret, i = -1; > >> @@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > >> connector->base.id, connector->name, > >> encoder->base.id, encoder->name); > >> > >> + old->restore_state = NULL; > >> + > >> retry: > >> ret = drm_modeset_lock(&config->connection_mutex, ctx); > >> if (ret) > >> @@ -10441,24 +10444,15 @@ retry: > >> */ > >> > >> /* See if we already have a CRTC for this connector */ > >> - if (encoder->crtc) { > >> - crtc = encoder->crtc; > >> + if (connector->state->crtc) { > >> + crtc = connector->state->crtc; > >> > >> ret = drm_modeset_lock(&crtc->mutex, ctx); > >> if (ret) > >> goto fail; > >> - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > >> - if (ret) > >> - goto fail; > >> - > >> - old->dpms_mode = connector->dpms; > >> - old->load_detect_temp = false; > >> > >> /* Make sure the crtc and connector are running */ > >> - if (connector->dpms != DRM_MODE_DPMS_ON) > >> - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); > >> - > >> - return true; > >> + goto found; > >> } > >> > >> /* Find an unused one (if possible) */ > >> @@ -10466,8 +10460,15 @@ retry: > >> i++; > >> if (!(encoder->possible_crtcs & (1 << i))) > >> continue; > >> - if (possible_crtc->state->enable) > >> + > >> + ret = drm_modeset_lock(&possible_crtc->mutex, ctx); > >> + if (ret) > >> + goto fail; > >> + > >> + if (possible_crtc->state->enable) { > >> + drm_modeset_unlock(&possible_crtc->mutex); > >> continue; > >> + } > >> > >> crtc = possible_crtc; > >> break; > >> @@ -10481,23 +10482,22 @@ retry: > >> goto fail; > >> } > >> > >> - ret = drm_modeset_lock(&crtc->mutex, ctx); > >> - if (ret) > >> - goto fail; > >> +found: > >> + intel_crtc = to_intel_crtc(crtc); > >> + > >> ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > >> if (ret) > >> goto fail; > >> > >> - intel_crtc = to_intel_crtc(crtc); > >> - old->dpms_mode = connector->dpms; > >> - old->load_detect_temp = true; > >> - old->release_fb = NULL; > >> - > >> state = drm_atomic_state_alloc(dev); > >> - if (!state) > >> - return false; > >> + restore_state = drm_atomic_state_alloc(dev); > >> + if (!state || !restore_state) { > >> + ret = -ENOMEM; > >> + goto fail; > >> + } > >> > >> state->acquire_ctx = ctx; > >> + restore_state->acquire_ctx = ctx; > >> > >> connector_state = drm_atomic_get_connector_state(state, connector); > >> if (IS_ERR(connector_state)) { > >> @@ -10505,7 +10505,9 @@ retry: > >> goto fail; > >> } > >> > >> - connector_state->crtc = crtc; > >> + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc); > >> + if (ret) > >> + goto fail; > >> > >> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > >> if (IS_ERR(crtc_state)) { > >> @@ -10529,7 +10531,6 @@ retry: > >> if (fb == NULL) { > >> DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); > >> 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(fb)) { > >> @@ -10541,15 +10542,28 @@ retry: > >> if (ret) > >> goto fail; > >> > >> - drm_mode_copy(&crtc_state->base.mode, mode); > >> + drm_framebuffer_unreference(fb); > >> + > >> + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); > >> + if (ret) > >> + goto fail; > >> + > >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); > >> + if (!ret) > >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); > >> + if (!ret) > >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); > >> + if (ret) { > >> + DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); > >> + goto fail; > >> + } > >> > >> if (drm_atomic_commit(state)) { > >> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); > >> - if (old->release_fb) > >> - old->release_fb->funcs->destroy(old->release_fb); > >> goto fail; > >> } > >> - crtc->primary->crtc = crtc; > >> + > >> + old->restore_state = restore_state; > >> > >> /* let the connector get through one full cycle before testing */ > >> intel_wait_for_vblank(dev, intel_crtc->pipe); > >> @@ -10557,7 +10571,8 @@ retry: > >> > >> fail: > >> drm_atomic_state_free(state); > >> - state = NULL; > >> + drm_atomic_state_free(restore_state); > >> + restore_state = state = NULL; > >> > >> if (ret == -EDEADLK) { > >> drm_modeset_backoff(ctx); > >> @@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > >> struct intel_load_detect_pipe *old, > >> struct drm_modeset_acquire_ctx *ctx) > >> { > >> - struct drm_device *dev = connector->dev; > >> struct intel_encoder *intel_encoder = > >> intel_attached_encoder(connector); > >> struct drm_encoder *encoder = &intel_encoder->base; > >> - struct drm_crtc *crtc = encoder->crtc; > >> + struct drm_crtc *crtc = connector->state->crtc; > >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> - struct drm_atomic_state *state; > >> - struct drm_connector_state *connector_state; > >> + struct drm_atomic_state *state = old->restore_state; > >> struct intel_crtc_state *crtc_state; > >> int ret; > >> > >> @@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, > >> connector->base.id, connector->name, > >> encoder->base.id, encoder->name); > >> > >> - if (old->load_detect_temp) { > >> - state = drm_atomic_state_alloc(dev); > >> - if (!state) > >> - goto fail; > >> - > >> - state->acquire_ctx = ctx; > >> - > >> - connector_state = drm_atomic_get_connector_state(state, connector); > >> - if (IS_ERR(connector_state)) > >> - goto fail; > >> - > >> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > >> - if (IS_ERR(crtc_state)) > >> - goto fail; > >> - > >> - connector_state->crtc = NULL; > >> - > >> - crtc_state->base.enable = crtc_state->base.active = false; > >> - > >> - ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, > >> - 0, 0); > >> - if (ret) > >> - goto fail; > >> - > >> - ret = drm_atomic_commit(state); > >> - if (ret) > >> - goto fail; > >> - > >> - if (old->release_fb) { > >> - drm_framebuffer_unregister_private(old->release_fb); > >> - drm_framebuffer_unreference(old->release_fb); > >> - } > >> - > >> + if (!state) > >> return; > >> - } > >> > >> - /* Switch crtc and encoder back off if necessary */ > >> - if (old->dpms_mode != DRM_MODE_DPMS_ON) > >> - connector->funcs->dpms(connector, old->dpms_mode); > >> + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > >> + crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll; > > Hmm. What's this shared_dpll stuff? > It's for intel_modeset_clear_plls, normally we duplicate the new state and it's handled correctly, > but when we duplicate the old state the pll may be different and unexpected behavior could occur > otherwise. Sounds fragile. Any chance of making it do the right thing (tm) without having to jump through such hoops?
Op 03-02-16 om 17:34 schreef Ville Syrjälä: > On Wed, Feb 03, 2016 at 09:57:55AM +0100, Maarten Lankhorst wrote: >> Op 02-02-16 om 18:32 schreef Ville Syrjälä: >>> On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote: >>>> drm/i915: Use atomic state to obtain load detection crtc, v2. >>>> >>>> Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate >>>> an atomic state before the new state is committed, and commit it the old state >>>> in intel_release_load_detect_pipe. >>>> >>>> Changes since v1: >>>> - Use a real atomic state. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 4d8c9f7857db..c7ba8f4a971e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev, >>>> if (obj->base.size < mode->vdisplay * fb->pitches[0]) >>>> return NULL; >>>> >>>> + drm_framebuffer_reference(fb); >>>> return fb; >>>> #else >>>> return NULL; >>>> @@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >>>> struct drm_device *dev = encoder->dev; >>>> struct drm_framebuffer *fb; >>>> struct drm_mode_config *config = &dev->mode_config; >>>> - struct drm_atomic_state *state = NULL; >>>> + struct drm_atomic_state *state = NULL, *restore_state = NULL; >>>> struct drm_connector_state *connector_state; >>>> struct intel_crtc_state *crtc_state; >>>> int ret, i = -1; >>>> @@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >>>> connector->base.id, connector->name, >>>> encoder->base.id, encoder->name); >>>> >>>> + old->restore_state = NULL; >>>> + >>>> retry: >>>> ret = drm_modeset_lock(&config->connection_mutex, ctx); >>>> if (ret) >>>> @@ -10441,24 +10444,15 @@ retry: >>>> */ >>>> >>>> /* See if we already have a CRTC for this connector */ >>>> - if (encoder->crtc) { >>>> - crtc = encoder->crtc; >>>> + if (connector->state->crtc) { >>>> + crtc = connector->state->crtc; >>>> >>>> ret = drm_modeset_lock(&crtc->mutex, ctx); >>>> if (ret) >>>> goto fail; >>>> - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); >>>> - if (ret) >>>> - goto fail; >>>> - >>>> - old->dpms_mode = connector->dpms; >>>> - old->load_detect_temp = false; >>>> >>>> /* Make sure the crtc and connector are running */ >>>> - if (connector->dpms != DRM_MODE_DPMS_ON) >>>> - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); >>>> - >>>> - return true; >>>> + goto found; >>>> } >>>> >>>> /* Find an unused one (if possible) */ >>>> @@ -10466,8 +10460,15 @@ retry: >>>> i++; >>>> if (!(encoder->possible_crtcs & (1 << i))) >>>> continue; >>>> - if (possible_crtc->state->enable) >>>> + >>>> + ret = drm_modeset_lock(&possible_crtc->mutex, ctx); >>>> + if (ret) >>>> + goto fail; >>>> + >>>> + if (possible_crtc->state->enable) { >>>> + drm_modeset_unlock(&possible_crtc->mutex); >>>> continue; >>>> + } >>>> >>>> crtc = possible_crtc; >>>> break; >>>> @@ -10481,23 +10482,22 @@ retry: >>>> goto fail; >>>> } >>>> >>>> - ret = drm_modeset_lock(&crtc->mutex, ctx); >>>> - if (ret) >>>> - goto fail; >>>> +found: >>>> + intel_crtc = to_intel_crtc(crtc); >>>> + >>>> ret = drm_modeset_lock(&crtc->primary->mutex, ctx); >>>> if (ret) >>>> goto fail; >>>> >>>> - intel_crtc = to_intel_crtc(crtc); >>>> - old->dpms_mode = connector->dpms; >>>> - old->load_detect_temp = true; >>>> - old->release_fb = NULL; >>>> - >>>> state = drm_atomic_state_alloc(dev); >>>> - if (!state) >>>> - return false; >>>> + restore_state = drm_atomic_state_alloc(dev); >>>> + if (!state || !restore_state) { >>>> + ret = -ENOMEM; >>>> + goto fail; >>>> + } >>>> >>>> state->acquire_ctx = ctx; >>>> + restore_state->acquire_ctx = ctx; >>>> >>>> connector_state = drm_atomic_get_connector_state(state, connector); >>>> if (IS_ERR(connector_state)) { >>>> @@ -10505,7 +10505,9 @@ retry: >>>> goto fail; >>>> } >>>> >>>> - connector_state->crtc = crtc; >>>> + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc); >>>> + if (ret) >>>> + goto fail; >>>> >>>> crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >>>> if (IS_ERR(crtc_state)) { >>>> @@ -10529,7 +10531,6 @@ retry: >>>> if (fb == NULL) { >>>> DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); >>>> 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(fb)) { >>>> @@ -10541,15 +10542,28 @@ retry: >>>> if (ret) >>>> goto fail; >>>> >>>> - drm_mode_copy(&crtc_state->base.mode, mode); >>>> + drm_framebuffer_unreference(fb); >>>> + >>>> + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); >>>> + if (ret) >>>> + goto fail; >>>> + >>>> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); >>>> + if (!ret) >>>> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); >>>> + if (!ret) >>>> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); >>>> + if (ret) { >>>> + DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); >>>> + goto fail; >>>> + } >>>> >>>> if (drm_atomic_commit(state)) { >>>> DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); >>>> - if (old->release_fb) >>>> - old->release_fb->funcs->destroy(old->release_fb); >>>> goto fail; >>>> } >>>> - crtc->primary->crtc = crtc; >>>> + >>>> + old->restore_state = restore_state; >>>> >>>> /* let the connector get through one full cycle before testing */ >>>> intel_wait_for_vblank(dev, intel_crtc->pipe); >>>> @@ -10557,7 +10571,8 @@ retry: >>>> >>>> fail: >>>> drm_atomic_state_free(state); >>>> - state = NULL; >>>> + drm_atomic_state_free(restore_state); >>>> + restore_state = state = NULL; >>>> >>>> if (ret == -EDEADLK) { >>>> drm_modeset_backoff(ctx); >>>> @@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >>>> struct intel_load_detect_pipe *old, >>>> struct drm_modeset_acquire_ctx *ctx) >>>> { >>>> - struct drm_device *dev = connector->dev; >>>> struct intel_encoder *intel_encoder = >>>> intel_attached_encoder(connector); >>>> struct drm_encoder *encoder = &intel_encoder->base; >>>> - struct drm_crtc *crtc = encoder->crtc; >>>> + struct drm_crtc *crtc = connector->state->crtc; >>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> - struct drm_atomic_state *state; >>>> - struct drm_connector_state *connector_state; >>>> + struct drm_atomic_state *state = old->restore_state; >>>> struct intel_crtc_state *crtc_state; >>>> int ret; >>>> >>>> @@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >>>> connector->base.id, connector->name, >>>> encoder->base.id, encoder->name); >>>> >>>> - if (old->load_detect_temp) { >>>> - state = drm_atomic_state_alloc(dev); >>>> - if (!state) >>>> - goto fail; >>>> - >>>> - state->acquire_ctx = ctx; >>>> - >>>> - connector_state = drm_atomic_get_connector_state(state, connector); >>>> - if (IS_ERR(connector_state)) >>>> - goto fail; >>>> - >>>> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >>>> - if (IS_ERR(crtc_state)) >>>> - goto fail; >>>> - >>>> - connector_state->crtc = NULL; >>>> - >>>> - crtc_state->base.enable = crtc_state->base.active = false; >>>> - >>>> - ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, >>>> - 0, 0); >>>> - if (ret) >>>> - goto fail; >>>> - >>>> - ret = drm_atomic_commit(state); >>>> - if (ret) >>>> - goto fail; >>>> - >>>> - if (old->release_fb) { >>>> - drm_framebuffer_unregister_private(old->release_fb); >>>> - drm_framebuffer_unreference(old->release_fb); >>>> - } >>>> - >>>> + if (!state) >>>> return; >>>> - } >>>> >>>> - /* Switch crtc and encoder back off if necessary */ >>>> - if (old->dpms_mode != DRM_MODE_DPMS_ON) >>>> - connector->funcs->dpms(connector, old->dpms_mode); >>>> + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >>>> + crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll; >>> Hmm. What's this shared_dpll stuff? >> It's for intel_modeset_clear_plls, normally we duplicate the new state and it's handled correctly, >> but when we duplicate the old state the pll may be different and unexpected behavior could occur >> otherwise. > Sounds fragile. Any chance of making it do the right thing (tm) without > having to jump through such hoops? I'm not completely sure if that's possible, I'll have to check if making intel_modeset_clear_plls only clear the mask from old_crtc_state->shared_dpll will break anything first. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4d8c9f7857db..c7ba8f4a971e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev, if (obj->base.size < mode->vdisplay * fb->pitches[0]) return NULL; + drm_framebuffer_reference(fb); return fb; #else return NULL; @@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, struct drm_device *dev = encoder->dev; struct drm_framebuffer *fb; struct drm_mode_config *config = &dev->mode_config; - struct drm_atomic_state *state = NULL; + struct drm_atomic_state *state = NULL, *restore_state = NULL; struct drm_connector_state *connector_state; struct intel_crtc_state *crtc_state; int ret, i = -1; @@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, connector->base.id, connector->name, encoder->base.id, encoder->name); + old->restore_state = NULL; + retry: ret = drm_modeset_lock(&config->connection_mutex, ctx); if (ret) @@ -10441,24 +10444,15 @@ retry: */ /* See if we already have a CRTC for this connector */ - if (encoder->crtc) { - crtc = encoder->crtc; + if (connector->state->crtc) { + crtc = connector->state->crtc; ret = drm_modeset_lock(&crtc->mutex, ctx); if (ret) goto fail; - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); - if (ret) - goto fail; - - old->dpms_mode = connector->dpms; - old->load_detect_temp = false; /* Make sure the crtc and connector are running */ - if (connector->dpms != DRM_MODE_DPMS_ON) - connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); - - return true; + goto found; } /* Find an unused one (if possible) */ @@ -10466,8 +10460,15 @@ retry: i++; if (!(encoder->possible_crtcs & (1 << i))) continue; - if (possible_crtc->state->enable) + + ret = drm_modeset_lock(&possible_crtc->mutex, ctx); + if (ret) + goto fail; + + if (possible_crtc->state->enable) { + drm_modeset_unlock(&possible_crtc->mutex); continue; + } crtc = possible_crtc; break; @@ -10481,23 +10482,22 @@ retry: goto fail; } - ret = drm_modeset_lock(&crtc->mutex, ctx); - if (ret) - goto fail; +found: + intel_crtc = to_intel_crtc(crtc); + ret = drm_modeset_lock(&crtc->primary->mutex, ctx); if (ret) goto fail; - intel_crtc = to_intel_crtc(crtc); - old->dpms_mode = connector->dpms; - old->load_detect_temp = true; - old->release_fb = NULL; - state = drm_atomic_state_alloc(dev); - if (!state) - return false; + restore_state = drm_atomic_state_alloc(dev); + if (!state || !restore_state) { + ret = -ENOMEM; + goto fail; + } state->acquire_ctx = ctx; + restore_state->acquire_ctx = ctx; connector_state = drm_atomic_get_connector_state(state, connector); if (IS_ERR(connector_state)) { @@ -10505,7 +10505,9 @@ retry: goto fail; } - connector_state->crtc = crtc; + ret = drm_atomic_set_crtc_for_connector(connector_state, crtc); + if (ret) + goto fail; crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); if (IS_ERR(crtc_state)) { @@ -10529,7 +10531,6 @@ retry: if (fb == NULL) { DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); 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(fb)) { @@ -10541,15 +10542,28 @@ retry: if (ret) goto fail; - drm_mode_copy(&crtc_state->base.mode, mode); + drm_framebuffer_unreference(fb); + + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); + if (ret) + goto fail; + + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); + if (!ret) + ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); + if (!ret) + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); + if (ret) { + DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); + goto fail; + } if (drm_atomic_commit(state)) { DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); - if (old->release_fb) - old->release_fb->funcs->destroy(old->release_fb); goto fail; } - crtc->primary->crtc = crtc; + + old->restore_state = restore_state; /* let the connector get through one full cycle before testing */ intel_wait_for_vblank(dev, intel_crtc->pipe); @@ -10557,7 +10571,8 @@ retry: fail: drm_atomic_state_free(state); - state = NULL; + drm_atomic_state_free(restore_state); + restore_state = state = NULL; if (ret == -EDEADLK) { drm_modeset_backoff(ctx); @@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, struct intel_load_detect_pipe *old, struct drm_modeset_acquire_ctx *ctx) { - struct drm_device *dev = connector->dev; struct intel_encoder *intel_encoder = intel_attached_encoder(connector); struct drm_encoder *encoder = &intel_encoder->base; - struct drm_crtc *crtc = encoder->crtc; + struct drm_crtc *crtc = connector->state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct drm_atomic_state *state; - struct drm_connector_state *connector_state; + struct drm_atomic_state *state = old->restore_state; struct intel_crtc_state *crtc_state; int ret; @@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, connector->base.id, connector->name, encoder->base.id, encoder->name); - if (old->load_detect_temp) { - state = drm_atomic_state_alloc(dev); - if (!state) - goto fail; - - state->acquire_ctx = ctx; - - connector_state = drm_atomic_get_connector_state(state, connector); - if (IS_ERR(connector_state)) - goto fail; - - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); - if (IS_ERR(crtc_state)) - goto fail; - - connector_state->crtc = NULL; - - crtc_state->base.enable = crtc_state->base.active = false; - - ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL, - 0, 0); - if (ret) - goto fail; - - ret = drm_atomic_commit(state); - if (ret) - goto fail; - - if (old->release_fb) { - drm_framebuffer_unregister_private(old->release_fb); - drm_framebuffer_unreference(old->release_fb); - } - + if (!state) return; - } - /* Switch crtc and encoder back off if necessary */ - if (old->dpms_mode != DRM_MODE_DPMS_ON) - connector->funcs->dpms(connector, old->dpms_mode); + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); + crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll; - return; -fail: - DRM_DEBUG_KMS("Couldn't release load detect pipe.\n"); - drm_atomic_state_free(state); + ret = drm_atomic_commit(state); + if (ret) { + DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); + drm_atomic_state_free(state); + } } static int i9xx_pll_refclk(struct drm_device *dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 93ba14a3bb76..7d10ca7797a9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -912,9 +912,7 @@ struct intel_unpin_work { }; struct intel_load_detect_pipe { - struct drm_framebuffer *release_fb; - bool load_detect_temp; - int dpms_mode; + struct drm_atomic_state *restore_state; }; static inline struct intel_encoder *
drm/i915: Use atomic state to obtain load detection crtc, v2. Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate an atomic state before the new state is committed, and commit it the old state in intel_release_load_detect_pipe. Changes since v1: - Use a real atomic state. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> ---