[PATCH 0/4] drm/exynos: rework vblank handling

[PATCH 0/4] drm/exynos: rework vblank handling

[Andrzej Hajda]

Hi Inki,

This patchset fixes long standing issue with occassional page faults
or vblank event timeouts on TM2 targets due to delayed vblank handling.
DECON driver should now handle properly all scenarios described in drm
docs [1][2], at least it was my intention.

I have successfully tested it on TM2 panel, TV and both.

[1]: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=drm_crtc_state#c.drm_crtc_arm_vblank_event
[2]: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=drm_crtc_state#c.drm_crtc_state

Regards
Andrzej


Andrzej Hajda (4):
  drm/exynos: move crtc event handling to drivers callbacks
  drm/exynos: simplify completion event handling
  drm/exynos/decon5433: fix vblank event handling
  drm/exynos/decon5433: signal frame done interrupt at VSYNC

 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 63 +++++++++++++++++++++++++--
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  1 +
 drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 32 +++++++-------
 drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  2 +
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  2 +
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  1 +
 drivers/gpu/drm/exynos/exynos_mixer.c         |  1 +
 include/video/exynos5433_decon.h              | 13 ++++++
 8 files changed, 96 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/4] drm/exynos: move crtc event handling to drivers callbacks

[Andrzej Hajda]

CRTC event is currently send with next vblank, or instantly in case crtc
is being disabled. This approach usually works, but in corner cases it can
result in premature event generation. Only device driver is able to verify
if the event can be sent. This patch is a first step in that direction - it
moves event handling to the drivers.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  1 +
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  1 +
 drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 29 +++++++++++++++------------
 drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  2 ++
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  2 ++
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  1 +
 drivers/gpu/drm/exynos/exynos_mixer.c         |  1 +
 7 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 0fd6f7a..147911e 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -378,6 +378,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
 
 	if (ctx->out_type & IFTYPE_I80)
 		set_bit(BIT_WIN_UPDATED, &ctx->flags);
+	exynos_crtc_handle_event(crtc);
 }
 
 static void decon_swreset(struct decon_context *ctx)
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index f9ab19e..4881180 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -526,6 +526,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
 
 	for (i = 0; i < WINDOWS_NR; i++)
 		decon_shadow_protect_win(ctx, i, false);
+	exynos_crtc_handle_event(crtc);
 }
 
 static void decon_init(struct decon_context *ctx)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 5367b66..c65f450 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -85,16 +85,28 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
 				     struct drm_crtc_state *old_crtc_state)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct drm_pending_vblank_event *event;
-	unsigned long flags;
 
 	if (exynos_crtc->ops->atomic_flush)
 		exynos_crtc->ops->atomic_flush(exynos_crtc);
+}
+
+static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
+	.enable		= exynos_drm_crtc_enable,
+	.disable	= exynos_drm_crtc_disable,
+	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
+	.atomic_check	= exynos_crtc_atomic_check,
+	.atomic_begin	= exynos_crtc_atomic_begin,
+	.atomic_flush	= exynos_crtc_atomic_flush,
+};
+
+void exynos_crtc_handle_event(struct exynos_drm_crtc *exynos_crtc)
+{
+	struct drm_crtc *crtc = &exynos_crtc->base;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	unsigned long flags;
 
-	event = crtc->state->event;
 	if (event) {
 		crtc->state->event = NULL;
-
 		spin_lock_irqsave(&crtc->dev->event_lock, flags);
 		if (drm_crtc_vblank_get(crtc) == 0)
 			drm_crtc_arm_vblank_event(crtc, event);
@@ -105,15 +117,6 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
 
 }
 
-static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
-	.enable		= exynos_drm_crtc_enable,
-	.disable	= exynos_drm_crtc_disable,
-	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
-	.atomic_check	= exynos_crtc_atomic_check,
-	.atomic_begin	= exynos_crtc_atomic_begin,
-	.atomic_flush	= exynos_crtc_atomic_flush,
-};
-
 static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 6a581a8..abd5d6c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -40,4 +40,6 @@ int exynos_drm_crtc_get_pipe_from_type(struct drm_device *drm_dev,
  */
 void exynos_drm_crtc_te_handler(struct drm_crtc *crtc);
 
+void exynos_crtc_handle_event(struct exynos_drm_crtc *exynos_crtc);
+
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a9fa444..69ebed0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -723,6 +723,8 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc)
 
 	for (i = 0; i < WINDOWS_NR; i++)
 		fimd_shadow_protect_win(ctx, i, false);
+
+	exynos_crtc_handle_event(crtc);
 }
 
 static void fimd_update_plane(struct exynos_drm_crtc *crtc,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 57fe514..5d9a62a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -170,6 +170,7 @@ static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
 	.enable_vblank = vidi_enable_vblank,
 	.disable_vblank = vidi_disable_vblank,
 	.update_plane = vidi_update_plane,
+	.atomic_flush = exynos_crtc_handle_event,
 };
 
 static void vidi_fake_vblank_timer(unsigned long arg)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 72143ac..25edb63 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1012,6 +1012,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 		return;
 
 	mixer_vsync_set_update(mixer_ctx, true);
+	exynos_crtc_handle_event(crtc);
 }
 
 static void mixer_enable(struct exynos_drm_crtc *crtc)
-- 
2.7.4

[PATCH 2/4] drm/exynos: simplify completion event handling

[Andrzej Hajda]

All Exynos CRTC drivers shouldn't fail at referencing vblank events,
alternate path is actually dead code.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index c65f450..5b7cd77 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -105,16 +105,15 @@ void exynos_crtc_handle_event(struct exynos_drm_crtc *exynos_crtc)
 	struct drm_pending_vblank_event *event = crtc->state->event;
 	unsigned long flags;
 
-	if (event) {
-		crtc->state->event = NULL;
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
-		if (drm_crtc_vblank_get(crtc) == 0)
-			drm_crtc_arm_vblank_event(crtc, event);
-		else
-			drm_crtc_send_vblank_event(crtc, event);
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-	}
+	if (!event)
+		return;
+	crtc->state->event = NULL;
+
+	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
 
+	spin_lock_irqsave(&crtc->dev->event_lock, flags);
+	drm_crtc_arm_vblank_event(crtc, event);
+	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 }
 
 static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
-- 
2.7.4

[PATCH 3/4] drm/exynos/decon5433: fix vblank event handling

[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);
+
+	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;
+	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;
+
+	/* 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;
+		}
+
+	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
 #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)
-- 
2.7.4

Re: [PATCH 3/4] drm/exynos/decon5433: fix vblank event handling

[Inki Dae]

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.

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'.

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

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

> +
> +	/* 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.

> +		}
> +
> +	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.

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

Re: [PATCH 3/4] drm/exynos/decon5433: fix vblank event handling

[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?

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

Re: [PATCH 3/4] drm/exynos/decon5433: fix vblank event handling

[Inki Dae]

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

[PATCH 4/4] drm/exynos/decon5433: signal frame done interrupt at VSYNC

[Andrzej Hajda]

DECON in case of video mode generates interrupt by default at start
of vertical back porch. As this interrupt is used to generate VBLANK
events more optimal point is start of vertical front porch.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 +-
 include/video/exynos5433_decon.h              | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index bfa9396..2694b32 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -105,7 +105,7 @@ static int decon_enable_vblank(struct exynos_drm_crtc *crtc)
 		if (ctx->out_type & IFTYPE_I80)
 			val |= VIDINTCON0_FRAMEDONE;
 		else
-			val |= VIDINTCON0_INTFRMEN;
+			val |= VIDINTCON0_INTFRMEN | VIDINTCON0_FRAMESEL_FP;
 
 		writel(val, ctx->addr + DECON_VIDINTCON0);
 	}
diff --git a/include/video/exynos5433_decon.h b/include/video/exynos5433_decon.h
index beefc62..2391b03 100644
--- a/include/video/exynos5433_decon.h
+++ b/include/video/exynos5433_decon.h
@@ -128,6 +128,10 @@
 
 /* VIDINTCON0 */
 #define VIDINTCON0_FRAMEDONE		(1 << 17)
+#define VIDINTCON0_FRAMESEL_BP		(0 << 15)
+#define VIDINTCON0_FRAMESEL_VS		(1 << 15)
+#define VIDINTCON0_FRAMESEL_AC		(2 << 15)
+#define VIDINTCON0_FRAMESEL_FP		(3 << 15)
 #define VIDINTCON0_INTFRMEN		(1 << 12)
 #define VIDINTCON0_INTEN		(1 << 0)
 
-- 
2.7.4

Re: [PATCH 4/4] drm/exynos/decon5433: signal frame done interrupt at VSYNC

[Inki Dae]

2017년 02월 23일 01:05에 Andrzej Hajda 이(가) 쓴 글:
> DECON in case of video mode generates interrupt by default at start
> of vertical back porch. As this interrupt is used to generate VBLANK
> events more optimal point is start of vertical front porch.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 +-
>  include/video/exynos5433_decon.h              | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index bfa9396..2694b32 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -105,7 +105,7 @@ static int decon_enable_vblank(struct exynos_drm_crtc *crtc)
>  		if (ctx->out_type & IFTYPE_I80)
>  			val |= VIDINTCON0_FRAMEDONE;
>  		else
> -			val |= VIDINTCON0_INTFRMEN;
> +			val |= VIDINTCON0_INTFRMEN | VIDINTCON0_FRAMESEL_FP;
>  
>  		writel(val, ctx->addr + DECON_VIDINTCON0);
>  	}
> diff --git a/include/video/exynos5433_decon.h b/include/video/exynos5433_decon.h
> index beefc62..2391b03 100644
> --- a/include/video/exynos5433_decon.h
> +++ b/include/video/exynos5433_decon.h
> @@ -128,6 +128,10 @@
>  
>  /* VIDINTCON0 */
>  #define VIDINTCON0_FRAMEDONE		(1 << 17)
> +#define VIDINTCON0_FRAMESEL_BP		(0 << 15)
> +#define VIDINTCON0_FRAMESEL_VS		(1 << 15)
> +#define VIDINTCON0_FRAMESEL_AC		(2 << 15)

Above definitions are not used.

Thanks.

> +#define VIDINTCON0_FRAMESEL_FP		(3 << 15)
>  #define VIDINTCON0_INTFRMEN		(1 << 12)
>  #define VIDINTCON0_INTEN		(1 << 0)
>  
> 

Re: [PATCH 4/4] drm/exynos/decon5433: signal frame done interrupt at VSYNC

[Andrzej Hajda]

On 07.03.2017 10:14, Inki Dae wrote:
>
> 2017년 02월 23일 01:05에 Andrzej Hajda 이(가) 쓴 글:
>> DECON in case of video mode generates interrupt by default at start
>> of vertical back porch. As this interrupt is used to generate VBLANK
>> events more optimal point is start of vertical front porch.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 +-
>>  include/video/exynos5433_decon.h              | 4 ++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> index bfa9396..2694b32 100644
>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> @@ -105,7 +105,7 @@ static int decon_enable_vblank(struct exynos_drm_crtc *crtc)
>>  		if (ctx->out_type & IFTYPE_I80)
>>  			val |= VIDINTCON0_FRAMEDONE;
>>  		else
>> -			val |= VIDINTCON0_INTFRMEN;
>> +			val |= VIDINTCON0_INTFRMEN | VIDINTCON0_FRAMESEL_FP;
>>  
>>  		writel(val, ctx->addr + DECON_VIDINTCON0);
>>  	}
>> diff --git a/include/video/exynos5433_decon.h b/include/video/exynos5433_decon.h
>> index beefc62..2391b03 100644
>> --- a/include/video/exynos5433_decon.h
>> +++ b/include/video/exynos5433_decon.h
>> @@ -128,6 +128,10 @@
>>  
>>  /* VIDINTCON0 */
>>  #define VIDINTCON0_FRAMEDONE		(1 << 17)
>> +#define VIDINTCON0_FRAMESEL_BP		(0 << 15)
>> +#define VIDINTCON0_FRAMESEL_VS		(1 << 15)
>> +#define VIDINTCON0_FRAMESEL_AC		(2 << 15)
> Above definitions are not used.

Yes, similarly to multiple other definitions in this file and as in many
other files with HW registers related definitions. I can remove it if
you prefer, but maybe it would be better to leave it as it documents the
hardware.

Regards
Andrzej

>
> Thanks.
>
>> +#define VIDINTCON0_FRAMESEL_FP		(3 << 15)
>>  #define VIDINTCON0_INTFRMEN		(1 << 12)
>>  #define VIDINTCON0_INTEN		(1 << 0)
>>  
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel