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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
