| Message ID | 20191211184526.142413-10-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,rebased,01/11] drm: Add __drm_atomic_helper_crtc_state_reset() & co. | expand |
On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza wrote: > Check if fastset is allowed by external dependencies like other pipes > and transcoders. > > Right now it only forces a fullmodeset when the MST master transcoder > did not changed but the pipe of the master transcoder needs a > fullmodeset so all slaves also needs to do a fullmodeset. > But it will probably be need for port sync as well. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 41 ++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 092412b10d7c..0c24d7dfa152 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state) > return 0; > } > > +/** > + * Check if fastset is allowed by external dependencies like other pipes and > + * transcoders. > + * > + * Right now it only forces a fullmodeset when the MST master transcoder did > + * not changed but the pipe of the master transcoder needs a fullmodeset so > + * all slaves also needs to do a fullmodeset. > + */ > +static bool > +intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state, > + struct intel_crtc_state *new_crtc_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state); > + struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev); > + struct intel_crtc_state *new_crtc_state_iter; > + struct intel_crtc *crtc_iter; > + int i; > + > + if (INTEL_GEN(dev_priv) < 12) > + return true; > + > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) || > + intel_dp_mst_is_master_trans(new_crtc_state)) > + return true; > + > + for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) { > + if (new_crtc_state_iter->cpu_transcoder != > + new_crtc_state->mst_master_transcoder) > + continue; > + > + return !needs_modeset(new_crtc_state_iter); > + } > + > + DRM_ERROR("Master MST transcoder of pipe not found\n"); > + return false; > +} > + > static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state) > { > if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true)) > return; > + if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state, > + new_crtc_state)) > + return; I don't think this will work. We've not yet .compute_config()'d everything so the master assignments are still up in the air. I think we need the logic higher up in intel_atomic_check(): for_each_crtc() compute_config() if (can_fastset()) needs_modeset=false update_pipe=true; } } for_each_crtc() if (mst_slave && master.needs_modeset() { needs_modeset=true; update_pipe=false; } for_each_crtc() if (update_pipe) copy_over_old_state(); + we should probably rename/split update_pipes or add some lkind of needs_fastset() wrapper to make this less confusing. > + > > new_crtc_state->uapi.mode_changed = false; > new_crtc_state->update_pipe = true; > -- > 2.24.1
On Thu, Dec 12, 2019 at 11:28:30PM +0200, Ville Syrjälä wrote: > On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza wrote: > > Check if fastset is allowed by external dependencies like other pipes > > and transcoders. > > > > Right now it only forces a fullmodeset when the MST master transcoder > > did not changed but the pipe of the master transcoder needs a > > fullmodeset so all slaves also needs to do a fullmodeset. > > But it will probably be need for port sync as well. But in case of port sync, now I have this function in intel_atomic_check() that will force a full modeset on all the connectors with has_tile if one of the tiled connectors needs a full modeset. So we wouldnt need to worry about fastset happening for one of the tiles in port sync. This patch: https://patchwork.freedesktop.org/patch/344766/?series=70788&rev=1 Manasi > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 41 ++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 092412b10d7c..0c24d7dfa152 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state) > > return 0; > > } > > > > +/** > > + * Check if fastset is allowed by external dependencies like other pipes and > > + * transcoders. > > + * > > + * Right now it only forces a fullmodeset when the MST master transcoder did > > + * not changed but the pipe of the master transcoder needs a fullmodeset so > > + * all slaves also needs to do a fullmodeset. > > + */ > > +static bool > > +intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state, > > + struct intel_crtc_state *new_crtc_state) > > +{ > > + struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state); > > + struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev); > > + struct intel_crtc_state *new_crtc_state_iter; > > + struct intel_crtc *crtc_iter; > > + int i; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return true; > > + > > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) || > > + intel_dp_mst_is_master_trans(new_crtc_state)) > > + return true; > > + > > + for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) { > > + if (new_crtc_state_iter->cpu_transcoder != > > + new_crtc_state->mst_master_transcoder) > > + continue; > > + > > + return !needs_modeset(new_crtc_state_iter); > > + } > > + > > + DRM_ERROR("Master MST transcoder of pipe not found\n"); > > + return false; > > +} > > + > > static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state, > > struct intel_crtc_state *new_crtc_state) > > { > > if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true)) > > return; > > + if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state, > > + new_crtc_state)) > > + return; > > I don't think this will work. We've not yet .compute_config()'d > everything so the master assignments are still up in the air. > > I think we need the logic higher up in intel_atomic_check(): > > for_each_crtc() > compute_config() > if (can_fastset()) > needs_modeset=false > update_pipe=true; > } > } > > for_each_crtc() > if (mst_slave && master.needs_modeset() { > needs_modeset=true; > update_pipe=false; > } > > for_each_crtc() > if (update_pipe) > copy_over_old_state(); > > > + we should probably rename/split update_pipes or add some > lkind of needs_fastset() wrapper to make this less confusing. > > > > + > > > > new_crtc_state->uapi.mode_changed = false; > > new_crtc_state->update_pipe = true; > > -- > > 2.24.1 > > -- > Ville Syrjälä > Intel
On Thu, 2019-12-12 at 23:28 +0200, Ville Syrjälä wrote: > On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza > wrote: > > Check if fastset is allowed by external dependencies like other > > pipes > > and transcoders. > > > > Right now it only forces a fullmodeset when the MST master > > transcoder > > did not changed but the pipe of the master transcoder needs a > > fullmodeset so all slaves also needs to do a fullmodeset. > > But it will probably be need for port sync as well. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 41 > > ++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 092412b10d7c..0c24d7dfa152 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct > > intel_atomic_state *state) > > return 0; > > } > > > > +/** > > + * Check if fastset is allowed by external dependencies like other > > pipes and > > + * transcoders. > > + * > > + * Right now it only forces a fullmodeset when the MST master > > transcoder did > > + * not changed but the pipe of the master transcoder needs a > > fullmodeset so > > + * all slaves also needs to do a fullmodeset. > > + */ > > +static bool > > +intel_crtc_check_external_dependencies_fastset(const struct > > intel_crtc_state *old_crtc_state, > > + struct intel_crtc_state > > *new_crtc_state) > > +{ > > + struct intel_atomic_state *state = > > to_intel_atomic_state(new_crtc_state->uapi.state); > > + struct drm_i915_private *dev_priv = to_i915(new_crtc_state- > > >uapi.crtc->dev); > > + struct intel_crtc_state *new_crtc_state_iter; > > + struct intel_crtc *crtc_iter; > > + int i; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return true; > > + > > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) > > || > > + intel_dp_mst_is_master_trans(new_crtc_state)) > > + return true; > > + > > + for_each_new_intel_crtc_in_state(state, crtc_iter, > > new_crtc_state_iter, i) { > > + if (new_crtc_state_iter->cpu_transcoder != > > + new_crtc_state->mst_master_transcoder) > > + continue; > > + > > + return !needs_modeset(new_crtc_state_iter); > > + } > > + > > + DRM_ERROR("Master MST transcoder of pipe not found\n"); > > + return false; > > +} > > + > > static void intel_crtc_check_fastset(const struct intel_crtc_state > > *old_crtc_state, > > struct intel_crtc_state > > *new_crtc_state) > > { > > if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, > > true)) > > return; > > + if > > (!intel_crtc_check_external_dependencies_fastset(old_crtc_state, > > + new_crtc_st > > ate)) > > + return; > > I don't think this will work. We've not yet .compute_config()'d > everything so the master assignments are still up in the air. As the MST master is the smallest pipe/transcoder id it is computed first, so it works for MST but that is not the port sync case. Will do a suggested bellow to already support port sync. > > I think we need the logic higher up in intel_atomic_check(): > > for_each_crtc() > compute_config() > if (can_fastset()) > needs_modeset=false > update_pipe=true; > } > } > > for_each_crtc() > if (mst_slave && master.needs_modeset() { > needs_modeset=true; > update_pipe=false; > } > > for_each_crtc() > if (update_pipe) > copy_over_old_state(); > > > + we should probably rename/split update_pipes or add some > lkind of needs_fastset() wrapper to make this less confusing. > > > > + > > > > new_crtc_state->uapi.mode_changed = false; > > new_crtc_state->update_pipe = true; > > -- > > 2.24.1
On Thu, 2019-12-12 at 13:41 -0800, Manasi Navare wrote: > On Thu, Dec 12, 2019 at 11:28:30PM +0200, Ville Syrjälä wrote: > > On Wed, Dec 11, 2019 at 10:45:25AM -0800, José Roberto de Souza > > wrote: > > > Check if fastset is allowed by external dependencies like other > > > pipes > > > and transcoders. > > > > > > Right now it only forces a fullmodeset when the MST master > > > transcoder > > > did not changed but the pipe of the master transcoder needs a > > > fullmodeset so all slaves also needs to do a fullmodeset. > > > But it will probably be need for port sync as well. > > But in case of port sync, now I have this function in > intel_atomic_check() > that will force a full modeset on all the connectors with has_tile if > one of > the tiled connectors needs a full modeset. > So we wouldnt need to worry about fastset happening for one of the > tiles in port sync. > > This patch: > https://patchwork.freedesktop.org/patch/344766/?series=70788&rev=1 This is not enough, test it or read the context of intel_crtc_check_fastset() > > Manasi > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 41 > > > ++++++++++++++++++++ > > > 1 file changed, 41 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 092412b10d7c..0c24d7dfa152 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct > > > intel_atomic_state *state) > > > return 0; > > > } > > > > > > +/** > > > + * Check if fastset is allowed by external dependencies like > > > other pipes and > > > + * transcoders. > > > + * > > > + * Right now it only forces a fullmodeset when the MST master > > > transcoder did > > > + * not changed but the pipe of the master transcoder needs a > > > fullmodeset so > > > + * all slaves also needs to do a fullmodeset. > > > + */ > > > +static bool > > > +intel_crtc_check_external_dependencies_fastset(const struct > > > intel_crtc_state *old_crtc_state, > > > + struct intel_crtc_state > > > *new_crtc_state) > > > +{ > > > + struct intel_atomic_state *state = > > > to_intel_atomic_state(new_crtc_state->uapi.state); > > > + struct drm_i915_private *dev_priv = to_i915(new_crtc_state- > > > >uapi.crtc->dev); > > > + struct intel_crtc_state *new_crtc_state_iter; > > > + struct intel_crtc *crtc_iter; > > > + int i; > > > + > > > + if (INTEL_GEN(dev_priv) < 12) > > > + return true; > > > + > > > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) > > > || > > > + intel_dp_mst_is_master_trans(new_crtc_state)) > > > + return true; > > > + > > > + for_each_new_intel_crtc_in_state(state, crtc_iter, > > > new_crtc_state_iter, i) { > > > + if (new_crtc_state_iter->cpu_transcoder != > > > + new_crtc_state->mst_master_transcoder) > > > + continue; > > > + > > > + return !needs_modeset(new_crtc_state_iter); > > > + } > > > + > > > + DRM_ERROR("Master MST transcoder of pipe not found\n"); > > > + return false; > > > +} > > > + > > > static void intel_crtc_check_fastset(const struct > > > intel_crtc_state *old_crtc_state, > > > struct intel_crtc_state > > > *new_crtc_state) > > > { > > > if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, > > > true)) > > > return; > > > + if > > > (!intel_crtc_check_external_dependencies_fastset(old_crtc_state, > > > + new_crtc_st > > > ate)) > > > + return; > > > > I don't think this will work. We've not yet .compute_config()'d > > everything so the master assignments are still up in the air. > > > > I think we need the logic higher up in intel_atomic_check(): > > > > for_each_crtc() > > compute_config() > > if (can_fastset()) > > needs_modeset=false > > update_pipe=true; > > } > > } > > > > for_each_crtc() > > if (mst_slave && master.needs_modeset() { > > needs_modeset=true; > > update_pipe=false; > > } > > > > for_each_crtc() > > if (update_pipe) > > copy_over_old_state(); > > > > > > + we should probably rename/split update_pipes or add some > > lkind of needs_fastset() wrapper to make this less confusing. > > > > > > > + > > > > > > new_crtc_state->uapi.mode_changed = false; > > > new_crtc_state->update_pipe = true; > > > -- > > > 2.24.1 > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 092412b10d7c..0c24d7dfa152 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -13930,11 +13930,52 @@ static int calc_watermark_data(struct intel_atomic_state *state) return 0; } +/** + * Check if fastset is allowed by external dependencies like other pipes and + * transcoders. + * + * Right now it only forces a fullmodeset when the MST master transcoder did + * not changed but the pipe of the master transcoder needs a fullmodeset so + * all slaves also needs to do a fullmodeset. + */ +static bool +intel_crtc_check_external_dependencies_fastset(const struct intel_crtc_state *old_crtc_state, + struct intel_crtc_state *new_crtc_state) +{ + struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->uapi.state); + struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev); + struct intel_crtc_state *new_crtc_state_iter; + struct intel_crtc *crtc_iter; + int i; + + if (INTEL_GEN(dev_priv) < 12) + return true; + + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST) || + intel_dp_mst_is_master_trans(new_crtc_state)) + return true; + + for_each_new_intel_crtc_in_state(state, crtc_iter, new_crtc_state_iter, i) { + if (new_crtc_state_iter->cpu_transcoder != + new_crtc_state->mst_master_transcoder) + continue; + + return !needs_modeset(new_crtc_state_iter); + } + + DRM_ERROR("Master MST transcoder of pipe not found\n"); + return false; +} + static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state, struct intel_crtc_state *new_crtc_state) { if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true)) return; + if (!intel_crtc_check_external_dependencies_fastset(old_crtc_state, + new_crtc_state)) + return; + new_crtc_state->uapi.mode_changed = false; new_crtc_state->update_pipe = true;
Check if fastset is allowed by external dependencies like other pipes and transcoders. Right now it only forces a fullmodeset when the MST master transcoder did not changed but the pipe of the master transcoder needs a fullmodeset so all slaves also needs to do a fullmodeset. But it will probably be need for port sync as well. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+)