[Intel-gfx] [PATCH 13/17] drm/i915: Intrduce better global state handling
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Jan 28 15:29:40 UTC 2020
On Tue, Jan 28, 2020 at 02:44:04PM +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2020-01-20 at 19:47 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > 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 at linux.intel.com>
>
> 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.
One just locks things which means other nonblocking commits can still
execute in parallel once the locks are dropped. The serialize variant
adds all the crtcs to the state meaning their other commits will get
serialized against the current commit no matter on which wq they
execute. So just an attempt to avoid needless serialization and the
resulting framerate stutters.
>
> Anyways,
>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
>
> > 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.
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list