| Message ID | 1440806249-8345-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. After a modeset disable you should be able to put in any wm value in .crtc_enable because no plane will be active.
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@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? Matt > > After a modeset disable you should be able to put in any wm value in > .crtc_enable because no plane will be active.
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@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. 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. > > > Matt > > > > > After a modeset disable you should be able to put in any wm value in > > .crtc_enable because no plane will be active. > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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. > 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. ~Maarten
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@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. > > 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.
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@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
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@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. It'll never be obvious as long as the on->off->on case exists. > > 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
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@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. > 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?
On Wed, Sep 02, 2015 at 04:22:56PM +0200, Maarten Lankhorst wrote: > 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@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. > > 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? I don't understand what you're saying. If we had the intermediate atomic state, plane code wouldn't need to know at all what's happening to the pipe. And for pipes there can only be an on->off or off->on transition.
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 9336e80..3ffc385 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -101,6 +101,20 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) crtc_state->base.crtc = crtc; + crtc_state->wait_for_flips = false; + crtc_state->disable_fbc = false; + crtc_state->disable_ips = false; + crtc_state->disable_cxsr = false; + crtc_state->pre_disable_primary = false; + crtc_state->update_wm_pre = false; + crtc_state->update_wm_post = false; + crtc_state->disabled_planes = 0; + crtc_state->fb_bits = 0; + crtc_state->wait_vblank = false; + crtc_state->update_fbc = false; + crtc_state->post_enable_primary = false; + crtc_state->update_sprite_watermarks = 0; + return &crtc_state->base; } @@ -212,7 +226,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev, * minimum required validation. */ if (plane->type == DRM_PLANE_TYPE_PRIMARY) - intel_crtc->atomic.wait_for_flips = true; + crtc_state->wait_for_flips = true; crtc_state->base.planes_changed = true; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f604ce1..68b5f2a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4749,44 +4749,42 @@ intel_pre_disable_primary(struct drm_crtc *crtc) static void intel_post_plane_update(struct intel_crtc *crtc) { - struct intel_crtc_atomic_commit *atomic = &crtc->atomic; + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_plane *plane; - if (atomic->wait_vblank) + if (cstate->wait_vblank) intel_wait_for_vblank(dev, crtc->pipe); - intel_frontbuffer_flip(dev, atomic->fb_bits); + intel_frontbuffer_flip(dev, cstate->fb_bits); - if (atomic->disable_cxsr) + if (cstate->disable_cxsr) crtc->wm.cxsr_allowed = true; - if (crtc->atomic.update_wm_post) + if (cstate->update_wm_post) intel_update_watermarks(&crtc->base); - if (atomic->update_fbc) + if (cstate->update_fbc) intel_fbc_update(dev_priv); - if (atomic->post_enable_primary) + if (cstate->post_enable_primary) intel_post_enable_primary(&crtc->base); - drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks) + drm_for_each_plane_mask(plane, dev, cstate->update_sprite_watermarks) intel_update_sprite_watermarks(plane, &crtc->base, 0, 0, 0, false, false); - - memset(atomic, 0, sizeof(*atomic)); } static void intel_pre_plane_update(struct intel_crtc *crtc) { + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc_atomic_commit *atomic = &crtc->atomic; struct drm_plane *p; /* Track fb's for any planes being disabled */ - drm_for_each_plane_mask(p, dev, atomic->disabled_planes) { + drm_for_each_plane_mask(p, dev, cstate->disabled_planes) { struct intel_plane *plane = to_intel_plane(p); mutex_lock(&dev->struct_mutex); @@ -4795,19 +4793,19 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) mutex_unlock(&dev->struct_mutex); } - if (atomic->wait_for_flips) + if (cstate->wait_for_flips) intel_crtc_wait_for_pending_flips(&crtc->base); - if (atomic->disable_fbc) + if (cstate->disable_fbc) intel_fbc_disable_crtc(crtc); - if (crtc->atomic.disable_ips) + if (cstate->disable_ips) hsw_disable_ips(crtc); - if (atomic->pre_disable_primary) + if (cstate->pre_disable_primary) intel_pre_disable_primary(&crtc->base); - if (atomic->disable_cxsr) { + if (cstate->disable_cxsr) { crtc->wm.cxsr_allowed = false; intel_set_memory_cxsr(dev_priv, false); } @@ -11553,6 +11551,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, { struct drm_crtc *crtc = crtc_state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *intel_crtc_state = + to_intel_crtc_state(crtc_state); struct drm_plane *plane = plane_state->plane; struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -11567,10 +11567,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, bool turn_off, turn_on, visible, was_visible; struct drm_framebuffer *fb = plane_state->fb; - if (crtc_state && INTEL_INFO(dev)->gen >= 9 && + if (INTEL_INFO(dev)->gen >= 9 && plane->type != DRM_PLANE_TYPE_CURSOR) { ret = skl_update_scaler_plane( - to_intel_crtc_state(crtc_state), + intel_crtc_state, to_intel_plane_state(plane_state)); if (ret) return ret; @@ -11582,7 +11582,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, * get called by the plane helpers. */ if (old_plane_state->base.fb && !fb) - intel_crtc->atomic.disabled_planes |= 1 << i; + intel_crtc_state->disabled_planes |= 1 << i; was_visible = old_plane_state->visible; visible = to_intel_plane_state(plane_state)->visible; @@ -11607,35 +11607,35 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, turn_off, turn_on, mode_changed); if (turn_on) { - intel_crtc->atomic.update_wm_pre = true; + intel_crtc_state->update_wm_pre = true; /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) { - intel_crtc->atomic.disable_cxsr = true; + intel_crtc_state->disable_cxsr = true; /* to potentially re-enable cxsr */ - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_wm_post = true; + intel_crtc_state->wait_vblank = true; + intel_crtc_state->update_wm_post = true; } } else if (turn_off) { - intel_crtc->atomic.update_wm_post = true; + intel_crtc_state->update_wm_post = true; /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) { if (is_crtc_enabled) - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.disable_cxsr = true; + intel_crtc_state->wait_vblank = true; + intel_crtc_state->disable_cxsr = true; } } else if (intel_wm_need_update(plane, plane_state)) { - intel_crtc->atomic.update_wm_pre = true; + intel_crtc_state->update_wm_pre = true; } if (visible) - intel_crtc->atomic.fb_bits |= + intel_crtc_state->fb_bits |= to_intel_plane(plane)->frontbuffer_bit; switch (plane->type) { case DRM_PLANE_TYPE_PRIMARY: - intel_crtc->atomic.wait_for_flips = true; - intel_crtc->atomic.pre_disable_primary = turn_off; - intel_crtc->atomic.post_enable_primary = turn_on; + intel_crtc_state->wait_for_flips = true; + intel_crtc_state->pre_disable_primary = turn_off; + intel_crtc_state->post_enable_primary = turn_on; if (turn_off) { /* @@ -11646,9 +11646,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, * update_primary_plane function IPS needs to be * disable. */ - intel_crtc->atomic.disable_ips = true; + intel_crtc_state->disable_ips = true; - intel_crtc->atomic.disable_fbc = true; + intel_crtc_state->disable_fbc = true; } /* @@ -11666,7 +11666,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && dev_priv->fbc.crtc == intel_crtc && plane_state->rotation != BIT(DRM_ROTATE_0)) - intel_crtc->atomic.disable_fbc = true; + intel_crtc_state->disable_fbc = true; /* * BDW signals flip done immediately if the plane @@ -11674,16 +11674,16 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, * armed to occur at the next vblank :( */ if (turn_on && IS_BROADWELL(dev)) - intel_crtc->atomic.wait_vblank = true; + intel_crtc_state->wait_vblank = true; - intel_crtc->atomic.update_fbc |= visible || mode_changed; + intel_crtc_state->update_fbc |= visible || mode_changed; break; case DRM_PLANE_TYPE_CURSOR: break; case DRM_PLANE_TYPE_OVERLAY: if (turn_off && !mode_changed) { - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_sprite_watermarks |= + intel_crtc_state->wait_vblank = true; + intel_crtc_state->update_sprite_watermarks |= 1 << i; } } @@ -11758,7 +11758,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, } if (mode_changed && !crtc_state->active) - intel_crtc->atomic.update_wm_post = true; + pipe_config->update_wm_post = true; if (mode_changed && crtc_state->enable && dev_priv->display.crtc_compute_clock && @@ -13473,8 +13473,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); - if (intel_crtc->atomic.update_wm_pre) + if (cstate->update_wm_pre) intel_update_watermarks(crtc); /* Perform vblank evasion around commit operation */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 81b7d77..fd09087 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -455,6 +455,22 @@ struct intel_crtc_state { /* w/a for waiting 2 vblanks during crtc enable */ enum pipe hsw_workaround_pipe; + + /* Sleepable operations to perform before commit */ + bool wait_for_flips; + bool disable_fbc; + bool disable_ips; + bool disable_cxsr; + bool pre_disable_primary; + bool update_wm_pre, update_wm_post; + unsigned disabled_planes; + + /* Sleepable operations to perform after commit */ + unsigned fb_bits; + bool wait_vblank; + bool update_fbc; + bool post_enable_primary; + unsigned update_sprite_watermarks; }; struct vlv_wm_state { @@ -488,30 +504,6 @@ struct skl_pipe_wm { uint32_t linetime; }; -/* - * Tracking of operations that need to be performed at the beginning/end of an - * atomic commit, outside the atomic section where interrupts are disabled. - * These are generally operations that grab mutexes or might otherwise sleep - * and thus can't be run with interrupts disabled. - */ -struct intel_crtc_atomic_commit { - /* Sleepable operations to perform before commit */ - bool wait_for_flips; - bool disable_fbc; - bool disable_ips; - bool disable_cxsr; - bool pre_disable_primary; - bool update_wm_pre, update_wm_post; - unsigned disabled_planes; - - /* Sleepable operations to perform after commit */ - unsigned fb_bits; - bool wait_vblank; - bool update_fbc; - bool post_enable_primary; - unsigned update_sprite_watermarks; -}; - struct intel_crtc { struct drm_crtc base; enum pipe pipe; @@ -563,7 +555,6 @@ struct intel_crtc { int scanline_offset; unsigned start_vbl_count; - struct intel_crtc_atomic_commit atomic; /* scalers available on this crtc */ int num_scalers;
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@intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 16 ++++++- drivers/gpu/drm/i915/intel_display.c | 83 ++++++++++++++++++------------------ drivers/gpu/drm/i915/intel_drv.h | 41 +++++++----------- 3 files changed, 73 insertions(+), 67 deletions(-)