[Intel-gfx] [PATCH 04/42] drm/i915: use intel_crtc_control everywhere

[Daniel Stone]

Hi,

On 12 May 2015 at 14:16, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, May 12, 2015 at 02:06:39PM +0200, Maarten Lankhorst wrote:
>> Op 11-05-15 om 19:11 schreef Daniel Vetter:
>> > On Mon, May 11, 2015 at 04:24:40PM +0200, Maarten Lankhorst wrote:
>> >> @@ -6079,26 +6059,29 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>> >>    enum intel_display_power_domain domain;
>> >>    unsigned long domains;
>> >>
>> >> +  if (enable == intel_crtc->active)
>> >> +          return;
>> >> +
>> >> +  if (enable && !crtc->state->enable)
>> >> +          return;
>> > I think we only need to check for !state->enable here. Changing dpms while
>> > the crtc is fully of is totally legit. And at least -modesetting loves to
>> > do just that iirc.
>> >
>> > I'll will be caught by state->active implying state->enable, but that's
>> > hard to read imo.
>> > -Daniel
>> As discussed on irc it's not. :-)
>
> Hm there's actually a bug in drm_atomic_helper_connector_dpms I think ...
> we seem to unconditionally update crtc_state->active.
>
> Oh I'm missing that ->enable == false also implies that no connectors are
> connected to the crtc, so we can't ever end up setting this to true. So
> indeed changing active while enable == false is impossible.

Not the only place we seem to rather carelessly do this, mind:
https://git.collabora.com/cgit/user/daniels/linux.git/commit/?h=wip/4.1.x/unify-flip-modeset&id=132a1d4086e20cac95094c7fd568f992e4a0cb6d

Cheers,
Daniel