* [PATCH 0/2] drm/rockchip: Optimization vop dpms control @ 2015-01-22 7:05 ` Mark Yao 0 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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(-) -- 1.7.9.5 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/2] drm/rockchip: Optimization vop dpms control @ 2015-01-22 7:05 ` Mark Yao 0 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode 2015-01-22 7:05 ` Mark Yao @ 2015-01-22 7:05 ` Mark Yao -1 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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; -- 1.7.9.5 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode @ 2015-01-22 7:05 ` Mark Yao 0 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode 2015-01-22 7:05 ` Mark Yao @ 2015-01-22 7:33 ` Daniel Vetter -1 siblings, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2015-01-22 7:33 UTC (permalink / raw) To: Mark Yao; +Cc: linux-kernel, linux-rockchip, dri-devel 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) { > + 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; > > -- > 1.7.9.5 > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode @ 2015-01-22 7:33 ` Daniel Vetter 0 siblings, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2015-01-22 7:33 UTC (permalink / raw) To: Mark Yao Cc: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip 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) { > + 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; > > -- > 1.7.9.5 > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode 2015-01-22 7:33 ` Daniel Vetter @ 2015-01-22 8:56 ` Mark yao -1 siblings, 0 replies; 12+ messages in thread From: Mark yao @ 2015-01-22 8:56 UTC (permalink / raw) To: David Airlie, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip 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) { >> + 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; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode @ 2015-01-22 8:56 ` Mark yao 0 siblings, 0 replies; 12+ messages in thread From: Mark yao @ 2015-01-22 8:56 UTC (permalink / raw) To: David Airlie, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip 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) { >> + 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; >> >> -- >> 1.7.9.5 >> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode 2015-01-22 8:56 ` Mark yao @ 2015-01-22 13:27 ` Daniel Vetter -1 siblings, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2015-01-22 13:27 UTC (permalink / raw) To: Mark yao; +Cc: linux-kernel, dri-devel, linux-rockchip 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode @ 2015-01-22 13:27 ` Daniel Vetter 0 siblings, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2015-01-22 13:27 UTC (permalink / raw) To: Mark yao Cc: David Airlie, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] drm/rockchip: vop: set vop enabled after enable iommu 2015-01-22 7:05 ` Mark Yao @ 2015-01-22 7:05 ` Mark Yao -1 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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; } /* -- 1.7.9.5 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] drm/rockchip: vop: set vop enabled after enable iommu @ 2015-01-22 7:05 ` Mark Yao 0 siblings, 0 replies; 12+ messages in thread From: Mark Yao @ 2015-01-22 7:05 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Clark, Philipp Zabel, Daniel Kurtz, dri-devel, linux-kernel, linux-rockchip Cc: Mark Yao 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; } /* -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-01-22 13:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-22 7:05 [PATCH 0/2] drm/rockchip: Optimization vop dpms control Mark Yao 2015-01-22 7:05 ` Mark Yao 2015-01-22 7:05 ` [PATCH 1/2] drm/rockchip: vop use is_enabled instead of dpms mode Mark Yao 2015-01-22 7:05 ` Mark Yao 2015-01-22 7:33 ` Daniel Vetter 2015-01-22 7:33 ` Daniel Vetter 2015-01-22 8:56 ` Mark yao 2015-01-22 8:56 ` Mark yao 2015-01-22 13:27 ` Daniel Vetter 2015-01-22 13:27 ` Daniel Vetter 2015-01-22 7:05 ` [PATCH 2/2] drm/rockchip: vop: set vop enabled after enable iommu Mark Yao 2015-01-22 7:05 ` Mark Yao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.