Re: [RFC 1/3] drm/msm/mdp5: Add global state as a private atomic object

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 21, 2017 at 11:44:23AM +0530, Archit Taneja wrote:
> Global shared resources (hwpipes, hwmixers and SMP) for MDP5 are
> implemented as a part of atomic state by subclassing drm_atomic_state.
> 
> The preferred approach is to use the drm_private_obj infrastructure
> available in the atomic core.
> 
> mdp5_global_state is introduced as a drm atomic private object. The two
> funcs mdp5_get_global_state() and mdp5_get_existing_global_state() are
> the two variants that will be used to access mdp5_global_state.
> 
> This will replace the existing mdp5_state struct (which subclasses
> drm_atomic_state) and the funcs around it. These will be removed later
> once we mdp5_global_state is put to use everywhere.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 86 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 27 +++++++++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index f7c0698fec40..dfc4d81124d5 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -106,6 +106,86 @@ static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state)
>  	swap(to_kms_state(state)->state, mdp5_kms->state);
>  }
>  
> +/* 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 mdp5_global_state *
> +mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms)
> +{
> +	return to_mdp5_global_state(mdp5_kms->glob_base.state);

This doesn't match the existing state stuff for everything else. Here you
look at the global state, but not at the one hanging off drm_atomic_state.

Maybe we should add a drm_atomic_get_existing_private_obj_state function
to make this easier?

I'm also not 100% sure on what semantics you want precisely.


> +}
> +
> +/*
> + * This acquires the modeset lock set aside for global state, creates
> + * a new duplicated private object state.
> + */
> +struct mdp5_global_state *mdp5_get_global_state(struct drm_atomic_state *s)
> +{
> +	struct msm_drm_private *priv = s->dev->dev_private;
> +	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> +	struct drm_private_state *priv_state;
> +	int ret;
> +
> +	ret = drm_modeset_lock(&mdp5_kms->glob_state_lock, s->acquire_ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	priv_state = drm_atomic_get_private_obj_state(s, &mdp5_kms->glob_base);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_mdp5_global_state(priv_state);
> +}
> +
> +static struct drm_private_state *
> +mdp5_global_duplicate_state(struct drm_private_obj *obj)
> +{
> +	struct mdp5_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 mdp5_global_destroy_state(struct drm_private_obj *obj,
> +				      struct drm_private_state *state)
> +{
> +	struct mdp5_global_state *mdp5_state = to_mdp5_global_state(state);
> +
> +	kfree(mdp5_state);
> +}
> +
> +static const struct drm_private_state_funcs mdp5_global_state_funcs = {
> +	.atomic_duplicate_state = mdp5_global_duplicate_state,
> +	.atomic_destroy_state = mdp5_global_destroy_state,
> +};
> +
> +static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
> +{
> +	struct mdp5_global_state *state;
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_lock_init(&mdp5_kms->glob_state_lock);

I thought a bit the last few days about how to integrate modeset locking
into driver private state objects. I think the simplest solution would be
if we just add a drm_modeset_lock to drm_private_obj, and push the locking
(both here and in the mst helper) into the core private obj code.

Per-object locking might be a bit overkill (it's definitely overkill for
mst), but it's also easier to avoid special cases.

That would reduce the boilerplate here a bit more, essentially converting
the wrappers into type casting functions.
-Daniel

> +
> +	state->mdp5_kms = mdp5_kms;
> +
> +	drm_atomic_private_obj_init(&mdp5_kms->glob_base,
> +				    &state->base,
> +				    &mdp5_global_state_funcs);
> +	return 0;
> +}
> +
>  static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
>  {
>  	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> @@ -727,6 +807,8 @@ static void mdp5_destroy(struct platform_device *pdev)
>  	if (mdp5_kms->rpm_enabled)
>  		pm_runtime_disable(&pdev->dev);
>  
> +	drm_atomic_private_obj_fini(&mdp5_kms->glob_base);
> +
>  	kfree(mdp5_kms->state);
>  }
>  
> @@ -887,6 +969,10 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev)
>  		goto fail;
>  	}
>  
> +	ret = mdp5_global_obj_init(mdp5_kms);
> +	if (ret)
> +		goto fail;
> +
>  	mdp5_kms->mmio = msm_ioremap(pdev, "mdp_phys", "MDP5");
>  	if (IS_ERR(mdp5_kms->mmio)) {
>  		ret = PTR_ERR(mdp5_kms->mmio);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index 9b3fe01089d1..522ddb835593 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -55,6 +55,13 @@ struct mdp5_kms {
>  	struct mdp5_state *state;
>  	struct drm_modeset_lock state_lock;
>  
> +	/*
> +	 * Global private object state, Do not access directly, use
> +	 * mdp5_global_get_state()
> +	 */
> +	struct drm_private_obj glob_base;
> +	struct drm_modeset_lock glob_state_lock;
> +
>  	struct mdp5_smp *smp;
>  	struct mdp5_ctl_manager *ctlm;
>  
> @@ -95,6 +102,26 @@ struct mdp5_state {
>  struct mdp5_state *__must_check
>  mdp5_get_state(struct drm_atomic_state *s);
>  
> +/* Global private object state for tracking resources that are shared across
> + * multiple kms objects (planes/crtcs/etc).
> + */
> +#define to_mdp5_global_state(x) container_of(x, struct mdp5_global_state, base)
> +struct mdp5_global_state {
> +	struct drm_private_state base;
> +
> +	struct drm_atomic_state *state;
> +	struct mdp5_kms *mdp5_kms;
> +
> +	struct mdp5_hw_pipe_state hwpipe;
> +	struct mdp5_hw_mixer_state hwmixer;
> +	struct mdp5_smp_state smp;
> +};
> +
> +struct mdp5_global_state *
> +mdp5_get_existing_global_state(struct mdp5_kms *mdp5_kms);
> +struct mdp5_global_state *__must_check
> +mdp5_get_global_state(struct drm_atomic_state *s);
> +
>  /* Atomic plane state.  Subclasses the base drm_plane_state in order to
>   * track assigned hwpipe and hw specific state.
>   */
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

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