<div dir="ltr">As Paulo told the part 2 of his proposal is still missing and we will get the WARN of reading fbc register at fbc_enabled when on runtime_suspend...<div><br></div><div>But I couldn't find the propper place to check for intel_crtc->active. This was another issue reported by power team that on screen off there was to much activity on our driver. So we need to check it more carefully the proper place to avoid all unecessary inactivities with display off.</div><div><br></div><div>Do you have any suggestion Daniel?</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 5, 2014 at 12:35 PM, Rodrigo Vivi <span dir="ltr"><<a href="mailto:rodrigo.vivi@gmail.com" target="_blank">rodrigo.vivi@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><span class="">On Fri, Sep 5, 2014 at 11:28 AM, Paulo Zanoni <span dir="ltr"><<a href="mailto:przanoni@gmail.com" target="_blank">przanoni@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">2014-08-04 7:51 GMT-03:00 Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com" target="_blank">rodrigo.vivi@intel.com</a>>:<br>
<span>> According to spec FBC on BDW and HSW are identical without any gaps.<br>
> So let's copy the nuke and let FBC really start compressing stuff.<br>
><br>
> Without this patch we can verify with false color that nothing is being<br>
> compressed. With the nuke in place and false color it is possible<br>
> to see false color debugs.<br>
><br>
> Unfortunatelly on some rings like BCS on BDW we have to avoid Bits 22:18 on<br>
> LRIs due to a high risk of hung. So, when using Blt ring for frontbuffer rend<br>
> cache would never been cleaned and FBC would stop compressing buffer.<br>
> One alternative is to cache clean on software frontbuffer tracking.<br>
><br>
> v2: Fix rebase conflict.<br>
> v3: Do not clean cache on BCS ring. Instead use sw frontbuffer tracking.<br>
<br>
</span>This patch causes lots of WARNs when running igt/pm_rpm/cursor and a<br>
few other similar test cases. Now the interesting this is that we're<br>
trying to do FBC flushing, but:<br></blockquote><div><br></div></span><div>As I told you on the IRC already, thank you for finding this. It is good to get reasons and arguments.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- FBC is disabled by default</blockquote><div><br></div></span><div>Yeah, we need to fix it although it just matter much </div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- The screen is disabled</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- We're runtime suspended<br></blockquote><div><br></div></span><div>These are another error. Not caused by this patch.</div><div>Why frontbuffer_flush is being called with screen disabled or on runtime suspend?</div><span class=""><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- We're touching cursors when we decide to flush FBC.<br></blockquote><div><br></div></span><div>This is not actually a flush. But cleaning the cache to allow compression restart.</div><div>I believe we could have some variable to indicate when we touched ring flush than on next frontbuffer we do the mmio cache clean.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
This is the first WARN that happens:<br>
<br>
[ 123.581179] [drm:intel_runtime_suspend] Device suspended<br>
[ 123.680666] ------------[ cut here ]------------<br>
[ 123.680697] WARNING: CPU: 1 PID: 1776 at<br>
drivers/gpu/drm/i915/intel_uncore.c:47<br>
assert_device_not_suspended.isra.8+0x43/0x50 [i915]()<br>
[ 123.680699] Device suspended<br>
[ 123.680700] Modules linked in: intel_rapl x86_pkg_temp_thermal<br>
serio_raw intel_powerclamp efivars btusb iwlmvm iwlwifi mei_me mei<br>
int3403_thermal snd_hda_codec_hdmi snd_hda_intel snd_hda_controller<br>
i2c_designware_platform snd_hda_codec i2c_designware_core snd_hwdep<br>
snd_pcm_oss snd_mixer_oss acpi_pad snd_pcm snd_timer fuse nls_utf8<br>
nls_cp437 vfat fat sd_mod ahci libahci i915 sdhci_pci e1000e<br>
drm_kms_helper drm sdhci_acpi sdhci<br>
[ 123.680733] CPU: 1 PID: 1776 Comm: pm_rpm Not tainted<br>
3.17.0-rc2.1409051503pz+ #1100<br>
[ 123.680736] Hardware name: Intel Corporation Broadwell Client<br>
platform/Wilson Beach SDS, BIOS BDW-E2R1.86C.0072.R03.1405072127<br>
05/07/2014<br>
[ 123.680738] 0000000000000009 ffff88024174fab8 ffffffff816f6d23<br>
ffff88024174fb00<br>
[ 123.680742] ffff88024174faf0 ffffffff8107b368 ffff880037700000<br>
0000000000050380<br>
[ 123.680746] 0000000000050380 ffff880037700068 0000000000000001<br>
ffff88024174fb50<br>
[ 123.680751] Call Trace:<br>
[ 123.680758] [<ffffffff816f6d23>] dump_stack+0x4d/0x66<br>
[ 123.680763] [<ffffffff8107b368>] warn_slowpath_common+0x78/0xa0<br>
[ 123.680766] [<ffffffff8107b3d7>] warn_slowpath_fmt+0x47/0x50<br>
[ 123.680772] [<ffffffff810c0efd>] ? trace_hardirqs_on_caller+0x15d/0x200<br>
[ 123.680791] [<ffffffffa0118c63>]<br>
assert_device_not_suspended.isra.8+0x43/0x50 [i915]<br>
[ 123.680809] [<ffffffffa011cbd5>] gen8_write32+0x35/0x180 [i915]<br>
[ 123.680821] [<ffffffffa00dbb49>] gen8_fbc_sw_flush+0x29/0x30 [i915]<br>
[ 123.680840] [<ffffffffa013150d>] intel_frontbuffer_flush+0x7d/0x90 [i915]<br>
[ 123.680859] [<ffffffffa0135f6b>]<br>
intel_cursor_plane_update+0x13b/0x150 [i915]<br>
[ 123.680876] [<ffffffffa00276b0>] setplane_internal+0x260/0x2a0 [drm]<br>
[ 123.680889] [<ffffffffa002780e>] drm_mode_cursor_common+0x11e/0x310 [drm]<br>
[ 123.680904] [<ffffffffa002ae1c>] drm_mode_cursor_ioctl+0x3c/0x40 [drm]<br>
[ 123.680914] [<ffffffffa001cc9f>] drm_ioctl+0x1df/0x6a0 [drm]<br>
[ 123.680920] [<ffffffff816fe769>] ? mutex_unlock+0x9/0x10<br>
[ 123.680924] [<ffffffff811f3856>] ? seq_read+0xb6/0x3e0<br>
[ 123.680929] [<ffffffff811e2540>] do_vfs_ioctl+0x2e0/0x4e0<br>
[ 123.680934] [<ffffffff81701077>] ? sysret_check+0x1b/0x56<br>
[ 123.680939] [<ffffffff810c0efd>] ? trace_hardirqs_on_caller+0x15d/0x200<br>
[ 123.680943] [<ffffffff811e27c1>] SyS_ioctl+0x81/0xa0<br>
[ 123.680947] [<ffffffff81701052>] system_call_fastpath+0x16/0x1b<br>
[ 123.680949] ---[ end trace 9f389682639d2eb9 ]---<br>
[ 123.680953] ------------[ cut here ]------------<br>
<br>
<br>
So, what we can/could do:<br>
1 - Change gen8_fbc_sw_flush() do to nothing when FBC is disabled.<br></blockquote><div><br></div></div></div><div>Agreed!</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
2 - At some point of the stack above, avoid calling everything else if<br>
the screen and/or plane and/or crtc is disabled. But when? At<br>
intel_cursor_plane_update? At intel_frontbuffer_flush?<br></blockquote><div><br></div></span><div>Good question!</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
3 - Don't call gen8_fbc_sw_flush if the frontbuffer_bits don't include<br>
the primary plane bits (since, AFAIR, FBC ignores the sprite and<br>
cursors, right?)<br></blockquote><div><br></div></span><div>not sure. but we could try check primary bits + do only when we did a ring flush on blt ring. Actually other ring than render.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Is anybody against any of the 3 items above?<br></blockquote><div><br></div></span><div>should I change this patch itself or provide another one on top?</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Also notice that the "cursor" subtest is not the only one that causes<br>
these WARNs, so I expect similar backtraces with other similar<br>
solutions for equivalent problems with the other planes.<br></blockquote><div><br></div></span><div>so check for primary bit is the way!</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks,<br>
Paulo<br></blockquote><div><br></div><div>Thank you!</div><span class="HOEnZb"><font color="#888888"><div>Rodrigo.</div></font></span><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
><br>
> Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com" target="_blank">rodrigo.vivi@intel.com</a>><br>
> ---<br>
> drivers/gpu/drm/i915/i915_drv.h | 1 +<br>
> drivers/gpu/drm/i915/intel_display.c | 3 +++<br>
> drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++<br>
> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++++++-<br>
> 4 files changed, 23 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h<br>
> index 2a372f2..25d7365 100644<br>
> --- a/drivers/gpu/drm/i915/i915_drv.h<br>
> +++ b/drivers/gpu/drm/i915/i915_drv.h<br>
> @@ -2713,6 +2713,7 @@ extern void intel_modeset_setup_hw_state(struct drm_device *dev,<br>
> extern void i915_redisable_vga(struct drm_device *dev);<br>
> extern void i915_redisable_vga_power_on(struct drm_device *dev);<br>
> extern bool intel_fbc_enabled(struct drm_device *dev);<br>
> +extern void gen8_fbc_sw_flush(struct drm_device *dev, u32 value);<br>
> extern void intel_disable_fbc(struct drm_device *dev);<br>
> extern bool ironlake_set_drps(struct drm_device *dev, u8 val);<br>
> extern void intel_init_pch_refclk(struct drm_device *dev);<br>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c<br>
> index 883af0b..c8421cd 100644<br>
> --- a/drivers/gpu/drm/i915/intel_display.c<br>
> +++ b/drivers/gpu/drm/i915/intel_display.c<br>
> @@ -9044,6 +9044,9 @@ void intel_frontbuffer_flush(struct drm_device *dev,<br>
> intel_mark_fb_busy(dev, frontbuffer_bits, NULL);<br>
><br>
> intel_edp_psr_flush(dev, frontbuffer_bits);<br>
> +<br>
> + if (IS_GEN8(dev))<br>
> + gen8_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);<br>
> }<br>
><br>
> /**<br>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c<br>
> index 684dc5f..de07d3e 100644<br>
> --- a/drivers/gpu/drm/i915/intel_pm.c<br>
> +++ b/drivers/gpu/drm/i915/intel_pm.c<br>
> @@ -345,6 +345,16 @@ bool intel_fbc_enabled(struct drm_device *dev)<br>
> return dev_priv->display.fbc_enabled(dev);<br>
> }<br>
><br>
> +void gen8_fbc_sw_flush(struct drm_device *dev, u32 value)<br>
> +{<br>
> + struct drm_i915_private *dev_priv = dev->dev_private;<br>
> +<br>
> + if (!IS_GEN8(dev))<br>
> + return;<br>
> +<br>
> + I915_WRITE(MSG_FBC_REND_STATE, value);<br>
> +}<br>
> +<br>
> static void intel_fbc_work_fn(struct work_struct *__work)<br>
> {<br>
> struct intel_fbc_work *work =<br>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c<br>
> index 2908896..2fe871c 100644<br>
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c<br>
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c<br>
> @@ -406,6 +406,7 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,<br>
> {<br>
> u32 flags = 0;<br>
> u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;<br>
> + int ret;<br>
><br>
> flags |= PIPE_CONTROL_CS_STALL;<br>
><br>
> @@ -424,7 +425,14 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,<br>
> flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;<br>
> }<br>
><br>
> - return gen8_emit_pipe_control(ring, flags, scratch_addr);<br>
> + ret = gen8_emit_pipe_control(ring, flags, scratch_addr);<br>
> + if (ret)<br>
> + return ret;<br>
> +<br>
> + if (!invalidate_domains && flush_domains)<br>
> + return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);<br>
> +<br>
> + return 0;<br>
> }<br>
><br>
> static void ring_write_tail(struct intel_engine_cs *ring,<br>
> --<br>
> 1.9.3<br>
><br>
> _______________________________________________<br>
> Intel-gfx mailing list<br>
> <a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
<br>
<br>
<br>
</div></div><span><font color="#888888">--<br>
Paulo Zanoni<br>
</font></span><div><div>_______________________________________________<br>
Intel-gfx mailing list<br>
<a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
</div></div></blockquote></div></div></div><br><br clear="all"><span class=""><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div>