In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 6 ++++-- drivers/gpu/drm/drm_atomic_helper.c | 8 ++++---- drivers/gpu/drm/drm_plane_helper.c | 4 ++-- drivers/gpu/drm/i915/intel_display.c | 10 ++++++---- drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 6 ++++-- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 6 ++++-- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 ++++-- drivers/gpu/drm/sti/sti_drm_crtc.c | 6 ++++-- drivers/gpu/drm/tegra/dc.c | 6 ++++-- include/drm/drm_crtc_helper.h | 6 ++++-- 10 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index f69b92535505..8b8fe3762ca9 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -239,7 +239,8 @@ static int atmel_hlcdc_crtc_atomic_check(struct drm_crtc *c, return atmel_hlcdc_plane_prepare_disc_area(s); }
-static void atmel_hlcdc_crtc_atomic_begin(struct drm_crtc *c) +static void atmel_hlcdc_crtc_atomic_begin(struct drm_crtc *c, + struct drm_crtc_state *old_s) { struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
@@ -253,7 +254,8 @@ static void atmel_hlcdc_crtc_atomic_begin(struct drm_crtc *c) } }
-static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc) +static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_s) { /* TODO: write common plane control register if available */ } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 536ae4da4665..50aef1d937e5 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1144,7 +1144,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_begin) continue;
- funcs->atomic_begin(crtc); + funcs->atomic_begin(crtc, old_crtc_state); }
for_each_plane_in_state(old_state, plane, old_plane_state, i) { @@ -1174,7 +1174,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_flush) continue;
- funcs->atomic_flush(crtc); + funcs->atomic_flush(crtc, old_crtc_state); } } EXPORT_SYMBOL(drm_atomic_helper_commit_planes); @@ -1210,7 +1210,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
crtc_funcs = crtc->helper_private; if (crtc_funcs && crtc_funcs->atomic_begin) - crtc_funcs->atomic_begin(crtc); + crtc_funcs->atomic_begin(crtc, old_crtc_state);
drm_for_each_plane_mask(plane, crtc->dev, plane_mask) { struct drm_plane_state *old_plane_state = @@ -1233,7 +1233,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state) }
if (crtc_funcs && crtc_funcs->atomic_flush) - crtc_funcs->atomic_flush(crtc); + crtc_funcs->atomic_flush(crtc, old_crtc_state); } EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc);
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 2f0ed11024eb..da27fc6f8e4c 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -436,7 +436,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
for (i = 0; i < 2; i++) { if (crtc_funcs[i] && crtc_funcs[i]->atomic_begin) - crtc_funcs[i]->atomic_begin(crtc[i]); + crtc_funcs[i]->atomic_begin(crtc[i], crtc[i]->state); }
/* @@ -451,7 +451,7 @@ int drm_plane_helper_commit(struct drm_plane *plane,
for (i = 0; i < 2; i++) { if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush) - crtc_funcs[i]->atomic_flush(crtc[i]); + crtc_funcs[i]->atomic_flush(crtc[i], crtc[i]->state); }
/* diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index afe91a8f7e36..599c3d078faa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -103,8 +103,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); static void chv_prepare_pll(struct intel_crtc *crtc, const struct intel_crtc_state *pipe_config); -static void intel_begin_crtc_commit(struct drm_crtc *crtc); -static void intel_finish_crtc_commit(struct drm_crtc *crtc); +static void intel_begin_crtc_commit(struct drm_crtc *, struct drm_crtc_state *); +static void intel_finish_crtc_commit(struct drm_crtc *, struct drm_crtc_state *); static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state, @@ -13697,7 +13697,8 @@ intel_disable_primary_plane(struct drm_plane *plane, dev_priv->display.update_primary_plane(crtc, NULL, 0, 0); }
-static void intel_begin_crtc_commit(struct drm_crtc *crtc) +static void intel_begin_crtc_commit(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -13752,7 +13753,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) &intel_crtc->atomic.start_vbl_count); }
-static void intel_finish_crtc_commit(struct drm_crtc *crtc) +static void intel_finish_crtc_commit(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c index 73afa21822b4..5ae578d569e2 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c @@ -327,13 +327,15 @@ static int mdp4_crtc_atomic_check(struct drm_crtc *crtc, return 0; }
-static void mdp4_crtc_atomic_begin(struct drm_crtc *crtc) +static void mdp4_crtc_atomic_begin(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct mdp4_crtc *mdp4_crtc = to_mdp4_crtc(crtc); DBG("%s: begin", mdp4_crtc->name); }
-static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc) +static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct mdp4_crtc *mdp4_crtc = to_mdp4_crtc(crtc); struct drm_device *dev = crtc->dev; diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c index c1530772187d..06801c142a07 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c @@ -367,13 +367,15 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc, return 0; }
-static void mdp5_crtc_atomic_begin(struct drm_crtc *crtc) +static void mdp5_crtc_atomic_begin(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc); DBG("%s: begin", mdp5_crtc->name); }
-static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc) +static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc); struct drm_device *dev = crtc->dev; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index e6a32c4e4040..4d44c69c2f35 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -483,7 +483,8 @@ static bool rcar_du_crtc_mode_fixup(struct drm_crtc *crtc, return true; }
-static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc) +static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct drm_pending_vblank_event *event = crtc->state->event; struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); @@ -499,7 +500,8 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc) } }
-static void rcar_du_crtc_atomic_flush(struct drm_crtc *crtc) +static void rcar_du_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
diff --git a/drivers/gpu/drm/sti/sti_drm_crtc.c b/drivers/gpu/drm/sti/sti_drm_crtc.c index 6b641c5a2ec7..26e63bf14efe 100644 --- a/drivers/gpu/drm/sti/sti_drm_crtc.c +++ b/drivers/gpu/drm/sti/sti_drm_crtc.c @@ -164,7 +164,8 @@ sti_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) sti_drm_crtc_mode_set(crtc, &crtc->state->adjusted_mode); }
-static void sti_drm_atomic_begin(struct drm_crtc *crtc) +static void sti_drm_atomic_begin(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct sti_mixer *mixer = to_sti_mixer(crtc);
@@ -178,7 +179,8 @@ static void sti_drm_atomic_begin(struct drm_crtc *crtc) } }
-static void sti_drm_atomic_flush(struct drm_crtc *crtc) +static void sti_drm_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { }
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index a287e4fec865..bf8ef3133e5b 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -1277,7 +1277,8 @@ static int tegra_crtc_atomic_check(struct drm_crtc *crtc, return 0; }
-static void tegra_crtc_atomic_begin(struct drm_crtc *crtc) +static void tegra_crtc_atomic_begin(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -1291,7 +1292,8 @@ static void tegra_crtc_atomic_begin(struct drm_crtc *crtc) } }
-static void tegra_crtc_atomic_flush(struct drm_crtc *crtc) +static void tegra_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct tegra_dc_state *state = to_dc_state(crtc->state); struct tegra_dc *dc = to_tegra_dc(crtc); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index c8fc187061de..01cafcbe7deb 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -108,8 +108,10 @@ struct drm_crtc_helper_funcs { /* atomic helpers */ int (*atomic_check)(struct drm_crtc *crtc, struct drm_crtc_state *state); - void (*atomic_begin)(struct drm_crtc *crtc); - void (*atomic_flush)(struct drm_crtc *crtc); + void (*atomic_begin)(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state); + void (*atomic_flush)(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state); };
/**
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
~Maarten
On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak this out of the state readout code but instead sanitize the plane state to make sense. Roughly this would be: - read out crtc state - try to reconstruct initial fb for primary plane, if this succeeds then fully link up the plane with the crtc in the plane_state. - then walk all planes for the crtc, and if any plane is enabled in the hw state but doesn't have fb/crtc set in the plane_state force-disable it.
We already do something similar with the vga plane, which we don't represent at all in kms.
Then none of that initial modeset stuff would ever leak into an atomic modeset since it would be all contained in sanitize_crtc. -Daniel
Op 15-06-15 om 11:13 schreef Daniel Vetter:
On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak this out of the state readout code but instead sanitize the plane state to make sense. Roughly this would be:
- read out crtc state
- try to reconstruct initial fb for primary plane, if this succeeds then fully link up the plane with the crtc in the plane_state.
Agreed. Right now get_initial_plane_config takes an initial_plane_state, could we make this atomic too?
- then walk all planes for the crtc, and if any plane is enabled in the hw state but doesn't have fb/crtc set in the plane_state force-disable it.
Can we disable those planes without penalty? Some of them call watermark update, this is a bug but still..
We already do something similar with the vga plane, which we don't represent at all in kms.
Then none of that initial modeset stuff would ever leak into an atomic modeset since it would be all contained in sanitize_crtc.
Ok.
~Maarten
On Mon, Jun 15, 2015 at 11:18:46AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 11:13 schreef Daniel Vetter:
On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak this out of the state readout code but instead sanitize the plane state to make sense. Roughly this would be:
- read out crtc state
- try to reconstruct initial fb for primary plane, if this succeeds then fully link up the plane with the crtc in the plane_state.
Agreed. Right now get_initial_plane_config takes an initial_plane_state, could we make this atomic too?
The initial fb takeover code is a bit tricky since we need to temporarily store a few things while not everything is set up yet fully. We could try to move that information into the plane state, but it would duplicate existing information stored in state->fb->i915_gem_object. Not sure whether it's worth it to have something fully atomic for plane state readout.
The other option would be to allow enabled planes without a full-blown fb object, but experience says this leads to piles of drama in the watermark code.
- then walk all planes for the crtc, and if any plane is enabled in the hw state but doesn't have fb/crtc set in the plane_state force-disable it.
Can we disable those planes without penalty? Some of them call watermark update, this is a bug but still..
What kind of penalty do you think of? Performance doesn't matter since if we get a bad state and need to frob planes around we'll likely also need a full modeset anyway.
Wrt watermark updates that needs to be avoided ofc, so similar to the "force disable all planes in crtc_disable" problem. We might be able to reuse a bit of code for that. -Daniel
On Mon, Jun 15, 2015 at 03:53:43PM +0200, Daniel Vetter wrote:
On Mon, Jun 15, 2015 at 11:18:46AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 11:13 schreef Daniel Vetter:
On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote:
In intel it's useful to keep track of some state changes with old crtc state vs new state, for example to disable initial planes or when a modeset's prevented during fastboot.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak this out of the state readout code but instead sanitize the plane state to make sense. Roughly this would be:
- read out crtc state
- try to reconstruct initial fb for primary plane, if this succeeds then fully link up the plane with the crtc in the plane_state.
Agreed. Right now get_initial_plane_config takes an initial_plane_state, could we make this atomic too?
The initial fb takeover code is a bit tricky since we need to temporarily store a few things while not everything is set up yet fully. We could try to move that information into the plane state, but it would duplicate existing information stored in state->fb->i915_gem_object. Not sure whether it's worth it to have something fully atomic for plane state readout.
The other option would be to allow enabled planes without a full-blown fb object, but experience says this leads to piles of drama in the watermark code.
We could create some kind of fake fb without an actual gem object. The wm code just needs the metadata.
On Tue, Jun 16, 2015 at 11:27:55AM +0300, Ville Syrjälä wrote:
On Mon, Jun 15, 2015 at 03:53:43PM +0200, Daniel Vetter wrote:
On Mon, Jun 15, 2015 at 11:18:46AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 11:13 schreef Daniel Vetter:
On Mon, Jun 15, 2015 at 09:30:19AM +0200, Maarten Lankhorst wrote:
Op 15-06-15 om 09:10 schreef Daniel Vetter:
On Fri, Jun 12, 2015 at 11:18:22AM +0200, Maarten Lankhorst wrote: > In intel it's useful to keep track of some state changes with old > crtc state vs new state, for example to disable initial planes or > when a modeset's prevented during fastboot. > > Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Hm, thus far the approach has been that the various ->check callbacks diff the state and set appropriate stuff like needs_modeset or planes_changed. And with intel_crtc->atomic we've kinda started to build up similar things for i915. What do you plan to use this for? -Daniel
On a modeset I want to disable all old planes by calling plane->disable_plane, which is old_crtc_state->plane_mask. This is for initial hw readout, where a plane might be active without a fb set. I want to run it during vblank evasion if possible, which means in atomic_begin or flush.
commit_plane is not called if the old and new state both have a NULL fb, so the initial plane would stay active in this case.
Hm, so this is for the i915 state readout code. Imo we shouldn't ever leak this out of the state readout code but instead sanitize the plane state to make sense. Roughly this would be:
- read out crtc state
- try to reconstruct initial fb for primary plane, if this succeeds then fully link up the plane with the crtc in the plane_state.
Agreed. Right now get_initial_plane_config takes an initial_plane_state, could we make this atomic too?
The initial fb takeover code is a bit tricky since we need to temporarily store a few things while not everything is set up yet fully. We could try to move that information into the plane state, but it would duplicate existing information stored in state->fb->i915_gem_object. Not sure whether it's worth it to have something fully atomic for plane state readout.
The other option would be to allow enabled planes without a full-blown fb object, but experience says this leads to piles of drama in the watermark code.
We could create some kind of fake fb without an actual gem object. The wm code just needs the metadata.
Yeah I considered that, but we have an awful lot of code that blindly upcasts to the underlying gem bo. Not sure whether that's a lot more solid than just having a NULL fb for code to trip over. But full-blown fb metadata with a NULL obj pointer is indeed an option to keep in mind. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Ville Syrjälä