Re: [Intel-gfx] [PATCH v4 26/30] drm/i915: Factor out function to clear pipe update flags
On Wed, Nov 01, 2023 at 12:17:30PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 30, 2023 at 05:58:39PM +0200, Imre Deak wrote:
> > Factor out a helper to clear the pipe update flags, used by a follow-up
> > patch to modeset an MST topology.
> >
> > v2:
> > - Move the intel_crtc_needs_modeset() check to the callers. (Ville)
> >
> > Reviewed-by: Stanislav Lisovskiy (v1)
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 52 ++--
> > 1 file changed, 27 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1133c2893bb38..d27afda0cfaa6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5551,6 +5551,14 @@ int intel_modeset_pipes_in_mask_early(struct
> > intel_atomic_state *state,
> > return 0;
> > }
> >
> > +static void
> > +clear_pipe_update_flags_on_modeset_crtc(struct intel_crtc_state
> > *crtc_state)
>
> The 'on_modeset_crtc()' part doesn't make sense anymore.
>
> Hmm. Another idea would be to move the mode_changed=true assignment
> here and call this something like intel_crtc_flag_fmodeset(), which
> might be a bit more descriptive.
Ok makes sense, will do the above.
In intel_crtc_check_fastset() this will set mode_changed=true where it
wasn't before, but that looks ok.
> > +{
> > + crtc_state->update_pipe = false;
> > + crtc_state->update_m_n = false;
> > + crtc_state->update_lrr = false;
> > +}
> > +
> > /**
> > * intel_modeset_all_pipes_late - force a full modeset on all pipes
> > * @state: intel atomic state
> > @@ -5584,9 +5592,8 @@ int intel_modeset_all_pipes_late(struct
> > intel_atomic_state *state,
> > if (ret)
> > return ret;
> >
> > - crtc_state->update_pipe = false;
> > - crtc_state->update_m_n = false;
> > - crtc_state->update_lrr = false;
> > + clear_pipe_update_flags_on_modeset_crtc(crtc_state);
> > +
> > crtc_state->update_planes |= crtc_state->active_planes;
> > crtc_state->async_flip_planes = 0;
> > crtc_state->do_async_flip = false;
> > @@ -5699,13 +5706,14 @@ static void intel_crtc_check_fastset(const struct
> > intel_crtc_state *old_crtc_sta
> > else
> > new_crtc_state->uapi.mode_changed = false;
> >
> > - if (intel_crtc_needs_modeset(new_crtc_state) ||
> > - intel_compare_link_m_n(_crtc_state->dp_m_n,
> > + if (intel_crtc_needs_modeset(new_crtc_state))
> > + clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
> > +
> > + if (intel_compare_link_m_n(_crtc_state->dp_m_n,
> >_crtc_state->dp_m_n))
> > new_crtc_state->update_m_n = false;
> >
> > - if (intel_crtc_needs_modeset(new_crtc_state) ||
> > - (old_crtc_state->hw.adjusted_mode.crtc_vtotal ==
> > new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> > + if ((old_crtc_state->hw.adjusted_mode.crtc_vtotal ==
> > new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> > old_crtc_state->hw.adjusted_mode.crtc_vblank_end ==
> > new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
> > new_crtc_state->update_lrr = false;
> >
> > @@ -6484,12 +6492,9 @@ int intel_atomic_check(struct drm_device *dev,
> > if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
> > enum transcoder master =
> > new_crtc_state->mst_master_transcoder;
> >
> > - if (intel_cpu_transcoders_need_modeset(state,
> > BIT(master))) {
> > - new_crtc_state->uapi.mode_changed = true;
> > - new_crtc_state->update_pipe = false;
> > - new_crtc_state->update_m_n = false;
> > - new_crtc_state->update_lrr = false;
> > - }
> > + if (intel_cpu_transcoders_need_modeset(state,
> > BIT(master)))
> > + intel_modeset_pipes_in_mask_early(state, "MST
> > master transcoder",
> > +
> > BIT(crtc->pipe));
>
> These changes don't really belong in this refactoring patch.
This had the benefit to show the reason for the modeset, but yes,
should've been a separate change. I'll remove it.
> > }
> >
> > if (is_trans_port_sync_mode(new_crtc_state)) {
> > @@ -6498,22 +6503,19 @@ int intel_atomic_check(struct drm_device *dev,
> > if (new_crtc_state->master_transcoder !=
> > INVALID_TRANSCODER)
> > trans |= BIT(new_crtc_state->master_transcoder);
> >
> > - if (intel_cpu_transcoders_need_modeset(state, trans)) {
> > - new_crtc_state->uapi.mode_changed = true;
> > - new_crtc_state->update_pipe = false;
> > -
Re: [Intel-gfx] [PATCH v4 26/30] drm/i915: Factor out function to clear pipe update flags
On Mon, Oct 30, 2023 at 05:58:39PM +0200, Imre Deak wrote:
> Factor out a helper to clear the pipe update flags, used by a follow-up
> patch to modeset an MST topology.
>
> v2:
> - Move the intel_crtc_needs_modeset() check to the callers. (Ville)
>
> Reviewed-by: Stanislav Lisovskiy (v1)
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 52 ++--
> 1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 1133c2893bb38..d27afda0cfaa6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5551,6 +5551,14 @@ int intel_modeset_pipes_in_mask_early(struct
> intel_atomic_state *state,
> return 0;
> }
>
> +static void
> +clear_pipe_update_flags_on_modeset_crtc(struct intel_crtc_state *crtc_state)
The 'on_modeset_crtc()' part doesn't make sense anymore.
Hmm. Another idea would be to move the mode_changed=true assignment
here and call this something like intel_crtc_flag_fmodeset(), which
might be a bit more descriptive.
> +{
> + crtc_state->update_pipe = false;
> + crtc_state->update_m_n = false;
> + crtc_state->update_lrr = false;
> +}
> +
> /**
> * intel_modeset_all_pipes_late - force a full modeset on all pipes
> * @state: intel atomic state
> @@ -5584,9 +5592,8 @@ int intel_modeset_all_pipes_late(struct
> intel_atomic_state *state,
> if (ret)
> return ret;
>
> - crtc_state->update_pipe = false;
> - crtc_state->update_m_n = false;
> - crtc_state->update_lrr = false;
> + clear_pipe_update_flags_on_modeset_crtc(crtc_state);
> +
> crtc_state->update_planes |= crtc_state->active_planes;
> crtc_state->async_flip_planes = 0;
> crtc_state->do_async_flip = false;
> @@ -5699,13 +5706,14 @@ static void intel_crtc_check_fastset(const struct
> intel_crtc_state *old_crtc_sta
> else
> new_crtc_state->uapi.mode_changed = false;
>
> - if (intel_crtc_needs_modeset(new_crtc_state) ||
> - intel_compare_link_m_n(_crtc_state->dp_m_n,
> + if (intel_crtc_needs_modeset(new_crtc_state))
> + clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
> +
> + if (intel_compare_link_m_n(_crtc_state->dp_m_n,
> _crtc_state->dp_m_n))
> new_crtc_state->update_m_n = false;
>
> - if (intel_crtc_needs_modeset(new_crtc_state) ||
> - (old_crtc_state->hw.adjusted_mode.crtc_vtotal ==
> new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> + if ((old_crtc_state->hw.adjusted_mode.crtc_vtotal ==
> new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
>old_crtc_state->hw.adjusted_mode.crtc_vblank_end ==
> new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
> new_crtc_state->update_lrr = false;
>
> @@ -6484,12 +6492,9 @@ int intel_atomic_check(struct drm_device *dev,
> if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
> enum transcoder master =
> new_crtc_state->mst_master_transcoder;
>
> - if (intel_cpu_transcoders_need_modeset(state,
> BIT(master))) {
> - new_crtc_state->uapi.mode_changed = true;
> - new_crtc_state->update_pipe = false;
> - new_crtc_state->update_m_n = false;
> - new_crtc_state->update_lrr = false;
> - }
> + if (intel_cpu_transcoders_need_modeset(state,
> BIT(master)))
> + intel_modeset_pipes_in_mask_early(state, "MST
> master transcoder",
> +
> BIT(crtc->pipe));
These changes don't really belong in this refactoring patch.
> }
>
> if (is_trans_port_sync_mode(new_crtc_state)) {
> @@ -6498,22 +6503,19 @@ int intel_atomic_check(struct drm_device *dev,
> if (new_crtc_state->master_transcoder !=
> INVALID_TRANSCODER)
> trans |= BIT(new_crtc_state->master_transcoder);
>
> - if (intel_cpu_transcoders_need_modeset(state, trans)) {
> - new_crtc_state->uapi.mode_changed = true;
> - new_crtc_state->update_pipe = false;
> - new_crtc_state->update_m_n = false;
> - new_crtc_state->update_lrr = false;
> - }
> + if (intel_cpu_transcoders_need_modeset(state, trans))
> + intel_modeset_pipes_in_mask_early(state, "port
> sync",
> +
> BIT(crtc->pipe));
> }
>
> if
