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....@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
