| Message ID | 20161219085828.5023-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote: > This gets rid of the last users of for_each_intel_connector(), remove > that too. > > The one exception is the loop in verify_encoder_state - that needs to > switch over to for_each_connector_in_state. > > v2: Maarten corrected me that in the state verifier we indeed must > only walk connectors in the atomic commit, not all of them! Ok, CI just told me this was a stupid idea. Since we don't walk all connectors, but still walk all encoders there's not plenty of state mismatches (e.g. if you have 2 crtc with different connectors enabled and you're doing a modeset on just one). Not exactly sure how to best fix this, since replacing the encoder walking with a connnector walking and then dereferencing connector->state->best_encoder to get at only the encoders relevant for us defeats the point of the cross check. -Daniel > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 ----- > drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++--------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 57914f008ed8..fe2b37fe0b91 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -488,11 +488,6 @@ struct i915_hotplug { > &(dev)->mode_config.encoder_list, \ > base.head) > > -#define for_each_intel_connector(dev, intel_connector) \ > - list_for_each_entry(intel_connector, \ > - &(dev)->mode_config.connector_list, \ > - base.head) > - > #define for_each_intel_connector_iter(intel_connector, iter) \ > while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter)))) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 438d27f93aca..9715c64eeb08 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12661,8 +12661,10 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { > static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > { > struct intel_connector *connector; > + struct drm_connector_list_iter conn_iter; > > - for_each_intel_connector(dev, connector) { > + drm_connector_list_iter_get(dev, &conn_iter); > + for_each_intel_connector_iter(connector, &conn_iter) { > if (connector->base.state->crtc) > drm_connector_unreference(&connector->base); > > @@ -12678,6 +12680,7 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > connector->base.state->crtc = NULL; > } > } > + drm_connector_list_iter_put(&conn_iter); > } > > static void > @@ -13606,10 +13609,12 @@ verify_connector_state(struct drm_device *dev, > } > > static void > -verify_encoder_state(struct drm_device *dev) > +verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state) > { > struct intel_encoder *encoder; > - struct intel_connector *connector; > + struct drm_connector *connector; > + struct drm_connector_state *old_conn_state; > + int i; > > for_each_intel_encoder(dev, encoder) { > bool enabled = false; > @@ -13619,12 +13624,12 @@ verify_encoder_state(struct drm_device *dev) > encoder->base.base.id, > encoder->base.name); > > - for_each_intel_connector(dev, connector) { > - if (connector->base.state->best_encoder != &encoder->base) > + for_each_connector_in_state(state, connector, old_conn_state, i) { > + if (connector->state->best_encoder != &encoder->base) > continue; > enabled = true; > > - I915_STATE_WARN(connector->base.state->crtc != > + I915_STATE_WARN(connector->state->crtc != > encoder->base.crtc, > "connector's crtc doesn't match encoder crtc\n"); > } > @@ -13827,7 +13832,7 @@ static void > intel_modeset_verify_disabled(struct drm_device *dev, > struct drm_atomic_state *state) > { > - verify_encoder_state(dev); > + verify_encoder_state(dev, state); > verify_connector_state(dev, state, NULL); > verify_disabled_dpll_state(dev); > } > @@ -16638,6 +16643,7 @@ int intel_modeset_init(struct drm_device *dev) > static void intel_enable_pipe_a(struct drm_device *dev) > { > struct intel_connector *connector; > + struct drm_connector_list_iter conn_iter; > struct drm_connector *crt = NULL; > struct intel_load_detect_pipe load_detect_temp; > struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx; > @@ -16645,12 +16651,14 @@ static void intel_enable_pipe_a(struct drm_device *dev) > /* We can't just switch on the pipe A, we need to set things up with a > * proper mode and output configuration. As a gross hack, enable pipe A > * by enabling the load detect pipe once. */ > - for_each_intel_connector(dev, connector) { > + drm_connector_list_iter_get(dev, &conn_iter); > + for_each_intel_connector_iter(connector, &conn_iter) { > if (connector->encoder->type == INTEL_OUTPUT_ANALOG) { > crt = &connector->base; > break; > } > } > + drm_connector_list_iter_put(&conn_iter); > > if (!crt) > return; > @@ -16896,6 +16904,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > struct intel_crtc *crtc; > struct intel_encoder *encoder; > struct intel_connector *connector; > + struct drm_connector_list_iter conn_iter; > int i; > > dev_priv->active_crtcs = 0; > @@ -16973,7 +16982,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > pipe_name(pipe)); > } > > - for_each_intel_connector(dev, connector) { > + drm_connector_list_iter_get(dev, &conn_iter); > + for_each_intel_connector_iter(connector, &conn_iter) { > if (connector->get_hw_state(connector)) { > connector->base.dpms = DRM_MODE_DPMS_ON; > > @@ -17001,6 +17011,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > connector->base.base.id, connector->base.name, > enableddisabled(connector->base.encoder)); > } > + drm_connector_list_iter_put(&conn_iter); > > for_each_intel_crtc(dev, crtc) { > crtc->base.hwmode = crtc->config->base.adjusted_mode; > -- > 2.11.0 >
Op 19-12-16 om 10:22 schreef Daniel Vetter: > On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote: >> This gets rid of the last users of for_each_intel_connector(), remove >> that too. >> >> The one exception is the loop in verify_encoder_state - that needs to >> switch over to for_each_connector_in_state. >> >> v2: Maarten corrected me that in the state verifier we indeed must >> only walk connectors in the atomic commit, not all of them! > Ok, CI just told me this was a stupid idea. Since we don't walk all > connectors, but still walk all encoders there's not plenty of state > mismatches (e.g. if you have 2 crtc with different connectors enabled and > you're doing a modeset on just one). > > Not exactly sure how to best fix this, since replacing the encoder > walking with a connnector walking and then dereferencing > connector->state->best_encoder to get at only the encoders relevant for us > defeats the point of the cross check. Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask. This way we'll have all encoders that we care about. ~Maarten
On Mon, Dec 19, 2016 at 10:47:25AM +0100, Maarten Lankhorst wrote: > Op 19-12-16 om 10:22 schreef Daniel Vetter: > > On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote: > >> This gets rid of the last users of for_each_intel_connector(), remove > >> that too. > >> > >> The one exception is the loop in verify_encoder_state - that needs to > >> switch over to for_each_connector_in_state. > >> > >> v2: Maarten corrected me that in the state verifier we indeed must > >> only walk connectors in the atomic commit, not all of them! > > Ok, CI just told me this was a stupid idea. Since we don't walk all > > connectors, but still walk all encoders there's not plenty of state > > mismatches (e.g. if you have 2 crtc with different connectors enabled and > > you're doing a modeset on just one). > > > > Not exactly sure how to best fix this, since replacing the encoder > > walking with a connnector walking and then dereferencing > > connector->state->best_encoder to get at only the encoders relevant for us > > defeats the point of the cross check. > Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask. > This way we'll have all encoders that we care about. Yeah, I think we have to rely on the correctness of the atomic states. For merging I think it's better to get v1 in to correct the connector_list enumeration, then fix up the encoder checking in a separate patch on top. Does that sound good? -Daniel
Op 20-12-16 om 19:29 schreef Daniel Vetter: > On Mon, Dec 19, 2016 at 10:47:25AM +0100, Maarten Lankhorst wrote: >> Op 19-12-16 om 10:22 schreef Daniel Vetter: >>> On Mon, Dec 19, 2016 at 09:58:28AM +0100, Daniel Vetter wrote: >>>> This gets rid of the last users of for_each_intel_connector(), remove >>>> that too. >>>> >>>> The one exception is the loop in verify_encoder_state - that needs to >>>> switch over to for_each_connector_in_state. >>>> >>>> v2: Maarten corrected me that in the state verifier we indeed must >>>> only walk connectors in the atomic commit, not all of them! >>> Ok, CI just told me this was a stupid idea. Since we don't walk all >>> connectors, but still walk all encoders there's not plenty of state >>> mismatches (e.g. if you have 2 crtc with different connectors enabled and >>> you're doing a modeset on just one). >>> >>> Not exactly sure how to best fix this, since replacing the encoder >>> walking with a connnector walking and then dereferencing >>> connector->state->best_encoder to get at only the encoders relevant for us >>> defeats the point of the cross check. >> Assuming we trust the atomic state, we could fix this by iterating either state and ORing (old,new)_crtc_state->encoder_mask. >> This way we'll have all encoders that we care about. > Yeah, I think we have to rely on the correctness of the atomic states. For > merging I think it's better to get v1 in to correct the connector_list > enumeration, then fix up the encoder checking in a separate patch on top. > Does that sound good? Yes sounds good. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 57914f008ed8..fe2b37fe0b91 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -488,11 +488,6 @@ struct i915_hotplug { &(dev)->mode_config.encoder_list, \ base.head) -#define for_each_intel_connector(dev, intel_connector) \ - list_for_each_entry(intel_connector, \ - &(dev)->mode_config.connector_list, \ - base.head) - #define for_each_intel_connector_iter(intel_connector, iter) \ while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter)))) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 438d27f93aca..9715c64eeb08 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12661,8 +12661,10 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) { struct intel_connector *connector; + struct drm_connector_list_iter conn_iter; - for_each_intel_connector(dev, connector) { + drm_connector_list_iter_get(dev, &conn_iter); + for_each_intel_connector_iter(connector, &conn_iter) { if (connector->base.state->crtc) drm_connector_unreference(&connector->base); @@ -12678,6 +12680,7 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) connector->base.state->crtc = NULL; } } + drm_connector_list_iter_put(&conn_iter); } static void @@ -13606,10 +13609,12 @@ verify_connector_state(struct drm_device *dev, } static void -verify_encoder_state(struct drm_device *dev) +verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state) { struct intel_encoder *encoder; - struct intel_connector *connector; + struct drm_connector *connector; + struct drm_connector_state *old_conn_state; + int i; for_each_intel_encoder(dev, encoder) { bool enabled = false; @@ -13619,12 +13624,12 @@ verify_encoder_state(struct drm_device *dev) encoder->base.base.id, encoder->base.name); - for_each_intel_connector(dev, connector) { - if (connector->base.state->best_encoder != &encoder->base) + for_each_connector_in_state(state, connector, old_conn_state, i) { + if (connector->state->best_encoder != &encoder->base) continue; enabled = true; - I915_STATE_WARN(connector->base.state->crtc != + I915_STATE_WARN(connector->state->crtc != encoder->base.crtc, "connector's crtc doesn't match encoder crtc\n"); } @@ -13827,7 +13832,7 @@ static void intel_modeset_verify_disabled(struct drm_device *dev, struct drm_atomic_state *state) { - verify_encoder_state(dev); + verify_encoder_state(dev, state); verify_connector_state(dev, state, NULL); verify_disabled_dpll_state(dev); } @@ -16638,6 +16643,7 @@ int intel_modeset_init(struct drm_device *dev) static void intel_enable_pipe_a(struct drm_device *dev) { struct intel_connector *connector; + struct drm_connector_list_iter conn_iter; struct drm_connector *crt = NULL; struct intel_load_detect_pipe load_detect_temp; struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx; @@ -16645,12 +16651,14 @@ static void intel_enable_pipe_a(struct drm_device *dev) /* We can't just switch on the pipe A, we need to set things up with a * proper mode and output configuration. As a gross hack, enable pipe A * by enabling the load detect pipe once. */ - for_each_intel_connector(dev, connector) { + drm_connector_list_iter_get(dev, &conn_iter); + for_each_intel_connector_iter(connector, &conn_iter) { if (connector->encoder->type == INTEL_OUTPUT_ANALOG) { crt = &connector->base; break; } } + drm_connector_list_iter_put(&conn_iter); if (!crt) return; @@ -16896,6 +16904,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) struct intel_crtc *crtc; struct intel_encoder *encoder; struct intel_connector *connector; + struct drm_connector_list_iter conn_iter; int i; dev_priv->active_crtcs = 0; @@ -16973,7 +16982,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) pipe_name(pipe)); } - for_each_intel_connector(dev, connector) { + drm_connector_list_iter_get(dev, &conn_iter); + for_each_intel_connector_iter(connector, &conn_iter) { if (connector->get_hw_state(connector)) { connector->base.dpms = DRM_MODE_DPMS_ON; @@ -17001,6 +17011,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) connector->base.base.id, connector->base.name, enableddisabled(connector->base.encoder)); } + drm_connector_list_iter_put(&conn_iter); for_each_intel_crtc(dev, crtc) { crtc->base.hwmode = crtc->config->base.adjusted_mode;
This gets rid of the last users of for_each_intel_connector(), remove that too. The one exception is the loop in verify_encoder_state - that needs to switch over to for_each_connector_in_state. v2: Maarten corrected me that in the state verifier we indeed must only walk connectors in the atomic commit, not all of them! Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 ----- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 14 deletions(-)