| Message ID | 1426240142-24538-8-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-03-19 at 00:38 +0000, Konduru, Chandra wrote: > > > -----Original Message----- > > From: Conselvan De Oliveira, Ander > > Sent: Friday, March 13, 2015 2:49 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander > > Subject: [PATCH 07/19] drm/i915: Copy the staged connector config to the > > legacy atomic state > > > > With this in place, we can start converting pieces of the modeset code to look at > > the connector atomic state instead of the staged config. > > > > v2: Handle the load detect staged config changes too. (Ander) > > Remove unnecessary blank line. (Daniel) > > > > Signed-off-by: Ander Conselvan de Oliveira > > <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 52 > > +++++++++++++++++++++++++++++++----- > > 1 file changed, 45 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index abdbd0c..6465f6d 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8805,6 +8805,7 @@ bool intel_get_load_detect_pipe(struct > > drm_connector *connector, > > struct drm_framebuffer *fb; > > struct drm_mode_config *config = &dev->mode_config; > > struct drm_atomic_state *state = NULL; > > + struct drm_connector_state *connector_state; > > int ret, i = -1; > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", @@ > > -8892,6 +8893,15 @@ retry: > > > > state->acquire_ctx = ctx; > > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > + if (IS_ERR(connector_state)) { > > + ret = PTR_ERR(connector_state); > > + goto fail; > > + } > > + > > + connector_state->crtc = crtc; > > + connector_state->best_encoder = &intel_encoder->base; > > + > > if (!mode) > > mode = &load_detect_mode; > > > > @@ -8957,6 +8967,7 @@ void intel_release_load_detect_pipe(struct > > drm_connector *connector, > > struct drm_crtc *crtc = encoder->crtc; > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > struct drm_atomic_state *state; > > + struct drm_connector_state *connector_state; > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", > > connector->base.id, connector->name, @@ -8964,17 > > +8975,23 @@ void intel_release_load_detect_pipe(struct drm_connector > > *connector, > > > > if (old->load_detect_temp) { > > state = drm_atomic_state_alloc(dev); > > - if (!state) { > > - DRM_DEBUG_KMS("can't release load detect pipe\n"); > > - return; > > - } > > + if (!state) > > + goto fail; > > Is the above deletion of lines from code added by another patch in this series or > from a different series? May be you can squash them into one. That was added in patch 3, the one that adds the drm_atomic_state parameter to intel_set_mode(). I chose to do it that way in order to not complicate that patch unnecessarily. It wasn't possible to convert each different call to intel_set_mode() separately, since that would create a state of breakage that prevents bisecting. So the approach I found most reasonable was to just add the state parameter in patch number 3, and leave the proper population of the atomic state to a separate patch. The main idea was to simplify the review process. > > > > > state->acquire_ctx = ctx; > > > > + connector_state = drm_atomic_get_connector_state(state, > > connector); > > + if (IS_ERR(connector_state)) > > + goto fail; > > + > > to_intel_connector(connector)->new_encoder = NULL; > > intel_encoder->new_crtc = NULL; > > intel_crtc->new_enabled = false; > > intel_crtc->new_config = NULL; > > + > > + connector_state->best_encoder = NULL; > > + connector_state->crtc = NULL; > > + > > intel_set_mode(crtc, NULL, 0, 0, NULL, state); > > > > drm_atomic_state_free(state); > > @@ -8990,6 +9007,11 @@ void intel_release_load_detect_pipe(struct > > drm_connector *connector, > > /* Switch crtc and encoder back off if necessary */ > > if (old->dpms_mode != DRM_MODE_DPMS_ON) > > connector->funcs->dpms(connector, old->dpms_mode); > > + > > + return; > > +fail: > > + DRM_DEBUG_KMS("Couldn't release load detect pipe.\n"); > > + drm_atomic_state_free(state); > > } > > Learning while reviewing connector/encoder side of handling. > But I think someone also should look at the encoder/connector side or > atomic handling. I think Daniel already did a high level review of this. Perhaps he could do it again for v2 of this patch. Ander > > > > static int i9xx_pll_refclk(struct drm_device *dev, @@ -11646,9 +11668,11 @@ > > intel_set_config_compute_mode_changes(struct drm_mode_set *set, static int > > intel_modeset_stage_output_state(struct drm_device *dev, > > struct drm_mode_set *set, > > - struct intel_set_config *config) > > + struct intel_set_config *config, > > + struct drm_atomic_state *state) > > { > > struct intel_connector *connector; > > + struct drm_connector_state *connector_state; > > struct intel_encoder *encoder; > > struct intel_crtc *crtc; > > int ro; > > @@ -11712,6 +11736,14 @@ intel_modeset_stage_output_state(struct > > drm_device *dev, > > } > > connector->new_encoder->new_crtc = to_intel_crtc(new_crtc); > > > > + connector_state = > > + drm_atomic_get_connector_state(state, &connector- > > >base); > > + if (IS_ERR(connector_state)) > > + return PTR_ERR(connector_state); > > + > > + connector_state->crtc = new_crtc; > > + connector_state->best_encoder = &connector->new_encoder- > > >base; > > + > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n", > > connector->base.base.id, > > connector->base.name, > > @@ -11744,9 +11776,15 @@ intel_modeset_stage_output_state(struct > > drm_device *dev, > > } > > /* Now we've also updated encoder->new_crtc for all encoders. */ > > for_each_intel_connector(dev, connector) { > > - if (connector->new_encoder) > > + connector_state = > > + drm_atomic_get_connector_state(state, &connector- > > >base); > > + > > + if (connector->new_encoder) { > > if (connector->new_encoder != connector->encoder) > > connector->encoder = connector- > > >new_encoder; > > + } else { > > + connector_state->crtc = NULL; > > + } > > } > > for_each_intel_crtc(dev, crtc) { > > crtc->new_enabled = false; > > @@ -11855,7 +11893,7 @@ static int intel_crtc_set_config(struct > > drm_mode_set *set) > > > > state->acquire_ctx = dev->mode_config.acquire_ctx; > > > > - ret = intel_modeset_stage_output_state(dev, set, config); > > + ret = intel_modeset_stage_output_state(dev, set, config, state); > > if (ret) > > goto fail; > > > > -- > > 2.1.0 >
On Thu, Mar 19, 2015 at 09:52:08AM +0200, Ander Conselvan De Oliveira wrote: > On Thu, 2015-03-19 at 00:38 +0000, Konduru, Chandra wrote: > > Learning while reviewing connector/encoder side of handling. > > But I think someone also should look at the encoder/connector side or > > atomic handling. > > I think Daniel already did a high level review of this. Perhaps he could > do it again for v2 of this patch. We have a massive pile of JIRA's to fix up interactions between connectors/encoders and the new atomic code. And I think Ander's patches here won't cause trouble. So I think we're good for now. Of course this being atomic there will be some surprises ... -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index abdbd0c..6465f6d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8805,6 +8805,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, struct drm_framebuffer *fb; struct drm_mode_config *config = &dev->mode_config; struct drm_atomic_state *state = NULL; + struct drm_connector_state *connector_state; int ret, i = -1; DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", @@ -8892,6 +8893,15 @@ retry: state->acquire_ctx = ctx; + connector_state = drm_atomic_get_connector_state(state, connector); + if (IS_ERR(connector_state)) { + ret = PTR_ERR(connector_state); + goto fail; + } + + connector_state->crtc = crtc; + connector_state->best_encoder = &intel_encoder->base; + if (!mode) mode = &load_detect_mode; @@ -8957,6 +8967,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, struct drm_crtc *crtc = encoder->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_atomic_state *state; + struct drm_connector_state *connector_state; DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", connector->base.id, connector->name, @@ -8964,17 +8975,23 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (old->load_detect_temp) { state = drm_atomic_state_alloc(dev); - if (!state) { - DRM_DEBUG_KMS("can't release load detect pipe\n"); - return; - } + if (!state) + goto fail; state->acquire_ctx = ctx; + connector_state = drm_atomic_get_connector_state(state, connector); + if (IS_ERR(connector_state)) + goto fail; + to_intel_connector(connector)->new_encoder = NULL; intel_encoder->new_crtc = NULL; intel_crtc->new_enabled = false; intel_crtc->new_config = NULL; + + connector_state->best_encoder = NULL; + connector_state->crtc = NULL; + intel_set_mode(crtc, NULL, 0, 0, NULL, state); drm_atomic_state_free(state); @@ -8990,6 +9007,11 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, /* Switch crtc and encoder back off if necessary */ if (old->dpms_mode != DRM_MODE_DPMS_ON) connector->funcs->dpms(connector, old->dpms_mode); + + return; +fail: + DRM_DEBUG_KMS("Couldn't release load detect pipe.\n"); + drm_atomic_state_free(state); } static int i9xx_pll_refclk(struct drm_device *dev, @@ -11646,9 +11668,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set, static int intel_modeset_stage_output_state(struct drm_device *dev, struct drm_mode_set *set, - struct intel_set_config *config) + struct intel_set_config *config, + struct drm_atomic_state *state) { struct intel_connector *connector; + struct drm_connector_state *connector_state; struct intel_encoder *encoder; struct intel_crtc *crtc; int ro; @@ -11712,6 +11736,14 @@ intel_modeset_stage_output_state(struct drm_device *dev, } connector->new_encoder->new_crtc = to_intel_crtc(new_crtc); + connector_state = + drm_atomic_get_connector_state(state, &connector->base); + if (IS_ERR(connector_state)) + return PTR_ERR(connector_state); + + connector_state->crtc = new_crtc; + connector_state->best_encoder = &connector->new_encoder->base; + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n", connector->base.base.id, connector->base.name, @@ -11744,9 +11776,15 @@ intel_modeset_stage_output_state(struct drm_device *dev, } /* Now we've also updated encoder->new_crtc for all encoders. */ for_each_intel_connector(dev, connector) { - if (connector->new_encoder) + connector_state = + drm_atomic_get_connector_state(state, &connector->base); + + if (connector->new_encoder) { if (connector->new_encoder != connector->encoder) connector->encoder = connector->new_encoder; + } else { + connector_state->crtc = NULL; + } } for_each_intel_crtc(dev, crtc) { crtc->new_enabled = false; @@ -11855,7 +11893,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) state->acquire_ctx = dev->mode_config.acquire_ctx; - ret = intel_modeset_stage_output_state(dev, set, config); + ret = intel_modeset_stage_output_state(dev, set, config, state); if (ret) goto fail;
With this in place, we can start converting pieces of the modeset code to look at the connector atomic state instead of the staged config. v2: Handle the load detect staged config changes too. (Ander) Remove unnecessary blank line. (Daniel) Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 52 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 7 deletions(-)