Re: [Patch v4 5/8] drm/omap: Add global state as a private atomic object

[Daniel Vetter]

On Thu, Oct 18, 2018 at 08:11:30AM -0500, Benoit Parrot wrote:
> Daniel Vetter <daniel@xxxxxxxx> wrote on Tue [2018-Oct-16 14:29:46 +0200]:
> > On Fri, Oct 12, 2018 at 03:17:00PM -0500, Benoit Parrot wrote:
> > > Global shared resources (like hw overlays) for omapdrm are implemented
> > > as a part of atomic state using the drm_private_obj infrastructure
> > > available in the atomic core.
> > > 
> > > omap_global_state is introduced as a drm atomic private object. The two
> > > funcs omap_get_global_state() and omap_get_existing_global_state() are
> > > the two variants that will be used to access omap_global_state.
> > > 
> > > Signed-off-by: Benoit Parrot <bparrot@xxxxxx>
> > > ---
> > >  drivers/gpu/drm/omapdrm/omap_drv.c | 97 +++++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/omapdrm/omap_drv.h | 23 +++++++++
> > >  2 files changed, 119 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > index 2921cc90f2d8..94658ec79c76 100644
> > > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > @@ -129,6 +129,94 @@ static const struct drm_mode_config_funcs omap_mode_config_funcs = {
> > >  	.atomic_commit = drm_atomic_helper_commit,
> > >  };
> > >  
> > > +/* Global/shared object state funcs */
> > > +
> > > +/*
> > > + * This is a helper that returns the private state currently in operation.
> > > + * Note that this would return the "old_state" if called in the atomic check
> > > + * path, and the "new_state" after the atomic swap has been done.
> > > + */
> > > +struct omap_global_state *
> > > +omap_get_existing_global_state(struct omap_drm_private *priv)
> > > +{
> > > +	return to_omap_global_state(priv->glob_obj.state);
> > > +}
> > > +
> > > +/*
> > > + * This acquires the modeset lock set aside for global state, creates
> > > + * a new duplicated private object state.
> > > + */
> > > +struct omap_global_state *__must_check
> > > +omap_get_global_state(struct drm_atomic_state *s)
> > > +{
> > > +	struct omap_drm_private *priv = s->dev->dev_private;
> > > +	struct drm_private_state *priv_state;
> > > +	int ret;
> > > +
> > > +	if (!drm_modeset_is_locked(&priv->glob_obj_lock)) {
> > > +		ret = drm_modeset_lock(&priv->glob_obj_lock, s->acquire_ctx);
> > > +		if (ret) {
> > > +			DBG("getting priv->glob_obj_lock (%p) failed %d",
> > > +			    &priv->glob_obj_lock, ret);
> > > +			return ERR_PTR(ret);
> > > +		}
> > > +	}
> > > +
> > > +	priv_state = drm_atomic_get_private_obj_state(s, &priv->glob_obj);
> > 
> > One of the refactors I had in mind (and which would be possible now that
> > private state structs are implemented as properly structs, instead of void
> > * pointers): Add a drm_modeset_lock to drm_private_obj and avoid having to
> > duplicate that over all implementations. Not everyone wants a per-obj
> > lock, but no one will be hurt by having a per-obj lock - drm_modeset_lock
> > is very extensible in that way. And we could drop the custom locking code
> > everyone has to roll themselves.
> 
> Thanks for the feedback. I was wondering the same when I was "duplicating"
> this code. I will take this under advisement, but I would probably see that
> as a separate patch set, either before or after this one :) 

Sure. Also note that this came up with a recent vc4 patch series by Boris
Brezillion "[RFC PATCH] drm/vc4: Add a load tracker to prevent HVS
underflow errors", so maybe you folks can work together.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch