| Message ID | 1431354318-11995-10-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote: > This should be safe. Usual request: A few more details about what you've changed to help guide the review would be great. E.g. which functions from the atomic helpers you're trying to copy here exactly. It looks like this models set_routing_links. I think it would be rather useful to expose this to drivers as a helper function, maybe with a more useful name like drm_atomic_helper_update_legacy_state or similar. Another thing I've noticed is that atomic helpers lost the call to drm_calc_timestamping_constants. Would be good to add that to the same function. -Daniel > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++++++------------------ > 1 file changed, 41 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a21b2e51c054..956c9964275d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11084,36 +11084,48 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > } > } > > -/* Fixup legacy state after an atomic state swap. > +/* > + * Fixup legacy state in a similar way to > + * drm_atomic_helper.c:set_routing_links. > */ > static void intel_modeset_fixup_state(struct drm_atomic_state *state) > { > - struct intel_crtc *crtc; > - struct intel_encoder *encoder; > - struct intel_connector *connector; > + struct drm_connector *connector; > + struct drm_crtc_state *crtc_state; > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + int i; > > - for_each_intel_connector(state->dev, connector) { > - connector->base.encoder = connector->base.state->best_encoder; > - if (connector->base.encoder) > - connector->base.encoder->crtc = > - connector->base.state->crtc; > + /* > + * swap crtc and connector and update legacy state, > + * plane state already gets swapped > + * by the plane helpers. Once .crtc_disable is fixed > + * all state should be swapped before disabling crtc's. > + */ > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + crtc->enabled = crtc->state->enable; > + crtc->mode = crtc->state->mode; > } > > - /* Update crtc of disabled encoders */ > - for_each_intel_encoder(state->dev, encoder) { > - int num_connectors = 0; > - > - for_each_intel_connector(state->dev, connector) > - if (connector->base.encoder == &encoder->base) > - num_connectors++; > + /* clear out existing links */ > + for_each_connector_in_state(state, connector, conn_state, i) { > + if (!connector->encoder) > + continue; > > - if (num_connectors == 0) > - encoder->base.crtc = NULL; > + WARN_ON(!connector->encoder->crtc); > + connector->encoder->crtc = NULL; > + connector->encoder = NULL; > } > > - for_each_intel_crtc(state->dev, crtc) { > - crtc->base.enabled = crtc->base.state->enable; > - crtc->config = to_intel_crtc_state(crtc->base.state); > + for_each_connector_in_state(state, connector, conn_state, i) { > + if (!connector->state->crtc) > + continue; > + > + if (WARN_ON(!connector->state->best_encoder)) > + continue; > + > + connector->encoder = connector->state->best_encoder; > + connector->encoder->crtc = connector->state->crtc; > } > } > > @@ -11559,9 +11571,16 @@ intel_modeset_update_state(struct drm_atomic_state *state) > > intel_modeset_fixup_state(state); > > - /* Double check state. */ > for_each_crtc(dev, crtc) { > + /* Double check state. */ > WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); > + > + if (!state->crtcs[drm_crtc_index(crtc)]) > + continue; > + > + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); > + if (crtc->state->active && needs_modeset(crtc->state)) > + drm_calc_timestamping_constants(crtc, &crtc->state->adjusted_mode); > } > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > @@ -12277,25 +12296,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > drm_plane_helper_disable(crtc->primary); > } > > - /* crtc->mode is already used by the ->mode_set callbacks, hence we need > - * to set it here already despite that we pass it down the callchain. > - * > - * Note we'll need to fix this up when we start tracking multiple > - * pipes; here we assume a single modeset_pipe and only track the > - * single crtc and mode. > - */ > - if (pipe_config->base.active && needs_modeset(&pipe_config->base)) { > - modeset_crtc->mode = pipe_config->base.mode; > - > - /* > - * Calculate and store various constants which > - * are later needed by vblank and swap-completion > - * timestamping. They are derived from true hwmode. > - */ > - drm_calc_timestamping_constants(modeset_crtc, > - &pipe_config->base.adjusted_mode); > - } > - > /* Only after disabling all output pipelines that will be changed can we > * update the the output configuration. */ > intel_modeset_update_state(state); > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 12-05-15 om 08:59 schreef Daniel Vetter: > On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote: >> This should be safe. > Usual request: A few more details about what you've changed to help guide > the review would be great. E.g. which functions from the atomic helpers > you're trying to copy here exactly. That's in the diff. :-) > It looks like this models set_routing_links. I think it would be rather > useful to expose this to drivers as a helper function, maybe with a more > useful name like drm_atomic_helper_update_legacy_state or similar. I think you're right about this one. > Another thing I've noticed is that atomic helpers lost the call to > drm_calc_timestamping_constants. Would be good to add that to the same > function. Yeah, but the precise vblanking stuff is not really atomic friendly, so I'm not sure it should be done outside the driver.
On Tue, May 12, 2015 at 02:41:17PM +0200, Maarten Lankhorst wrote: > Op 12-05-15 om 08:59 schreef Daniel Vetter: > > On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote: > >> This should be safe. > > Usual request: A few more details about what you've changed to help guide > > the review would be great. E.g. which functions from the atomic helpers > > you're trying to copy here exactly. > That's in the diff. :-) Indeed ;-) > > It looks like this models set_routing_links. I think it would be rather > > useful to expose this to drivers as a helper function, maybe with a more > > useful name like drm_atomic_helper_update_legacy_state or similar. > I think you're right about this one. > > Another thing I've noticed is that atomic helpers lost the call to > > drm_calc_timestamping_constants. Would be good to add that to the same > > function. > Yeah, but the precise vblanking stuff is not really atomic friendly, > so I'm not sure it should be done outside the driver. Hm what's unfriendly about it? It's definitely a regression to not update the constants needed by it compared to crtc helpers. I'll go about and fix that now. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a21b2e51c054..956c9964275d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11084,36 +11084,48 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) } } -/* Fixup legacy state after an atomic state swap. +/* + * Fixup legacy state in a similar way to + * drm_atomic_helper.c:set_routing_links. */ static void intel_modeset_fixup_state(struct drm_atomic_state *state) { - struct intel_crtc *crtc; - struct intel_encoder *encoder; - struct intel_connector *connector; + struct drm_connector *connector; + struct drm_crtc_state *crtc_state; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + int i; - for_each_intel_connector(state->dev, connector) { - connector->base.encoder = connector->base.state->best_encoder; - if (connector->base.encoder) - connector->base.encoder->crtc = - connector->base.state->crtc; + /* + * swap crtc and connector and update legacy state, + * plane state already gets swapped + * by the plane helpers. Once .crtc_disable is fixed + * all state should be swapped before disabling crtc's. + */ + for_each_crtc_in_state(state, crtc, crtc_state, i) { + crtc->enabled = crtc->state->enable; + crtc->mode = crtc->state->mode; } - /* Update crtc of disabled encoders */ - for_each_intel_encoder(state->dev, encoder) { - int num_connectors = 0; - - for_each_intel_connector(state->dev, connector) - if (connector->base.encoder == &encoder->base) - num_connectors++; + /* clear out existing links */ + for_each_connector_in_state(state, connector, conn_state, i) { + if (!connector->encoder) + continue; - if (num_connectors == 0) - encoder->base.crtc = NULL; + WARN_ON(!connector->encoder->crtc); + connector->encoder->crtc = NULL; + connector->encoder = NULL; } - for_each_intel_crtc(state->dev, crtc) { - crtc->base.enabled = crtc->base.state->enable; - crtc->config = to_intel_crtc_state(crtc->base.state); + for_each_connector_in_state(state, connector, conn_state, i) { + if (!connector->state->crtc) + continue; + + if (WARN_ON(!connector->state->best_encoder)) + continue; + + connector->encoder = connector->state->best_encoder; + connector->encoder->crtc = connector->state->crtc; } } @@ -11559,9 +11571,16 @@ intel_modeset_update_state(struct drm_atomic_state *state) intel_modeset_fixup_state(state); - /* Double check state. */ for_each_crtc(dev, crtc) { + /* Double check state. */ WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); + + if (!state->crtcs[drm_crtc_index(crtc)]) + continue; + + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); + if (crtc->state->active && needs_modeset(crtc->state)) + drm_calc_timestamping_constants(crtc, &crtc->state->adjusted_mode); } list_for_each_entry(connector, &dev->mode_config.connector_list, head) { @@ -12277,25 +12296,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, drm_plane_helper_disable(crtc->primary); } - /* crtc->mode is already used by the ->mode_set callbacks, hence we need - * to set it here already despite that we pass it down the callchain. - * - * Note we'll need to fix this up when we start tracking multiple - * pipes; here we assume a single modeset_pipe and only track the - * single crtc and mode. - */ - if (pipe_config->base.active && needs_modeset(&pipe_config->base)) { - modeset_crtc->mode = pipe_config->base.mode; - - /* - * Calculate and store various constants which - * are later needed by vblank and swap-completion - * timestamping. They are derived from true hwmode. - */ - drm_calc_timestamping_constants(modeset_crtc, - &pipe_config->base.adjusted_mode); - } - /* Only after disabling all output pipelines that will be changed can we * update the the output configuration. */ intel_modeset_update_state(state);
This should be safe. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 41 deletions(-)