[Intel-gfx] [PATCH 05/12] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v2.

[Daniel Stone]

Hi,

On 25 November 2015 at 12:21, Ander Conselvan De Oliveira
<conselvan2 at gmail.com> wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>> @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc
>> *crtc)
>>       if (atomic->post_enable_primary)
>>               intel_post_enable_primary(&crtc->base);
>>
>> +     if (needs_modeset(&pipe_config->base))
>> +             intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>> +
>>       memset(atomic, 0, sizeof(*atomic));
>>  }
>>
>> @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>>       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>               struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>               bool modeset = needs_modeset(crtc->state);
>> -             bool update_pipe = !modeset &&
>> -                     to_intel_crtc_state(crtc->state)->update_pipe;
>> +             struct intel_crtc_state *pipe_config =
>> +                     to_intel_crtc_state(crtc->state);
>> +             bool update_pipe = !modeset && pipe_config->update_pipe;
>>               unsigned long put_domains = 0;
>>
>>               if (modeset)
>> @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device
>> *dev,
>>                   (crtc->state->planes_changed || update_pipe))
>>                       drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>>
>> +             if (pipe_config->base.active &&
>> +                 (pipe_config->wm_changed || pipe_config->disable_cxsr ||
>> +                  pipe_config->fb_changed))
>> +                     crtc_vblank_mask |= 1 << i;
>> +
>>               if (put_domains)
>>                       modeset_put_power_domains(dev_priv, put_domains);
>> -
>> -             intel_post_plane_update(intel_crtc);
>> -
>> -             if (modeset)
>> -                     intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>
> Would it be too evil to move the power domain get/put out of the loop? If I
> understand correctly, intel_state->modeset already tells us if there is any crtc
> that needs a modeset, so we could just protect the calls with that. I'm not sure
> what is the impact, but at least we could avoid having the get in
> intel_atomic_commit() and the put in intel_post_plane_enable().

Even better would be if POWER_DOMAIN_MODESET was a part of
put_domains, and all domains were put after completion. I suggested
this during review of Patrik's series, but that seems to have got
lost. :(

Cheers,
Daniel