Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Fri, Mar 08, 2024 at 05:11:42PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 08, 2024 at 03:43:35PM +0200, Lisovskiy, Stanislav wrote:
> > On Fri, Mar 08, 2024 at 12:07:19PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote:
> > > > Currently we can't change MBUS join status without doing a modeset,
> > > > because we are lacking mechanism to synchronize those with vblank.
> > > > However then this means that we can't do a fastset, if there is a need
> > > > to change MBUS join state. Fix that by implementing such change.
> > > > We already call correspondent check and update at pre_plane dbuf update,
> > > > so the only thing left is to have a non-modeset version of that.
> > > > If active pipes stay the same then fastset is possible and only MBUS
> > > > join state/ddb allocation updates would be committed.
> > > >
> > > > v2: Implement additional changes according to BSpec.
> > > > Vblank wait is needed after MBus/Dbuf programming in case if
> > > > no modeset is done and we are switching from single to multiple
> > > > displays, i.e mbus join state switches from "joined" to
> > > > "non-joined"
> > > > state. Otherwise vblank wait is not needed according to spec.
> > > >
> > > > v3: Split mbus and dbox programming into to pre/post plane update parts,
> > > > how it should be done according to BSpec.
> > > >
> > > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX
> > > > programming
> > > > after DDB reallocation, but before crtc enabling(that is where is
> > > > has
> > > > to be according to spec).
> > > > - Check if crtc is still active, not only the old state.
> > > > - Do a vblank wait if MBUX DBOX register was modified.
> > > > - And of course do vblank wait only if crtc was active.
> > > > - Do vblank wait only if we are not doing a modeset, if we are doing
> > > > something before *commit_modeset_enables, because all crtcs might
> > > > be
> > > > disabled at this moment, so we will get WARN if try waiting for
> > > > vblank
> > > > then.
> > > > - Still getting FIFO underrun so try waiting for vblank in
> > > > pre_plane update
> > > > as well.
> > > > - Write also pipe that we need to sync with to MBUS_CTL register.
> > > >
> > > > v5: - Do vblank wait only for the first pipe, if mbus is joined
> > > > - Check also if new/old_dbuf_state is not NULL, before getting
> > > > single pipe
> > > > and active pipes.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 13 ++-
> > > > drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++
> > > > drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
> > > > 3 files changed, 96 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 00ac65a140298..989818f5d342f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct
> > > > intel_atomic_state *state)
> > > > }
> > > > }
> > > >
> > > > + /*
> > > > +* Some MBUS/DBuf update scenarios(mbus join disable) require
> > > > that
> > > > +* changes are done _after_ DDB reallocation, but _before_ crtc
> > > > enabling.
> > > > +* Typically we are disabling resources in
> > > > post_plane/crtc_enable hooks,
> > > > +* however in that case BSpec explicitly states that this
> > > > should be done
> > > > +* before we enable additional displays.
> > > > +* FIXME: Should we still call this also there(post_plane hook)
> > > > +* for extra-safety? If so, how to make sure, we don't call it
> > > > twice?
> > > > +*/
> > > > + intel_dbuf_mbus_post_ddb_update(state);
> > > > +
> > > > update_pipes = modeset_pipes;
> > > >
> > > > /*
> > > > @@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct
> > > > intel_atomic_state *state)
> > > > }
> > > >
> > > > intel_encoders_update_prepare(state);
> > > > -
> > > > intel_dbuf_pre_plane_update(state);
> > > > - intel_mbus_dbox_update(state);
> > > >
> > > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > > i) {
> > > > if (new_crtc_state->do_async_flip)
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > index 606b7ba9db9ce..f0604ede399f7 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > @@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> > > > if
Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Fri, Mar 08, 2024 at 03:43:35PM +0200, Lisovskiy, Stanislav wrote:
> On Fri, Mar 08, 2024 at 12:07:19PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote:
> > > Currently we can't change MBUS join status without doing a modeset,
> > > because we are lacking mechanism to synchronize those with vblank.
> > > However then this means that we can't do a fastset, if there is a need
> > > to change MBUS join state. Fix that by implementing such change.
> > > We already call correspondent check and update at pre_plane dbuf update,
> > > so the only thing left is to have a non-modeset version of that.
> > > If active pipes stay the same then fastset is possible and only MBUS
> > > join state/ddb allocation updates would be committed.
> > >
> > > v2: Implement additional changes according to BSpec.
> > > Vblank wait is needed after MBus/Dbuf programming in case if
> > > no modeset is done and we are switching from single to multiple
> > > displays, i.e mbus join state switches from "joined" to "non-joined"
> > > state. Otherwise vblank wait is not needed according to spec.
> > >
> > > v3: Split mbus and dbox programming into to pre/post plane update parts,
> > > how it should be done according to BSpec.
> > >
> > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX
> > > programming
> > > after DDB reallocation, but before crtc enabling(that is where is
> > > has
> > > to be according to spec).
> > > - Check if crtc is still active, not only the old state.
> > > - Do a vblank wait if MBUX DBOX register was modified.
> > > - And of course do vblank wait only if crtc was active.
> > > - Do vblank wait only if we are not doing a modeset, if we are doing
> > > something before *commit_modeset_enables, because all crtcs might be
> > > disabled at this moment, so we will get WARN if try waiting for
> > > vblank
> > > then.
> > > - Still getting FIFO underrun so try waiting for vblank in pre_plane
> > > update
> > > as well.
> > > - Write also pipe that we need to sync with to MBUS_CTL register.
> > >
> > > v5: - Do vblank wait only for the first pipe, if mbus is joined
> > > - Check also if new/old_dbuf_state is not NULL, before getting single
> > > pipe
> > > and active pipes.
> > >
> > > Signed-off-by: Stanislav Lisovskiy
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 13 ++-
> > > drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++
> > > drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
> > > 3 files changed, 96 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 00ac65a140298..989818f5d342f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct
> > > intel_atomic_state *state)
> > > }
> > > }
> > >
> > > + /*
> > > + * Some MBUS/DBuf update scenarios(mbus join disable) require that
> > > + * changes are done _after_ DDB reallocation, but _before_ crtc
> > > enabling.
> > > + * Typically we are disabling resources in post_plane/crtc_enable hooks,
> > > + * however in that case BSpec explicitly states that this should be done
> > > + * before we enable additional displays.
> > > + * FIXME: Should we still call this also there(post_plane hook)
> > > + * for extra-safety? If so, how to make sure, we don't call it twice?
> > > + */
> > > + intel_dbuf_mbus_post_ddb_update(state);
> > > +
> > > update_pipes = modeset_pipes;
> > >
> > > /*
> > > @@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct
> > > intel_atomic_state *state)
> > > }
> > >
> > > intel_encoders_update_prepare(state);
> > > -
> > > intel_dbuf_pre_plane_update(state);
> > > - intel_mbus_dbox_update(state);
> > >
> > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > if (new_crtc_state->do_async_flip)
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index 606b7ba9db9ce..f0604ede399f7 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> > > if (ret)
> > > return ret;
> > >
> > > - if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus)
> > > {
> > > - /* TODO: Implement vblank synchronized MBUS joining
> > > changes */
> > > - ret = intel_modeset_all_pipes_late(state, "MBUS joining
> > > change");
> > > - if (ret)
> > > - return ret;
> > > - }
> > > -
> > >
Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Fri, Mar 08, 2024 at 12:07:19PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote:
> > Currently we can't change MBUS join status without doing a modeset,
> > because we are lacking mechanism to synchronize those with vblank.
> > However then this means that we can't do a fastset, if there is a need
> > to change MBUS join state. Fix that by implementing such change.
> > We already call correspondent check and update at pre_plane dbuf update,
> > so the only thing left is to have a non-modeset version of that.
> > If active pipes stay the same then fastset is possible and only MBUS
> > join state/ddb allocation updates would be committed.
> >
> > v2: Implement additional changes according to BSpec.
> > Vblank wait is needed after MBus/Dbuf programming in case if
> > no modeset is done and we are switching from single to multiple
> > displays, i.e mbus join state switches from "joined" to "non-joined"
> > state. Otherwise vblank wait is not needed according to spec.
> >
> > v3: Split mbus and dbox programming into to pre/post plane update parts,
> > how it should be done according to BSpec.
> >
> > v4: - Place "single display to multiple displays scenario" MBUS/DBOX
> > programming
> > after DDB reallocation, but before crtc enabling(that is where is has
> > to be according to spec).
> > - Check if crtc is still active, not only the old state.
> > - Do a vblank wait if MBUX DBOX register was modified.
> > - And of course do vblank wait only if crtc was active.
> > - Do vblank wait only if we are not doing a modeset, if we are doing
> > something before *commit_modeset_enables, because all crtcs might be
> > disabled at this moment, so we will get WARN if try waiting for vblank
> > then.
> > - Still getting FIFO underrun so try waiting for vblank in pre_plane
> > update
> > as well.
> > - Write also pipe that we need to sync with to MBUS_CTL register.
> >
> > v5: - Do vblank wait only for the first pipe, if mbus is joined
> > - Check also if new/old_dbuf_state is not NULL, before getting single
> > pipe
> > and active pipes.
> >
> > Signed-off-by: Stanislav Lisovskiy
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 13 ++-
> > drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++
> > drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
> > 3 files changed, 96 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 00ac65a140298..989818f5d342f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct
> > intel_atomic_state *state)
> > }
> > }
> >
> > + /*
> > +* Some MBUS/DBuf update scenarios(mbus join disable) require that
> > +* changes are done _after_ DDB reallocation, but _before_ crtc
> > enabling.
> > +* Typically we are disabling resources in post_plane/crtc_enable hooks,
> > +* however in that case BSpec explicitly states that this should be done
> > +* before we enable additional displays.
> > +* FIXME: Should we still call this also there(post_plane hook)
> > +* for extra-safety? If so, how to make sure, we don't call it twice?
> > +*/
> > + intel_dbuf_mbus_post_ddb_update(state);
> > +
> > update_pipes = modeset_pipes;
> >
> > /*
> > @@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct
> > intel_atomic_state *state)
> > }
> >
> > intel_encoders_update_prepare(state);
> > -
> > intel_dbuf_pre_plane_update(state);
> > - intel_mbus_dbox_update(state);
> >
> > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > if (new_crtc_state->do_async_flip)
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 606b7ba9db9ce..f0604ede399f7 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> > if (ret)
> > return ret;
> >
> > - if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus)
> > {
> > - /* TODO: Implement vblank synchronized MBUS joining
> > changes */
> > - ret = intel_modeset_all_pipes_late(state, "MBUS joining
> > change");
> > - if (ret)
> > - return ret;
> > - }
> > -
> > drm_dbg_kms(>drm,
> > "Enabled dbuf slices 0x%x -> 0x%x (total dbuf
> > slices 0x%x), mbus joined? %s->%s\n",
> > old_dbuf_state->enabled_slices,
> > @@ -3539,8 +3532,9 @@ static
Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote:
> Currently we can't change MBUS join status without doing a modeset,
> because we are lacking mechanism to synchronize those with vblank.
> However then this means that we can't do a fastset, if there is a need
> to change MBUS join state. Fix that by implementing such change.
> We already call correspondent check and update at pre_plane dbuf update,
> so the only thing left is to have a non-modeset version of that.
> If active pipes stay the same then fastset is possible and only MBUS
> join state/ddb allocation updates would be committed.
>
> v2: Implement additional changes according to BSpec.
> Vblank wait is needed after MBus/Dbuf programming in case if
> no modeset is done and we are switching from single to multiple
> displays, i.e mbus join state switches from "joined" to "non-joined"
> state. Otherwise vblank wait is not needed according to spec.
>
> v3: Split mbus and dbox programming into to pre/post plane update parts,
> how it should be done according to BSpec.
>
> v4: - Place "single display to multiple displays scenario" MBUS/DBOX
> programming
> after DDB reallocation, but before crtc enabling(that is where is has
> to be according to spec).
> - Check if crtc is still active, not only the old state.
> - Do a vblank wait if MBUX DBOX register was modified.
> - And of course do vblank wait only if crtc was active.
> - Do vblank wait only if we are not doing a modeset, if we are doing
> something before *commit_modeset_enables, because all crtcs might be
> disabled at this moment, so we will get WARN if try waiting for vblank
> then.
> - Still getting FIFO underrun so try waiting for vblank in pre_plane
> update
> as well.
> - Write also pipe that we need to sync with to MBUS_CTL register.
>
> v5: - Do vblank wait only for the first pipe, if mbus is joined
> - Check also if new/old_dbuf_state is not NULL, before getting single pipe
> and active pipes.
>
> Signed-off-by: Stanislav Lisovskiy
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 13 ++-
> drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++
> drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
> 3 files changed, 96 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 00ac65a140298..989818f5d342f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct
> intel_atomic_state *state)
> }
> }
>
> + /*
> + * Some MBUS/DBuf update scenarios(mbus join disable) require that
> + * changes are done _after_ DDB reallocation, but _before_ crtc
> enabling.
> + * Typically we are disabling resources in post_plane/crtc_enable hooks,
> + * however in that case BSpec explicitly states that this should be done
> + * before we enable additional displays.
> + * FIXME: Should we still call this also there(post_plane hook)
> + * for extra-safety? If so, how to make sure, we don't call it twice?
> + */
> + intel_dbuf_mbus_post_ddb_update(state);
> +
> update_pipes = modeset_pipes;
>
> /*
> @@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
> }
>
> intel_encoders_update_prepare(state);
> -
> intel_dbuf_pre_plane_update(state);
> - intel_mbus_dbox_update(state);
>
> for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> if (new_crtc_state->do_async_flip)
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 606b7ba9db9ce..f0604ede399f7 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> if (ret)
> return ret;
>
> - if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus)
> {
> - /* TODO: Implement vblank synchronized MBUS joining
> changes */
> - ret = intel_modeset_all_pipes_late(state, "MBUS joining
> change");
> - if (ret)
> - return ret;
> - }
> -
> drm_dbg_kms(>drm,
> "Enabled dbuf slices 0x%x -> 0x%x (total dbuf
> slices 0x%x), mbus joined? %s->%s\n",
> old_dbuf_state->enabled_slices,
> @@ -3539,8 +3532,9 @@ static void intel_dbuf_mbus_update(struct
> intel_atomic_state *state)
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> u32 mbus_ctl, dbuf_min_tracker_val;
> enum dbuf_slice slice;
> -
Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote: > Currently we can't change MBUS join status without doing a modeset, > because we are lacking mechanism to synchronize those with vblank. > However then this means that we can't do a fastset, if there is a need > to change MBUS join state. Fix that by implementing such change. > We already call correspondent check and update at pre_plane dbuf update, > so the only thing left is to have a non-modeset version of that. > If active pipes stay the same then fastset is possible and only MBUS > join state/ddb allocation updates would be committed. > > v2: Implement additional changes according to BSpec. > Vblank wait is needed after MBus/Dbuf programming in case if > no modeset is done and we are switching from single to multiple > displays, i.e mbus join state switches from "joined" to "non-joined" > state. Otherwise vblank wait is not needed according to spec. > > v3: Split mbus and dbox programming into to pre/post plane update parts, > how it should be done according to BSpec. > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX > programming > after DDB reallocation, but before crtc enabling(that is where is has > to be according to spec). > - Check if crtc is still active, not only the old state. > - Do a vblank wait if MBUX DBOX register was modified. > - And of course do vblank wait only if crtc was active. > - Do vblank wait only if we are not doing a modeset, if we are doing > something before *commit_modeset_enables, because all crtcs might be > disabled at this moment, so we will get WARN if try waiting for vblank > then. > - Still getting FIFO underrun so try waiting for vblank in pre_plane > update > as well. > - Write also pipe that we need to sync with to MBUS_CTL register. > > v5: - Do vblank wait only for the first pipe, if mbus is joined > - Check also if new/old_dbuf_state is not NULL, before getting single pipe > and active pipes. > > Signed-off-by: Stanislav Lisovskiy Thank you for this patch, Stanislav! We tested it on a MTL-U based Chromebook (Screebo), using different configurations (eDP, eDP + HDMI, HDMI, etc.), and it worked well -- joined the mbus + no visual issues or i915 errors. Tested-by: Paz Zcharya Just a small note, checkpatch.pl is complaining about a few things. I assume you probably saw it, but flagging just in case.
[PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
Currently we can't change MBUS join status without doing a modeset,
because we are lacking mechanism to synchronize those with vblank.
However then this means that we can't do a fastset, if there is a need
to change MBUS join state. Fix that by implementing such change.
We already call correspondent check and update at pre_plane dbuf update,
so the only thing left is to have a non-modeset version of that.
If active pipes stay the same then fastset is possible and only MBUS
join state/ddb allocation updates would be committed.
v2: Implement additional changes according to BSpec.
Vblank wait is needed after MBus/Dbuf programming in case if
no modeset is done and we are switching from single to multiple
displays, i.e mbus join state switches from "joined" to "non-joined"
state. Otherwise vblank wait is not needed according to spec.
v3: Split mbus and dbox programming into to pre/post plane update parts,
how it should be done according to BSpec.
v4: - Place "single display to multiple displays scenario" MBUS/DBOX programming
after DDB reallocation, but before crtc enabling(that is where is has
to be according to spec).
- Check if crtc is still active, not only the old state.
- Do a vblank wait if MBUX DBOX register was modified.
- And of course do vblank wait only if crtc was active.
- Do vblank wait only if we are not doing a modeset, if we are doing
something before *commit_modeset_enables, because all crtcs might be
disabled at this moment, so we will get WARN if try waiting for vblank
then.
- Still getting FIFO underrun so try waiting for vblank in pre_plane update
as well.
- Write also pipe that we need to sync with to MBUS_CTL register.
v5: - Do vblank wait only for the first pipe, if mbus is joined
- Check also if new/old_dbuf_state is not NULL, before getting single pipe
and active pipes.
Signed-off-by: Stanislav Lisovskiy
---
drivers/gpu/drm/i915/display/intel_display.c | 13 ++-
drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++
drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
3 files changed, 96 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 00ac65a140298..989818f5d342f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct
intel_atomic_state *state)
}
}
+ /*
+* Some MBUS/DBuf update scenarios(mbus join disable) require that
+* changes are done _after_ DDB reallocation, but _before_ crtc
enabling.
+* Typically we are disabling resources in post_plane/crtc_enable hooks,
+* however in that case BSpec explicitly states that this should be done
+* before we enable additional displays.
+* FIXME: Should we still call this also there(post_plane hook)
+* for extra-safety? If so, how to make sure, we don't call it twice?
+*/
+ intel_dbuf_mbus_post_ddb_update(state);
+
update_pipes = modeset_pipes;
/*
@@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct
intel_atomic_state *state)
}
intel_encoders_update_prepare(state);
-
intel_dbuf_pre_plane_update(state);
- intel_mbus_dbox_update(state);
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
if (new_crtc_state->do_async_flip)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index 606b7ba9db9ce..f0604ede399f7 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
if (ret)
return ret;
- if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus)
{
- /* TODO: Implement vblank synchronized MBUS joining
changes */
- ret = intel_modeset_all_pipes_late(state, "MBUS joining
change");
- if (ret)
- return ret;
- }
-
drm_dbg_kms(>drm,
"Enabled dbuf slices 0x%x -> 0x%x (total dbuf
slices 0x%x), mbus joined? %s->%s\n",
old_dbuf_state->enabled_slices,
@@ -3539,8 +3532,9 @@ static void intel_dbuf_mbus_update(struct
intel_atomic_state *state)
struct drm_i915_private *i915 = to_i915(state->base.dev);
u32 mbus_ctl, dbuf_min_tracker_val;
enum dbuf_slice slice;
- const struct intel_dbuf_state *dbuf_state =
+ const struct intel_dbuf_state *new_dbuf_state =
intel_atomic_get_new_dbuf_state(state);
+ enum pipe pipe = ffs(new_dbuf_state->active_pipes) - 1;
[Intel-gfx] [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
Currently we can't change MBUS join status without doing a modeset,
because we are lacking mechanism to synchronize those with vblank.
However then this means that we can't do a fastset, if there is a need
to change MBUS join state. Fix that by implementing such change.
We already call correspondent check and update at pre_plane dbuf update,
so the only thing left is to have a non-modeset version of that.
If active pipes stay the same then fastset is possible and only MBUS
join state/ddb allocation updates would be committed.
v2: Implement additional changes according to BSpec.
Vblank wait is needed after MBus/Dbuf programming in case if
no modeset is done and we are switching from single to multiple
displays, i.e mbus join state switches from "joined" to "non-joined"
state. Otherwise vblank wait is not needed according to spec.
v3: Split mbus and dbox programming into to pre/post plane update parts,
how it should be done according to BSpec.
Signed-off-by: Stanislav Lisovskiy
Tested-by: Khaled Almahallawy
---
drivers/gpu/drm/i915/display/intel_display.c | 2 --
drivers/gpu/drm/i915/display/skl_watermark.c | 36 +++-
2 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 8c81206ce90d7..249ba955cce2a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7041,9 +7041,7 @@ static void intel_atomic_commit_tail(struct
intel_atomic_state *state)
}
intel_encoders_update_prepare(state);
-
intel_dbuf_pre_plane_update(state);
- intel_mbus_dbox_update(state);
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
if (new_crtc_state->do_async_flip)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index af99f2abd8446..b5c5fa9ecf43c 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -2614,13 +2614,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
if (ret)
return ret;
- if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus)
{
- /* TODO: Implement vblank synchronized MBUS joining
changes */
- ret = intel_modeset_all_pipes(state, "MBUS joining
change");
- if (ret)
- return ret;
- }
-
drm_dbg_kms(>drm,
"Enabled dbuf slices 0x%x -> 0x%x (total dbuf
slices 0x%x), mbus joined? %s->%s\n",
old_dbuf_state->enabled_slices,
@@ -3524,8 +3517,15 @@ void intel_dbuf_pre_plane_update(struct
intel_atomic_state *state)
WARN_ON(!new_dbuf_state->base.changed);
- if (hweight8(new_dbuf_state->active_pipes) <=
hweight8(old_dbuf_state->active_pipes))
+ /*
+* Switching from multiple to single display scenario.
+* Also we put here "<=" instead of "<" for suboptimal cases, when
+* we switch from single => single display, enabling mbus join.
+*/
+ if (hweight8(new_dbuf_state->active_pipes) <=
hweight8(old_dbuf_state->active_pipes)) {
intel_dbuf_mbus_update(state);
+ intel_mbus_dbox_update(state);
+ }
gen9_dbuf_slices_update(i915,
old_dbuf_state->enabled_slices |
@@ -3547,8 +3547,26 @@ void intel_dbuf_post_plane_update(struct
intel_atomic_state *state)
WARN_ON(!new_dbuf_state->base.changed);
- if (hweight8(new_dbuf_state->active_pipes) >
hweight8(old_dbuf_state->active_pipes))
+ /*
+* Switching from single to multiple display scenario
+*/
+ if (hweight8(new_dbuf_state->active_pipes) >
hweight8(old_dbuf_state->active_pipes)) {
+ struct intel_crtc *crtc;
+ struct intel_crtc_state *old_crtc_state;
+ int i;
intel_dbuf_mbus_update(state);
+ intel_mbus_dbox_update(state);
+
+ for_each_old_intel_crtc_in_state(state, crtc, old_crtc_state,
i) {
+ /*
+* According to BSpec we should wait vblank on
previously single display
+*/
+ if (!old_crtc_state->hw.active)
+ continue;
+
+ intel_crtc_wait_for_next_vblank(crtc);
+ }
+ }
gen9_dbuf_slices_update(i915,
new_dbuf_state->enabled_slices);
--
2.37.3
