On Thu, Aug 06, 2015 at 04:06:58PM +0200, Maarten Lankhorst wrote: > Op 06-08-15 om 15:12 schreef Daniel Vetter: > > On Wed, Aug 05, 2015 at 12:37:10PM +0200, Maarten Lankhorst wrote: > >> The rest will be a noop anyway, since without modeset there will be > >> no updated dplls and no modeset state to update. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 30 +++++++----------------------- > >> 1 file changed, 7 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 1fd8b7dec7da..aa444cbc2262 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -12181,33 +12181,15 @@ fail: > >> return ret; > >> } > >> > >> -static bool intel_crtc_in_use(struct drm_crtc *crtc) > >> -{ > >> - struct drm_encoder *encoder; > >> - struct drm_device *dev = crtc->dev; > >> - > >> - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > >> - if (encoder->crtc == crtc) > >> - return true; > >> - > >> - return false; > >> -} > >> - > >> static void > >> -intel_modeset_update_state(struct drm_atomic_state *state) > >> +intel_modeset_update_crtc_state(struct drm_atomic_state *state) > >> { > >> struct drm_crtc *crtc; > >> struct drm_crtc_state *crtc_state; > >> int i; > >> > >> - intel_shared_dpll_commit(state); > > This should be right next to swap_state, and we should probably rename it > > to be consistent. > After some digging I think this could work if we no longer check pll->config.crtc_mask > in intel_disable_shared_dpll. > > If we check crtc_mask in intel_enable_shared_dpll we can remove the one from disable > and prepare, and let the hw checker deal with it. Yeah I think moving all the state checks into the checker would be useful - my comment was really a high-level "this is how it should be" comment, without looking into the details ;-9 > >> - > >> - drm_atomic_helper_update_legacy_modeset_state(state->dev, state); > > This helper should always work, why try to optimize things? > I didn't see the point of going through the connector state, > but shrug I guess the extra loops probably don't matter. modeset is expensive, a few more loops won't hurt I think. > >> - > >> /* Double check state. */ > >> for_each_crtc_in_state(state, crtc, crtc_state, i) { > >> - WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); > > I guess this WARN_ON should be moved into the state checker? Or entirely > > removed if redundant. > It's removed because the atomic core does this for us. Separate patch and should be explained in the commit message ;-) -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