[parent not found: <CGME20170116091327epcas5p1250b06956c0fb805113c35c3223a94e3@epcas5p1.samsung.com>]
* [PATCH] drm/exynos: remove unnecessary codes
[not found] ` <CGME20170116091327epcas5p1250b06956c0fb805113c35c3223a94e3@epcas5p1.samsung.com>
@ 2017-01-16 9:13 ` Inki Dae
2017-01-16 9:21 ` Inki Dae
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-16 9:13 UTC (permalink / raw)
To: dri-devel; +Cc: linux-samsung-soc
This patch removes exynos_drm_crtc_cancel_page_flip call
when drm is closed because at that time, events will be released
by drm_events_release function.
Change-Id: I156ea27a4c90aa87a27a50415515fa334148c912
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 9d0df00..035d02e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -198,13 +198,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
static void exynos_drm_preclose(struct drm_device *dev,
struct drm_file *file)
{
- struct drm_crtc *crtc;
-
exynos_drm_subdrv_close(dev, file);
-
- 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)
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: remove unnecessary codes
2017-01-16 9:13 ` [PATCH] drm/exynos: remove unnecessary codes Inki Dae
@ 2017-01-16 9:21 ` Inki Dae
2017-01-16 9:37 ` Andrzej Hajda
2017-01-16 11:24 ` Tobias Jakobi
2 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-16 9:21 UTC (permalink / raw)
To: dri-devel; +Cc: linux-samsung-soc
2017년 01월 16일 18:13에 Inki Dae 이(가) 쓴 글:
> This patch removes exynos_drm_crtc_cancel_page_flip call
> when drm is closed because at that time, events will be released
> by drm_events_release function.
>
> Change-Id: I156ea27a4c90aa87a27a50415515fa334148c912
Oops, I forgot to remove above Changed-Id. Please ignore this line. I will merge it without the line.
Thanks.
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d0df00..035d02e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -198,13 +198,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
> static void exynos_drm_preclose(struct drm_device *dev,
> struct drm_file *file)
> {
> - struct drm_crtc *crtc;
> -
> exynos_drm_subdrv_close(dev, file);
> -
> - 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)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: remove unnecessary codes
2017-01-16 9:13 ` [PATCH] drm/exynos: remove unnecessary codes Inki Dae
2017-01-16 9:21 ` Inki Dae
@ 2017-01-16 9:37 ` Andrzej Hajda
2017-01-17 6:03 ` Inki Dae
2017-01-16 11:24 ` Tobias Jakobi
2 siblings, 1 reply; 14+ messages in thread
From: Andrzej Hajda @ 2017-01-16 9:37 UTC (permalink / raw)
To: Inki Dae, dri-devel; +Cc: linux-samsung-soc
On 16.01.2017 10:13, Inki Dae wrote:
> This patch removes exynos_drm_crtc_cancel_page_flip call
> when drm is closed because at that time, events will be released
> by drm_events_release function.
>
> Change-Id: I156ea27a4c90aa87a27a50415515fa334148c912
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d0df00..035d02e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -198,13 +198,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
> static void exynos_drm_preclose(struct drm_device *dev,
> struct drm_file *file)
> {
> - struct drm_crtc *crtc;
> -
> exynos_drm_subdrv_close(dev, file);
> -
> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> - exynos_drm_crtc_cancel_page_flip(crtc, file);
Since this is single user of exynos_drm_crtc_cancel_page_flip, this
function can be removed as well.
Beside this:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Regards
Andrzej
> -
> }
>
> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: remove unnecessary codes
2017-01-16 9:37 ` Andrzej Hajda
@ 2017-01-17 6:03 ` Inki Dae
0 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-17 6:03 UTC (permalink / raw)
To: Andrzej Hajda, dri-devel; +Cc: linux-samsung-soc
2017년 01월 16일 18:37에 Andrzej Hajda 이(가) 쓴 글:
> On 16.01.2017 10:13, Inki Dae wrote:
>> This patch removes exynos_drm_crtc_cancel_page_flip call
>> when drm is closed because at that time, events will be released
>> by drm_events_release function.
>>
>> Change-Id: I156ea27a4c90aa87a27a50415515fa334148c912
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 9d0df00..035d02e 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -198,13 +198,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>> static void exynos_drm_preclose(struct drm_device *dev,
>> struct drm_file *file)
>> {
>> - struct drm_crtc *crtc;
>> -
>> exynos_drm_subdrv_close(dev, file);
>> -
>> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>> - exynos_drm_crtc_cancel_page_flip(crtc, file);
>
> Since this is single user of exynos_drm_crtc_cancel_page_flip, this
> function can be removed as well.
Indeed. Will remove that function.
Thanks.
> Beside this:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
>
> Regards
> Andrzej
>
>> -
>> }
>>
>> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
>
>
> --
> 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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/exynos: remove unnecessary codes
2017-01-16 9:13 ` [PATCH] drm/exynos: remove unnecessary codes Inki Dae
2017-01-16 9:21 ` Inki Dae
2017-01-16 9:37 ` Andrzej Hajda
@ 2017-01-16 11:24 ` Tobias Jakobi
2 siblings, 0 replies; 14+ messages in thread
From: Tobias Jakobi @ 2017-01-16 11:24 UTC (permalink / raw)
To: Inki Dae, dri-devel; +Cc: airlied, linux-samsung-soc
Seen this quite often lately. It should be be 'code' and not 'codes'.
Code in the programming context is always a collection of instructions.
With best wishes,
Tobias
Inki Dae wrote:
> This patch removes exynos_drm_crtc_cancel_page_flip call
> when drm is closed because at that time, events will be released
> by drm_events_release function.
>
> Change-Id: I156ea27a4c90aa87a27a50415515fa334148c912
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d0df00..035d02e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -198,13 +198,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
> static void exynos_drm_preclose(struct drm_device *dev,
> struct drm_file *file)
> {
> - struct drm_crtc *crtc;
> -
> exynos_drm_subdrv_close(dev, file);
> -
> - 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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 9:13 ` [PATCH] drm/exynos: use atomic helper commit Inki Dae
[not found] ` <CGME20170116091327epcas5p1250b06956c0fb805113c35c3223a94e3@epcas5p1.samsung.com>
@ 2017-01-16 11:22 ` Tobias Jakobi
2017-01-17 6:16 ` Inki Dae
2017-01-16 19:08 ` Gustavo Padovan
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Tobias Jakobi @ 2017-01-16 11:22 UTC (permalink / raw)
To: Inki Dae, dri-devel; +Cc: airlied, linux-samsung-soc
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;
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 11:22 ` [PATCH] drm/exynos: use atomic helper commit Tobias Jakobi
@ 2017-01-17 6:16 ` Inki Dae
0 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-17 6:16 UTC (permalink / raw)
To: Tobias Jakobi, dri-devel; +Cc: airlied, linux-samsung-soc
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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 9:13 ` [PATCH] drm/exynos: use atomic helper commit Inki Dae
[not found] ` <CGME20170116091327epcas5p1250b06956c0fb805113c35c3223a94e3@epcas5p1.samsung.com>
2017-01-16 11:22 ` [PATCH] drm/exynos: use atomic helper commit Tobias Jakobi
@ 2017-01-16 19:08 ` Gustavo Padovan
2017-01-17 8:20 ` Inki Dae
2017-01-16 20:34 ` Laurent Pinchart
2017-01-17 9:17 ` Laurent Pinchart
4 siblings, 1 reply; 14+ messages in thread
From: Gustavo Padovan @ 2017-01-16 19:08 UTC (permalink / raw)
To: Inki Dae; +Cc: dri-devel, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 19:08 ` Gustavo Padovan
@ 2017-01-17 8:20 ` Inki Dae
0 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-17 8:20 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel, linux-samsung-soc
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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 9:13 ` [PATCH] drm/exynos: use atomic helper commit Inki Dae
` (2 preceding siblings ...)
2017-01-16 19:08 ` Gustavo Padovan
@ 2017-01-16 20:34 ` Laurent Pinchart
2017-01-17 6:20 ` Inki Dae
2017-01-17 9:17 ` Laurent Pinchart
4 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2017-01-16 20:34 UTC (permalink / raw)
To: dri-devel; +Cc: linux-samsung-soc
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;
> }
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 20:34 ` Laurent Pinchart
@ 2017-01-17 6:20 ` Inki Dae
0 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-17 6:20 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-samsung-soc
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;
>> }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-16 9:13 ` [PATCH] drm/exynos: use atomic helper commit Inki Dae
` (3 preceding siblings ...)
2017-01-16 20:34 ` Laurent Pinchart
@ 2017-01-17 9:17 ` Laurent Pinchart
2017-01-18 2:17 ` Inki Dae
4 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2017-01-17 9:17 UTC (permalink / raw)
To: dri-devel; +Cc: linux-samsung-soc
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]
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] drm/exynos: use atomic helper commit
2017-01-17 9:17 ` Laurent Pinchart
@ 2017-01-18 2:17 ` Inki Dae
0 siblings, 0 replies; 14+ messages in thread
From: Inki Dae @ 2017-01-18 2:17 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-samsung-soc
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]
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread