On Thu, Jun 20, 2019 at 11:46:09PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> .../gpu/drm/i915/display/intel_atomic_plane.c | 39 +++++++------
> .../gpu/drm/i915/display/intel_atomic_plane.h | 5 +-
> drivers/gpu/drm/i915/display/intel_display.c | 58 +++++++++----------
> 3 files changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 30bd4e76fff9..025c09461c9a 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -176,33 +176,36 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> new_crtc_state->data_rate[plane->id] =
> intel_plane_data_rate(new_crtc_state, new_plane_state);
>
> - return intel_plane_atomic_calc_changes(old_crtc_state,
> - &new_crtc_state->base,
> - old_plane_state,
> - &new_plane_state->base);
> + return intel_plane_atomic_calc_changes(old_crtc_state, new_crtc_state,
> + old_plane_state, new_plane_state);
> }
>
> static int intel_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *new_plane_state)
> {
> - struct drm_atomic_state *state = new_plane_state->state;
> - const struct drm_plane_state *old_plane_state =
> - drm_atomic_get_old_plane_state(state, plane);
> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
> - const struct drm_crtc_state *old_crtc_state;
> - struct drm_crtc_state *new_crtc_state;
> -
> - new_plane_state->visible = false;
> + struct intel_atomic_state *state =
> + to_intel_atomic_state(new_plane_state->state);
> + const struct intel_plane_state *old_intel_plane_state =
> + intel_atomic_get_old_plane_state(state, to_intel_plane(plane));
> + struct intel_plane_state *new_intel_plane_state =
> + to_intel_plane_state(new_plane_state);
I think we should do the _new_plane_state trick for the function
arguments and then use the non-underscore names for the intel types.
> + struct drm_crtc *crtc =
> + new_intel_plane_state->base.crtc ?: old_intel_plane_state->base.crtc;
> + struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
?: not needed. I also dislike the crtc vs. intel_crc thing. Maybe
extract this mess into a small function that just returns the
intel_crtc we want?
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> + const struct intel_crtc_state *old_crtc_state;
> + struct intel_crtc_state *new_crtc_state;
> +
> + new_intel_plane_state->base.visible = false;
> if (!crtc)
> return 0;
>
> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + old_crtc_state = intel_atomic_get_old_crtc_state(state, intel_crtc);
> + new_crtc_state = intel_atomic_get_new_crtc_state(state, intel_crtc);
>
> - return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state),
> - to_intel_crtc_state(new_crtc_state),
> - to_intel_plane_state(old_plane_state),
> - to_intel_plane_state(new_plane_state));
> + return intel_plane_atomic_check_with_state(old_crtc_state,
> + new_crtc_state,
> + old_intel_plane_state,
> + new_intel_plane_state);
> }
>
> static struct intel_plane *
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index 1437a8797e10..cb7ef4f9eafd 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -8,7 +8,6 @@
>
> #include <linux/types.h>
>
> -struct drm_crtc_state;
> struct drm_plane;
> struct drm_property;
> struct intel_atomic_state;
> @@ -43,8 +42,8 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> const struct intel_plane_state *old_plane_state,
> struct intel_plane_state *intel_state);
> int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
> - struct drm_crtc_state *crtc_state,
> + struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *old_plane_state,
> - struct drm_plane_state *plane_state);
> + struct intel_plane_state *plane_state);
>
> #endif /* __INTEL_ATOMIC_PLANE_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index baa0e1957ffe..5c1db1d3d12b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11286,7 +11286,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> *
> * Returns true or false.
> */
> -static bool intel_wm_need_update(struct intel_plane_state *cur,
> +static bool intel_wm_need_update(const struct intel_plane_state *cur,
> struct intel_plane_state *new)
> {
> /* Update watermarks on tiling or size changes. */
> @@ -11318,33 +11318,28 @@ static bool needs_scaling(const struct intel_plane_state *state)
> }
>
> int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
> - struct drm_crtc_state *crtc_state,
> + struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *old_plane_state,
> - struct drm_plane_state *plane_state)
> + struct intel_plane_state *plane_state)
> {
> - struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> - struct drm_crtc *crtc = crtc_state->crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_plane *plane = to_intel_plane(plane_state->plane);
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - bool mode_changed = needs_modeset(pipe_config);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + bool mode_changed = needs_modeset(crtc_state);
> bool was_crtc_enabled = old_crtc_state->base.active;
> - bool is_crtc_enabled = crtc_state->active;
> + bool is_crtc_enabled = crtc_state->base.active;
> bool turn_off, turn_on, visible, was_visible;
> - struct drm_framebuffer *fb = plane_state->fb;
> + struct drm_framebuffer *fb = plane_state->base.fb;
> int ret;
>
> if (INTEL_GEN(dev_priv) >= 9 && plane->id != PLANE_CURSOR) {
> - ret = skl_update_scaler_plane(
> - to_intel_crtc_state(crtc_state),
> - to_intel_plane_state(plane_state));
> + ret = skl_update_scaler_plane(crtc_state, plane_state);
> if (ret)
> return ret;
> }
>
> was_visible = old_plane_state->base.visible;
> - visible = plane_state->visible;
> + visible = plane_state->base.visible;
>
> if (!was_crtc_enabled && WARN_ON(was_visible))
> was_visible = false;
> @@ -11360,22 +11355,22 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> * only combine the results from all planes in the current place?
> */
> if (!is_crtc_enabled) {
> - plane_state->visible = visible = false;
> - to_intel_crtc_state(crtc_state)->active_planes &= ~BIT(plane->id);
> - to_intel_crtc_state(crtc_state)->data_rate[plane->id] = 0;
> + plane_state->base.visible = visible = false;
> + crtc_state->active_planes &= ~BIT(plane->id);
> + crtc_state->data_rate[plane->id] = 0;
> }
>
> if (!was_visible && !visible)
> return 0;
>
> if (fb != old_plane_state->base.fb)
> - pipe_config->fb_changed = true;
> + crtc_state->fb_changed = true;
>
> turn_off = was_visible && (!visible || mode_changed);
> turn_on = visible && (!was_visible || mode_changed);
>
> DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%d:%s] with fb %i\n",
> - intel_crtc->base.base.id, intel_crtc->base.name,
> + crtc->base.base.id, crtc->base.name,
> plane->base.base.id, plane->base.name,
> fb ? fb->base.id : -1);
>
> @@ -11386,29 +11381,28 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
>
> if (turn_on) {
> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> - pipe_config->update_wm_pre = true;
> + crtc_state->update_wm_pre = true;
>
> /* must disable cxsr around plane enable/disable */
> if (plane->id != PLANE_CURSOR)
> - pipe_config->disable_cxsr = true;
> + crtc_state->disable_cxsr = true;
> } else if (turn_off) {
> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> - pipe_config->update_wm_post = true;
> + crtc_state->update_wm_post = true;
>
> /* must disable cxsr around plane enable/disable */
> if (plane->id != PLANE_CURSOR)
> - pipe_config->disable_cxsr = true;
> - } else if (intel_wm_need_update(to_intel_plane_state(plane->base.state),
> - to_intel_plane_state(plane_state))) {
> + crtc_state->disable_cxsr = true;
> + } else if (intel_wm_need_update(old_plane_state, plane_state)) {
> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) {
> /* FIXME bollocks */
> - pipe_config->update_wm_pre = true;
> - pipe_config->update_wm_post = true;
> + crtc_state->update_wm_pre = true;
> + crtc_state->update_wm_post = true;
> }
> }
>
> if (visible || was_visible)
> - pipe_config->fb_bits |= plane->frontbuffer_bit;
> + crtc_state->fb_bits |= plane->frontbuffer_bit;
>
> /*
> * ILK/SNB DVSACNTR/Sprite Enable
> @@ -11447,8 +11441,8 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> (IS_GEN_RANGE(dev_priv, 5, 6) ||
> IS_IVYBRIDGE(dev_priv)) &&
> (turn_on || (!needs_scaling(old_plane_state) &&
> - needs_scaling(to_intel_plane_state(plane_state)))))
> - pipe_config->disable_lp_wm = true;
> + needs_scaling(plane_state))))
> + crtc_state->disable_lp_wm = true;
>
> return 0;
> }
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
