[Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state

[Maarten Lankhorst]

Op 02-09-15 om 12:35 schreef Ville Syrjälä:
> On Wed, Sep 02, 2015 at 07:15:25AM +0200, Maarten Lankhorst wrote:
>> Op 01-09-15 om 17:48 schreef Ville Syrjälä:
>>> On Tue, Sep 01, 2015 at 08:30:05AM -0700, Matt Roper wrote:
>>>> On Tue, Sep 01, 2015 at 07:24:19AM +0200, Maarten Lankhorst wrote:
>>>>> Op 29-08-15 om 01:57 schreef Matt Roper:
>>>>>> Way back at the beginning of i915's atomic conversion I added
>>>>>> intel_crtc->atomic as a temporary dumping ground for "stuff to do
>>>>>> outside vblank evasion" flags since CRTC states weren't properly wired
>>>>>> up and tracked at that time.  We've had proper CRTC state tracking for a
>>>>>> while now, so there's really no reason for this hack to continue to
>>>>>> exist.  Moving forward we want to store intermediate crtc/plane state
>>>>>> data for modesets in addition to the final state, so moving these fields
>>>>>> into the proper state object allows us to properly compute them for both
>>>>>> the intermediate and final state.
>>>>>>
>>>>>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>>>>> ---
>>>>> Can I shoot this patch down? It's better to add a field 'wm_changed'
>>>>> to the crtc_state, which gets reset to false for each crtc_state
>>>>> duplication. It's needed for checking if a cs pageflip can be done for
>>>>> atomic. It would remove the duplication of some checks there.
>>>>>
>>>>> The other atomic state members will die soon. I already have some
>>>>> patches to achieve that. :)
>>>>>
>>>>> I'm not sure if an intermediate state is a good idea. Any code that
>>>>> disables a crtc should only be looking at the old state.
>>>>> pre_plane_update runs all stuff in preparation for disabling planes,
>>>>> while post_plane_update runs everything needed for enabling planes. So
>>>>> no need to split it up I think, maybe put in some intermediate
>>>>> watermarks in intel_atomic_state, but no need for a full crtc_state.
>>>> Well, the intermediate state stuff was requested by Ville in response to
>>>> my watermark series, so I posted these patches as an RFC to make sure I
>>>> was understanding what he was looking for properly.
>>>>
>>>> Ville, can you comment?
>>> My opinion is that the current "disable is special" way of doing things
>>> is quite horrible. For one it makes it really hard to reason about what
>>> happens to a plane or crtc during the modeset. It's not just off->on,
>>> on->off, or same->same, but can be on->off->on. With the intermediate
>>> state in place, there can only be one transition, so really easy to
>>> think about what's going on.
>> pre_plane_update deals with all stuff related to disabling planes, while post_plane_update deals with changes after enabling.
>>
>> If the crtc goes from on -> off only you could just hammer in the final values after the disable.
>>
>> While for off->on or on->off->on you can put in the final values in .crtc_enable before lighting the pipe. I don't see why wm's would need more transitions.
> One special case after another. Yuck. Not to mention that the plane
> disable isn't even atomic in the current code, which can look ugly.
That's easily fixed by adding a pipe_update_start/end pair.
>>> It'll also mean don't have to sprinkle silly wm update calls all over
>>> the modeset path. They will just get updated in response to the plane
>>> state changes. Same for IPS/FBC etc.
>> IPS and FBC are already calculated correctly in response to modesets.
> Correctly perhaps, but not in an obvious way.
It will become more obvious again when pre_plane_update and post_plane_update are loops
instead of being precalculated from intel_plane_atomic_calc_changes.

But if you can precalculate fb_bits and know of wm changed post commit then post_plane_update
only cares about primary plane state.

~Maarten