Re: [PATCH v5 5/8] drm/omap: Add global state as a private atomic object
On 12/10/2021 15:38, Tomi Valkeinen wrote:
> On 12/10/2021 16:23, Neil Armstrong wrote:
>
+ struct drm_private_obj glob_obj;
+
struct drm_fb_helper *fbdev;
struct workqueue_struct *wq;
@@ -88,5 +105,9 @@ struct omap_drm_private {
void 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);
>>>
>>> These could also be separated by empty lines. At least to my eyes it gets
>>> confusing if those declarations are not separated.
>>
>> Atomic states can be extremely confusing, and hard to track.
>> I checked and they do what they are documented for...
>>
>> The omap_get_existing_global_state() is the most confusing since the result
>> depends if
>> we are in an atomic transaction of not.
>
> So here I was just talking about the cosmetics, how the lines above look
> like. I have trouble seeing where the function declaration starts and where
> it ends without looking closely, as both lines of the declaration start at
> the first column, and there are no empty lines between the declarations.
Ok, it's a legacy of the 80chars max, will reformat.
>
> But now that you mention, yes, the states are confusing =). And this series
> is somewhat difficult. I think it's important for future maintainability to
> include explanations and comments in this series for the confusing parts
> (plane-overlay mapping and state handling, mostly).
Yep I added some hopefully useful comments explaining that.
Neil
>
> Tomi
Re: [PATCH v5 5/8] drm/omap: Add global state as a private atomic object
On 12/10/2021 16:23, Neil Armstrong wrote:
+ struct drm_private_obj glob_obj;
+
struct drm_fb_helper *fbdev;
struct workqueue_struct *wq;
@@ -88,5 +105,9 @@ struct omap_drm_private {
void 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);
These could also be separated by empty lines. At least to my eyes it gets
confusing if those declarations are not separated.
Atomic states can be extremely confusing, and hard to track.
I checked and they do what they are documented for...
The omap_get_existing_global_state() is the most confusing since the result
depends if
we are in an atomic transaction of not.
So here I was just talking about the cosmetics, how the lines above look
like. I have trouble seeing where the function declaration starts and
where it ends without looking closely, as both lines of the declaration
start at the first column, and there are no empty lines between the
declarations.
But now that you mention, yes, the states are confusing =). And this
series is somewhat difficult. I think it's important for future
maintainability to include explanations and comments in this series for
the confusing parts (plane-overlay mapping and state handling, mostly).
Tomi
Re: [PATCH v5 5/8] drm/omap: Add global state as a private atomic object
Hi,
On 12/10/2021 12:44, Tomi Valkeinen wrote:
> On 23/09/2021 10:06, Neil Armstrong wrote:
>> From: Benoit Parrot
>>
>> 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.
>>
>> drm_mode_config_init() needs to be called earlier because it
>> creates/initializes the private_obj link list maintained by the atomic
>> framework. The private_obj link list has to exist prior to calling
>> drm_atomic_private_obj_init(). Similarly the cleanup handler are
>> reordered appropriately.
>
> I'm not really familiar with the private object. Did you check how current
> drivers use it? These patches are 3 years old, and things might have changed
> around the private object.
Indeed, I checked and this is used in vc4/tegra/arm/amd & msm in the same as
way.
>
>> Signed-off-by: Benoit Parrot
>> Signed-off-by: Neil Armstrong
>> ---
>> drivers/gpu/drm/omapdrm/omap_drv.c | 91 +-
>> drivers/gpu/drm/omapdrm/omap_drv.h | 21 +++
>> 2 files changed, 109 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index b994014b22e8..c7912374d393 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -128,6 +128,82 @@ 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;
>> +
>> + 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 drm_device *dev)
>> +{
>> + struct omap_drm_private *priv = dev->dev_private;
>> + struct omap_global_state *state;
>> +
>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + drm_atomic_private_obj_init(dev, &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);
>> +}
>> +
>> static void omap_disconnect_pipelines(struct drm_device *ddev)
>> {
>> struct omap_drm_private *priv = ddev->dev_private;
>> @@ -231,8 +307,6 @@ static int omap_modeset_init(struct drm_device *dev)
>> if (!omapdss_stack_is_ready())
>> return -EPROBE_DEFER;
>> - drm_mode_config_init(dev);
>> -
>> ret = omap_modeset_init_properties(dev);
>> if (ret < 0)
>> return ret;
>> @@ -583,10 +657,16 @@ static int omapdrm_init(struct omap_drm_private *priv,
>> struct device *dev)
>> omap_gem_init(ddev);
>> - ret = omap_hwoverlays_init(priv);
>> + drm_mode_config_init(ddev);
>> +
>> + ret = omap_global_obj_init(ddev);
>> if (ret)
>> goto err_gem_deinit;
>> + ret = omap_hwoverlays_init(priv);
>> + if (ret)
>> +
Re: [PATCH v5 5/8] drm/omap: Add global state as a private atomic object
On 23/09/2021 10:06, Neil Armstrong wrote:
From: Benoit Parrot
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.
drm_mode_config_init() needs to be called earlier because it
creates/initializes the private_obj link list maintained by the atomic
framework. The private_obj link list has to exist prior to calling
drm_atomic_private_obj_init(). Similarly the cleanup handler are
reordered appropriately.
I'm not really familiar with the private object. Did you check how
current drivers use it? These patches are 3 years old, and things might
have changed around the private object.
Signed-off-by: Benoit Parrot
Signed-off-by: Neil Armstrong
---
drivers/gpu/drm/omapdrm/omap_drv.c | 91 +-
drivers/gpu/drm/omapdrm/omap_drv.h | 21 +++
2 files changed, 109 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
b/drivers/gpu/drm/omapdrm/omap_drv.c
index b994014b22e8..c7912374d393 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -128,6 +128,82 @@ 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;
+
+ 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 drm_device *dev)
+{
+ struct omap_drm_private *priv = dev->dev_private;
+ struct omap_global_state *state;
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ drm_atomic_private_obj_init(dev, &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);
+}
+
static void omap_disconnect_pipelines(struct drm_device *ddev)
{
struct omap_drm_private *priv = ddev->dev_private;
@@ -231,8 +307,6 @@ static int omap_modeset_init(struct drm_device *dev)
if (!omapdss_stack_is_ready())
return -EPROBE_DEFER;
- drm_mode_config_init(dev);
-
ret = omap_modeset_init_properties(dev);
if (ret < 0)
return ret;
@@ -583,10 +657,16 @@ static int omapdrm_init(struct omap_drm_private *priv,
struct device *dev)
omap_gem_init(ddev);
- ret = omap_hwoverlays_init(priv);
+ drm_mode_config_init(ddev);
+
+ ret = omap_global_obj_init(ddev);
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);
@@ -624,7 +704,10 @@ static int omapdrm_init(struct omap_drm_private *priv,
struct device *dev)
omap_modeset_fini(ddev);
err_free_overlays:
omap_hwoverlays_destroy(priv);
+err_f
