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

[Daniel Vetter]

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@xxxxxx>
> ---
>  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
Software Engineer, Intel Corporation
http://blog.ffwll.ch