On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 11:57 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote:
> >> @@ -10421,10 +10414,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> >> if (IS_ERR(crtc_state))
> >> goto fail;
> >>
> >> - to_intel_connector(connector)->new_encoder = NULL;
> >> - intel_encoder->new_crtc = NULL;
> >> - intel_crtc->new_enabled = false;
> > load_detect changes should be a separate patch. Or the commit message
> > needs to explain why this needs to be one.
> These members no longer exist. ;-) all the new_ stuff was to restore things pre-atomic, the atomic updates are good enough here.
>
> Making it a separate patch's probably ok.
Yeah I think splitting out the new_* removal would address most of my
concerns here.
[snip]
> >> + struct intel_connector *conn;
> >> + struct intel_plane *plane;
> >> + struct drm_crtc *crtc;
> >> + int ret;
> >>
> >> - /*
> >> - * We need to use raw interfaces for restoring state to avoid
> >> - * checking (bogus) intermediate states.
> >> - */
> >> - for_each_pipe(dev_priv, pipe) {
> >> - struct drm_crtc *crtc =
> >> - dev_priv->pipe_to_crtc_mapping[pipe];
> >> + if (!state)
> > debug output missing that the state alloc failed. Perhaps just goto fail;
> > since state_free can cope with a NULL state.
> It can only fail because of kmalloc, which prints its own warnings.
Might still be useful just to have unified error reporting - you need to
guess the caller otherwise which would make debug (if this ever happesn)
harder. But really just a bikeshed.
>
> >> + return;
> >>
> >> - intel_crtc_restore_mode(crtc);
> >> - }
> >> - } else {
> >> - intel_modeset_update_staged_output_state(dev);
> >> + state->acquire_ctx = dev->mode_config.acquire_ctx;
> >> +
> >> + /* preserve complete old state, including dpll */
> >> + intel_atomic_get_shared_dpll_state(state);
> >> +
> >> + for_each_crtc(dev, crtc) {
> >> + struct drm_crtc_state *crtc_state =
> >> + drm_atomic_get_crtc_state(state, crtc);
> >> +
> >> + ret = PTR_ERR_OR_ZERO(crtc_state);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + /* force a restore */
> >> + crtc_state->mode_changed = true;
> >> + }
> >> +
> >> + for_each_intel_plane(dev, plane) {
> >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
> >> + if (ret)
> >> + goto err;
> >> + }
> >> +
> >> + for_each_intel_connector(dev, conn) {
> >> + ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
> >> + if (ret)
> >> + goto err;
> >> }
> >>
> >> - intel_modeset_check_state(dev);
> >> + intel_modeset_setup_hw_state(dev);
> >> +
> >> + i915_redisable_vga(dev);
> > Since we've only badly bruised escape this trap I think this deserves a
> > comment:
> >
> > /*
> > * WARNING: We can't do a full atomic modeset including
> > * compute/check phase here since especially encoder
> > * compute_config functions depend upon output detection state.
> > * And that's just not yet available at driver load. Therefore we
> > * must read out the entire relevant hw state (including any
> > * driver internal state) faithfully here and only apply the
> > * commit side.
> > */
> >
> > Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here
> > once atomic modeset is fully working?
> Not for initial hw readout unless you want to call detect in this function for all encoders.. resume's fine probably.
I meant calling dev->mode_config.funcs->atomic_commit(state) directly,
without calling ->atomic_check at all. That should avoid any state
recomputation (otherwise our check/commit split is botched) and hence be
exactly what we need here. I didn't check how close intel_set_mode is
compared our ->atomic_commit implementation after this series (didn't
apply them all). But I think from a semantic pov those two should match.
-Daniel
--
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
