On Mon, 2020-01-20 at 19:47 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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ä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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);
I believe that we can get rid of intel_atomic_lock_global_state and
intel_atomic_serialize_global_state() please take a look at
https://patchwork.freedesktop.org/series/72242/ there is one test
failing it you think this is something that can be used I will take a
look to the test failure.
> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
