Here it looks like, we are changing all the helpers to accept intel_crtc_state as an argument
and then use the intel_crtc_state->hw and ->uapi whereever needed inside the function.
But with seperating the drm_crtc_state now into hw and uapi, wasnt one of the goals
to retain teh helpers taking drm_crtc_state as an argument and not having to change
that all through the i915 driver?
Manasi
On Fri, Jun 14, 2019 at 10:21:15PM +0200, Maarten Lankhorst wrote:
> Use uapi for drm, and keep the internal state in hw.
>
> This patch is incomplete, just want feedback on the direction I'm going.
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 13 ++--
> drivers/gpu/drm/i915/intel_display.c | 88 +++++++++++++++-------------
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> 3 files changed, 57 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 1f891b554ead..36fbd408ba81 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -185,12 +185,14 @@ struct drm_crtc_state *
> intel_crtc_duplicate_state(struct drm_crtc *crtc)
> {
> struct intel_crtc_state *crtc_state;
> + const struct intel_crtc_state *old_crtc_state = to_intel_crtc_state(crtc->state);
>
> - crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> + crtc_state = kmemdup(old_crtc_state, sizeof(*crtc_state), GFP_KERNEL);
> if (!crtc_state)
> return NULL;
>
> - __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->hw);
> + __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi);
> + memset(&crtc_state->hw, 0, sizeof(crtc_state->hw));
>
> crtc_state->update_pipe = false;
> crtc_state->disable_lp_wm = false;
> @@ -203,7 +205,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> crtc_state->fb_bits = 0;
> crtc_state->update_planes = 0;
>
> - return &crtc_state->hw;
> + return &crtc_state->uapi;
> }
>
> /**
> @@ -218,7 +220,10 @@ void
> intel_crtc_destroy_state(struct drm_crtc *crtc,
> struct drm_crtc_state *state)
> {
> - drm_atomic_helper_crtc_destroy_state(crtc, state);
> + struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
> +
> + __drm_atomic_helper_crtc_destroy_state(crtc, &crtc_state->uapi);
> + kfree(crtc_state);
> }
>
> static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9aa273fb5415..2de978ab8fc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -514,9 +514,9 @@ icl_wa_scalerclkgating(struct drm_i915_private *dev_priv, enum pipe pipe,
> }
>
> static bool
> -needs_modeset(const struct drm_crtc_state *state)
> +needs_modeset(const struct intel_crtc_state *state)
> {
> - return drm_atomic_crtc_needs_modeset(state);
> + return drm_atomic_crtc_needs_modeset(&state->hw);
> }
>
> /*
> @@ -5795,7 +5795,7 @@ static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_s
> if (!old_crtc_state->ips_enabled)
> return false;
>
> - if (needs_modeset(&new_crtc_state->hw))
> + if (needs_modeset(new_crtc_state))
> return true;
>
> /*
> @@ -5822,7 +5822,7 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> if (!new_crtc_state->ips_enabled)
> return false;
>
> - if (needs_modeset(&new_crtc_state->hw))
> + if (needs_modeset(new_crtc_state))
> return true;
>
> /*
> @@ -5899,7 +5899,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> intel_fbc_post_update(crtc);
>
> if (new_primary_state->visible &&
> - (needs_modeset(&pipe_config->hw) ||
> + (needs_modeset(pipe_config) ||
> !old_primary_state->visible))
> intel_post_enable_primary(&crtc->base, pipe_config);
> }
> @@ -5923,7 +5923,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> struct drm_plane *primary = crtc->base.primary;
> struct drm_plane_state *old_primary_state =
> drm_atomic_get_old_plane_state(old_state, primary);
> - bool modeset = needs_modeset(&pipe_config->hw);
> + bool modeset = needs_modeset(pipe_config);
> struct intel_atomic_state *old_intel_state =
> to_intel_atomic_state(old_state);
>
> @@ -5983,7 +5983,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> * If we're doing a modeset, we're done. No need to do any pre-vblank
> * watermark programming here.
> */
> - if (needs_modeset(&pipe_config->hw))
> + if (needs_modeset(pipe_config))
> return;
>
> /*
> @@ -11338,7 +11338,7 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> 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(crtc_state);
> + bool mode_changed = needs_modeset(pipe_config);
> bool was_crtc_enabled = old_crtc_state->hw.active;
> bool is_crtc_enabled = crtc_state->active;
> bool turn_off, turn_on, visible, was_visible;
> @@ -11607,7 +11607,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
> int ret;
> - bool mode_changed = needs_modeset(crtc_state);
> + bool mode_changed = needs_modeset(pipe_config);
>
> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv) &&
> mode_changed && !crtc_state->active)
> @@ -13095,7 +13095,7 @@ intel_modeset_verify_crtc(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state,
> struct drm_crtc_state *new_state)
> {
> - if (!needs_modeset(new_state) &&
> + if (!needs_modeset(to_intel_crtc_state(new_state)) &&
> !to_intel_crtc_state(new_state)->update_pipe)
> return;
>
> @@ -13187,7 +13187,7 @@ static void intel_modeset_clear_plls(struct intel_atomic_state *state)
> struct intel_shared_dpll *old_dpll =
> old_crtc_state->shared_dpll;
>
> - if (!needs_modeset(&new_crtc_state->hw))
> + if (!needs_modeset(new_crtc_state))
> continue;
>
> new_crtc_state->shared_dpll = NULL;
> @@ -13217,7 +13217,7 @@ static int haswell_mode_set_planes_workaround(struct intel_atomic_state *state)
> /* look at all crtc's that are going to be enabled in during modeset */
> for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> if (!crtc_state->hw.active ||
> - !needs_modeset(&crtc_state->hw))
> + !needs_modeset(crtc_state))
> continue;
>
> if (first_crtc_state) {
> @@ -13242,7 +13242,7 @@ static int haswell_mode_set_planes_workaround(struct intel_atomic_state *state)
> crtc_state->hsw_workaround_pipe = INVALID_PIPE;
>
> if (!crtc_state->hw.active ||
> - needs_modeset(&crtc_state->hw))
> + needs_modeset(crtc_state))
> continue;
>
> /* 2 or more enabled crtcs means no need for w/a */
> @@ -13276,32 +13276,32 @@ static int intel_lock_all_pipes(struct drm_atomic_state *state)
> return 0;
> }
>
> -static int intel_modeset_all_pipes(struct drm_atomic_state *state)
> +static int intel_modeset_all_pipes(struct intel_atomic_state *state)
> {
> - struct drm_crtc *crtc;
> + struct intel_crtc *crtc;
>
> /*
> * Add all pipes to the state, and force
> * a modeset on all the active ones.
> */
> for_each_crtc(state->dev, crtc) {
> - struct drm_crtc_state *crtc_state;
> + struct intel_crtc_state *crtc_state;
> int ret;
>
> - crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + crtc_state = intel_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state))
> return PTR_ERR(crtc_state);
>
> - if (!crtc_state->active || needs_modeset(crtc_state))
> + if (!crtc_state->hw.active || needs_modeset(crtc_state))
> continue;
>
> crtc_state->mode_changed = true;
>
> - ret = drm_atomic_add_affected_connectors(state, crtc);
> + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> if (ret)
> return ret;
>
> - ret = drm_atomic_add_affected_planes(state, crtc);
> + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> if (ret)
> return ret;
> }
> @@ -13369,12 +13369,12 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
> }
>
> if (is_power_of_2(state->active_crtcs)) {
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *crtc_state;
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *crtc_state;
>
> pipe = ilog2(state->active_crtcs);
> - crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> - crtc_state = drm_atomic_get_new_crtc_state(&state->base, crtc);
> + crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> if (crtc_state && needs_modeset(crtc_state))
> pipe = INVALID_PIPE;
> } else {
> @@ -13393,7 +13393,7 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
> state->cdclk.pipe = pipe;
> } else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> &state->cdclk.actual)) {
> - ret = intel_modeset_all_pipes(&state->base);
> + ret = intel_modeset_all_pipes(state);
> if (ret < 0)
> return ret;
>
> @@ -13451,6 +13451,8 @@ static int intel_atomic_check(struct drm_device *dev,
> /* Catch I915_MODE_FLAG_INHERITED */
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> + new_intel_crtc_state->hw = new_intel_crtc_state->uapi;
> +
> if (new_crtc_state->hw.mode.private_flags !=
> old_crtc_state->hw.mode.private_flags)
> new_crtc_state->hw.mode_changed = true;
> @@ -13462,7 +13464,7 @@ static int intel_atomic_check(struct drm_device *dev,
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> - if (!needs_modeset(&new_crtc_state->hw))
> + if (!needs_modeset(new_crtc_state))
> continue;
>
> if (!new_crtc_state->hw.enable) {
> @@ -13480,7 +13482,7 @@ static int intel_atomic_check(struct drm_device *dev,
> new_crtc_state->update_pipe = true;
> }
>
> - if (needs_modeset(&new_crtc_state->hw))
> + if (needs_modeset(new_crtc_state))
> any_ms = true;
> }
>
> @@ -13515,12 +13517,12 @@ static int intel_atomic_check(struct drm_device *dev,
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> - if (!needs_modeset(&new_crtc_state->hw) &&
> + if (!needs_modeset(new_crtc_state) &&
> !new_crtc_state->update_pipe)
> continue;
>
> intel_dump_pipe_config(new_crtc_state, state,
> - needs_modeset(&new_crtc_state->hw) ?
> + needs_modeset(new_crtc_state) ?
> "[modeset]" : "[fastset]");
> }
>
> @@ -13567,7 +13569,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> - bool modeset = needs_modeset(new_crtc_state);
> + bool modeset = needs_modeset(pipe_config);
> struct intel_plane_state *new_plane_state =
> intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> to_intel_plane(crtc->primary));
> @@ -13776,20 +13778,23 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> intel_crtc = to_intel_crtc(crtc);
>
> - if (needs_modeset(new_crtc_state) ||
> - to_intel_crtc_state(new_crtc_state)->update_pipe) {
> + /* Copy for drm_atomic_helper_setup_commit() */
> + new_intel_crtc_state->hw = new_intel_crtc_state->uapi;
> +
> + if (needs_modeset(new_intel_crtc_state) ||
> + new_intel_crtc_state->update_pipe) {
>
> put_domains[intel_crtc->pipe] =
> modeset_get_crtc_power_domains(crtc,
> new_intel_crtc_state);
> }
>
> - if (!needs_modeset(new_crtc_state))
> + if (!needs_modeset(new_intel_crtc_state))
> continue;
>
> intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
>
> - if (old_crtc_state->active) {
> + if (new_intel_crtc_state->hw.active) {
> intel_crtc_disable_planes(intel_state, intel_crtc);
>
> /*
> @@ -13878,8 +13883,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>
> - if (new_crtc_state->active &&
> - !needs_modeset(new_crtc_state) &&
> + if (new_intel_crtc_state->hw.active &&
> + !needs_modeset(new_intel_crtc_state) &&
> (new_intel_crtc_state->hw.color_mgmt_changed ||
> new_intel_crtc_state->update_pipe))
> intel_color_load_luts(new_intel_crtc_state);
> @@ -14238,9 +14243,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> int ret;
>
> if (old_obj) {
> - struct drm_crtc_state *crtc_state =
> - drm_atomic_get_new_crtc_state(new_state->state,
> - plane->state->crtc);
> + struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(intel_state,
> + plane->state->crtc);
>
> /* Big Hammer, we also need to ensure that any pending
> * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> @@ -14401,7 +14406,7 @@ static void intel_begin_crtc_commit(struct intel_atomic_state *state,
> intel_atomic_get_old_crtc_state(state, crtc);
> struct intel_crtc_state *new_crtc_state =
> intel_atomic_get_new_crtc_state(state, crtc);
> - bool modeset = needs_modeset(&new_crtc_state->hw);
> + bool modeset = needs_modeset(new_crtc_state);
>
> /* Perform vblank evasion around commit operation */
> intel_pipe_update_start(new_crtc_state);
> @@ -14454,7 +14459,7 @@ static void intel_finish_crtc_commit(struct intel_atomic_state *state,
> intel_pipe_update_end(new_crtc_state);
>
> if (new_crtc_state->update_pipe &&
> - !needs_modeset(&new_crtc_state->hw) &&
> + !needs_modeset(new_crtc_state) &&
> old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED)
> intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
> }
> @@ -14608,6 +14613,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> ret = -ENOMEM;
> goto out_free;
> }
> + new_crtc_state->hw = new_crtc_state->uapi;
>
> drm_atomic_set_fb_for_plane(new_plane_state, fb);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ec50f9167161..d39ef2443ce9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -713,7 +713,7 @@ enum intel_output_format {
> };
>
> struct intel_crtc_state {
> - struct drm_crtc_state hw;
> + struct drm_crtc_state uapi, hw;
>
> /**
> * quirks - bitfield with hw state readout quirks
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
