| Message ID | 1438000541-3219-7-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote: > Instead of allocating pipe_config on the stack use the old crtc_state, > it's only going to freed from this point on. > > All crtc's encoders are now only checked once during modeset. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------ > 1 file changed, 56 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fbb257d4728c..e3afe611a78c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc_state); > struct drm_atomic_state *state = crtc_state->state; > - int ret, idx = crtc->base.id; > + int ret; > bool mode_changed = needs_modeset(crtc_state); > > if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { > @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > return -EINVAL; > } > > - I915_STATE_WARN(crtc->state->active != intel_crtc->active, > - "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", > - idx, crtc->state->active, intel_crtc->active); > - > if (mode_changed && !crtc_state->active) > intel_crtc->atomic.update_wm_post = true; > > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev) > > for_each_intel_encoder(dev, encoder) { > bool enabled = false; > - bool active = false; > - enum pipe pipe, tracked_pipe; > + enum pipe pipe; > > DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", > encoder->base.base.id, > encoder->base.name); > > for_each_intel_connector(dev, connector) { > - if (connector->base.encoder != &encoder->base) > + if (connector->base.state->best_encoder != &encoder->base) > continue; > enabled = true; > - if (connector->base.dpms != DRM_MODE_DPMS_OFF) > - active = true; > > I915_STATE_WARN(connector->base.state->crtc != > encoder->base.crtc, > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev) > "encoder's enabled state mismatch " > "(expected %i, found %i)\n", > !!encoder->base.crtc, enabled); > - I915_STATE_WARN(active && !encoder->base.crtc, > - "active encoder with no crtc\n"); > - > - active = encoder->get_hw_state(encoder, &pipe); > > if (!encoder->base.crtc) { > - I915_STATE_WARN(active, > - "encoder detached but not turned off.\n"); > + bool active; > > - continue; > + active = encoder->get_hw_state(encoder, &pipe); > + I915_STATE_WARN(active, > + "encoder detached but still enabled on pipe %c.\n", > + pipe_name(pipe)); > } > - > - I915_STATE_WARN(active != encoder->base.crtc->state->active, > - "encoder's hw state doesn't match sw tracking " > - "(expected %i, found %i)\n", > - encoder->base.crtc->state->active, active); > - > - > - tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe; > - I915_STATE_WARN(active && pipe != tracked_pipe, > - "active encoder's pipe doesn't match" > - "(expected %i, found %i)\n", > - tracked_pipe, pipe); > - > } > } > > static void > -check_crtc_state(struct drm_device *dev) > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *crtc; > struct intel_encoder *encoder; > - struct intel_crtc_state pipe_config; > + struct drm_crtc_state *crtc_state; > + struct drm_crtc *crtc; > + int i; > > - for_each_intel_crtc(dev, crtc) { > + for_each_crtc_in_state(state, crtc, crtc_state, i) { So now we only check the state of crtcs affected by the modeset. In the unlikely case of a bug that changes the hw state of another crtc that should have been unchanged, the old code might catch the issue but the new one doesn't. Isn't it better to just check everything? Ander > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *pipe_config, *sw_config; > bool active; > > - memset(&pipe_config, 0, sizeof(pipe_config)); > + if (!needs_modeset(crtc->state)) > + continue; > > - DRM_DEBUG_KMS("[CRTC:%d]\n", > - crtc->base.base.id); > + __drm_atomic_helper_crtc_destroy_state(crtc, crtc_state); > + pipe_config = to_intel_crtc_state(crtc_state); > + memset(pipe_config, 0, sizeof(*pipe_config)); > + crtc_state->crtc = crtc; > + crtc_state->state = state; > > - I915_STATE_WARN(crtc->active && !crtc->base.state->enable, > - "active crtc, but not enabled in sw tracking\n"); > + DRM_DEBUG_KMS("[CRTC:%d]\n", > + crtc->base.id); > > - active = dev_priv->display.get_pipe_config(crtc, > - &pipe_config); > + active = dev_priv->display.get_pipe_config(intel_crtc, > + pipe_config); > > /* hw state is inconsistent with the pipe quirk */ > - if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) || > - (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)) > - active = crtc->active; > + if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) || > + (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)) > + active = crtc->state->active; > > - for_each_intel_encoder(dev, encoder) { > - enum pipe pipe; > - if (encoder->base.crtc != &crtc->base) > - continue; > - if (encoder->get_hw_state(encoder, &pipe)) > - encoder->get_config(encoder, &pipe_config); > - } > - > - I915_STATE_WARN(crtc->active != active, > + I915_STATE_WARN(crtc->state->active != active, > "crtc active state doesn't match with hw state " > - "(expected %i, found %i)\n", crtc->active, active); > + "(expected %i, found %i)\n", crtc->state->active, active); > > - I915_STATE_WARN(crtc->active != crtc->base.state->active, > + I915_STATE_WARN(intel_crtc->active != crtc->state->active, > "transitional active state does not match atomic hw state " > - "(expected %i, found %i)\n", crtc->base.state->active, crtc->active); > + "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active); > + > + for_each_encoder_on_crtc(dev, crtc, encoder) { > + enum pipe pipe; > + > + active = encoder->get_hw_state(encoder, &pipe); > + I915_STATE_WARN(active != crtc->state->active, > + "[ENCODER:%i] active %i with crtc active %i\n", > + encoder->base.base.id, active, crtc->state->active); > + > + I915_STATE_WARN(active && intel_crtc->pipe != pipe, > + "Encoder connected to wrong pipe %c\n", > + pipe_name(pipe)); > + > + if (active) > + encoder->get_config(encoder, pipe_config); > + } > > - if (!active) > + if (!crtc->state->active) > continue; > > - if (!intel_pipe_config_compare(dev, crtc->config, > - &pipe_config, false)) { > + sw_config = to_intel_crtc_state(crtc->state); > + if (!intel_pipe_config_compare(dev, sw_config, > + pipe_config, false)) { > I915_STATE_WARN(1, "pipe state doesn't match!\n"); > - intel_dump_pipe_config(crtc, &pipe_config, > + intel_dump_pipe_config(intel_crtc, pipe_config, > "[hw state]"); > - intel_dump_pipe_config(crtc, crtc->config, > + intel_dump_pipe_config(intel_crtc, sw_config, > "[sw state]"); > } > } > @@ -12884,7 +12878,7 @@ intel_modeset_check_state(struct drm_device *dev, > check_wm_state(dev); > check_connector_state(dev, state); > check_encoder_state(dev); > - check_crtc_state(dev); > + check_crtc_state(dev, state); > check_shared_dpll_state(dev); > } >
On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira wrote: > On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote: > > Instead of allocating pipe_config on the stack use the old crtc_state, > > it's only going to freed from this point on. > > > > All crtc's encoders are now only checked once during modeset. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------ > > 1 file changed, 56 insertions(+), 62 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index fbb257d4728c..e3afe611a78c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > > struct intel_crtc_state *pipe_config = > > to_intel_crtc_state(crtc_state); > > struct drm_atomic_state *state = crtc_state->state; > > - int ret, idx = crtc->base.id; > > + int ret; > > bool mode_changed = needs_modeset(crtc_state); > > > > if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { > > @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > > return -EINVAL; > > } > > > > - I915_STATE_WARN(crtc->state->active != intel_crtc->active, > > - "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", > > - idx, crtc->state->active, intel_crtc->active); > > - > > if (mode_changed && !crtc_state->active) > > intel_crtc->atomic.update_wm_post = true; > > > > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev) > > > > for_each_intel_encoder(dev, encoder) { > > bool enabled = false; > > - bool active = false; > > - enum pipe pipe, tracked_pipe; > > + enum pipe pipe; > > > > DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", > > encoder->base.base.id, > > encoder->base.name); > > > > for_each_intel_connector(dev, connector) { > > - if (connector->base.encoder != &encoder->base) > > + if (connector->base.state->best_encoder != &encoder->base) > > continue; > > enabled = true; > > - if (connector->base.dpms != DRM_MODE_DPMS_OFF) > > - active = true; > > > > I915_STATE_WARN(connector->base.state->crtc != > > encoder->base.crtc, > > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev) > > "encoder's enabled state mismatch " > > "(expected %i, found %i)\n", > > !!encoder->base.crtc, enabled); > > - I915_STATE_WARN(active && !encoder->base.crtc, > > - "active encoder with no crtc\n"); > > - > > - active = encoder->get_hw_state(encoder, &pipe); > > > > if (!encoder->base.crtc) { > > - I915_STATE_WARN(active, > > - "encoder detached but not turned off.\n"); > > + bool active; > > > > - continue; > > + active = encoder->get_hw_state(encoder, &pipe); > > + I915_STATE_WARN(active, > > + "encoder detached but still enabled on pipe %c.\n", > > + pipe_name(pipe)); > > } > > - > > - I915_STATE_WARN(active != encoder->base.crtc->state->active, > > - "encoder's hw state doesn't match sw tracking " > > - "(expected %i, found %i)\n", > > - encoder->base.crtc->state->active, active); > > - > > - > > - tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe; > > - I915_STATE_WARN(active && pipe != tracked_pipe, > > - "active encoder's pipe doesn't match" > > - "(expected %i, found %i)\n", > > - tracked_pipe, pipe); > > - > > } > > } > > > > static void > > -check_crtc_state(struct drm_device *dev) > > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > - struct intel_crtc *crtc; > > struct intel_encoder *encoder; > > - struct intel_crtc_state pipe_config; > > + struct drm_crtc_state *crtc_state; > > + struct drm_crtc *crtc; > > + int i; > > > > - for_each_intel_crtc(dev, crtc) { > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > So now we only check the state of crtcs affected by the modeset. In the > unlikely case of a bug that changes the hw state of another crtc that > should have been unchanged, the old code might catch the issue but the > new one doesn't. Isn't it better to just check everything? We can't since that other crtc might be doing some other async commit. But eventually we should be doing a modeset on that one too and spot that something went wrong. -Daniel
On Wed, 2015-07-29 at 14:04 +0200, Daniel Vetter wrote: > On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira > wrote: > > On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote: > > > Instead of allocating pipe_config on the stack use the old > > > crtc_state, > > > it's only going to freed from this point on. > > > > > > All crtc's encoders are now only checked once during modeset. > > > > > > Signed-off-by: Maarten Lankhorst < > > > maarten.lankhorst@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++---- > > > -------------- > > > 1 file changed, 56 insertions(+), 62 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index fbb257d4728c..e3afe611a78c 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct > > > drm_crtc *crtc, > > > struct intel_crtc_state *pipe_config = > > > to_intel_crtc_state(crtc_state); > > > struct drm_atomic_state *state = crtc_state->state; > > > - int ret, idx = crtc->base.id; > > > + int ret; > > > bool mode_changed = needs_modeset(crtc_state); > > > > > > if (mode_changed && !check_encoder_cloning(state, > > > intel_crtc)) { > > > @@ -11837,10 +11837,6 @@ static int > > > intel_crtc_atomic_check(struct drm_crtc *crtc, > > > return -EINVAL; > > > } > > > > > > - I915_STATE_WARN(crtc->state->active != intel_crtc > > > ->active, > > > - "[CRTC:%i] mismatch between state->active(%i) > > > and crtc->active(%i)\n", > > > - idx, crtc->state->active, intel_crtc->active); > > > - > > > if (mode_changed && !crtc_state->active) > > > intel_crtc->atomic.update_wm_post = true; > > > > > > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device > > > *dev) > > > > > > for_each_intel_encoder(dev, encoder) { > > > bool enabled = false; > > > - bool active = false; > > > - enum pipe pipe, tracked_pipe; > > > + enum pipe pipe; > > > > > > DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", > > > encoder->base.base.id, > > > encoder->base.name); > > > > > > for_each_intel_connector(dev, connector) { > > > - if (connector->base.encoder != &encoder > > > ->base) > > > + if (connector->base.state->best_encoder > > > != &encoder->base) > > > continue; > > > enabled = true; > > > - if (connector->base.dpms != > > > DRM_MODE_DPMS_OFF) > > > - active = true; > > > > > > I915_STATE_WARN(connector->base.state > > > ->crtc != > > > encoder->base.crtc, > > > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device > > > *dev) > > > "encoder's enabled state mismatch " > > > "(expected %i, found %i)\n", > > > !!encoder->base.crtc, enabled); > > > - I915_STATE_WARN(active && !encoder->base.crtc, > > > - "active encoder with no crtc\n"); > > > - > > > - active = encoder->get_hw_state(encoder, &pipe); > > > > > > if (!encoder->base.crtc) { > > > - I915_STATE_WARN(active, > > > - "encoder detached but not turned > > > off.\n"); > > > + bool active; > > > > > > - continue; > > > + active = encoder->get_hw_state(encoder, > > > &pipe); > > > + I915_STATE_WARN(active, > > > + "encoder detached but still enabled > > > on pipe %c.\n", > > > + pipe_name(pipe)); > > > } > > > - > > > - I915_STATE_WARN(active != encoder->base.crtc > > > ->state->active, > > > - "encoder's hw state doesn't match sw > > > tracking " > > > - "(expected %i, found %i)\n", > > > - encoder->base.crtc->state->active, active); > > > - > > > - > > > - tracked_pipe = to_intel_crtc(encoder->base.crtc) > > > ->pipe; > > > - I915_STATE_WARN(active && pipe != tracked_pipe, > > > - "active encoder's pipe doesn't match" > > > - "(expected %i, found %i)\n", > > > - tracked_pipe, pipe); > > > - > > > } > > > } > > > > > > static void > > > -check_crtc_state(struct drm_device *dev) > > > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state > > > *state) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > - struct intel_crtc *crtc; > > > struct intel_encoder *encoder; > > > - struct intel_crtc_state pipe_config; > > > + struct drm_crtc_state *crtc_state; > > > + struct drm_crtc *crtc; > > > + int i; > > > > > > - for_each_intel_crtc(dev, crtc) { > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > > So now we only check the state of crtcs affected by the modeset. In > > the > > unlikely case of a bug that changes the hw state of another crtc > > that > > should have been unchanged, the old code might catch the issue but > > the > > new one doesn't. Isn't it better to just check everything? > > We can't since that other crtc might be doing some other async > commit. But > eventually we should be doing a modeset on that one too and spot that > something went wrong. Right, I forgot about async. Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Op 29-07-15 om 14:31 schreef Ander Conselvan De Oliveira: > On Wed, 2015-07-29 at 14:04 +0200, Daniel Vetter wrote: >> On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira >> wrote: >>> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote: >>>> Instead of allocating pipe_config on the stack use the old >>>> crtc_state, >>>> it's only going to freed from this point on. >>>> >>>> All crtc's encoders are now only checked once during modeset. >>>> >>>> Signed-off-by: Maarten Lankhorst < >>>> maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++---- >>>> -------------- >>>> 1 file changed, 56 insertions(+), 62 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>>> b/drivers/gpu/drm/i915/intel_display.c >>>> index fbb257d4728c..e3afe611a78c 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct >>>> drm_crtc *crtc, >>>> struct intel_crtc_state *pipe_config = >>>> to_intel_crtc_state(crtc_state); >>>> struct drm_atomic_state *state = crtc_state->state; >>>> - int ret, idx = crtc->base.id; >>>> + int ret; >>>> bool mode_changed = needs_modeset(crtc_state); >>>> >>>> if (mode_changed && !check_encoder_cloning(state, >>>> intel_crtc)) { >>>> @@ -11837,10 +11837,6 @@ static int >>>> intel_crtc_atomic_check(struct drm_crtc *crtc, >>>> return -EINVAL; >>>> } >>>> >>>> - I915_STATE_WARN(crtc->state->active != intel_crtc >>>> ->active, >>>> - "[CRTC:%i] mismatch between state->active(%i) >>>> and crtc->active(%i)\n", >>>> - idx, crtc->state->active, intel_crtc->active); >>>> - >>>> if (mode_changed && !crtc_state->active) >>>> intel_crtc->atomic.update_wm_post = true; >>>> >>>> @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device >>>> *dev) >>>> >>>> for_each_intel_encoder(dev, encoder) { >>>> bool enabled = false; >>>> - bool active = false; >>>> - enum pipe pipe, tracked_pipe; >>>> + enum pipe pipe; >>>> >>>> DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", >>>> encoder->base.base.id, >>>> encoder->base.name); >>>> >>>> for_each_intel_connector(dev, connector) { >>>> - if (connector->base.encoder != &encoder >>>> ->base) >>>> + if (connector->base.state->best_encoder >>>> != &encoder->base) >>>> continue; >>>> enabled = true; >>>> - if (connector->base.dpms != >>>> DRM_MODE_DPMS_OFF) >>>> - active = true; >>>> >>>> I915_STATE_WARN(connector->base.state >>>> ->crtc != >>>> encoder->base.crtc, >>>> @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device >>>> *dev) >>>> "encoder's enabled state mismatch " >>>> "(expected %i, found %i)\n", >>>> !!encoder->base.crtc, enabled); >>>> - I915_STATE_WARN(active && !encoder->base.crtc, >>>> - "active encoder with no crtc\n"); >>>> - >>>> - active = encoder->get_hw_state(encoder, &pipe); >>>> >>>> if (!encoder->base.crtc) { >>>> - I915_STATE_WARN(active, >>>> - "encoder detached but not turned >>>> off.\n"); >>>> + bool active; >>>> >>>> - continue; >>>> + active = encoder->get_hw_state(encoder, >>>> &pipe); >>>> + I915_STATE_WARN(active, >>>> + "encoder detached but still enabled >>>> on pipe %c.\n", >>>> + pipe_name(pipe)); >>>> } >>>> - >>>> - I915_STATE_WARN(active != encoder->base.crtc >>>> ->state->active, >>>> - "encoder's hw state doesn't match sw >>>> tracking " >>>> - "(expected %i, found %i)\n", >>>> - encoder->base.crtc->state->active, active); >>>> - >>>> - >>>> - tracked_pipe = to_intel_crtc(encoder->base.crtc) >>>> ->pipe; >>>> - I915_STATE_WARN(active && pipe != tracked_pipe, >>>> - "active encoder's pipe doesn't match" >>>> - "(expected %i, found %i)\n", >>>> - tracked_pipe, pipe); >>>> - >>>> } >>>> } >>>> >>>> static void >>>> -check_crtc_state(struct drm_device *dev) >>>> +check_crtc_state(struct drm_device *dev, struct drm_atomic_state >>>> *state) >>>> { >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> - struct intel_crtc *crtc; >>>> struct intel_encoder *encoder; >>>> - struct intel_crtc_state pipe_config; >>>> + struct drm_crtc_state *crtc_state; >>>> + struct drm_crtc *crtc; >>>> + int i; >>>> >>>> - for_each_intel_crtc(dev, crtc) { >>>> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >>> So now we only check the state of crtcs affected by the modeset. In >>> the >>> unlikely case of a bug that changes the hw state of another crtc >>> that >>> should have been unchanged, the old code might catch the issue but >>> the >>> new one doesn't. Isn't it better to just check everything? >> We can't since that other crtc might be doing some other async >> commit. But >> eventually we should be doing a modeset on that one too and spot that >> something went wrong. > Right, I forgot about async. > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> async is not the only issue, crtc->state may disappear underneath too. :-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fbb257d4728c..e3afe611a78c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); struct drm_atomic_state *state = crtc_state->state; - int ret, idx = crtc->base.id; + int ret; bool mode_changed = needs_modeset(crtc_state); if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, return -EINVAL; } - I915_STATE_WARN(crtc->state->active != intel_crtc->active, - "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", - idx, crtc->state->active, intel_crtc->active); - if (mode_changed && !crtc_state->active) intel_crtc->atomic.update_wm_post = true; @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev) for_each_intel_encoder(dev, encoder) { bool enabled = false; - bool active = false; - enum pipe pipe, tracked_pipe; + enum pipe pipe; DRM_DEBUG_KMS("[ENCODER:%d:%s]\n", encoder->base.base.id, encoder->base.name); for_each_intel_connector(dev, connector) { - if (connector->base.encoder != &encoder->base) + if (connector->base.state->best_encoder != &encoder->base) continue; enabled = true; - if (connector->base.dpms != DRM_MODE_DPMS_OFF) - active = true; I915_STATE_WARN(connector->base.state->crtc != encoder->base.crtc, @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev) "encoder's enabled state mismatch " "(expected %i, found %i)\n", !!encoder->base.crtc, enabled); - I915_STATE_WARN(active && !encoder->base.crtc, - "active encoder with no crtc\n"); - - active = encoder->get_hw_state(encoder, &pipe); if (!encoder->base.crtc) { - I915_STATE_WARN(active, - "encoder detached but not turned off.\n"); + bool active; - continue; + active = encoder->get_hw_state(encoder, &pipe); + I915_STATE_WARN(active, + "encoder detached but still enabled on pipe %c.\n", + pipe_name(pipe)); } - - I915_STATE_WARN(active != encoder->base.crtc->state->active, - "encoder's hw state doesn't match sw tracking " - "(expected %i, found %i)\n", - encoder->base.crtc->state->active, active); - - - tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe; - I915_STATE_WARN(active && pipe != tracked_pipe, - "active encoder's pipe doesn't match" - "(expected %i, found %i)\n", - tracked_pipe, pipe); - } } static void -check_crtc_state(struct drm_device *dev) +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *crtc; struct intel_encoder *encoder; - struct intel_crtc_state pipe_config; + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + int i; - for_each_intel_crtc(dev, crtc) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *pipe_config, *sw_config; bool active; - memset(&pipe_config, 0, sizeof(pipe_config)); + if (!needs_modeset(crtc->state)) + continue; - DRM_DEBUG_KMS("[CRTC:%d]\n", - crtc->base.base.id); + __drm_atomic_helper_crtc_destroy_state(crtc, crtc_state); + pipe_config = to_intel_crtc_state(crtc_state); + memset(pipe_config, 0, sizeof(*pipe_config)); + crtc_state->crtc = crtc; + crtc_state->state = state; - I915_STATE_WARN(crtc->active && !crtc->base.state->enable, - "active crtc, but not enabled in sw tracking\n"); + DRM_DEBUG_KMS("[CRTC:%d]\n", + crtc->base.id); - active = dev_priv->display.get_pipe_config(crtc, - &pipe_config); + active = dev_priv->display.get_pipe_config(intel_crtc, + pipe_config); /* hw state is inconsistent with the pipe quirk */ - if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) || - (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)) - active = crtc->active; + if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) || + (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)) + active = crtc->state->active; - for_each_intel_encoder(dev, encoder) { - enum pipe pipe; - if (encoder->base.crtc != &crtc->base) - continue; - if (encoder->get_hw_state(encoder, &pipe)) - encoder->get_config(encoder, &pipe_config); - } - - I915_STATE_WARN(crtc->active != active, + I915_STATE_WARN(crtc->state->active != active, "crtc active state doesn't match with hw state " - "(expected %i, found %i)\n", crtc->active, active); + "(expected %i, found %i)\n", crtc->state->active, active); - I915_STATE_WARN(crtc->active != crtc->base.state->active, + I915_STATE_WARN(intel_crtc->active != crtc->state->active, "transitional active state does not match atomic hw state " - "(expected %i, found %i)\n", crtc->base.state->active, crtc->active); + "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active); + + for_each_encoder_on_crtc(dev, crtc, encoder) { + enum pipe pipe; + + active = encoder->get_hw_state(encoder, &pipe); + I915_STATE_WARN(active != crtc->state->active, + "[ENCODER:%i] active %i with crtc active %i\n", + encoder->base.base.id, active, crtc->state->active); + + I915_STATE_WARN(active && intel_crtc->pipe != pipe, + "Encoder connected to wrong pipe %c\n", + pipe_name(pipe)); + + if (active) + encoder->get_config(encoder, pipe_config); + } - if (!active) + if (!crtc->state->active) continue; - if (!intel_pipe_config_compare(dev, crtc->config, - &pipe_config, false)) { + sw_config = to_intel_crtc_state(crtc->state); + if (!intel_pipe_config_compare(dev, sw_config, + pipe_config, false)) { I915_STATE_WARN(1, "pipe state doesn't match!\n"); - intel_dump_pipe_config(crtc, &pipe_config, + intel_dump_pipe_config(intel_crtc, pipe_config, "[hw state]"); - intel_dump_pipe_config(crtc, crtc->config, + intel_dump_pipe_config(intel_crtc, sw_config, "[sw state]"); } } @@ -12884,7 +12878,7 @@ intel_modeset_check_state(struct drm_device *dev, check_wm_state(dev); check_connector_state(dev, state); check_encoder_state(dev); - check_crtc_state(dev); + check_crtc_state(dev, state); check_shared_dpll_state(dev); }
Instead of allocating pipe_config on the stack use the old crtc_state, it's only going to freed from this point on. All crtc's encoders are now only checked once during modeset. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------ 1 file changed, 56 insertions(+), 62 deletions(-)