[PATCH v1, 0/2] drm/mediatek: Fix external display issue

[PATCH v1, 0/2] drm/mediatek: Fix external display issue

[yongqiang.niu]

From: Yongqiang Niu <yongqiang.niu@mediatek.com>

Fix external display issue

Yongqiang Niu (2):
  drm/mediatek: Fixup external display black screen issue
  drm/mediatek: Fix external display vblank timeout issue

 drivers/gpu/drm/mediatek/mtk_dpi.c          | 14 +++++----
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     | 45 ++++++++++++++++++++---------
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 14 +++++++++
 3 files changed, 54 insertions(+), 19 deletions(-)

-- 
1.8.1.1.dirty

[PATCH v1, 1/2] drm/mediatek: Fixup external display black screen issue

[yongqiang.niu]

From: Yongqiang Niu <yongqiang.niu@mediatek.com>

Problem:
overlay hangup when external display hotplut test

Fix:
disable overlay when crtc disable

Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 4fb346c..7eca02f 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 	mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
 	mtk_disp_mutex_enable(mtk_crtc->mutex);
 
+	/* Initially configure all planes */
+	for (i = 0; i < mtk_crtc->layer_nr; i++) {
+		struct drm_plane *plane = &mtk_crtc->planes[i];
+		struct mtk_plane_state *plane_state;
+		struct mtk_ddp_comp *comp;
+		unsigned int local_layer;
+
+		plane_state = to_mtk_plane_state(plane->state);
+		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
+		if (comp)
+			mtk_ddp_comp_layer_config(comp, local_layer,
+						  plane_state, NULL);
+	}
+
 	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
 		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
 		enum mtk_ddp_comp_id prev;
@@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 		mtk_ddp_comp_start(comp);
 	}
 
-	/* Initially configure all planes */
-	for (i = 0; i < mtk_crtc->layer_nr; i++) {
-		struct drm_plane *plane = &mtk_crtc->planes[i];
-		struct mtk_plane_state *plane_state;
-		struct mtk_ddp_comp *comp;
-		unsigned int local_layer;
-
-		plane_state = to_mtk_plane_state(plane->state);
-		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
-		if (comp)
-			mtk_ddp_comp_layer_config(comp, local_layer,
-						  plane_state, NULL);
-	}
-
 	return 0;
 
 err_mutex_unprepare:
@@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
 	for (i = 0; i < mtk_crtc->layer_nr; i++) {
 		struct drm_plane *plane = &mtk_crtc->planes[i];
 		struct mtk_plane_state *plane_state;
+		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
+		unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
+		unsigned int local_layer;
 
 		plane_state = to_mtk_plane_state(plane->state);
 		plane_state->pending.enable = false;
 		plane_state->pending.config = true;
+
+		if (i >= comp_layer_nr) {
+			comp = mtk_crtc->ddp_comp[1];
+			local_layer = i - comp_layer_nr;
+		} else
+			local_layer = i;
+		mtk_ddp_comp_layer_config(comp, local_layer,
+					  plane_state, NULL);
 	}
 	mtk_crtc->pending_planes = true;
 
-- 
1.8.1.1.dirty

[PATCH v1, 2/2] drm/mediatek: Fix external display vblank timeout issue

[yongqiang.niu]

From: Yongqiang Niu <yongqiang.niu@mediatek.com>

Fix external display vblank timeout issue

Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c          | 14 +++++++++-----
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  6 ++++++
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 14 ++++++++++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index be6d95c..38cabbe 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -387,8 +387,12 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
 {
 	int ret;
 
-	if (++dpi->refcount != 1)
+	if (++dpi->refcount != 1) {
+		dev_warn(dpi->dev, "%s refcount: %d\n", __func__, dpi->refcount);
 		return 0;
+	}
+
+	DRM_DEBUG_DRIVER("%s refcount %d\n", __func__, dpi->refcount);
 
 	ret = clk_prepare_enable(dpi->engine_clk);
 	if (ret) {
@@ -563,14 +567,14 @@ static int mtk_dpi_atomic_check(struct drm_encoder *encoder,
 	.atomic_check = mtk_dpi_atomic_check,
 };
 
-static void mtk_dpi_start(struct mtk_ddp_comp *comp)
+static void mtk_dpi_prepare(struct mtk_ddp_comp *comp)
 {
 	struct mtk_dpi *dpi = container_of(comp, struct mtk_dpi, ddp_comp);
 
 	mtk_dpi_power_on(dpi);
 }
 
-static void mtk_dpi_stop(struct mtk_ddp_comp *comp)
+static void mtk_dpi_unprepare(struct mtk_ddp_comp *comp)
 {
 	struct mtk_dpi *dpi = container_of(comp, struct mtk_dpi, ddp_comp);
 
@@ -578,8 +582,8 @@ static void mtk_dpi_stop(struct mtk_ddp_comp *comp)
 }
 
 static const struct mtk_ddp_comp_funcs mtk_dpi_funcs = {
-	.start = mtk_dpi_start,
-	.stop = mtk_dpi_stop,
+	.prepare = mtk_dpi_prepare,
+	.unprepare = mtk_dpi_unprepare,
 };
 
 static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 7eca02f..a6d3d97 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -345,6 +345,9 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 		return ret;
 	}
 
+	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
+		mtk_ddp_comp_prepare(mtk_crtc->ddp_comp[i]);
+
 	ret = mtk_disp_mutex_prepare(mtk_crtc->mutex);
 	if (ret < 0) {
 		DRM_ERROR("Failed to enable mutex clock: %d\n", ret);
@@ -434,6 +437,9 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
 	mtk_crtc_ddp_clk_disable(mtk_crtc);
 	mtk_disp_mutex_unprepare(mtk_crtc->mutex);
 
+	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
+		mtk_ddp_comp_unprepare(mtk_crtc->ddp_comp[i]);
+
 	pm_runtime_put(drm->dev);
 
 	if (crtc->state->event && !crtc->state->active) {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index 5b0a3d4..097b90d 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -78,6 +78,8 @@ struct mtk_ddp_comp_funcs {
 	void (*stop)(struct mtk_ddp_comp *comp);
 	void (*enable_vblank)(struct mtk_ddp_comp *comp, struct drm_crtc *crtc);
 	void (*disable_vblank)(struct mtk_ddp_comp *comp);
+	void (*prepare)(struct mtk_ddp_comp *comp);
+	void (*unprepare)(struct mtk_ddp_comp *comp);
 	unsigned int (*supported_rotations)(struct mtk_ddp_comp *comp);
 	unsigned int (*layer_nr)(struct mtk_ddp_comp *comp);
 	void (*layer_on)(struct mtk_ddp_comp *comp, unsigned int idx,
@@ -117,6 +119,18 @@ static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
 		comp->funcs->config(comp, w, h, vrefresh, bpc, cmdq_pkt);
 }
 
+static inline void mtk_ddp_comp_prepare(struct mtk_ddp_comp *comp)
+{
+	if (comp->funcs && comp->funcs->prepare)
+		comp->funcs->prepare(comp);
+}
+
+static inline void mtk_ddp_comp_unprepare(struct mtk_ddp_comp *comp)
+{
+	if (comp->funcs && comp->funcs->unprepare)
+		comp->funcs->unprepare(comp);
+}
+
 static inline void mtk_ddp_comp_start(struct mtk_ddp_comp *comp)
 {
 	if (comp->funcs && comp->funcs->start)
-- 
1.8.1.1.dirty

Re: [PATCH v1, 1/2] drm/mediatek: Fixup external display black screen issue

[CK Hu]

Hi, Yongqiang:

On Wed, 2019-11-27 at 09:17 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
> 
> Problem:
> overlay hangup when external display hotplut test
> 
> Fix:
> disable overlay when crtc disable

I think you do two things in this patch. The first is to config layer
before component start, and the second is disable layer when crtc
disable. So separate to two patches.

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 4fb346c..7eca02f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>  	mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
>  	mtk_disp_mutex_enable(mtk_crtc->mutex);
>  
> +	/* Initially configure all planes */
> +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> +		struct drm_plane *plane = &mtk_crtc->planes[i];
> +		struct mtk_plane_state *plane_state;
> +		struct mtk_ddp_comp *comp;
> +		unsigned int local_layer;
> +
> +		plane_state = to_mtk_plane_state(plane->state);
> +		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> +		if (comp)
> +			mtk_ddp_comp_layer_config(comp, local_layer,
> +						  plane_state, NULL);
> +	}
> +
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
>  		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
>  		enum mtk_ddp_comp_id prev;
> @@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>  		mtk_ddp_comp_start(comp);
>  	}
>  
> -	/* Initially configure all planes */
> -	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> -		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> -		struct mtk_ddp_comp *comp;
> -		unsigned int local_layer;
> -
> -		plane_state = to_mtk_plane_state(plane->state);
> -		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> -		if (comp)
> -			mtk_ddp_comp_layer_config(comp, local_layer,
> -						  plane_state, NULL);
> -	}
> -
>  	return 0;
>  
>  err_mutex_unprepare:
> @@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
>  		struct mtk_plane_state *plane_state;
> +		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> +		unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> +		unsigned int local_layer;
>  
>  		plane_state = to_mtk_plane_state(plane->state);
>  		plane_state->pending.enable = false;
>  		plane_state->pending.config = true;
> +
> +		if (i >= comp_layer_nr) {
> +			comp = mtk_crtc->ddp_comp[1];
> +			local_layer = i - comp_layer_nr;
> +		} else
> +			local_layer = i;
> +		mtk_ddp_comp_layer_config(comp, local_layer,
> +					  plane_state, NULL);

I'm confused with this part. The design of this loop is to set
plane_state->pending.enable = false and wait for irq handler to write
register. Why do you directly write register?

Regards,
CK

>  	}
>  	mtk_crtc->pending_planes = true;
>  

Re: [PATCH v1, 2/2] drm/mediatek: Fix external display vblank timeout issue

[CK Hu]

Hi, Yongqiang:

I would like the title could clearly express what does this patch do. I
think what this patch do is to implement prepare/unprepare interface for
dpi driver. And you could describe why do this in commit message.


On Wed, 2019-11-27 at 09:17 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
> 
> Fix external display vblank timeout issue

I think you move dpi_power_on to more early stage. But why this would
fix vblank timeout. Please describe more about this so that I would
agree this patch. 

This is a bug fix, so please add a 'Fixes' tag.

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c          | 14 +++++++++-----
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  6 ++++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 14 ++++++++++++++
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index be6d95c..38cabbe 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -387,8 +387,12 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
>  {
>  	int ret;
>  
> -	if (++dpi->refcount != 1)
> +	if (++dpi->refcount != 1) {
> +		dev_warn(dpi->dev, "%s refcount: %d\n", __func__, dpi->refcount);
>  		return 0;
> +	}
> +
> +	DRM_DEBUG_DRIVER("%s refcount %d\n", __func__, dpi->refcount);
>  
>  	ret = clk_prepare_enable(dpi->engine_clk);
>  	if (ret) {
> @@ -563,14 +567,14 @@ static int mtk_dpi_atomic_check(struct drm_encoder *encoder,
>  	.atomic_check = mtk_dpi_atomic_check,
>  };
>  
> -static void mtk_dpi_start(struct mtk_ddp_comp *comp)
> +static void mtk_dpi_prepare(struct mtk_ddp_comp *comp)
>  {
>  	struct mtk_dpi *dpi = container_of(comp, struct mtk_dpi, ddp_comp);
>  
>  	mtk_dpi_power_on(dpi);
>  }
>  
> -static void mtk_dpi_stop(struct mtk_ddp_comp *comp)
> +static void mtk_dpi_unprepare(struct mtk_ddp_comp *comp)
>  {
>  	struct mtk_dpi *dpi = container_of(comp, struct mtk_dpi, ddp_comp);
>  
> @@ -578,8 +582,8 @@ static void mtk_dpi_stop(struct mtk_ddp_comp *comp)
>  }
>  
>  static const struct mtk_ddp_comp_funcs mtk_dpi_funcs = {
> -	.start = mtk_dpi_start,
> -	.stop = mtk_dpi_stop,
> +	.prepare = mtk_dpi_prepare,
> +	.unprepare = mtk_dpi_unprepare,
>  };
>  
>  static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 7eca02f..a6d3d97 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -345,6 +345,9 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>  		return ret;
>  	}
>  
> +	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
> +		mtk_ddp_comp_prepare(mtk_crtc->ddp_comp[i]);
> +
>  	ret = mtk_disp_mutex_prepare(mtk_crtc->mutex);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to enable mutex clock: %d\n", ret);
> @@ -434,6 +437,9 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
>  	mtk_crtc_ddp_clk_disable(mtk_crtc);
>  	mtk_disp_mutex_unprepare(mtk_crtc->mutex);
>  
> +	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
> +		mtk_ddp_comp_unprepare(mtk_crtc->ddp_comp[i]);
> +
>  	pm_runtime_put(drm->dev);
>  
>  	if (crtc->state->event && !crtc->state->active) {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index 5b0a3d4..097b90d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -78,6 +78,8 @@ struct mtk_ddp_comp_funcs {
>  	void (*stop)(struct mtk_ddp_comp *comp);
>  	void (*enable_vblank)(struct mtk_ddp_comp *comp, struct drm_crtc *crtc);
>  	void (*disable_vblank)(struct mtk_ddp_comp *comp);
> +	void (*prepare)(struct mtk_ddp_comp *comp);
> +	void (*unprepare)(struct mtk_ddp_comp *comp);
>  	unsigned int (*supported_rotations)(struct mtk_ddp_comp *comp);
>  	unsigned int (*layer_nr)(struct mtk_ddp_comp *comp);
>  	void (*layer_on)(struct mtk_ddp_comp *comp, unsigned int idx,
> @@ -117,6 +119,18 @@ static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
>  		comp->funcs->config(comp, w, h, vrefresh, bpc, cmdq_pkt);
>  }
>  
> +static inline void mtk_ddp_comp_prepare(struct mtk_ddp_comp *comp)
> +{
> +	if (comp->funcs && comp->funcs->prepare)
> +		comp->funcs->prepare(comp);
> +}
> +
> +static inline void mtk_ddp_comp_unprepare(struct mtk_ddp_comp *comp)
> +{
> +	if (comp->funcs && comp->funcs->unprepare)
> +		comp->funcs->unprepare(comp);
> +}
> +
>  static inline void mtk_ddp_comp_start(struct mtk_ddp_comp *comp)
>  {
>  	if (comp->funcs && comp->funcs->start)

Re: [PATCH v1, 1/2] drm/mediatek: Fixup external display black screen issue

[Yongqiang Niu]

On Tue, 2019-12-03 at 13:48 +0800, CK Hu wrote:
> Hi, Yongqiang:
> 
> On Wed, 2019-11-27 at 09:17 +0800, yongqiang.niu@mediatek.com wrote:
> > From: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > 
> > Problem:
> > overlay hangup when external display hotplut test
> > 
> > Fix:
> > disable overlay when crtc disable
> 
> I think you do two things in this patch. The first is to config layer
> before component start, and the second is disable layer when crtc
> disable. So separate to two patches.
> 
> > 
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 4fb346c..7eca02f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> >  	mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
> >  	mtk_disp_mutex_enable(mtk_crtc->mutex);
> >  
> > +	/* Initially configure all planes */
> > +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +		struct drm_plane *plane = &mtk_crtc->planes[i];
> > +		struct mtk_plane_state *plane_state;
> > +		struct mtk_ddp_comp *comp;
> > +		unsigned int local_layer;
> > +
> > +		plane_state = to_mtk_plane_state(plane->state);
> > +		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > +		if (comp)
> > +			mtk_ddp_comp_layer_config(comp, local_layer,
> > +						  plane_state, NULL);
> > +	}
> > +
> >  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> >  		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> >  		enum mtk_ddp_comp_id prev;
> > @@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> >  		mtk_ddp_comp_start(comp);
> >  	}
> >  
> > -	/* Initially configure all planes */
> > -	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > -		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > -		struct mtk_ddp_comp *comp;
> > -		unsigned int local_layer;
> > -
> > -		plane_state = to_mtk_plane_state(plane->state);
> > -		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > -		if (comp)
> > -			mtk_ddp_comp_layer_config(comp, local_layer,
> > -						  plane_state, NULL);
> > -	}
> > -
> >  	return 0;
> >  
> >  err_mutex_unprepare:
> > @@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
> >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> >  		struct mtk_plane_state *plane_state;
> > +		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> > +		unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> > +		unsigned int local_layer;
> >  
> >  		plane_state = to_mtk_plane_state(plane->state);
> >  		plane_state->pending.enable = false;
> >  		plane_state->pending.config = true;
> > +
> > +		if (i >= comp_layer_nr) {
> > +			comp = mtk_crtc->ddp_comp[1];
> > +			local_layer = i - comp_layer_nr;
> > +		} else
> > +			local_layer = i;
> > +		mtk_ddp_comp_layer_config(comp, local_layer,
> > +					  plane_state, NULL);
> 
> I'm confused with this part. The design of this loop is to set
> plane_state->pending.enable = false and wait for irq handler to write
> register. Why do you directly write register?
> 
> Regards,
> CK

when cmdq enable, mtk_crtc->cmdq_client will be always true when crtc
create, there is no chance for mtk_crtc_ddp_config process in ddp irq 
callback function
> 
> >  	}
> >  	mtk_crtc->pending_planes = true;
> >  
> 
> 

Re: [PATCH v1, 1/2] drm/mediatek: Fixup external display black screen issue

[CK Hu]

Hi, Yongqiang:

On Tue, 2019-12-03 at 15:43 +0800, Yongqiang Niu wrote:
> On Tue, 2019-12-03 at 13:48 +0800, CK Hu wrote:
> > Hi, Yongqiang:
> > 
> > On Wed, 2019-11-27 at 09:17 +0800, yongqiang.niu@mediatek.com wrote:
> > > From: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > > 
> > > Problem:
> > > overlay hangup when external display hotplut test
> > > 
> > > Fix:
> > > disable overlay when crtc disable
> > 
> > I think you do two things in this patch. The first is to config layer
> > before component start, and the second is disable layer when crtc
> > disable. So separate to two patches.
> > 
> > > 
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
> > >  1 file changed, 25 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > index 4fb346c..7eca02f 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > @@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > >  	mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
> > >  	mtk_disp_mutex_enable(mtk_crtc->mutex);
> > >  
> > > +	/* Initially configure all planes */
> > > +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > > +		struct drm_plane *plane = &mtk_crtc->planes[i];
> > > +		struct mtk_plane_state *plane_state;
> > > +		struct mtk_ddp_comp *comp;
> > > +		unsigned int local_layer;
> > > +
> > > +		plane_state = to_mtk_plane_state(plane->state);
> > > +		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > > +		if (comp)
> > > +			mtk_ddp_comp_layer_config(comp, local_layer,
> > > +						  plane_state, NULL);
> > > +	}
> > > +
> > >  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> > >  		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> > >  		enum mtk_ddp_comp_id prev;
> > > @@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > >  		mtk_ddp_comp_start(comp);
> > >  	}
> > >  
> > > -	/* Initially configure all planes */
> > > -	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > > -		struct drm_plane *plane = &mtk_crtc->planes[i];
> > > -		struct mtk_plane_state *plane_state;
> > > -		struct mtk_ddp_comp *comp;
> > > -		unsigned int local_layer;
> > > -
> > > -		plane_state = to_mtk_plane_state(plane->state);
> > > -		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > > -		if (comp)
> > > -			mtk_ddp_comp_layer_config(comp, local_layer,
> > > -						  plane_state, NULL);
> > > -	}
> > > -
> > >  	return 0;
> > >  
> > >  err_mutex_unprepare:
> > > @@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
> > >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> > >  		struct mtk_plane_state *plane_state;
> > > +		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> > > +		unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> > > +		unsigned int local_layer;
> > >  
> > >  		plane_state = to_mtk_plane_state(plane->state);
> > >  		plane_state->pending.enable = false;
> > >  		plane_state->pending.config = true;
> > > +
> > > +		if (i >= comp_layer_nr) {
> > > +			comp = mtk_crtc->ddp_comp[1];
> > > +			local_layer = i - comp_layer_nr;
> > > +		} else
> > > +			local_layer = i;
> > > +		mtk_ddp_comp_layer_config(comp, local_layer,
> > > +					  plane_state, NULL);
> > 
> > I'm confused with this part. The design of this loop is to set
> > plane_state->pending.enable = false and wait for irq handler to write
> > register. Why do you directly write register?
> > 
> > Regards,
> > CK
> 
> when cmdq enable, mtk_crtc->cmdq_client will be always true when crtc
> create, there is no chance for mtk_crtc_ddp_config process in ddp irq 
> callback function

I think this is a bug of patch [1] which has not been upstream yet. So
this part should be moved to that patch.

[1] https://patchwork.kernel.org/patch/11270637/

> > 
> > >  	}
> > >  	mtk_crtc->pending_planes = true;
> > >  
> > 
> > 
> 
> 

Re: [v1,1/2] drm/mediatek: Fixup external display black screen issue

[Hsin-Yi Wang]

On Wed, Nov 27, 2019 at 1:17 AM <yongqiang.niu@mediatek.com> wrote:
>
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> Problem:
> overlay hangup when external display hotplut test
>
> Fix:
> disable overlay when crtc disable
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
>
> --
> 1.8.1.1.dirty
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 4fb346c..7eca02f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>         mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
>         mtk_disp_mutex_enable(mtk_crtc->mutex);
>
> +       /* Initially configure all planes */
> +       for (i = 0; i < mtk_crtc->layer_nr; i++) {
> +               struct drm_plane *plane = &mtk_crtc->planes[i];
> +               struct mtk_plane_state *plane_state;
> +               struct mtk_ddp_comp *comp;
> +               unsigned int local_layer;
> +
> +               plane_state = to_mtk_plane_state(plane->state);
> +               comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> +               if (comp)
> +                       mtk_ddp_comp_layer_config(comp, local_layer,
> +                                                 plane_state, NULL);
> +       }
> +
>         for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
>                 struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
>                 enum mtk_ddp_comp_id prev;
> @@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>                 mtk_ddp_comp_start(comp);
>         }
>
> -       /* Initially configure all planes */
> -       for (i = 0; i < mtk_crtc->layer_nr; i++) {
> -               struct drm_plane *plane = &mtk_crtc->planes[i];
> -               struct mtk_plane_state *plane_state;
> -               struct mtk_ddp_comp *comp;
> -               unsigned int local_layer;
> -
> -               plane_state = to_mtk_plane_state(plane->state);
> -               comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> -               if (comp)
> -                       mtk_ddp_comp_layer_config(comp, local_layer,
> -                                                 plane_state, NULL);
> -       }
> -
>         return 0;
>
>  err_mutex_unprepare:
> @@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>         for (i = 0; i < mtk_crtc->layer_nr; i++) {
>                 struct drm_plane *plane = &mtk_crtc->planes[i];
>                 struct mtk_plane_state *plane_state;
> +               struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> +               unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> +               unsigned int local_layer;
>
>                 plane_state = to_mtk_plane_state(plane->state);
>                 plane_state->pending.enable = false;
>                 plane_state->pending.config = true;
> +
> +               if (i >= comp_layer_nr) {
> +                       comp = mtk_crtc->ddp_comp[1];
> +                       local_layer = i - comp_layer_nr;
> +               } else
> +                       local_layer = i;
> +               mtk_ddp_comp_layer_config(comp, local_layer,
> +                                         plane_state, NULL);
This part should be moved to mtk_crtc_ddp_hw_fini(), or at least
called after drm_crtc_vblank_off(). Otherwise we would see
drm_wait_one_vblank warnings on 8173 when display turns off.

[   25.696182] Call trace:
[   25.698624]  drm_wait_one_vblank+0x1f0/0x1fc
[   25.702886]  drm_crtc_wait_one_vblank+0x20/0x2c
[   25.707415]  mtk_drm_crtc_atomic_disable+0xf0/0x308
[   25.712287]  drm_atomic_helper_commit_modeset_disables+0x1b8/0x3c0
[   25.718461]  mtk_atomic_complete+0x88/0x16c
[   25.722638]  mtk_atomic_commit+0xa8/0xb0
[   25.726553]  drm_atomic_commit+0x50/0x5c
[   25.730469]  drm_atomic_helper_set_config+0x98/0xa0
[   25.735341]  drm_mode_setcrtc+0x280/0x608
[   25.739344]  drm_ioctl_kernel+0xcc/0x10c
[   25.743261]  drm_ioctl+0x240/0x3c0
[   25.746658]  drm_compat_ioctl+0xd8/0xe8
[   25.750487]  __se_compat_sys_ioctl+0x100/0x26fc
[   25.755009]  __arm64_compat_sys_ioctl+0x20/0x2c
[   25.759534]  el0_svc_common+0xa4/0x154
[   25.763277]  el0_svc_compat_handler+0x2c/0x38
[   25.767628]  el0_svc_compat+0x8/0x18
[   25.771195] ---[ end trace f4619fdac8f1c0ff ]---

>

Re: [v1,1/2] drm/mediatek: Fixup external display black screen issue

[Yongqiang Niu]

On Thu, 2019-12-05 at 14:15 +0800, Hsin-Yi Wang wrote:
> On Wed, Nov 27, 2019 at 1:17 AM <yongqiang.niu@mediatek.com> wrote:
> >
> > From: Yongqiang Niu <yongqiang.niu@mediatek.com>
> >
> > Problem:
> > overlay hangup when external display hotplut test
> >
> > Fix:
> > disable overlay when crtc disable
> >
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 39 +++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 14 deletions(-)
> >
> > --
> > 1.8.1.1.dirty
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 4fb346c..7eca02f 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -369,6 +369,20 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> >         mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
> >         mtk_disp_mutex_enable(mtk_crtc->mutex);
> >
> > +       /* Initially configure all planes */
> > +       for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +               struct drm_plane *plane = &mtk_crtc->planes[i];
> > +               struct mtk_plane_state *plane_state;
> > +               struct mtk_ddp_comp *comp;
> > +               unsigned int local_layer;
> > +
> > +               plane_state = to_mtk_plane_state(plane->state);
> > +               comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > +               if (comp)
> > +                       mtk_ddp_comp_layer_config(comp, local_layer,
> > +                                                 plane_state, NULL);
> > +       }
> > +
> >         for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> >                 struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> >                 enum mtk_ddp_comp_id prev;
> > @@ -385,20 +399,6 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> >                 mtk_ddp_comp_start(comp);
> >         }
> >
> > -       /* Initially configure all planes */
> > -       for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > -               struct drm_plane *plane = &mtk_crtc->planes[i];
> > -               struct mtk_plane_state *plane_state;
> > -               struct mtk_ddp_comp *comp;
> > -               unsigned int local_layer;
> > -
> > -               plane_state = to_mtk_plane_state(plane->state);
> > -               comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
> > -               if (comp)
> > -                       mtk_ddp_comp_layer_config(comp, local_layer,
> > -                                                 plane_state, NULL);
> > -       }
> > -
> >         return 0;
> >
> >  err_mutex_unprepare:
> > @@ -607,10 +607,21 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
> >         for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >                 struct drm_plane *plane = &mtk_crtc->planes[i];
> >                 struct mtk_plane_state *plane_state;
> > +               struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> > +               unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> > +               unsigned int local_layer;
> >
> >                 plane_state = to_mtk_plane_state(plane->state);
> >                 plane_state->pending.enable = false;
> >                 plane_state->pending.config = true;
> > +
> > +               if (i >= comp_layer_nr) {
> > +                       comp = mtk_crtc->ddp_comp[1];
> > +                       local_layer = i - comp_layer_nr;
> > +               } else
> > +                       local_layer = i;
> > +               mtk_ddp_comp_layer_config(comp, local_layer,
> > +                                         plane_state, NULL);
> This part should be moved to mtk_crtc_ddp_hw_fini(), or at least
> called after drm_crtc_vblank_off(). Otherwise we would see
> drm_wait_one_vblank warnings on 8173 when display turns off.
> 
> [   25.696182] Call trace:
> [   25.698624]  drm_wait_one_vblank+0x1f0/0x1fc
> [   25.702886]  drm_crtc_wait_one_vblank+0x20/0x2c
> [   25.707415]  mtk_drm_crtc_atomic_disable+0xf0/0x308
> [   25.712287]  drm_atomic_helper_commit_modeset_disables+0x1b8/0x3c0
> [   25.718461]  mtk_atomic_complete+0x88/0x16c
> [   25.722638]  mtk_atomic_commit+0xa8/0xb0
> [   25.726553]  drm_atomic_commit+0x50/0x5c
> [   25.730469]  drm_atomic_helper_set_config+0x98/0xa0
> [   25.735341]  drm_mode_setcrtc+0x280/0x608
> [   25.739344]  drm_ioctl_kernel+0xcc/0x10c
> [   25.743261]  drm_ioctl+0x240/0x3c0
> [   25.746658]  drm_compat_ioctl+0xd8/0xe8
> [   25.750487]  __se_compat_sys_ioctl+0x100/0x26fc
> [   25.755009]  __arm64_compat_sys_ioctl+0x20/0x2c
> [   25.759534]  el0_svc_common+0xa4/0x154
> [   25.763277]  el0_svc_compat_handler+0x2c/0x38
> [   25.767628]  el0_svc_compat+0x8/0x18
> [   25.771195] ---[ end trace f4619fdac8f1c0ff ]---
> 
> >
please double confirm is this dump stack log is ruining on MT8173 real
IC or not.
if yes, that may caused ovl hang when disable layer not in blanking,
then cause vblank time out.
i will disable overlay with cmdq in next version.


> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [v1,1/2] drm/mediatek: Fixup external display black screen issue

[Hsin-Yi Wang]

On Thu, Dec 5, 2019 at 3:13 PM Yongqiang Niu <yongqiang.niu@mediatek.com> wrote:
>
> please double confirm is this dump stack log is ruining on MT8173 real
> IC or not.
> if yes, that may caused ovl hang when disable layer not in blanking,
> then cause vblank time out.
> i will disable overlay with cmdq in next version.
>
It is running on MT8173 acer chromebook. Error only happens when
turning off display.
>