[PATCH v2] drm/exynos: use atomic helper commit
Tobias Jakobi
tjakobi at math.uni-bielefeld.de
Mon Jan 23 12:44:43 UTC 2017
Inki Dae wrote:
>
>
> 2017년 01월 20일 22:05에 Tobias Jakobi 이(가) 쓴 글:
>> Inki Dae wrote:
>>> Hi Tobias,
>>>
>>> 2017년 01월 19일 21:49에 Tobias Jakobi 이(가) 쓴 글:
>>>> What about Laurent's comment stating that drm_atomic_helper_commit() is
>>>> broken at the moment? Shouldn't there be some kind of warning in the
>>>> commit message that this patch is only safe to apply once the fixes for
>>>> drm_atomic_helper_commit() have landed? I'm already seeing this getting
>>>> merged by accident, without Maarten's series even being reviewed.
>>>
>>> What patches you mean?
>> The patchset that Laurent mentioned.
>> [PATCH v3 0/7] drm/atomic: Add accessor macros for all atomic state.
>> https://www.spinics.net/lists/dri-devel/msg129537.html
>>
>>
>>> According to Laurent's comment, async commit support of drm_atomic_helper_commit is subect to a race condition.
>>> So I'm checking whether there is any case to use async commit in Exynos DRM driver.
>> I'm not sure what you're exactly checking here, because it is userspace
>> that requests a atomic commit to be executed asynchronously.
>
> Hm... See the below code from mainline,
>
> nt drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> ...
>
> if ((arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) &&
> !dev->mode_config.async_page_flip)
> return -EINVAL;
> ...
>
> I'm not sure you checked Exynos drm driver but Exynos drm driver doesn't support async page flip.
> And you are right. userspace requests async commit but that is also depend on specific driver.
OK, I assumed that this mandatory by now. Nevermind then.
>>> As of now, I don't see any case. even without Maarten's patch set, it works well - actually, I had a test with atomic test app more than 10 hours..
>> Can you provide this test application? In particular I'm asking this
>> because libdrm currently doesn't provide any tests using the atomic API.
>> So this application might be of interest also for other people.
>
> Below is the app I tested. Know that this application is from chromiumOS tree and I just fixed some parts for internal test.
> https://review.tizen.org/git/?p=atform/upstream/libdrm.git;a=commitdiff;h�3bd95f2c5a9b4b69062a3ff008947054b94f55
Thanks, any chance this is going to be submitted upstream?
>>> And important thing is that this posting is just for review not merge.
>> In this case the patches should be tagged as 'RFC', something which I
>> don't see here.
>>
>>
>>> So if there is any critical problem with this patch, I will stop to merge it. This would be my role, maintainer.
>> Let me phrase it this way. Your patch is not fixing a bug, it is a
>
> This patch definitely phrased 'This patch replaces specific atomic commit function with atomic helper commit one' not fixing a bug.
Please read the sentences in full.
- Tobias
>
> Thanks.
>
>> cleanup patch that reduces driver specific code with DRM core code. But
>> as Laurent has pointed out this core code currently has some known (!)
>> issues. In the interest of not breaking anything I would advise against
>> merging this before Maarten's patches have landed.
>>
>>
>> With best wishes,
>> Tobias
>>
>>
>>>
>>> Thanks.
>>>
>>>>
>>>> The commit message looks much better now. Still some spelling issues remain:
>>>> implemention -> implementation
>>>>
>>>>
>>>> With best wishes,
>>>> Tobias
>>>>
>>>>
>>>> Inki Dae wrote:
>>>>> This patch replaces specific atomic commit function
>>>>> with atomic helper commit one.
>>>>>
>>>>> For this, it removes existing atomic commit function
>>>>> and relevant code specific to Exynos DRM and makes
>>>>> atomic helper commit to be used instead.
>>>>>
>>>>> Below are changes for the use of atomic helper commit:
>>>>> - add atomic_commit_tail callback specific to Exynos DRM
>>>>> . 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
>>>>> prior to drm_atomic_helper_commit_planes function call.
>>>>> atomic_commit_tail callback implements this call ordering.
>>>>> - allow plane commit only in case that CRTC device is enabled.
>>>>> . for this, it calls atomic_helper_commit_planes function
>>>>> with DRM_PLANE_COMMIT_ACTIVE_ONLY flag in atomic_commit_tail callback.
>>>>>
>>>>> Changelog v1:
>>>>> - fix comment
>>>>> - fix trivial things
>>>>> - revive existing comment which explains why plane commit should be
>>>>> called after crtc and encoder device are enabled.
>>>>>
>>>>> Signed-off-by: Inki Dae <inki.dae at samsung.com>
>>>>> Reviewed-by: Gustavo Padovan <gustavo.padovan at collabora.com>
>>>>> ---
>>>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 10 ++-
>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 109 -------------------------------
>>>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 32 ++++++++-
>>>>> 3 files changed, 40 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..8f13bce 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 =L;
>>>>> + }
>>>>> }
>>>>>
>>>>> static void
>>>>> @@ -94,9 +102,9 @@ 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 >>>> .enable = xynos_drm_crtc_enable,
>>>>> .disable =nos_drm_crtc_disable,
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>>> index 3ec0535..c8f3eeb 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 =mit->dev;
>>>>> - struct exynos_drm_private *priv =->dev_private;
>>>>> - struct drm_atomic_state *state =mit->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 &=mmit->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 =tainer_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 =L;
>>>>> }
>>>>>
>>>>> -static int commit_is_pending(struct exynos_drm_private *priv, u32 crtcs)
>>>>> -{
>>>>> - bool pending;
>>>>> -
>>>>> - spin_lock(&priv->lock);
>>>>> - pending =v->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_private;
>>>>> - struct exynos_atomic_commit *commit;
>>>>> - struct drm_crtc *crtc;
>>>>> - struct drm_crtc_state *crtc_state;
>>>>> - int i, ret;
>>>>> -
>>>>> - commit =lloc(sizeof(*commit), GFP_KERNEL);
>>>>> - if (!commit)
>>>>> - return -ENOMEM;
>>>>> -
>>>>> - ret =_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 =;
>>>>> - commit->state =te;
>>>>> -
>>>>> - /* 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 |=_crtc_mask(crtc);
>>>>> -
>>>>> - wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs));
>>>>> -
>>>>> - spin_lock(&priv->lock);
>>>>> - priv->pending |=mit->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)
>>>>> {
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>>>> index 68d4142..c77a5ac 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>>>> @@ -187,11 +187,40 @@ 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 =te->dev;
>>>>> +
>>>>> + 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,
>>>>> + 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 = xynos_drm_atomic_commit_tail,
>>>>> +};
>>>>> +
>>>>> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs >>>> .fb_create = xynos_user_fb_create,
>>>>> .output_poll_changed =nos_drm_output_poll_changed,
>>>>> .atomic_check =nos_atomic_check,
>>>>> - .atomic_commit =nos_atomic_commit,
>>>>> + .atomic_commit =_atomic_helper_commit,
>>>>> };
>>>>>
>>>>> void exynos_drm_mode_config_init(struct drm_device *dev)
>>>>> @@ -208,4 +237,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
>>>>> dev->mode_config.max_height 6;
>>>>>
>>>>> dev->mode_config.funcs =ynos_drm_mode_config_funcs;
>>>>> + dev->mode_config.helper_private =ynos_drm_mode_config_helpers;
>>>>> }
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>>> the body of a message to majordomo at vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
More information about the dri-devel
mailing list