On Wed, Mar 04, 2020 at 11:25:42PM +0000, Souza, Jose wrote:
> On Wed, 2020-03-04 at 13:46 +0200, Ville Syrjälä wrote:
> > On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote:
> > > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > Currently we're comparing the watermarks between the old and new
> > > > states
> > > > before we've fully computed the new watermarks. In particular
> > > > skl_build_pipe_wm() will not account for the amount of ddb space
> > > > we'll
> > > > have. That information is only available during skl_compute_ddb()
> > > > which will proceed to zero out any watermark level exceeding the
> > > > ddb allocation. If we're short on ddb space this will end up
> > > > adding the plane to the state due erronously determining that the
> > > > watermarks have changed. Fix the problem by deferring
> > > > skl_wm_add_affected_planes() until we have the final watermarks
> > > > computed.
> > > >
> > > > Noticed this when trying enable transition watermarks on glk.
> > > > We now computed the trans_wm as 28, but we only had 14 blocks
> > > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor
> > > > trans_wm every time. Thus we ended up adding the cursor to every
> > > > commit that didn't actually affect the cursor at all.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
> > > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 39299811b650..a3d76e69caae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state
> > > > *state)
> > > > ret = skl_build_pipe_wm(new_crtc_state);
> > > > if (ret)
> > > > return ret;
> > > > -
> > > > - ret = skl_wm_add_affected_planes(state, crtc);
> > > > - if (ret)
> > > > - return ret;
> > > > }
> > > >
> > > > ret = skl_compute_ddb(state);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + /*
> > > > + * skl_compute_ddb() will have adjusted the final watermarks
> > > > + * based on how much ddb is available. Now we can actually
> > > > + * check if the final watermarks changed.
> > > > + */
> > > > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > > old_crtc_state,
> > > > + new_crtc_state, i) {
> > > > + ret = skl_wm_add_affected_planes(state, crtc);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > >
> > > skl_compute_ddb() is already calling skl_wm_add_affected_planes()
> > > after
> > > do the ddb allocation for each pipe, so we could remove this chunk,
> >
> > skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a
> > different thing..
>
> Thanks
No, thank you for the review. Series pushed to dinq.
>
> Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
>
> >
> > > with that:
> > >
> > > Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > >
> > > > +
> > > > skl_print_wm_changes(state);
> > > >
> > > > return 0;
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
