On Mon, Jan 20, 2020 at 07:47:24PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > Our current global state handling is pretty ad-hoc. Let's try to > make it better by imitating the standard drm core private object > approach. > > The reason why we don't want to directly use the private objects > is locking; Each private object has its own lock so if we > introduce any global private objects we get serialized by that > single lock across all pipes. The global state apporoach instead > uses a read/write lock type of approach where each individual > crtc lock counts as a read lock, and grabbing all the crtc locks > allows one write access. > > Signed-off-by: Ville Syrjälä <[email protected]>
Didn't spot any issues: Reviewed-by: Imre Deak <[email protected]> Would be good to explain somewhere the difference between only locking vs. serializing the state. > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/display/intel_atomic.c | 7 +- > drivers/gpu/drm/i915/display/intel_atomic.h | 4 +- > drivers/gpu/drm/i915/display/intel_audio.c | 2 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 8 +- > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > .../drm/i915/display/intel_display_types.h | 4 + > .../gpu/drm/i915/display/intel_global_state.c | 223 ++++++++++++++++++ > .../gpu/drm/i915/display/intel_global_state.h | 87 +++++++ > drivers/gpu/drm/i915/i915_drv.h | 3 + > 10 files changed, 342 insertions(+), 12 deletions(-) > create mode 100644 drivers/gpu/drm/i915/display/intel_global_state.c > create mode 100644 drivers/gpu/drm/i915/display/intel_global_state.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 3c88d7d8c764..787ffe669810 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -190,6 +190,7 @@ i915-y += \ > display/intel_fbc.o \ > display/intel_fifo_underrun.o \ > display/intel_frontbuffer.o \ > + display/intel_global_state.o \ > display/intel_hdcp.o \ > display/intel_hotplug.o \ > display/intel_lpe_audio.o \ > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > b/drivers/gpu/drm/i915/display/intel_atomic.c > index 1c13423d4945..45842ebcdebd 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -37,6 +37,7 @@ > #include "intel_atomic.h" > #include "intel_cdclk.h" > #include "intel_display_types.h" > +#include "intel_global_state.h" > #include "intel_hdcp.h" > #include "intel_psr.h" > #include "intel_sprite.h" > @@ -500,6 +501,7 @@ void intel_atomic_state_free(struct drm_atomic_state > *_state) > struct intel_atomic_state *state = to_intel_atomic_state(_state); > > drm_atomic_state_default_release(&state->base); > + kfree(state->global_objs); > > i915_sw_fence_fini(&state->commit_ready); > > @@ -511,6 +513,7 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) > struct intel_atomic_state *state = to_intel_atomic_state(s); > > drm_atomic_state_default_clear(&state->base); > + intel_atomic_clear_global_state(state); > > state->dpll_set = state->modeset = false; > state->global_state_changed = false; > @@ -530,7 +533,7 @@ intel_atomic_get_crtc_state(struct drm_atomic_state > *state, > return to_intel_crtc_state(crtc_state); > } > > -int intel_atomic_lock_global_state(struct intel_atomic_state *state) > +int _intel_atomic_lock_global_state(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc *crtc; > @@ -549,7 +552,7 @@ int intel_atomic_lock_global_state(struct > intel_atomic_state *state) > return 0; > } > > -int intel_atomic_serialize_global_state(struct intel_atomic_state *state) > +int _intel_atomic_serialize_global_state(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc *crtc; > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h > b/drivers/gpu/drm/i915/display/intel_atomic.h > index 88133eea0a17..11146292b06f 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.h > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h > @@ -56,8 +56,8 @@ int intel_atomic_setup_scalers(struct drm_i915_private > *dev_priv, > struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state); > > -int intel_atomic_lock_global_state(struct intel_atomic_state *state); > +int _intel_atomic_lock_global_state(struct intel_atomic_state *state); > > -int intel_atomic_serialize_global_state(struct intel_atomic_state *state); > +int _intel_atomic_serialize_global_state(struct intel_atomic_state *state); > > #endif /* __INTEL_ATOMIC_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c > b/drivers/gpu/drm/i915/display/intel_audio.c > index 32e722128638..12626fd94d29 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -820,7 +820,7 @@ static void glk_force_audio_cdclk(struct drm_i915_private > *dev_priv, > enable ? 2 * 96000 : 0; > > /* Protects dev_priv->cdclk.force_min_cdclk */ > - ret = intel_atomic_lock_global_state(to_intel_atomic_state(state)); > + ret = _intel_atomic_lock_global_state(to_intel_atomic_state(state)); > if (!ret) > ret = drm_atomic_commit(state); > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 701a63c3ca38..3b7932ae2a77 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2063,7 +2063,7 @@ static int intel_compute_min_cdclk(struct > intel_atomic_state *state) > > cdclk_state->min_cdclk[i] = min_cdclk; > > - ret = intel_atomic_lock_global_state(state); > + ret = _intel_atomic_lock_global_state(state); > if (ret) > return ret; > } > @@ -2111,7 +2111,7 @@ static int bxt_compute_min_voltage_level(struct > intel_atomic_state *state) > > cdclk_state->min_voltage_level[i] = min_voltage_level; > > - ret = intel_atomic_lock_global_state(state); > + ret = _intel_atomic_lock_global_state(state); > if (ret) > return ret; > } > @@ -2386,12 +2386,12 @@ int intel_modeset_calc_cdclk(struct > intel_atomic_state *state) > * Also serialize commits across all crtcs > * if the actual hw needs to be poked. > */ > - ret = intel_atomic_serialize_global_state(state); > + ret = _intel_atomic_serialize_global_state(state); > if (ret) > return ret; > } else if (intel_cdclk_changed(&old_cdclk_state->logical, > &new_cdclk_state->logical)) { > - ret = intel_atomic_lock_global_state(state); > + ret = _intel_atomic_lock_global_state(state); > if (ret) > return ret; > } else { > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 3b725764bdcd..70eb6eaab095 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14381,7 +14381,7 @@ static int intel_modeset_checks(struct > intel_atomic_state *state) > } > > if (state->active_pipe_changes) { > - ret = intel_atomic_lock_global_state(state); > + ret = _intel_atomic_lock_global_state(state); > if (ret) > return ret; > } > @@ -15652,6 +15652,8 @@ static int intel_atomic_commit(struct drm_device *dev, > ret = drm_atomic_helper_setup_commit(&state->base, nonblock); > if (!ret) > ret = drm_atomic_helper_swap_state(&state->base, true); > + if (!ret) > + intel_atomic_swap_global_state(state); > > if (ret) { > i915_sw_fence_commit(&state->commit_ready); > @@ -17518,6 +17520,7 @@ static void intel_mode_config_init(struct > drm_i915_private *i915) > struct drm_mode_config *mode_config = &i915->drm.mode_config; > > drm_mode_config_init(&i915->drm); > + INIT_LIST_HEAD(&i915->global_obj_list); > > mode_config->min_width = 0; > mode_config->min_height = 0; > @@ -17559,6 +17562,12 @@ static void intel_mode_config_init(struct > drm_i915_private *i915) > } > } > > +static void intel_mode_config_cleanup(struct drm_i915_private *i915) > +{ > + intel_atomic_global_obj_cleanup(i915); > + drm_mode_config_cleanup(&i915->drm); > +} > + > int intel_modeset_init(struct drm_i915_private *i915) > { > struct drm_device *dev = &i915->drm; > @@ -17598,7 +17607,7 @@ int intel_modeset_init(struct drm_i915_private *i915) > for_each_pipe(i915, pipe) { > ret = intel_crtc_init(i915, pipe); > if (ret) { > - drm_mode_config_cleanup(dev); > + intel_mode_config_cleanup(i915); > return ret; > } > } > @@ -18551,7 +18560,7 @@ void intel_modeset_driver_remove(struct > drm_i915_private *i915) > > intel_hdcp_component_fini(i915); > > - drm_mode_config_cleanup(&i915->drm); > + intel_mode_config_cleanup(i915); > > intel_overlay_cleanup(i915); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index b31ed828fa8f..628c4a56a9e9 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -46,6 +46,7 @@ > #include "i915_drv.h" > > struct drm_printer; > +struct __intel_global_objs_state; > > /* > * Display related stuff > @@ -461,6 +462,9 @@ struct intel_atomic_state { > > intel_wakeref_t wakeref; > > + struct __intel_global_objs_state *global_objs; > + int num_global_objs; > + > struct intel_cdclk_state cdclk_state; > > bool dpll_set, modeset; > diff --git a/drivers/gpu/drm/i915/display/intel_global_state.c > b/drivers/gpu/drm/i915/display/intel_global_state.c > new file mode 100644 > index 000000000000..a0cc894c3868 > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_global_state.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2020 Intel Corporation > + */ > + > +#include <linux/string.h> > + > +#include "i915_drv.h" > +#include "intel_atomic.h" > +#include "intel_display_types.h" > +#include "intel_global_state.h" > + > +void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv, > + struct intel_global_obj *obj, > + struct intel_global_state *state, > + const struct intel_global_state_funcs *funcs) > +{ > + memset(obj, 0, sizeof(*obj)); > + > + obj->state = state; > + obj->funcs = funcs; > + list_add_tail(&obj->head, &dev_priv->global_obj_list); > +} > + > +void intel_atomic_global_obj_cleanup(struct drm_i915_private *dev_priv) > +{ > + struct intel_global_obj *obj, *next; > + > + list_for_each_entry_safe(obj, next, &dev_priv->global_obj_list, head) { > + list_del(&obj->head); > + obj->funcs->atomic_destroy_state(obj, obj->state); > + } > +} > + > +static void assert_global_state_write_locked(struct drm_i915_private > *dev_priv) > +{ > + struct intel_crtc *crtc; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) > + drm_modeset_lock_assert_held(&crtc->base.mutex); > +} > + > +static bool modeset_lock_is_held(struct drm_modeset_acquire_ctx *ctx, > + struct drm_modeset_lock *lock) > +{ > + struct drm_modeset_lock *l; > + > + list_for_each_entry(l, &ctx->locked, head) { > + if (lock == l) > + return true; > + } > + > + return false; > +} > + > +static void assert_global_state_read_locked(struct intel_atomic_state *state) > +{ > + struct drm_modeset_acquire_ctx *ctx = state->base.acquire_ctx; > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_crtc *crtc; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) { > + if (modeset_lock_is_held(ctx, &crtc->base.mutex)) > + return; > + } > + > + WARN(1, "Global state not read locked\n"); > +} > + > +struct intel_global_state * > +intel_atomic_get_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj) > +{ > + int index, num_objs, i; > + size_t size; > + struct __intel_global_objs_state *arr; > + struct intel_global_state *obj_state; > + > + for (i = 0; i < state->num_global_objs; i++) > + if (obj == state->global_objs[i].ptr) > + return state->global_objs[i].state; > + > + assert_global_state_read_locked(state); > + > + num_objs = state->num_global_objs + 1; > + size = sizeof(*state->global_objs) * num_objs; > + arr = krealloc(state->global_objs, size, GFP_KERNEL); > + if (!arr) > + return ERR_PTR(-ENOMEM); > + > + state->global_objs = arr; > + index = state->num_global_objs; > + memset(&state->global_objs[index], 0, sizeof(*state->global_objs)); > + > + obj_state = obj->funcs->atomic_duplicate_state(obj); > + if (!obj_state) > + return ERR_PTR(-ENOMEM); > + > + obj_state->changed = false; > + > + state->global_objs[index].state = obj_state; > + state->global_objs[index].old_state = obj->state; > + state->global_objs[index].new_state = obj_state; > + state->global_objs[index].ptr = obj; > + obj_state->state = state; > + > + state->num_global_objs = num_objs; > + > + DRM_DEBUG_ATOMIC("Added new global object %p state %p to %p\n", > + obj, obj_state, state); > + > + return obj_state; > +} > + > +struct intel_global_state * > +intel_atomic_get_old_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj) > +{ > + int i; > + > + for (i = 0; i < state->num_global_objs; i++) > + if (obj == state->global_objs[i].ptr) > + return state->global_objs[i].old_state; > + > + return NULL; > +} > + > +struct intel_global_state * > +intel_atomic_get_new_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj) > +{ > + int i; > + > + for (i = 0; i < state->num_global_objs; i++) > + if (obj == state->global_objs[i].ptr) > + return state->global_objs[i].new_state; > + > + return NULL; > +} > + > +void intel_atomic_swap_global_state(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_global_state *old_obj_state, *new_obj_state; > + struct intel_global_obj *obj; > + int i; > + > + for_each_oldnew_global_obj_in_state(state, obj, old_obj_state, > + new_obj_state, i) { > + WARN_ON(obj->state != old_obj_state); > + > + /* > + * If the new state wasn't modified (and properly > + * locked for write access) we throw it away. > + */ > + if (!new_obj_state->changed) > + continue; > + > + assert_global_state_write_locked(dev_priv); > + > + old_obj_state->state = state; > + new_obj_state->state = NULL; > + > + state->global_objs[i].state = old_obj_state; > + obj->state = new_obj_state; > + } > +} > + > +void intel_atomic_clear_global_state(struct intel_atomic_state *state) > +{ > + int i; > + > + for (i = 0; i < state->num_global_objs; i++) { > + struct intel_global_obj *obj = state->global_objs[i].ptr; > + > + obj->funcs->atomic_destroy_state(obj, > + state->global_objs[i].state); > + state->global_objs[i].ptr = NULL; > + state->global_objs[i].state = NULL; > + state->global_objs[i].old_state = NULL; > + state->global_objs[i].new_state = NULL; > + } > + state->num_global_objs = 0; > +} > + > +int intel_atomic_lock_global_state(struct intel_global_state *obj_state) > +{ > + struct intel_atomic_state *state = obj_state->state; > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_crtc *crtc; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) { > + int ret; > + > + ret = drm_modeset_lock(&crtc->base.mutex, > + state->base.acquire_ctx); > + if (ret) > + return ret; > + } > + > + obj_state->changed = true; > + > + return 0; > +} > + > +int intel_atomic_serialize_global_state(struct intel_global_state *obj_state) > +{ > + struct intel_atomic_state *state = obj_state->state; > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_crtc *crtc; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) { > + struct intel_crtc_state *crtc_state; > + > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + } > + > + obj_state->changed = true; > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/display/intel_global_state.h > b/drivers/gpu/drm/i915/display/intel_global_state.h > new file mode 100644 > index 000000000000..e6163a469029 > --- /dev/null > +++ b/drivers/gpu/drm/i915/display/intel_global_state.h > @@ -0,0 +1,87 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2020 Intel Corporation > + */ > + > +#ifndef __INTEL_GLOBAL_STATE_H__ > +#define __INTEL_GLOBAL_STATE_H__ > + > +#include <linux/list.h> > + > +struct drm_i915_private; > +struct intel_atomic_state; > +struct intel_global_obj; > +struct intel_global_state; > + > +struct intel_global_state_funcs { > + struct intel_global_state *(*atomic_duplicate_state)(struct > intel_global_obj *obj); > + void (*atomic_destroy_state)(struct intel_global_obj *obj, > + struct intel_global_state *state); > +}; > + > +struct intel_global_obj { > + struct list_head head; > + struct intel_global_state *state; > + const struct intel_global_state_funcs *funcs; > +}; > + > +#define intel_for_each_global_obj(obj, dev_priv) \ > + list_for_each_entry(obj, &(dev_priv)->global_obj_list, head) > + > +#define for_each_new_global_obj_in_state(__state, obj, new_obj_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_global_objs && \ > + ((obj) = (__state)->global_objs[__i].ptr, \ > + (new_obj_state) = (__state)->global_objs[__i].new_state, > 1); \ > + (__i)++) \ > + for_each_if(obj) > + > +#define for_each_old_global_obj_in_state(__state, obj, new_obj_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_global_objs && \ > + ((obj) = (__state)->global_objs[__i].ptr, \ > + (new_obj_state) = (__state)->global_objs[__i].old_state, > 1); \ > + (__i)++) \ > + for_each_if(obj) > + > +#define for_each_oldnew_global_obj_in_state(__state, obj, old_obj_state, > new_obj_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_global_objs && \ > + ((obj) = (__state)->global_objs[__i].ptr, \ > + (old_obj_state) = (__state)->global_objs[__i].old_state, \ > + (new_obj_state) = (__state)->global_objs[__i].new_state, > 1); \ > + (__i)++) \ > + for_each_if(obj) > + > +struct intel_global_state { > + struct intel_atomic_state *state; > + bool changed; > +}; > + > +struct __intel_global_objs_state { > + struct intel_global_obj *ptr; > + struct intel_global_state *state, *old_state, *new_state; > +}; > + > +void intel_atomic_global_obj_init(struct drm_i915_private *dev_priv, > + struct intel_global_obj *obj, > + struct intel_global_state *state, > + const struct intel_global_state_funcs *funcs); > +void intel_atomic_global_obj_cleanup(struct drm_i915_private *dev_priv); > + > +struct intel_global_state * > +intel_atomic_get_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj); > +struct intel_global_state * > +intel_atomic_get_old_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj); > +struct intel_global_state * > +intel_atomic_get_new_global_obj_state(struct intel_atomic_state *state, > + struct intel_global_obj *obj); > + > +void intel_atomic_swap_global_state(struct intel_atomic_state *state); > +void intel_atomic_clear_global_state(struct intel_atomic_state *state); > +int intel_atomic_lock_global_state(struct intel_global_state *obj_state); > +int intel_atomic_serialize_global_state(struct intel_global_state > *obj_state); > + > +#endif > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1787bfdd057f..b558e68b4dbd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -71,6 +71,7 @@ > #include "display/intel_dpll_mgr.h" > #include "display/intel_dsb.h" > #include "display/intel_frontbuffer.h" > +#include "display/intel_global_state.h" > #include "display/intel_gmbus.h" > #include "display/intel_opregion.h" > > @@ -1100,6 +1101,8 @@ struct drm_i915_private { > */ > struct mutex dpll_lock; > > + struct list_head global_obj_list; > + > /* > * For reading active_pipes, cdclk_state holding any crtc > * lock is sufficient, for writing must hold all of them. > -- > 2.24.1 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
