| Message ID | 20171221061425.5511-2-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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 >
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@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. 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. 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 >> >
On Wed, Jan 3, 2018 at 6:32 AM, Archit Taneja <architt@codeaurora.org> 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@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. > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 21, 2017 at 4:56 AM, Daniel Vetter <daniel@ffwll.ch> 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@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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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); + + 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. */
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(+)