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

[Neil Armstrong]

Hi,

On 12/10/2021 12:44, Tomi Valkeinen wrote:
> On 23/09/2021 10:06, Neil Armstrong wrote:
>> From: Benoit Parrot <bparrot@xxxxxx>
>>
>> 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 <bparrot@xxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>   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_free_priv_obj:
>> +    omap_global_obj_fini(priv);
>>   err_gem_deinit:
>> +    drm_mode_config_cleanup(ddev);
>>       omap_gem_deinit(ddev);
>>       destroy_workqueue(priv->wq);
>>       omap_disconnect_pipelines(ddev);
>> @@ -649,6 +732,8 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
>>         omap_modeset_fini(ddev);
>>       omap_hwoverlays_destroy(priv);
>> +    omap_global_obj_fini(priv);
>> +    drm_mode_config_cleanup(ddev);
>>       omap_gem_deinit(ddev);
>>         destroy_workqueue(priv->wq);
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
>> index b4d9c2062723..280cdd27bc8e 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -14,6 +14,7 @@
>>   #include "dss/omapdss.h"
>>   #include "dss/dss.h"
>>   +#include <drm/drm_atomic.h>
>>   #include <drm/drm_gem.h>
>>   #include <drm/omap_drm.h>
>>   @@ -41,6 +42,15 @@ struct omap_drm_pipeline {
>>       unsigned int alias_id;
>>   };
>>   +/*
>> + * 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)
> 
> Add empty line here.

Ack

> 
>> +struct omap_global_state {
>> +    struct drm_private_state base;
>> +};
>> +
>>   struct omap_drm_private {
>>       struct drm_device *ddev;
>>       struct device *dev;
>> @@ -61,6 +71,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;
> 
> This is not used... What am I missing?

It's a leftover from v4, now the lock has been moved into drm_atomic_get_private_obj_state()

> 
>> +    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.

Neil

> 
>  Tomi