[Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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
---
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(-)
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->d
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > --- 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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > > --- > 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. -- 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
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > > > --- > > 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 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 --- >>> 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > --- > >>> 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. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 >> --- > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > >> --- > > 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 -- Ville Syrjälä Intel OTC - Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 --- >>> 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
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 > --- > >>> 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. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
