On Mon, 2020-01-20 at 19:47 +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]>
Looks like, I would prefer to get your global changes landed first
as you have almost all patches reviewed by now. Once those land,
I will fetch those changes and use new global state handling in
my SAGV patches.
> ---
> 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
>
>
> +
> +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;
> +}
Just out of curiousity, aren't we supposed to lock global state,
by just grabbing crtcs always? One for read, all for write. I see now
you have two alternate ways of doing this, i.e either call
intel_atomic_lock_global_state or intel_atomic_serialize_global_state.
To me it seems better to have somewhat unified approach for global
state locking, however may be I'm missing something here.
Anyways,
Reviewed-by: Stanislav Lisovskiy <[email protected]>
> 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
