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@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>
async is not the only issue, crtc->state may disappear underneath too. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
