| Message ID | 1438771031-23227-13-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > --- > 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. > - > - drm_atomic_helper_update_legacy_modeset_state(state->dev, state); This helper should always work, why try to optimize things? > - > /* 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. > - > to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); > > /* Update hwmode for vblank functions */ > @@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev, > > /* Only after disabling all output pipelines that will be changed can we > * update the the output configuration. */ > - intel_modeset_update_state(state); > + intel_modeset_update_crtc_state(state); > > - /* The state has been swaped above, so state actually contains the > - * old state now. */ > - if (any_ms) > + if (any_ms) { > + intel_shared_dpll_commit(state); > + > + drm_atomic_helper_update_legacy_modeset_state(state->dev, state); > modeset_update_crtc_power_domains(state); > + } > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@linux.intel.com> >> --- >> 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. >> - >> - 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. >> - >> /* 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. >> - >> to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); >> >> /* Update hwmode for vblank functions */ >> @@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev, >> >> /* Only after disabling all output pipelines that will be changed can we >> * update the the output configuration. */ >> - intel_modeset_update_state(state); >> + intel_modeset_update_crtc_state(state); >> >> - /* The state has been swaped above, so state actually contains the >> - * old state now. */ >> - if (any_ms) >> + if (any_ms) { >> + intel_shared_dpll_commit(state); >> + >> + drm_atomic_helper_update_legacy_modeset_state(state->dev, state); >> modeset_update_crtc_power_domains(state); >> + } >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> -- >> 2.1.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@linux.intel.com> > >> --- > >> 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
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); - - drm_atomic_helper_update_legacy_modeset_state(state->dev, state); - /* Double check state. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); - to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); /* Update hwmode for vblank functions */ @@ -13110,12 +13092,14 @@ static int intel_atomic_commit(struct drm_device *dev, /* Only after disabling all output pipelines that will be changed can we * update the the output configuration. */ - intel_modeset_update_state(state); + intel_modeset_update_crtc_state(state); - /* The state has been swaped above, so state actually contains the - * old state now. */ - if (any_ms) + if (any_ms) { + intel_shared_dpll_commit(state); + + drm_atomic_helper_update_legacy_modeset_state(state->dev, state); modeset_update_crtc_power_domains(state); + } /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) {
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@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-)