From: Inki Dae <inki.dae@samsung.com>
To: Andrzej Hajda <a.hajda@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Javier Martinez Canillas <javier@osg.samsung.com>,
dri-devel@lists.freedesktop.org,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH 3/4] drm/exynos/decon5433: fix vblank event handling
Date: Wed, 08 Mar 2017 10:12:58 +0900 [thread overview]
Message-ID: <58BF5A9A.3050303@samsung.com> (raw)
In-Reply-To: <066f255d-076c-f16f-d3c6-29cb2b2e4b53@samsung.com>
2017년 03월 07일 18:58에 Andrzej Hajda 이(가) 쓴 글:
> On 07.03.2017 10:12, Inki Dae wrote:
>> Thanks for fixing it.
>>
>> Andrzej,
>> DECON_CRFMID register is new to me. Where did you refer this register description from? I couldn't find this register in datasheet I have for Exynos5433.
>
> I have found it in android sources.
>
>>
>> Below are a little bit trivial comments.
>>
>> 2017년 02월 23일 01:05에 Andrzej Hajda 이(가) 쓴 글:
>>> Current implementation of event handling assumes that vblank interrupt is
>>> always called at the right time. It is not true, it can be delayed due to
>>> various reasons. As a result different races can happen. The patch fixes
>>> the issue by using hardware frame counter present in DECON to serialize
>>> vblank and commit completion events.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 62 +++++++++++++++++++++++++--
>>> include/video/exynos5433_decon.h | 9 ++++
>>> 2 files changed, 67 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> index 147911e..bfa9396 100644
>>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> @@ -68,6 +68,8 @@ struct decon_context {
>>> unsigned long flags;
>>> unsigned long out_type;
>>> int first_win;
>>> + spinlock_t vblank_lock;
>>> + u32 frame_id;
>>> };
>>>
>>> static const uint32_t decon_formats[] = {
>>> @@ -365,25 +367,32 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>>> static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
>>> {
>>> struct decon_context *ctx = crtc->ctx;
>>> + unsigned long flags;
>>> int i;
>>>
>>> if (test_bit(BIT_SUSPENDED, &ctx->flags))
>>> return;
>>>
>>> + spin_lock_irqsave(&ctx->vblank_lock, flags);
>>> +
>>> for (i = ctx->first_win; i < WINDOWS_NR; i++)
>>> decon_shadow_protect_win(ctx, i, false);
>>>
>>> + if (ctx->out_type & IFTYPE_I80)
>>> + set_bit(BIT_WIN_UPDATED, &ctx->flags);
>>> +
>>> if (test_and_clear_bit(BIT_REQUEST_UPDATE, &ctx->flags))
>>> decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>>>
>>> - if (ctx->out_type & IFTYPE_I80)
>>> - set_bit(BIT_WIN_UPDATED, &ctx->flags);
>>> - exynos_crtc_handle_event(crtc);
>>> + exynos_crtc_handle_event(ctx->crtc);
>> You don't have to change 'crtc' to 'ctx->crtc'. Keep 'crtc'.
>
> OK
>
>>
>>> +
>>> + spin_unlock_irqrestore(&ctx->vblank_lock, flags);
>>> }
>>>
>>> static void decon_swreset(struct decon_context *ctx)
>>> {
>>> unsigned int tries;
>>> + unsigned long flags;
>>>
>>> writel(0, ctx->addr + DECON_VIDCON0);
>>> for (tries = 2000; tries; --tries) {
>>> @@ -401,6 +410,10 @@ static void decon_swreset(struct decon_context *ctx)
>>>
>>> WARN(tries == 0, "failed to software reset DECON\n");
>>>
>>> + spin_lock_irqsave(&ctx->vblank_lock, flags);
>>> + ctx->frame_id = 0;
>>> + spin_unlock_irqrestore(&ctx->vblank_lock, flags);
>>> +
>>> if (!(ctx->out_type & IFTYPE_HDMI))
>>> return;
>>>
>>> @@ -579,6 +592,46 @@ static const struct component_ops decon_component_ops = {
>>> .unbind = decon_unbind,
>>> };
>>>
>>> +static void decon_handle_vblank(struct decon_context *ctx)
>>> +{
>>> + u32 frm, pfrm, status, cnt;
>>> +
>>> + spin_lock(&ctx->vblank_lock);
>>> +
>>> + /* To get consistent result repeat read until frame id is stable. */
>>> + frm = readl(ctx->addr + DECON_CRFMID);
>>> + cnt = 3;
>> Is there some guide that initial value of cnt should be 3?
>
> No, this is my arbitrary choice. In general the loop will be passed only
> once. In rare case when code runs at frame change time it will be run
> twice. It never should run more than two times - it would be sign of HW
> error, incorrect DECON programming or serious bottleneck.
> I initially left cnt=3 to detect such case, but after all I did not
> report such situation, so I can either change cnt to 2, or add error log
> if after loop cnt is 0, what do you prefer?
I don't care. I just wondered why cnt should be 3. :) It'd be better to comment why you set cnt to 3. Seems enough with above your comment.
Thanks.
>
>>
>>> + do {
>>> + status = readl(ctx->addr + DECON_VIDCON1);
>>> + pfrm = frm;
>>> + frm = readl(ctx->addr + DECON_CRFMID);
>>> + } while (frm != pfrm && --cnt);
>>> +
>>> + status &= VIDCON1_VSTATUS_MASK | VIDCON1_I80_ACTIVE;
>> I couldn't find I80_ACTIVE field on DECON_VIDCON1 register descrption. Do you have other datasheet, new one?
>
> No, this is just result of my hardware analysis/debugging.
>
>>
>>> +
>>> + /* In case of delayed vblank CRFMID could be already incremented,
>>> + * it should be taken into account.
>>> + */
>>> + if (frm > 0)
>>> + switch (status) {
>>> + case VIDCON1_VSTATUS_VS:
>>> + if (ctx->out_type & IFTYPE_I80)
>>> + break;
>>> + case VIDCON1_I80_ACTIVE:
>>> + case VIDCON1_VSTATUS_BP:
>>> + case VIDCON1_VSTATUS_AC:
>>> + --frm;
>> Let's add 'break;' and 'default: break;' explicitly.
>
> OK, anyway this code will be superseded by frame counter patch.
>
>>
>>> + }
>>> +
>>> + if (frm != ctx->frame_id) {
>>> + if (frm > ctx->frame_id)
>>> + drm_crtc_handle_vblank(&ctx->crtc->base);
>>> + ctx->frame_id = frm;
>>> + }
>>> +
>>> + spin_unlock(&ctx->vblank_lock);
>>> +}
>>> +
>>> static irqreturn_t decon_irq_handler(int irq, void *dev_id)
>>> {
>>> struct decon_context *ctx = dev_id;
>>> @@ -599,7 +652,7 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
>>> (VIDOUT_INTERLACE_EN_F | VIDOUT_INTERLACE_FIELD_F))
>>> return IRQ_HANDLED;
>>> }
>>> - drm_crtc_handle_vblank(&ctx->crtc->base);
>>> + decon_handle_vblank(ctx);
>>> }
>>>
>>> out:
>>> @@ -672,6 +725,7 @@ static int exynos5433_decon_probe(struct platform_device *pdev)
>>> __set_bit(BIT_SUSPENDED, &ctx->flags);
>>> ctx->dev = dev;
>>> ctx->out_type = (unsigned long)of_device_get_match_data(dev);
>>> + spin_lock_init(&ctx->vblank_lock);
>>>
>>> if (ctx->out_type & IFTYPE_HDMI) {
>>> ctx->first_win = 1;
>>> diff --git a/include/video/exynos5433_decon.h b/include/video/exynos5433_decon.h
>>> index ef8e2a8..beefc62 100644
>>> --- a/include/video/exynos5433_decon.h
>>> +++ b/include/video/exynos5433_decon.h
>>> @@ -46,6 +46,8 @@
>>> #define DECON_FRAMEFIFO_STATUS 0x0524
>>> #define DECON_CMU 0x1404
>>> #define DECON_UPDATE 0x1410
>>> +#define DECON_CRFMID 0x1414
>>> +#define DECON_RRFRMID 0x1418
>> Above definition is not used in this patch.
>
> I have added it to make DECON registry space better documented.
> Of course I will remove it if you like, as it does not serves any
> purpose at the moment.
>
> Regards
> Andrzej
>
>
>>
>> Thanks.
>>
>>> #define DECON_UPDATE_SCHEME 0x1438
>>> #define DECON_VIDCON1 0x2000
>>> #define DECON_VIDCON2 0x2004
>>> @@ -142,6 +144,13 @@
>>> #define STANDALONE_UPDATE_F (1 << 0)
>>>
>>> /* DECON_VIDCON1 */
>>> +#define VIDCON1_LINECNT_MASK (0x0fff << 16)
>>> +#define VIDCON1_I80_ACTIVE (1 << 15)
>>> +#define VIDCON1_VSTATUS_MASK (0x3 << 13)
>>> +#define VIDCON1_VSTATUS_VS (0 << 13)
>>> +#define VIDCON1_VSTATUS_BP (1 << 13)
>>> +#define VIDCON1_VSTATUS_AC (2 << 13)
>>> +#define VIDCON1_VSTATUS_FP (3 << 13)
>>> #define VIDCON1_VCLK_MASK (0x3 << 9)
>>> #define VIDCON1_VCLK_RUN_VDEN_DISABLE (0x3 << 9)
>>> #define VIDCON1_VCLK_HOLD (0x0 << 9)
>>>
>>
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-03-08 1:12 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20170222160515eucas1p207f5f7b3a956b56405f547befd4daf63@eucas1p2.samsung.com>
2017-02-22 16:05 ` [PATCH 0/4] drm/exynos: rework vblank handling Andrzej Hajda
[not found] ` <CGME20170222160515eucas1p2ced846577ae3e435ffb2e17fa60fbd6e@eucas1p2.samsung.com>
2017-02-22 16:05 ` [PATCH 1/4] drm/exynos: move crtc event handling to drivers callbacks Andrzej Hajda
[not found] ` <CGME20170222160516eucas1p298a994725044878022fece1600f9e36a@eucas1p2.samsung.com>
2017-02-22 16:05 ` [PATCH 2/4] drm/exynos: simplify completion event handling Andrzej Hajda
[not found] ` <CGME20170222160516eucas1p102bcb93b7f0356cfafbb707987b9e85f@eucas1p1.samsung.com>
2017-02-22 16:05 ` [PATCH 3/4] drm/exynos/decon5433: fix vblank " Andrzej Hajda
2017-03-07 9:12 ` Inki Dae
2017-03-07 9:58 ` Andrzej Hajda
2017-03-08 1:12 ` Inki Dae [this message]
[not found] ` <CGME20170222160517eucas1p1a254801abe01ae5ee77681ff4f955ba8@eucas1p1.samsung.com>
2017-02-22 16:05 ` [PATCH 4/4] drm/exynos/decon5433: signal frame done interrupt at VSYNC Andrzej Hajda
2017-03-07 9:14 ` Inki Dae
2017-03-08 10:47 ` Andrzej Hajda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=58BF5A9A.3050303@samsung.com \
--to=inki.dae@samsung.com \
--cc=a.hajda@samsung.com \
--cc=b.zolnierkie@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=javier@osg.samsung.com \
--cc=krzk@kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).