Re: [Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
On Wed, Jun 04, 2014 at 06:10:44PM +0200, Daniel Vetter wrote: On Wed, Jun 04, 2014 at 11:01:01AM -0300, Paulo Zanoni wrote: This function is only called at init/resume. It populates the software state with something that matches the current hardware state. I guess a comment explaning the purpose of the function is the best we can do here, or do you have a better idea? The problem is that we can use the get_hw_state functions not only to check driver state a init/resume, but also to do state tracking assertions at certain points of the code. Since most (all?) the other HW state readout functions don't have side-effects, there's a possibility that someone may add code to do HW state assertion at some points, and just call these things without realizing the potential side effects. A comment would help, but moving the assignment to another place would also solve the problem for me. You choose. We already do that in other places, e.g. the edp bit depth hacks we have. But a comment might be justified. I haven't looked at the details here, just a high-level comment that we do play such ugly tricks already. And I agree that it's unexpected. Well IIRC I didn't even call this function get_hw_state() until someone asked me to rename it ;) I could rename it back to whatever it was originally. It was never meant to be just a read hardware state into a temp structure type of thing since it's been updating intel_crtc-wm.active from the start. With the rename I should be able to drop the ugly underscore from _ilk_wm_get_hw_state(). -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
2014-06-03 16:32 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
On Tue, Jun 03, 2014 at 03:50:12PM -0300, Paulo Zanoni wrote:
2014-05-22 11:48 GMT-03:00 ville.syrj...@linux.intel.com:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Add a mechanism by which you can queue up watermark update to happen
after the vblank counter has reached a certain value. The vblank
interrupt handler will schedule a work which will do the actual
watermark programming in process context.
v2: Rebase and s/intel_crtc/crtc/
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_irq.c | 12 ++-
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 27 +++
drivers/gpu/drm/i915/intel_pm.c | 144
+++
5 files changed, 183 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index a2302a7..c90d5ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1541,6 +1541,8 @@ struct drm_i915_private {
* state as well as the actual hardware registers
*/
struct mutex mutex;
+
+ struct work_struct work;
} wm;
struct i915_runtime_pm pm;
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index 304f86a..c680020 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2067,8 +2067,10 @@ static void ilk_display_irq_handler(struct
drm_device *dev, u32 de_iir)
DRM_ERROR(Poison interrupt\n);
for_each_pipe(pipe) {
- if (de_iir DE_PIPE_VBLANK(pipe))
+ if (de_iir DE_PIPE_VBLANK(pipe)) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (de_iir DE_PIPE_FIFO_UNDERRUN(pipe))
if (intel_set_cpu_fifo_underrun_reporting(dev,
pipe, false))
@@ -2117,8 +2119,10 @@ static void ivb_display_irq_handler(struct
drm_device *dev, u32 de_iir)
intel_opregion_asle_intr(dev);
for_each_pipe(pipe) {
- if (de_iir (DE_PIPE_VBLANK_IVB(pipe)))
+ if (de_iir (DE_PIPE_VBLANK_IVB(pipe))) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
/* plane/pipes map 1:1 on ilk+ */
if (de_iir DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2260,8 +2264,10 @@ static irqreturn_t gen8_irq_handler(int irq, void
*arg)
continue;
pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
- if (pipe_iir GEN8_PIPE_VBLANK)
+ if (pipe_iir GEN8_PIPE_VBLANK) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (pipe_iir GEN8_PIPE_PRIMARY_FLIP_DONE) {
intel_prepare_page_flip(dev, pipe);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a11bd78..408b238 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10961,6 +10961,7 @@ static void intel_crtc_init(struct drm_device
*dev, int pipe)
intel_crtc-plane = !pipe;
}
+ spin_lock_init(intel_crtc-wm.lock);
init_waitqueue_head(intel_crtc-vbl_wait);
BUG_ON(pipe = ARRAY_SIZE(dev_priv-plane_to_crtc_mapping) ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index d75cc2b..72f01b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -409,6 +409,32 @@ struct intel_crtc {
* protected by dev_priv-wm.mutex
*/
struct intel_pipe_wm active;
+ /*
+* watermarks queued for next vblank
+* protected by dev_priv-wm.mutex
+*/
+ struct intel_pipe_wm pending;
+
+ /*
+* the vblank count after which we can switch over to
'pending'
+* protected by intel_crtc-wm.lock
+*/
+ u32 pending_vbl_count;
+ /*
+* indicates that 'pending' contains changed watermarks
+* protected by intel_crtc-wm.lock
+*/
+ bool dirty;
+ /*
+* watermark update has a vblank reference?
+* protected by intel_crtc-wm.lock
+*/
+
Re: [Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
On Wed, Jun 04, 2014 at 11:01:01AM -0300, Paulo Zanoni wrote: This function is only called at init/resume. It populates the software state with something that matches the current hardware state. I guess a comment explaning the purpose of the function is the best we can do here, or do you have a better idea? The problem is that we can use the get_hw_state functions not only to check driver state a init/resume, but also to do state tracking assertions at certain points of the code. Since most (all?) the other HW state readout functions don't have side-effects, there's a possibility that someone may add code to do HW state assertion at some points, and just call these things without realizing the potential side effects. A comment would help, but moving the assignment to another place would also solve the problem for me. You choose. We already do that in other places, e.g. the edp bit depth hacks we have. But a comment might be justified. I haven't looked at the details here, just a high-level comment that we do play such ugly tricks already. And I agree that it's unexpected. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
2014-05-22 11:48 GMT-03:00 ville.syrj...@linux.intel.com:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Add a mechanism by which you can queue up watermark update to happen
after the vblank counter has reached a certain value. The vblank
interrupt handler will schedule a work which will do the actual
watermark programming in process context.
v2: Rebase and s/intel_crtc/crtc/
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_irq.c | 12 ++-
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 27 +++
drivers/gpu/drm/i915/intel_pm.c | 144
+++
5 files changed, 183 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2302a7..c90d5ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1541,6 +1541,8 @@ struct drm_i915_private {
* state as well as the actual hardware registers
*/
struct mutex mutex;
+
+ struct work_struct work;
} wm;
struct i915_runtime_pm pm;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 304f86a..c680020 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2067,8 +2067,10 @@ static void ilk_display_irq_handler(struct drm_device
*dev, u32 de_iir)
DRM_ERROR(Poison interrupt\n);
for_each_pipe(pipe) {
- if (de_iir DE_PIPE_VBLANK(pipe))
+ if (de_iir DE_PIPE_VBLANK(pipe)) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (de_iir DE_PIPE_FIFO_UNDERRUN(pipe))
if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
false))
@@ -2117,8 +2119,10 @@ static void ivb_display_irq_handler(struct drm_device
*dev, u32 de_iir)
intel_opregion_asle_intr(dev);
for_each_pipe(pipe) {
- if (de_iir (DE_PIPE_VBLANK_IVB(pipe)))
+ if (de_iir (DE_PIPE_VBLANK_IVB(pipe))) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
/* plane/pipes map 1:1 on ilk+ */
if (de_iir DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2260,8 +2264,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
continue;
pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
- if (pipe_iir GEN8_PIPE_VBLANK)
+ if (pipe_iir GEN8_PIPE_VBLANK) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (pipe_iir GEN8_PIPE_PRIMARY_FLIP_DONE) {
intel_prepare_page_flip(dev, pipe);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a11bd78..408b238 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10961,6 +10961,7 @@ static void intel_crtc_init(struct drm_device *dev,
int pipe)
intel_crtc-plane = !pipe;
}
+ spin_lock_init(intel_crtc-wm.lock);
init_waitqueue_head(intel_crtc-vbl_wait);
BUG_ON(pipe = ARRAY_SIZE(dev_priv-plane_to_crtc_mapping) ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index d75cc2b..72f01b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -409,6 +409,32 @@ struct intel_crtc {
* protected by dev_priv-wm.mutex
*/
struct intel_pipe_wm active;
+ /*
+* watermarks queued for next vblank
+* protected by dev_priv-wm.mutex
+*/
+ struct intel_pipe_wm pending;
+
+ /*
+* the vblank count after which we can switch over to
'pending'
+* protected by intel_crtc-wm.lock
+*/
+ u32 pending_vbl_count;
+ /*
+* indicates that 'pending' contains changed watermarks
+* protected by intel_crtc-wm.lock
+*/
+ bool dirty;
+ /*
+* watermark update has a vblank reference?
+* protected by intel_crtc-wm.lock
+*/
+ bool vblank;
I would rename this variable. Maybe has_vblank_ref?
+
+ /*
+* protects some intel_crtc-wm state
Please be more precise on what some actually means, since it's easy
to guess that a spinlock protects some
Re: [Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
On Tue, Jun 03, 2014 at 03:50:12PM -0300, Paulo Zanoni wrote:
2014-05-22 11:48 GMT-03:00 ville.syrj...@linux.intel.com:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Add a mechanism by which you can queue up watermark update to happen
after the vblank counter has reached a certain value. The vblank
interrupt handler will schedule a work which will do the actual
watermark programming in process context.
v2: Rebase and s/intel_crtc/crtc/
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_irq.c | 12 ++-
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 27 +++
drivers/gpu/drm/i915/intel_pm.c | 144
+++
5 files changed, 183 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index a2302a7..c90d5ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1541,6 +1541,8 @@ struct drm_i915_private {
* state as well as the actual hardware registers
*/
struct mutex mutex;
+
+ struct work_struct work;
} wm;
struct i915_runtime_pm pm;
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index 304f86a..c680020 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2067,8 +2067,10 @@ static void ilk_display_irq_handler(struct
drm_device *dev, u32 de_iir)
DRM_ERROR(Poison interrupt\n);
for_each_pipe(pipe) {
- if (de_iir DE_PIPE_VBLANK(pipe))
+ if (de_iir DE_PIPE_VBLANK(pipe)) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (de_iir DE_PIPE_FIFO_UNDERRUN(pipe))
if (intel_set_cpu_fifo_underrun_reporting(dev,
pipe, false))
@@ -2117,8 +2119,10 @@ static void ivb_display_irq_handler(struct
drm_device *dev, u32 de_iir)
intel_opregion_asle_intr(dev);
for_each_pipe(pipe) {
- if (de_iir (DE_PIPE_VBLANK_IVB(pipe)))
+ if (de_iir (DE_PIPE_VBLANK_IVB(pipe))) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
/* plane/pipes map 1:1 on ilk+ */
if (de_iir DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2260,8 +2264,10 @@ static irqreturn_t gen8_irq_handler(int irq, void
*arg)
continue;
pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
- if (pipe_iir GEN8_PIPE_VBLANK)
+ if (pipe_iir GEN8_PIPE_VBLANK) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (pipe_iir GEN8_PIPE_PRIMARY_FLIP_DONE) {
intel_prepare_page_flip(dev, pipe);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a11bd78..408b238 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10961,6 +10961,7 @@ static void intel_crtc_init(struct drm_device *dev,
int pipe)
intel_crtc-plane = !pipe;
}
+ spin_lock_init(intel_crtc-wm.lock);
init_waitqueue_head(intel_crtc-vbl_wait);
BUG_ON(pipe = ARRAY_SIZE(dev_priv-plane_to_crtc_mapping) ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index d75cc2b..72f01b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -409,6 +409,32 @@ struct intel_crtc {
* protected by dev_priv-wm.mutex
*/
struct intel_pipe_wm active;
+ /*
+* watermarks queued for next vblank
+* protected by dev_priv-wm.mutex
+*/
+ struct intel_pipe_wm pending;
+
+ /*
+* the vblank count after which we can switch over to
'pending'
+* protected by intel_crtc-wm.lock
+*/
+ u32 pending_vbl_count;
+ /*
+* indicates that 'pending' contains changed watermarks
+* protected by intel_crtc-wm.lock
+*/
+ bool dirty;
+ /*
+* watermark update has a vblank reference?
+* protected by intel_crtc-wm.lock
+*/
+ bool vblank;
I would rename this variable. Maybe
[Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
From: Ville Syrjälä ville.syrj...@linux.intel.com
Add a mechanism by which you can queue up watermark update to happen
after the vblank counter has reached a certain value. The vblank
interrupt handler will schedule a work which will do the actual
watermark programming in process context.
v2: Rebase and s/intel_crtc/crtc/
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_irq.c | 12 ++-
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 27 +++
drivers/gpu/drm/i915/intel_pm.c | 144 +++
5 files changed, 183 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2302a7..c90d5ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1541,6 +1541,8 @@ struct drm_i915_private {
* state as well as the actual hardware registers
*/
struct mutex mutex;
+
+ struct work_struct work;
} wm;
struct i915_runtime_pm pm;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 304f86a..c680020 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2067,8 +2067,10 @@ static void ilk_display_irq_handler(struct drm_device
*dev, u32 de_iir)
DRM_ERROR(Poison interrupt\n);
for_each_pipe(pipe) {
- if (de_iir DE_PIPE_VBLANK(pipe))
+ if (de_iir DE_PIPE_VBLANK(pipe)) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (de_iir DE_PIPE_FIFO_UNDERRUN(pipe))
if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
false))
@@ -2117,8 +2119,10 @@ static void ivb_display_irq_handler(struct drm_device
*dev, u32 de_iir)
intel_opregion_asle_intr(dev);
for_each_pipe(pipe) {
- if (de_iir (DE_PIPE_VBLANK_IVB(pipe)))
+ if (de_iir (DE_PIPE_VBLANK_IVB(pipe))) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
/* plane/pipes map 1:1 on ilk+ */
if (de_iir DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2260,8 +2264,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
continue;
pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
- if (pipe_iir GEN8_PIPE_VBLANK)
+ if (pipe_iir GEN8_PIPE_VBLANK) {
intel_pipe_handle_vblank(dev, pipe);
+ ilk_update_pipe_wm(dev, pipe);
+ }
if (pipe_iir GEN8_PIPE_PRIMARY_FLIP_DONE) {
intel_prepare_page_flip(dev, pipe);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a11bd78..408b238 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10961,6 +10961,7 @@ static void intel_crtc_init(struct drm_device *dev, int
pipe)
intel_crtc-plane = !pipe;
}
+ spin_lock_init(intel_crtc-wm.lock);
init_waitqueue_head(intel_crtc-vbl_wait);
BUG_ON(pipe = ARRAY_SIZE(dev_priv-plane_to_crtc_mapping) ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d75cc2b..72f01b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -409,6 +409,32 @@ struct intel_crtc {
* protected by dev_priv-wm.mutex
*/
struct intel_pipe_wm active;
+ /*
+* watermarks queued for next vblank
+* protected by dev_priv-wm.mutex
+*/
+ struct intel_pipe_wm pending;
+
+ /*
+* the vblank count after which we can switch over to 'pending'
+* protected by intel_crtc-wm.lock
+*/
+ u32 pending_vbl_count;
+ /*
+* indicates that 'pending' contains changed watermarks
+* protected by intel_crtc-wm.lock
+*/
+ bool dirty;
+ /*
+* watermark update has a vblank reference?
+* protected by intel_crtc-wm.lock
+*/
+ bool vblank;
+
+ /*
+* protects some intel_crtc-wm state
+*/
+ spinlock_t lock;
} wm;
wait_queue_head_t vbl_wait;
@@ -974,6 +1000,7 @@ void intel_runtime_pm_put(struct drm_i915_private
*dev_priv);
void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
void
