* [PATCH v2 0/6] drm/msm: Switch to atomic helpers @ 2018-03-28 19:06 Sean Paul 2018-03-28 19:06 ` Sean Paul ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: architt-sgV2jX0FEOL9JmXXK+q4OQ, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, Sean Paul, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-F7+t8E8rja9g9hUCZPvPmw I noticed a couple of rough edges yesterday as I was running some tests, so here's v2 which fixes them. Most notably, I was experiencing issues when the crtc was being disabled with abandoned events and vblank timeouts. Note that I've bundled the private_obj change into this set since it shares the same theme. It's technically v5, but I'll label it v2 with the rest of the set. PTAL, Sean Sean Paul (6): drm/msm: Use drm_private_obj/state instead of subclassing drm/msm: Refactor complete_commit() to look more the helpers drm/msm: Mark the crtc->state->event consumed drm/msm: Issue queued events when disabling crtc drm/msm: Remove msm_commit/worker, use atomic helper commit drm/msm: Switch to atomic_helper_commit() drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 77 +++---- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 11 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 28 ++- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 19 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +- drivers/gpu/drm/msm/msm_atomic.c | 223 +-------------------- drivers/gpu/drm/msm/msm_drv.c | 95 ++++++++- drivers/gpu/drm/msm/msm_drv.h | 10 +- drivers/gpu/drm/msm/msm_kms.h | 21 +- 12 files changed, 204 insertions(+), 307 deletions(-) -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/6] drm/msm: Use drm_private_obj/state instead of subclassing 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel; +Cc: linux-kernel, abhinavk, hoegsberg Now that we have private state handled by the core, we can use those instead of rolling our own swap_state for private data. Originally posted here: https://patchwork.freedesktop.org/patch/211361/ Changes in v2: - Use state->state in disp duplicate_state callback (Jeykumar) Changes in v3: - Update comment describing msm_kms_state (Jeykumar) Changes in v4: - Rebased on msm-next - Don't always use private state from atomic state (Archit) - Renamed some of the state accessors - Tested on mdp5 db410c Changes in v5: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Cc: Archit Taneja <architt@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 77 ++++++++++--------- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 11 +-- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++- drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 28 ++++--- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 19 +++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +- drivers/gpu/drm/msm/msm_atomic.c | 37 --------- drivers/gpu/drm/msm/msm_drv.c | 87 +++++++++++++++++++++- drivers/gpu/drm/msm/msm_drv.h | 3 - drivers/gpu/drm/msm/msm_kms.h | 21 ++++-- 10 files changed, 183 insertions(+), 116 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 6d8e3a9a6fc0..366670043190 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -70,60 +70,62 @@ static int mdp5_hw_init(struct msm_kms *kms) return 0; } -struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s) +struct mdp5_state *mdp5_state_from_atomic(struct drm_atomic_state *state) { - struct msm_drm_private *priv = s->dev->dev_private; - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct msm_kms_state *state = to_kms_state(s); - struct mdp5_state *new_state; - int ret; + struct msm_kms_state *kms_state = msm_kms_state_from_atomic(state); - if (state->state) - return state->state; + if (IS_ERR_OR_NULL(kms_state)) + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ - ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx); - if (ret) - return ERR_PTR(ret); + return kms_state->state; +} - new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!new_state) - return ERR_PTR(-ENOMEM); +struct mdp5_state *mdp5_state_from_dev(struct drm_device *dev) +{ + struct msm_kms_state *kms_state = msm_kms_state_from_dev(dev); - /* Copy state: */ - new_state->hwpipe = mdp5_kms->state->hwpipe; - new_state->hwmixer = mdp5_kms->state->hwmixer; - if (mdp5_kms->smp) - new_state->smp = mdp5_kms->state->smp; + if (IS_ERR_OR_NULL(kms_state)) + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ - state->state = new_state; + return kms_state->state; +} + +static void *mdp5_duplicate_state(void *state) +{ + if (!state) + return kzalloc(sizeof(struct mdp5_state), GFP_KERNEL); - return new_state; + return kmemdup(state, sizeof(struct mdp5_state), GFP_KERNEL); } -static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_destroy_state(void *state) { - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); - swap(to_kms_state(state)->state, mdp5_kms->state); + struct mdp5_state *mdp_state = (struct mdp5_state *)state; + kfree(mdp_state); } -static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_prepare_commit(struct msm_kms *kms, + struct drm_atomic_state *old_state) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); struct device *dev = &mdp5_kms->pdev->dev; pm_runtime_get_sync(dev); if (mdp5_kms->smp) - mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_state->smp); } -static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_complete_commit(struct msm_kms *kms, + struct drm_atomic_state *old_state) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); struct device *dev = &mdp5_kms->pdev->dev; if (mdp5_kms->smp) - mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_state->smp); pm_runtime_put_sync(dev); } @@ -229,7 +231,8 @@ static const struct mdp_kms_funcs kms_funcs = { .irq = mdp5_irq, .enable_vblank = mdp5_enable_vblank, .disable_vblank = mdp5_disable_vblank, - .swap_state = mdp5_swap_state, + .duplicate_state = mdp5_duplicate_state, + .destroy_state = mdp5_destroy_state, .prepare_commit = mdp5_prepare_commit, .complete_commit = mdp5_complete_commit, .wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done, @@ -726,8 +729,6 @@ static void mdp5_destroy(struct platform_device *pdev) if (mdp5_kms->rpm_enabled) pm_runtime_disable(&pdev->dev); - - kfree(mdp5_kms->state); } static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, @@ -859,7 +860,8 @@ static int interface_init(struct mdp5_kms *mdp5_kms) return 0; } -static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) +static int mdp5_init(struct platform_device *pdev, struct drm_device *dev, + struct msm_kms_state *initial_state) { struct msm_drm_private *priv = dev->dev_private; struct mdp5_kms *mdp5_kms; @@ -880,9 +882,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) mdp5_kms->dev = dev; mdp5_kms->pdev = pdev; - drm_modeset_lock_init(&mdp5_kms->state_lock); - mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!mdp5_kms->state) { + initial_state->state = mdp5_duplicate_state(NULL); + if (!initial_state->state) { ret = -ENOMEM; goto fail; } @@ -940,7 +941,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) * this section initializes the SMP: */ if (mdp5_kms->caps & MDP_CAP_SMP) { - mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp); + mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp, + initial_state->state); if (IS_ERR(mdp5_kms->smp)) { ret = PTR_ERR(mdp5_kms->smp); mdp5_kms->smp = NULL; @@ -980,10 +982,11 @@ static int mdp5_bind(struct device *dev, struct device *master, void *data) { struct drm_device *ddev = dev_get_drvdata(master); struct platform_device *pdev = to_platform_device(dev); + struct msm_kms_state *initial_state = (struct msm_kms_state *)data; DBG(""); - return mdp5_init(pdev, ddev); + return mdp5_init(pdev, ddev, initial_state); } static void mdp5_unbind(struct device *dev, struct device *master, diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h index 425a03d213e5..e23117b82aad 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h @@ -28,8 +28,6 @@ #include "mdp5_ctl.h" #include "mdp5_smp.h" -struct mdp5_state; - struct mdp5_kms { struct mdp_kms base; @@ -49,12 +47,6 @@ struct mdp5_kms { struct mdp5_cfg_handler *cfg; uint32_t caps; /* MDP capabilities (MDP_CAP_XXX bits) */ - /** - * Global atomic state. Do not access directly, use mdp5_get_state() - */ - struct mdp5_state *state; - struct drm_modeset_lock state_lock; - struct mdp5_smp *smp; struct mdp5_ctl_manager *ctlm; @@ -93,7 +85,8 @@ struct mdp5_state { }; struct mdp5_state *__must_check -mdp5_get_state(struct drm_atomic_state *s); +mdp5_state_from_atomic(struct drm_atomic_state *s); +struct mdp5_state *__must_check mdp5_state_from_dev(struct drm_device *dev); /* Atomic plane state. Subclasses the base drm_plane_state in order to * track assigned hwpipe and hw specific state. diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c index 8a00991f03c7..fdd5e12a9d56 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c @@ -52,10 +52,11 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); + struct mdp5_state *state; struct mdp5_hw_mixer_state *new_state; int i; + state = mdp5_state_from_atomic(s); if (IS_ERR(state)) return PTR_ERR(state); @@ -129,12 +130,17 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) { - struct mdp5_state *state = mdp5_get_state(s); - struct mdp5_hw_mixer_state *new_state = &state->hwmixer; + struct mdp5_state *state; + struct mdp5_hw_mixer_state *new_state; if (!mixer) return; + state = mdp5_state_from_atomic(s); + if (IS_ERR(state)) + return; + + new_state = &state->hwmixer; if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) return; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c index ff52c49095f9..dc66ab9fdd50 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c @@ -24,17 +24,20 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state; + struct mdp5_state *old_mdp5_state, *new_mdp5_state; struct mdp5_hw_pipe_state *old_state, *new_state; int i, j; - state = mdp5_get_state(s); - if (IS_ERR(state)) - return PTR_ERR(state); + new_mdp5_state = mdp5_state_from_atomic(s); + if (IS_ERR(new_mdp5_state)) + return PTR_ERR(new_mdp5_state); - /* grab old_state after mdp5_get_state(), since now we hold lock: */ - old_state = &mdp5_kms->state->hwpipe; - new_state = &state->hwpipe; + old_mdp5_state = mdp5_state_from_dev(s->dev); + if (IS_ERR(old_mdp5_state)) + return PTR_ERR(old_mdp5_state); + + old_state = &old_mdp5_state->hwpipe; + new_state = &new_mdp5_state->hwpipe; for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i]; @@ -107,7 +110,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, WARN_ON(r_hwpipe); DBG("%s: alloc SMP blocks", (*hwpipe)->name); - ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp, + ret = mdp5_smp_assign(mdp5_kms->smp, &new_mdp5_state->smp, (*hwpipe)->pipe, blkcfg); if (ret) return -ENOMEM; @@ -132,12 +135,17 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe) { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); - struct mdp5_hw_pipe_state *new_state = &state->hwpipe; + struct mdp5_state *state; + struct mdp5_hw_pipe_state *new_state; if (!hwpipe) return; + state = mdp5_state_from_atomic(s); + if (IS_ERR(state)) + return; + + new_state = &state->hwpipe; if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx])) return; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c index ae4983d9d0a5..918e4123e781 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c @@ -338,6 +338,7 @@ void mdp5_smp_complete_commit(struct mdp5_smp *smp, struct mdp5_smp_state *state void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) { struct mdp5_kms *mdp5_kms = get_kms(smp); + struct mdp5_state *mdp5_state; struct mdp5_hw_pipe_state *hwpstate; struct mdp5_smp_state *state; int total = 0, i, j; @@ -346,11 +347,12 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) drm_printf(p, "----\t-----\t-----\n"); if (drm_can_sleep()) - drm_modeset_lock(&mdp5_kms->state_lock, NULL); + drm_modeset_lock_all(mdp5_kms->dev); - /* grab these *after* we hold the state_lock */ - hwpstate = &mdp5_kms->state->hwpipe; - state = &mdp5_kms->state->smp; + /* grab these *after* we hold the modeset_lock */ + mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); + hwpstate = &mdp5_state->hwpipe; + state = &mdp5_state->smp; for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i]; @@ -374,7 +376,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) bitmap_weight(state->state, smp->blk_cnt)); if (drm_can_sleep()) - drm_modeset_unlock(&mdp5_kms->state_lock); + drm_modeset_unlock_all(mdp5_kms->dev); } void mdp5_smp_destroy(struct mdp5_smp *smp) @@ -382,12 +384,15 @@ void mdp5_smp_destroy(struct mdp5_smp *smp) kfree(smp); } -struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg) +struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, + const struct mdp5_smp_block *cfg, + struct mdp5_state *mdp5_state) { - struct mdp5_smp_state *state = &mdp5_kms->state->smp; + struct mdp5_smp_state *state; struct mdp5_smp *smp = NULL; int ret; + state = &mdp5_state->smp; smp = kzalloc(sizeof(*smp), GFP_KERNEL); if (unlikely(!smp)) { ret = -ENOMEM; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h index b41d0448fbe8..1bfa22b63a2d 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h @@ -69,6 +69,7 @@ struct mdp5_smp_state { }; struct mdp5_kms; +struct mdp5_state; struct mdp5_smp; /* @@ -78,7 +79,8 @@ struct mdp5_smp; */ struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, - const struct mdp5_smp_block *cfg); + const struct mdp5_smp_block *cfg, + struct mdp5_state *mdp5_state); void mdp5_smp_destroy(struct mdp5_smp *smp); void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p); diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index bf5f8c39f34d..e792158676aa 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -220,16 +220,6 @@ int msm_atomic_commit(struct drm_device *dev, BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); - /* - * This is the point of no return - everything below never fails except - * when the hw goes bonghits. Which means we can commit the new state on - * the software side now. - * - * swap driver private state while still holding state_lock - */ - if (to_kms_state(state)->state) - priv->kms->funcs->swap_state(priv->kms, state); - /* * Everything below can be run asynchronously without the need to grab * any modeset locks at all under one conditions: It must be guaranteed @@ -262,30 +252,3 @@ int msm_atomic_commit(struct drm_device *dev, drm_atomic_helper_cleanup_planes(dev, state); return ret; } - -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev) -{ - struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL); - - if (!state || drm_atomic_state_init(dev, &state->base) < 0) { - kfree(state); - return NULL; - } - - return &state->base; -} - -void msm_atomic_state_clear(struct drm_atomic_state *s) -{ - struct msm_kms_state *state = to_kms_state(s); - drm_atomic_state_default_clear(&state->base); - kfree(state->state); - state->state = NULL; -} - -void msm_atomic_state_free(struct drm_atomic_state *state) -{ - kfree(to_kms_state(state)->state); - drm_atomic_state_default_release(state); - kfree(state); -} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 30cd514d8f7c..7e8f640062ef 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -42,11 +42,79 @@ static const struct drm_mode_config_funcs mode_config_funcs = { .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, .atomic_commit = msm_atomic_commit, - .atomic_state_alloc = msm_atomic_state_alloc, - .atomic_state_clear = msm_atomic_state_clear, - .atomic_state_free = msm_atomic_state_free, }; +static inline +struct msm_kms *msm_kms_from_obj(struct drm_private_obj *obj) +{ + return container_of(obj, struct msm_kms, base); +} + +static inline +struct msm_kms_state *msm_kms_state_from_priv(struct drm_private_state *priv) +{ + return container_of(priv, struct msm_kms_state, base); +} + + +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state) +{ + struct msm_drm_private *priv = state->dev->dev_private; + struct drm_private_obj *obj = &priv->kms->base; + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(state, obj); + if (IS_ERR_OR_NULL(priv_state)) + return (struct msm_kms_state *)state; /* casting ERR_PTR */ + + return msm_kms_state_from_priv(priv_state); +} + +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev) +{ + struct msm_drm_private *priv = dev->dev_private; + struct drm_private_obj *obj = &priv->kms->base; + struct drm_private_state *priv_state = obj->state; + + return msm_kms_state_from_priv(priv_state); +} + +static struct drm_private_state * +msm_kms_duplicate_state(struct drm_private_obj *obj) +{ + struct msm_kms *kms = msm_kms_from_obj(obj); + struct msm_kms_state *state = msm_kms_state_from_priv(obj->state); + struct msm_kms_state *new_state; + + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL); + if (!new_state) + return NULL; + + if (kms->funcs->duplicate_state) + new_state->state = kms->funcs->duplicate_state(state->state); + + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base); + + return &new_state->base; +} + +static void msm_kms_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *priv_state) +{ + struct msm_kms *kms = msm_kms_from_obj(obj); + struct msm_kms_state *state = msm_kms_state_from_priv(priv_state); + + if (kms->funcs->destroy_state) + kms->funcs->destroy_state(state->state); + kfree(state); +} + +static const struct drm_private_state_funcs kms_state_funcs = { + .atomic_duplicate_state = msm_kms_duplicate_state, + .atomic_destroy_state = msm_kms_destroy_state, +}; + + #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING static bool reglog = false; MODULE_PARM_DESC(reglog, "Enable register read/write logging"); @@ -245,6 +313,8 @@ static int msm_drm_uninit(struct device *dev) flush_workqueue(priv->atomic_wq); destroy_workqueue(priv->atomic_wq); + drm_atomic_private_obj_fini(&kms->base); + if (kms && kms->funcs) kms->funcs->destroy(kms); @@ -356,6 +426,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) struct drm_device *ddev; struct msm_drm_private *priv; struct msm_kms *kms; + struct msm_kms_state *initial_state; int ret; ddev = drm_dev_alloc(drv, dev); @@ -364,6 +435,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) return PTR_ERR(ddev); } + initial_state = kzalloc(sizeof(*initial_state), GFP_KERNEL); + if (!initial_state) + return -ENOMEM; + platform_set_drvdata(pdev, ddev); priv = kzalloc(sizeof(*priv), GFP_KERNEL); @@ -394,7 +469,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) drm_mode_config_init(ddev); /* Bind all our sub-components: */ - ret = component_bind_all(dev, ddev); + ret = component_bind_all(dev, initial_state); if (ret) { msm_mdss_destroy(ddev); kfree(priv); @@ -433,6 +508,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) goto fail; } + drm_atomic_private_obj_init(&kms->base, + &initial_state->base, + &kms_state_funcs); + if (kms) { ret = kms->funcs->hw_init(kms); if (ret) { diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 48ed5b9a8580..c5b8c989b859 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -162,9 +162,6 @@ struct msm_format { int msm_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock); -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev); -void msm_atomic_state_clear(struct drm_atomic_state *state); -void msm_atomic_state_free(struct drm_atomic_state *state); void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, struct sg_table *sgt); diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h index 17d5824417ad..24d09fcebf16 100644 --- a/drivers/gpu/drm/msm/msm_kms.h +++ b/drivers/gpu/drm/msm/msm_kms.h @@ -42,6 +42,8 @@ struct msm_kms_funcs { void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); /* swap global atomic state: */ void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state); + void *(*duplicate_state)(void *state); + void (*destroy_state)(void *state); /* modeset, bracketing atomic_commit(): */ void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state); void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state); @@ -68,6 +70,8 @@ struct msm_kms_funcs { }; struct msm_kms { + struct drm_private_obj base; + const struct msm_kms_funcs *funcs; /* irq number to be passed on to drm_irq_install */ @@ -78,16 +82,23 @@ struct msm_kms { }; /** - * Subclass of drm_atomic_state, to allow kms backend to have driver + * Subclass of drm_private_state, to allow kms backend to have driver * private global state. The kms backend can do whatever it wants - * with the ->state ptr. On ->atomic_state_clear() the ->state ptr - * is kfree'd and set back to NULL. + * with the ->state ptr. */ struct msm_kms_state { - struct drm_atomic_state base; + struct drm_private_state base; void *state; }; -#define to_kms_state(x) container_of(x, struct msm_kms_state, base) + +/** + * Extracts the msm_kms_state from either an atomic state, or the current + * device. One or the other might be desirable depending on whether you want the + * currently configured msm_kms_state (from_dev), or you would like the state + * which is about to be applied (from_atomic). + */ +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev); +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state); static inline void msm_kms_init(struct msm_kms *kms, const struct msm_kms_funcs *funcs) -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 1/6] drm/msm: Use drm_private_obj/state instead of subclassing @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Now that we have private state handled by the core, we can use those instead of rolling our own swap_state for private data. Originally posted here: https://patchwork.freedesktop.org/patch/211361/ Changes in v2: - Use state->state in disp duplicate_state callback (Jeykumar) Changes in v3: - Update comment describing msm_kms_state (Jeykumar) Changes in v4: - Rebased on msm-next - Don't always use private state from atomic state (Archit) - Renamed some of the state accessors - Tested on mdp5 db410c Changes in v5: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Cc: Archit Taneja <architt@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 77 ++++++++++--------- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 11 +-- drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++- drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 28 ++++--- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 19 +++-- drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +- drivers/gpu/drm/msm/msm_atomic.c | 37 --------- drivers/gpu/drm/msm/msm_drv.c | 87 +++++++++++++++++++++- drivers/gpu/drm/msm/msm_drv.h | 3 - drivers/gpu/drm/msm/msm_kms.h | 21 ++++-- 10 files changed, 183 insertions(+), 116 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 6d8e3a9a6fc0..366670043190 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -70,60 +70,62 @@ static int mdp5_hw_init(struct msm_kms *kms) return 0; } -struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s) +struct mdp5_state *mdp5_state_from_atomic(struct drm_atomic_state *state) { - struct msm_drm_private *priv = s->dev->dev_private; - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct msm_kms_state *state = to_kms_state(s); - struct mdp5_state *new_state; - int ret; + struct msm_kms_state *kms_state = msm_kms_state_from_atomic(state); - if (state->state) - return state->state; + if (IS_ERR_OR_NULL(kms_state)) + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ - ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx); - if (ret) - return ERR_PTR(ret); + return kms_state->state; +} - new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!new_state) - return ERR_PTR(-ENOMEM); +struct mdp5_state *mdp5_state_from_dev(struct drm_device *dev) +{ + struct msm_kms_state *kms_state = msm_kms_state_from_dev(dev); - /* Copy state: */ - new_state->hwpipe = mdp5_kms->state->hwpipe; - new_state->hwmixer = mdp5_kms->state->hwmixer; - if (mdp5_kms->smp) - new_state->smp = mdp5_kms->state->smp; + if (IS_ERR_OR_NULL(kms_state)) + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ - state->state = new_state; + return kms_state->state; +} + +static void *mdp5_duplicate_state(void *state) +{ + if (!state) + return kzalloc(sizeof(struct mdp5_state), GFP_KERNEL); - return new_state; + return kmemdup(state, sizeof(struct mdp5_state), GFP_KERNEL); } -static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_destroy_state(void *state) { - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); - swap(to_kms_state(state)->state, mdp5_kms->state); + struct mdp5_state *mdp_state = (struct mdp5_state *)state; + kfree(mdp_state); } -static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_prepare_commit(struct msm_kms *kms, + struct drm_atomic_state *old_state) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); struct device *dev = &mdp5_kms->pdev->dev; pm_runtime_get_sync(dev); if (mdp5_kms->smp) - mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_state->smp); } -static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state) +static void mdp5_complete_commit(struct msm_kms *kms, + struct drm_atomic_state *old_state) { struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); struct device *dev = &mdp5_kms->pdev->dev; if (mdp5_kms->smp) - mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp); + mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_state->smp); pm_runtime_put_sync(dev); } @@ -229,7 +231,8 @@ static const struct mdp_kms_funcs kms_funcs = { .irq = mdp5_irq, .enable_vblank = mdp5_enable_vblank, .disable_vblank = mdp5_disable_vblank, - .swap_state = mdp5_swap_state, + .duplicate_state = mdp5_duplicate_state, + .destroy_state = mdp5_destroy_state, .prepare_commit = mdp5_prepare_commit, .complete_commit = mdp5_complete_commit, .wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done, @@ -726,8 +729,6 @@ static void mdp5_destroy(struct platform_device *pdev) if (mdp5_kms->rpm_enabled) pm_runtime_disable(&pdev->dev); - - kfree(mdp5_kms->state); } static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, @@ -859,7 +860,8 @@ static int interface_init(struct mdp5_kms *mdp5_kms) return 0; } -static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) +static int mdp5_init(struct platform_device *pdev, struct drm_device *dev, + struct msm_kms_state *initial_state) { struct msm_drm_private *priv = dev->dev_private; struct mdp5_kms *mdp5_kms; @@ -880,9 +882,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) mdp5_kms->dev = dev; mdp5_kms->pdev = pdev; - drm_modeset_lock_init(&mdp5_kms->state_lock); - mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); - if (!mdp5_kms->state) { + initial_state->state = mdp5_duplicate_state(NULL); + if (!initial_state->state) { ret = -ENOMEM; goto fail; } @@ -940,7 +941,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) * this section initializes the SMP: */ if (mdp5_kms->caps & MDP_CAP_SMP) { - mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp); + mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp, + initial_state->state); if (IS_ERR(mdp5_kms->smp)) { ret = PTR_ERR(mdp5_kms->smp); mdp5_kms->smp = NULL; @@ -980,10 +982,11 @@ static int mdp5_bind(struct device *dev, struct device *master, void *data) { struct drm_device *ddev = dev_get_drvdata(master); struct platform_device *pdev = to_platform_device(dev); + struct msm_kms_state *initial_state = (struct msm_kms_state *)data; DBG(""); - return mdp5_init(pdev, ddev); + return mdp5_init(pdev, ddev, initial_state); } static void mdp5_unbind(struct device *dev, struct device *master, diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h index 425a03d213e5..e23117b82aad 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h @@ -28,8 +28,6 @@ #include "mdp5_ctl.h" #include "mdp5_smp.h" -struct mdp5_state; - struct mdp5_kms { struct mdp_kms base; @@ -49,12 +47,6 @@ struct mdp5_kms { struct mdp5_cfg_handler *cfg; uint32_t caps; /* MDP capabilities (MDP_CAP_XXX bits) */ - /** - * Global atomic state. Do not access directly, use mdp5_get_state() - */ - struct mdp5_state *state; - struct drm_modeset_lock state_lock; - struct mdp5_smp *smp; struct mdp5_ctl_manager *ctlm; @@ -93,7 +85,8 @@ struct mdp5_state { }; struct mdp5_state *__must_check -mdp5_get_state(struct drm_atomic_state *s); +mdp5_state_from_atomic(struct drm_atomic_state *s); +struct mdp5_state *__must_check mdp5_state_from_dev(struct drm_device *dev); /* Atomic plane state. Subclasses the base drm_plane_state in order to * track assigned hwpipe and hw specific state. diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c index 8a00991f03c7..fdd5e12a9d56 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c @@ -52,10 +52,11 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); + struct mdp5_state *state; struct mdp5_hw_mixer_state *new_state; int i; + state = mdp5_state_from_atomic(s); if (IS_ERR(state)) return PTR_ERR(state); @@ -129,12 +130,17 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) { - struct mdp5_state *state = mdp5_get_state(s); - struct mdp5_hw_mixer_state *new_state = &state->hwmixer; + struct mdp5_state *state; + struct mdp5_hw_mixer_state *new_state; if (!mixer) return; + state = mdp5_state_from_atomic(s); + if (IS_ERR(state)) + return; + + new_state = &state->hwmixer; if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) return; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c index ff52c49095f9..dc66ab9fdd50 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c @@ -24,17 +24,20 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state; + struct mdp5_state *old_mdp5_state, *new_mdp5_state; struct mdp5_hw_pipe_state *old_state, *new_state; int i, j; - state = mdp5_get_state(s); - if (IS_ERR(state)) - return PTR_ERR(state); + new_mdp5_state = mdp5_state_from_atomic(s); + if (IS_ERR(new_mdp5_state)) + return PTR_ERR(new_mdp5_state); - /* grab old_state after mdp5_get_state(), since now we hold lock: */ - old_state = &mdp5_kms->state->hwpipe; - new_state = &state->hwpipe; + old_mdp5_state = mdp5_state_from_dev(s->dev); + if (IS_ERR(old_mdp5_state)) + return PTR_ERR(old_mdp5_state); + + old_state = &old_mdp5_state->hwpipe; + new_state = &new_mdp5_state->hwpipe; for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i]; @@ -107,7 +110,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, WARN_ON(r_hwpipe); DBG("%s: alloc SMP blocks", (*hwpipe)->name); - ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp, + ret = mdp5_smp_assign(mdp5_kms->smp, &new_mdp5_state->smp, (*hwpipe)->pipe, blkcfg); if (ret) return -ENOMEM; @@ -132,12 +135,17 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe) { struct msm_drm_private *priv = s->dev->dev_private; struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); - struct mdp5_state *state = mdp5_get_state(s); - struct mdp5_hw_pipe_state *new_state = &state->hwpipe; + struct mdp5_state *state; + struct mdp5_hw_pipe_state *new_state; if (!hwpipe) return; + state = mdp5_state_from_atomic(s); + if (IS_ERR(state)) + return; + + new_state = &state->hwpipe; if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx])) return; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c index ae4983d9d0a5..918e4123e781 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c @@ -338,6 +338,7 @@ void mdp5_smp_complete_commit(struct mdp5_smp *smp, struct mdp5_smp_state *state void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) { struct mdp5_kms *mdp5_kms = get_kms(smp); + struct mdp5_state *mdp5_state; struct mdp5_hw_pipe_state *hwpstate; struct mdp5_smp_state *state; int total = 0, i, j; @@ -346,11 +347,12 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) drm_printf(p, "----\t-----\t-----\n"); if (drm_can_sleep()) - drm_modeset_lock(&mdp5_kms->state_lock, NULL); + drm_modeset_lock_all(mdp5_kms->dev); - /* grab these *after* we hold the state_lock */ - hwpstate = &mdp5_kms->state->hwpipe; - state = &mdp5_kms->state->smp; + /* grab these *after* we hold the modeset_lock */ + mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); + hwpstate = &mdp5_state->hwpipe; + state = &mdp5_state->smp; for (i = 0; i < mdp5_kms->num_hwpipes; i++) { struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i]; @@ -374,7 +376,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) bitmap_weight(state->state, smp->blk_cnt)); if (drm_can_sleep()) - drm_modeset_unlock(&mdp5_kms->state_lock); + drm_modeset_unlock_all(mdp5_kms->dev); } void mdp5_smp_destroy(struct mdp5_smp *smp) @@ -382,12 +384,15 @@ void mdp5_smp_destroy(struct mdp5_smp *smp) kfree(smp); } -struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg) +struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, + const struct mdp5_smp_block *cfg, + struct mdp5_state *mdp5_state) { - struct mdp5_smp_state *state = &mdp5_kms->state->smp; + struct mdp5_smp_state *state; struct mdp5_smp *smp = NULL; int ret; + state = &mdp5_state->smp; smp = kzalloc(sizeof(*smp), GFP_KERNEL); if (unlikely(!smp)) { ret = -ENOMEM; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h index b41d0448fbe8..1bfa22b63a2d 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h @@ -69,6 +69,7 @@ struct mdp5_smp_state { }; struct mdp5_kms; +struct mdp5_state; struct mdp5_smp; /* @@ -78,7 +79,8 @@ struct mdp5_smp; */ struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, - const struct mdp5_smp_block *cfg); + const struct mdp5_smp_block *cfg, + struct mdp5_state *mdp5_state); void mdp5_smp_destroy(struct mdp5_smp *smp); void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p); diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index bf5f8c39f34d..e792158676aa 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -220,16 +220,6 @@ int msm_atomic_commit(struct drm_device *dev, BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); - /* - * This is the point of no return - everything below never fails except - * when the hw goes bonghits. Which means we can commit the new state on - * the software side now. - * - * swap driver private state while still holding state_lock - */ - if (to_kms_state(state)->state) - priv->kms->funcs->swap_state(priv->kms, state); - /* * Everything below can be run asynchronously without the need to grab * any modeset locks at all under one conditions: It must be guaranteed @@ -262,30 +252,3 @@ int msm_atomic_commit(struct drm_device *dev, drm_atomic_helper_cleanup_planes(dev, state); return ret; } - -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev) -{ - struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL); - - if (!state || drm_atomic_state_init(dev, &state->base) < 0) { - kfree(state); - return NULL; - } - - return &state->base; -} - -void msm_atomic_state_clear(struct drm_atomic_state *s) -{ - struct msm_kms_state *state = to_kms_state(s); - drm_atomic_state_default_clear(&state->base); - kfree(state->state); - state->state = NULL; -} - -void msm_atomic_state_free(struct drm_atomic_state *state) -{ - kfree(to_kms_state(state)->state); - drm_atomic_state_default_release(state); - kfree(state); -} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 30cd514d8f7c..7e8f640062ef 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -42,11 +42,79 @@ static const struct drm_mode_config_funcs mode_config_funcs = { .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, .atomic_commit = msm_atomic_commit, - .atomic_state_alloc = msm_atomic_state_alloc, - .atomic_state_clear = msm_atomic_state_clear, - .atomic_state_free = msm_atomic_state_free, }; +static inline +struct msm_kms *msm_kms_from_obj(struct drm_private_obj *obj) +{ + return container_of(obj, struct msm_kms, base); +} + +static inline +struct msm_kms_state *msm_kms_state_from_priv(struct drm_private_state *priv) +{ + return container_of(priv, struct msm_kms_state, base); +} + + +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state) +{ + struct msm_drm_private *priv = state->dev->dev_private; + struct drm_private_obj *obj = &priv->kms->base; + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(state, obj); + if (IS_ERR_OR_NULL(priv_state)) + return (struct msm_kms_state *)state; /* casting ERR_PTR */ + + return msm_kms_state_from_priv(priv_state); +} + +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev) +{ + struct msm_drm_private *priv = dev->dev_private; + struct drm_private_obj *obj = &priv->kms->base; + struct drm_private_state *priv_state = obj->state; + + return msm_kms_state_from_priv(priv_state); +} + +static struct drm_private_state * +msm_kms_duplicate_state(struct drm_private_obj *obj) +{ + struct msm_kms *kms = msm_kms_from_obj(obj); + struct msm_kms_state *state = msm_kms_state_from_priv(obj->state); + struct msm_kms_state *new_state; + + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL); + if (!new_state) + return NULL; + + if (kms->funcs->duplicate_state) + new_state->state = kms->funcs->duplicate_state(state->state); + + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base); + + return &new_state->base; +} + +static void msm_kms_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *priv_state) +{ + struct msm_kms *kms = msm_kms_from_obj(obj); + struct msm_kms_state *state = msm_kms_state_from_priv(priv_state); + + if (kms->funcs->destroy_state) + kms->funcs->destroy_state(state->state); + kfree(state); +} + +static const struct drm_private_state_funcs kms_state_funcs = { + .atomic_duplicate_state = msm_kms_duplicate_state, + .atomic_destroy_state = msm_kms_destroy_state, +}; + + #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING static bool reglog = false; MODULE_PARM_DESC(reglog, "Enable register read/write logging"); @@ -245,6 +313,8 @@ static int msm_drm_uninit(struct device *dev) flush_workqueue(priv->atomic_wq); destroy_workqueue(priv->atomic_wq); + drm_atomic_private_obj_fini(&kms->base); + if (kms && kms->funcs) kms->funcs->destroy(kms); @@ -356,6 +426,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) struct drm_device *ddev; struct msm_drm_private *priv; struct msm_kms *kms; + struct msm_kms_state *initial_state; int ret; ddev = drm_dev_alloc(drv, dev); @@ -364,6 +435,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) return PTR_ERR(ddev); } + initial_state = kzalloc(sizeof(*initial_state), GFP_KERNEL); + if (!initial_state) + return -ENOMEM; + platform_set_drvdata(pdev, ddev); priv = kzalloc(sizeof(*priv), GFP_KERNEL); @@ -394,7 +469,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) drm_mode_config_init(ddev); /* Bind all our sub-components: */ - ret = component_bind_all(dev, ddev); + ret = component_bind_all(dev, initial_state); if (ret) { msm_mdss_destroy(ddev); kfree(priv); @@ -433,6 +508,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) goto fail; } + drm_atomic_private_obj_init(&kms->base, + &initial_state->base, + &kms_state_funcs); + if (kms) { ret = kms->funcs->hw_init(kms); if (ret) { diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 48ed5b9a8580..c5b8c989b859 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -162,9 +162,6 @@ struct msm_format { int msm_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock); -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev); -void msm_atomic_state_clear(struct drm_atomic_state *state); -void msm_atomic_state_free(struct drm_atomic_state *state); void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, struct sg_table *sgt); diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h index 17d5824417ad..24d09fcebf16 100644 --- a/drivers/gpu/drm/msm/msm_kms.h +++ b/drivers/gpu/drm/msm/msm_kms.h @@ -42,6 +42,8 @@ struct msm_kms_funcs { void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); /* swap global atomic state: */ void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state); + void *(*duplicate_state)(void *state); + void (*destroy_state)(void *state); /* modeset, bracketing atomic_commit(): */ void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state); void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state); @@ -68,6 +70,8 @@ struct msm_kms_funcs { }; struct msm_kms { + struct drm_private_obj base; + const struct msm_kms_funcs *funcs; /* irq number to be passed on to drm_irq_install */ @@ -78,16 +82,23 @@ struct msm_kms { }; /** - * Subclass of drm_atomic_state, to allow kms backend to have driver + * Subclass of drm_private_state, to allow kms backend to have driver * private global state. The kms backend can do whatever it wants - * with the ->state ptr. On ->atomic_state_clear() the ->state ptr - * is kfree'd and set back to NULL. + * with the ->state ptr. */ struct msm_kms_state { - struct drm_atomic_state base; + struct drm_private_state base; void *state; }; -#define to_kms_state(x) container_of(x, struct msm_kms_state, base) + +/** + * Extracts the msm_kms_state from either an atomic state, or the current + * device. One or the other might be desirable depending on whether you want the + * currently configured msm_kms_state (from_dev), or you would like the state + * which is about to be applied (from_atomic). + */ +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev); +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state); static inline void msm_kms_init(struct msm_kms *kms, const struct msm_kms_funcs *funcs) -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <20180328190657.218661-2-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* Re: [PATCH v2 1/6] drm/msm: Use drm_private_obj/state instead of subclassing 2018-03-28 19:06 ` Sean Paul @ 2018-04-02 5:01 ` Archit Taneja -1 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:01 UTC (permalink / raw) To: Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: jsanka-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, hoegsberg-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Now that we have private state handled by the core, we can use those > instead of rolling our own swap_state for private data. > > Originally posted here: https://patchwork.freedesktop.org/patch/211361/ > > Changes in v2: > - Use state->state in disp duplicate_state callback (Jeykumar) > Changes in v3: > - Update comment describing msm_kms_state (Jeykumar) > Changes in v4: > - Rebased on msm-next > - Don't always use private state from atomic state (Archit) > - Renamed some of the state accessors > - Tested on mdp5 db410c > Changes in v5: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 77 ++++++++++--------- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 11 +-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++- > drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 28 ++++--- > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 19 +++-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +- > drivers/gpu/drm/msm/msm_atomic.c | 37 --------- > drivers/gpu/drm/msm/msm_drv.c | 87 +++++++++++++++++++++- > drivers/gpu/drm/msm/msm_drv.h | 3 - > drivers/gpu/drm/msm/msm_kms.h | 21 ++++-- > 10 files changed, 183 insertions(+), 116 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 6d8e3a9a6fc0..366670043190 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -70,60 +70,62 @@ static int mdp5_hw_init(struct msm_kms *kms) > return 0; > } > > -struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s) > +struct mdp5_state *mdp5_state_from_atomic(struct drm_atomic_state *state) > { > - struct msm_drm_private *priv = s->dev->dev_private; > - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct msm_kms_state *state = to_kms_state(s); > - struct mdp5_state *new_state; > - int ret; > + struct msm_kms_state *kms_state = msm_kms_state_from_atomic(state); > > - if (state->state) > - return state->state; > + if (IS_ERR_OR_NULL(kms_state)) > + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ > > - ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx); > - if (ret) > - return ERR_PTR(ret); > + return kms_state->state; > +} > > - new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); > - if (!new_state) > - return ERR_PTR(-ENOMEM); > +struct mdp5_state *mdp5_state_from_dev(struct drm_device *dev) > +{ > + struct msm_kms_state *kms_state = msm_kms_state_from_dev(dev); > > - /* Copy state: */ > - new_state->hwpipe = mdp5_kms->state->hwpipe; > - new_state->hwmixer = mdp5_kms->state->hwmixer; > - if (mdp5_kms->smp) > - new_state->smp = mdp5_kms->state->smp; > + if (IS_ERR_OR_NULL(kms_state)) > + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ > > - state->state = new_state; > + return kms_state->state; > +} > + > +static void *mdp5_duplicate_state(void *state) > +{ > + if (!state) > + return kzalloc(sizeof(struct mdp5_state), GFP_KERNEL); > > - return new_state; > + return kmemdup(state, sizeof(struct mdp5_state), GFP_KERNEL); > } > > -static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_destroy_state(void *state) > { > - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > - swap(to_kms_state(state)->state, mdp5_kms->state); > + struct mdp5_state *mdp_state = (struct mdp5_state *)state; > + kfree(mdp_state); > } > > -static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_prepare_commit(struct msm_kms *kms, > + struct drm_atomic_state *old_state) > { > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > struct device *dev = &mdp5_kms->pdev->dev; > > pm_runtime_get_sync(dev); > > if (mdp5_kms->smp) > - mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp); > + mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_state->smp); > } > > -static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_complete_commit(struct msm_kms *kms, > + struct drm_atomic_state *old_state) > { > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > struct device *dev = &mdp5_kms->pdev->dev; > > if (mdp5_kms->smp) > - mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp); > + mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_state->smp); > > pm_runtime_put_sync(dev); > } > @@ -229,7 +231,8 @@ static const struct mdp_kms_funcs kms_funcs = { > .irq = mdp5_irq, > .enable_vblank = mdp5_enable_vblank, > .disable_vblank = mdp5_disable_vblank, > - .swap_state = mdp5_swap_state, > + .duplicate_state = mdp5_duplicate_state, > + .destroy_state = mdp5_destroy_state, > .prepare_commit = mdp5_prepare_commit, > .complete_commit = mdp5_complete_commit, > .wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done, > @@ -726,8 +729,6 @@ static void mdp5_destroy(struct platform_device *pdev) > > if (mdp5_kms->rpm_enabled) > pm_runtime_disable(&pdev->dev); > - > - kfree(mdp5_kms->state); > } > > static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, > @@ -859,7 +860,8 @@ static int interface_init(struct mdp5_kms *mdp5_kms) > return 0; > } > > -static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > +static int mdp5_init(struct platform_device *pdev, struct drm_device *dev, > + struct msm_kms_state *initial_state) > { > struct msm_drm_private *priv = dev->dev_private; > struct mdp5_kms *mdp5_kms; > @@ -880,9 +882,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > mdp5_kms->dev = dev; > mdp5_kms->pdev = pdev; > > - drm_modeset_lock_init(&mdp5_kms->state_lock); > - mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); > - if (!mdp5_kms->state) { > + initial_state->state = mdp5_duplicate_state(NULL); > + if (!initial_state->state) { > ret = -ENOMEM; > goto fail; > } > @@ -940,7 +941,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > * this section initializes the SMP: > */ > if (mdp5_kms->caps & MDP_CAP_SMP) { > - mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp); > + mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp, > + initial_state->state); > if (IS_ERR(mdp5_kms->smp)) { > ret = PTR_ERR(mdp5_kms->smp); > mdp5_kms->smp = NULL; > @@ -980,10 +982,11 @@ static int mdp5_bind(struct device *dev, struct device *master, void *data) > { > struct drm_device *ddev = dev_get_drvdata(master); > struct platform_device *pdev = to_platform_device(dev); > + struct msm_kms_state *initial_state = (struct msm_kms_state *)data; > > DBG(""); > > - return mdp5_init(pdev, ddev); > + return mdp5_init(pdev, ddev, initial_state); > } > > static void mdp5_unbind(struct device *dev, struct device *master, > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > index 425a03d213e5..e23117b82aad 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > @@ -28,8 +28,6 @@ > #include "mdp5_ctl.h" > #include "mdp5_smp.h" > > -struct mdp5_state; > - > struct mdp5_kms { > struct mdp_kms base; > > @@ -49,12 +47,6 @@ struct mdp5_kms { > struct mdp5_cfg_handler *cfg; > uint32_t caps; /* MDP capabilities (MDP_CAP_XXX bits) */ > > - /** > - * Global atomic state. Do not access directly, use mdp5_get_state() > - */ > - struct mdp5_state *state; > - struct drm_modeset_lock state_lock; > - > struct mdp5_smp *smp; > struct mdp5_ctl_manager *ctlm; > > @@ -93,7 +85,8 @@ struct mdp5_state { > }; > > struct mdp5_state *__must_check > -mdp5_get_state(struct drm_atomic_state *s); > +mdp5_state_from_atomic(struct drm_atomic_state *s); > +struct mdp5_state *__must_check mdp5_state_from_dev(struct drm_device *dev); > > /* Atomic plane state. Subclasses the base drm_plane_state in order to > * track assigned hwpipe and hw specific state. > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > index 8a00991f03c7..fdd5e12a9d56 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > @@ -52,10 +52,11 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state = mdp5_get_state(s); > + struct mdp5_state *state; > struct mdp5_hw_mixer_state *new_state; > int i; > > + state = mdp5_state_from_atomic(s); > if (IS_ERR(state)) > return PTR_ERR(state); > > @@ -129,12 +130,17 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > > void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > { > - struct mdp5_state *state = mdp5_get_state(s); > - struct mdp5_hw_mixer_state *new_state = &state->hwmixer; > + struct mdp5_state *state; > + struct mdp5_hw_mixer_state *new_state; > > if (!mixer) > return; > > + state = mdp5_state_from_atomic(s); > + if (IS_ERR(state)) > + return; > + > + new_state = &state->hwmixer; > if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) > return; > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > index ff52c49095f9..dc66ab9fdd50 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > @@ -24,17 +24,20 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state; > + struct mdp5_state *old_mdp5_state, *new_mdp5_state; > struct mdp5_hw_pipe_state *old_state, *new_state; > int i, j; > > - state = mdp5_get_state(s); > - if (IS_ERR(state)) > - return PTR_ERR(state); > + new_mdp5_state = mdp5_state_from_atomic(s); > + if (IS_ERR(new_mdp5_state)) > + return PTR_ERR(new_mdp5_state); > > - /* grab old_state after mdp5_get_state(), since now we hold lock: */ > - old_state = &mdp5_kms->state->hwpipe; > - new_state = &state->hwpipe; > + old_mdp5_state = mdp5_state_from_dev(s->dev); > + if (IS_ERR(old_mdp5_state)) > + return PTR_ERR(old_mdp5_state); > + > + old_state = &old_mdp5_state->hwpipe; > + new_state = &new_mdp5_state->hwpipe; > > for (i = 0; i < mdp5_kms->num_hwpipes; i++) { > struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i]; > @@ -107,7 +110,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, > WARN_ON(r_hwpipe); > > DBG("%s: alloc SMP blocks", (*hwpipe)->name); > - ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp, > + ret = mdp5_smp_assign(mdp5_kms->smp, &new_mdp5_state->smp, > (*hwpipe)->pipe, blkcfg); > if (ret) > return -ENOMEM; > @@ -132,12 +135,17 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe) > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state = mdp5_get_state(s); > - struct mdp5_hw_pipe_state *new_state = &state->hwpipe; > + struct mdp5_state *state; > + struct mdp5_hw_pipe_state *new_state; > > if (!hwpipe) > return; > > + state = mdp5_state_from_atomic(s); > + if (IS_ERR(state)) > + return; > + > + new_state = &state->hwpipe; > if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx])) > return; > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > index ae4983d9d0a5..918e4123e781 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > @@ -338,6 +338,7 @@ void mdp5_smp_complete_commit(struct mdp5_smp *smp, struct mdp5_smp_state *state > void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > { > struct mdp5_kms *mdp5_kms = get_kms(smp); > + struct mdp5_state *mdp5_state; > struct mdp5_hw_pipe_state *hwpstate; > struct mdp5_smp_state *state; > int total = 0, i, j; > @@ -346,11 +347,12 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > drm_printf(p, "----\t-----\t-----\n"); > > if (drm_can_sleep()) > - drm_modeset_lock(&mdp5_kms->state_lock, NULL); > + drm_modeset_lock_all(mdp5_kms->dev); > > - /* grab these *after* we hold the state_lock */ > - hwpstate = &mdp5_kms->state->hwpipe; > - state = &mdp5_kms->state->smp; > + /* grab these *after* we hold the modeset_lock */ > + mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > + hwpstate = &mdp5_state->hwpipe; > + state = &mdp5_state->smp; > > for (i = 0; i < mdp5_kms->num_hwpipes; i++) { > struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i]; > @@ -374,7 +376,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > bitmap_weight(state->state, smp->blk_cnt)); > > if (drm_can_sleep()) > - drm_modeset_unlock(&mdp5_kms->state_lock); > + drm_modeset_unlock_all(mdp5_kms->dev); > } > > void mdp5_smp_destroy(struct mdp5_smp *smp) > @@ -382,12 +384,15 @@ void mdp5_smp_destroy(struct mdp5_smp *smp) > kfree(smp); > } > > -struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg) > +struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, > + const struct mdp5_smp_block *cfg, > + struct mdp5_state *mdp5_state) > { > - struct mdp5_smp_state *state = &mdp5_kms->state->smp; > + struct mdp5_smp_state *state; > struct mdp5_smp *smp = NULL; > int ret; > > + state = &mdp5_state->smp; > smp = kzalloc(sizeof(*smp), GFP_KERNEL); > if (unlikely(!smp)) { > ret = -ENOMEM; > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > index b41d0448fbe8..1bfa22b63a2d 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > @@ -69,6 +69,7 @@ struct mdp5_smp_state { > }; > > struct mdp5_kms; > +struct mdp5_state; > struct mdp5_smp; > > /* > @@ -78,7 +79,8 @@ struct mdp5_smp; > */ > > struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, > - const struct mdp5_smp_block *cfg); > + const struct mdp5_smp_block *cfg, > + struct mdp5_state *mdp5_state); > void mdp5_smp_destroy(struct mdp5_smp *smp); > > void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p); > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index bf5f8c39f34d..e792158676aa 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -220,16 +220,6 @@ int msm_atomic_commit(struct drm_device *dev, > > BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); > > - /* > - * This is the point of no return - everything below never fails except > - * when the hw goes bonghits. Which means we can commit the new state on > - * the software side now. > - * > - * swap driver private state while still holding state_lock > - */ > - if (to_kms_state(state)->state) > - priv->kms->funcs->swap_state(priv->kms, state); > - > /* > * Everything below can be run asynchronously without the need to grab > * any modeset locks at all under one conditions: It must be guaranteed > @@ -262,30 +252,3 @@ int msm_atomic_commit(struct drm_device *dev, > drm_atomic_helper_cleanup_planes(dev, state); > return ret; > } > - > -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev) > -{ > - struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > - > - if (!state || drm_atomic_state_init(dev, &state->base) < 0) { > - kfree(state); > - return NULL; > - } > - > - return &state->base; > -} > - > -void msm_atomic_state_clear(struct drm_atomic_state *s) > -{ > - struct msm_kms_state *state = to_kms_state(s); > - drm_atomic_state_default_clear(&state->base); > - kfree(state->state); > - state->state = NULL; > -} > - > -void msm_atomic_state_free(struct drm_atomic_state *state) > -{ > - kfree(to_kms_state(state)->state); > - drm_atomic_state_default_release(state); > - kfree(state); > -} > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 30cd514d8f7c..7e8f640062ef 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -42,11 +42,79 @@ static const struct drm_mode_config_funcs mode_config_funcs = { > .output_poll_changed = drm_fb_helper_output_poll_changed, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = msm_atomic_commit, > - .atomic_state_alloc = msm_atomic_state_alloc, > - .atomic_state_clear = msm_atomic_state_clear, > - .atomic_state_free = msm_atomic_state_free, > }; > > +static inline > +struct msm_kms *msm_kms_from_obj(struct drm_private_obj *obj) > +{ > + return container_of(obj, struct msm_kms, base); > +} > + > +static inline > +struct msm_kms_state *msm_kms_state_from_priv(struct drm_private_state *priv) > +{ > + return container_of(priv, struct msm_kms_state, base); > +} > + > + > +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state) > +{ > + struct msm_drm_private *priv = state->dev->dev_private; > + struct drm_private_obj *obj = &priv->kms->base; > + struct drm_private_state *priv_state; > + > + priv_state = drm_atomic_get_private_obj_state(state, obj); > + if (IS_ERR_OR_NULL(priv_state)) > + return (struct msm_kms_state *)state; /* casting ERR_PTR */ > + > + return msm_kms_state_from_priv(priv_state); > +} > + > +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev) > +{ > + struct msm_drm_private *priv = dev->dev_private; > + struct drm_private_obj *obj = &priv->kms->base; > + struct drm_private_state *priv_state = obj->state; > + > + return msm_kms_state_from_priv(priv_state); > +} > + > +static struct drm_private_state * > +msm_kms_duplicate_state(struct drm_private_obj *obj) > +{ > + struct msm_kms *kms = msm_kms_from_obj(obj); > + struct msm_kms_state *state = msm_kms_state_from_priv(obj->state); > + struct msm_kms_state *new_state; > + > + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL); > + if (!new_state) > + return NULL; > + > + if (kms->funcs->duplicate_state) > + new_state->state = kms->funcs->duplicate_state(state->state); > + > + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base); > + > + return &new_state->base; > +} > + > +static void msm_kms_destroy_state(struct drm_private_obj *obj, > + struct drm_private_state *priv_state) > +{ > + struct msm_kms *kms = msm_kms_from_obj(obj); > + struct msm_kms_state *state = msm_kms_state_from_priv(priv_state); > + > + if (kms->funcs->destroy_state) > + kms->funcs->destroy_state(state->state); > + kfree(state); > +} > + > +static const struct drm_private_state_funcs kms_state_funcs = { > + .atomic_duplicate_state = msm_kms_duplicate_state, > + .atomic_destroy_state = msm_kms_destroy_state, > +}; > + > + Minor nit: Two blank lines here^ > #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING > static bool reglog = false; > MODULE_PARM_DESC(reglog, "Enable register read/write logging"); > @@ -245,6 +313,8 @@ static int msm_drm_uninit(struct device *dev) > flush_workqueue(priv->atomic_wq); > destroy_workqueue(priv->atomic_wq); > > + drm_atomic_private_obj_fini(&kms->base); > + > if (kms && kms->funcs) > kms->funcs->destroy(kms); > > @@ -356,6 +426,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > struct drm_device *ddev; > struct msm_drm_private *priv; > struct msm_kms *kms; > + struct msm_kms_state *initial_state; > int ret; > > ddev = drm_dev_alloc(drv, dev); > @@ -364,6 +435,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > return PTR_ERR(ddev); > } > > + initial_state = kzalloc(sizeof(*initial_state), GFP_KERNEL); > + if (!initial_state) > + return -ENOMEM; > + > platform_set_drvdata(pdev, ddev); > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > @@ -394,7 +469,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > drm_mode_config_init(ddev); > > /* Bind all our sub-components: */ > - ret = component_bind_all(dev, ddev); > + ret = component_bind_all(dev, initial_state); Only the core kms component (mdp4/mdp5/dpu) will use initial_state, and the encoders drivers (DSI/HDMI) would discard it, right? I guess it's still better than passing ddev, which can be derived from the component master. Reviewed-by: Archit Taneja <architt@codeaurora.org> > if (ret) { > msm_mdss_destroy(ddev); > kfree(priv); > @@ -433,6 +508,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > goto fail; > } > > + drm_atomic_private_obj_init(&kms->base, > + &initial_state->base, > + &kms_state_funcs); > + > if (kms) { > ret = kms->funcs->hw_init(kms); > if (ret) { > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 48ed5b9a8580..c5b8c989b859 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -162,9 +162,6 @@ struct msm_format { > > int msm_atomic_commit(struct drm_device *dev, > struct drm_atomic_state *state, bool nonblock); > -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev); > -void msm_atomic_state_clear(struct drm_atomic_state *state); > -void msm_atomic_state_free(struct drm_atomic_state *state); > > void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, > struct msm_gem_vma *vma, struct sg_table *sgt); > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h > index 17d5824417ad..24d09fcebf16 100644 > --- a/drivers/gpu/drm/msm/msm_kms.h > +++ b/drivers/gpu/drm/msm/msm_kms.h > @@ -42,6 +42,8 @@ struct msm_kms_funcs { > void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); > /* swap global atomic state: */ > void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state); > + void *(*duplicate_state)(void *state); > + void (*destroy_state)(void *state); > /* modeset, bracketing atomic_commit(): */ > void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state); > void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state); > @@ -68,6 +70,8 @@ struct msm_kms_funcs { > }; > > struct msm_kms { > + struct drm_private_obj base; > + > const struct msm_kms_funcs *funcs; > > /* irq number to be passed on to drm_irq_install */ > @@ -78,16 +82,23 @@ struct msm_kms { > }; > > /** > - * Subclass of drm_atomic_state, to allow kms backend to have driver > + * Subclass of drm_private_state, to allow kms backend to have driver > * private global state. The kms backend can do whatever it wants > - * with the ->state ptr. On ->atomic_state_clear() the ->state ptr > - * is kfree'd and set back to NULL. > + * with the ->state ptr. > */ > struct msm_kms_state { > - struct drm_atomic_state base; > + struct drm_private_state base; > void *state; > }; > -#define to_kms_state(x) container_of(x, struct msm_kms_state, base) > + > +/** > + * Extracts the msm_kms_state from either an atomic state, or the current > + * device. One or the other might be desirable depending on whether you want the > + * currently configured msm_kms_state (from_dev), or you would like the state > + * which is about to be applied (from_atomic). > + */ > +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev); > +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state); > > static inline void msm_kms_init(struct msm_kms *kms, > const struct msm_kms_funcs *funcs) > _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] drm/msm: Use drm_private_obj/state instead of subclassing @ 2018-04-02 5:01 ` Archit Taneja 0 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:01 UTC (permalink / raw) To: Sean Paul, freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, linux-kernel On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Now that we have private state handled by the core, we can use those > instead of rolling our own swap_state for private data. > > Originally posted here: https://patchwork.freedesktop.org/patch/211361/ > > Changes in v2: > - Use state->state in disp duplicate_state callback (Jeykumar) > Changes in v3: > - Update comment describing msm_kms_state (Jeykumar) > Changes in v4: > - Rebased on msm-next > - Don't always use private state from atomic state (Archit) > - Renamed some of the state accessors > - Tested on mdp5 db410c > Changes in v5: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 77 ++++++++++--------- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 11 +-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 12 ++- > drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 28 ++++--- > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 19 +++-- > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 4 +- > drivers/gpu/drm/msm/msm_atomic.c | 37 --------- > drivers/gpu/drm/msm/msm_drv.c | 87 +++++++++++++++++++++- > drivers/gpu/drm/msm/msm_drv.h | 3 - > drivers/gpu/drm/msm/msm_kms.h | 21 ++++-- > 10 files changed, 183 insertions(+), 116 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 6d8e3a9a6fc0..366670043190 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -70,60 +70,62 @@ static int mdp5_hw_init(struct msm_kms *kms) > return 0; > } > > -struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s) > +struct mdp5_state *mdp5_state_from_atomic(struct drm_atomic_state *state) > { > - struct msm_drm_private *priv = s->dev->dev_private; > - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct msm_kms_state *state = to_kms_state(s); > - struct mdp5_state *new_state; > - int ret; > + struct msm_kms_state *kms_state = msm_kms_state_from_atomic(state); > > - if (state->state) > - return state->state; > + if (IS_ERR_OR_NULL(kms_state)) > + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ > > - ret = drm_modeset_lock(&mdp5_kms->state_lock, s->acquire_ctx); > - if (ret) > - return ERR_PTR(ret); > + return kms_state->state; > +} > > - new_state = kmalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); > - if (!new_state) > - return ERR_PTR(-ENOMEM); > +struct mdp5_state *mdp5_state_from_dev(struct drm_device *dev) > +{ > + struct msm_kms_state *kms_state = msm_kms_state_from_dev(dev); > > - /* Copy state: */ > - new_state->hwpipe = mdp5_kms->state->hwpipe; > - new_state->hwmixer = mdp5_kms->state->hwmixer; > - if (mdp5_kms->smp) > - new_state->smp = mdp5_kms->state->smp; > + if (IS_ERR_OR_NULL(kms_state)) > + return (struct mdp5_state *)kms_state; /* casting ERR_PTR */ > > - state->state = new_state; > + return kms_state->state; > +} > + > +static void *mdp5_duplicate_state(void *state) > +{ > + if (!state) > + return kzalloc(sizeof(struct mdp5_state), GFP_KERNEL); > > - return new_state; > + return kmemdup(state, sizeof(struct mdp5_state), GFP_KERNEL); > } > > -static void mdp5_swap_state(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_destroy_state(void *state) > { > - struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > - swap(to_kms_state(state)->state, mdp5_kms->state); > + struct mdp5_state *mdp_state = (struct mdp5_state *)state; > + kfree(mdp_state); > } > > -static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_prepare_commit(struct msm_kms *kms, > + struct drm_atomic_state *old_state) > { > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > struct device *dev = &mdp5_kms->pdev->dev; > > pm_runtime_get_sync(dev); > > if (mdp5_kms->smp) > - mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_kms->state->smp); > + mdp5_smp_prepare_commit(mdp5_kms->smp, &mdp5_state->smp); > } > > -static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state) > +static void mdp5_complete_commit(struct msm_kms *kms, > + struct drm_atomic_state *old_state) > { > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms)); > + struct mdp5_state *mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > struct device *dev = &mdp5_kms->pdev->dev; > > if (mdp5_kms->smp) > - mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_kms->state->smp); > + mdp5_smp_complete_commit(mdp5_kms->smp, &mdp5_state->smp); > > pm_runtime_put_sync(dev); > } > @@ -229,7 +231,8 @@ static const struct mdp_kms_funcs kms_funcs = { > .irq = mdp5_irq, > .enable_vblank = mdp5_enable_vblank, > .disable_vblank = mdp5_disable_vblank, > - .swap_state = mdp5_swap_state, > + .duplicate_state = mdp5_duplicate_state, > + .destroy_state = mdp5_destroy_state, > .prepare_commit = mdp5_prepare_commit, > .complete_commit = mdp5_complete_commit, > .wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done, > @@ -726,8 +729,6 @@ static void mdp5_destroy(struct platform_device *pdev) > > if (mdp5_kms->rpm_enabled) > pm_runtime_disable(&pdev->dev); > - > - kfree(mdp5_kms->state); > } > > static int construct_pipes(struct mdp5_kms *mdp5_kms, int cnt, > @@ -859,7 +860,8 @@ static int interface_init(struct mdp5_kms *mdp5_kms) > return 0; > } > > -static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > +static int mdp5_init(struct platform_device *pdev, struct drm_device *dev, > + struct msm_kms_state *initial_state) > { > struct msm_drm_private *priv = dev->dev_private; > struct mdp5_kms *mdp5_kms; > @@ -880,9 +882,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > mdp5_kms->dev = dev; > mdp5_kms->pdev = pdev; > > - drm_modeset_lock_init(&mdp5_kms->state_lock); > - mdp5_kms->state = kzalloc(sizeof(*mdp5_kms->state), GFP_KERNEL); > - if (!mdp5_kms->state) { > + initial_state->state = mdp5_duplicate_state(NULL); > + if (!initial_state->state) { > ret = -ENOMEM; > goto fail; > } > @@ -940,7 +941,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > * this section initializes the SMP: > */ > if (mdp5_kms->caps & MDP_CAP_SMP) { > - mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp); > + mdp5_kms->smp = mdp5_smp_init(mdp5_kms, &config->hw->smp, > + initial_state->state); > if (IS_ERR(mdp5_kms->smp)) { > ret = PTR_ERR(mdp5_kms->smp); > mdp5_kms->smp = NULL; > @@ -980,10 +982,11 @@ static int mdp5_bind(struct device *dev, struct device *master, void *data) > { > struct drm_device *ddev = dev_get_drvdata(master); > struct platform_device *pdev = to_platform_device(dev); > + struct msm_kms_state *initial_state = (struct msm_kms_state *)data; > > DBG(""); > > - return mdp5_init(pdev, ddev); > + return mdp5_init(pdev, ddev, initial_state); > } > > static void mdp5_unbind(struct device *dev, struct device *master, > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > index 425a03d213e5..e23117b82aad 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h > @@ -28,8 +28,6 @@ > #include "mdp5_ctl.h" > #include "mdp5_smp.h" > > -struct mdp5_state; > - > struct mdp5_kms { > struct mdp_kms base; > > @@ -49,12 +47,6 @@ struct mdp5_kms { > struct mdp5_cfg_handler *cfg; > uint32_t caps; /* MDP capabilities (MDP_CAP_XXX bits) */ > > - /** > - * Global atomic state. Do not access directly, use mdp5_get_state() > - */ > - struct mdp5_state *state; > - struct drm_modeset_lock state_lock; > - > struct mdp5_smp *smp; > struct mdp5_ctl_manager *ctlm; > > @@ -93,7 +85,8 @@ struct mdp5_state { > }; > > struct mdp5_state *__must_check > -mdp5_get_state(struct drm_atomic_state *s); > +mdp5_state_from_atomic(struct drm_atomic_state *s); > +struct mdp5_state *__must_check mdp5_state_from_dev(struct drm_device *dev); > > /* Atomic plane state. Subclasses the base drm_plane_state in order to > * track assigned hwpipe and hw specific state. > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > index 8a00991f03c7..fdd5e12a9d56 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c > @@ -52,10 +52,11 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state = mdp5_get_state(s); > + struct mdp5_state *state; > struct mdp5_hw_mixer_state *new_state; > int i; > > + state = mdp5_state_from_atomic(s); > if (IS_ERR(state)) > return PTR_ERR(state); > > @@ -129,12 +130,17 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc, > > void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer) > { > - struct mdp5_state *state = mdp5_get_state(s); > - struct mdp5_hw_mixer_state *new_state = &state->hwmixer; > + struct mdp5_state *state; > + struct mdp5_hw_mixer_state *new_state; > > if (!mixer) > return; > > + state = mdp5_state_from_atomic(s); > + if (IS_ERR(state)) > + return; > + > + new_state = &state->hwmixer; > if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx])) > return; > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > index ff52c49095f9..dc66ab9fdd50 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c > @@ -24,17 +24,20 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state; > + struct mdp5_state *old_mdp5_state, *new_mdp5_state; > struct mdp5_hw_pipe_state *old_state, *new_state; > int i, j; > > - state = mdp5_get_state(s); > - if (IS_ERR(state)) > - return PTR_ERR(state); > + new_mdp5_state = mdp5_state_from_atomic(s); > + if (IS_ERR(new_mdp5_state)) > + return PTR_ERR(new_mdp5_state); > > - /* grab old_state after mdp5_get_state(), since now we hold lock: */ > - old_state = &mdp5_kms->state->hwpipe; > - new_state = &state->hwpipe; > + old_mdp5_state = mdp5_state_from_dev(s->dev); > + if (IS_ERR(old_mdp5_state)) > + return PTR_ERR(old_mdp5_state); > + > + old_state = &old_mdp5_state->hwpipe; > + new_state = &new_mdp5_state->hwpipe; > > for (i = 0; i < mdp5_kms->num_hwpipes; i++) { > struct mdp5_hw_pipe *cur = mdp5_kms->hwpipes[i]; > @@ -107,7 +110,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct drm_plane *plane, > WARN_ON(r_hwpipe); > > DBG("%s: alloc SMP blocks", (*hwpipe)->name); > - ret = mdp5_smp_assign(mdp5_kms->smp, &state->smp, > + ret = mdp5_smp_assign(mdp5_kms->smp, &new_mdp5_state->smp, > (*hwpipe)->pipe, blkcfg); > if (ret) > return -ENOMEM; > @@ -132,12 +135,17 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe) > { > struct msm_drm_private *priv = s->dev->dev_private; > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > - struct mdp5_state *state = mdp5_get_state(s); > - struct mdp5_hw_pipe_state *new_state = &state->hwpipe; > + struct mdp5_state *state; > + struct mdp5_hw_pipe_state *new_state; > > if (!hwpipe) > return; > > + state = mdp5_state_from_atomic(s); > + if (IS_ERR(state)) > + return; > + > + new_state = &state->hwpipe; > if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx])) > return; > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > index ae4983d9d0a5..918e4123e781 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c > @@ -338,6 +338,7 @@ void mdp5_smp_complete_commit(struct mdp5_smp *smp, struct mdp5_smp_state *state > void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > { > struct mdp5_kms *mdp5_kms = get_kms(smp); > + struct mdp5_state *mdp5_state; > struct mdp5_hw_pipe_state *hwpstate; > struct mdp5_smp_state *state; > int total = 0, i, j; > @@ -346,11 +347,12 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > drm_printf(p, "----\t-----\t-----\n"); > > if (drm_can_sleep()) > - drm_modeset_lock(&mdp5_kms->state_lock, NULL); > + drm_modeset_lock_all(mdp5_kms->dev); > > - /* grab these *after* we hold the state_lock */ > - hwpstate = &mdp5_kms->state->hwpipe; > - state = &mdp5_kms->state->smp; > + /* grab these *after* we hold the modeset_lock */ > + mdp5_state = mdp5_state_from_dev(mdp5_kms->dev); > + hwpstate = &mdp5_state->hwpipe; > + state = &mdp5_state->smp; > > for (i = 0; i < mdp5_kms->num_hwpipes; i++) { > struct mdp5_hw_pipe *hwpipe = mdp5_kms->hwpipes[i]; > @@ -374,7 +376,7 @@ void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p) > bitmap_weight(state->state, smp->blk_cnt)); > > if (drm_can_sleep()) > - drm_modeset_unlock(&mdp5_kms->state_lock); > + drm_modeset_unlock_all(mdp5_kms->dev); > } > > void mdp5_smp_destroy(struct mdp5_smp *smp) > @@ -382,12 +384,15 @@ void mdp5_smp_destroy(struct mdp5_smp *smp) > kfree(smp); > } > > -struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, const struct mdp5_smp_block *cfg) > +struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, > + const struct mdp5_smp_block *cfg, > + struct mdp5_state *mdp5_state) > { > - struct mdp5_smp_state *state = &mdp5_kms->state->smp; > + struct mdp5_smp_state *state; > struct mdp5_smp *smp = NULL; > int ret; > > + state = &mdp5_state->smp; > smp = kzalloc(sizeof(*smp), GFP_KERNEL); > if (unlikely(!smp)) { > ret = -ENOMEM; > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > index b41d0448fbe8..1bfa22b63a2d 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h > @@ -69,6 +69,7 @@ struct mdp5_smp_state { > }; > > struct mdp5_kms; > +struct mdp5_state; > struct mdp5_smp; > > /* > @@ -78,7 +79,8 @@ struct mdp5_smp; > */ > > struct mdp5_smp *mdp5_smp_init(struct mdp5_kms *mdp5_kms, > - const struct mdp5_smp_block *cfg); > + const struct mdp5_smp_block *cfg, > + struct mdp5_state *mdp5_state); > void mdp5_smp_destroy(struct mdp5_smp *smp); > > void mdp5_smp_dump(struct mdp5_smp *smp, struct drm_printer *p); > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index bf5f8c39f34d..e792158676aa 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -220,16 +220,6 @@ int msm_atomic_commit(struct drm_device *dev, > > BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); > > - /* > - * This is the point of no return - everything below never fails except > - * when the hw goes bonghits. Which means we can commit the new state on > - * the software side now. > - * > - * swap driver private state while still holding state_lock > - */ > - if (to_kms_state(state)->state) > - priv->kms->funcs->swap_state(priv->kms, state); > - > /* > * Everything below can be run asynchronously without the need to grab > * any modeset locks at all under one conditions: It must be guaranteed > @@ -262,30 +252,3 @@ int msm_atomic_commit(struct drm_device *dev, > drm_atomic_helper_cleanup_planes(dev, state); > return ret; > } > - > -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev) > -{ > - struct msm_kms_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > - > - if (!state || drm_atomic_state_init(dev, &state->base) < 0) { > - kfree(state); > - return NULL; > - } > - > - return &state->base; > -} > - > -void msm_atomic_state_clear(struct drm_atomic_state *s) > -{ > - struct msm_kms_state *state = to_kms_state(s); > - drm_atomic_state_default_clear(&state->base); > - kfree(state->state); > - state->state = NULL; > -} > - > -void msm_atomic_state_free(struct drm_atomic_state *state) > -{ > - kfree(to_kms_state(state)->state); > - drm_atomic_state_default_release(state); > - kfree(state); > -} > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 30cd514d8f7c..7e8f640062ef 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -42,11 +42,79 @@ static const struct drm_mode_config_funcs mode_config_funcs = { > .output_poll_changed = drm_fb_helper_output_poll_changed, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = msm_atomic_commit, > - .atomic_state_alloc = msm_atomic_state_alloc, > - .atomic_state_clear = msm_atomic_state_clear, > - .atomic_state_free = msm_atomic_state_free, > }; > > +static inline > +struct msm_kms *msm_kms_from_obj(struct drm_private_obj *obj) > +{ > + return container_of(obj, struct msm_kms, base); > +} > + > +static inline > +struct msm_kms_state *msm_kms_state_from_priv(struct drm_private_state *priv) > +{ > + return container_of(priv, struct msm_kms_state, base); > +} > + > + > +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state) > +{ > + struct msm_drm_private *priv = state->dev->dev_private; > + struct drm_private_obj *obj = &priv->kms->base; > + struct drm_private_state *priv_state; > + > + priv_state = drm_atomic_get_private_obj_state(state, obj); > + if (IS_ERR_OR_NULL(priv_state)) > + return (struct msm_kms_state *)state; /* casting ERR_PTR */ > + > + return msm_kms_state_from_priv(priv_state); > +} > + > +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev) > +{ > + struct msm_drm_private *priv = dev->dev_private; > + struct drm_private_obj *obj = &priv->kms->base; > + struct drm_private_state *priv_state = obj->state; > + > + return msm_kms_state_from_priv(priv_state); > +} > + > +static struct drm_private_state * > +msm_kms_duplicate_state(struct drm_private_obj *obj) > +{ > + struct msm_kms *kms = msm_kms_from_obj(obj); > + struct msm_kms_state *state = msm_kms_state_from_priv(obj->state); > + struct msm_kms_state *new_state; > + > + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL); > + if (!new_state) > + return NULL; > + > + if (kms->funcs->duplicate_state) > + new_state->state = kms->funcs->duplicate_state(state->state); > + > + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base); > + > + return &new_state->base; > +} > + > +static void msm_kms_destroy_state(struct drm_private_obj *obj, > + struct drm_private_state *priv_state) > +{ > + struct msm_kms *kms = msm_kms_from_obj(obj); > + struct msm_kms_state *state = msm_kms_state_from_priv(priv_state); > + > + if (kms->funcs->destroy_state) > + kms->funcs->destroy_state(state->state); > + kfree(state); > +} > + > +static const struct drm_private_state_funcs kms_state_funcs = { > + .atomic_duplicate_state = msm_kms_duplicate_state, > + .atomic_destroy_state = msm_kms_destroy_state, > +}; > + > + Minor nit: Two blank lines here^ > #ifdef CONFIG_DRM_MSM_REGISTER_LOGGING > static bool reglog = false; > MODULE_PARM_DESC(reglog, "Enable register read/write logging"); > @@ -245,6 +313,8 @@ static int msm_drm_uninit(struct device *dev) > flush_workqueue(priv->atomic_wq); > destroy_workqueue(priv->atomic_wq); > > + drm_atomic_private_obj_fini(&kms->base); > + > if (kms && kms->funcs) > kms->funcs->destroy(kms); > > @@ -356,6 +426,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > struct drm_device *ddev; > struct msm_drm_private *priv; > struct msm_kms *kms; > + struct msm_kms_state *initial_state; > int ret; > > ddev = drm_dev_alloc(drv, dev); > @@ -364,6 +435,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > return PTR_ERR(ddev); > } > > + initial_state = kzalloc(sizeof(*initial_state), GFP_KERNEL); > + if (!initial_state) > + return -ENOMEM; > + > platform_set_drvdata(pdev, ddev); > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > @@ -394,7 +469,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > drm_mode_config_init(ddev); > > /* Bind all our sub-components: */ > - ret = component_bind_all(dev, ddev); > + ret = component_bind_all(dev, initial_state); Only the core kms component (mdp4/mdp5/dpu) will use initial_state, and the encoders drivers (DSI/HDMI) would discard it, right? I guess it's still better than passing ddev, which can be derived from the component master. Reviewed-by: Archit Taneja <architt@codeaurora.org> > if (ret) { > msm_mdss_destroy(ddev); > kfree(priv); > @@ -433,6 +508,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) > goto fail; > } > > + drm_atomic_private_obj_init(&kms->base, > + &initial_state->base, > + &kms_state_funcs); > + > if (kms) { > ret = kms->funcs->hw_init(kms); > if (ret) { > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 48ed5b9a8580..c5b8c989b859 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -162,9 +162,6 @@ struct msm_format { > > int msm_atomic_commit(struct drm_device *dev, > struct drm_atomic_state *state, bool nonblock); > -struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev); > -void msm_atomic_state_clear(struct drm_atomic_state *state); > -void msm_atomic_state_free(struct drm_atomic_state *state); > > void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, > struct msm_gem_vma *vma, struct sg_table *sgt); > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h > index 17d5824417ad..24d09fcebf16 100644 > --- a/drivers/gpu/drm/msm/msm_kms.h > +++ b/drivers/gpu/drm/msm/msm_kms.h > @@ -42,6 +42,8 @@ struct msm_kms_funcs { > void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc); > /* swap global atomic state: */ > void (*swap_state)(struct msm_kms *kms, struct drm_atomic_state *state); > + void *(*duplicate_state)(void *state); > + void (*destroy_state)(void *state); > /* modeset, bracketing atomic_commit(): */ > void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state); > void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state); > @@ -68,6 +70,8 @@ struct msm_kms_funcs { > }; > > struct msm_kms { > + struct drm_private_obj base; > + > const struct msm_kms_funcs *funcs; > > /* irq number to be passed on to drm_irq_install */ > @@ -78,16 +82,23 @@ struct msm_kms { > }; > > /** > - * Subclass of drm_atomic_state, to allow kms backend to have driver > + * Subclass of drm_private_state, to allow kms backend to have driver > * private global state. The kms backend can do whatever it wants > - * with the ->state ptr. On ->atomic_state_clear() the ->state ptr > - * is kfree'd and set back to NULL. > + * with the ->state ptr. > */ > struct msm_kms_state { > - struct drm_atomic_state base; > + struct drm_private_state base; > void *state; > }; > -#define to_kms_state(x) container_of(x, struct msm_kms_state, base) > + > +/** > + * Extracts the msm_kms_state from either an atomic state, or the current > + * device. One or the other might be desirable depending on whether you want the > + * currently configured msm_kms_state (from_dev), or you would like the state > + * which is about to be applied (from_atomic). > + */ > +struct msm_kms_state *msm_kms_state_from_dev(struct drm_device *dev); > +struct msm_kms_state *msm_kms_state_from_atomic(struct drm_atomic_state *state); > > static inline void msm_kms_init(struct msm_kms *kms, > const struct msm_kms_funcs *funcs) > ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* [PATCH v2 2/6] drm/msm: Refactor complete_commit() to look more the helpers 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: architt-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, Sean Paul, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-F7+t8E8rja9g9hUCZPvPmw Factor out the commit_tail() portions of complete_commit() into a separate function to facilitate moving to the atomic helpers in future patches. Changes in v2: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index e792158676aa..671a18ee977d 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -97,18 +97,12 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, } } -/* The (potentially) asynchronous part of the commit. At this point - * nothing can fail short of armageddon. - */ -static void complete_commit(struct msm_commit *c, bool async) +static void msm_atomic_commit_tail(struct drm_atomic_state *state) { - struct drm_atomic_state *state = c->state; struct drm_device *dev = state->dev; struct msm_drm_private *priv = dev->dev_private; struct msm_kms *kms = priv->kms; - drm_atomic_helper_wait_for_fences(dev, state, false); - kms->funcs->prepare_commit(kms, state); drm_atomic_helper_commit_modeset_disables(dev, state); @@ -135,6 +129,19 @@ static void complete_commit(struct msm_commit *c, bool async) drm_atomic_helper_cleanup_planes(dev, state); kms->funcs->complete_commit(kms, state); +} + +/* The (potentially) asynchronous part of the commit. At this point + * nothing can fail short of armageddon. + */ +static void complete_commit(struct msm_commit *c) +{ + struct drm_atomic_state *state = c->state; + struct drm_device *dev = state->dev; + + drm_atomic_helper_wait_for_fences(dev, state, false); + + msm_atomic_commit_tail(state); drm_atomic_state_put(state); @@ -143,7 +150,7 @@ static void complete_commit(struct msm_commit *c, bool async) static void commit_worker(struct work_struct *work) { - complete_commit(container_of(work, struct msm_commit, work), true); + complete_commit(container_of(work, struct msm_commit, work)); } /** @@ -242,7 +249,7 @@ int msm_atomic_commit(struct drm_device *dev, return 0; } - complete_commit(c, false); + complete_commit(c); return 0; -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/6] drm/msm: Refactor complete_commit() to look more the helpers @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Factor out the commit_tail() portions of complete_commit() into a separate function to facilitate moving to the atomic helpers in future patches. Changes in v2: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index e792158676aa..671a18ee977d 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -97,18 +97,12 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, } } -/* The (potentially) asynchronous part of the commit. At this point - * nothing can fail short of armageddon. - */ -static void complete_commit(struct msm_commit *c, bool async) +static void msm_atomic_commit_tail(struct drm_atomic_state *state) { - struct drm_atomic_state *state = c->state; struct drm_device *dev = state->dev; struct msm_drm_private *priv = dev->dev_private; struct msm_kms *kms = priv->kms; - drm_atomic_helper_wait_for_fences(dev, state, false); - kms->funcs->prepare_commit(kms, state); drm_atomic_helper_commit_modeset_disables(dev, state); @@ -135,6 +129,19 @@ static void complete_commit(struct msm_commit *c, bool async) drm_atomic_helper_cleanup_planes(dev, state); kms->funcs->complete_commit(kms, state); +} + +/* The (potentially) asynchronous part of the commit. At this point + * nothing can fail short of armageddon. + */ +static void complete_commit(struct msm_commit *c) +{ + struct drm_atomic_state *state = c->state; + struct drm_device *dev = state->dev; + + drm_atomic_helper_wait_for_fences(dev, state, false); + + msm_atomic_commit_tail(state); drm_atomic_state_put(state); @@ -143,7 +150,7 @@ static void complete_commit(struct msm_commit *c, bool async) static void commit_worker(struct work_struct *work) { - complete_commit(container_of(work, struct msm_commit, work), true); + complete_commit(container_of(work, struct msm_commit, work)); } /** @@ -242,7 +249,7 @@ int msm_atomic_commit(struct drm_device *dev, return 0; } - complete_commit(c, false); + complete_commit(c); return 0; -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <20180328190657.218661-3-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* Re: [PATCH v2 2/6] drm/msm: Refactor complete_commit() to look more the helpers 2018-03-28 19:06 ` Sean Paul @ 2018-04-02 5:22 ` Archit Taneja -1 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:22 UTC (permalink / raw) To: Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: jsanka-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, hoegsberg-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Factor out the commit_tail() portions of complete_commit() into a > separate function to facilitate moving to the atomic helpers in future > patches. > Reviewed-by: Archit Taneja <architt@codeaurora.org> > Changes in v2: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/msm_atomic.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index e792158676aa..671a18ee977d 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -97,18 +97,12 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, > } > } > > -/* The (potentially) asynchronous part of the commit. At this point > - * nothing can fail short of armageddon. > - */ > -static void complete_commit(struct msm_commit *c, bool async) > +static void msm_atomic_commit_tail(struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = c->state; > struct drm_device *dev = state->dev; > struct msm_drm_private *priv = dev->dev_private; > struct msm_kms *kms = priv->kms; > > - drm_atomic_helper_wait_for_fences(dev, state, false); > - > kms->funcs->prepare_commit(kms, state); > > drm_atomic_helper_commit_modeset_disables(dev, state); > @@ -135,6 +129,19 @@ static void complete_commit(struct msm_commit *c, bool async) > drm_atomic_helper_cleanup_planes(dev, state); > > kms->funcs->complete_commit(kms, state); > +} > + > +/* The (potentially) asynchronous part of the commit. At this point > + * nothing can fail short of armageddon. > + */ > +static void complete_commit(struct msm_commit *c) > +{ > + struct drm_atomic_state *state = c->state; > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_wait_for_fences(dev, state, false); > + > + msm_atomic_commit_tail(state); > > drm_atomic_state_put(state); > > @@ -143,7 +150,7 @@ static void complete_commit(struct msm_commit *c, bool async) > > static void commit_worker(struct work_struct *work) > { > - complete_commit(container_of(work, struct msm_commit, work), true); > + complete_commit(container_of(work, struct msm_commit, work)); > } > > /** > @@ -242,7 +249,7 @@ int msm_atomic_commit(struct drm_device *dev, > return 0; > } > > - complete_commit(c, false); > + complete_commit(c); > > return 0; > > _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] drm/msm: Refactor complete_commit() to look more the helpers @ 2018-04-02 5:22 ` Archit Taneja 0 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:22 UTC (permalink / raw) To: Sean Paul, freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, linux-kernel On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Factor out the commit_tail() portions of complete_commit() into a > separate function to facilitate moving to the atomic helpers in future > patches. > Reviewed-by: Archit Taneja <architt@codeaurora.org> > Changes in v2: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/msm_atomic.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index e792158676aa..671a18ee977d 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -97,18 +97,12 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, > } > } > > -/* The (potentially) asynchronous part of the commit. At this point > - * nothing can fail short of armageddon. > - */ > -static void complete_commit(struct msm_commit *c, bool async) > +static void msm_atomic_commit_tail(struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = c->state; > struct drm_device *dev = state->dev; > struct msm_drm_private *priv = dev->dev_private; > struct msm_kms *kms = priv->kms; > > - drm_atomic_helper_wait_for_fences(dev, state, false); > - > kms->funcs->prepare_commit(kms, state); > > drm_atomic_helper_commit_modeset_disables(dev, state); > @@ -135,6 +129,19 @@ static void complete_commit(struct msm_commit *c, bool async) > drm_atomic_helper_cleanup_planes(dev, state); > > kms->funcs->complete_commit(kms, state); > +} > + > +/* The (potentially) asynchronous part of the commit. At this point > + * nothing can fail short of armageddon. > + */ > +static void complete_commit(struct msm_commit *c) > +{ > + struct drm_atomic_state *state = c->state; > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_wait_for_fences(dev, state, false); > + > + msm_atomic_commit_tail(state); > > drm_atomic_state_put(state); > > @@ -143,7 +150,7 @@ static void complete_commit(struct msm_commit *c, bool async) > > static void commit_worker(struct work_struct *work) > { > - complete_commit(container_of(work, struct msm_commit, work), true); > + complete_commit(container_of(work, struct msm_commit, work)); > } > > /** > @@ -242,7 +249,7 @@ int msm_atomic_commit(struct drm_device *dev, > return 0; > } > > - complete_commit(c, false); > + complete_commit(c); > > return 0; > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] drm/msm: Mark the crtc->state->event consumed 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: architt-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, Sean Paul, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-F7+t8E8rja9g9hUCZPvPmw Don't leave the event != NULL once it's consumed, this is used a signal to the atomic helpers that the event will be handled by the driver. Changes in v2: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + 2 files changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c index 6e5e1aa54ce1..b001699297c4 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c @@ -351,6 +351,7 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&dev->event_lock, flags); mdp4_crtc->event = crtc->state->event; + crtc->state->event = NULL; spin_unlock_irqrestore(&dev->event_lock, flags); blend_setup(crtc); diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c index 9893e43ba6c5..76b96081916f 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c @@ -708,6 +708,7 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&dev->event_lock, flags); mdp5_crtc->event = crtc->state->event; + crtc->state->event = NULL; spin_unlock_irqrestore(&dev->event_lock, flags); /* -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] drm/msm: Mark the crtc->state->event consumed @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Don't leave the event != NULL once it's consumed, this is used a signal to the atomic helpers that the event will be handled by the driver. Changes in v2: - None Cc: Jeykumar Sankaran <jsanka@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + 2 files changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c index 6e5e1aa54ce1..b001699297c4 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c @@ -351,6 +351,7 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&dev->event_lock, flags); mdp4_crtc->event = crtc->state->event; + crtc->state->event = NULL; spin_unlock_irqrestore(&dev->event_lock, flags); blend_setup(crtc); diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c index 9893e43ba6c5..76b96081916f 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c @@ -708,6 +708,7 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&dev->event_lock, flags); mdp5_crtc->event = crtc->state->event; + crtc->state->event = NULL; spin_unlock_irqrestore(&dev->event_lock, flags); /* -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <20180328190657.218661-4-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* Re: [PATCH v2 3/6] drm/msm: Mark the crtc->state->event consumed 2018-03-28 19:06 ` Sean Paul @ 2018-04-02 5:22 ` Archit Taneja -1 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:22 UTC (permalink / raw) To: Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: jsanka-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, hoegsberg-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Don't leave the event != NULL once it's consumed, this is used a signal s/used a/used as a ? > to the atomic helpers that the event will be handled by the driver. > Reviewed-by: Archit Taneja <architt@codeaurora.org> > Changes in v2: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > index 6e5e1aa54ce1..b001699297c4 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > @@ -351,6 +351,7 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc, > > spin_lock_irqsave(&dev->event_lock, flags); > mdp4_crtc->event = crtc->state->event; > + crtc->state->event = NULL; > spin_unlock_irqrestore(&dev->event_lock, flags); > > blend_setup(crtc); > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index 9893e43ba6c5..76b96081916f 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -708,6 +708,7 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, > > spin_lock_irqsave(&dev->event_lock, flags); > mdp5_crtc->event = crtc->state->event; > + crtc->state->event = NULL; > spin_unlock_irqrestore(&dev->event_lock, flags); > > /* > _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] drm/msm: Mark the crtc->state->event consumed @ 2018-04-02 5:22 ` Archit Taneja 0 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:22 UTC (permalink / raw) To: Sean Paul, freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, linux-kernel On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Don't leave the event != NULL once it's consumed, this is used a signal s/used a/used as a ? > to the atomic helpers that the event will be handled by the driver. > Reviewed-by: Archit Taneja <architt@codeaurora.org> > Changes in v2: > - None > > Cc: Jeykumar Sankaran <jsanka@codeaurora.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > index 6e5e1aa54ce1..b001699297c4 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > @@ -351,6 +351,7 @@ static void mdp4_crtc_atomic_flush(struct drm_crtc *crtc, > > spin_lock_irqsave(&dev->event_lock, flags); > mdp4_crtc->event = crtc->state->event; > + crtc->state->event = NULL; > spin_unlock_irqrestore(&dev->event_lock, flags); > > blend_setup(crtc); > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index 9893e43ba6c5..76b96081916f 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -708,6 +708,7 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, > > spin_lock_irqsave(&dev->event_lock, flags); > mdp5_crtc->event = crtc->state->event; > + crtc->state->event = NULL; > spin_unlock_irqrestore(&dev->event_lock, flags); > > /* > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] drm/msm: Issue queued events when disabling crtc 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: architt-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, Sean Paul, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-F7+t8E8rja9g9hUCZPvPmw Ensure that any queued events are issued when disabling the crtc. This avoids timeouts when we come back and wait for dependencies (like the previous frame's flip_done). Changes in v2: - None Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c index 76b96081916f..10271359789e 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c @@ -430,6 +430,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state); struct mdp5_kms *mdp5_kms = get_kms(crtc); struct device *dev = &mdp5_kms->pdev->dev; + unsigned long flags; DBG("%s", crtc->name); @@ -445,6 +446,14 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->err); pm_runtime_put_sync(dev); + if (crtc->state->event && !crtc->state->active) { + WARN_ON(mdp5_crtc->event); + spin_lock_irqsave(&mdp5_kms->dev->event_lock, flags); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; + spin_unlock_irqrestore(&mdp5_kms->dev->event_lock, flags); + } + mdp5_crtc->enabled = false; } -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] drm/msm: Issue queued events when disabling crtc @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Ensure that any queued events are issued when disabling the crtc. This avoids timeouts when we come back and wait for dependencies (like the previous frame's flip_done). Changes in v2: - None Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c index 76b96081916f..10271359789e 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c @@ -430,6 +430,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state); struct mdp5_kms *mdp5_kms = get_kms(crtc); struct device *dev = &mdp5_kms->pdev->dev; + unsigned long flags; DBG("%s", crtc->name); @@ -445,6 +446,14 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->err); pm_runtime_put_sync(dev); + if (crtc->state->event && !crtc->state->active) { + WARN_ON(mdp5_crtc->event); + spin_lock_irqsave(&mdp5_kms->dev->event_lock, flags); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; + spin_unlock_irqrestore(&mdp5_kms->dev->event_lock, flags); + } + mdp5_crtc->enabled = false; } -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <20180328190657.218661-5-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* Re: [PATCH v2 4/6] drm/msm: Issue queued events when disabling crtc 2018-03-28 19:06 ` Sean Paul @ 2018-04-02 5:23 ` Archit Taneja -1 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:23 UTC (permalink / raw) To: Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: jsanka-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, hoegsberg-F7+t8E8rja9g9hUCZPvPmw, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Ensure that any queued events are issued when disabling the crtc. This > avoids timeouts when we come back and wait for dependencies (like the > previous frame's flip_done). Reviewed-by: Archit Taneja <architt@codeaurora.org> > > Changes in v2: > - None > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index 76b96081916f..10271359789e 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -430,6 +430,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, > struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state); > struct mdp5_kms *mdp5_kms = get_kms(crtc); > struct device *dev = &mdp5_kms->pdev->dev; > + unsigned long flags; > > DBG("%s", crtc->name); > > @@ -445,6 +446,14 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, > mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->err); > pm_runtime_put_sync(dev); > > + if (crtc->state->event && !crtc->state->active) { > + WARN_ON(mdp5_crtc->event); > + spin_lock_irqsave(&mdp5_kms->dev->event_lock, flags); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + crtc->state->event = NULL; > + spin_unlock_irqrestore(&mdp5_kms->dev->event_lock, flags); > + } > + > mdp5_crtc->enabled = false; > } > > _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/6] drm/msm: Issue queued events when disabling crtc @ 2018-04-02 5:23 ` Archit Taneja 0 siblings, 0 replies; 21+ messages in thread From: Archit Taneja @ 2018-04-02 5:23 UTC (permalink / raw) To: Sean Paul, freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, linux-kernel On Thursday 29 March 2018 12:36 AM, Sean Paul wrote: > Ensure that any queued events are issued when disabling the crtc. This > avoids timeouts when we come back and wait for dependencies (like the > previous frame's flip_done). Reviewed-by: Archit Taneja <architt@codeaurora.org> > > Changes in v2: > - None > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > index 76b96081916f..10271359789e 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c > @@ -430,6 +430,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, > struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state); > struct mdp5_kms *mdp5_kms = get_kms(crtc); > struct device *dev = &mdp5_kms->pdev->dev; > + unsigned long flags; > > DBG("%s", crtc->name); > > @@ -445,6 +446,14 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc, > mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->err); > pm_runtime_put_sync(dev); > > + if (crtc->state->event && !crtc->state->active) { > + WARN_ON(mdp5_crtc->event); > + spin_lock_irqsave(&mdp5_kms->dev->event_lock, flags); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + crtc->state->event = NULL; > + spin_unlock_irqrestore(&mdp5_kms->dev->event_lock, flags); > + } > + > mdp5_crtc->enabled = false; > } > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] drm/msm: Remove msm_commit/worker, use atomic helper commit 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: architt-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, Sean Paul, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, hoegsberg-F7+t8E8rja9g9hUCZPvPmw Moving further towards switching fully to the the atomic helpers, this patch removes the hand-rolled worker nonblock commit code and uses the atomic helpers commit_work model. Changes in v2: - Remove commit_destroy() - Shuffle order of commit_tail calls to further serialize commits - Use stall in swap_state to avoid abandoned events on disable Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 153 +++++++++---------------------- drivers/gpu/drm/msm/msm_drv.c | 1 - drivers/gpu/drm/msm/msm_drv.h | 4 - 3 files changed, 42 insertions(+), 116 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 671a18ee977d..52249565cc03 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -20,66 +20,6 @@ #include "msm_gem.h" #include "msm_fence.h" -struct msm_commit { - struct drm_device *dev; - struct drm_atomic_state *state; - struct work_struct work; - uint32_t crtc_mask; -}; - -static void commit_worker(struct work_struct *work); - -/* block until specified crtcs are no longer pending update, and - * atomically mark them as pending update - */ -static int start_atomic(struct msm_drm_private *priv, uint32_t crtc_mask) -{ - int ret; - - spin_lock(&priv->pending_crtcs_event.lock); - ret = wait_event_interruptible_locked(priv->pending_crtcs_event, - !(priv->pending_crtcs & crtc_mask)); - if (ret == 0) { - DBG("start: %08x", crtc_mask); - priv->pending_crtcs |= crtc_mask; - } - spin_unlock(&priv->pending_crtcs_event.lock); - - return ret; -} - -/* clear specified crtcs (no longer pending update) - */ -static void end_atomic(struct msm_drm_private *priv, uint32_t crtc_mask) -{ - spin_lock(&priv->pending_crtcs_event.lock); - DBG("end: %08x", crtc_mask); - priv->pending_crtcs &= ~crtc_mask; - wake_up_all_locked(&priv->pending_crtcs_event); - spin_unlock(&priv->pending_crtcs_event.lock); -} - -static struct msm_commit *commit_init(struct drm_atomic_state *state) -{ - struct msm_commit *c = kzalloc(sizeof(*c), GFP_KERNEL); - - if (!c) - return NULL; - - c->dev = state->dev; - c->state = state; - - INIT_WORK(&c->work, commit_worker); - - return c; -} - -static void commit_destroy(struct msm_commit *c) -{ - end_atomic(c->dev->dev_private, c->crtc_mask); - kfree(c); -} - static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) { @@ -126,31 +66,37 @@ static void msm_atomic_commit_tail(struct drm_atomic_state *state) msm_atomic_wait_for_commit_done(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); - kms->funcs->complete_commit(kms, state); + + drm_atomic_helper_wait_for_vblanks(dev, state); + + drm_atomic_helper_commit_hw_done(state); + + drm_atomic_helper_cleanup_planes(dev, state); } /* The (potentially) asynchronous part of the commit. At this point * nothing can fail short of armageddon. */ -static void complete_commit(struct msm_commit *c) +static void commit_tail(struct drm_atomic_state *state) { - struct drm_atomic_state *state = c->state; - struct drm_device *dev = state->dev; + drm_atomic_helper_wait_for_fences(state->dev, state, false); - drm_atomic_helper_wait_for_fences(dev, state, false); + drm_atomic_helper_wait_for_dependencies(state); msm_atomic_commit_tail(state); - drm_atomic_state_put(state); + drm_atomic_helper_commit_cleanup_done(state); - commit_destroy(c); + drm_atomic_state_put(state); } -static void commit_worker(struct work_struct *work) +static void commit_work(struct work_struct *work) { - complete_commit(container_of(work, struct msm_commit, work)); + struct drm_atomic_state *state = container_of(work, + struct drm_atomic_state, + commit_work); + commit_tail(state); } /** @@ -169,17 +115,12 @@ int msm_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock) { struct msm_drm_private *priv = dev->dev_private; - struct msm_commit *c; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret; - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - /* * Note that plane->atomic_async_check() should fail if we need * to re-assign hwpipe or anything that touches global atomic @@ -187,45 +128,39 @@ int msm_atomic_commit(struct drm_device *dev, * cases. */ if (state->async_update) { + ret = drm_atomic_helper_prepare_planes(dev, state); + if (ret) + return ret; + drm_atomic_helper_async_commit(dev, state); drm_atomic_helper_cleanup_planes(dev, state); return 0; } - c = commit_init(state); - if (!c) { - ret = -ENOMEM; - goto error; - } + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (ret) + return ret; - /* - * Figure out what crtcs we have: - */ - for_each_new_crtc_in_state(state, crtc, crtc_state, i) - c->crtc_mask |= drm_crtc_mask(crtc); + INIT_WORK(&state->commit_work, commit_work); - /* - * Figure out what fence to wait for: - */ - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { - if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) { - struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0); - struct msm_gem_object *msm_obj = to_msm_bo(obj); - struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv); - - drm_atomic_set_fence_for_plane(new_plane_state, fence); - } + ret = drm_atomic_helper_prepare_planes(dev, state); + if (ret) + return ret; + + if (!nonblock) { + ret = drm_atomic_helper_wait_for_fences(dev, state, true); + if (ret) + goto error; } /* - * Wait for pending updates on any of the same crtc's and then - * mark our set of crtc's as busy: + * This is the point of no return - everything below never fails except + * when the hw goes bonghits. Which means we can commit the new state on + * the software side now. + * + * swap driver private state while still holding state_lock */ - ret = start_atomic(dev->dev_private, c->crtc_mask); - if (ret) - goto err_free; - - BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); + BUG_ON(drm_atomic_helper_swap_state(state, true) < 0); /* * Everything below can be run asynchronously without the need to grab @@ -244,17 +179,13 @@ int msm_atomic_commit(struct drm_device *dev, */ drm_atomic_state_get(state); - if (nonblock) { - queue_work(priv->atomic_wq, &c->work); - return 0; - } - - complete_commit(c); + if (nonblock) + queue_work(system_unbound_wq, &state->commit_work); + else + commit_tail(state); return 0; -err_free: - kfree(c); error: drm_atomic_helper_cleanup_planes(dev, state); return ret; diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 7e8f640062ef..d1a45a68c287 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -459,7 +459,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->wq = alloc_ordered_workqueue("msm", 0); priv->atomic_wq = alloc_ordered_workqueue("msm:atomic", 0); - init_waitqueue_head(&priv->pending_crtcs_event); INIT_LIST_HEAD(&priv->inactive_list); INIT_LIST_HEAD(&priv->vblank_ctrl.event_list); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index c5b8c989b859..b89fa5186eb8 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -117,10 +117,6 @@ struct msm_drm_private { struct workqueue_struct *wq; struct workqueue_struct *atomic_wq; - /* crtcs pending async atomic updates: */ - uint32_t pending_crtcs; - wait_queue_head_t pending_crtcs_event; - unsigned int num_planes; struct drm_plane *planes[16]; -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] drm/msm: Remove msm_commit/worker, use atomic helper commit @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Moving further towards switching fully to the the atomic helpers, this patch removes the hand-rolled worker nonblock commit code and uses the atomic helpers commit_work model. Changes in v2: - Remove commit_destroy() - Shuffle order of commit_tail calls to further serialize commits - Use stall in swap_state to avoid abandoned events on disable Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 153 +++++++++---------------------- drivers/gpu/drm/msm/msm_drv.c | 1 - drivers/gpu/drm/msm/msm_drv.h | 4 - 3 files changed, 42 insertions(+), 116 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 671a18ee977d..52249565cc03 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -20,66 +20,6 @@ #include "msm_gem.h" #include "msm_fence.h" -struct msm_commit { - struct drm_device *dev; - struct drm_atomic_state *state; - struct work_struct work; - uint32_t crtc_mask; -}; - -static void commit_worker(struct work_struct *work); - -/* block until specified crtcs are no longer pending update, and - * atomically mark them as pending update - */ -static int start_atomic(struct msm_drm_private *priv, uint32_t crtc_mask) -{ - int ret; - - spin_lock(&priv->pending_crtcs_event.lock); - ret = wait_event_interruptible_locked(priv->pending_crtcs_event, - !(priv->pending_crtcs & crtc_mask)); - if (ret == 0) { - DBG("start: %08x", crtc_mask); - priv->pending_crtcs |= crtc_mask; - } - spin_unlock(&priv->pending_crtcs_event.lock); - - return ret; -} - -/* clear specified crtcs (no longer pending update) - */ -static void end_atomic(struct msm_drm_private *priv, uint32_t crtc_mask) -{ - spin_lock(&priv->pending_crtcs_event.lock); - DBG("end: %08x", crtc_mask); - priv->pending_crtcs &= ~crtc_mask; - wake_up_all_locked(&priv->pending_crtcs_event); - spin_unlock(&priv->pending_crtcs_event.lock); -} - -static struct msm_commit *commit_init(struct drm_atomic_state *state) -{ - struct msm_commit *c = kzalloc(sizeof(*c), GFP_KERNEL); - - if (!c) - return NULL; - - c->dev = state->dev; - c->state = state; - - INIT_WORK(&c->work, commit_worker); - - return c; -} - -static void commit_destroy(struct msm_commit *c) -{ - end_atomic(c->dev->dev_private, c->crtc_mask); - kfree(c); -} - static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) { @@ -126,31 +66,37 @@ static void msm_atomic_commit_tail(struct drm_atomic_state *state) msm_atomic_wait_for_commit_done(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); - kms->funcs->complete_commit(kms, state); + + drm_atomic_helper_wait_for_vblanks(dev, state); + + drm_atomic_helper_commit_hw_done(state); + + drm_atomic_helper_cleanup_planes(dev, state); } /* The (potentially) asynchronous part of the commit. At this point * nothing can fail short of armageddon. */ -static void complete_commit(struct msm_commit *c) +static void commit_tail(struct drm_atomic_state *state) { - struct drm_atomic_state *state = c->state; - struct drm_device *dev = state->dev; + drm_atomic_helper_wait_for_fences(state->dev, state, false); - drm_atomic_helper_wait_for_fences(dev, state, false); + drm_atomic_helper_wait_for_dependencies(state); msm_atomic_commit_tail(state); - drm_atomic_state_put(state); + drm_atomic_helper_commit_cleanup_done(state); - commit_destroy(c); + drm_atomic_state_put(state); } -static void commit_worker(struct work_struct *work) +static void commit_work(struct work_struct *work) { - complete_commit(container_of(work, struct msm_commit, work)); + struct drm_atomic_state *state = container_of(work, + struct drm_atomic_state, + commit_work); + commit_tail(state); } /** @@ -169,17 +115,12 @@ int msm_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock) { struct msm_drm_private *priv = dev->dev_private; - struct msm_commit *c; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret; - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - /* * Note that plane->atomic_async_check() should fail if we need * to re-assign hwpipe or anything that touches global atomic @@ -187,45 +128,39 @@ int msm_atomic_commit(struct drm_device *dev, * cases. */ if (state->async_update) { + ret = drm_atomic_helper_prepare_planes(dev, state); + if (ret) + return ret; + drm_atomic_helper_async_commit(dev, state); drm_atomic_helper_cleanup_planes(dev, state); return 0; } - c = commit_init(state); - if (!c) { - ret = -ENOMEM; - goto error; - } + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (ret) + return ret; - /* - * Figure out what crtcs we have: - */ - for_each_new_crtc_in_state(state, crtc, crtc_state, i) - c->crtc_mask |= drm_crtc_mask(crtc); + INIT_WORK(&state->commit_work, commit_work); - /* - * Figure out what fence to wait for: - */ - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { - if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) { - struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0); - struct msm_gem_object *msm_obj = to_msm_bo(obj); - struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv); - - drm_atomic_set_fence_for_plane(new_plane_state, fence); - } + ret = drm_atomic_helper_prepare_planes(dev, state); + if (ret) + return ret; + + if (!nonblock) { + ret = drm_atomic_helper_wait_for_fences(dev, state, true); + if (ret) + goto error; } /* - * Wait for pending updates on any of the same crtc's and then - * mark our set of crtc's as busy: + * This is the point of no return - everything below never fails except + * when the hw goes bonghits. Which means we can commit the new state on + * the software side now. + * + * swap driver private state while still holding state_lock */ - ret = start_atomic(dev->dev_private, c->crtc_mask); - if (ret) - goto err_free; - - BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); + BUG_ON(drm_atomic_helper_swap_state(state, true) < 0); /* * Everything below can be run asynchronously without the need to grab @@ -244,17 +179,13 @@ int msm_atomic_commit(struct drm_device *dev, */ drm_atomic_state_get(state); - if (nonblock) { - queue_work(priv->atomic_wq, &c->work); - return 0; - } - - complete_commit(c); + if (nonblock) + queue_work(system_unbound_wq, &state->commit_work); + else + commit_tail(state); return 0; -err_free: - kfree(c); error: drm_atomic_helper_cleanup_planes(dev, state); return ret; diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 7e8f640062ef..d1a45a68c287 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -459,7 +459,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->wq = alloc_ordered_workqueue("msm", 0); priv->atomic_wq = alloc_ordered_workqueue("msm:atomic", 0); - init_waitqueue_head(&priv->pending_crtcs_event); INIT_LIST_HEAD(&priv->inactive_list); INIT_LIST_HEAD(&priv->vblank_ctrl.event_list); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index c5b8c989b859..b89fa5186eb8 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -117,10 +117,6 @@ struct msm_drm_private { struct workqueue_struct *wq; struct workqueue_struct *atomic_wq; - /* crtcs pending async atomic updates: */ - uint32_t pending_crtcs; - wait_queue_head_t pending_crtcs_event; - unsigned int num_planes; struct drm_plane *planes[16]; -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] drm/msm: Switch to atomic_helper_commit() 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul @ 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` Sean Paul 2 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel; +Cc: linux-kernel, abhinavk, hoegsberg Now that all of the msm-specific goo is tucked safely away we can switch over to using the atomic helper commit directly. \o/ Changes in v2: - None Cc: Abhinav Kumar <abhinavk@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 120 +------------------------------ drivers/gpu/drm/msm/msm_drv.c | 7 +- drivers/gpu/drm/msm/msm_drv.h | 3 +- 3 files changed, 8 insertions(+), 122 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 52249565cc03..d593ac3de1df 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -17,8 +17,6 @@ #include "msm_drv.h" #include "msm_kms.h" -#include "msm_gem.h" -#include "msm_fence.h" static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) @@ -37,7 +35,7 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, } } -static void msm_atomic_commit_tail(struct drm_atomic_state *state) +void msm_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct msm_drm_private *priv = dev->dev_private; @@ -74,119 +72,3 @@ static void msm_atomic_commit_tail(struct drm_atomic_state *state) drm_atomic_helper_cleanup_planes(dev, state); } - -/* The (potentially) asynchronous part of the commit. At this point - * nothing can fail short of armageddon. - */ -static void commit_tail(struct drm_atomic_state *state) -{ - drm_atomic_helper_wait_for_fences(state->dev, state, false); - - drm_atomic_helper_wait_for_dependencies(state); - - msm_atomic_commit_tail(state); - - drm_atomic_helper_commit_cleanup_done(state); - - drm_atomic_state_put(state); -} - -static void commit_work(struct work_struct *work) -{ - struct drm_atomic_state *state = container_of(work, - struct drm_atomic_state, - commit_work); - commit_tail(state); -} - -/** - * drm_atomic_helper_commit - commit validated state object - * @dev: DRM device - * @state: the driver state object - * @nonblock: nonblocking commit - * - * This function commits a with drm_atomic_helper_check() pre-validated state - * object. This can still fail when e.g. the framebuffer reservation fails. - * - * RETURNS - * Zero for success or -errno. - */ -int msm_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock) -{ - struct msm_drm_private *priv = dev->dev_private; - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - struct drm_plane *plane; - struct drm_plane_state *old_plane_state, *new_plane_state; - int i, ret; - - /* - * Note that plane->atomic_async_check() should fail if we need - * to re-assign hwpipe or anything that touches global atomic - * state, so we'll never go down the async update path in those - * cases. - */ - if (state->async_update) { - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - drm_atomic_helper_async_commit(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); - return 0; - } - - ret = drm_atomic_helper_setup_commit(state, nonblock); - if (ret) - return ret; - - INIT_WORK(&state->commit_work, commit_work); - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - if (!nonblock) { - ret = drm_atomic_helper_wait_for_fences(dev, state, true); - if (ret) - goto error; - } - - /* - * This is the point of no return - everything below never fails except - * when the hw goes bonghits. Which means we can commit the new state on - * the software side now. - * - * swap driver private state while still holding state_lock - */ - BUG_ON(drm_atomic_helper_swap_state(state, true) < 0); - - /* - * Everything below can be run asynchronously without the need to grab - * any modeset locks at all under one conditions: It must be guaranteed - * that the asynchronous work has either been cancelled (if the driver - * supports it, which at least requires that the framebuffers get - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed - * before the new state gets committed on the software side with - * drm_atomic_helper_swap_state(). - * - * This scheme allows new atomic state updates to be prepared and - * checked in parallel to the asynchronous completion of the previous - * update. Which is important since compositors need to figure out the - * composition of the next frame right after having submitted the - * current layout. - */ - - drm_atomic_state_get(state); - if (nonblock) - queue_work(system_unbound_wq, &state->commit_work); - else - commit_tail(state); - - return 0; - -error: - drm_atomic_helper_cleanup_planes(dev, state); - return ret; -} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index d1a45a68c287..eb5569d4dd57 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -41,7 +41,11 @@ static const struct drm_mode_config_funcs mode_config_funcs = { .fb_create = msm_framebuffer_create, .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, - .atomic_commit = msm_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, +}; + +static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = { + .atomic_commit_tail = msm_atomic_commit_tail, }; static inline @@ -520,6 +524,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) } ddev->mode_config.funcs = &mode_config_funcs; + ddev->mode_config.helper_private = &mode_config_helper_funcs; ret = drm_vblank_init(ddev, priv->num_crtcs); if (ret < 0) { diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index b89fa5186eb8..3e89d7eac02f 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -156,8 +156,7 @@ struct msm_format { uint32_t pixel_format; }; -int msm_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock); +void msm_atomic_commit_tail(struct drm_atomic_state *state); void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, struct sg_table *sgt); -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] drm/msm: Switch to atomic_helper_commit() @ 2018-03-28 19:06 ` Sean Paul 0 siblings, 0 replies; 21+ messages in thread From: Sean Paul @ 2018-03-28 19:06 UTC (permalink / raw) To: freedreno, linux-arm-msm, dri-devel Cc: robdclark, hoegsberg, jsanka, abhinavk, architt, Sean Paul, linux-kernel Now that all of the msm-specific goo is tucked safely away we can switch over to using the atomic helper commit directly. \o/ Changes in v2: - None Cc: Abhinav Kumar <abhinavk@codeaurora.org> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/msm_atomic.c | 120 +------------------------------ drivers/gpu/drm/msm/msm_drv.c | 7 +- drivers/gpu/drm/msm/msm_drv.h | 3 +- 3 files changed, 8 insertions(+), 122 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 52249565cc03..d593ac3de1df 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -17,8 +17,6 @@ #include "msm_drv.h" #include "msm_kms.h" -#include "msm_gem.h" -#include "msm_fence.h" static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) @@ -37,7 +35,7 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, } } -static void msm_atomic_commit_tail(struct drm_atomic_state *state) +void msm_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct msm_drm_private *priv = dev->dev_private; @@ -74,119 +72,3 @@ static void msm_atomic_commit_tail(struct drm_atomic_state *state) drm_atomic_helper_cleanup_planes(dev, state); } - -/* The (potentially) asynchronous part of the commit. At this point - * nothing can fail short of armageddon. - */ -static void commit_tail(struct drm_atomic_state *state) -{ - drm_atomic_helper_wait_for_fences(state->dev, state, false); - - drm_atomic_helper_wait_for_dependencies(state); - - msm_atomic_commit_tail(state); - - drm_atomic_helper_commit_cleanup_done(state); - - drm_atomic_state_put(state); -} - -static void commit_work(struct work_struct *work) -{ - struct drm_atomic_state *state = container_of(work, - struct drm_atomic_state, - commit_work); - commit_tail(state); -} - -/** - * drm_atomic_helper_commit - commit validated state object - * @dev: DRM device - * @state: the driver state object - * @nonblock: nonblocking commit - * - * This function commits a with drm_atomic_helper_check() pre-validated state - * object. This can still fail when e.g. the framebuffer reservation fails. - * - * RETURNS - * Zero for success or -errno. - */ -int msm_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock) -{ - struct msm_drm_private *priv = dev->dev_private; - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - struct drm_plane *plane; - struct drm_plane_state *old_plane_state, *new_plane_state; - int i, ret; - - /* - * Note that plane->atomic_async_check() should fail if we need - * to re-assign hwpipe or anything that touches global atomic - * state, so we'll never go down the async update path in those - * cases. - */ - if (state->async_update) { - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - drm_atomic_helper_async_commit(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); - return 0; - } - - ret = drm_atomic_helper_setup_commit(state, nonblock); - if (ret) - return ret; - - INIT_WORK(&state->commit_work, commit_work); - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - if (!nonblock) { - ret = drm_atomic_helper_wait_for_fences(dev, state, true); - if (ret) - goto error; - } - - /* - * This is the point of no return - everything below never fails except - * when the hw goes bonghits. Which means we can commit the new state on - * the software side now. - * - * swap driver private state while still holding state_lock - */ - BUG_ON(drm_atomic_helper_swap_state(state, true) < 0); - - /* - * Everything below can be run asynchronously without the need to grab - * any modeset locks at all under one conditions: It must be guaranteed - * that the asynchronous work has either been cancelled (if the driver - * supports it, which at least requires that the framebuffers get - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed - * before the new state gets committed on the software side with - * drm_atomic_helper_swap_state(). - * - * This scheme allows new atomic state updates to be prepared and - * checked in parallel to the asynchronous completion of the previous - * update. Which is important since compositors need to figure out the - * composition of the next frame right after having submitted the - * current layout. - */ - - drm_atomic_state_get(state); - if (nonblock) - queue_work(system_unbound_wq, &state->commit_work); - else - commit_tail(state); - - return 0; - -error: - drm_atomic_helper_cleanup_planes(dev, state); - return ret; -} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index d1a45a68c287..eb5569d4dd57 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -41,7 +41,11 @@ static const struct drm_mode_config_funcs mode_config_funcs = { .fb_create = msm_framebuffer_create, .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = drm_atomic_helper_check, - .atomic_commit = msm_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, +}; + +static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = { + .atomic_commit_tail = msm_atomic_commit_tail, }; static inline @@ -520,6 +524,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) } ddev->mode_config.funcs = &mode_config_funcs; + ddev->mode_config.helper_private = &mode_config_helper_funcs; ret = drm_vblank_init(ddev, priv->num_crtcs); if (ret < 0) { diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index b89fa5186eb8..3e89d7eac02f 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -156,8 +156,7 @@ struct msm_format { uint32_t pixel_format; }; -int msm_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock); +void msm_atomic_commit_tail(struct drm_atomic_state *state); void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, struct sg_table *sgt); -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-04-02 5:23 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-28 19:06 [PATCH v2 0/6] drm/msm: Switch to atomic helpers Sean Paul 2018-03-28 19:06 ` [PATCH v2 1/6] drm/msm: Use drm_private_obj/state instead of subclassing Sean Paul 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-2-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-04-02 5:01 ` Archit Taneja 2018-04-02 5:01 ` Archit Taneja [not found] ` <20180328190657.218661-1-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-03-28 19:06 ` [PATCH v2 2/6] drm/msm: Refactor complete_commit() to look more the helpers Sean Paul 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-3-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-04-02 5:22 ` Archit Taneja 2018-04-02 5:22 ` Archit Taneja 2018-03-28 19:06 ` [PATCH v2 3/6] drm/msm: Mark the crtc->state->event consumed Sean Paul 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-4-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-04-02 5:22 ` Archit Taneja 2018-04-02 5:22 ` Archit Taneja 2018-03-28 19:06 ` [PATCH v2 4/6] drm/msm: Issue queued events when disabling crtc Sean Paul 2018-03-28 19:06 ` Sean Paul [not found] ` <20180328190657.218661-5-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 2018-04-02 5:23 ` Archit Taneja 2018-04-02 5:23 ` Archit Taneja 2018-03-28 19:06 ` [PATCH v2 5/6] drm/msm: Remove msm_commit/worker, use atomic helper commit Sean Paul 2018-03-28 19:06 ` Sean Paul 2018-03-28 19:06 ` [PATCH v2 6/6] drm/msm: Switch to atomic_helper_commit() Sean Paul 2018-03-28 19:06 ` Sean Paul
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.