Op 18-05-15 om 17:30 schreef Daniel Vetter:
> On Wed, May 13, 2015 at 10:23:37PM +0200, Maarten Lankhorst wrote:
>> crtc_state->enable means a crtc is configured, but it may be turned
>> off for dpms. Until the previous commit crtc_state->active was not
>> updated on crtc off, but now that we do we should use that for tracking
>> whether a crtc is scanning out or not.
>>
>> At this point crtc->active should mirror crtc_state->active,
>> so some paranoia from the crtc_disable functions can be removed.
>>
>> Note that intel_set_mode_setup_plls still checks for ->enable,
>> because all resources that are needed have to be calculated, so
>> dpms changes will still succeed.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> A few detail comments below.
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
>> 2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 557af8877a2e..ca457317a8ac 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>> return -EINVAL;
>> }
>>
>> - if (!crtc->state->enable) {
>> + if (!crtc->state->active) {
> This change looks unjustified I think.
Why? Can you get a vblank on crtc that is dpms off? I think not..
Either way later on I use hwmode->crtc_clock which renders this moot.
>> <snip>
>> @@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>>
>> /* add all active pipes to the state */
>> for_each_crtc(state->dev, crtc) {
>> - if (!crtc->state->enable)
>> + if (!crtc->state->active)
> This is a functional change to the cdclk code and might break it and/or
> conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to
> be split out.
No this function just sets mode_changed on all active crtc's.
This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off.
>> continue;
>>
>> crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> @@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>>
>> /* disable/enable all currently active pipes while we change cdclk */
>> for_each_crtc_in_state(state, crtc, crtc_state, i)
>> - if (crtc_state->enable)
>> + if (crtc_state->active)
>> crtc_state->mode_changed = true;
> Same here.
>
> Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and
> instead just look at crtc_state->mode_changed? That way we don't need to
> duplicate the same checks twice, once to set ->mode_changed and once to
> for the prepare_pipes mask. Or is that duplication already getting
> removed?
I think we could get rid of a lot of those extra loops if we want to later,
but for now readability is more important.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
