| Message ID | 20181012201703.29065-6-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Add virtual-planes support | expand |
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. Just a drive-by refactor idea. -Daniel > + if (IS_ERR(priv_state)) > + return ERR_CAST(priv_state); > + > + return to_omap_global_state(priv_state); > +} > + > +static struct drm_private_state * > +omap_global_duplicate_state(struct drm_private_obj *obj) > +{ > + struct omap_global_state *state; > + > + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL); > + if (!state) > + return NULL; > + > + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); > + > + return &state->base; > +} > + > +static void omap_global_destroy_state(struct drm_private_obj *obj, > + struct drm_private_state *state) > +{ > + struct omap_global_state *omap_state = to_omap_global_state(state); > + > + kfree(omap_state); > +} > + > +static const struct drm_private_state_funcs omap_global_state_funcs = { > + .atomic_duplicate_state = omap_global_duplicate_state, > + .atomic_destroy_state = omap_global_destroy_state, > +}; > + > +static int omap_global_obj_init(struct omap_drm_private *priv) > +{ > + struct omap_global_state *state; > + > + drm_modeset_lock_init(&priv->glob_obj_lock); > + > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!state) > + return -ENOMEM; > + > + drm_atomic_private_obj_init(&priv->glob_obj, &state->base, > + &omap_global_state_funcs); > + return 0; > +} > + > +static void omap_global_obj_fini(struct omap_drm_private *priv) > +{ > + drm_atomic_private_obj_fini(&priv->glob_obj); > + drm_modeset_lock_fini(&priv->glob_obj_lock); > +} > + > static void omap_disconnect_pipelines(struct drm_device *ddev) > { > struct omap_drm_private *priv = ddev->dev_private; > @@ -569,10 +657,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) > > omap_gem_init(ddev); > > - ret = omap_hwoverlays_init(priv); > + ret = omap_global_obj_init(priv); > if (ret) > goto err_gem_deinit; > > + ret = omap_hwoverlays_init(priv); > + if (ret) > + goto err_free_priv_obj; > + > ret = omap_modeset_init(ddev); > if (ret) { > dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret); > @@ -612,6 +704,8 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) > err_cleanup_modeset: > drm_mode_config_cleanup(ddev); > omap_drm_irq_uninstall(ddev); > +err_free_priv_obj: > + omap_global_obj_fini(priv); > err_free_overlays: > omap_hwoverlays_destroy(priv); > err_gem_deinit: > @@ -644,6 +738,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv) > omap_drm_irq_uninstall(ddev); > omap_gem_deinit(ddev); > > + omap_global_obj_fini(priv); > omap_hwoverlays_destroy(priv); > > destroy_workqueue(priv->wq); > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h > index 929f04e7cc3b..f374dc100447 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.h > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h > @@ -23,6 +23,7 @@ > #include <linux/workqueue.h> > > #include <drm/drmP.h> > +#include <drm/drm_atomic.h> > #include <drm/drm_crtc_helper.h> > #include <drm/drm_gem.h> > #include <drm/omap_drm.h> > @@ -54,6 +55,17 @@ struct omap_drm_pipeline { > struct omap_dss_device *display; > }; > > +/* > + * Global private object state for tracking resources that are shared across > + * multiple kms objects (planes/crtcs/etc). > + */ > +#define to_omap_global_state(x) container_of(x, struct omap_global_state, base) > +struct omap_global_state { > + struct drm_private_state base; > + > + struct drm_atomic_state *state; > +}; > + > struct omap_drm_private { > struct drm_device *ddev; > struct device *dev; > @@ -73,6 +85,13 @@ struct omap_drm_private { > unsigned int num_ovls; > struct omap_hw_overlay *overlays[8]; > > + /* > + * Global private object state, Do not access directly, use > + * omap_global_get_state() > + */ > + struct drm_modeset_lock glob_obj_lock; > + struct drm_private_obj glob_obj; > + > struct drm_fb_helper *fbdev; > > struct workqueue_struct *wq; > @@ -100,5 +119,9 @@ struct omap_drm_private { > > > int omap_debugfs_init(struct drm_minor *minor); > +struct omap_global_state *__must_check > +omap_get_global_state(struct drm_atomic_state *s); > +struct omap_global_state * > +omap_get_existing_global_state(struct omap_drm_private *priv); > > #endif /* __OMAPDRM_DRV_H__ */ > -- > 2.9.0 >
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
On Thu, Oct 18, 2018 at 08:11:30AM -0500, Benoit Parrot wrote: > 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 :) 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
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); + if (IS_ERR(priv_state)) + return ERR_CAST(priv_state); + + return to_omap_global_state(priv_state); +} + +static struct drm_private_state * +omap_global_duplicate_state(struct drm_private_obj *obj) +{ + struct omap_global_state *state; + + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL); + if (!state) + return NULL; + + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); + + return &state->base; +} + +static void omap_global_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *state) +{ + struct omap_global_state *omap_state = to_omap_global_state(state); + + kfree(omap_state); +} + +static const struct drm_private_state_funcs omap_global_state_funcs = { + .atomic_duplicate_state = omap_global_duplicate_state, + .atomic_destroy_state = omap_global_destroy_state, +}; + +static int omap_global_obj_init(struct omap_drm_private *priv) +{ + struct omap_global_state *state; + + drm_modeset_lock_init(&priv->glob_obj_lock); + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!state) + return -ENOMEM; + + drm_atomic_private_obj_init(&priv->glob_obj, &state->base, + &omap_global_state_funcs); + return 0; +} + +static void omap_global_obj_fini(struct omap_drm_private *priv) +{ + drm_atomic_private_obj_fini(&priv->glob_obj); + drm_modeset_lock_fini(&priv->glob_obj_lock); +} + static void omap_disconnect_pipelines(struct drm_device *ddev) { struct omap_drm_private *priv = ddev->dev_private; @@ -569,10 +657,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) omap_gem_init(ddev); - ret = omap_hwoverlays_init(priv); + ret = omap_global_obj_init(priv); if (ret) goto err_gem_deinit; + ret = omap_hwoverlays_init(priv); + if (ret) + goto err_free_priv_obj; + ret = omap_modeset_init(ddev); if (ret) { dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret); @@ -612,6 +704,8 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) err_cleanup_modeset: drm_mode_config_cleanup(ddev); omap_drm_irq_uninstall(ddev); +err_free_priv_obj: + omap_global_obj_fini(priv); err_free_overlays: omap_hwoverlays_destroy(priv); err_gem_deinit: @@ -644,6 +738,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv) omap_drm_irq_uninstall(ddev); omap_gem_deinit(ddev); + omap_global_obj_fini(priv); omap_hwoverlays_destroy(priv); destroy_workqueue(priv->wq); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 929f04e7cc3b..f374dc100447 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -23,6 +23,7 @@ #include <linux/workqueue.h> #include <drm/drmP.h> +#include <drm/drm_atomic.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_gem.h> #include <drm/omap_drm.h> @@ -54,6 +55,17 @@ struct omap_drm_pipeline { struct omap_dss_device *display; }; +/* + * Global private object state for tracking resources that are shared across + * multiple kms objects (planes/crtcs/etc). + */ +#define to_omap_global_state(x) container_of(x, struct omap_global_state, base) +struct omap_global_state { + struct drm_private_state base; + + struct drm_atomic_state *state; +}; + struct omap_drm_private { struct drm_device *ddev; struct device *dev; @@ -73,6 +85,13 @@ struct omap_drm_private { unsigned int num_ovls; struct omap_hw_overlay *overlays[8]; + /* + * Global private object state, Do not access directly, use + * omap_global_get_state() + */ + struct drm_modeset_lock glob_obj_lock; + struct drm_private_obj glob_obj; + struct drm_fb_helper *fbdev; struct workqueue_struct *wq; @@ -100,5 +119,9 @@ struct omap_drm_private { int omap_debugfs_init(struct drm_minor *minor); +struct omap_global_state *__must_check +omap_get_global_state(struct drm_atomic_state *s); +struct omap_global_state * +omap_get_existing_global_state(struct omap_drm_private *priv); #endif /* __OMAPDRM_DRV_H__ */
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(-)