| Message ID | 20191207011832.422566-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/tgl: Select master transcoder for MST stream | expand |
On Fri, Dec 06, 2019 at 05:18:30PM -0800, José Roberto de Souza wrote: > Due to DDB overlaps the pipe enabling sequence is not always crescent. > As the previous patch selects the first pipe/transcoder in the MST > stream to the MST master and it needs to be enabled first this > changes were needed to guarantee that. > > So here it will first loop through all the MST masters and other > pipes that do not have pipe dependencies and enabling then, as when > the master is being enabled all the slaves are also going to a full > modeset they will not overlap with each other. > Then on the second loop it will enable all the MST slaves. > > I have tried to put port sync pipes into those two loops but > intel_update_trans_port_sync_crtcs() is doing way more than just > enable pipes, reading spec I guess it could be accomplish but I will > leave it to people working on port sync. > At least now the port sync pipes are enabled by last so the slave DDB > allocation will not overlap with other pipes. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 120 ++++++++++++++++--- > 1 file changed, 105 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index f89494c849ce..2f74c0bfb2a8 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14576,6 +14576,39 @@ static void intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc, > state); > } > > +static void > +skl_commit_modeset_enable_pipe(struct intel_crtc *crtc, > + struct intel_crtc_state *old_crtc_state, > + struct intel_crtc_state *new_crtc_state, > + unsigned int *updated, bool *progress, > + struct skl_ddb_entry *entry) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(old_crtc_state->uapi.state); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + bool vbl_wait = false; > + > + *updated = *updated | BIT(crtc->pipe); > + *progress = true; > + *entry = new_crtc_state->wm.skl.ddb; > + > + /* > + * If this is an already active pipe, it's DDB changed, > + * and this isn't the last pipe that needs updating > + * then we need to wait for a vblank to pass for the > + * new ddb allocation to take effect. > + */ > + if (!needs_modeset(new_crtc_state) && > + state->wm_results.dirty_pipes != *updated && > + !skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb, > + &old_crtc_state->wm.skl.ddb)) > + vbl_wait = true; > + > + intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state); > + > + if (vbl_wait) > + intel_wait_for_vblank(dev_priv, crtc->pipe); > +} > + > static void skl_commit_modeset_enables(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > @@ -14600,18 +14633,84 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state) > /* > * Whenever the number of active pipes changes, we need to make sure we > * update the pipes in the right order so that their ddb allocations > - * never overlap with eachother inbetween CRTC updates. Otherwise we'll > + * never overlap with each other between CRTC updates. Otherwise we'll > * cause pipe underruns and other bad stuff. > + * > + * First enable all the pipes that do not depends on other pipes while > + * respecting the DDB allocation overlaps. > + * > + * TODO: integrate port sync to the loops bellow. > + * Port sync is not respecting the DDB allocation overlaps as it > + * was not checking for the slave port overlaps and there is more than > + * just a pipe enable in intel_update_trans_port_sync_crtcs() > */ > do { > progress = false; > > - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > + if (updated & BIT(crtc->pipe) || > + !new_crtc_state->hw.active) > + continue; > + > + if (intel_dp_mst_is_slave_trans(new_crtc_state) || > + is_trans_port_sync_mode(new_crtc_state)) > + continue; > + > + if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > + entries, > + INTEL_NUM_PIPES(dev_priv), i)) > + continue; > + > + skl_commit_modeset_enable_pipe(crtc, old_crtc_state, > + new_crtc_state, &updated, > + &progress, &entries[i]); > + } > + } while (progress); > + > + /* > + * Now enable all the pipes that depends on other pipe aka MST slaves > + */ > + do { > + progress = false; I think we probably want to split the modeset from the other updates now so that we can avoid repeating all this stuff for modesets. Something like: do { for_each_crtc() { if (needs_modeset() continue; // current thing } } while (progress); for_each_crtc() { if (!needs_modeset()) continue; if (port_sync || mst_slave) continue; WARN_ON(allocaiton_overlaps()); enable_crtc(); } for_each_crtc() { if (!needs_modeset()) continue; if (port_sync) continue; WARN_ON(allocaiton_overlaps()); enable_crtc(); } for_each_crtc() { if (!needs_modeset()) continue; if (!port_sync_master) continue; WARN_ON(allocaiton_overlaps()); enable_port_sync(); } Still repetitive, but at leeast we don't have to repeat all the ddb overlap avoidance stuff. > + > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > + if (updated & BIT(crtc->pipe) || > + !new_crtc_state->hw.active) > + continue; > + > + if (is_trans_port_sync_mode(new_crtc_state)) > + continue; > + > + if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > + entries, > + INTEL_NUM_PIPES(dev_priv), i)) > + continue; > + > + skl_commit_modeset_enable_pipe(crtc, old_crtc_state, > + new_crtc_state, &updated, > + &progress, &entries[i]); > + } > + } while (progress); > + > + /* Port sync loop */ > + do { > + progress = false; > + > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > enum pipe pipe = crtc->pipe; > bool vbl_wait = false; > bool modeset = needs_modeset(new_crtc_state); > > - if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active) > + if (updated & BIT(pipe) || !new_crtc_state->hw.active) > + continue; > + > + if (!is_trans_port_sync_master(new_crtc_state)) > + continue; > + > + if (!modeset) > continue; > > if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > @@ -14634,18 +14733,9 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state) > state->wm_results.dirty_pipes != updated) > vbl_wait = true; > > - if (modeset && is_trans_port_sync_mode(new_crtc_state)) { > - if (is_trans_port_sync_master(new_crtc_state)) > - intel_update_trans_port_sync_crtcs(crtc, > - state, > - old_crtc_state, > - new_crtc_state); > - else > - continue; > - } else { > - intel_update_crtc(crtc, state, old_crtc_state, > - new_crtc_state); > - } > + intel_update_trans_port_sync_crtcs(crtc, state, > + old_crtc_state, > + new_crtc_state); > > if (vbl_wait) > intel_wait_for_vblank(dev_priv, pipe); > -- > 2.24.0
On Mon, 2019-12-09 at 19:19 +0200, Ville Syrjälä wrote: > On Fri, Dec 06, 2019 at 05:18:30PM -0800, José Roberto de Souza > wrote: > > Due to DDB overlaps the pipe enabling sequence is not always > > crescent. > > As the previous patch selects the first pipe/transcoder in the MST > > stream to the MST master and it needs to be enabled first this > > changes were needed to guarantee that. > > > > So here it will first loop through all the MST masters and other > > pipes that do not have pipe dependencies and enabling then, as when > > the master is being enabled all the slaves are also going to a full > > modeset they will not overlap with each other. > > Then on the second loop it will enable all the MST slaves. > > > > I have tried to put port sync pipes into those two loops but > > intel_update_trans_port_sync_crtcs() is doing way more than just > > enable pipes, reading spec I guess it could be accomplish but I > > will > > leave it to people working on port sync. > > At least now the port sync pipes are enabled by last so the slave > > DDB > > allocation will not overlap with other pipes. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 120 > > ++++++++++++++++--- > > 1 file changed, 105 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index f89494c849ce..2f74c0bfb2a8 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -14576,6 +14576,39 @@ static void > > intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc, > > state); > > } > > > > +static void > > +skl_commit_modeset_enable_pipe(struct intel_crtc *crtc, > > + struct intel_crtc_state *old_crtc_state, > > + struct intel_crtc_state *new_crtc_state, > > + unsigned int *updated, bool *progress, > > + struct skl_ddb_entry *entry) > > +{ > > + struct intel_atomic_state *state = > > to_intel_atomic_state(old_crtc_state->uapi.state); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + bool vbl_wait = false; > > + > > + *updated = *updated | BIT(crtc->pipe); > > + *progress = true; > > + *entry = new_crtc_state->wm.skl.ddb; > > + > > + /* > > + * If this is an already active pipe, it's DDB changed, > > + * and this isn't the last pipe that needs updating > > + * then we need to wait for a vblank to pass for the > > + * new ddb allocation to take effect. > > + */ > > + if (!needs_modeset(new_crtc_state) && > > + state->wm_results.dirty_pipes != *updated && > > + !skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb, > > + &old_crtc_state->wm.skl.ddb)) > > + vbl_wait = true; > > + > > + intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state); > > + > > + if (vbl_wait) > > + intel_wait_for_vblank(dev_priv, crtc->pipe); > > +} > > + > > static void skl_commit_modeset_enables(struct intel_atomic_state > > *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > @@ -14600,18 +14633,84 @@ static void > > skl_commit_modeset_enables(struct intel_atomic_state *state) > > /* > > * Whenever the number of active pipes changes, we need to make > > sure we > > * update the pipes in the right order so that their ddb > > allocations > > - * never overlap with eachother inbetween CRTC updates. > > Otherwise we'll > > + * never overlap with each other between CRTC updates. > > Otherwise we'll > > * cause pipe underruns and other bad stuff. > > + * > > + * First enable all the pipes that do not depends on other > > pipes while > > + * respecting the DDB allocation overlaps. > > + * > > + * TODO: integrate port sync to the loops bellow. > > + * Port sync is not respecting the DDB allocation overlaps as > > it > > + * was not checking for the slave port overlaps and there is > > more than > > + * just a pipe enable in intel_update_trans_port_sync_crtcs() > > */ > > do { > > progress = false; > > > > - for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, new_crtc_state, i) { > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > + new_crtc_state, i) > > { > > + if (updated & BIT(crtc->pipe) || > > + !new_crtc_state->hw.active) > > + continue; > > + > > + if (intel_dp_mst_is_slave_trans(new_crtc_state) > > || > > + is_trans_port_sync_mode(new_crtc_state)) > > + continue; > > + > > + if > > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > > + entries, > > + INTEL_NUM_PIPES > > (dev_priv), i)) > > + continue; > > + > > + skl_commit_modeset_enable_pipe(crtc, > > old_crtc_state, > > + new_crtc_state, > > &updated, > > + &progress, > > &entries[i]); > > + } > > + } while (progress); > > + > > + /* > > + * Now enable all the pipes that depends on other pipe aka MST > > slaves > > + */ > > + do { > > + progress = false; > > I think we probably want to split the modeset from the other updates > now so that we can avoid repeating all this stuff for modesets. > > Something like: > > do { > for_each_crtc() { > if (needs_modeset() > continue; > // current thing > } > } while (progress); > > for_each_crtc() { > if (!needs_modeset()) > continue; > > if (port_sync || mst_slave) > continue; > > WARN_ON(allocaiton_overlaps()); > enable_crtc(); > } > > for_each_crtc() { > if (!needs_modeset()) > continue; > > if (port_sync) > continue; > > WARN_ON(allocaiton_overlaps()); > enable_crtc(); > } > > for_each_crtc() { > if (!needs_modeset()) > continue; > > if (!port_sync_master) > continue; > > WARN_ON(allocaiton_overlaps()); > enable_port_sync(); > } > > Still repetitive, but at leeast we don't have to repeat all the ddb > overlap avoidance stuff. Looks good, will update it. > > > + > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > + new_crtc_state, i) > > { > > + if (updated & BIT(crtc->pipe) || > > + !new_crtc_state->hw.active) > > + continue; > > + > > + if (is_trans_port_sync_mode(new_crtc_state)) > > + continue; > > + > > + if > > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > > + entries, > > + INTEL_NUM_PIPES > > (dev_priv), i)) > > + continue; > > + > > + skl_commit_modeset_enable_pipe(crtc, > > old_crtc_state, > > + new_crtc_state, > > &updated, > > + &progress, > > &entries[i]); > > + } > > + } while (progress); > > + > > + /* Port sync loop */ > > + do { > > + progress = false; > > + > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > + new_crtc_state, i) > > { > > enum pipe pipe = crtc->pipe; > > bool vbl_wait = false; > > bool modeset = needs_modeset(new_crtc_state); > > > > - if (updated & BIT(crtc->pipe) || > > !new_crtc_state->hw.active) > > + if (updated & BIT(pipe) || !new_crtc_state- > > >hw.active) > > + continue; > > + > > + if (!is_trans_port_sync_master(new_crtc_state)) > > + continue; > > + > > + if (!modeset) > > continue; > > > > if > > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > > @@ -14634,18 +14733,9 @@ static void > > skl_commit_modeset_enables(struct intel_atomic_state *state) > > state->wm_results.dirty_pipes != updated) > > vbl_wait = true; > > > > - if (modeset && > > is_trans_port_sync_mode(new_crtc_state)) { > > - if > > (is_trans_port_sync_master(new_crtc_state)) > > - intel_update_trans_port_sync_cr > > tcs(crtc, > > - > > state, > > - > > old_crtc_state, > > - > > new_crtc_state); > > - else > > - continue; > > - } else { > > - intel_update_crtc(crtc, state, > > old_crtc_state, > > - new_crtc_state); > > - } > > + intel_update_trans_port_sync_crtcs(crtc, state, > > + old_crtc_sta > > te, > > + new_crtc_sta > > te); > > > > if (vbl_wait) > > intel_wait_for_vblank(dev_priv, pipe); > > -- > > 2.24.0
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index f89494c849ce..2f74c0bfb2a8 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -14576,6 +14576,39 @@ static void intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc, state); } +static void +skl_commit_modeset_enable_pipe(struct intel_crtc *crtc, + struct intel_crtc_state *old_crtc_state, + struct intel_crtc_state *new_crtc_state, + unsigned int *updated, bool *progress, + struct skl_ddb_entry *entry) +{ + struct intel_atomic_state *state = to_intel_atomic_state(old_crtc_state->uapi.state); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + bool vbl_wait = false; + + *updated = *updated | BIT(crtc->pipe); + *progress = true; + *entry = new_crtc_state->wm.skl.ddb; + + /* + * If this is an already active pipe, it's DDB changed, + * and this isn't the last pipe that needs updating + * then we need to wait for a vblank to pass for the + * new ddb allocation to take effect. + */ + if (!needs_modeset(new_crtc_state) && + state->wm_results.dirty_pipes != *updated && + !skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb, + &old_crtc_state->wm.skl.ddb)) + vbl_wait = true; + + intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state); + + if (vbl_wait) + intel_wait_for_vblank(dev_priv, crtc->pipe); +} + static void skl_commit_modeset_enables(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); @@ -14600,18 +14633,84 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state) /* * Whenever the number of active pipes changes, we need to make sure we * update the pipes in the right order so that their ddb allocations - * never overlap with eachother inbetween CRTC updates. Otherwise we'll + * never overlap with each other between CRTC updates. Otherwise we'll * cause pipe underruns and other bad stuff. + * + * First enable all the pipes that do not depends on other pipes while + * respecting the DDB allocation overlaps. + * + * TODO: integrate port sync to the loops bellow. + * Port sync is not respecting the DDB allocation overlaps as it + * was not checking for the slave port overlaps and there is more than + * just a pipe enable in intel_update_trans_port_sync_crtcs() */ do { progress = false; - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { + if (updated & BIT(crtc->pipe) || + !new_crtc_state->hw.active) + continue; + + if (intel_dp_mst_is_slave_trans(new_crtc_state) || + is_trans_port_sync_mode(new_crtc_state)) + continue; + + if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, + entries, + INTEL_NUM_PIPES(dev_priv), i)) + continue; + + skl_commit_modeset_enable_pipe(crtc, old_crtc_state, + new_crtc_state, &updated, + &progress, &entries[i]); + } + } while (progress); + + /* + * Now enable all the pipes that depends on other pipe aka MST slaves + */ + do { + progress = false; + + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { + if (updated & BIT(crtc->pipe) || + !new_crtc_state->hw.active) + continue; + + if (is_trans_port_sync_mode(new_crtc_state)) + continue; + + if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, + entries, + INTEL_NUM_PIPES(dev_priv), i)) + continue; + + skl_commit_modeset_enable_pipe(crtc, old_crtc_state, + new_crtc_state, &updated, + &progress, &entries[i]); + } + } while (progress); + + /* Port sync loop */ + do { + progress = false; + + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { enum pipe pipe = crtc->pipe; bool vbl_wait = false; bool modeset = needs_modeset(new_crtc_state); - if (updated & BIT(crtc->pipe) || !new_crtc_state->hw.active) + if (updated & BIT(pipe) || !new_crtc_state->hw.active) + continue; + + if (!is_trans_port_sync_master(new_crtc_state)) + continue; + + if (!modeset) continue; if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, @@ -14634,18 +14733,9 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state) state->wm_results.dirty_pipes != updated) vbl_wait = true; - if (modeset && is_trans_port_sync_mode(new_crtc_state)) { - if (is_trans_port_sync_master(new_crtc_state)) - intel_update_trans_port_sync_crtcs(crtc, - state, - old_crtc_state, - new_crtc_state); - else - continue; - } else { - intel_update_crtc(crtc, state, old_crtc_state, - new_crtc_state); - } + intel_update_trans_port_sync_crtcs(crtc, state, + old_crtc_state, + new_crtc_state); if (vbl_wait) intel_wait_for_vblank(dev_priv, pipe);
Due to DDB overlaps the pipe enabling sequence is not always crescent. As the previous patch selects the first pipe/transcoder in the MST stream to the MST master and it needs to be enabled first this changes were needed to guarantee that. So here it will first loop through all the MST masters and other pipes that do not have pipe dependencies and enabling then, as when the master is being enabled all the slaves are also going to a full modeset they will not overlap with each other. Then on the second loop it will enable all the MST slaves. I have tried to put port sync pipes into those two loops but intel_update_trans_port_sync_crtcs() is doing way more than just enable pipes, reading spec I guess it could be accomplish but I will leave it to people working on port sync. At least now the port sync pipes are enabled by last so the slave DDB allocation will not overlap with other pipes. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Manasi Navare <manasi.d.navare@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 120 ++++++++++++++++--- 1 file changed, 105 insertions(+), 15 deletions(-)