drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

Paulo noticed that the fbc frontbuffer tracking flush callback occasionally gets a call without any bit set. This can happen when we have to filter flush calls due to e.g. gpu rendering. Filter these out.

Reported-by Paulo Zanoni <[email protected]>

Cc Paulo Zanoni <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_frontbuffer.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/intel_frontbuffer.c +3 -1

index 2a1611a7ce1d..899b6112bbe0 100644
@@ -179,6 +179,9 @@ void intel_frontbuffer_flush(struct drm_device *dev,
        frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
        mutex_unlock(&dev_priv->fb_tracking.lock);
 
+       if (!frontbuffer_bits)
+               return;
+
        intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
 
        intel_edp_drrs_flush(dev, frontbuffer_bits);
--

2.1.0

Useful to figure out whether stuck bits are due to the frontbuffer tracking code as opposed to individual consumers (who have their own bitmask tracking).

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/gpu/drm/i915/i915_debugfs.c +16 -1

index c49fe2afa223..f3b80627d5b7 100644
@@ -1581,6 +1581,21 @@ static int i915_drpc_info(struct seq_file *m, void *unused)
                return ironlake_drpc_info(m);
 }
 
+static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
+{
+       struct drm_info_node *node = m->private;
+       struct drm_device *dev = node->minor->dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
+
+       seq_printf(m, "FB tracking busy bits: 0x%08x\n",
+                  dev_priv->fb_tracking.busy_bits);
+
+       seq_printf(m, "FB tracking flip bits: 0x%08x\n",
+                  dev_priv->fb_tracking.flip_bits);
+
+       return 0;
+}
+
 static int i915_fbc_status(struct seq_file *m, void *unused)
 {
        struct drm_info_node *node = m->private;

@@ -5024,6 +5039,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
        {"i915_drpc_info", i915_drpc_info, 0},
        {"i915_emon_status", i915_emon_status, 0},
        {"i915_ring_freq_table", i915_ring_freq_table, 0},
+       {"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
        {"i915_fbc_status", i915_fbc_status, 0},
        {"i915_ips_status", i915_ips_status, 0},
        {"i915_sr_status", i915_sr_status, 0},
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye.

I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking.

Quoted 7 lines · Daniel Vetter

With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye.

I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking.

Can you nuke just the intel_mark_busy side? Keeping the mode finder intact would be useful as the intrepid reader need only then implement the intel_frontbuffer callbacks and have the harder part of matching appropriate modes and switching routines ready to plug in. (Those latter ones I expect to be tweaked over time, and so the reader's first step of reverting this commit would conflict in such a way as to dissuade them.)

Quoted 9 lines · Chris Wilson

Quoted 7 lines · Daniel Vetter

With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye.

I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking.

Quoted 6 lines

Can you nuke just the intel_mark_busy side? Keeping the mode finder intact would be useful as the intrepid reader need only then implement the intel_frontbuffer callbacks and have the harder part of matching appropriate modes and switching routines ready to plug in. (Those latter ones I expect to be tweaked over time, and so the reader's first step of reverting this commit would conflict in such a way as to dissuade them.)

Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured getting rid of that is good - it really should be stored somewhere in intel_lvds or in the pipe_config. Also given that no one ever really bothered to fix this up since gen5 (where the bit to change frequency moved around iirc) I don't think anyone will ever resurrect this. Hence the much more eager delete.

Quoted 17 lines · Daniel Vetter

Quoted 9 lines · Chris Wilson

Quoted 7 lines · Daniel Vetter

With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye.

I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking.

Quoted 6 lines

Can you nuke just the intel_mark_busy side? Keeping the mode finder intact would be useful as the intrepid reader need only then implement the intel_frontbuffer callbacks and have the harder part of matching appropriate modes and switching routines ready to plug in. (Those latter ones I expect to be tweaked over time, and so the reader's first step of reverting this commit would conflict in such a way as to dissuade them.)

Quoted 6 lines

Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured getting rid of that is good - it really should be stored somewhere in intel_lvds or in the pipe_config. Also given that no one ever really bothered to fix this up since gen5 (where the bit to change frequency moved around iirc) I don't think anyone will ever resurrect this. Hence the much more eager delete.

*shrug* we know that people do try to use it, I was just thinking of a way that we could make it easier for them to refresh the code. (Ideally, I would prefer that they wouldn't have to and the new framework provided the impetus needed to solidify LVDS reclocking. All that was lacking was the vblank task to do the reclocking on the refresh to hide any flicker on transition. That and so few manufacturers supplied dual-mode panels.)

Quoted 25 lines · Chris Wilson

Quoted 17 lines · Daniel Vetter

Quoted 9 lines · Chris Wilson

Quoted 7 lines · Daniel Vetter

With the new DRRS code it kinda sticks out, and we never managed to get this to work well enough without causing issues. Time to wave goodbye.

I've decided to keep the logic for programming the reduced clocks intact, but everything else is gone. If anyone ever wants to resurrect this we need to redo it all anyway on top of the frontbuffer tracking.

Quoted 6 lines

Can you nuke just the intel_mark_busy side? Keeping the mode finder intact would be useful as the intrepid reader need only then implement the intel_frontbuffer callbacks and have the harder part of matching appropriate modes and switching routines ready to plug in. (Those latter ones I expect to be tweaked over time, and so the reader's first step of reverting this commit would conflict in such a way as to dissuade them.)

Quoted 6 lines

Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured getting rid of that is good - it really should be stored somewhere in intel_lvds or in the pipe_config. Also given that no one ever really bothered to fix this up since gen5 (where the bit to change frequency moved around iirc) I don't think anyone will ever resurrect this. Hence the much more eager delete.

Quoted 6 lines

*shrug* we know that people do try to use it, I was just thinking of a way that we could make it easier for them to refresh the code. (Ideally, I would prefer that they wouldn't have to and the new framework provided the impetus needed to solidify LVDS reclocking. All that was lacking was the vblank task to do the reclocking on the refresh to hide any flicker on transition. That and so few manufacturers supplied dual-mode panels.)

Not sure users trying to use it is a good argument - they enthusiastically try it on gen5+ too (where we don't frob the right pipe bits to make the frequency switch) and on machines without lvds panels. I expect a dummy i915.save_me_some_power option would get enabled too ;-)

Btw for the vblank work I think the crucial bit is that we want seamless DRRS (which edp drrs checks in the vbt) and lvds drrs never did check that. At least that might explain why it flickered often badly.

Given that ack or nack?

I was momentarily confused until I've double-checked that these functions really only compute state and don't update the hardware state. They once did that, but since Ander's rework of the dpll computation flow that's no longer the case.

Rename them to avoid further confusion.

Note that the ilk code already follows the compute_dpll naming scheme for computing the actual register value. DDI code goes with _calc_, but that is close enough.

Cc Ander Conselvan de Oliveira <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

drivers/gpu/drm/i915/intel_display.c +20 -21

index 45a93c87d48e..a8d9dbd5bd34 100644
@@ -7233,8 +7233,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
                intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc,
-                          struct intel_crtc_state *pipe_config)
+static void vlv_compute_dpll(struct intel_crtc *crtc,
+                            struct intel_crtc_state *pipe_config)
 {
        u32 dpll, dpll_md;
 

@@ -7347,8 +7347,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
        mutex_unlock(&dev_priv->sb_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc,
-                          struct intel_crtc_state *pipe_config)
+static void chv_compute_dpll(struct intel_crtc *crtc,
+                            struct intel_crtc_state *pipe_config)
 {
        pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
                DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |

@@ -7487,11 +7487,11 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
        };
 
        if (IS_CHERRYVIEW(dev)) {
-               chv_update_pll(crtc, &pipe_config);
+               chv_compute_dpll(crtc, &pipe_config);
                chv_prepare_pll(crtc, &pipe_config);
                chv_enable_pll(crtc, &pipe_config);
        } else {
-               vlv_update_pll(crtc, &pipe_config);
+               vlv_compute_dpll(crtc, &pipe_config);
                vlv_prepare_pll(crtc, &pipe_config);
                vlv_enable_pll(crtc, &pipe_config);
        }

@@ -7513,10 +7513,10 @@ void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
                vlv_disable_pll(to_i915(dev), pipe);
 }
 
-static void i9xx_update_pll(struct intel_crtc *crtc,
-                           struct intel_crtc_state *crtc_state,
-                           intel_clock_t *reduced_clock,
-                           int num_connectors)
+static void i9xx_compute_dpll(struct intel_crtc *crtc,
+                             struct intel_crtc_state *crtc_state,
+                             intel_clock_t *reduced_clock,
+                             int num_connectors)
 {
        struct drm_device *dev = crtc->base.dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

@@ -7590,10 +7590,10 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
        }
 }
 
-static void i8xx_update_pll(struct intel_crtc *crtc,
-                           struct intel_crtc_state *crtc_state,
-                           intel_clock_t *reduced_clock,
-                           int num_connectors)
+static void i8xx_compute_dpll(struct intel_crtc *crtc,
+                             struct intel_crtc_state *crtc_state,
+                             intel_clock_t *reduced_clock,
+                             int num_connectors)
 {
        struct drm_device *dev = crtc->base.dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

@@ -7887,15 +7887,15 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
        }
 
        if (IS_GEN2(dev)) {
-               i8xx_update_pll(crtc, crtc_state, NULL,
-                               num_connectors);
+               i8xx_compute_dpll(crtc, crtc_state, NULL,
+                                 num_connectors);
        } else if (IS_CHERRYVIEW(dev)) {
-               chv_update_pll(crtc, crtc_state);
+               chv_compute_dpll(crtc, crtc_state);
        } else if (IS_VALLEYVIEW(dev)) {
-               vlv_update_pll(crtc, crtc_state);
+               vlv_compute_dpll(crtc, crtc_state);
        } else {
-               i9xx_update_pll(crtc, crtc_state, NULL,
-                               num_connectors);
+               i9xx_compute_dpll(crtc, crtc_state, NULL,
+                                 num_connectors);
        }
 
        return 0;
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-06-18 5:30 GMT-03:00 Daniel Vetter <[email protected]>:

Quoted 7 lines

I was momentarily confused until I've double-checked that these functions really only compute state and don't update the hardware state. They once did that, but since Ander's rework of the dpll computation flow that's no longer the case.

Rename them to avoid further confusion.

Quoted 3 lines

Note that the ilk code already follows the compute_dpll naming scheme for computing the actual register value. DDI code goes with _calc_, but that is close enough.

My only bikeshed would be to point that we now have x_prepare_pll(), x_enable_pll() and x_compute_dpll(), with the "d" to break the standard. OTOH the ILK function also has the "d" letter, so meh...

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 106 lines

Cc Ander Conselvan de Oliveira <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

drivers/gpu/drm/i915/intel_display.c +20 -21

index 45a93c87d48e..a8d9dbd5bd34 100644
@@ -7233,8 +7233,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
                intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc,
-                          struct intel_crtc_state *pipe_config)
+static void vlv_compute_dpll(struct intel_crtc *crtc,
+                            struct intel_crtc_state *pipe_config)
 {
        u32 dpll, dpll_md;
 

@@ -7347,8 +7347,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
        mutex_unlock(&dev_priv->sb_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc,
-                          struct intel_crtc_state *pipe_config)
+static void chv_compute_dpll(struct intel_crtc *crtc,
+                            struct intel_crtc_state *pipe_config)
 {
        pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
                DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |

@@ -7487,11 +7487,11 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
        };
 
        if (IS_CHERRYVIEW(dev)) {
-               chv_update_pll(crtc, &pipe_config);
+               chv_compute_dpll(crtc, &pipe_config);
                chv_prepare_pll(crtc, &pipe_config);
                chv_enable_pll(crtc, &pipe_config);
        } else {
-               vlv_update_pll(crtc, &pipe_config);
+               vlv_compute_dpll(crtc, &pipe_config);
                vlv_prepare_pll(crtc, &pipe_config);
                vlv_enable_pll(crtc, &pipe_config);
        }

@@ -7513,10 +7513,10 @@ void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
                vlv_disable_pll(to_i915(dev), pipe);
 }
 
-static void i9xx_update_pll(struct intel_crtc *crtc,
-                           struct intel_crtc_state *crtc_state,
-                           intel_clock_t *reduced_clock,
-                           int num_connectors)
+static void i9xx_compute_dpll(struct intel_crtc *crtc,
+                             struct intel_crtc_state *crtc_state,
+                             intel_clock_t *reduced_clock,
+                             int num_connectors)
 {
        struct drm_device *dev = crtc->base.dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

@@ -7590,10 +7590,10 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
        }
 }
 
-static void i8xx_update_pll(struct intel_crtc *crtc,
-                           struct intel_crtc_state *crtc_state,
-                           intel_clock_t *reduced_clock,
-                           int num_connectors)
+static void i8xx_compute_dpll(struct intel_crtc *crtc,
+                             struct intel_crtc_state *crtc_state,
+                             intel_clock_t *reduced_clock,
+                             int num_connectors)
 {
        struct drm_device *dev = crtc->base.dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

@@ -7887,15 +7887,15 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
        }
 
        if (IS_GEN2(dev)) {
-               i8xx_update_pll(crtc, crtc_state, NULL,
-                               num_connectors);
+               i8xx_compute_dpll(crtc, crtc_state, NULL,
+                                 num_connectors);
        } else if (IS_CHERRYVIEW(dev)) {
-               chv_update_pll(crtc, crtc_state);
+               chv_compute_dpll(crtc, crtc_state);
        } else if (IS_VALLEYVIEW(dev)) {
-               vlv_update_pll(crtc, crtc_state);
+               vlv_compute_dpll(crtc, crtc_state);
        } else {
-               i9xx_update_pll(crtc, crtc_state, NULL,
-                               num_connectors);
+               i9xx_compute_dpll(crtc, crtc_state, NULL,
+                                 num_connectors);
        }
 
        return 0;
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Paulo Zanoni

The current code tracks business across all pipes, but we're only really interested in the one pipe DRRS is enabled on. Fairly tiny optimization, but something I noticed while reading the code. But it might matter a bit when e.g. showing a video or something only on the external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take appropriate actions.

Cc Ramalingam C <[email protected]>

Cc Sivakumar Thulasimani <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +7 -7

index f52eef138247..3e8c88d119bb 100644
@@ -5485,16 +5485,15 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
        crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
        pipe = to_intel_crtc(crtc)->pipe;
 
+       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+       dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
+
        /* invalidate means busy screen hence upclock */
-       if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
+       if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
                intel_dp_set_drrs_state(dev_priv->dev,
                                dev_priv->drrs.dp->attached_connector->panel.
                                fixed_mode->vrefresh);
-       }
-
-       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 
-       dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
        mutex_unlock(&dev_priv->drrs.mutex);
 }
 

@@ -5530,10 +5529,12 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 
        crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
        pipe = to_intel_crtc(crtc)->pipe;
+
+       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
        dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
        /* flush means busy screen hence upclock */
-       if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+       if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
                intel_dp_set_drrs_state(dev_priv->dev,
                                dev_priv->drrs.dp->attached_connector->panel.
                                fixed_mode->vrefresh);
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

The current code tracks business across all pipes, but we're only really interested in the one pipe DRRS is enabled on. Fairly tiny

optimization, but something I noticed while reading the code. But it might matter a bit when e.g. showing a video or something only on the external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take appropriate actions.

Cc Rodrigo Vivi <[email protected]>

Cc Durgadoss R <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_psr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_psr.c +7 -5

index 5ee0fa57ed19..e354ceacb628 100644
@@ -663,11 +663,12 @@ void intel_psr_invalidate(struct drm_device *dev,
        crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
        pipe = to_intel_crtc(crtc)->pipe;
 
-       intel_psr_exit(dev);
-
        frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
-
        dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
+
+       if (frontbuffer_bits)
+               intel_psr_exit(dev);
+
        mutex_unlock(&dev_priv->psr.lock);
 }
 

@@ -698,6 +699,8 @@ void intel_psr_flush(struct drm_device *dev,
 
        crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
        pipe = to_intel_crtc(crtc)->pipe;
+
+       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
        dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
        /*

@@ -716,7 +719,7 @@ void intel_psr_flush(struct drm_device *dev,
         * invalidating. Which means we need to manually fake this in
         * software for all flushes, not just when we've seen a preceding
         * invalidation through frontbuffer rendering. */
-       if (!HAS_DDI(dev))
+       if (frontbuffer_bits && !HAS_DDI(dev))
                intel_psr_exit(dev);
 
        if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-06-18 5:30 GMT-03:00 Daniel Vetter <[email protected]>:

Quoted 2 lines

The current code tracks business across all pipes, but we're only really interested in the one pipe DRRS is enabled on. Fairly tiny

s/DRRS/PSR/

Quoted 6 lines

optimization, but something I noticed while reading the code. But it might matter a bit when e.g. showing a video or something only on the external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take appropriate actions.

One of the ideas I had last year was to implement a way to tell user space (possibly through debugs) when FBC/PSR gets enabled/disabled. My initial idea was to do this through timestamps. Our IGT suite would then verify those timestamps: if we have 2 pipes enabled, then we write on the non-FBC buffer, the FBC timestamps can't change. This type of test would catch exactly the bug you're solving here.

I even implemented this, and added support for it on kms_frontbuffer_tracking, but I ended never sending the Kernel patch to the list due to the possible slowness created by the constant timestamp generation. Maybe I should resurrect this and hide it under some debug kconfig option. I also considered maybe moving the implementation from timestamps debugfs file events, or maybe tracing code, or something else. Ideas and bikesheds are welcome here.

So I added some debug messages to the PSR code, then I ran:
sudo ./kms_frontbuffer_tracking --run-subtest psr-2p-scndscrn-pri-sfb-draw-mmap-cpu --step --step

And I kept watching dmesg as I stepped. Without the patch, I could see psr being disabled/enabled as we were writing on the secondary screen frontbuffer. With the patch, we stop calling intel_psr_exit()! So:

The frontbuffer code gives us accurate information about activity, let's use it. Again this should avoid unecessary updates when multiple screens are on.

Also realing function paramaters, I couldn't resist that bit of OCD.

The frontbuffer code gives us accurate information about activity, let's use it. Again this should avoid unecessary updates when multiple screens are on.

Also realing function paramaters, I couldn't resist that bit of OCD.

Cc Rodrigo Vivi <[email protected]>

Cc Durgadoss R <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_drv.h         |  7 ++++---
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_psr.c         | 22 +++++++++++++---------
 3 files changed, 18 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +4 -3

index b7c69460fb20..0ba9065dec88 100644
@@ -1310,11 +1310,12 @@ void intel_backlight_unregister(struct drm_device *dev);
 void intel_psr_enable(struct intel_dp *intel_dp);
 void intel_psr_disable(struct intel_dp *intel_dp);
 void intel_psr_invalidate(struct drm_device *dev,
-                             unsigned frontbuffer_bits);
+                         unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
-                        unsigned frontbuffer_bits);
+                    unsigned frontbuffer_bits);
 void intel_psr_init(struct drm_device *dev);
-void intel_psr_single_frame_update(struct drm_device *dev);
+void intel_psr_single_frame_update(struct drm_device *dev,
+                                  unsigned frontbuffer_bits);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);

drivers/gpu/drm/i915/intel_frontbuffer.c +1 -1

index 3c4862c8629d..ede6ccc45375 100644
@@ -190,7 +190,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
        dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
        mutex_unlock(&dev_priv->fb_tracking.lock);
 
-       intel_psr_single_frame_update(dev);
+       intel_psr_single_frame_update(dev, frontbuffer_bits);
 }
 
 /**

drivers/gpu/drm/i915/intel_psr.c +13 -10

index e354ceacb628..d79ba58637d7 100644
@@ -596,13 +596,15 @@ static void intel_psr_exit(struct drm_device *dev)
 /**
  * intel_psr_single_frame_update - Single Frame Update
  * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
  *
  * Some platforms support a single frame update feature that is used to
  * send and update only one frame on Remote Frame Buffer.
  * So far it is only implemented for Valleyview and Cherryview because
  * hardware requires this to be done before a page flip.
  */
-void intel_psr_single_frame_update(struct drm_device *dev)
+void intel_psr_single_frame_update(struct drm_device *dev,
+                                  unsigned frontbuffer_bits)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct drm_crtc *crtc;

@@ -624,14 +626,16 @@ void intel_psr_single_frame_update(struct drm_device *dev)
 
        crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
        pipe = to_intel_crtc(crtc)->pipe;
-       val = I915_READ(VLV_PSRCTL(pipe));
 
-       /*
-        * We need to set this bit before writing registers for a flip.
-        * This bit will be self-clear when it gets to the PSR active state.
-        */
-       I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+       if (frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)) {
+               val = I915_READ(VLV_PSRCTL(pipe));
 
+               /*
+                * We need to set this bit before writing registers for a flip.
+                * This bit will be self-clear when it gets to the PSR active state.
+                */
+               I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+       }
        mutex_unlock(&dev_priv->psr.lock);
 }
 

@@ -648,7 +652,7 @@ void intel_psr_single_frame_update(struct drm_device *dev)
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
  */
 void intel_psr_invalidate(struct drm_device *dev,
-                             unsigned frontbuffer_bits)
+                         unsigned frontbuffer_bits)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct drm_crtc *crtc;

@@ -685,7 +689,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
  */
 void intel_psr_flush(struct drm_device *dev,
-                        unsigned frontbuffer_bits)
+                    unsigned frontbuffer_bits)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct drm_crtc *crtc;
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Must have missed the transition.

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/i915/intel_frontbuffer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_frontbuffer.c +6 -7

index ede6ccc45375..0635ee004c29 100644
@@ -82,7 +82,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
                             enum fb_op_origin origin)
 {
        struct drm_device *dev = obj->base.dev;
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
 
        WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 

@@ -117,7 +117,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 void intel_frontbuffer_flush(struct drm_device *dev,
                             unsigned frontbuffer_bits)
 {
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
 
        /* Delay flushing when rings are still busy.*/
        mutex_lock(&dev_priv->fb_tracking.lock);

@@ -145,7 +145,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
                        bool retire)
 {
        struct drm_device *dev = obj->base.dev;
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
        unsigned frontbuffer_bits;
 
        WARN_ON(!mutex_is_locked(&dev->struct_mutex));

@@ -182,7 +182,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 void intel_frontbuffer_flip_prepare(struct drm_device *dev,
                                    unsigned frontbuffer_bits)
 {
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
 
        mutex_lock(&dev_priv->fb_tracking.lock);
        dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;

@@ -206,7 +206,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 void intel_frontbuffer_flip_complete(struct drm_device *dev,
                                     unsigned frontbuffer_bits)
 {
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
 
        mutex_lock(&dev_priv->fb_tracking.lock);
        /* Mask any cancelled flips. */

@@ -232,7 +232,7 @@ inline
 void intel_frontbuffer_flip(struct drm_device *dev,
                            unsigned frontbuffer_bits)
 {
-       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_private *dev_priv = to_i915(dev);
 
        mutex_lock(&dev_priv->fb_tracking.lock);
        dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
--

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

The current/old frontbuffer might still have gpu frontbuffer rendering pending. But once flipped it won't have the corresponding frontbuffer bits any more and hence the request retire function won't ever clear the corresponding busy bits. The async flip tracking (with the flip_prepare and flip_complete functions) already does this, but somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer with busy_bits since all our plane update code waits for previous rendering to complete before displaying a new buffer. Hence a new buffer will never be busy.

Reported-by Paulo Zanoni <[email protected]>

Cc Paulo Zanoni <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs. busy buffer tests.

---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +0 -3

index bcafefcf048b..b7c69460fb20 100644
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device

drivers/gpu/drm/i915/intel_drv.h +1 -4

 void intel_frontbuffer_flip(struct drm_device *dev,
-			    unsigned frontbuffer_bits)
-{
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
-}
+			    unsigned frontbuffer_bits);
 
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,

drivers/gpu/drm/i915/intel_frontbuffer.c +26 -0

index 57095f54c1f2..2a1611a7ce1d 100644
@@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
 	intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
+
+/**
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+inline
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
+	/* Remove stale busy bits due to the old buffer. */
+	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 38 lines · Daniel Vetter

The current/old frontbuffer might still have gpu frontbuffer rendering pending. But once flipped it won't have the corresponding frontbuffer bits any more and hence the request retire function won't ever clear the corresponding busy bits. The async flip tracking (with the flip_prepare and flip_complete functions) already does this, but somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer with busy_bits since all our plane update code waits for previous rendering to complete before displaying a new buffer. Hence a new buffer will never be busy.

Reported-by Paulo Zanoni <[email protected]>

Cc Paulo Zanoni <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs. busy buffer tests.

---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +0 -7

index bcafefcf048b..b7c69460fb20 100644
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called after scheduling a flip on @obj. This is for
- * synchronous plane updates which will happen on the next vblank and which will

drivers/gpu/drm/i915/intel_drv.h void intel_frontbuffer_flip_complete(struct drm_device *dev, · line 966

Quoted 6 lines

drivers/gpu/drm/i915/intel_drv.h +1 -5

- * not get delayed by pending gpu rendering.
- *
- * Can be called without any locks held.
- */
-static inline
+inline

inline?

The current/old frontbuffer might still have gpu frontbuffer rendering pending. But once flipped it won't have the corresponding frontbuffer bits any more and hence the request retire function won't ever clear the corresponding busy bits. The async flip tracking (with the flip_prepare and flip_complete functions) already does this, but somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer with busy_bits since all our plane update code waits for previous rendering to complete before displaying a new buffer. Hence a new buffer will never be busy.

v2: Drop the spurious inline Ville spotted.

Reported-by Paulo Zanoni <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs. busy buffer tests.

---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +2 -16

index bcafefcf048b..b7c69460fb20 100644
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
                                     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
                             unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called after scheduling a flip on @obj. This is for
- * synchronous plane updates which will happen on the next vblank and which will
- * not get delayed by pending gpu rendering.
- *
- * Can be called without any locks held.
- */
-static inline
+inline
 void intel_frontbuffer_flip(struct drm_device *dev,
-                           unsigned frontbuffer_bits)
-{
-       intel_frontbuffer_flush(dev, frontbuffer_bits);
-}
+                           unsigned frontbuffer_bits);
 
 unsigned int intel_fb_align_height(struct drm_device *dev,
                                   unsigned int height,

drivers/gpu/drm/i915/intel_frontbuffer.c +10 -0

index 57095f54c1f2..c3559fde25e5 100644
@@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
        intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
+
+/**
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *

drivers/gpu/drm/i915/intel_frontbuffer.c +6 -1

+       /* Remove stale busy bits due to the old buffer. */
+       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+       mutex_unlock(&dev_priv->fb_tracking.lock);
+
+       intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
--

2.1.0

2015-06-18 6:23 GMT-03:00 Daniel Vetter <[email protected]>:

Quoted 11 lines

The current/old frontbuffer might still have gpu frontbuffer rendering pending. But once flipped it won't have the corresponding frontbuffer bits any more and hence the request retire function won't ever clear the corresponding busy bits. The async flip tracking (with the flip_prepare and flip_complete functions) already does this, but somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer with busy_bits since all our plane update code waits for previous rendering to complete before displaying a new buffer. Hence a new buffer will never be busy.

Steps to reproduce:
- Boot your machine to the display manager
- Kill it, returning to fbcon
- Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they all fail because FBC never gets enabled
- Do any blt/render operation to the frontbuffer
- Now the tests will succeed

I also submitted a new test for this, that doesn't require the steps above, so we can also add:
Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy

More below:

Quoted 72 lines

v2: Drop the spurious inline Ville spotted.

Reported-by Paulo Zanoni <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs. busy buffer tests.

---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +2 -16

index bcafefcf048b..b7c69460fb20 100644
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
                                     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
                             unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called after scheduling a flip on @obj. This is for
- * synchronous plane updates which will happen on the next vblank and which will
- * not get delayed by pending gpu rendering.
- *
- * Can be called without any locks held.
- */
-static inline
+inline
 void intel_frontbuffer_flip(struct drm_device *dev,
-                           unsigned frontbuffer_bits)
-{
-       intel_frontbuffer_flush(dev, frontbuffer_bits);
-}
+                           unsigned frontbuffer_bits);
 
 unsigned int intel_fb_align_height(struct drm_device *dev,
                                   unsigned int height,

drivers/gpu/drm/i915/intel_frontbuffer.c +19 -0

index 57095f54c1f2..c3559fde25e5 100644
@@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
        intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
+
+/**
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+
+void intel_frontbuffer_flip(struct drm_device *dev,
+                           unsigned frontbuffer_bits)
+{
+       struct drm_i915_private *dev_priv = dev->dev_private;
+
+       mutex_lock(&dev_priv->fb_tracking.lock);

drivers/gpu/drm/i915/intel_frontbuffer.c void intel_frontbuffer_flip_complete(struct drm_device *dev, · line 292

Quoted 1 line

drivers/gpu/drm/i915/intel_frontbuffer.c +1 -0

+       dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;

I don't understand the purpose of the line above. Is this really necessary? It seems that flip_bits is only necessary for the asynchronous flip case, where someone schedules a flip then draws on the buffer, so we don't flush the buffer at flip_complete. We do unconditional flush at intel_frontbuffer_flip(), so it seems like we shouldn't be touching flip_bits here.

I tested both this version and the version without the flip_bits change, and both appear to solve the problem I can reproduce, so for both:

Tested-by Paulo Zanoni <[email protected]>

And, with the flip_bits problem explained or fixed:

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 9 lines

drivers/gpu/drm/i915/intel_frontbuffer.c +6 -1

+       /* Remove stale busy bits due to the old buffer. */
+       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+       mutex_unlock(&dev_priv->fb_tracking.lock);
+
+       intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
--

2.1.0

Signature · 1 line

Paulo Zanoni