| Message ID | 20240916162413.8555-6-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Some wm/cxsr cleanups | expand |
On Mon, 2024-09-16 at 19:24 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > As with other watermark calculations, the dodgy pre-g4x > update_wm_{pre,post} flag calcultion would like to know Typo: calculation > if a modeset is about to happen or not, and technically > later stages in the atomic_check() may still flag one. > In practice that shouldn't happen as we don't have dynamic > CDCLK implemented for these old platforms. > > Regardless it'll be nice to move this old cruft out from > the supposedly platform agnostic plane code. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/i9xx_wm.c | 74 +++++++++++++++++++ > .../gpu/drm/i915/display/intel_atomic_plane.c | 36 --------- > 2 files changed, 74 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c > index 3151a31a5653..15ed3b810947 100644 > --- a/drivers/gpu/drm/i915/display/i9xx_wm.c > +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c > @@ -705,6 +705,76 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) > } > } > > +static bool i9xx_wm_need_update(const struct intel_plane_state *old_plane_state, > + const struct intel_plane_state *new_plane_state) > +{ > + /* Update watermarks on tiling or size changes. */ > + if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > + return true; > + > + if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > + return false; > + > + if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > + old_plane_state->hw.rotation != new_plane_state->hw.rotation || > + drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > >uapi.src) || > + drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > >uapi.src) || > + drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > >uapi.dst) || > + drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > >uapi.dst)) > + return true; > + > + return false; > +} > + > +static void i9xx_wm_compute(struct intel_crtc_state *new_crtc_state, > + const struct intel_plane_state *old_plane_state, > + const struct intel_plane_state *new_plane_state) > +{ > + bool turn_off, turn_on, visible, was_visible, mode_changed; > + > + mode_changed = intel_crtc_needs_modeset(new_crtc_state); > + was_visible = old_plane_state->uapi.visible; > + visible = new_plane_state->uapi.visible; > + > + if (!was_visible && !visible) > + return; > + > + turn_off = was_visible && (!visible || mode_changed); > + turn_on = visible && (!was_visible || mode_changed); > + > + /* FIXME nuke when all wm code is atomic */ > + if (turn_on) { > + new_crtc_state->update_wm_pre = true; > + } else if (turn_off) { > + new_crtc_state->update_wm_post = true; > + } else if (i9xx_wm_need_update(old_plane_state, new_plane_state)) { > + /* FIXME bollocks */ > + new_crtc_state->update_wm_pre = true; > + new_crtc_state->update_wm_post = true; > + } > +} > + > +static int i9xx_compute_watermarks(struct intel_atomic_state *state, > + struct intel_crtc *crtc) > +{ > + struct intel_crtc_state *new_crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + const struct intel_plane_state *old_plane_state; > + const struct intel_plane_state *new_plane_state; > + struct intel_plane *plane; > + int i; > + > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > + new_plane_state, i) { > + if (plane->pipe != crtc->pipe) > + continue; > + > + i9xx_wm_compute(new_crtc_state, old_plane_state, new_plane_state); > + } > + > + return 0; > +} > + > /* > * Documentation says: > * "If the line size is small, the TLB fetches can get in the way of the > @@ -4056,18 +4126,22 @@ static const struct intel_wm_funcs g4x_wm_funcs = { > }; > > static const struct intel_wm_funcs pnv_wm_funcs = { > + .compute_watermarks = i9xx_compute_watermarks, > .update_wm = pnv_update_wm, > }; > > static const struct intel_wm_funcs i965_wm_funcs = { > + .compute_watermarks = i9xx_compute_watermarks, > .update_wm = i965_update_wm, > }; > > static const struct intel_wm_funcs i9xx_wm_funcs = { > + .compute_watermarks = i9xx_compute_watermarks, > .update_wm = i9xx_update_wm, > }; > > static const struct intel_wm_funcs i845_wm_funcs = { > + .compute_watermarks = i9xx_compute_watermarks, > .update_wm = i845_update_wm, > }; > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 2aeb4cd5b5a1..33fec36ec0bd 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -392,28 +392,6 @@ void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, > plane_state->uapi.visible = false; > } > > -/* FIXME nuke when all wm code is atomic */ > -static bool intel_wm_need_update(const struct intel_plane_state *old_plane_state, > - const struct intel_plane_state *new_plane_state) > -{ > - /* Update watermarks on tiling or size changes. */ > - if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > - return true; > - > - if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > - return false; > - > - if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > - old_plane_state->hw.rotation != new_plane_state->hw.rotation || > - drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > >uapi.src) || > - drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > >uapi.src) || > - drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > >uapi.dst) || > - drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > >uapi.dst)) > - return true; > - > - return false; > -} > - > static bool intel_plane_is_scaled(const struct intel_plane_state *plane_state) > { > int src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > @@ -602,20 +580,6 @@ static int intel_plane_atomic_calc_changes(const struct intel_crtc_state > *old_cr > was_visible, visible, > turn_off, turn_on, mode_changed); > > - if (turn_on) { > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > - new_crtc_state->update_wm_pre = true; > - } else if (turn_off) { > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > - new_crtc_state->update_wm_post = true; > - } else if (intel_wm_need_update(old_plane_state, new_plane_state)) { > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) { > - /* FIXME bollocks */ > - new_crtc_state->update_wm_pre = true; > - new_crtc_state->update_wm_post = true; > - } > - } > - With this change, update_wm_pre/post flag will move from intel_atomic_check_planes() to intel_atomic_check_crtcs() which will call compute_watermarks() and update the flag. Just wanted to clarify if this is expected. BR Vinod > if (visible || was_visible) > new_crtc_state->fb_bits |= plane->frontbuffer_bit; >
On Sun, Sep 22, 2024 at 10:40:32AM +0000, Govindapillai, Vinod wrote: > On Mon, 2024-09-16 at 19:24 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > As with other watermark calculations, the dodgy pre-g4x > > update_wm_{pre,post} flag calcultion would like to know > Typo: calculation > > if a modeset is about to happen or not, and technically > > later stages in the atomic_check() may still flag one. > > In practice that shouldn't happen as we don't have dynamic > > CDCLK implemented for these old platforms. > > > > Regardless it'll be nice to move this old cruft out from > > the supposedly platform agnostic plane code. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > drivers/gpu/drm/i915/display/i9xx_wm.c | 74 +++++++++++++++++++ > > .../gpu/drm/i915/display/intel_atomic_plane.c | 36 --------- > > 2 files changed, 74 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c > > index 3151a31a5653..15ed3b810947 100644 > > --- a/drivers/gpu/drm/i915/display/i9xx_wm.c > > +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c > > @@ -705,6 +705,76 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) > > } > > } > > > > +static bool i9xx_wm_need_update(const struct intel_plane_state *old_plane_state, > > + const struct intel_plane_state *new_plane_state) > > +{ > > + /* Update watermarks on tiling or size changes. */ > > + if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > + return true; > > + > > + if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > + return false; > > + > > + if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > + old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > + drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > >uapi.src) || > > + drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > >uapi.src) || > > + drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > >uapi.dst) || > > + drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > >uapi.dst)) > > + return true; > > + > > + return false; > > +} > > + > > +static void i9xx_wm_compute(struct intel_crtc_state *new_crtc_state, > > + const struct intel_plane_state *old_plane_state, > > + const struct intel_plane_state *new_plane_state) > > +{ > > + bool turn_off, turn_on, visible, was_visible, mode_changed; > > + > > + mode_changed = intel_crtc_needs_modeset(new_crtc_state); > > + was_visible = old_plane_state->uapi.visible; > > + visible = new_plane_state->uapi.visible; > > + > > + if (!was_visible && !visible) > > + return; > > + > > + turn_off = was_visible && (!visible || mode_changed); > > + turn_on = visible && (!was_visible || mode_changed); > > + > > + /* FIXME nuke when all wm code is atomic */ > > + if (turn_on) { > > + new_crtc_state->update_wm_pre = true; > > + } else if (turn_off) { > > + new_crtc_state->update_wm_post = true; > > + } else if (i9xx_wm_need_update(old_plane_state, new_plane_state)) { > > + /* FIXME bollocks */ > > + new_crtc_state->update_wm_pre = true; > > + new_crtc_state->update_wm_post = true; > > + } > > +} > > + > > +static int i9xx_compute_watermarks(struct intel_atomic_state *state, > > + struct intel_crtc *crtc) > > +{ > > + struct intel_crtc_state *new_crtc_state = > > + intel_atomic_get_new_crtc_state(state, crtc); > > + const struct intel_plane_state *old_plane_state; > > + const struct intel_plane_state *new_plane_state; > > + struct intel_plane *plane; > > + int i; > > + > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > > + new_plane_state, i) { > > + if (plane->pipe != crtc->pipe) > > + continue; > > + > > + i9xx_wm_compute(new_crtc_state, old_plane_state, new_plane_state); > > + } > > + > > + return 0; > > +} > > + > > /* > > * Documentation says: > > * "If the line size is small, the TLB fetches can get in the way of the > > @@ -4056,18 +4126,22 @@ static const struct intel_wm_funcs g4x_wm_funcs = { > > }; > > > > static const struct intel_wm_funcs pnv_wm_funcs = { > > + .compute_watermarks = i9xx_compute_watermarks, > > .update_wm = pnv_update_wm, > > }; > > > > static const struct intel_wm_funcs i965_wm_funcs = { > > + .compute_watermarks = i9xx_compute_watermarks, > > .update_wm = i965_update_wm, > > }; > > > > static const struct intel_wm_funcs i9xx_wm_funcs = { > > + .compute_watermarks = i9xx_compute_watermarks, > > .update_wm = i9xx_update_wm, > > }; > > > > static const struct intel_wm_funcs i845_wm_funcs = { > > + .compute_watermarks = i9xx_compute_watermarks, > > .update_wm = i845_update_wm, > > }; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 2aeb4cd5b5a1..33fec36ec0bd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -392,28 +392,6 @@ void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, > > plane_state->uapi.visible = false; > > } > > > > -/* FIXME nuke when all wm code is atomic */ > > -static bool intel_wm_need_update(const struct intel_plane_state *old_plane_state, > > - const struct intel_plane_state *new_plane_state) > > -{ > > - /* Update watermarks on tiling or size changes. */ > > - if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > - return true; > > - > > - if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > - return false; > > - > > - if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > - old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > - drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > >uapi.src) || > > - drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > >uapi.src) || > > - drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > >uapi.dst) || > > - drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > >uapi.dst)) > > - return true; > > - > > - return false; > > -} > > - > > static bool intel_plane_is_scaled(const struct intel_plane_state *plane_state) > > { > > int src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > > @@ -602,20 +580,6 @@ static int intel_plane_atomic_calc_changes(const struct intel_crtc_state > > *old_cr > > was_visible, visible, > > turn_off, turn_on, mode_changed); > > > > - if (turn_on) { > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > - new_crtc_state->update_wm_pre = true; > > - } else if (turn_off) { > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > - new_crtc_state->update_wm_post = true; > > - } else if (intel_wm_need_update(old_plane_state, new_plane_state)) { > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) { > > - /* FIXME bollocks */ > > - new_crtc_state->update_wm_pre = true; > > - new_crtc_state->update_wm_post = true; > > - } > > - } > > - > > With this change, update_wm_pre/post flag will move from intel_atomic_check_planes() to > intel_atomic_check_crtcs() which will call compute_watermarks() and update the flag. Just wanted to > clarify if this is expected. That is the whole point of the patch.
On Mon, Sep 23, 2024 at 08:35:06PM +0300, Ville Syrjälä wrote: > On Sun, Sep 22, 2024 at 10:40:32AM +0000, Govindapillai, Vinod wrote: > > On Mon, 2024-09-16 at 19:24 +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > As with other watermark calculations, the dodgy pre-g4x > > > update_wm_{pre,post} flag calcultion would like to know > > Typo: calculation > > > if a modeset is about to happen or not, and technically > > > later stages in the atomic_check() may still flag one. > > > In practice that shouldn't happen as we don't have dynamic > > > CDCLK implemented for these old platforms. > > > > > > Regardless it'll be nice to move this old cruft out from > > > the supposedly platform agnostic plane code. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > drivers/gpu/drm/i915/display/i9xx_wm.c | 74 +++++++++++++++++++ > > > .../gpu/drm/i915/display/intel_atomic_plane.c | 36 --------- > > > 2 files changed, 74 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c > > > index 3151a31a5653..15ed3b810947 100644 > > > --- a/drivers/gpu/drm/i915/display/i9xx_wm.c > > > +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c > > > @@ -705,6 +705,76 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) > > > } > > > } > > > > > > +static bool i9xx_wm_need_update(const struct intel_plane_state *old_plane_state, > > > + const struct intel_plane_state *new_plane_state) > > > +{ > > > + /* Update watermarks on tiling or size changes. */ > > > + if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > > + return true; > > > + > > > + if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > > + return false; > > > + > > > + if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > > + old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > > + drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > > >uapi.src) || > > > + drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > > >uapi.src) || > > > + drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > > >uapi.dst) || > > > + drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > > >uapi.dst)) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > +static void i9xx_wm_compute(struct intel_crtc_state *new_crtc_state, > > > + const struct intel_plane_state *old_plane_state, > > > + const struct intel_plane_state *new_plane_state) > > > +{ > > > + bool turn_off, turn_on, visible, was_visible, mode_changed; > > > + > > > + mode_changed = intel_crtc_needs_modeset(new_crtc_state); > > > + was_visible = old_plane_state->uapi.visible; > > > + visible = new_plane_state->uapi.visible; > > > + > > > + if (!was_visible && !visible) > > > + return; > > > + > > > + turn_off = was_visible && (!visible || mode_changed); > > > + turn_on = visible && (!was_visible || mode_changed); > > > + > > > + /* FIXME nuke when all wm code is atomic */ > > > + if (turn_on) { > > > + new_crtc_state->update_wm_pre = true; > > > + } else if (turn_off) { > > > + new_crtc_state->update_wm_post = true; > > > + } else if (i9xx_wm_need_update(old_plane_state, new_plane_state)) { > > > + /* FIXME bollocks */ > > > + new_crtc_state->update_wm_pre = true; > > > + new_crtc_state->update_wm_post = true; > > > + } > > > +} > > > + > > > +static int i9xx_compute_watermarks(struct intel_atomic_state *state, > > > + struct intel_crtc *crtc) > > > +{ > > > + struct intel_crtc_state *new_crtc_state = > > > + intel_atomic_get_new_crtc_state(state, crtc); > > > + const struct intel_plane_state *old_plane_state; > > > + const struct intel_plane_state *new_plane_state; > > > + struct intel_plane *plane; > > > + int i; > > > + > > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > > > + new_plane_state, i) { > > > + if (plane->pipe != crtc->pipe) > > > + continue; > > > + > > > + i9xx_wm_compute(new_crtc_state, old_plane_state, new_plane_state); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /* > > > * Documentation says: > > > * "If the line size is small, the TLB fetches can get in the way of the > > > @@ -4056,18 +4126,22 @@ static const struct intel_wm_funcs g4x_wm_funcs = { > > > }; > > > > > > static const struct intel_wm_funcs pnv_wm_funcs = { > > > + .compute_watermarks = i9xx_compute_watermarks, > > > .update_wm = pnv_update_wm, > > > }; > > > > > > static const struct intel_wm_funcs i965_wm_funcs = { > > > + .compute_watermarks = i9xx_compute_watermarks, > > > .update_wm = i965_update_wm, > > > }; > > > > > > static const struct intel_wm_funcs i9xx_wm_funcs = { > > > + .compute_watermarks = i9xx_compute_watermarks, > > > .update_wm = i9xx_update_wm, > > > }; > > > > > > static const struct intel_wm_funcs i845_wm_funcs = { > > > + .compute_watermarks = i9xx_compute_watermarks, > > > .update_wm = i845_update_wm, > > > }; > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > index 2aeb4cd5b5a1..33fec36ec0bd 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -392,28 +392,6 @@ void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, > > > plane_state->uapi.visible = false; > > > } > > > > > > -/* FIXME nuke when all wm code is atomic */ > > > -static bool intel_wm_need_update(const struct intel_plane_state *old_plane_state, > > > - const struct intel_plane_state *new_plane_state) > > > -{ > > > - /* Update watermarks on tiling or size changes. */ > > > - if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > > - return true; > > > - > > > - if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > > - return false; > > > - > > > - if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > > - old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > > - drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > > >uapi.src) || > > > - drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > > >uapi.src) || > > > - drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > > >uapi.dst) || > > > - drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > > >uapi.dst)) > > > - return true; > > > - > > > - return false; > > > -} > > > - > > > static bool intel_plane_is_scaled(const struct intel_plane_state *plane_state) > > > { > > > int src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > > > @@ -602,20 +580,6 @@ static int intel_plane_atomic_calc_changes(const struct intel_crtc_state > > > *old_cr > > > was_visible, visible, > > > turn_off, turn_on, mode_changed); > > > > > > - if (turn_on) { > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > > - new_crtc_state->update_wm_pre = true; > > > - } else if (turn_off) { > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > > - new_crtc_state->update_wm_post = true; > > > - } else if (intel_wm_need_update(old_plane_state, new_plane_state)) { > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) { > > > - /* FIXME bollocks */ > > > - new_crtc_state->update_wm_pre = true; > > > - new_crtc_state->update_wm_post = true; > > > - } > > > - } > > > - > > > > With this change, update_wm_pre/post flag will move from intel_atomic_check_planes() to > > intel_atomic_check_crtcs() which will call compute_watermarks() and update the flag. Just wanted to > > clarify if this is expected. > > That is the whole point of the patch. Well, I suppose not quite the whole point, but most of it. The other purpose was just to get this stuff out from the platform agnostic code.
On Tue, 2024-09-24 at 00:59 +0300, Ville Syrjälä wrote: > On Mon, Sep 23, 2024 at 08:35:06PM +0300, Ville Syrjälä wrote: > > On Sun, Sep 22, 2024 at 10:40:32AM +0000, Govindapillai, Vinod wrote: > > > On Mon, 2024-09-16 at 19:24 +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > As with other watermark calculations, the dodgy pre-g4x > > > > update_wm_{pre,post} flag calcultion would like to know > > > Typo: calculation > > > > if a modeset is about to happen or not, and technically > > > > later stages in the atomic_check() may still flag one. > > > > In practice that shouldn't happen as we don't have dynamic > > > > CDCLK implemented for these old platforms. > > > > > > > > Regardless it'll be nice to move this old cruft out from > > > > the supposedly platform agnostic plane code. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > --- > > > > drivers/gpu/drm/i915/display/i9xx_wm.c | 74 +++++++++++++++++++ > > > > .../gpu/drm/i915/display/intel_atomic_plane.c | 36 --------- > > > > 2 files changed, 74 insertions(+), 36 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c > > > > index 3151a31a5653..15ed3b810947 100644 > > > > --- a/drivers/gpu/drm/i915/display/i9xx_wm.c > > > > +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c > > > > @@ -705,6 +705,76 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) > > > > } > > > > } > > > > > > > > +static bool i9xx_wm_need_update(const struct intel_plane_state *old_plane_state, > > > > + const struct intel_plane_state *new_plane_state) > > > > +{ > > > > + /* Update watermarks on tiling or size changes. */ > > > > + if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > > > + return true; > > > > + > > > > + if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > > > + return false; > > > > + > > > > + if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > > > + old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > > > + drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > > > > uapi.src) || > > > > + drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > > > > uapi.src) || > > > > + drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > > > > uapi.dst) || > > > > + drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > > > > uapi.dst)) > > > > + return true; > > > > + > > > > + return false; > > > > +} > > > > + > > > > +static void i9xx_wm_compute(struct intel_crtc_state *new_crtc_state, > > > > + const struct intel_plane_state *old_plane_state, > > > > + const struct intel_plane_state *new_plane_state) > > > > +{ > > > > + bool turn_off, turn_on, visible, was_visible, mode_changed; > > > > + > > > > + mode_changed = intel_crtc_needs_modeset(new_crtc_state); > > > > + was_visible = old_plane_state->uapi.visible; > > > > + visible = new_plane_state->uapi.visible; > > > > + > > > > + if (!was_visible && !visible) > > > > + return; > > > > + > > > > + turn_off = was_visible && (!visible || mode_changed); > > > > + turn_on = visible && (!was_visible || mode_changed); > > > > + > > > > + /* FIXME nuke when all wm code is atomic */ > > > > + if (turn_on) { > > > > + new_crtc_state->update_wm_pre = true; > > > > + } else if (turn_off) { > > > > + new_crtc_state->update_wm_post = true; > > > > + } else if (i9xx_wm_need_update(old_plane_state, new_plane_state)) { > > > > + /* FIXME bollocks */ > > > > + new_crtc_state->update_wm_pre = true; > > > > + new_crtc_state->update_wm_post = true; > > > > + } > > > > +} > > > > + > > > > +static int i9xx_compute_watermarks(struct intel_atomic_state *state, > > > > + struct intel_crtc *crtc) > > > > +{ > > > > + struct intel_crtc_state *new_crtc_state = > > > > + intel_atomic_get_new_crtc_state(state, crtc); > > > > + const struct intel_plane_state *old_plane_state; > > > > + const struct intel_plane_state *new_plane_state; > > > > + struct intel_plane *plane; > > > > + int i; > > > > + > > > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, > > > > + new_plane_state, i) { > > > > + if (plane->pipe != crtc->pipe) > > > > + continue; > > > > + > > > > + i9xx_wm_compute(new_crtc_state, old_plane_state, new_plane_state); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /* > > > > * Documentation says: > > > > * "If the line size is small, the TLB fetches can get in the way of the > > > > @@ -4056,18 +4126,22 @@ static const struct intel_wm_funcs g4x_wm_funcs = { > > > > }; > > > > > > > > static const struct intel_wm_funcs pnv_wm_funcs = { > > > > + .compute_watermarks = i9xx_compute_watermarks, > > > > .update_wm = pnv_update_wm, > > > > }; > > > > > > > > static const struct intel_wm_funcs i965_wm_funcs = { > > > > + .compute_watermarks = i9xx_compute_watermarks, > > > > .update_wm = i965_update_wm, > > > > }; > > > > > > > > static const struct intel_wm_funcs i9xx_wm_funcs = { > > > > + .compute_watermarks = i9xx_compute_watermarks, > > > > .update_wm = i9xx_update_wm, > > > > }; > > > > > > > > static const struct intel_wm_funcs i845_wm_funcs = { > > > > + .compute_watermarks = i9xx_compute_watermarks, > > > > .update_wm = i845_update_wm, > > > > }; > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > index 2aeb4cd5b5a1..33fec36ec0bd 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > @@ -392,28 +392,6 @@ void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, > > > > plane_state->uapi.visible = false; > > > > } > > > > > > > > -/* FIXME nuke when all wm code is atomic */ > > > > -static bool intel_wm_need_update(const struct intel_plane_state *old_plane_state, > > > > - const struct intel_plane_state *new_plane_state) > > > > -{ > > > > - /* Update watermarks on tiling or size changes. */ > > > > - if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) > > > > - return true; > > > > - > > > > - if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) > > > > - return false; > > > > - > > > > - if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || > > > > - old_plane_state->hw.rotation != new_plane_state->hw.rotation || > > > > - drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state- > > > > > uapi.src) || > > > > - drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state- > > > > > uapi.src) || > > > > - drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state- > > > > > uapi.dst) || > > > > - drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state- > > > > > uapi.dst)) > > > > - return true; > > > > - > > > > - return false; > > > > -} > > > > - > > > > static bool intel_plane_is_scaled(const struct intel_plane_state *plane_state) > > > > { > > > > int src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > > > > @@ -602,20 +580,6 @@ static int intel_plane_atomic_calc_changes(const struct > > > > intel_crtc_state > > > > *old_cr > > > > was_visible, visible, > > > > turn_off, turn_on, mode_changed); > > > > > > > > - if (turn_on) { > > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > > > - new_crtc_state->update_wm_pre = true; > > > > - } else if (turn_off) { > > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) > > > > - new_crtc_state->update_wm_post = true; > > > > - } else if (intel_wm_need_update(old_plane_state, new_plane_state)) { > > > > - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) { > > > > - /* FIXME bollocks */ > > > > - new_crtc_state->update_wm_pre = true; > > > > - new_crtc_state->update_wm_post = true; > > > > - } > > > > - } > > > > - > > > > > > With this change, update_wm_pre/post flag will move from intel_atomic_check_planes() to > > > intel_atomic_check_crtcs() which will call compute_watermarks() and update the flag. Just > > > wanted to > > > clarify if this is expected. > > > > That is the whole point of the patch. > > Well, I suppose not quite the whole point, but most of it. > The other purpose was just to get this stuff out from the > platform agnostic code. > Thanks.. Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c index 3151a31a5653..15ed3b810947 100644 --- a/drivers/gpu/drm/i915/display/i9xx_wm.c +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c @@ -705,6 +705,76 @@ static void pnv_update_wm(struct drm_i915_private *dev_priv) } } +static bool i9xx_wm_need_update(const struct intel_plane_state *old_plane_state, + const struct intel_plane_state *new_plane_state) +{ + /* Update watermarks on tiling or size changes. */ + if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) + return true; + + if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) + return false; + + if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || + old_plane_state->hw.rotation != new_plane_state->hw.rotation || + drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state->uapi.src) || + drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state->uapi.src) || + drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state->uapi.dst) || + drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state->uapi.dst)) + return true; + + return false; +} + +static void i9xx_wm_compute(struct intel_crtc_state *new_crtc_state, + const struct intel_plane_state *old_plane_state, + const struct intel_plane_state *new_plane_state) +{ + bool turn_off, turn_on, visible, was_visible, mode_changed; + + mode_changed = intel_crtc_needs_modeset(new_crtc_state); + was_visible = old_plane_state->uapi.visible; + visible = new_plane_state->uapi.visible; + + if (!was_visible && !visible) + return; + + turn_off = was_visible && (!visible || mode_changed); + turn_on = visible && (!was_visible || mode_changed); + + /* FIXME nuke when all wm code is atomic */ + if (turn_on) { + new_crtc_state->update_wm_pre = true; + } else if (turn_off) { + new_crtc_state->update_wm_post = true; + } else if (i9xx_wm_need_update(old_plane_state, new_plane_state)) { + /* FIXME bollocks */ + new_crtc_state->update_wm_pre = true; + new_crtc_state->update_wm_post = true; + } +} + +static int i9xx_compute_watermarks(struct intel_atomic_state *state, + struct intel_crtc *crtc) +{ + struct intel_crtc_state *new_crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); + const struct intel_plane_state *old_plane_state; + const struct intel_plane_state *new_plane_state; + struct intel_plane *plane; + int i; + + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, + new_plane_state, i) { + if (plane->pipe != crtc->pipe) + continue; + + i9xx_wm_compute(new_crtc_state, old_plane_state, new_plane_state); + } + + return 0; +} + /* * Documentation says: * "If the line size is small, the TLB fetches can get in the way of the @@ -4056,18 +4126,22 @@ static const struct intel_wm_funcs g4x_wm_funcs = { }; static const struct intel_wm_funcs pnv_wm_funcs = { + .compute_watermarks = i9xx_compute_watermarks, .update_wm = pnv_update_wm, }; static const struct intel_wm_funcs i965_wm_funcs = { + .compute_watermarks = i9xx_compute_watermarks, .update_wm = i965_update_wm, }; static const struct intel_wm_funcs i9xx_wm_funcs = { + .compute_watermarks = i9xx_compute_watermarks, .update_wm = i9xx_update_wm, }; static const struct intel_wm_funcs i845_wm_funcs = { + .compute_watermarks = i9xx_compute_watermarks, .update_wm = i845_update_wm, }; diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 2aeb4cd5b5a1..33fec36ec0bd 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -392,28 +392,6 @@ void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, plane_state->uapi.visible = false; } -/* FIXME nuke when all wm code is atomic */ -static bool intel_wm_need_update(const struct intel_plane_state *old_plane_state, - const struct intel_plane_state *new_plane_state) -{ - /* Update watermarks on tiling or size changes. */ - if (old_plane_state->uapi.visible != new_plane_state->uapi.visible) - return true; - - if (!old_plane_state->hw.fb || !new_plane_state->hw.fb) - return false; - - if (old_plane_state->hw.fb->modifier != new_plane_state->hw.fb->modifier || - old_plane_state->hw.rotation != new_plane_state->hw.rotation || - drm_rect_width(&old_plane_state->uapi.src) != drm_rect_width(&new_plane_state->uapi.src) || - drm_rect_height(&old_plane_state->uapi.src) != drm_rect_height(&new_plane_state->uapi.src) || - drm_rect_width(&old_plane_state->uapi.dst) != drm_rect_width(&new_plane_state->uapi.dst) || - drm_rect_height(&old_plane_state->uapi.dst) != drm_rect_height(&new_plane_state->uapi.dst)) - return true; - - return false; -} - static bool intel_plane_is_scaled(const struct intel_plane_state *plane_state) { int src_w = drm_rect_width(&plane_state->uapi.src) >> 16; @@ -602,20 +580,6 @@ static int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_cr was_visible, visible, turn_off, turn_on, mode_changed); - if (turn_on) { - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) - new_crtc_state->update_wm_pre = true; - } else if (turn_off) { - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) - new_crtc_state->update_wm_post = true; - } else if (intel_wm_need_update(old_plane_state, new_plane_state)) { - if (DISPLAY_VER(dev_priv) < 5 && !IS_G4X(dev_priv)) { - /* FIXME bollocks */ - new_crtc_state->update_wm_pre = true; - new_crtc_state->update_wm_post = true; - } - } - if (visible || was_visible) new_crtc_state->fb_bits |= plane->frontbuffer_bit;