[PATCH 02/41] drm/rockchip: Get rid of some unnecessary code

[PATCH 02/41] drm/rockchip: Get rid of some unnecessary code

[Sean Paul]

From: Tomasz Figa <tfiga@chromium.org>

Current code implements prepare_fb and cleanup_fb callbacks only to
grab/release fb references, which is already done by atomic framework
when creating/destryoing plane state. Also there are some unused fields
vop and vop_win structs. Let's remove these unused bits.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 2151e1cee4b4..0539afb6b7c8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -108,9 +108,6 @@ struct vop {
 	struct drm_device *drm_dev;
 	bool is_enabled;
 
-	/* mutex vsync_ work */
-	struct mutex vsync_mutex;
-	bool vsync_work_pending;
 	struct completion dsp_hold_completion;
 
 	/* protected by dev->event_lock */
@@ -1592,8 +1589,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	spin_lock_init(&vop->reg_lock);
 	spin_lock_init(&vop->irq_lock);
 
-	mutex_init(&vop->vsync_mutex);
-
 	ret = devm_request_irq(dev, vop->irq, vop_isr,
 			       IRQF_SHARED, dev_name(dev), vop);
 	if (ret)
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 02/41] drm/rockchip: Get rid of some unnecessary code

[Heiko Stuebner]

Am Donnerstag, 9. M?rz 2017, 23:32:17 CET schrieb Sean Paul:
> From: Tomasz Figa <tfiga@chromium.org>
> 
> Current code implements prepare_fb and cleanup_fb callbacks only to
> grab/release fb references, which is already done by atomic framework
> when creating/destryoing plane state. Also there are some unused fields
> vop and vop_win structs. Let's remove these unused bits.
> 
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

There seems to be a slight discrepancy between patch message and code.
The commit message talks about *_fb-callbacks, while the patch removes
a seemingly unsused mutex, which I guess falls under the "some unused fields"
part.

In any case, I didn't see any ill effects when testing this patch, so
Tested-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 2151e1cee4b4..0539afb6b7c8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -108,9 +108,6 @@ struct vop {
>  	struct drm_device *drm_dev;
>  	bool is_enabled;
>  
> -	/* mutex vsync_ work */
> -	struct mutex vsync_mutex;
> -	bool vsync_work_pending;
>  	struct completion dsp_hold_completion;
>  
>  	/* protected by dev->event_lock */
> @@ -1592,8 +1589,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>  	spin_lock_init(&vop->reg_lock);
>  	spin_lock_init(&vop->irq_lock);
>  
> -	mutex_init(&vop->vsync_mutex);
> -
>  	ret = devm_request_irq(dev, vop->irq, vop_isr,
>  			       IRQF_SHARED, dev_name(dev), vop);
>  	if (ret)
> 

[PATCH 03/41] drm/rockchip: support prime import sg table

[Sean Paul]

From: Haixia Shi <hshi@chromium.org>

The prime fd to handle ioctl was not used with rockchip before. Support
was added in order to pass graphics_Gbm and to support potential uses
within Chrome OS (e.g. zero-copy video decode, camera).

Signed-off-by: Haixia Shi <hshi@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 118 ++++++++++++++++++++++++++--
 drivers/gpu/drm/rockchip/rockchip_drm_gem.h |   5 +-
 3 files changed, 115 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index ccf456938792..1e371b7a1506 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -238,6 +238,7 @@ static struct drm_driver rockchip_drm_driver = {
 	.gem_prime_import	= drm_gem_prime_import,
 	.gem_prime_export	= drm_gem_prime_export,
 	.gem_prime_get_sg_table	= rockchip_gem_prime_get_sg_table,
+	.gem_prime_import_sg_table	= rockchip_gem_prime_import_sg_table,
 	.gem_prime_vmap		= rockchip_gem_prime_vmap,
 	.gem_prime_vunmap	= rockchip_gem_prime_vunmap,
 	.gem_prime_mmap		= rockchip_gem_mmap_buf,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index df9e57064f19..5dffe276d2a7 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -16,6 +16,8 @@
 #include <drm/drmP.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_vma_manager.h>
+
+#include <linux/dma-buf.h>
 #include <linux/iommu.h>
 
 #include "rockchip_drm_drv.h"
@@ -308,12 +310,10 @@ static void rockchip_gem_release_object(struct rockchip_gem_object *rk_obj)
 }
 
 struct rockchip_gem_object *
-	rockchip_gem_create_object(struct drm_device *drm, unsigned int size,
-				   bool alloc_kmap)
+	rockchip_gem_alloc_object(struct drm_device *drm, unsigned int size)
 {
 	struct rockchip_gem_object *rk_obj;
 	struct drm_gem_object *obj;
-	int ret;
 
 	size = round_up(size, PAGE_SIZE);
 
@@ -325,6 +325,20 @@ struct rockchip_gem_object *
 
 	drm_gem_object_init(drm, obj, size);
 
+	return rk_obj;
+}
+
+struct rockchip_gem_object *
+rockchip_gem_create_object(struct drm_device *drm, unsigned int size,
+			   bool alloc_kmap)
+{
+	struct rockchip_gem_object *rk_obj;
+	int ret;
+
+	rk_obj = rockchip_gem_alloc_object(drm, size);
+	if (IS_ERR(rk_obj))
+		return rk_obj;
+
 	ret = rockchip_gem_alloc_buf(rk_obj, alloc_kmap);
 	if (ret)
 		goto err_free_rk_obj;
@@ -342,11 +356,21 @@ struct rockchip_gem_object *
  */
 void rockchip_gem_free_object(struct drm_gem_object *obj)
 {
-	struct rockchip_gem_object *rk_obj;
-
-	rk_obj = to_rockchip_obj(obj);
+	struct drm_device *drm = obj->dev;
+	struct rockchip_drm_private *private = drm->dev_private;
+	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
 
-	rockchip_gem_free_buf(rk_obj);
+	if (obj->import_attach) {
+		if (private->domain) {
+			rockchip_gem_iommu_unmap(rk_obj);
+		} else {
+			dma_unmap_sg(drm->dev, rk_obj->sgt->sgl,
+				     rk_obj->sgt->nents, DMA_BIDIRECTIONAL);
+		}
+		drm_prime_gem_destroy(obj, rk_obj->sgt);
+	} else {
+		rockchip_gem_free_buf(rk_obj);
+	}
 
 	rockchip_gem_release_object(rk_obj);
 }
@@ -476,6 +500,86 @@ struct sg_table *rockchip_gem_prime_get_sg_table(struct drm_gem_object *obj)
 	return sgt;
 }
 
+static unsigned long rockchip_sg_get_contiguous_size(struct sg_table *sgt,
+						     int count)
+{
+	struct scatterlist *s;
+	dma_addr_t expected = sg_dma_address(sgt->sgl);
+	unsigned int i;
+	unsigned long size = 0;
+
+	for_each_sg(sgt->sgl, s, count, i) {
+		if (sg_dma_address(s) != expected)
+			break;
+		expected = sg_dma_address(s) + sg_dma_len(s);
+		size += sg_dma_len(s);
+	}
+	return size;
+}
+
+static int
+rockchip_gem_iommu_map_sg(struct drm_device *drm,
+			  struct dma_buf_attachment *attach,
+			  struct sg_table *sg,
+			  struct rockchip_gem_object *rk_obj)
+{
+	rk_obj->sgt = sg;
+	return rockchip_gem_iommu_map(rk_obj);
+}
+
+static int
+rockchip_gem_dma_map_sg(struct drm_device *drm,
+			struct dma_buf_attachment *attach,
+			struct sg_table *sg,
+			struct rockchip_gem_object *rk_obj)
+{
+	int count = dma_map_sg(drm->dev, sg->sgl, sg->nents,
+			       DMA_BIDIRECTIONAL);
+	if (!count)
+		return -EINVAL;
+
+	if (rockchip_sg_get_contiguous_size(sg, count) < attach->dmabuf->size) {
+		DRM_ERROR("failed to map sg_table to contiguous linear address.\n");
+		dma_unmap_sg(drm->dev, sg->sgl, sg->nents,
+			     DMA_BIDIRECTIONAL);
+		return -EINVAL;
+	}
+
+	rk_obj->dma_addr = sg_dma_address(sg->sgl);
+	rk_obj->sgt = sg;
+	return 0;
+}
+
+struct drm_gem_object *
+rockchip_gem_prime_import_sg_table(struct drm_device *drm,
+				   struct dma_buf_attachment *attach,
+				   struct sg_table *sg)
+{
+	struct rockchip_drm_private *private = drm->dev_private;
+	struct rockchip_gem_object *rk_obj;
+	int ret;
+
+	rk_obj = rockchip_gem_alloc_object(drm, attach->dmabuf->size);
+	if (IS_ERR(rk_obj))
+		return ERR_CAST(rk_obj);
+
+	if (private->domain)
+		ret = rockchip_gem_iommu_map_sg(drm, attach, sg, rk_obj);
+	else
+		ret = rockchip_gem_dma_map_sg(drm, attach, sg, rk_obj);
+
+	if (ret < 0) {
+		DRM_ERROR("failed to import sg table: %d\n", ret);
+		goto err_free_rk_obj;
+	}
+
+	return &rk_obj->base;
+
+err_free_rk_obj:
+	rockchip_gem_release_object(rk_obj);
+	return ERR_PTR(ret);
+}
+
 void *rockchip_gem_prime_vmap(struct drm_gem_object *obj)
 {
 	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.h b/drivers/gpu/drm/rockchip/rockchip_drm_gem.h
index 3f6ea4d18a5c..89209a5f5e2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.h
@@ -36,8 +36,9 @@ struct rockchip_gem_object {
 
 struct sg_table *rockchip_gem_prime_get_sg_table(struct drm_gem_object *obj);
 struct drm_gem_object *
-rockchip_gem_prime_import_sg_table(struct drm_device *dev, size_t size,
-				   struct sg_table *sgt);
+rockchip_gem_prime_import_sg_table(struct drm_device *dev,
+				   struct dma_buf_attachment *attach,
+				   struct sg_table *sg);
 void *rockchip_gem_prime_vmap(struct drm_gem_object *obj);
 void rockchip_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
 
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 03/41] drm/rockchip: support prime import sg table

[Heiko Stuebner]

Am Donnerstag, 9. M?rz 2017, 23:32:18 CET schrieb Sean Paul:
> From: Haixia Shi <hshi@chromium.org>
> 
> The prime fd to handle ioctl was not used with rockchip before. Support
> was added in order to pass graphics_Gbm and to support potential uses
> within Chrome OS (e.g. zero-copy video decode, camera).
> 
> Signed-off-by: Haixia Shi <hshi@chromium.org>

missing a further Signed-off from Sean Paul as the sender.

I've tested this on the rk3328 together with the wip Lima driver and my wip
Mesa Rockchip Pipe and the kmscube improved from a failure about importing
the buffer to producing actual output, so

Tested-by: Heiko Stuebner <heiko@sntech.de>

[PATCH 04/41] drm/rockchip: Respect page offset for PRIME mmap calls

[Sean Paul]

From: ?rjan Eide <orjan.eide@arm.com>

When mapping external DMA-bufs through the PRIME mmap call, we might be
given an offset which has to be respected. However for the internal DRM
GEM mmap path, we have to ignore the fake mmap offset used to identify
the buffer only. Currently the code always zeroes out vma->vm_pgoff,
which breaks the former.

This patch fixes the problem by moving the vm_pgoff assignment to a
function that is used only for GEM mmap path, so that the PRIME path
retains the original offset.

BUG=chrome-os-partner:56615
TEST=graphics_GLBench

Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: ?rjan Eide <orjan.eide@arm.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 5dffe276d2a7..7fa2cb77bdc0 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -263,7 +263,6 @@ static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
 	 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
 	 */
 	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_pgoff = 0;
 
 	if (rk_obj->pages)
 		ret = rockchip_drm_gem_object_mmap_iommu(obj, vma);
@@ -298,6 +297,12 @@ int rockchip_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	if (ret)
 		return ret;
 
+	/*
+	 * Set vm_pgoff (used as a fake buffer offset by DRM) to 0 and map the
+	 * whole buffer from the start.
+	 */
+	vma->vm_pgoff = 0;
+
 	obj = vma->vm_private_data;
 
 	return rockchip_drm_gem_object_mmap(obj, vma);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 04/41] drm/rockchip: Respect page offset for PRIME mmap calls

[Heiko Stuebner]

Hi,

Am Donnerstag, 9. M?rz 2017, 23:32:19 CET schrieb Sean Paul:
> From: ?rjan Eide <orjan.eide@arm.com>
> 
> When mapping external DMA-bufs through the PRIME mmap call, we might be
> given an offset which has to be respected. However for the internal DRM
> GEM mmap path, we have to ignore the fake mmap offset used to identify
> the buffer only. Currently the code always zeroes out vma->vm_pgoff,
> which breaks the former.
> 
> This patch fixes the problem by moving the vm_pgoff assignment to a
> function that is used only for GEM mmap path, so that the PRIME path
> retains the original offset.
> 
> BUG=chrome-os-partner:56615
> TEST=graphics_GLBench

Unneeded chromeos-cruft.

> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: ?rjan Eide <orjan.eide@arm.com>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

With the same testas patch 3  (rk3328 + lima)
Tested-by: Heiko Stuebner <heiko@sntech.de>

[PATCH 05/41] drm/bridge: analogix_dp: set psr activate/deactivate when enable/disable bridge

[Sean Paul]

From: zain wang <wzz@rock-chips.com>

There's a race between when bridge_disable and when vop_crtc_disable
are called. If the flush timer triggers a new psr work between these,
we will operate eDP without power shutdowned by bridge_disable. In this
case, moving activate/deactivate to enable/disable bridge to avoid it.

Cc: St?phane Marchesin <marcheu@chromium.org>
Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |  7 +++++-
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c     | 30 ++++++++++++++++++++-----
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h     |  4 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |  4 ----
 4 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 8548e8271639..28c0d4406454 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -159,12 +159,17 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
 		return ret;
 	}
 
-	return 0;
+	return rockchip_drm_psr_activate(&dp->encoder);
 }
 
 static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = to_dp(plat_data);
+	int ret;
+
+	ret = rockchip_drm_psr_deactivate(&dp->encoder);
+	if (ret != 0)
+		return ret;
 
 	clk_disable_unprepare(dp->pclk);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index a553e182ff53..4c379e90b178 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -57,6 +57,24 @@ static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
 	return psr;
 }
 
+static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
+{
+	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
+	struct psr_drv *psr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	list_for_each_entry(psr, &drm_drv->psr_list, list) {
+		if (psr->encoder == encoder)
+			goto out;
+	}
+	psr = ERR_PTR(-ENODEV);
+
+out:
+	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	return psr;
+}
+
 static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state)
 {
 	/*
@@ -115,14 +133,14 @@ static void psr_flush_handler(unsigned long data)
 
 /**
  * rockchip_drm_psr_activate - activate PSR on the given pipe
- * @crtc: CRTC to obtain the PSR encoder
+ * @encoder: encoder to obtain the PSR encoder
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int rockchip_drm_psr_activate(struct drm_crtc *crtc)
+int rockchip_drm_psr_activate(struct drm_encoder *encoder)
 {
-	struct psr_drv *psr = find_psr_by_crtc(crtc);
+	struct psr_drv *psr = find_psr_by_encoder(encoder);
 	unsigned long flags;
 
 	if (IS_ERR(psr))
@@ -138,14 +156,14 @@ EXPORT_SYMBOL(rockchip_drm_psr_activate);
 
 /**
  * rockchip_drm_psr_deactivate - deactivate PSR on the given pipe
- * @crtc: CRTC to obtain the PSR encoder
+ * @encoder: encoder to obtain the PSR encoder
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
-int rockchip_drm_psr_deactivate(struct drm_crtc *crtc)
+int rockchip_drm_psr_deactivate(struct drm_encoder *encoder)
 {
-	struct psr_drv *psr = find_psr_by_crtc(crtc);
+	struct psr_drv *psr = find_psr_by_encoder(encoder);
 	unsigned long flags;
 
 	if (IS_ERR(psr))
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
index b420cf1bf902..b1ea0155e57c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
@@ -18,8 +18,8 @@
 void rockchip_drm_psr_flush_all(struct drm_device *dev);
 int rockchip_drm_psr_flush(struct drm_crtc *crtc);
 
-int rockchip_drm_psr_activate(struct drm_crtc *crtc);
-int rockchip_drm_psr_deactivate(struct drm_crtc *crtc);
+int rockchip_drm_psr_activate(struct drm_encoder *encoder);
+int rockchip_drm_psr_deactivate(struct drm_encoder *encoder);
 
 int rockchip_drm_psr_register(struct drm_encoder *encoder,
 			void (*psr_set)(struct drm_encoder *, bool enable));
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 0539afb6b7c8..1c1e8535ad28 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -568,8 +568,6 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 
 	WARN_ON(vop->event);
 
-	rockchip_drm_psr_deactivate(&vop->crtc);
-
 	/*
 	 * We need to make sure that all windows are disabled before we
 	 * disable that crtc. Otherwise we might try to scan from a destroyed
@@ -981,8 +979,6 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
 
 	VOP_CTRL_SET(vop, standby, 0);
-
-	rockchip_drm_psr_activate(&vop->crtc);
 }
 
 static bool vop_fs_irq_is_pending(struct vop *vop)
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 07/41] drm/rockchip: Don't use atomic constructs for psr

[Sean Paul]

Instead of using timer and spinlocks, use delayed_work and
mutexes for rockchip psr. This allows us to make blocking
calls when enabling/disabling psr (which is sort of important
given we're talking over dpcd to the display).

Cc: Caesar Wang <wxt@rock-chips.com>
Cc: ?? ? <wzz@rock-chips.com>
Cc: St?phane Marchesin <marcheu@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 14 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c     |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c     | 68 +++++++++++--------------
 4 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 28c0d4406454..90eb7908e2e5 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -72,7 +72,7 @@ struct rockchip_dp_device {
 	struct reset_control     *rst;
 
 	struct work_struct	 psr_work;
-	spinlock_t		 psr_lock;
+	struct mutex		 psr_lock;
 	unsigned int             psr_state;
 
 	const struct rockchip_dp_chip_data *data;
@@ -83,21 +83,20 @@ struct rockchip_dp_device {
 static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 {
 	struct rockchip_dp_device *dp = to_dp(encoder);
-	unsigned long flags;
 
 	if (!analogix_dp_psr_supported(dp->dev))
 		return;
 
 	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
 
-	spin_lock_irqsave(&dp->psr_lock, flags);
+	mutex_lock(&dp->psr_lock);
 	if (enabled)
 		dp->psr_state = EDP_VSC_PSR_STATE_ACTIVE;
 	else
 		dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
 
 	schedule_work(&dp->psr_work);
-	spin_unlock_irqrestore(&dp->psr_lock, flags);
+	mutex_unlock(&dp->psr_lock);
 }
 
 static void analogix_dp_psr_work(struct work_struct *work)
@@ -108,7 +107,6 @@ static void analogix_dp_psr_work(struct work_struct *work)
 	int psr_state = dp->psr_state;
 	int vact_end;
 	int ret;
-	unsigned long flags;
 
 	if (!crtc)
 		return;
@@ -122,12 +120,12 @@ static void analogix_dp_psr_work(struct work_struct *work)
 		return;
 	}
 
-	spin_lock_irqsave(&dp->psr_lock, flags);
+	mutex_lock(&dp->psr_lock);
 	if (psr_state == EDP_VSC_PSR_STATE_ACTIVE)
 		analogix_dp_enable_psr(dp->dev);
 	else
 		analogix_dp_disable_psr(dp->dev);
-	spin_unlock_irqrestore(&dp->psr_lock, flags);
+	mutex_unlock(&dp->psr_lock);
 }
 
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
@@ -406,7 +404,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
-	spin_lock_init(&dp->psr_lock);
+	mutex_init(&dp->psr_lock);
 	dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
 	INIT_WORK(&dp->psr_work, analogix_dp_psr_work);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 1e371b7a1506..6b767b8dabd5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -134,7 +134,7 @@ static int rockchip_drm_bind(struct device *dev)
 	drm_dev->dev_private = private;
 
 	INIT_LIST_HEAD(&private->psr_list);
-	spin_lock_init(&private->psr_list_lock);
+	mutex_init(&private->psr_list_lock);
 
 	drm_mode_config_init(drm_dev);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 8aca219ec4c8..1844951118da 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -55,7 +55,7 @@ struct rockchip_drm_private {
 	struct mutex mm_lock;
 	struct drm_mm mm;
 	struct list_head psr_list;
-	spinlock_t psr_list_lock;
+	struct mutex psr_list_lock;
 };
 
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index 4c379e90b178..b339ca943139 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -18,7 +18,7 @@
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_psr.h"
 
-#define PSR_FLUSH_TIMEOUT	msecs_to_jiffies(100)
+#define PSR_FLUSH_TIMEOUT_MS	100
 
 enum psr_state {
 	PSR_FLUSH,
@@ -30,11 +30,11 @@ struct psr_drv {
 	struct list_head	list;
 	struct drm_encoder	*encoder;
 
-	spinlock_t		lock;
+	struct mutex		lock;
 	bool			active;
 	enum psr_state		state;
 
-	struct timer_list	flush_timer;
+	struct delayed_work	flush_work;
 
 	void (*set)(struct drm_encoder *encoder, bool enable);
 };
@@ -43,9 +43,8 @@ static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
 {
 	struct rockchip_drm_private *drm_drv = crtc->dev->dev_private;
 	struct psr_drv *psr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	mutex_lock(&drm_drv->psr_list_lock);
 	list_for_each_entry(psr, &drm_drv->psr_list, list) {
 		if (psr->encoder->crtc == crtc)
 			goto out;
@@ -53,7 +52,7 @@ static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
 	psr = ERR_PTR(-ENODEV);
 
 out:
-	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	mutex_unlock(&drm_drv->psr_list_lock);
 	return psr;
 }
 
@@ -61,9 +60,8 @@ static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
 {
 	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
 	struct psr_drv *psr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	mutex_lock(&drm_drv->psr_list_lock);
 	list_for_each_entry(psr, &drm_drv->psr_list, list) {
 		if (psr->encoder == encoder)
 			goto out;
@@ -71,7 +69,7 @@ static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
 	psr = ERR_PTR(-ENODEV);
 
 out:
-	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	mutex_unlock(&drm_drv->psr_list_lock);
 	return psr;
 }
 
@@ -112,23 +110,21 @@ static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state)
 
 static void psr_set_state(struct psr_drv *psr, enum psr_state state)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&psr->lock, flags);
+	mutex_lock(&psr->lock);
 	psr_set_state_locked(psr, state);
-	spin_unlock_irqrestore(&psr->lock, flags);
+	mutex_unlock(&psr->lock);
 }
 
-static void psr_flush_handler(unsigned long data)
+static void psr_flush_handler(struct work_struct *work)
 {
-	struct psr_drv *psr = (struct psr_drv *)data;
-	unsigned long flags;
+	struct psr_drv *psr = container_of(to_delayed_work(work),
+					   struct psr_drv, flush_work);
 
 	/* If the state has changed since we initiated the flush, do nothing */
-	spin_lock_irqsave(&psr->lock, flags);
+	mutex_lock(&psr->lock);
 	if (psr->state == PSR_FLUSH)
 		psr_set_state_locked(psr, PSR_ENABLE);
-	spin_unlock_irqrestore(&psr->lock, flags);
+	mutex_unlock(&psr->lock);
 }
 
 /**
@@ -141,14 +137,13 @@ static void psr_flush_handler(unsigned long data)
 int rockchip_drm_psr_activate(struct drm_encoder *encoder)
 {
 	struct psr_drv *psr = find_psr_by_encoder(encoder);
-	unsigned long flags;
 
 	if (IS_ERR(psr))
 		return PTR_ERR(psr);
 
-	spin_lock_irqsave(&psr->lock, flags);
+	mutex_lock(&psr->lock);
 	psr->active = true;
-	spin_unlock_irqrestore(&psr->lock, flags);
+	mutex_unlock(&psr->lock);
 
 	return 0;
 }
@@ -164,15 +159,14 @@ EXPORT_SYMBOL(rockchip_drm_psr_activate);
 int rockchip_drm_psr_deactivate(struct drm_encoder *encoder)
 {
 	struct psr_drv *psr = find_psr_by_encoder(encoder);
-	unsigned long flags;
 
 	if (IS_ERR(psr))
 		return PTR_ERR(psr);
 
-	spin_lock_irqsave(&psr->lock, flags);
+	mutex_lock(&psr->lock);
 	psr->active = false;
-	spin_unlock_irqrestore(&psr->lock, flags);
-	del_timer_sync(&psr->flush_timer);
+	mutex_unlock(&psr->lock);
+	cancel_delayed_work_sync(&psr->flush_work);
 
 	return 0;
 }
@@ -180,9 +174,8 @@ EXPORT_SYMBOL(rockchip_drm_psr_deactivate);
 
 static void rockchip_drm_do_flush(struct psr_drv *psr)
 {
-	mod_timer(&psr->flush_timer,
-		  round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
 	psr_set_state(psr, PSR_FLUSH);
+	mod_delayed_work(system_wq, &psr->flush_work, PSR_FLUSH_TIMEOUT_MS);
 }
 
 /**
@@ -219,12 +212,11 @@ void rockchip_drm_psr_flush_all(struct drm_device *dev)
 {
 	struct rockchip_drm_private *drm_drv = dev->dev_private;
 	struct psr_drv *psr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	mutex_lock(&drm_drv->psr_list_lock);
 	list_for_each_entry(psr, &drm_drv->psr_list, list)
 		rockchip_drm_do_flush(psr);
-	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	mutex_unlock(&drm_drv->psr_list_lock);
 }
 EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
 
@@ -241,7 +233,6 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
 {
 	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
 	struct psr_drv *psr;
-	unsigned long flags;
 
 	if (!encoder || !psr_set)
 		return -EINVAL;
@@ -250,17 +241,17 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
 	if (!psr)
 		return -ENOMEM;
 
-	setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
-	spin_lock_init(&psr->lock);
+	INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
+	mutex_init(&psr->lock);
 
 	psr->active = true;
 	psr->state = PSR_DISABLE;
 	psr->encoder = encoder;
 	psr->set = psr_set;
 
-	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	mutex_lock(&drm_drv->psr_list_lock);
 	list_add_tail(&psr->list, &drm_drv->psr_list);
-	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	mutex_unlock(&drm_drv->psr_list_lock);
 
 	return 0;
 }
@@ -278,16 +269,15 @@ void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
 {
 	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
 	struct psr_drv *psr, *n;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
+	mutex_lock(&drm_drv->psr_list_lock);
 	list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
 		if (psr->encoder == encoder) {
-			del_timer(&psr->flush_timer);
+			cancel_delayed_work_sync(&psr->flush_work);
 			list_del(&psr->list);
 			kfree(psr);
 		}
 	}
-	spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
+	mutex_unlock(&drm_drv->psr_list_lock);
 }
 EXPORT_SYMBOL(rockchip_drm_psr_unregister);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 09/41] drm/rockchip: Remove analogix psr worker

[Sean Paul]

Now that the spinlocks and timers are gone, we can remove the psr
worker located in rockchip's analogix driver and do the enable/disable
directly. This should simplify the code and remove races on disable.

Cc: ?? ? <wzz@rock-chips.com>
Cc: St?phane Marchesin <marcheu@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 38 ++++---------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 90eb7908e2e5..64e7e2c0bc58 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -71,10 +71,6 @@ struct rockchip_dp_device {
 	struct regmap            *grf;
 	struct reset_control     *rst;
 
-	struct work_struct	 psr_work;
-	struct mutex		 psr_lock;
-	unsigned int             psr_state;
-
 	const struct rockchip_dp_chip_data *data;
 
 	struct analogix_dp_plat_data plat_data;
@@ -83,30 +79,14 @@ struct rockchip_dp_device {
 static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 {
 	struct rockchip_dp_device *dp = to_dp(encoder);
+	struct drm_crtc *crtc = dp->encoder.crtc;
+	int vact_end;
+	int ret;
 
 	if (!analogix_dp_psr_supported(dp->dev))
 		return;
 
-	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
-
-	mutex_lock(&dp->psr_lock);
-	if (enabled)
-		dp->psr_state = EDP_VSC_PSR_STATE_ACTIVE;
-	else
-		dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
-
-	schedule_work(&dp->psr_work);
-	mutex_unlock(&dp->psr_lock);
-}
-
-static void analogix_dp_psr_work(struct work_struct *work)
-{
-	struct rockchip_dp_device *dp =
-				container_of(work, typeof(*dp), psr_work);
-	struct drm_crtc *crtc = dp->encoder.crtc;
-	int psr_state = dp->psr_state;
-	int vact_end;
-	int ret;
+	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
 
 	if (!crtc)
 		return;
@@ -120,12 +100,10 @@ static void analogix_dp_psr_work(struct work_struct *work)
 		return;
 	}
 
-	mutex_lock(&dp->psr_lock);
-	if (psr_state == EDP_VSC_PSR_STATE_ACTIVE)
+	if (enabled)
 		analogix_dp_enable_psr(dp->dev);
 	else
 		analogix_dp_disable_psr(dp->dev);
-	mutex_unlock(&dp->psr_lock);
 }
 
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
@@ -142,8 +120,6 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
 	struct rockchip_dp_device *dp = to_dp(plat_data);
 	int ret;
 
-	cancel_work_sync(&dp->psr_work);
-
 	ret = clk_prepare_enable(dp->pclk);
 	if (ret < 0) {
 		dev_err(dp->dev, "failed to enable pclk %d\n", ret);
@@ -404,10 +380,6 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
-	mutex_init(&dp->psr_lock);
-	dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;
-	INIT_WORK(&dp->psr_work, analogix_dp_psr_work);
-
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
 	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Sean Paul]

From: zain wang <wzz@rock-chips.com>

There is a race between AUX CH bring-up and enabling bridge which will
cause link training to fail. To avoid hitting it, don't change psr state
while enabling the bridge.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
[seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
 include/drm/bridge/analogix_dp.h                   |  2 +-
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 8a8f05fe9da3..64d94a34874d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 	return 0;
 }
 
-int analogix_dp_psr_supported(struct device *dev)
+int analogix_dp_psr_enabled(struct device *dev)
 {
 	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 
-	return dp->psr_support;
+	return dp->psr_enable;
 }
-EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
+EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
 
 int analogix_dp_enable_psr(struct device *dev)
 {
 	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 	struct edp_vsc_psr psr_vsc;
 
-	if (!dp->psr_support)
+	if (!dp->psr_enable)
 		return 0;
 
 	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
@@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
 	struct edp_vsc_psr psr_vsc;
 	int ret;
 
-	if (!dp->psr_support)
+	if (!dp->psr_enable)
 		return 0;
 
 	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
@@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
 	/* Enable video */
 	analogix_dp_start_video(dp);
 
-	dp->psr_support = analogix_dp_detect_sink_psr(dp);
-	if (dp->psr_support)
+	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
+	if (dp->psr_enable)
 		analogix_dp_enable_sink_psr(dp);
 }
 
@@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	if (ret)
 		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
 
+	dp->psr_enable = false;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b039b28d8fcc..e135a42cb19e 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -170,7 +170,7 @@ struct analogix_dp_device {
 	int			dpms_mode;
 	int			hpd_gpio;
 	bool                    force_hpd;
-	bool			psr_support;
+	bool			psr_enable;
 
 	struct mutex		panel_lock;
 	bool			panel_is_modeset;
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 64e7e2c0bc58..f44756029478 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 	int vact_end;
 	int ret;
 
-	if (!analogix_dp_psr_supported(dp->dev))
+	if (!analogix_dp_psr_enabled(dp->dev))
 		return;
 
 	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index c99d6eaef1ac..4fc0165ed3f5 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
 			 struct drm_connector *);
 };
 
-int analogix_dp_psr_supported(struct device *dev);
+int analogix_dp_psr_enabled(struct device *dev);
 int analogix_dp_enable_psr(struct device *dev);
 int analogix_dp_disable_psr(struct device *dev);
 
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Andrzej Hajda]

On 10.03.2017 05:32, Sean Paul wrote:
> From: zain wang <wzz@rock-chips.com>
>
> There is a race between AUX CH bring-up and enabling bridge which will
> cause link training to fail. To avoid hitting it, don't change psr state
> while enabling the bridge.
>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: zain wang <wzz@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> [seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Hmm, beside resetting psr_enable in analogix_dp_bridge_disable I do not
see functional change, or am I blind?

Regards
Andrzej

> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
>  include/drm/bridge/analogix_dp.h                   |  2 +-
>  4 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 8a8f05fe9da3..64d94a34874d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>  	return 0;
>  }
>  
> -int analogix_dp_psr_supported(struct device *dev)
> +int analogix_dp_psr_enabled(struct device *dev)
>  {
>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>  
> -	return dp->psr_support;
> +	return dp->psr_enable;
>  }
> -EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
> +EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
>  
>  int analogix_dp_enable_psr(struct device *dev)
>  {
>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>  	struct edp_vsc_psr psr_vsc;
>  
> -	if (!dp->psr_support)
> +	if (!dp->psr_enable)
>  		return 0;
>  
>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> @@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
>  	struct edp_vsc_psr psr_vsc;
>  	int ret;
>  
> -	if (!dp->psr_support)
> +	if (!dp->psr_enable)
>  		return 0;
>  
>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> @@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
>  	/* Enable video */
>  	analogix_dp_start_video(dp);
>  
> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
> -	if (dp->psr_support)
> +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
> +	if (dp->psr_enable)
>  		analogix_dp_enable_sink_psr(dp);
>  }
>  
> @@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>  	if (ret)
>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>  
> +	dp->psr_enable = false;
>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>  }
>  
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b039b28d8fcc..e135a42cb19e 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -170,7 +170,7 @@ struct analogix_dp_device {
>  	int			dpms_mode;
>  	int			hpd_gpio;
>  	bool                    force_hpd;
> -	bool			psr_support;
> +	bool			psr_enable;
>  
>  	struct mutex		panel_lock;
>  	bool			panel_is_modeset;
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 64e7e2c0bc58..f44756029478 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
>  	int vact_end;
>  	int ret;
>  
> -	if (!analogix_dp_psr_supported(dp->dev))
> +	if (!analogix_dp_psr_enabled(dp->dev))
>  		return;
>  
>  	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index c99d6eaef1ac..4fc0165ed3f5 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
>  			 struct drm_connector *);
>  };
>  
> -int analogix_dp_psr_supported(struct device *dev);
> +int analogix_dp_psr_enabled(struct device *dev);
>  int analogix_dp_enable_psr(struct device *dev);
>  int analogix_dp_disable_psr(struct device *dev);
>  

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Sean Paul]

On Thu, Mar 16, 2017 at 02:40:21PM +0100, Andrzej Hajda wrote:
> On 10.03.2017 05:32, Sean Paul wrote:
> > From: zain wang <wzz@rock-chips.com>
> >
> > There is a race between AUX CH bring-up and enabling bridge which will
> > cause link training to fail. To avoid hitting it, don't change psr state
> > while enabling the bridge.
> >
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: zain wang <wzz@rock-chips.com>
> > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > [seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> Hmm, beside resetting psr_enable in analogix_dp_bridge_disable I do not
> see functional change, or am I blind?

The patch also adds a check of psr_enable in analogix_dp_commit. This will avoid
trying to enable psr when the bridge is disabled (that's why psr_enable is
reset).

Sean

> 
> Regards
> Andrzej
> 
> > ---
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
> >  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
> >  include/drm/bridge/analogix_dp.h                   |  2 +-
> >  4 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 8a8f05fe9da3..64d94a34874d 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
> >  	return 0;
> >  }
> >  
> > -int analogix_dp_psr_supported(struct device *dev)
> > +int analogix_dp_psr_enabled(struct device *dev)
> >  {
> >  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
> >  
> > -	return dp->psr_support;
> > +	return dp->psr_enable;
> >  }
> > -EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
> > +EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> >  
> >  int analogix_dp_enable_psr(struct device *dev)
> >  {
> >  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
> >  	struct edp_vsc_psr psr_vsc;
> >  
> > -	if (!dp->psr_support)
> > +	if (!dp->psr_enable)
> >  		return 0;
> >  
> >  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > @@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
> >  	struct edp_vsc_psr psr_vsc;
> >  	int ret;
> >  
> > -	if (!dp->psr_support)
> > +	if (!dp->psr_enable)
> >  		return 0;
> >  
> >  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > @@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> >  	/* Enable video */
> >  	analogix_dp_start_video(dp);
> >  
> > -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
> > -	if (dp->psr_support)
> > +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
> > +	if (dp->psr_enable)
> >  		analogix_dp_enable_sink_psr(dp);
> >  }
> >  
> > @@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> >  	if (ret)
> >  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> >  
> > +	dp->psr_enable = false;
> >  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > index b039b28d8fcc..e135a42cb19e 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > @@ -170,7 +170,7 @@ struct analogix_dp_device {
> >  	int			dpms_mode;
> >  	int			hpd_gpio;
> >  	bool                    force_hpd;
> > -	bool			psr_support;
> > +	bool			psr_enable;
> >  
> >  	struct mutex		panel_lock;
> >  	bool			panel_is_modeset;
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index 64e7e2c0bc58..f44756029478 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
> >  	int vact_end;
> >  	int ret;
> >  
> > -	if (!analogix_dp_psr_supported(dp->dev))
> > +	if (!analogix_dp_psr_enabled(dp->dev))
> >  		return;
> >  
> >  	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
> > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> > index c99d6eaef1ac..4fc0165ed3f5 100644
> > --- a/include/drm/bridge/analogix_dp.h
> > +++ b/include/drm/bridge/analogix_dp.h
> > @@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
> >  			 struct drm_connector *);
> >  };
> >  
> > -int analogix_dp_psr_supported(struct device *dev);
> > +int analogix_dp_psr_enabled(struct device *dev);
> >  int analogix_dp_enable_psr(struct device *dev);
> >  int analogix_dp_disable_psr(struct device *dev);
> >  
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Andrzej Hajda]

On 21.03.2017 20:58, Sean Paul wrote:
> On Thu, Mar 16, 2017 at 02:40:21PM +0100, Andrzej Hajda wrote:
>> On 10.03.2017 05:32, Sean Paul wrote:
>>> From: zain wang <wzz@rock-chips.com>
>>>
>>> There is a race between AUX CH bring-up and enabling bridge which will
>>> cause link training to fail. To avoid hitting it, don't change psr state
>>> while enabling the bridge.
>>>
>>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Signed-off-by: zain wang <wzz@rock-chips.com>
>>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>>> [seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Hmm, beside resetting psr_enable in analogix_dp_bridge_disable I do not
>> see functional change, or am I blind?
> The patch also adds a check of psr_enable in analogix_dp_commit. This will avoid
> trying to enable psr when the bridge is disabled (that's why psr_enable is
> reset).

Copy/paste from analogix_dp_commit chunk below:

-	dp->psr_support = analogix_dp_detect_sink_psr(dp);
-	if (dp->psr_support)
+	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
+	if (dp->psr_enable)
 		analogix_dp_enable_sink_psr(dp);


What is added here?

Regards
Andrzej

>
> Sean
>
>> Regards
>> Andrzej
>>
>>> ---
>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
>>>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
>>>  include/drm/bridge/analogix_dp.h                   |  2 +-
>>>  4 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 8a8f05fe9da3..64d94a34874d 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>>  	return 0;
>>>  }
>>>  
>>> -int analogix_dp_psr_supported(struct device *dev)
>>> +int analogix_dp_psr_enabled(struct device *dev)
>>>  {
>>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>  
>>> -	return dp->psr_support;
>>> +	return dp->psr_enable;
>>>  }
>>> -EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
>>> +EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
>>>  
>>>  int analogix_dp_enable_psr(struct device *dev)
>>>  {
>>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>  	struct edp_vsc_psr psr_vsc;
>>>  
>>> -	if (!dp->psr_support)
>>> +	if (!dp->psr_enable)
>>>  		return 0;
>>>  
>>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>> @@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
>>>  	struct edp_vsc_psr psr_vsc;
>>>  	int ret;
>>>  
>>> -	if (!dp->psr_support)
>>> +	if (!dp->psr_enable)
>>>  		return 0;
>>>  
>>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>> @@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
>>>  	/* Enable video */
>>>  	analogix_dp_start_video(dp);
>>>  
>>> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>> -	if (dp->psr_support)
>>> +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
>>> +	if (dp->psr_enable)
>>>  		analogix_dp_enable_sink_psr(dp);
>>>  }
>>>  
>>> @@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>>  	if (ret)
>>>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>>>  
>>> +	dp->psr_enable = false;
>>>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>>>  }
>>>  
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> index b039b28d8fcc..e135a42cb19e 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> @@ -170,7 +170,7 @@ struct analogix_dp_device {
>>>  	int			dpms_mode;
>>>  	int			hpd_gpio;
>>>  	bool                    force_hpd;
>>> -	bool			psr_support;
>>> +	bool			psr_enable;
>>>  
>>>  	struct mutex		panel_lock;
>>>  	bool			panel_is_modeset;
>>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> index 64e7e2c0bc58..f44756029478 100644
>>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> @@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
>>>  	int vact_end;
>>>  	int ret;
>>>  
>>> -	if (!analogix_dp_psr_supported(dp->dev))
>>> +	if (!analogix_dp_psr_enabled(dp->dev))
>>>  		return;
>>>  
>>>  	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
>>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>>> index c99d6eaef1ac..4fc0165ed3f5 100644
>>> --- a/include/drm/bridge/analogix_dp.h
>>> +++ b/include/drm/bridge/analogix_dp.h
>>> @@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
>>>  			 struct drm_connector *);
>>>  };
>>>  
>>> -int analogix_dp_psr_supported(struct device *dev);
>>> +int analogix_dp_psr_enabled(struct device *dev);
>>>  int analogix_dp_enable_psr(struct device *dev);
>>>  int analogix_dp_disable_psr(struct device *dev);
>>>  

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Sean Paul]

On Wed, Mar 22, 2017 at 09:36:34AM +0100, Andrzej Hajda wrote:
> On 21.03.2017 20:58, Sean Paul wrote:
> > On Thu, Mar 16, 2017 at 02:40:21PM +0100, Andrzej Hajda wrote:
> >> On 10.03.2017 05:32, Sean Paul wrote:
> >>> From: zain wang <wzz@rock-chips.com>
> >>>
> >>> There is a race between AUX CH bring-up and enabling bridge which will
> >>> cause link training to fail. To avoid hitting it, don't change psr state
> >>> while enabling the bridge.
> >>>
> >>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >>> Cc: Sean Paul <seanpaul@chromium.org>
> >>> Signed-off-by: zain wang <wzz@rock-chips.com>
> >>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> >>> [seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
> >>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> Hmm, beside resetting psr_enable in analogix_dp_bridge_disable I do not
> >> see functional change, or am I blind?
> > The patch also adds a check of psr_enable in analogix_dp_commit. This will avoid
> > trying to enable psr when the bridge is disabled (that's why psr_enable is
> > reset).
> 
> Copy/paste from analogix_dp_commit chunk below:
> 
> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
> -	if (dp->psr_support)
> +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
> +	if (dp->psr_enable)
>  		analogix_dp_enable_sink_psr(dp);
> 
> 
> What is added here?

I guess this is what I get for skimming, apologies. I'll hopefully do a better
job explaining below.


> 
> Regards
> Andrzej
> 
> >
> > Sean
> >
> >> Regards
> >> Andrzej
> >>
> >>> ---
> >>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
> >>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
> >>>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
> >>>  include/drm/bridge/analogix_dp.h                   |  2 +-
> >>>  4 files changed, 11 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >>> index 8a8f05fe9da3..64d94a34874d 100644
> >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >>> @@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> -int analogix_dp_psr_supported(struct device *dev)
> >>> +int analogix_dp_psr_enabled(struct device *dev)
> >>>  {
> >>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
> >>>  
> >>> -	return dp->psr_support;
> >>> +	return dp->psr_enable;
> >>>  }
> >>> -EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
> >>> +EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> >>>  
> >>>  int analogix_dp_enable_psr(struct device *dev)
> >>>  {
> >>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
> >>>  	struct edp_vsc_psr psr_vsc;
> >>>  
> >>> -	if (!dp->psr_support)
> >>> +	if (!dp->psr_enable)

This gates enabling psr on the panel. Previously it was put in place to avoid
trying to enable psr on devices which did not support it.

> >>>  		return 0;
> >>>  
> >>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> >>> @@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
> >>>  	struct edp_vsc_psr psr_vsc;
> >>>  	int ret;
> >>>  
> >>> -	if (!dp->psr_support)
> >>> +	if (!dp->psr_enable)
> >>>  		return 0;
> >>>  
> >>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> >>> @@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> >>>  	/* Enable video */
> >>>  	analogix_dp_start_video(dp);
> >>>  
> >>> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
> >>> -	if (dp->psr_support)
> >>> +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
> >>> +	if (dp->psr_enable)

This probes the hardware to see if psr is supported. If it is, psr_enable will
transition to true. Before this patch, psr_support/psr_enable would be flipped
true on the first modeset and stay that way forever, even when the panel was
turned off.

> >>>  		analogix_dp_enable_sink_psr(dp);
> >>>  }
> >>>  
> >>> @@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> >>>  	if (ret)
> >>>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> >>>  
> >>> +	dp->psr_enable = false;

By setting psr_enable to false here, the driver ensures that any subsequent calls
to analogix_dp_enable_psr() will return early and will not attempt to change
hardware (that would fail). Once the panel is re-enabled, psr_enable will return
to true if the hardware supports it.

This is why the variable was renamed from psr_support to psr_enable. It no
longer just tracks whether psr is supported, but rather that it is supported and
eligible to be enabled.

Sean


> >>>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> >>>  }
> >>>  
> >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> >>> index b039b28d8fcc..e135a42cb19e 100644
> >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> >>> @@ -170,7 +170,7 @@ struct analogix_dp_device {
> >>>  	int			dpms_mode;
> >>>  	int			hpd_gpio;
> >>>  	bool                    force_hpd;
> >>> -	bool			psr_support;
> >>> +	bool			psr_enable;
> >>>  
> >>>  	struct mutex		panel_lock;
> >>>  	bool			panel_is_modeset;
> >>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> >>> index 64e7e2c0bc58..f44756029478 100644
> >>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> >>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> >>> @@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
> >>>  	int vact_end;
> >>>  	int ret;
> >>>  
> >>> -	if (!analogix_dp_psr_supported(dp->dev))
> >>> +	if (!analogix_dp_psr_enabled(dp->dev))
> >>>  		return;
> >>>  
> >>>  	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
> >>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> >>> index c99d6eaef1ac..4fc0165ed3f5 100644
> >>> --- a/include/drm/bridge/analogix_dp.h
> >>> +++ b/include/drm/bridge/analogix_dp.h
> >>> @@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
> >>>  			 struct drm_connector *);
> >>>  };
> >>>  
> >>> -int analogix_dp_psr_supported(struct device *dev);
> >>> +int analogix_dp_psr_enabled(struct device *dev);
> >>>  int analogix_dp_enable_psr(struct device *dev);
> >>>  int analogix_dp_disable_psr(struct device *dev);
> >>>  
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 10/41] drm/bridge: analogix_dp: Don't change psr while bridge is disabled

[Andrzej Hajda]

On 22.03.2017 16:19, Sean Paul wrote:
> On Wed, Mar 22, 2017 at 09:36:34AM +0100, Andrzej Hajda wrote:
>> On 21.03.2017 20:58, Sean Paul wrote:
>>> On Thu, Mar 16, 2017 at 02:40:21PM +0100, Andrzej Hajda wrote:
>>>> On 10.03.2017 05:32, Sean Paul wrote:
>>>>> From: zain wang <wzz@rock-chips.com>
>>>>>
>>>>> There is a race between AUX CH bring-up and enabling bridge which will
>>>>> cause link training to fail. To avoid hitting it, don't change psr state
>>>>> while enabling the bridge.
>>>>>
>>>>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>>> Signed-off-by: zain wang <wzz@rock-chips.com>
>>>>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>>>>> [seanpaul fixed up the commit message a bit and renamed *_supported to *_enabled]
>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>>> Hmm, beside resetting psr_enable in analogix_dp_bridge_disable I do not
>>>> see functional change, or am I blind?
>>> The patch also adds a check of psr_enable in analogix_dp_commit. This will avoid
>>> trying to enable psr when the bridge is disabled (that's why psr_enable is
>>> reset).
>> Copy/paste from analogix_dp_commit chunk below:
>>
>> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
>> -	if (dp->psr_support)
>> +	dp->psr_enable = analogix_dp_detect_sink_psr(dp);
>> +	if (dp->psr_enable)
>>  		analogix_dp_enable_sink_psr(dp);
>>
>>
>> What is added here?
> I guess this is what I get for skimming, apologies. I'll hopefully do a better
> job explaining below.
>
>
>> Regards
>> Andrzej
>>
>>> Sean
>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 15 ++++++++-------
>>>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 +-
>>>>>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  2 +-
>>>>>  include/drm/bridge/analogix_dp.h                   |  2 +-
>>>>>  4 files changed, 11 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>>> index 8a8f05fe9da3..64d94a34874d 100644
>>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>>> @@ -98,20 +98,20 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> -int analogix_dp_psr_supported(struct device *dev)
>>>>> +int analogix_dp_psr_enabled(struct device *dev)
>>>>>  {
>>>>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>>>  
>>>>> -	return dp->psr_support;
>>>>> +	return dp->psr_enable;
>>>>>  }
>>>>> -EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
>>>>> +EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
>>>>>  
>>>>>  int analogix_dp_enable_psr(struct device *dev)
>>>>>  {
>>>>>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>>>  	struct edp_vsc_psr psr_vsc;
>>>>>  
>>>>> -	if (!dp->psr_support)
>>>>> +	if (!dp->psr_enable)
> This gates enabling psr on the panel. Previously it was put in place to avoid
> trying to enable psr on devices which did not support it.
>
>>>>>  		return 0;
>>>>>  
>>>>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>>>> @@ -134,7 +134,7 @@ int analogix_dp_disable_psr(struct device *dev)
>>>>>  	struct edp_vsc_psr psr_vsc;
>>>>>  	int ret;
>>>>>  
>>>>> -	if (!dp->psr_support)
>>>>> +	if (!dp->psr_enable)
>>>>>  		return 0;
>>>>>  
>>>>>  	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>>>> @@ -878,8 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
>>>>>  	/* Enable video */
>>>>>  	analogix_dp_start_video(dp);
>>>>>  
>>>>> -	dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>>>> -	if (dp->psr_support)
>>>>> +	dp->psr_enable = analogix_dp_detect_si1]nk_psr(dp);
>>>>> +	if (dp->psr_enable)
> This probes the hardware to see if psr is supported. If it is, psr_enable will
> transition to true. Before this patch, psr_support/psr_enable would be flipped
> true on the first modeset and stay that way forever, even when the panel was
> turned off.

But here and above we have still only renaming, no functional change.


>
>>>>>  		analogix_dp_enable_sink_psr(dp);
>>>>>  }
>>>>>  
>>>>> @@ -1120,6 +1120,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>>>>  	if (ret)
>>>>>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>>>>>  
>>>>> +	dp->psr_enable = false;
> By setting psr_enable to false here, the driver ensures that any subsequent calls
> to analogix_dp_enable_psr() will return early and will not attempt to change
> hardware (that would fail). Once the panel is re-enabled, psr_enable will return
> to true if the hardware supports it.
>
> This is why the variable was renamed from psr_support to psr_enable. It no
> longer just tracks whether psr is supported, but rather that it is supported and
> eligible to be enabled.

OK so the patch does actually two things:
- renaming psr_support* -> psr_enable*
- reset psr_enable on bridge disable.

Renaming obscures the patch and hides functional change, so please say
about real change in description.
And regarding description, it does not seem adequate to the patch, do we
really have race here? Or as subject says we prevent touching psr on
disabled bridge.

Regards
Andrzej



>
> Sean
>
>
>>>>>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>>>>>  }
>>>>>  
>>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>>> index b039b28d8fcc..e135a42cb19e 100644
>>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>>> @@ -170,7 +170,7 @@ struct analogix_dp_device {
>>>>>  	int			dpms_mode;
>>>>>  	int			hpd_gpio;
>>>>>  	bool                    force_hpd;
>>>>> -	bool			psr_support;
>>>>> +	bool			psr_enable;
>>>>>  
>>>>>  	struct mutex		panel_lock;
>>>>>  	bool			panel_is_modeset;
>>>>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>>>> index 64e7e2c0bc58..f44756029478 100644
>>>>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>>>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>>>> @@ -83,7 +83,7 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
>>>>>  	int vact_end;
>>>>>  	int ret;
>>>>>  
>>>>> -	if (!analogix_dp_psr_supported(dp->dev))
>>>>> +	if (!analogix_dp_psr_enabled(dp->dev))
>>>>>  		return;
>>>>>  
>>>>>  	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
>>>>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>>>>> index c99d6eaef1ac..4fc0165ed3f5 100644
>>>>> --- a/include/drm/bridge/analogix_dp.h
>>>>> +++ b/include/drm/bridge/analogix_dp.h
>>>>> @@ -38,7 +38,7 @@ struct analogix_dp_plat_data {
>>>>>  			 struct drm_connector *);
>>>>>  };
>>>>>  
>>>>> -int analogix_dp_psr_supported(struct device *dev);
>>>>> +int analogix_dp_psr_enabled(struct device *dev);
>>>>>  int analogix_dp_enable_psr(struct device *dev);
>>>>>  int analogix_dp_disable_psr(struct device *dev);
>>>>>  

[PATCH 11/41] drm/rockchip: add mutex vop lock

[Sean Paul]

From: zain wang <wzz@rock-chips.com>

Add a lock to vop to avoid disabling the crtc while waiting for a line
flag while enabling psr. If we disable in the middle of waiting for the
line flag, we'll end up timing out or worse.

Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 30 +++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 1c1e8535ad28..aa5c528c59fc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -130,6 +130,7 @@ struct vop {
 	spinlock_t reg_lock;
 	/* lock vop irq reg */
 	spinlock_t irq_lock;
+	struct mutex vop_lock;
 
 	unsigned int irq;
 
@@ -568,6 +569,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 
 	WARN_ON(vop->event);
 
+	mutex_lock(&vop->vop_lock);
 	/*
 	 * We need to make sure that all windows are disabled before we
 	 * disable that crtc. Otherwise we might try to scan from a destroyed
@@ -619,6 +621,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 	clk_disable(vop->aclk);
 	clk_disable(vop->hclk);
 	pm_runtime_put(vop->dev);
+	mutex_unlock(&vop->vop_lock);
 
 	if (crtc->state->event && !crtc->state->active) {
 		spin_lock_irq(&crtc->dev->event_lock);
@@ -885,10 +888,13 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	uint32_t pin_pol, val;
 	int ret;
 
+	mutex_lock(&vop->vop_lock);
+
 	WARN_ON(vop->event);
 
 	ret = vop_enable(crtc);
 	if (ret) {
+		mutex_unlock(&vop->vop_lock);
 		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
 		return;
 	}
@@ -979,6 +985,7 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
 
 	VOP_CTRL_SET(vop, standby, 0);
+	mutex_unlock(&vop->vop_lock);
 }
 
 static bool vop_fs_irq_is_pending(struct vop *vop)
@@ -1506,15 +1513,22 @@ int rockchip_drm_wait_line_flag(struct drm_crtc *crtc, unsigned int line_num,
 {
 	struct vop *vop = to_vop(crtc);
 	unsigned long jiffies_left;
+	int ret = 0;
 
 	if (!crtc || !vop->is_enabled)
 		return -ENODEV;
 
-	if (line_num > crtc->mode.vtotal || mstimeout <= 0)
-		return -EINVAL;
+	mutex_lock(&vop->vop_lock);
+
+	if (line_num > crtc->mode.vtotal || mstimeout <= 0) {
+		ret = -EINVAL;
+		goto out;
+	}
 
-	if (vop_line_flag_irq_is_enabled(vop))
-		return -EBUSY;
+	if (vop_line_flag_irq_is_enabled(vop)) {
+		ret = -EBUSY;
+		goto out;
+	}
 
 	reinit_completion(&vop->line_flag_completion);
 	vop_line_flag_irq_enable(vop, line_num);
@@ -1525,10 +1539,13 @@ int rockchip_drm_wait_line_flag(struct drm_crtc *crtc, unsigned int line_num,
 
 	if (jiffies_left == 0) {
 		dev_err(vop->dev, "Timeout waiting for IRQ\n");
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto out;
 	}
 
-	return 0;
+out:
+	mutex_unlock(&vop->vop_lock);
+	return ret;
 }
 EXPORT_SYMBOL(rockchip_drm_wait_line_flag);
 
@@ -1584,6 +1601,7 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	spin_lock_init(&vop->reg_lock);
 	spin_lock_init(&vop->irq_lock);
+	mutex_init(&vop->vop_lock);
 
 	ret = devm_request_irq(dev, vop->irq, vop_isr,
 			       IRQF_SHARED, dev_name(dev), vop);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 13/41] drm/rockchip: pre dither down when output bpc is 8bit

[Sean Paul]

From: Mark Yao <mark.yao@rock-chips.com>

Some encoder have a crc verification check, crc check fail if
input and output data is not equal.

That means encoder input and output need use same color depth,
vop can output 10bit data to encoder, but some panel only support
8bit depth, that would make crc check die.

So pre dither down vop data to 8bit if panel's bpc is 8.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 4 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     | 1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     | 4 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h     | 1 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c     | 6 ++++--
 5 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index f44756029478..b6c6d6d09d7e 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -225,6 +225,7 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
 	struct rockchip_dp_device *dp = to_dp(encoder);
+	struct drm_display_info *di = &conn_state->connector->display_info;
 	int ret;
 
 	/*
@@ -248,6 +249,9 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
 			s->output_mode = ROCKCHIP_OUT_MODE_P888;
 	}
 
+	s->output_bpc = di->bpc;
+	s->output_type = DRM_MODE_CONNECTOR_eDP;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 1844951118da..a966d1d37378 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -36,6 +36,7 @@ struct rockchip_crtc_state {
 	struct drm_crtc_state base;
 	int output_type;
 	int output_mode;
+	int output_bpc;
 };
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index aa5c528c59fc..ea3fff4170d9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -968,6 +968,10 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 		DRM_DEV_ERROR(vop->dev, "unsupported connector_type [%d]\n",
 			      s->output_type);
 	}
+	if (s->output_mode == ROCKCHIP_OUT_MODE_AAAA && s->output_bpc == 8)
+		VOP_CTRL_SET(vop, pre_dither_down, 1);
+	else
+		VOP_CTRL_SET(vop, pre_dither_down, 0);
 	VOP_CTRL_SET(vop, out_mode, s->output_mode);
 
 	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 5a4faa85dbd2..9a1eb83271eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -47,6 +47,7 @@ struct vop_ctrl {
 	struct vop_reg mipi_en;
 	struct vop_reg dp_en;
 	struct vop_reg out_mode;
+	struct vop_reg pre_dither_down;
 	struct vop_reg dither_down;
 	struct vop_reg dither_up;
 	struct vop_reg pin_pol;
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 91fbc7b52147..e645a9621721 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -212,7 +212,8 @@ static const struct vop_ctrl rk3288_ctrl_data = {
 	.hdmi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 13),
 	.edp_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 14),
 	.mipi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 15),
-	.dither_down = VOP_REG(RK3288_DSP_CTRL1, 0xf, 1),
+	.pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1),
+	.dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x7, 2),
 	.dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6),
 	.data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19),
 	.out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0),
@@ -289,7 +290,8 @@ static const struct vop_ctrl rk3399_ctrl_data = {
 	.hdmi_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 13),
 	.edp_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 14),
 	.mipi_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 15),
-	.dither_down = VOP_REG(RK3399_DSP_CTRL1, 0xf, 1),
+	.pre_dither_down = VOP_REG(RK3399_DSP_CTRL1, 0x1, 1),
+	.dither_down = VOP_REG(RK3399_DSP_CTRL1, 0x7, 2),
 	.dither_up = VOP_REG(RK3399_DSP_CTRL1, 0x1, 6),
 	.data_blank = VOP_REG(RK3399_DSP_CTRL0, 0x1, 19),
 	.out_mode = VOP_REG(RK3399_DSP_CTRL0, 0xf, 0),
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 27/41] drm/rockchip: Restore psr->state when enable/disable psr failed

[Sean Paul]

From: zain wang <wzz@rock-chips.com>

If we failed disable psr, it would hang the display until next psr
cycle coming. So we should restore psr->state when it failed.

Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +++-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 12 ++++++------
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c        | 20 +++++++++++++-------
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h        |  2 +-
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index dcf39ea0d14b..35ea04ff4078 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -156,8 +156,10 @@ int analogix_dp_disable_psr(struct device *dev)
 	psr_vsc.DB1 = 0;
 
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-	if (ret != 1)
+	if (ret != 1) {
 		dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
+		return ret;
+	}
 
 	return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
 }
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index b6c6d6d09d7e..0614d32c5435 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -76,7 +76,7 @@ struct rockchip_dp_device {
 	struct analogix_dp_plat_data plat_data;
 };
 
-static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
+static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 {
 	struct rockchip_dp_device *dp = to_dp(encoder);
 	struct drm_crtc *crtc = dp->encoder.crtc;
@@ -84,12 +84,12 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 	int ret;
 
 	if (!analogix_dp_psr_enabled(dp->dev))
-		return;
+		return 0;
 
 	dev_dbg(dp->dev, "%s PSR...\n", enabled ? "enable" : "disable");
 
 	if (!crtc)
-		return;
+		return -EINVAL;
 
 	vact_end = crtc->mode.vtotal - crtc->mode.vsync_start + crtc->mode.vdisplay;
 
@@ -97,13 +97,13 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 					  PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
 	if (ret) {
 		dev_err(dp->dev, "line flag interrupt did not arrive\n");
-		return;
+		return -ETIMEDOUT;
 	}
 
 	if (enabled)
-		analogix_dp_enable_psr(dp->dev);
+		return analogix_dp_enable_psr(dp->dev);
 	else
-		analogix_dp_disable_psr(dp->dev);
+		return analogix_dp_disable_psr(dp->dev);
 }
 
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index b339ca943139..9376f4396b6b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -36,7 +36,7 @@ struct psr_drv {
 
 	struct delayed_work	flush_work;
 
-	void (*set)(struct drm_encoder *encoder, bool enable);
+	int (*set)(struct drm_encoder *encoder, bool enable);
 };
 
 static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
@@ -93,19 +93,25 @@ static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state)
 		return;
 	}
 
-	psr->state = state;
-
 	/* Actually commit the state change to hardware */
-	switch (psr->state) {
+	switch (state) {
 	case PSR_ENABLE:
-		psr->set(psr->encoder, true);
+		if (psr->set(psr->encoder, true))
+			return;
 		break;
 
 	case PSR_DISABLE:
 	case PSR_FLUSH:
-		psr->set(psr->encoder, false);
+		if (psr->set(psr->encoder, false))
+			return;
 		break;
+
+	default:
+		pr_err("%s: Unknown state %d\n", __func__, state);
+		return;
 	}
+
+	psr->state = state;
 }
 
 static void psr_set_state(struct psr_drv *psr, enum psr_state state)
@@ -229,7 +235,7 @@ EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
  * Zero on success, negative errno on failure.
  */
 int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			void (*psr_set)(struct drm_encoder *, bool enable))
+			int (*psr_set)(struct drm_encoder *, bool enable))
 {
 	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
 	struct psr_drv *psr;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
index b1ea0155e57c..06537ee27565 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
@@ -22,7 +22,7 @@ int rockchip_drm_psr_activate(struct drm_encoder *encoder);
 int rockchip_drm_psr_deactivate(struct drm_encoder *encoder);
 
 int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			void (*psr_set)(struct drm_encoder *, bool enable));
+			int (*psr_set)(struct drm_encoder *, bool enable));
 void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
 
 #endif /* __ROCKCHIP_DRM_PSR__ */
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 32/41] drm/rockchip: Flush PSR before committing modeset disables/enables

[Sean Paul]

From: Tomasz Figa <tfiga@chromium.org>

Currently PSR flush is triggered from CRTC's .atomic_begin() callback,
which is executed after modeset disables and enables and before plane
updates are committed. Since PSR flush and re-enable can be triggered
asynchronously by external sources (input event, delayed work), it can
race with hardware programming done in the aforementioned stages.

To avoid the race, we can trigger PSR flush before committing modeset
disables/enables. This also has the advantage of removing some
PSR-specific knowledge from the VOP driver.

Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 13 +++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  7 -------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 81f9548672b0..2067f4afadf3 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -175,10 +175,23 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
 }
 
 static void
+rockchip_drm_psr_flush_state(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		rockchip_drm_psr_flush(crtc);
+}
+
+static void
 rockchip_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 
+	rockchip_drm_psr_flush_state(state);
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ea3fff4170d9..879efc3a6c67 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1067,18 +1067,11 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 	}
 }
 
-static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
-				  struct drm_crtc_state *old_crtc_state)
-{
-	rockchip_drm_psr_flush(crtc);
-}
-
 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
 	.enable = vop_crtc_enable,
 	.disable = vop_crtc_disable,
 	.mode_fixup = vop_crtc_mode_fixup,
 	.atomic_flush = vop_crtc_atomic_flush,
-	.atomic_begin = vop_crtc_atomic_begin,
 };
 
 static void vop_crtc_destroy(struct drm_crtc *crtc)
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 33/41] drm/rockchip: Disable VOP windows when PSR is active

[Sean Paul]

From: zain wang <wzz@rock-chips.com>

We do not have to drive the display when it is in PSR mode, so we can
save some power by disabling active VOP windows, until a next PSR flush
turns them back on.

Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Signed-off-by: zain wang <wzz@rock-chips.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 34 +++++++++++++++++--------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |  1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     | 31 ++++++++++++++++++++++
 3 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 0614d32c5435..de23cc6fd05d 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -91,19 +91,31 @@ static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
 	if (!crtc)
 		return -EINVAL;
 
-	vact_end = crtc->mode.vtotal - crtc->mode.vsync_start + crtc->mode.vdisplay;
+	if (enabled) {
+		vact_end = crtc->mode.vtotal - crtc->mode.vsync_start +
+			crtc->mode.vdisplay;
+		ret = rockchip_drm_wait_line_flag(dp->encoder.crtc, vact_end,
+						PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
+		if (ret) {
+			dev_err(dp->dev, "line flag interrupt did not arrive\n");
+			return -ETIMEDOUT;
+		}
 
-	ret = rockchip_drm_wait_line_flag(dp->encoder.crtc, vact_end,
-					  PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
-	if (ret) {
-		dev_err(dp->dev, "line flag interrupt did not arrive\n");
-		return -ETIMEDOUT;
+		ret = analogix_dp_enable_psr(dp->dev);
+		if (ret) {
+			dev_err(dp->dev, "failed to enable psr %d\n", ret);
+			return ret;
+		}
+		rockchip_drm_set_win_enabled(crtc, false);
+	} else {
+		rockchip_drm_set_win_enabled(crtc, true);
+		ret = analogix_dp_disable_psr(dp->dev);
+		if (ret) {
+			dev_err(dp->dev, "failed to disable psr %d\n", ret);
+			return ret;
+		}
 	}
-
-	if (enabled)
-		return analogix_dp_enable_psr(dp->dev);
-	else
-		return analogix_dp_disable_psr(dp->dev);
+	return 0;
 }
 
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index a966d1d37378..be4ad9b670cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -65,5 +65,6 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 				    struct device *dev);
 int rockchip_drm_wait_line_flag(struct drm_crtc *crtc, unsigned int line_num,
 				unsigned int mstimeout);
+void rockchip_drm_set_win_enabled(struct drm_crtc *ctrc, bool enabled);
 
 #endif /* _ROCKCHIP_DRM_DRV_H_ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 879efc3a6c67..e90a32ee5a36 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -89,6 +89,9 @@
 #define VOP_WIN_GET_YRGBADDR(vop, win) \
 		vop_readl(vop, win->base + win->phy->yrgb_mst.offset)
 
+#define VOP_WIN_TO_INDEX(vop_win) \
+	((vop_win) - (vop_win)->vop->win)
+
 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
 
@@ -108,6 +111,8 @@ struct vop {
 	struct drm_device *drm_dev;
 	bool is_enabled;
 
+	unsigned int win_enabled;
+
 	struct completion dsp_hold_completion;
 
 	/* protected by dev->event_lock */
@@ -562,6 +567,27 @@ static int vop_enable(struct drm_crtc *crtc)
 	return ret;
 }
 
+void rockchip_drm_set_win_enabled(struct drm_crtc *crtc, bool enabled)
+{
+	struct vop *vop = to_vop(crtc);
+	int i;
+
+	spin_lock(&vop->reg_lock);
+
+	for (i = 0; i < vop->data->win_size; i++) {
+		struct vop_win *vop_win = &vop->win[i];
+		const struct vop_win_data *win = vop_win->data;
+
+		VOP_WIN_SET(vop, win, enable,
+			    enabled && (vop->win_enabled & BIT(i)));
+	}
+
+	vop_cfg_done(vop);
+
+	spin_unlock(&vop->reg_lock);
+}
+EXPORT_SYMBOL(rockchip_drm_set_win_enabled);
+
 static void vop_crtc_disable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -581,6 +607,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 
 		spin_lock(&vop->reg_lock);
 		VOP_WIN_SET(vop, win, enable, 0);
+		vop->win_enabled &= ~BIT(i);
 		spin_unlock(&vop->reg_lock);
 	}
 
@@ -700,6 +727,7 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
 	spin_lock(&vop->reg_lock);
 
 	VOP_WIN_SET(vop, win, enable, 0);
+	vop->win_enabled &= ~BIT(VOP_WIN_TO_INDEX(vop_win));
 
 	spin_unlock(&vop->reg_lock);
 }
@@ -807,6 +835,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	}
 
 	VOP_WIN_SET(vop, win, enable, 1);
+	vop->win_enabled |= BIT(VOP_WIN_TO_INDEX(vop_win));
+
 	spin_unlock(&vop->reg_lock);
 }
 
@@ -1443,6 +1473,7 @@ static int vop_initial(struct vop *vop)
 		const struct vop_win_data *win = &vop_data->win[i];
 
 		VOP_WIN_SET(vop, win, enable, 0);
+		vop->win_enabled &= ~BIT(i);
 	}
 
 	vop_cfg_done(vop);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 35/41] drm/rockchip: analogix_dp: Fix invalid implementation of unbind

[Sean Paul]

From: Tomasz Figa <tfiga@chromium.org>

Current implementation of unbind dereferences the drvdata pointer
assuming that it's its own data, however the ownership belongs to the
analogix code, which means that the pointer is dereferenced with wrong
type. Fix this by using the recently added platform data .cleanup()
callback to do Rockchip-specific things at unbind.

Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index de23cc6fd05d..0a92d9e3ffac 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -179,6 +179,13 @@ static int rockchip_dp_get_modes(struct analogix_dp_plat_data *plat_data,
 	return 0;
 }
 
+static void rockchip_dp_cleanup(struct analogix_dp_plat_data *plat_data)
+{
+	struct rockchip_dp_device *dp = to_dp(plat_data);
+
+	rockchip_drm_psr_unregister(&dp->encoder);
+}
+
 static bool
 rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 				   const struct drm_display_mode *mode,
@@ -395,25 +402,16 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.power_on = rockchip_dp_poweron;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
+	dp->plat_data.cleanup = rockchip_dp_cleanup;
 
 	rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
 
 	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 }
 
-static void rockchip_dp_unbind(struct device *dev, struct device *master,
-			       void *data)
-{
-	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
-
-	rockchip_drm_psr_unregister(&dp->encoder);
-
-	return analogix_dp_unbind(dev, master, data);
-}
-
 static const struct component_ops rockchip_dp_component_ops = {
 	.bind = rockchip_dp_bind,
-	.unbind = rockchip_dp_unbind,
+	.unbind = analogix_dp_unbind,
 };
 
 static int rockchip_dp_probe(struct platform_device *pdev)
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 37/41] drm/rockchip: analogix_dp: Wire the shutdown callback to disable PSR

[Sean Paul]

From: Tomasz Figa <tfiga@chromium.org>

We have to disable PSR before shutdown to avoid any asynchronous PSR
code to interfere with Rockchip DRM device shutdown. We use the recently
added analogix_dp_shutdown() function and rockchip_dp_cleanup() we
already use for unregistering PSR at unbind time.

Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 0a92d9e3ffac..14f68e792e6f 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -468,6 +468,11 @@ static int rockchip_dp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void rockchip_dp_shutdown(struct platform_device *pdev)
+{
+	analogix_dp_shutdown(&pdev->dev);
+}
+
 static const struct dev_pm_ops rockchip_dp_pm_ops = {
 #ifdef CONFIG_PM_SLEEP
 	.suspend = analogix_dp_suspend,
@@ -499,6 +504,7 @@ MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
 static struct platform_driver rockchip_dp_driver = {
 	.probe = rockchip_dp_probe,
 	.remove = rockchip_dp_remove,
+	.shutdown = rockchip_dp_shutdown,
 	.driver = {
 		   .name = "rockchip-dp",
 		   .pm = &rockchip_dp_pm_ops,
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 39/41] drm/bridge: analogix_dp: Split the platform-specific poweron in two parts

[Sean Paul]

From: Douglas Anderson <dianders@chromium.org>

Some of the platform-specific stuff in rockchip_dp_poweron() needs to
happen before the generic code.  Some needs to happen after.  Let's
split the callback in two.

Specifically we can't start doing PSR work until _after_ the whole
controller is up, so don't set the enable until the end.

Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
[seanpaul added exynos change]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  7 +++++--
 drivers/gpu/drm/exynos/exynos_dp.c                 |  2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 12 ++++++++++--
 include/drm/bridge/analogix_dp.h                   |  3 ++-
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index a05bc0271188..bc9a0a28ab1d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1254,8 +1254,8 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 		goto out_dp_clk_pre;
 	}
 
-	if (dp->plat_data->power_on)
-		dp->plat_data->power_on(dp->plat_data);
+	if (dp->plat_data->power_on_start)
+		dp->plat_data->power_on_start(dp->plat_data);
 
 	phy_power_on(dp->phy);
 
@@ -1278,6 +1278,9 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 		goto out_dp_init;
 	}
 
+	if (dp->plat_data->power_on_end)
+		dp->plat_data->power_on_end(dp->plat_data);
+
 	enable_irq(dp->irq);
 	return 0;
 
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index 1ef0be338b85..6b79c3b3802f 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -168,7 +168,7 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->drm_dev = drm_dev;
 
 	dp->plat_data.dev_type = EXYNOS_DP;
-	dp->plat_data.power_on = exynos_dp_poweron;
+	dp->plat_data.power_on_start = exynos_dp_poweron;
 	dp->plat_data.power_off = exynos_dp_poweroff;
 	dp->plat_data.attach = exynos_dp_bridge_attach;
 	dp->plat_data.get_modes = exynos_dp_get_modes;
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 14f68e792e6f..07dd09e1e32f 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -127,7 +127,7 @@ static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
 	return 0;
 }
 
-static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
+static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = to_dp(plat_data);
 	int ret;
@@ -145,6 +145,13 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
 		return ret;
 	}
 
+	return ret;
+}
+
+static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
+{
+	struct rockchip_dp_device *dp = to_dp(plat_data);
+
 	return rockchip_drm_psr_activate(&dp->encoder);
 }
 
@@ -399,7 +406,8 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.encoder = &dp->encoder;
 
 	dp->plat_data.dev_type = dp->data->chip_type;
-	dp->plat_data.power_on = rockchip_dp_poweron;
+	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
+	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 	dp->plat_data.cleanup = rockchip_dp_cleanup;
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index c079ca1ffc76..1049ee56ae76 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -30,7 +30,8 @@ struct analogix_dp_plat_data {
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 
-	int (*power_on)(struct analogix_dp_plat_data *);
+	int (*power_on_start)(struct analogix_dp_plat_data *);
+	int (*power_on_end)(struct analogix_dp_plat_data *);
 	int (*power_off)(struct analogix_dp_plat_data *);
 	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
 		      struct drm_connector *);
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 39/41] drm/bridge: analogix_dp: Split the platform-specific poweron in two parts

[Andrzej Hajda]

On 10.03.2017 05:32, Sean Paul wrote:
> From: Douglas Anderson <dianders@chromium.org>
>
> Some of the platform-specific stuff in rockchip_dp_poweron() needs to
> happen before the generic code.  Some needs to happen after.  Let's
> split the callback in two.
>
> Specifically we can't start doing PSR work until _after_ the whole
> controller is up, so don't set the enable until the end.
>
> Cc: Kristian H. Kristensen <hoegsberg@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> [seanpaul added exynos change]
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Sounds like mid-layer issue. I do not know bridge details so I can only
guess but probably passing full control to drivers and call generic code
via helpers could be better solution.
Anyway it can be done later.

For now:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>


Regards
Andrzej

> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  7 +++++--
>  drivers/gpu/drm/exynos/exynos_dp.c                 |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 12 ++++++++++--
>  include/drm/bridge/analogix_dp.h                   |  3 ++-
>  4 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index a05bc0271188..bc9a0a28ab1d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1254,8 +1254,8 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>  		goto out_dp_clk_pre;
>  	}
>  
> -	if (dp->plat_data->power_on)
> -		dp->plat_data->power_on(dp->plat_data);
> +	if (dp->plat_data->power_on_start)
> +		dp->plat_data->power_on_start(dp->plat_data);
>  
>  	phy_power_on(dp->phy);
>  
> @@ -1278,6 +1278,9 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>  		goto out_dp_init;
>  	}
>  
> +	if (dp->plat_data->power_on_end)
> +		dp->plat_data->power_on_end(dp->plat_data);
> +
>  	enable_irq(dp->irq);
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 1ef0be338b85..6b79c3b3802f 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -168,7 +168,7 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>  	dp->drm_dev = drm_dev;
>  
>  	dp->plat_data.dev_type = EXYNOS_DP;
> -	dp->plat_data.power_on = exynos_dp_poweron;
> +	dp->plat_data.power_on_start = exynos_dp_poweron;
>  	dp->plat_data.power_off = exynos_dp_poweroff;
>  	dp->plat_data.attach = exynos_dp_bridge_attach;
>  	dp->plat_data.get_modes = exynos_dp_get_modes;
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 14f68e792e6f..07dd09e1e32f 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -127,7 +127,7 @@ static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
>  	return 0;
>  }
>  
> -static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
> +static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
>  {
>  	struct rockchip_dp_device *dp = to_dp(plat_data);
>  	int ret;
> @@ -145,6 +145,13 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
>  		return ret;
>  	}
>  
> +	return ret;
> +}
> +
> +static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
> +{
> +	struct rockchip_dp_device *dp = to_dp(plat_data);
> +
>  	return rockchip_drm_psr_activate(&dp->encoder);
>  }
>  
> @@ -399,7 +406,8 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  	dp->plat_data.encoder = &dp->encoder;
>  
>  	dp->plat_data.dev_type = dp->data->chip_type;
> -	dp->plat_data.power_on = rockchip_dp_poweron;
> +	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
> +	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
>  	dp->plat_data.power_off = rockchip_dp_powerdown;
>  	dp->plat_data.get_modes = rockchip_dp_get_modes;
>  	dp->plat_data.cleanup = rockchip_dp_cleanup;
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index c079ca1ffc76..1049ee56ae76 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -30,7 +30,8 @@ struct analogix_dp_plat_data {
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  
> -	int (*power_on)(struct analogix_dp_plat_data *);
> +	int (*power_on_start)(struct analogix_dp_plat_data *);
> +	int (*power_on_end)(struct analogix_dp_plat_data *);
>  	int (*power_off)(struct analogix_dp_plat_data *);
>  	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>  		      struct drm_connector *);