Re: [Patch v4 5/8] drm/omap: Add global state as a private atomic object
On Thu, Oct 18, 2018 at 08:11:30AM -0500, Benoit Parrot wrote:
> Daniel Vetter 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
> > > ---
> > > 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Patch v4 5/8] drm/omap: Add global state as a private atomic object
Daniel Vetter 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
> > ---
> > 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Patch v4 5/8] drm/omap: Add global state as a private atomic object
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
> ---
> 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_in
[Patch v4 5/8] drm/omap: Add global state as a private atomic object
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
---
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);
+ 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
