drm dpms have many power modes, ON,OFF,SUSPEND,STANDBY, etc. but vop only have enable/disable mode, maybe case such bug: --> DRM_DPMS_ON: power on vop --> DRM_DPMS_SUSPEND: power off vop --> DRM_DPMS_OFF: already power off at SUSPEND, crash so use a bool val is more suitable.
another problem at vop_crtc_dpms: vop_enable()->drm_vblank_on, drm_vblank_on may call vop enable vblank. if it happen, vblank enable would failed, then cause irq status error. because is_enabled value is set after drm_vblank_on.
Mark Yao (2): drm/rockchip: vop use is_enabled instead of dpms mode drm/rockchip: vop: set vop enabled after enable iommu
drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 2 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 17 +++++------------ drivers/gpu/drm/rockchip/rockchip_drm_gem.h | 3 +-- 3 files changed, 7 insertions(+), 15 deletions(-)
drm dpms have many power modes: ON,OFF,SUSPEND,STANDBY, etc. but vop only have enable/disable mode, maybe case such bug: --> DRM_DPMS_ON: power on vop --> DRM_DPMS_SUSPEND: power off vop --> DRM_DPMS_OFF: already power off at SUSPEND, crash so use a bool val is more suitable.
Signed-off-by: Mark Yao mark.yao@rock-chips.com --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 ++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 9a5c571..f278c09 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -81,7 +81,7 @@ struct vop { struct drm_crtc crtc; struct device *dev; struct drm_device *drm_dev; - unsigned int dpms; + bool is_enabled;
int connector_type; int connector_out_mode; @@ -387,6 +387,9 @@ static void vop_enable(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); int ret;
+ if (vop->is_enabled) + return; + ret = clk_enable(vop->hclk); if (ret < 0) { dev_err(vop->dev, "failed to enable hclk - %d\n", ret); @@ -427,6 +430,8 @@ static void vop_enable(struct drm_crtc *crtc)
drm_vblank_on(vop->drm_dev, vop->pipe);
+ vop->is_enabled = false; + return;
err_disable_aclk: @@ -441,6 +446,9 @@ static void vop_disable(struct drm_crtc *crtc) { struct vop *vop = to_vop(crtc);
+ if (vop->is_enabled) + return; + drm_vblank_off(crtc->dev, vop->pipe);
disable_irq(vop->irq); @@ -463,6 +471,8 @@ static void vop_disable(struct drm_crtc *crtc)
clk_disable(vop->aclk); clk_disable(vop->hclk); + + vop->is_enabled = false; }
/* @@ -742,7 +752,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON) + if (!vop->is_enabled) return -EPERM;
spin_lock_irqsave(&vop->irq_lock, flags); @@ -759,8 +769,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON) + if (!vop->is_enabled) return; + spin_lock_irqsave(&vop->irq_lock, flags); vop_mask_write(vop, INTR_CTRL0, FS_INTR_MASK, FS_INTR_EN(0)); spin_unlock_irqrestore(&vop->irq_lock, flags); @@ -773,15 +784,8 @@ static const struct rockchip_crtc_funcs private_crtc_funcs = {
static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) { - struct vop *vop = to_vop(crtc); - DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
- if (vop->dpms == mode) { - DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n"); - return; - } - switch (mode) { case DRM_MODE_DPMS_ON: vop_enable(crtc); @@ -795,8 +799,6 @@ static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) DRM_DEBUG_KMS("unspecified mode %d\n", mode); break; } - - vop->dpms = mode; }
static void vop_crtc_prepare(struct drm_crtc *crtc) @@ -934,9 +936,9 @@ static int vop_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *old_fb = crtc->primary->fb; int ret;
- /* when the page flip is requested, crtc's dpms should be on */ - if (vop->dpms > DRM_MODE_DPMS_ON) { - DRM_DEBUG("failed page flip request at dpms[%d].\n", vop->dpms); + /* when the page flip is requested, crtc should be on */ + if (!vop->is_enabled) { + DRM_DEBUG("page flip request rejected because crtc is off.\n"); return 0; }
@@ -1302,7 +1304,7 @@ static int vop_initial(struct vop *vop)
clk_disable(vop->hclk);
- vop->dpms = DRM_MODE_DPMS_OFF; + vop->is_enabled = false;
return 0;
On Thu, Jan 22, 2015 at 03:05:32PM +0800, Mark Yao wrote:
drm dpms have many power modes: ON,OFF,SUSPEND,STANDBY, etc. but vop only have enable/disable mode, maybe case such bug: --> DRM_DPMS_ON: power on vop --> DRM_DPMS_SUSPEND: power off vop --> DRM_DPMS_OFF: already power off at SUSPEND, crash so use a bool val is more suitable.
Signed-off-by: Mark Yao mark.yao@rock-chips.com
Long term I highly suggest you switch to atomic, since with atomic all the legacy dpms modes are remapped to a simple on/off. Also the new atomic helpers make sure that your backend isn't called multiple times, so you can ditch all your is_enabled tracking with that. -Daniel
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 ++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 9a5c571..f278c09 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -81,7 +81,7 @@ struct vop { struct drm_crtc crtc; struct device *dev; struct drm_device *drm_dev;
- unsigned int dpms;
bool is_enabled;
int connector_type; int connector_out_mode;
@@ -387,6 +387,9 @@ static void vop_enable(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); int ret;
- if (vop->is_enabled)
return;
- ret = clk_enable(vop->hclk); if (ret < 0) { dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
@@ -427,6 +430,8 @@ static void vop_enable(struct drm_crtc *crtc)
drm_vblank_on(vop->drm_dev, vop->pipe);
- vop->is_enabled = false;
- return;
err_disable_aclk: @@ -441,6 +446,9 @@ static void vop_disable(struct drm_crtc *crtc) { struct vop *vop = to_vop(crtc);
if (vop->is_enabled)
return;
drm_vblank_off(crtc->dev, vop->pipe);
disable_irq(vop->irq);
@@ -463,6 +471,8 @@ static void vop_disable(struct drm_crtc *crtc)
clk_disable(vop->aclk); clk_disable(vop->hclk);
- vop->is_enabled = false;
}
/* @@ -742,7 +752,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON)
if (!vop->is_enabled) return -EPERM;
spin_lock_irqsave(&vop->irq_lock, flags);
@@ -759,8 +769,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON)
- if (!vop->is_enabled) return;
- spin_lock_irqsave(&vop->irq_lock, flags); vop_mask_write(vop, INTR_CTRL0, FS_INTR_MASK, FS_INTR_EN(0)); spin_unlock_irqrestore(&vop->irq_lock, flags);
@@ -773,15 +784,8 @@ static const struct rockchip_crtc_funcs private_crtc_funcs = {
static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) {
struct vop *vop = to_vop(crtc);
DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
if (vop->dpms == mode) {
DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
return;
}
switch (mode) { case DRM_MODE_DPMS_ON: vop_enable(crtc);
@@ -795,8 +799,6 @@ static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) DRM_DEBUG_KMS("unspecified mode %d\n", mode); break; }
- vop->dpms = mode;
}
static void vop_crtc_prepare(struct drm_crtc *crtc) @@ -934,9 +936,9 @@ static int vop_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *old_fb = crtc->primary->fb; int ret;
- /* when the page flip is requested, crtc's dpms should be on */
- if (vop->dpms > DRM_MODE_DPMS_ON) {
DRM_DEBUG("failed page flip request at dpms[%d].\n", vop->dpms);
- /* when the page flip is requested, crtc should be on */
- if (!vop->is_enabled) {
return 0; }DRM_DEBUG("page flip request rejected because crtc is off.\n");
@@ -1302,7 +1304,7 @@ static int vop_initial(struct vop *vop)
clk_disable(vop->hclk);
- vop->dpms = DRM_MODE_DPMS_OFF;
vop->is_enabled = false;
return 0;
-- 1.7.9.5
On 2015年01月22日 15:33, Daniel Vetter wrote:
On Thu, Jan 22, 2015 at 03:05:32PM +0800, Mark Yao wrote:
drm dpms have many power modes: ON,OFF,SUSPEND,STANDBY, etc. but vop only have enable/disable mode, maybe case such bug: --> DRM_DPMS_ON: power on vop --> DRM_DPMS_SUSPEND: power off vop --> DRM_DPMS_OFF: already power off at SUSPEND, crash so use a bool val is more suitable.
Signed-off-by: Mark Yao mark.yao@rock-chips.com
Long term I highly suggest you switch to atomic, since with atomic all the legacy dpms modes are remapped to a simple on/off. Also the new atomic helpers make sure that your backend isn't called multiple times, so you can ditch all your is_enabled tracking with that. -Daniel
Hi Daniel is there some documents teach me how to switch to atomic easily? I found many other drivers which use atomic also remap dpms modes to simple on/off at its driver, and I don't know where atomic helper do the remapped, can you give me some suggestions?
- Mark.
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 ++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 9a5c571..f278c09 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -81,7 +81,7 @@ struct vop { struct drm_crtc crtc; struct device *dev; struct drm_device *drm_dev;
- unsigned int dpms;
bool is_enabled;
int connector_type; int connector_out_mode;
@@ -387,6 +387,9 @@ static void vop_enable(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); int ret;
- if (vop->is_enabled)
return;
- ret = clk_enable(vop->hclk); if (ret < 0) { dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
@@ -427,6 +430,8 @@ static void vop_enable(struct drm_crtc *crtc)
drm_vblank_on(vop->drm_dev, vop->pipe);
vop->is_enabled = false;
return;
err_disable_aclk:
@@ -441,6 +446,9 @@ static void vop_disable(struct drm_crtc *crtc) { struct vop *vop = to_vop(crtc);
if (vop->is_enabled)
return;
drm_vblank_off(crtc->dev, vop->pipe);
disable_irq(vop->irq);
@@ -463,6 +471,8 @@ static void vop_disable(struct drm_crtc *crtc)
clk_disable(vop->aclk); clk_disable(vop->hclk);
vop->is_enabled = false; }
/*
@@ -742,7 +752,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON)
if (!vop->is_enabled) return -EPERM;
spin_lock_irqsave(&vop->irq_lock, flags);
@@ -759,8 +769,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) struct vop *vop = to_vop(crtc); unsigned long flags;
- if (vop->dpms != DRM_MODE_DPMS_ON)
- if (!vop->is_enabled) return;
- spin_lock_irqsave(&vop->irq_lock, flags); vop_mask_write(vop, INTR_CTRL0, FS_INTR_MASK, FS_INTR_EN(0)); spin_unlock_irqrestore(&vop->irq_lock, flags);
@@ -773,15 +784,8 @@ static const struct rockchip_crtc_funcs private_crtc_funcs = {
static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) {
struct vop *vop = to_vop(crtc);
DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
if (vop->dpms == mode) {
DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
return;
}
switch (mode) { case DRM_MODE_DPMS_ON: vop_enable(crtc);
@@ -795,8 +799,6 @@ static void vop_crtc_dpms(struct drm_crtc *crtc, int mode) DRM_DEBUG_KMS("unspecified mode %d\n", mode); break; }
vop->dpms = mode; }
static void vop_crtc_prepare(struct drm_crtc *crtc)
@@ -934,9 +936,9 @@ static int vop_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *old_fb = crtc->primary->fb; int ret;
- /* when the page flip is requested, crtc's dpms should be on */
- if (vop->dpms > DRM_MODE_DPMS_ON) {
DRM_DEBUG("failed page flip request at dpms[%d].\n", vop->dpms);
- /* when the page flip is requested, crtc should be on */
- if (!vop->is_enabled) {
return 0; }DRM_DEBUG("page flip request rejected because crtc is off.\n");
@@ -1302,7 +1304,7 @@ static int vop_initial(struct vop *vop)
clk_disable(vop->hclk);
- vop->dpms = DRM_MODE_DPMS_OFF;
vop->is_enabled = false;
return 0;
-- 1.7.9.5
On Thu, Jan 22, 2015 at 04:56:09PM +0800, Mark yao wrote:
On 2015年01月22日 15:33, Daniel Vetter wrote:
On Thu, Jan 22, 2015 at 03:05:32PM +0800, Mark Yao wrote:
drm dpms have many power modes: ON,OFF,SUSPEND,STANDBY, etc. but vop only have enable/disable mode, maybe case such bug: --> DRM_DPMS_ON: power on vop --> DRM_DPMS_SUSPEND: power off vop --> DRM_DPMS_OFF: already power off at SUSPEND, crash so use a bool val is more suitable.
Signed-off-by: Mark Yao mark.yao@rock-chips.com
Long term I highly suggest you switch to atomic, since with atomic all the legacy dpms modes are remapped to a simple on/off. Also the new atomic helpers make sure that your backend isn't called multiple times, so you can ditch all your is_enabled tracking with that. -Daniel
Hi Daniel is there some documents teach me how to switch to atomic easily? I found many other drivers which use atomic also remap dpms modes to simple on/off at its driver, and I don't know where atomic helper do the remapped, can you give me some suggestions?
The dpms remapping patches are still in-flight. But for the general atomic conversion please look at
http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
If you want to look at an actual driver there's msm already merged, tegra (conversion just posted) and exynos (iirc not yet published all, but Gustavo should have a branch for you to look at somewhere).
Cheers, Daniel
there is a Bug that: vop_enable()->drm_vblank_on, drm_vblank_on may call vop enable vblank. if it happen, vblank enable would failed, then cause irq status error. because is_enabled value is set after drm_vblank_on.
after enable vop clocks and iommu regs, we can sure that R/W vop regs and do vop plane flip is safe, so place is_enabled = true after enable iommu is suitable.
Signed-off-by: Mark Yao mark.yao@rock-chips.com --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index f278c09..a009457 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -420,6 +420,11 @@ static void vop_enable(struct drm_crtc *crtc) goto err_disable_aclk; }
+ /* + * At here, vop clock & iommu is enable, R/W vop regs would be safe. + */ + vop->is_enabled = false; + spin_lock(&vop->reg_lock);
VOP_CTRL_SET(vop, standby, 0); @@ -430,8 +435,6 @@ static void vop_enable(struct drm_crtc *crtc)
drm_vblank_on(vop->drm_dev, vop->pipe);
- vop->is_enabled = false; - return;
err_disable_aclk: @@ -462,6 +465,8 @@ static void vop_disable(struct drm_crtc *crtc) VOP_CTRL_SET(vop, standby, 1);
spin_unlock(&vop->reg_lock); + + vop->is_enabled = false; /* * disable dclk to stop frame scan, so we can safely detach iommu, */ @@ -471,8 +476,6 @@ static void vop_disable(struct drm_crtc *crtc)
clk_disable(vop->aclk); clk_disable(vop->hclk); - - vop->is_enabled = false; }
/*
dri-devel@lists.freedesktop.org