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?
>
>> + 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)
>>
>
--
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
