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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 21, 2017 at 4:56 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxx>
>> ---
>>  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 is used for read-only access to current state, a couple places in
atomic update post-swap (where
drm_atomic_get_existing_private_obj_state() would work, but also for
things like debugfs, where it wouldn't)..

Probably should const'ify the return value to make it clear that this
is read-only.

>
>> +}
>> +
>> +/*
>> + * 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.

I like the idea of adding a modeset lock to private objs.. I've got a
WIP patch to do this.

BR,
-R

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux