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

[Benoit Parrot <bparrot@ti.com>]

Daniel Vetter <daniel@ffwll.ch> 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@ti.com>
> > ---
> >  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 :) 

Benoit

> 
> Just a drive-by refactor idea.
> -Daniel