Op 02-09-15 om 13:15 schreef Ville Syrjälä: > On Wed, Sep 02, 2015 at 01:08:31PM +0200, Maarten Lankhorst wrote: >> 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@xxxxxxxxx> >>>>>>>> --- >>>>>>> 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. > It'll never be obvious as long as the on->off->on case exists. > But On -> off will always be a special case because any enable might depend on the disable, for example taking over the pll or cdclk changes. It can never be the same, so why pretend it is? _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
