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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
