Re: [PATCH] drm/exynos: use atomic helper commit
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 >> --- >> 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[inde
Re: [PATCH] drm/exynos: use atomic helper commit
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 > --- > 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] -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: use atomic helper commit
2017년 01월 17일 04:08에 Gustavo Padovan 이(가) 쓴 글: > Hi Inki, > > 2017-01-16 Inki Dae : > >> 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 >> --- >> 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_MAJOR1 >> #define DRIVER_MINOR0 >> >> -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-
Re: [PATCH] drm/exynos: use atomic helper commit
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 >> --- >> 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_MAJOR1 >> #define DRIVER_MINOR0 >> >> -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 = kz
Re: [PATCH] drm/exynos: use atomic helper commit
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 >> --- >> 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_MAJOR1 >> #define DRIVER_MINOR0 >> >> -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; >> -
Re: [PATCH] drm/exynos: use atomic helper commit
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 > --- > 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 r
Re: [PATCH] drm/exynos: use atomic helper commit
Hi Inki, 2017-01-16 Inki Dae : > 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 > --- > 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->
Re: [PATCH] drm/exynos: use atomic helper commit
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 > --- > 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
[PATCH] drm/exynos: use atomic helper commit
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 --- 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(pri