From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.
Date: Wed, 3 Feb 2016 18:34:08 +0200 [thread overview]
Message-ID: <20160203163408.GF23290@intel.com> (raw)
In-Reply-To: <56B1C113.403@linux.intel.com>
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?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-02-03 16:34 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-01 13:43 [PATCH 0/6] Use more atomic state in i915 Maarten Lankhorst
2016-02-01 13:43 ` [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc Maarten Lankhorst
2016-02-01 16:08 ` Ville Syrjälä
2016-02-02 10:41 ` Maarten Lankhorst
2016-02-01 17:09 ` Ville Syrjälä
2016-02-02 12:46 ` Maarten Lankhorst
2016-02-02 12:48 ` [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2 Maarten Lankhorst
2016-02-02 17:32 ` Ville Syrjälä
2016-02-03 8:57 ` Maarten Lankhorst
2016-02-03 16:34 ` Ville Syrjälä [this message]
2016-02-09 8:57 ` Maarten Lankhorst
2016-02-09 12:52 ` [PATCH 1/3] drm/i915: Clear shared dpll based on old state Maarten Lankhorst
2016-02-09 12:52 ` [PATCH 2/3] drm/i915: Use atomic helpers for suspend Maarten Lankhorst
2016-02-09 13:37 ` Ville Syrjälä
2016-02-09 14:05 ` Maarten Lankhorst
2016-02-09 14:58 ` Ville Syrjälä
2016-02-15 12:34 ` Maarten Lankhorst
2016-02-16 9:06 ` [PATCH v2 2/3] drm/i915: Use atomic helpers for suspend, v2 Maarten Lankhorst
2016-02-16 9:55 ` Ville Syrjälä
2016-02-09 12:52 ` [PATCH 3/3] drm/i915: Use atomic state to obtain load detection crtc, v3 Maarten Lankhorst
2016-02-16 11:13 ` Ville Syrjälä
2016-02-09 13:25 ` [PATCH 1/3] drm/i915: Clear shared dpll based on old state Ville Syrjälä
2016-02-11 8:56 ` [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc Daniel Vetter
2016-02-01 13:43 ` [PATCH 2/6] drm/i915: Use atomic state in crt load detection Maarten Lankhorst
2016-02-01 13:43 ` [PATCH 3/6] drm/i915: Use atomic state in tv " Maarten Lankhorst
2016-02-01 13:44 ` [PATCH 4/6] drm/i915: Use correct dpms for intel_enable_crt Maarten Lankhorst
2016-02-01 13:44 ` [IGT PATCH 5/6] kms_force_connector_basic: Add force-load-detect test Maarten Lankhorst
2016-02-11 8:59 ` Daniel Vetter
2016-02-11 11:50 ` Maarten Lankhorst
2016-02-01 13:44 ` [PATCH 6/6] drm/i915: Use atomic state in intel_fb_initial_config Maarten Lankhorst
2016-02-11 14:38 ` Ville Syrjälä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160203163408.GF23290@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.