On Wed, Jan 3, 2018 at 6:32 AM, Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
>
>
> On 12/21/2017 03:26 PM, Daniel Vetter 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.
>
>
> I just wanted a func that returns me the private obj's state that is
> currently
> "in use". I.e, the old state pre-swap, and the new state post-swap. If I use
> drm_atomic_get_private_obj_state() after the swap has occurred, I get the
> old
> state back. In other words, there aren't funcs for private objs like
> drm_atomic_get_new_crtc_state() and drm_atomic_get_old_crtc_state() that we
> can use. I'll go through the code again carefully to see if my understanding
> isn't screwed up.
>
>
>>
>>
>>> +}
>>> +
>>> +/*
>>> + * 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'm also a bit unclear on how many private objs one should create. In this
> patchset,
> I just create one private obj instance, and stuff all of our shared
> resources into
> it (see mdp5_global_state below). I didn't see the point in creating one
> priv object
> per shared resource, since a single lock for all of them seemed okay here.
>
fwiw, with the current code, I stuck with just a single modeset lock,
since if you are re-assigning hwpipe's you are also re-assigning SMP
blocks. Perhaps we should already split that into two now that we
added dynamic assignment of mixers. (Ie. so a modeset on one output
wouldn't block pagefile on another.)
Maybe just separate private objects + locks for each thing would make
more sense than trying to micro-optimize the locking ;-)
BR,
-R
> Thanks,
> Archit
>
>
>>
>> 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
>>>
>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
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
