| Message ID | 1484558003-18691-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Inki Dae wrote: > This patch relpaces specific atomic commit function > with atomic helper commit one, which also includes > atomic_commit_tail callback for Exynos SoC becasue > crtc devices on Exynos SoC uses power domain device > so drm_atomic_helper_commit_planes should be called > after drm_atomic_helper_commit_modeset_enables. The commit message needs fixing. I think I know my way around Exynos DRM a bit, but reading this just confuses me. In particular the first part can probably be dropped, since it only describes what the patch does (and I can already see this from the diff itself). Also some spelling issues: relpaces -> replaces becasue - because With best wishes, Tobias > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +------------------------------ > drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- > 3 files changed, 33 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index 2530bf5..47da612 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc) > > if (exynos_crtc->ops->disable) > exynos_crtc->ops->disable(exynos_crtc); > + > + if (crtc->state->event && !crtc->state->active) { > + spin_lock_irq(&crtc->dev->event_lock); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + spin_unlock_irq(&crtc->dev->event_lock); > + > + crtc->state->event = NULL; > + } > } > > static void > @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc, > drm_crtc_send_vblank_event(crtc, event); > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > } > - > } > > static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index 3ec0535..9d0df00 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -38,56 +38,6 @@ > #define DRIVER_MAJOR 1 > #define DRIVER_MINOR 0 > > -struct exynos_atomic_commit { > - struct work_struct work; > - struct drm_device *dev; > - struct drm_atomic_state *state; > - u32 crtcs; > -}; > - > -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) > -{ > - struct drm_device *dev = commit->dev; > - struct exynos_drm_private *priv = dev->dev_private; > - struct drm_atomic_state *state = commit->state; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ > - > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > - > - drm_atomic_state_put(state); > - > - spin_lock(&priv->lock); > - priv->pending &= ~commit->crtcs; > - spin_unlock(&priv->lock); > - > - wake_up_all(&priv->wait); > - > - kfree(commit); > -} > - > -static void exynos_drm_atomic_work(struct work_struct *work) > -{ > - struct exynos_atomic_commit *commit = container_of(work, > - struct exynos_atomic_commit, work); > - > - exynos_atomic_commit_complete(commit); > -} > - > static struct device *exynos_drm_get_dma_device(void); > > static int exynos_drm_load(struct drm_device *dev, unsigned long flags) > @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) > dev->dev_private = NULL; > } > > -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) > -{ > - bool pending; > - > - spin_lock(&priv->lock); > - pending = priv->pending & crtcs; > - spin_unlock(&priv->lock); > - > - return pending; > -} > - > -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, > - bool nonblock) > -{ > - struct exynos_drm_private *priv = dev->dev_private; > - struct exynos_atomic_commit *commit; > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - int i, ret; > - > - commit = kzalloc(sizeof(*commit), GFP_KERNEL); > - if (!commit) > - return -ENOMEM; > - > - ret = drm_atomic_helper_prepare_planes(dev, state); > - if (ret) { > - kfree(commit); > - return ret; > - } > - > - /* This is the point of no return */ > - > - INIT_WORK(&commit->work, exynos_drm_atomic_work); > - commit->dev = dev; > - commit->state = state; > - > - /* Wait until all affected CRTCs have completed previous commits and > - * mark them as pending. > - */ > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - commit->crtcs |= drm_crtc_mask(crtc); > - > - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); > - > - spin_lock(&priv->lock); > - priv->pending |= commit->crtcs; > - spin_unlock(&priv->lock); > - > - drm_atomic_helper_swap_state(state, true); > - > - drm_atomic_state_get(state); > - if (nonblock) > - schedule_work(&commit->work); > - else > - exynos_atomic_commit_complete(commit); > - > - return 0; > -} > - > int exynos_atomic_check(struct drm_device *dev, > struct drm_atomic_state *state) > { > @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > exynos_drm_crtc_cancel_page_flip(crtc, file); > + > } > > static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > index 68d4142..1e10b73 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > return exynos_fb->dma_addr[index]; > } > > +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > +{ > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, state); > + > + drm_atomic_helper_commit_modeset_enables(dev, state); > + > + drm_atomic_helper_commit_planes(dev, state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); > + > + drm_atomic_helper_commit_hw_done(state); > + > + drm_atomic_helper_wait_for_vblanks(dev, state); > + > + drm_atomic_helper_cleanup_planes(dev, state); > +} > + > +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { > + .atomic_commit_tail = exynos_drm_atomic_commit_tail, > +}; > + > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > .fb_create = exynos_user_fb_create, > .output_poll_changed = exynos_drm_output_poll_changed, > .atomic_check = exynos_atomic_check, > - .atomic_commit = exynos_atomic_commit, > + .atomic_commit = drm_atomic_helper_commit, > }; > > void exynos_drm_mode_config_init(struct drm_device *dev) > @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) > dev->mode_config.max_height = 4096; > > dev->mode_config.funcs = &exynos_drm_mode_config_funcs; > + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; > } >
Hi Inki, 2017-01-16 Inki Dae <inki.dae@samsung.com>: > This patch relpaces specific atomic commit function > with atomic helper commit one, which also includes > atomic_commit_tail callback for Exynos SoC becasue > crtc devices on Exynos SoC uses power domain device > so drm_atomic_helper_commit_planes should be called > after drm_atomic_helper_commit_modeset_enables. Indeed, the commit message needs fixing. > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +------------------------------ > drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- > 3 files changed, 33 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index 2530bf5..47da612 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc) > > if (exynos_crtc->ops->disable) > exynos_crtc->ops->disable(exynos_crtc); > + > + if (crtc->state->event && !crtc->state->active) { > + spin_lock_irq(&crtc->dev->event_lock); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + spin_unlock_irq(&crtc->dev->event_lock); > + > + crtc->state->event = NULL; > + } > } > > static void > @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc, > drm_crtc_send_vblank_event(crtc, event); > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > } > - Nitpick: I wouldn't include changes like this in the patch. > } > > static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index 3ec0535..9d0df00 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -38,56 +38,6 @@ > #define DRIVER_MAJOR 1 > #define DRIVER_MINOR 0 > > -struct exynos_atomic_commit { > - struct work_struct work; > - struct drm_device *dev; > - struct drm_atomic_state *state; > - u32 crtcs; > -}; > - > -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) > -{ > - struct drm_device *dev = commit->dev; > - struct exynos_drm_private *priv = dev->dev_private; > - struct drm_atomic_state *state = commit->state; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ Please move this comment to the commit_tail function instead of deleting it. > - > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > - > - drm_atomic_state_put(state); > - > - spin_lock(&priv->lock); > - priv->pending &= ~commit->crtcs; > - spin_unlock(&priv->lock); > - > - wake_up_all(&priv->wait); > - > - kfree(commit); > -} > - > -static void exynos_drm_atomic_work(struct work_struct *work) > -{ > - struct exynos_atomic_commit *commit = container_of(work, > - struct exynos_atomic_commit, work); > - > - exynos_atomic_commit_complete(commit); > -} > - > static struct device *exynos_drm_get_dma_device(void); > > static int exynos_drm_load(struct drm_device *dev, unsigned long flags) > @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) > dev->dev_private = NULL; > } > > -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) > -{ > - bool pending; > - > - spin_lock(&priv->lock); > - pending = priv->pending & crtcs; > - spin_unlock(&priv->lock); > - > - return pending; > -} > - > -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, > - bool nonblock) > -{ > - struct exynos_drm_private *priv = dev->dev_private; > - struct exynos_atomic_commit *commit; > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - int i, ret; > - > - commit = kzalloc(sizeof(*commit), GFP_KERNEL); > - if (!commit) > - return -ENOMEM; > - > - ret = drm_atomic_helper_prepare_planes(dev, state); > - if (ret) { > - kfree(commit); > - return ret; > - } > - > - /* This is the point of no return */ > - > - INIT_WORK(&commit->work, exynos_drm_atomic_work); > - commit->dev = dev; > - commit->state = state; > - > - /* Wait until all affected CRTCs have completed previous commits and > - * mark them as pending. > - */ > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - commit->crtcs |= drm_crtc_mask(crtc); > - > - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); > - > - spin_lock(&priv->lock); > - priv->pending |= commit->crtcs; > - spin_unlock(&priv->lock); > - > - drm_atomic_helper_swap_state(state, true); > - > - drm_atomic_state_get(state); > - if (nonblock) > - schedule_work(&commit->work); > - else > - exynos_atomic_commit_complete(commit); > - > - return 0; > -} > - > int exynos_atomic_check(struct drm_device *dev, > struct drm_atomic_state *state) > { > @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > exynos_drm_crtc_cancel_page_flip(crtc, file); > + This change shouldn't be here too. > } > > static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > index 68d4142..1e10b73 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > return exynos_fb->dma_addr[index]; > } > > +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > +{ > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, state); > + > + drm_atomic_helper_commit_modeset_enables(dev, state); > + > + drm_atomic_helper_commit_planes(dev, state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); > + > + drm_atomic_helper_commit_hw_done(state); > + > + drm_atomic_helper_wait_for_vblanks(dev, state); > + > + drm_atomic_helper_cleanup_planes(dev, state); > +} > + > +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { > + .atomic_commit_tail = exynos_drm_atomic_commit_tail, > +}; > + > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > .fb_create = exynos_user_fb_create, > .output_poll_changed = exynos_drm_output_poll_changed, > .atomic_check = exynos_atomic_check, > - .atomic_commit = exynos_atomic_commit, > + .atomic_commit = drm_atomic_helper_commit, > }; > > void exynos_drm_mode_config_init(struct drm_device *dev) > @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) > dev->mode_config.max_height = 4096; > > dev->mode_config.funcs = &exynos_drm_mode_config_funcs; > + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; With all that fixed: Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Gustavo
Hi Inki, Thank you for the patch. On Monday 16 Jan 2017 18:13:22 Inki Dae wrote: > This patch relpaces specific atomic commit function > with atomic helper commit one, which also includes > atomic_commit_tail callback for Exynos SoC becasue > crtc devices on Exynos SoC uses power domain device > so drm_atomic_helper_commit_planes should be called > after drm_atomic_helper_commit_modeset_enables. Please note that drm_atomic_helper_commit() is currently broken, its async commit support is subject to a race condition. Maarten's "[PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state" patch series is an attempt to fix that, I'll try to review it ASAP. > Signed-off-by: Inki Dae <inki.dae@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +--------------------------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- > 3 files changed, 33 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc > *crtc) > > if (exynos_crtc->ops->disable) > exynos_crtc->ops->disable(exynos_crtc); > + > + if (crtc->state->event && !crtc->state->active) { > + spin_lock_irq(&crtc->dev->event_lock); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + spin_unlock_irq(&crtc->dev->event_lock); > + > + crtc->state->event = NULL; > + } > } > > static void > @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc > *crtc, drm_crtc_send_vblank_event(crtc, event); > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > } > - > } > > static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -38,56 +38,6 @@ > #define DRIVER_MAJOR 1 > #define DRIVER_MINOR 0 > > -struct exynos_atomic_commit { > - struct work_struct work; > - struct drm_device *dev; > - struct drm_atomic_state *state; > - u32 crtcs; > -}; > - > -static void exynos_atomic_commit_complete(struct exynos_atomic_commit > *commit) -{ > - struct drm_device *dev = commit->dev; > - struct exynos_drm_private *priv = dev->dev_private; > - struct drm_atomic_state *state = commit->state; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ > - > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > - > - drm_atomic_state_put(state); > - > - spin_lock(&priv->lock); > - priv->pending &= ~commit->crtcs; > - spin_unlock(&priv->lock); > - > - wake_up_all(&priv->wait); > - > - kfree(commit); > -} > - > -static void exynos_drm_atomic_work(struct work_struct *work) > -{ > - struct exynos_atomic_commit *commit = container_of(work, > - struct exynos_atomic_commit, work); > - > - exynos_atomic_commit_complete(commit); > -} > - > static struct device *exynos_drm_get_dma_device(void); > > static int exynos_drm_load(struct drm_device *dev, unsigned long flags) > @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) > dev->dev_private = NULL; > } > > -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) > -{ > - bool pending; > - > - spin_lock(&priv->lock); > - pending = priv->pending & crtcs; > - spin_unlock(&priv->lock); > - > - return pending; > -} > - > -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state > *state, - bool nonblock) > -{ > - struct exynos_drm_private *priv = dev->dev_private; > - struct exynos_atomic_commit *commit; > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - int i, ret; > - > - commit = kzalloc(sizeof(*commit), GFP_KERNEL); > - if (!commit) > - return -ENOMEM; > - > - ret = drm_atomic_helper_prepare_planes(dev, state); > - if (ret) { > - kfree(commit); > - return ret; > - } > - > - /* This is the point of no return */ > - > - INIT_WORK(&commit->work, exynos_drm_atomic_work); > - commit->dev = dev; > - commit->state = state; > - > - /* Wait until all affected CRTCs have completed previous commits and > - * mark them as pending. > - */ > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - commit->crtcs |= drm_crtc_mask(crtc); > - > - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); > - > - spin_lock(&priv->lock); > - priv->pending |= commit->crtcs; > - spin_unlock(&priv->lock); > - > - drm_atomic_helper_swap_state(state, true); > - > - drm_atomic_state_get(state); > - if (nonblock) > - schedule_work(&commit->work); > - else > - exynos_atomic_commit_complete(commit); > - > - return 0; > -} > - > int exynos_atomic_check(struct drm_device *dev, > struct drm_atomic_state *state) > { > @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > exynos_drm_crtc_cancel_page_flip(crtc, file); > + > } > > static void exynos_drm_postclose(struct drm_device *dev, struct drm_file > *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c > b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct > drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; > } > > +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > +{ > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, state); > + > + drm_atomic_helper_commit_modeset_enables(dev, state); > + > + drm_atomic_helper_commit_planes(dev, state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); > + > + drm_atomic_helper_commit_hw_done(state); > + > + drm_atomic_helper_wait_for_vblanks(dev, state); > + > + drm_atomic_helper_cleanup_planes(dev, state); > +} > + > +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = > { + .atomic_commit_tail = exynos_drm_atomic_commit_tail, > +}; > + > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > .fb_create = exynos_user_fb_create, > .output_poll_changed = exynos_drm_output_poll_changed, > .atomic_check = exynos_atomic_check, > - .atomic_commit = exynos_atomic_commit, > + .atomic_commit = drm_atomic_helper_commit, > }; > > void exynos_drm_mode_config_init(struct drm_device *dev) > @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) > dev->mode_config.max_height = 4096; > > dev->mode_config.funcs = &exynos_drm_mode_config_funcs; > + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; > }
2017년 01월 16일 20:22에 Tobias Jakobi 이(가) 쓴 글: > Inki Dae wrote: >> This patch relpaces specific atomic commit function >> with atomic helper commit one, which also includes >> atomic_commit_tail callback for Exynos SoC becasue >> crtc devices on Exynos SoC uses power domain device >> so drm_atomic_helper_commit_planes should be called >> after drm_atomic_helper_commit_modeset_enables. > The commit message needs fixing. I think I know my way around Exynos DRM > a bit, but reading this just confuses me. Seems I didn't describe it. Thanks for pointing. Will fix it. Thanks. > > In particular the first part can probably be dropped, since it only > describes what the patch does (and I can already see this from the diff > itself). > Also some spelling issues: > relpaces -> replaces > becasue - because > > With best wishes, > Tobias > > >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +------------------------------ >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- >> 3 files changed, 33 insertions(+), 111 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> index 2530bf5..47da612 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc) >> >> if (exynos_crtc->ops->disable) >> exynos_crtc->ops->disable(exynos_crtc); >> + >> + if (crtc->state->event && !crtc->state->active) { >> + spin_lock_irq(&crtc->dev->event_lock); >> + drm_crtc_send_vblank_event(crtc, crtc->state->event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + >> + crtc->state->event = NULL; >> + } >> } >> >> static void >> @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc, >> drm_crtc_send_vblank_event(crtc, event); >> spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> } >> - >> } >> >> static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> index 3ec0535..9d0df00 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> @@ -38,56 +38,6 @@ >> #define DRIVER_MAJOR 1 >> #define DRIVER_MINOR 0 >> >> -struct exynos_atomic_commit { >> - struct work_struct work; >> - struct drm_device *dev; >> - struct drm_atomic_state *state; >> - u32 crtcs; >> -}; >> - >> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) >> -{ >> - struct drm_device *dev = commit->dev; >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct drm_atomic_state *state = commit->state; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - /* >> - * Exynos can't update planes with CRTCs and encoders disabled, >> - * its updates routines, specially for FIMD, requires the clocks >> - * to be enabled. So it is necessary to handle the modeset operations >> - * *before* the commit_planes() step, this way it will always >> - * have the relevant clocks enabled to perform the update. >> - */ >> - >> - drm_atomic_helper_commit_planes(dev, state, 0); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> - >> - drm_atomic_state_put(state); >> - >> - spin_lock(&priv->lock); >> - priv->pending &= ~commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - wake_up_all(&priv->wait); >> - >> - kfree(commit); >> -} >> - >> -static void exynos_drm_atomic_work(struct work_struct *work) >> -{ >> - struct exynos_atomic_commit *commit = container_of(work, >> - struct exynos_atomic_commit, work); >> - >> - exynos_atomic_commit_complete(commit); >> -} >> - >> static struct device *exynos_drm_get_dma_device(void); >> >> static int exynos_drm_load(struct drm_device *dev, unsigned long flags) >> @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) >> dev->dev_private = NULL; >> } >> >> -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) >> -{ >> - bool pending; >> - >> - spin_lock(&priv->lock); >> - pending = priv->pending & crtcs; >> - spin_unlock(&priv->lock); >> - >> - return pending; >> -} >> - >> -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, >> - bool nonblock) >> -{ >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct exynos_atomic_commit *commit; >> - struct drm_crtc *crtc; >> - struct drm_crtc_state *crtc_state; >> - int i, ret; >> - >> - commit = kzalloc(sizeof(*commit), GFP_KERNEL); >> - if (!commit) >> - return -ENOMEM; >> - >> - ret = drm_atomic_helper_prepare_planes(dev, state); >> - if (ret) { >> - kfree(commit); >> - return ret; >> - } >> - >> - /* This is the point of no return */ >> - >> - INIT_WORK(&commit->work, exynos_drm_atomic_work); >> - commit->dev = dev; >> - commit->state = state; >> - >> - /* Wait until all affected CRTCs have completed previous commits and >> - * mark them as pending. >> - */ >> - for_each_crtc_in_state(state, crtc, crtc_state, i) >> - commit->crtcs |= drm_crtc_mask(crtc); >> - >> - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); >> - >> - spin_lock(&priv->lock); >> - priv->pending |= commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - drm_atomic_helper_swap_state(state, true); >> - >> - drm_atomic_state_get(state); >> - if (nonblock) >> - schedule_work(&commit->work); >> - else >> - exynos_atomic_commit_complete(commit); >> - >> - return 0; >> -} >> - >> int exynos_atomic_check(struct drm_device *dev, >> struct drm_atomic_state *state) >> { >> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> exynos_drm_crtc_cancel_page_flip(crtc, file); >> + >> } >> >> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> index 68d4142..1e10b73 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) >> return exynos_fb->dma_addr[index]; >> } >> >> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) >> +{ >> + struct drm_device *dev = state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, state); >> + >> + drm_atomic_helper_commit_planes(dev, state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); >> + >> + drm_atomic_helper_commit_hw_done(state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, state); >> + >> + drm_atomic_helper_cleanup_planes(dev, state); >> +} >> + >> +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { >> + .atomic_commit_tail = exynos_drm_atomic_commit_tail, >> +}; >> + >> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { >> .fb_create = exynos_user_fb_create, >> .output_poll_changed = exynos_drm_output_poll_changed, >> .atomic_check = exynos_atomic_check, >> - .atomic_commit = exynos_atomic_commit, >> + .atomic_commit = drm_atomic_helper_commit, >> }; >> >> void exynos_drm_mode_config_init(struct drm_device *dev) >> @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) >> dev->mode_config.max_height = 4096; >> >> dev->mode_config.funcs = &exynos_drm_mode_config_funcs; >> + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; >> } >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
2017년 01월 17일 05:34에 Laurent Pinchart 이(가) 쓴 글: > Hi Inki, > > Thank you for the patch. > > On Monday 16 Jan 2017 18:13:22 Inki Dae wrote: >> This patch relpaces specific atomic commit function >> with atomic helper commit one, which also includes >> atomic_commit_tail callback for Exynos SoC becasue >> crtc devices on Exynos SoC uses power domain device >> so drm_atomic_helper_commit_planes should be called >> after drm_atomic_helper_commit_modeset_enables. > > Please note that drm_atomic_helper_commit() is currently broken, its async > commit support is subject to a race condition. Maarten's "[PATCH v3 0/7] > drm/atomic: Add accessor macros for all atomic state" patch series is an > attempt to fix that, I'll try to review it ASAP. Thanks for share and if you give me a review, it would help me a lot. > >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +--------------------------- >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- >> 3 files changed, 33 insertions(+), 111 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc >> *crtc) >> >> if (exynos_crtc->ops->disable) >> exynos_crtc->ops->disable(exynos_crtc); >> + >> + if (crtc->state->event && !crtc->state->active) { >> + spin_lock_irq(&crtc->dev->event_lock); >> + drm_crtc_send_vblank_event(crtc, crtc->state->event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + >> + crtc->state->event = NULL; >> + } >> } >> >> static void >> @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc >> *crtc, drm_crtc_send_vblank_event(crtc, event); >> spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> } >> - >> } >> >> static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> @@ -38,56 +38,6 @@ >> #define DRIVER_MAJOR 1 >> #define DRIVER_MINOR 0 >> >> -struct exynos_atomic_commit { >> - struct work_struct work; >> - struct drm_device *dev; >> - struct drm_atomic_state *state; >> - u32 crtcs; >> -}; >> - >> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit >> *commit) -{ >> - struct drm_device *dev = commit->dev; >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct drm_atomic_state *state = commit->state; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - /* >> - * Exynos can't update planes with CRTCs and encoders disabled, >> - * its updates routines, specially for FIMD, requires the clocks >> - * to be enabled. So it is necessary to handle the modeset operations >> - * *before* the commit_planes() step, this way it will always >> - * have the relevant clocks enabled to perform the update. >> - */ >> - >> - drm_atomic_helper_commit_planes(dev, state, 0); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> - >> - drm_atomic_state_put(state); >> - >> - spin_lock(&priv->lock); >> - priv->pending &= ~commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - wake_up_all(&priv->wait); >> - >> - kfree(commit); >> -} >> - >> -static void exynos_drm_atomic_work(struct work_struct *work) >> -{ >> - struct exynos_atomic_commit *commit = container_of(work, >> - struct exynos_atomic_commit, work); >> - >> - exynos_atomic_commit_complete(commit); >> -} >> - >> static struct device *exynos_drm_get_dma_device(void); >> >> static int exynos_drm_load(struct drm_device *dev, unsigned long flags) >> @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) >> dev->dev_private = NULL; >> } >> >> -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) >> -{ >> - bool pending; >> - >> - spin_lock(&priv->lock); >> - pending = priv->pending & crtcs; >> - spin_unlock(&priv->lock); >> - >> - return pending; >> -} >> - >> -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state >> *state, - bool nonblock) >> -{ >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct exynos_atomic_commit *commit; >> - struct drm_crtc *crtc; >> - struct drm_crtc_state *crtc_state; >> - int i, ret; >> - >> - commit = kzalloc(sizeof(*commit), GFP_KERNEL); >> - if (!commit) >> - return -ENOMEM; >> - >> - ret = drm_atomic_helper_prepare_planes(dev, state); >> - if (ret) { >> - kfree(commit); >> - return ret; >> - } >> - >> - /* This is the point of no return */ >> - >> - INIT_WORK(&commit->work, exynos_drm_atomic_work); >> - commit->dev = dev; >> - commit->state = state; >> - >> - /* Wait until all affected CRTCs have completed previous commits and >> - * mark them as pending. >> - */ >> - for_each_crtc_in_state(state, crtc, crtc_state, i) >> - commit->crtcs |= drm_crtc_mask(crtc); >> - >> - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); >> - >> - spin_lock(&priv->lock); >> - priv->pending |= commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - drm_atomic_helper_swap_state(state, true); >> - >> - drm_atomic_state_get(state); >> - if (nonblock) >> - schedule_work(&commit->work); >> - else >> - exynos_atomic_commit_complete(commit); >> - >> - return 0; >> -} >> - >> int exynos_atomic_check(struct drm_device *dev, >> struct drm_atomic_state *state) >> { >> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> exynos_drm_crtc_cancel_page_flip(crtc, file); >> + >> } >> >> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file >> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct >> drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; >> } >> >> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) >> +{ >> + struct drm_device *dev = state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, state); >> + >> + drm_atomic_helper_commit_planes(dev, state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); >> + >> + drm_atomic_helper_commit_hw_done(state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, state); >> + >> + drm_atomic_helper_cleanup_planes(dev, state); >> +} >> + >> +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = >> { + .atomic_commit_tail = exynos_drm_atomic_commit_tail, >> +}; >> + >> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { >> .fb_create = exynos_user_fb_create, >> .output_poll_changed = exynos_drm_output_poll_changed, >> .atomic_check = exynos_atomic_check, >> - .atomic_commit = exynos_atomic_commit, >> + .atomic_commit = drm_atomic_helper_commit, >> }; >> >> void exynos_drm_mode_config_init(struct drm_device *dev) >> @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) >> dev->mode_config.max_height = 4096; >> >> dev->mode_config.funcs = &exynos_drm_mode_config_funcs; >> + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; >> } >
2017년 01월 17일 04:08에 Gustavo Padovan 이(가) 쓴 글: > Hi Inki, > > 2017-01-16 Inki Dae <inki.dae@samsung.com>: > >> This patch relpaces specific atomic commit function >> with atomic helper commit one, which also includes >> atomic_commit_tail callback for Exynos SoC becasue >> crtc devices on Exynos SoC uses power domain device >> so drm_atomic_helper_commit_planes should be called >> after drm_atomic_helper_commit_modeset_enables. > > Indeed, the commit message needs fixing. Right, above comment should be fixed. I will update the comment like below, "because default implemention of atomic helper doesn't mesh well with runtime PM so The device driver which supports runtime PM should call drm_atomic_helper_commit_modeset_enables function before drm_atomic_helper_commit_planes function is called. atomic_commit_tail callback implements this call ordering." > >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +------------------------------ >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- >> 3 files changed, 33 insertions(+), 111 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> index 2530bf5..47da612 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc) >> >> if (exynos_crtc->ops->disable) >> exynos_crtc->ops->disable(exynos_crtc); >> + >> + if (crtc->state->event && !crtc->state->active) { >> + spin_lock_irq(&crtc->dev->event_lock); >> + drm_crtc_send_vblank_event(crtc, crtc->state->event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + >> + crtc->state->event = NULL; >> + } >> } >> >> static void >> @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc, >> drm_crtc_send_vblank_event(crtc, event); >> spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> } >> - > > Nitpick: I wouldn't include changes like this in the patch. > >> } >> >> static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> index 3ec0535..9d0df00 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> @@ -38,56 +38,6 @@ >> #define DRIVER_MAJOR 1 >> #define DRIVER_MINOR 0 >> >> -struct exynos_atomic_commit { >> - struct work_struct work; >> - struct drm_device *dev; >> - struct drm_atomic_state *state; >> - u32 crtcs; >> -}; >> - >> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) >> -{ >> - struct drm_device *dev = commit->dev; >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct drm_atomic_state *state = commit->state; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - /* >> - * Exynos can't update planes with CRTCs and encoders disabled, >> - * its updates routines, specially for FIMD, requires the clocks >> - * to be enabled. So it is necessary to handle the modeset operations >> - * *before* the commit_planes() step, this way it will always >> - * have the relevant clocks enabled to perform the update. >> - */ > > Please move this comment to the commit_tail function instead of deleting > it. I'm not sure why we have to keep above comment becuase this is why atomic_commit_tail callback is added. For this, I will leave this as a description of this patch like I already left my comment above. > >> - >> - drm_atomic_helper_commit_planes(dev, state, 0); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> - >> - drm_atomic_state_put(state); >> - >> - spin_lock(&priv->lock); >> - priv->pending &= ~commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - wake_up_all(&priv->wait); >> - >> - kfree(commit); >> -} >> - >> -static void exynos_drm_atomic_work(struct work_struct *work) >> -{ >> - struct exynos_atomic_commit *commit = container_of(work, >> - struct exynos_atomic_commit, work); >> - >> - exynos_atomic_commit_complete(commit); >> -} >> - >> static struct device *exynos_drm_get_dma_device(void); >> >> static int exynos_drm_load(struct drm_device *dev, unsigned long flags) >> @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) >> dev->dev_private = NULL; >> } >> >> -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) >> -{ >> - bool pending; >> - >> - spin_lock(&priv->lock); >> - pending = priv->pending & crtcs; >> - spin_unlock(&priv->lock); >> - >> - return pending; >> -} >> - >> -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, >> - bool nonblock) >> -{ >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct exynos_atomic_commit *commit; >> - struct drm_crtc *crtc; >> - struct drm_crtc_state *crtc_state; >> - int i, ret; >> - >> - commit = kzalloc(sizeof(*commit), GFP_KERNEL); >> - if (!commit) >> - return -ENOMEM; >> - >> - ret = drm_atomic_helper_prepare_planes(dev, state); >> - if (ret) { >> - kfree(commit); >> - return ret; >> - } >> - >> - /* This is the point of no return */ >> - >> - INIT_WORK(&commit->work, exynos_drm_atomic_work); >> - commit->dev = dev; >> - commit->state = state; >> - >> - /* Wait until all affected CRTCs have completed previous commits and >> - * mark them as pending. >> - */ >> - for_each_crtc_in_state(state, crtc, crtc_state, i) >> - commit->crtcs |= drm_crtc_mask(crtc); >> - >> - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); >> - >> - spin_lock(&priv->lock); >> - priv->pending |= commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - drm_atomic_helper_swap_state(state, true); >> - >> - drm_atomic_state_get(state); >> - if (nonblock) >> - schedule_work(&commit->work); >> - else >> - exynos_atomic_commit_complete(commit); >> - >> - return 0; >> -} >> - >> int exynos_atomic_check(struct drm_device *dev, >> struct drm_atomic_state *state) >> { >> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> exynos_drm_crtc_cancel_page_flip(crtc, file); >> + > > This change shouldn't be here too. > >> } >> >> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> index 68d4142..1e10b73 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) >> return exynos_fb->dma_addr[index]; >> } >> >> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) >> +{ >> + struct drm_device *dev = state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, state); >> + >> + drm_atomic_helper_commit_planes(dev, state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); >> + >> + drm_atomic_helper_commit_hw_done(state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, state); >> + >> + drm_atomic_helper_cleanup_planes(dev, state); >> +} >> + >> +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { >> + .atomic_commit_tail = exynos_drm_atomic_commit_tail, >> +}; >> + >> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { >> .fb_create = exynos_user_fb_create, >> .output_poll_changed = exynos_drm_output_poll_changed, >> .atomic_check = exynos_atomic_check, >> - .atomic_commit = exynos_atomic_commit, >> + .atomic_commit = drm_atomic_helper_commit, >> }; >> >> void exynos_drm_mode_config_init(struct drm_device *dev) >> @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) >> dev->mode_config.max_height = 4096; >> >> dev->mode_config.funcs = &exynos_drm_mode_config_funcs; >> + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; > > With all that fixed: > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Thanks. > > Gustavo > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hi Inki, Thank you for the patch. On Monday 16 Jan 2017 18:13:22 Inki Dae wrote: > This patch relpaces specific atomic commit function > with atomic helper commit one, which also includes > atomic_commit_tail callback for Exynos SoC becasue > crtc devices on Exynos SoC uses power domain device > so drm_atomic_helper_commit_planes should be called > after drm_atomic_helper_commit_modeset_enables. > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +--------------------------- > drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- > 3 files changed, 33 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc > *crtc) > > if (exynos_crtc->ops->disable) > exynos_crtc->ops->disable(exynos_crtc); > + > + if (crtc->state->event && !crtc->state->active) { > + spin_lock_irq(&crtc->dev->event_lock); > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > + spin_unlock_irq(&crtc->dev->event_lock); > + > + crtc->state->event = NULL; > + } You also need to handle events for exynos_drm_crtc_enable(). I'm not too familiar with the exynos drm driver, is that taken care of by event handling in exynos_crtc_atomic_flush() ? You could also split this change into a separate patch with a clear explanation of why it's needed in the commit message. > } [snip] > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c [snip] > -static void exynos_atomic_commit_complete(struct exynos_atomic_commit > *commit) > -{ > - struct drm_device *dev = commit->dev; > - struct exynos_drm_private *priv = dev->dev_private; > - struct drm_atomic_state *state = commit->state; > - > - drm_atomic_helper_commit_modeset_disables(dev, state); > - > - drm_atomic_helper_commit_modeset_enables(dev, state); > - > - /* > - * Exynos can't update planes with CRTCs and encoders disabled, > - * its updates routines, specially for FIMD, requires the clocks > - * to be enabled. So it is necessary to handle the modeset operations > - * *before* the commit_planes() step, this way it will always > - * have the relevant clocks enabled to perform the update. > - */ There's a value in this comment, I would copy it to exynos_drm_atomic_commit_tail() > - drm_atomic_helper_commit_planes(dev, state, 0); > - > - drm_atomic_helper_wait_for_vblanks(dev, state); > - > - drm_atomic_helper_cleanup_planes(dev, state); > - > - drm_atomic_state_put(state); > - > - spin_lock(&priv->lock); > - priv->pending &= ~commit->crtcs; > - spin_unlock(&priv->lock); > - > - wake_up_all(&priv->wait); > - > - kfree(commit); > -} [snip] > @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > exynos_drm_crtc_cancel_page_flip(crtc, file); > + This isn't needed. > } > > static void exynos_drm_postclose(struct drm_device *dev, struct drm_file > *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c > b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct > drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; > } > > +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) > +{ > + struct drm_device *dev = state->dev; > + > + drm_atomic_helper_commit_modeset_disables(dev, state); > + > + drm_atomic_helper_commit_modeset_enables(dev, state); > + > + drm_atomic_helper_commit_planes(dev, state, > + DRM_PLANE_COMMIT_ACTIVE_ONLY); The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change should go in a separate patch (or at least be documented in the commit message). > + drm_atomic_helper_commit_hw_done(state); > + > + drm_atomic_helper_wait_for_vblanks(dev, state); > + > + drm_atomic_helper_cleanup_planes(dev, state); > +} [snip]
Hi Laurent, 2017년 01월 17일 18:17에 Laurent Pinchart 이(가) 쓴 글: > Hi Inki, > > Thank you for the patch. > > On Monday 16 Jan 2017 18:13:22 Inki Dae wrote: >> This patch relpaces specific atomic commit function >> with atomic helper commit one, which also includes >> atomic_commit_tail callback for Exynos SoC becasue >> crtc devices on Exynos SoC uses power domain device >> so drm_atomic_helper_commit_planes should be called >> after drm_atomic_helper_commit_modeset_enables. >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +--------------------------- >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- >> 3 files changed, 33 insertions(+), 111 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc >> *crtc) >> >> if (exynos_crtc->ops->disable) >> exynos_crtc->ops->disable(exynos_crtc); >> + >> + if (crtc->state->event && !crtc->state->active) { >> + spin_lock_irq(&crtc->dev->event_lock); >> + drm_crtc_send_vblank_event(crtc, crtc->state->event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + >> + crtc->state->event = NULL; >> + } > > You also need to handle events for exynos_drm_crtc_enable(). I'm not too Is there any corner case? I think there should be no event which is not handled when exynos_drm_crtc_enable() is called. > familiar with the exynos drm driver, is that taken care of by event handling > in exynos_crtc_atomic_flush() ? Yes, exynos_crtc_atomic_flush() handles the event - making crtc->state->event to NULL and handing the event. However, I think no need to handle the event at this funtion so relevant code in exynos_crtc_ctomic_flush() is not clear to me excepting setting crtc->state->event to NULL - without this code, hw_done function will say warning. This patch would be a first step to clean up atomic interfaces of Exynos DRM driver. > > You could also split this change into a separate patch with a clear > explanation of why it's needed in the commit message. > >> } > > [snip] > >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > [snip] > >> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit >> *commit) >> -{ >> - struct drm_device *dev = commit->dev; >> - struct exynos_drm_private *priv = dev->dev_private; >> - struct drm_atomic_state *state = commit->state; >> - >> - drm_atomic_helper_commit_modeset_disables(dev, state); >> - >> - drm_atomic_helper_commit_modeset_enables(dev, state); >> - >> - /* >> - * Exynos can't update planes with CRTCs and encoders disabled, >> - * its updates routines, specially for FIMD, requires the clocks >> - * to be enabled. So it is necessary to handle the modeset operations >> - * *before* the commit_planes() step, this way it will always >> - * have the relevant clocks enabled to perform the update. >> - */ > > There's a value in this comment, I would copy it to > exynos_drm_atomic_commit_tail() As I already left the comment at other email thread, I will update description of this patch instead because atomic kms helper already described why atomic_commit_tail callback is required - the driver using runtime PM should have different call order. But.. ok, this is what Gustavo wanted also. Seems better to leave above comment because the more explanation, the better. > >> - drm_atomic_helper_commit_planes(dev, state, 0); >> - >> - drm_atomic_helper_wait_for_vblanks(dev, state); >> - >> - drm_atomic_helper_cleanup_planes(dev, state); >> - >> - drm_atomic_state_put(state); >> - >> - spin_lock(&priv->lock); >> - priv->pending &= ~commit->crtcs; >> - spin_unlock(&priv->lock); >> - >> - wake_up_all(&priv->wait); >> - >> - kfree(commit); >> -} > > [snip] > >> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> exynos_drm_crtc_cancel_page_flip(crtc, file); >> + > > This isn't needed. > >> } >> >> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file >> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c >> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct >> drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; >> } >> >> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) >> +{ >> + struct drm_device *dev = state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, state); >> + >> + drm_atomic_helper_commit_planes(dev, state, >> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > > The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change > should go in a separate patch (or at least be documented in the commit > message). Agree. I will update description of this patch about why DRM_PLANE_COMMIT_ACTIVE_ONLY flag is used. Thanks. > >> + drm_atomic_helper_commit_hw_done(state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, state); >> + >> + drm_atomic_helper_cleanup_planes(dev, state); >> +} > > [snip] >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc) if (exynos_crtc->ops->disable) exynos_crtc->ops->disable(exynos_crtc); + + if (crtc->state->event && !crtc->state->active) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + + crtc->state->event = NULL; + } } static void @@ -94,7 +102,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc, drm_crtc_send_vblank_event(crtc, event); spin_unlock_irqrestore(&crtc->dev->event_lock, flags); } - } static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = { diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -38,56 +38,6 @@ #define DRIVER_MAJOR 1 #define DRIVER_MINOR 0 -struct exynos_atomic_commit { - struct work_struct work; - struct drm_device *dev; - struct drm_atomic_state *state; - u32 crtcs; -}; - -static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) -{ - struct drm_device *dev = commit->dev; - struct exynos_drm_private *priv = dev->dev_private; - struct drm_atomic_state *state = commit->state; - - drm_atomic_helper_commit_modeset_disables(dev, state); - - drm_atomic_helper_commit_modeset_enables(dev, state); - - /* - * Exynos can't update planes with CRTCs and encoders disabled, - * its updates routines, specially for FIMD, requires the clocks - * to be enabled. So it is necessary to handle the modeset operations - * *before* the commit_planes() step, this way it will always - * have the relevant clocks enabled to perform the update. - */ - - drm_atomic_helper_commit_planes(dev, state, 0); - - drm_atomic_helper_wait_for_vblanks(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); - - drm_atomic_state_put(state); - - spin_lock(&priv->lock); - priv->pending &= ~commit->crtcs; - spin_unlock(&priv->lock); - - wake_up_all(&priv->wait); - - kfree(commit); -} - -static void exynos_drm_atomic_work(struct work_struct *work) -{ - struct exynos_atomic_commit *commit = container_of(work, - struct exynos_atomic_commit, work); - - exynos_atomic_commit_complete(commit); -} - static struct device *exynos_drm_get_dma_device(void); static int exynos_drm_load(struct drm_device *dev, unsigned long flags) @@ -202,65 +152,6 @@ static void exynos_drm_unload(struct drm_device *dev) dev->dev_private = NULL; } -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs) -{ - bool pending; - - spin_lock(&priv->lock); - pending = priv->pending & crtcs; - spin_unlock(&priv->lock); - - return pending; -} - -int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, - bool nonblock) -{ - struct exynos_drm_private *priv = dev->dev_private; - struct exynos_atomic_commit *commit; - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - int i, ret; - - commit = kzalloc(sizeof(*commit), GFP_KERNEL); - if (!commit) - return -ENOMEM; - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) { - kfree(commit); - return ret; - } - - /* This is the point of no return */ - - INIT_WORK(&commit->work, exynos_drm_atomic_work); - commit->dev = dev; - commit->state = state; - - /* Wait until all affected CRTCs have completed previous commits and - * mark them as pending. - */ - for_each_crtc_in_state(state, crtc, crtc_state, i) - commit->crtcs |= drm_crtc_mask(crtc); - - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs)); - - spin_lock(&priv->lock); - priv->pending |= commit->crtcs; - spin_unlock(&priv->lock); - - drm_atomic_helper_swap_state(state, true); - - drm_atomic_state_get(state); - if (nonblock) - schedule_work(&commit->work); - else - exynos_atomic_commit_complete(commit); - - return 0; -} - int exynos_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev, list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) exynos_drm_crtc_cancel_page_flip(crtc, file); + } static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index]; } +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state) +{ + struct drm_device *dev = state->dev; + + drm_atomic_helper_commit_modeset_disables(dev, state); + + drm_atomic_helper_commit_modeset_enables(dev, state); + + drm_atomic_helper_commit_planes(dev, state, + DRM_PLANE_COMMIT_ACTIVE_ONLY); + + drm_atomic_helper_commit_hw_done(state); + + drm_atomic_helper_wait_for_vblanks(dev, state); + + drm_atomic_helper_cleanup_planes(dev, state); +} + +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { + .atomic_commit_tail = exynos_drm_atomic_commit_tail, +}; + static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { .fb_create = exynos_user_fb_create, .output_poll_changed = exynos_drm_output_poll_changed, .atomic_check = exynos_atomic_check, - .atomic_commit = exynos_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, }; void exynos_drm_mode_config_init(struct drm_device *dev) @@ -208,4 +230,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev) dev->mode_config.max_height = 4096; dev->mode_config.funcs = &exynos_drm_mode_config_funcs; + dev->mode_config.helper_private = &exynos_drm_mode_config_helpers; }
This patch relpaces specific atomic commit function with atomic helper commit one, which also includes atomic_commit_tail callback for Exynos SoC becasue crtc devices on Exynos SoC uses power domain device so drm_atomic_helper_commit_planes should be called after drm_atomic_helper_commit_modeset_enables. Signed-off-by: Inki Dae <inki.dae@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++- drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +------------------------------ drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++- 3 files changed, 33 insertions(+), 111 deletions(-)