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@xxxxxxxxxxx> >>>>> Reviewed-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx> >>>>> --- >>>>> 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@xxxxxxxxxxxxxxx >>>> 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@xxxxxxxxxxxxxxx >>> 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@xxxxxxxxxxxxxxx >> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
