| Message ID | 1458588418-31129-5-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paulo, [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on next-20160321] [cannot apply to v4.5] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Paulo-Zanoni/Enable-FBC-on-SKL/20160322-045829 base: git://anongit.freedesktop.org/drm-intel for-linux-next reproduce: make htmldocs All warnings (new ones prefixed by >>): include/drm/drm_crtc.h:776: warning: No description found for parameter 'name' include/drm/drm_crtc.h:1235: warning: No description found for parameter 'connector_id' include/drm/drm_crtc.h:1235: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:1274: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:1536: warning: No description found for parameter 'name' include/drm/drm_crtc.h:1536: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:1536: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'connector_ida' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_mode_id' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:2146: warning: No description found for parameter 'allow_fb_modifiers' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' >> drivers/gpu/drm/i915/intel_frontbuffer.c:261: warning: No description found for parameter 'flags' >> drivers/gpu/drm/i915/intel_frontbuffer.c:261: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1461: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:2019: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' drivers/gpu/drm/i915/i915_gem.c:2919: warning: No description found for parameter 'ring' drivers/gpu/drm/i915/i915_gem.c:3050: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:3100: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3727: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3727: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3802: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3802: warning: No description found for parameter 'cache_level' drivers/gpu/drm/i915/i915_gem.c:4076: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:4076: warning: No description found for parameter 'write' drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' Warning: didn't use docs for i915_hotplug_interrupt_update Warning: didn't use docs for ilk_update_display_irq Warning: didn't use docs for ilk_update_gt_irq Warning: didn't use docs for snb_update_pm_irq Warning: didn't use docs for bdw_update_port_irq Warning: didn't use docs for bdw_update_pipe_irq Warning: didn't use docs for ibx_display_interrupt_update Warning: didn't use docs for i915_enable_asle_pipestat Warning: didn't use docs for ivybridge_parity_work Warning: didn't use docs for i915_reset_and_wakeup Warning: didn't use docs for i915_handle_error Warning: didn't use docs for intel_irq_install Warning: didn't use docs for intel_irq_uninstall vim +/flags +261 drivers/gpu/drm/i915/intel_frontbuffer.c 245 /** 246 * intel_fb_obj_mmap_wa - notifies about objects being mmapped 247 * @obj: GEM object being mmapped 248 * 249 * This function gets called whenever an object gets mmapped. Not every user 250 * space application follows the protocol assumed by the frontbuffer tracking 251 * subsystem when it was created, so this mmap notify callback can be used to 252 * completely disable frontbuffer features such as FBC and PSR. Even if at some 253 * point we fix ever user space application, there's still the possibility that 254 * the user may have a new Kernel with the old user space. 255 * 256 * Also notice that there's no munmap API because user space calls munmap() 257 * directly. Even if we had, it probably wouldn't help since munmap() calls are 258 * not common. 259 */ 260 void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags) > 261 { 262 struct drm_i915_private *dev_priv = obj->base.dev->dev_private; 263 264 if (!IS_GEN9(dev_priv)) 265 return; 266 267 obj->fb_mmap_wa_flags |= flags; 268 269 if (!obj->frontbuffer_bits) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > +enum fb_mmap_wa_flags { > + FB_MMAP_WA_CPU = 1 << 0, > + FB_MMAP_WA_GTT = 1 << 1, > + FB_MMAP_WA_DISABLE = 1 << 2, > + FB_MMAP_WA_FLAG_COUNT = 3, > +}; Drive-by review, adding bit flags as enums doesn't feel like what enums should be used for. I'd go for macros instead. BR, Jani.
On Tue, Mar 22, 2016 at 12:28:20PM +0200, Jani Nikula wrote: > On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > > +enum fb_mmap_wa_flags { > > + FB_MMAP_WA_CPU = 1 << 0, > > + FB_MMAP_WA_GTT = 1 << 1, > > + FB_MMAP_WA_DISABLE = 1 << 2, > > + FB_MMAP_WA_FLAG_COUNT = 3, > > +}; > > Drive-by review, adding bit flags as enums doesn't feel like what enums > should be used for. I'd go for macros instead. Pretty established convention, some like it since it allows gdb&friends to decode your bitflags for you. So abusing enums as flag set is imo ok. -Daniel
On Mon, Mar 21, 2016 at 04:26:57PM -0300, Paulo Zanoni wrote: > FBC and the frontbuffer tracking infrastructure were designed assuming > that user space applications would follow a specific set of rules > regarding frontbuffer management and mmapping. I recently discovered > that current user space is not exactly following these rules: my > investigation led me to the conclusion that the generic backend from > SNA - used by SKL and the other new platforms without a specific > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the CPU > and WC mmaps. I discovered this when running lightdm: I would type the > password and nothing would appear on the screen unless I moved the > mouse over the place where the letters were supposed to appear. > > Since we can't detect whether the current DRM master is a well-behaved > application or not, let's use the sledgehammer approach and assume > that every user space client on every gen is bad by disabling FBC when > the frontbuffer is CPU/WC mmapped. This will allow us to enable FBC > on SKL, moving its FBC-related power savings from "always 0%" to "> 0% > in some cases". > > There's also some additional code to disable the workaround for > frontbuffers that ever received a sw_finish or dirty_fb IOCTL, since > the current bad user space never issues those calls. This should help > for some specific cases and our IGT tests, but won't be enough for a > new xf86-video-intel using TearFree. > > Notice that we may need an equivalent commit for PSR too. We also need > to investigate whether PSR needs to be disabled on GTT mmaps: if > that's the case, we'll have problems since we seem to always have GTT > mmaps on our current user space, so we would end up keeping PSR > disabled forever. > > v2: > - Rename some things. > - Disable the WA on sw_finish/dirty_fb (Daniel). > - Fix NULL obj checking. > - Restric to Gen 9. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++------- > drivers/gpu/drm/i915/intel_display.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_fbc.c | 33 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_frontbuffer.c | 31 ++++++++++++++++++++++++++++++ > 6 files changed, 89 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index efca534..45e31d2 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -873,6 +873,13 @@ enum fb_op_origin { > ORIGIN_DIRTYFB, > }; > > +enum fb_mmap_wa_flags { > + FB_MMAP_WA_CPU = 1 << 0, > + FB_MMAP_WA_GTT = 1 << 1, > + FB_MMAP_WA_DISABLE = 1 << 2, > + FB_MMAP_WA_FLAG_COUNT = 3, > +}; > + > struct intel_fbc { > /* This is always the inner lock when overlapping with struct_mutex and > * it's the outer lock when overlapping with stolen_lock. */ > @@ -910,6 +917,7 @@ struct intel_fbc { > unsigned int stride; > int fence_reg; > unsigned int tiling_mode; > + unsigned int mmap_wa_flags; > } fb; > } state_cache; > > @@ -2143,6 +2151,7 @@ struct drm_i915_gem_object { > unsigned int cache_dirty:1; > > unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; > + unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT; > > unsigned int pin_display; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8588c83..d44f27e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > + > /* Pinned buffers may be scanout, so flush the cache */ > if (obj->pin_display) > i915_gem_object_flush_cpu_write_domain(obj); > @@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct drm_file *file) > { > struct drm_i915_gem_mmap *args = data; > - struct drm_gem_object *obj; > + struct drm_i915_gem_object *obj; > unsigned long addr; > > if (args->flags & ~(I915_MMAP_WC)) > @@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > if (args->flags & I915_MMAP_WC && !cpu_has_pat) > return -ENODEV; > > - obj = drm_gem_object_lookup(dev, file, args->handle); > - if (obj == NULL) > + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > + if (&obj->base == NULL) > return -ENOENT; > > /* prime objects have no backing filp to GEM mmap > * pages from. > */ > - if (!obj->filp) { > - drm_gem_object_unreference_unlocked(obj); > + if (!obj->base.filp) { > + drm_gem_object_unreference_unlocked(&obj->base); > return -EINVAL; > } > > - addr = vm_mmap(obj->filp, 0, args->size, > + addr = vm_mmap(obj->base.filp, 0, args->size, > PROT_READ | PROT_WRITE, MAP_SHARED, > args->offset); > if (args->flags & I915_MMAP_WC) { > @@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > addr = -ENOMEM; > up_write(&mm->mmap_sem); > } > - drm_gem_object_unreference_unlocked(obj); > + drm_gem_object_unreference_unlocked(&obj->base); > if (IS_ERR((void *)addr)) > return addr; > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU); > + > args->addr_ptr = (uint64_t) addr; > > return 0; > @@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file, > goto out; > > *offset = drm_vma_node_offset_addr(&obj->base.vma_node); > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT); > > out: > drm_gem_object_unreference(&obj->base); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 28ead66..6e7aa9e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14705,6 +14705,7 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > struct drm_i915_gem_object *obj = intel_fb->obj; > > mutex_lock(&dev->struct_mutex); > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB); > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ba45245..bbea7c4 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1082,6 +1082,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev, > unsigned frontbuffer_bits); > void intel_frontbuffer_flip(struct drm_device *dev, > unsigned frontbuffer_bits); > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags); > unsigned int intel_fb_align_height(struct drm_device *dev, > unsigned int height, > uint32_t pixel_format, > @@ -1371,6 +1372,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > enum fb_op_origin origin); > void intel_fbc_flush(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits, enum fb_op_origin origin); > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > + unsigned int frontbuffer_bits, unsigned int flags); > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > > /* intel_hdmi.c */ > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 7101880..718ac38 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -745,6 +745,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) > cache->fb.stride = fb->pitches[0]; > cache->fb.fence_reg = obj->fence_reg; > cache->fb.tiling_mode = obj->tiling_mode; > + cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags; > +} > + > +static bool need_mmap_disable_workaround(struct intel_fbc *fbc) > +{ > + unsigned int flags = fbc->state_cache.fb.mmap_wa_flags; > + > + return (flags & FB_MMAP_WA_CPU) && !(flags & FB_MMAP_WA_DISABLE); > } > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > @@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > + if (need_mmap_disable_workaround(fbc)) { > + fbc->no_fbc_reason = "FB is CPU or WC mmapped"; > + return false; > + } > + > return true; > } > > @@ -1008,6 +1021,26 @@ out: > mutex_unlock(&fbc->lock); > } > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > + unsigned int frontbuffer_bits, unsigned int flags) > +{ > + struct intel_fbc *fbc = &dev_priv->fbc; > + > + if (!fbc_supported(dev_priv)) > + return; > + > + mutex_lock(&fbc->lock); > + > + if (fbc->enabled && > + (intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits)) { > + fbc->state_cache.fb.mmap_wa_flags = flags; > + if (need_mmap_disable_workaround(fbc)) > + intel_fbc_deactivate(dev_priv); > + } > + > + mutex_unlock(&fbc->lock); > +} > + > /** > * intel_fbc_choose_crtc - select a CRTC to enable FBC on > * @dev_priv: i915 device instance > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c > index ac85357..8d9b327 100644 > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > @@ -241,3 +241,34 @@ void intel_frontbuffer_flip(struct drm_device *dev, > > intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP); > } > + > +/** > + * intel_fb_obj_mmap_wa - notifies about objects being mmapped > + * @obj: GEM object being mmapped > + * > + * This function gets called whenever an object gets mmapped. Not every user > + * space application follows the protocol assumed by the frontbuffer tracking > + * subsystem when it was created, so this mmap notify callback can be used to > + * completely disable frontbuffer features such as FBC and PSR. Even if at some > + * point we fix ever user space application, there's still the possibility that > + * the user may have a new Kernel with the old user space. > + * > + * Also notice that there's no munmap API because user space calls munmap() > + * directly. Even if we had, it probably wouldn't help since munmap() calls are > + * not common. > + */ > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags) > +{ > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > + > + if (!IS_GEN9(dev_priv)) Please only IS_SKYLAKE, I don't want to extend this to either bxt or kbl, and both are still under prelim_hw_support and not yet shipping, i.e. we can break them ;-) Wrt the implementation: I think it would be great to get PSR on board to avoid duplicating the logic. One option that might would be to create an invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect (until the first sw_finish or dirty_fb) the frontbuffer tracking code itself would filter all flushes. fbc could then use ORIGIN_MMAP_WA invalidate to permanently disable (well, not re-enable after flip) until it sees a flush. And PSR would Just Work (I hope). Cheers, Daniel > + return; > + > + obj->fb_mmap_wa_flags |= flags; > + > + if (!obj->frontbuffer_bits) > + return; > + > + intel_fbc_mmap_wa(dev_priv, obj->frontbuffer_bits, > + obj->fb_mmap_wa_flags); > +} > -- > 2.7.0 >
On Tue, 22 Mar 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > [ text/plain ] > On Tue, Mar 22, 2016 at 12:28:20PM +0200, Jani Nikula wrote: >> On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: >> > +enum fb_mmap_wa_flags { >> > + FB_MMAP_WA_CPU = 1 << 0, >> > + FB_MMAP_WA_GTT = 1 << 1, >> > + FB_MMAP_WA_DISABLE = 1 << 2, >> > + FB_MMAP_WA_FLAG_COUNT = 3, >> > +}; >> >> Drive-by review, adding bit flags as enums doesn't feel like what enums >> should be used for. I'd go for macros instead. > > Pretty established convention, some like it since it allows gdb&friends to > decode your bitflags for you. So abusing enums as flag set is imo ok. Bah, prevents GCC and friends from giving you sensible warnings on switch cases etc. :p BR, Jani.
Em Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu: > On Mon, Mar 21, 2016 at 04:26:57PM -0300, Paulo Zanoni wrote: > > > > FBC and the frontbuffer tracking infrastructure were designed > > assuming > > that user space applications would follow a specific set of rules > > regarding frontbuffer management and mmapping. I recently > > discovered > > that current user space is not exactly following these rules: my > > investigation led me to the conclusion that the generic backend > > from > > SNA - used by SKL and the other new platforms without a specific > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the > > CPU > > and WC mmaps. I discovered this when running lightdm: I would type > > the > > password and nothing would appear on the screen unless I moved the > > mouse over the place where the letters were supposed to appear. > > > > Since we can't detect whether the current DRM master is a well- > > behaved > > application or not, let's use the sledgehammer approach and assume > > that every user space client on every gen is bad by disabling FBC > > when > > the frontbuffer is CPU/WC mmapped. This will allow us to enable > > FBC > > on SKL, moving its FBC-related power savings from "always 0%" to "> > > 0% > > in some cases". > > > > There's also some additional code to disable the workaround for > > frontbuffers that ever received a sw_finish or dirty_fb IOCTL, > > since > > the current bad user space never issues those calls. This should > > help > > for some specific cases and our IGT tests, but won't be enough for > > a > > new xf86-video-intel using TearFree. > > > > Notice that we may need an equivalent commit for PSR too. We also > > need > > to investigate whether PSR needs to be disabled on GTT mmaps: if > > that's the case, we'll have problems since we seem to always have > > GTT > > mmaps on our current user space, so we would end up keeping PSR > > disabled forever. > > > > v2: > > - Rename some things. > > - Disable the WA on sw_finish/dirty_fb (Daniel). > > - Fix NULL obj checking. > > - Restric to Gen 9. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > > drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++------- > > drivers/gpu/drm/i915/intel_display.c | 1 + > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > drivers/gpu/drm/i915/intel_fbc.c | 33 > > ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_frontbuffer.c | 31 > > ++++++++++++++++++++++++++++++ > > 6 files changed, 89 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index efca534..45e31d2 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -873,6 +873,13 @@ enum fb_op_origin { > > ORIGIN_DIRTYFB, > > }; > > > > +enum fb_mmap_wa_flags { > > + FB_MMAP_WA_CPU = 1 << 0, > > + FB_MMAP_WA_GTT = 1 << 1, > > + FB_MMAP_WA_DISABLE = 1 << 2, > > + FB_MMAP_WA_FLAG_COUNT = 3, > > +}; > > + I'll do the change to macros you/Jani mentioned in the other emails. > > struct intel_fbc { > > /* This is always the inner lock when overlapping with > > struct_mutex and > > * it's the outer lock when overlapping with stolen_lock. > > */ > > @@ -910,6 +917,7 @@ struct intel_fbc { > > unsigned int stride; > > int fence_reg; > > unsigned int tiling_mode; > > + unsigned int mmap_wa_flags; > > } fb; > > } state_cache; > > > > @@ -2143,6 +2151,7 @@ struct drm_i915_gem_object { > > unsigned int cache_dirty:1; > > > > unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; > > + unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT; > > > > unsigned int pin_display; > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index 8588c83..d44f27e 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct drm_device > > *dev, void *data, > > goto unlock; > > } > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > + > > /* Pinned buffers may be scanout, so flush the cache */ > > if (obj->pin_display) > > i915_gem_object_flush_cpu_write_domain(obj); > > @@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > void *data, > > struct drm_file *file) > > { > > struct drm_i915_gem_mmap *args = data; > > - struct drm_gem_object *obj; > > + struct drm_i915_gem_object *obj; > > unsigned long addr; > > > > if (args->flags & ~(I915_MMAP_WC)) > > @@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > void *data, > > if (args->flags & I915_MMAP_WC && !cpu_has_pat) > > return -ENODEV; > > > > - obj = drm_gem_object_lookup(dev, file, args->handle); > > - if (obj == NULL) > > + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args- > > >handle)); > > + if (&obj->base == NULL) > > return -ENOENT; > > > > /* prime objects have no backing filp to GEM mmap > > * pages from. > > */ > > - if (!obj->filp) { > > - drm_gem_object_unreference_unlocked(obj); > > + if (!obj->base.filp) { > > + drm_gem_object_unreference_unlocked(&obj->base); > > return -EINVAL; > > } > > > > - addr = vm_mmap(obj->filp, 0, args->size, > > + addr = vm_mmap(obj->base.filp, 0, args->size, > > PROT_READ | PROT_WRITE, MAP_SHARED, > > args->offset); > > if (args->flags & I915_MMAP_WC) { > > @@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > void *data, > > addr = -ENOMEM; > > up_write(&mm->mmap_sem); > > } > > - drm_gem_object_unreference_unlocked(obj); > > + drm_gem_object_unreference_unlocked(&obj->base); > > if (IS_ERR((void *)addr)) > > return addr; > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU); > > + > > args->addr_ptr = (uint64_t) addr; > > > > return 0; > > @@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file, > > goto out; > > > > *offset = drm_vma_node_offset_addr(&obj->base.vma_node); > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT); > > > > out: > > drm_gem_object_unreference(&obj->base); > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 28ead66..6e7aa9e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14705,6 +14705,7 @@ static int > > intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > > struct drm_i915_gem_object *obj = intel_fb->obj; > > > > mutex_lock(&dev->struct_mutex); > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB); > > mutex_unlock(&dev->struct_mutex); > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index ba45245..bbea7c4 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1082,6 +1082,7 @@ void intel_frontbuffer_flip_complete(struct > > drm_device *dev, > > unsigned frontbuffer_bits); > > void intel_frontbuffer_flip(struct drm_device *dev, > > unsigned frontbuffer_bits); > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > unsigned int flags); > > unsigned int intel_fb_align_height(struct drm_device *dev, > > unsigned int height, > > uint32_t pixel_format, > > @@ -1371,6 +1372,8 @@ void intel_fbc_invalidate(struct > > drm_i915_private *dev_priv, > > enum fb_op_origin origin); > > void intel_fbc_flush(struct drm_i915_private *dev_priv, > > unsigned int frontbuffer_bits, enum > > fb_op_origin origin); > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > + unsigned int frontbuffer_bits, unsigned int > > flags); > > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > > > > /* intel_hdmi.c */ > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index 7101880..718ac38 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -745,6 +745,14 @@ static void > > intel_fbc_update_state_cache(struct intel_crtc *crtc) > > cache->fb.stride = fb->pitches[0]; > > cache->fb.fence_reg = obj->fence_reg; > > cache->fb.tiling_mode = obj->tiling_mode; > > + cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags; > > +} > > + > > +static bool need_mmap_disable_workaround(struct intel_fbc *fbc) > > +{ > > + unsigned int flags = fbc->state_cache.fb.mmap_wa_flags; > > + > > + return (flags & FB_MMAP_WA_CPU) && !(flags & > > FB_MMAP_WA_DISABLE); > > } > > > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > @@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct > > intel_crtc *crtc) > > return false; > > } > > > > + if (need_mmap_disable_workaround(fbc)) { > > + fbc->no_fbc_reason = "FB is CPU or WC mmapped"; > > + return false; > > + } > > + > > return true; > > } > > > > @@ -1008,6 +1021,26 @@ out: > > mutex_unlock(&fbc->lock); > > } > > > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > + unsigned int frontbuffer_bits, unsigned int > > flags) > > +{ > > + struct intel_fbc *fbc = &dev_priv->fbc; > > + > > + if (!fbc_supported(dev_priv)) > > + return; > > + > > + mutex_lock(&fbc->lock); > > + > > + if (fbc->enabled && > > + (intel_fbc_get_frontbuffer_bit(fbc) & > > frontbuffer_bits)) { > > + fbc->state_cache.fb.mmap_wa_flags = flags; > > + if (need_mmap_disable_workaround(fbc)) > > + intel_fbc_deactivate(dev_priv); > > + } > > + > > + mutex_unlock(&fbc->lock); > > +} > > + > > /** > > * intel_fbc_choose_crtc - select a CRTC to enable FBC on > > * @dev_priv: i915 device instance > > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c > > b/drivers/gpu/drm/i915/intel_frontbuffer.c > > index ac85357..8d9b327 100644 > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > > @@ -241,3 +241,34 @@ void intel_frontbuffer_flip(struct drm_device > > *dev, > > > > intel_frontbuffer_flush(dev, frontbuffer_bits, > > ORIGIN_FLIP); > > } > > + > > +/** > > + * intel_fb_obj_mmap_wa - notifies about objects being mmapped > > + * @obj: GEM object being mmapped > > + * > > + * This function gets called whenever an object gets mmapped. Not > > every user > > + * space application follows the protocol assumed by the > > frontbuffer tracking > > + * subsystem when it was created, so this mmap notify callback can > > be used to > > + * completely disable frontbuffer features such as FBC and PSR. > > Even if at some > > + * point we fix ever user space application, there's still the > > possibility that > > + * the user may have a new Kernel with the old user space. > > + * > > + * Also notice that there's no munmap API because user space calls > > munmap() > > + * directly. Even if we had, it probably wouldn't help since > > munmap() calls are > > + * not common. > > + */ > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > unsigned int flags) > > +{ > > + struct drm_i915_private *dev_priv = obj->base.dev- > > >dev_private; > > + > > + if (!IS_GEN9(dev_priv)) > Please only IS_SKYLAKE, I don't want to extend this to either bxt or > kbl, > and both are still under prelim_hw_support and not yet shipping, i.e. > we > can break them ;-) Mmmkay. > > Wrt the implementation: I think it would be great to get PSR on board > to > avoid duplicating the logic. The goal of this patch going through intel_frontbuffer.c instead of directly to intel_fbc.c is exactly to allow for PSR to do something similar. > One option that might would be to create an > invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect > (until the first sw_finish or dirty_fb) the frontbuffer tracking code > itself would filter all flushes. That's an non-trivial distortion of the trivial invalidate/flush API. I prefer having a special codepath for this special case instead of adding second meanings to flush/invalidate. Besides, even with the invalidate/flush API being simple, look at how many times we already tweaked/broke/fixed those functions, both for PSR and FBC. > fbc could then use ORIGIN_MMAP_WA > invalidate to permanently disable (well, not re-enable after flip) > until > it sees a flush. And PSR would Just Work (I hope). I think any PSR fixes related to this should be separate patches. I have to confess I've just been studying the FBC part of the problem, so I was waiting for Rodrigo's opinions here before any conclusions. Anyway, we can always change this code (in this patch or later) to better suit PSR. > > Cheers, Daniel > > > > > + return; > > + > > + obj->fb_mmap_wa_flags |= flags; > > + > > + if (!obj->frontbuffer_bits) > > + return; > > + > > + intel_fbc_mmap_wa(dev_priv, obj->frontbuffer_bits, > > + obj->fb_mmap_wa_flags); > > +} > > > > > -- > > 2.7.0 > >
On Tue, Mar 22, 2016 at 09:48:00PM +0000, Zanoni, Paulo R wrote: > Em Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu: > > On Mon, Mar 21, 2016 at 04:26:57PM -0300, Paulo Zanoni wrote: > > > > > > FBC and the frontbuffer tracking infrastructure were designed > > > assuming > > > that user space applications would follow a specific set of rules > > > regarding frontbuffer management and mmapping. I recently > > > discovered > > > that current user space is not exactly following these rules: my > > > investigation led me to the conclusion that the generic backend > > > from > > > SNA - used by SKL and the other new platforms without a specific > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the > > > CPU > > > and WC mmaps. I discovered this when running lightdm: I would type > > > the > > > password and nothing would appear on the screen unless I moved the > > > mouse over the place where the letters were supposed to appear. > > > > > > Since we can't detect whether the current DRM master is a well- > > > behaved > > > application or not, let's use the sledgehammer approach and assume > > > that every user space client on every gen is bad by disabling FBC > > > when > > > the frontbuffer is CPU/WC mmapped. This will allow us to enable > > > FBC > > > on SKL, moving its FBC-related power savings from "always 0%" to "> > > > 0% > > > in some cases". > > > > > > There's also some additional code to disable the workaround for > > > frontbuffers that ever received a sw_finish or dirty_fb IOCTL, > > > since > > > the current bad user space never issues those calls. This should > > > help > > > for some specific cases and our IGT tests, but won't be enough for > > > a > > > new xf86-video-intel using TearFree. > > > > > > Notice that we may need an equivalent commit for PSR too. We also > > > need > > > to investigate whether PSR needs to be disabled on GTT mmaps: if > > > that's the case, we'll have problems since we seem to always have > > > GTT > > > mmaps on our current user space, so we would end up keeping PSR > > > disabled forever. > > > > > > v2: > > > - Rename some things. > > > - Disable the WA on sw_finish/dirty_fb (Daniel). > > > - Fix NULL obj checking. > > > - Restric to Gen 9. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > > > drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++------- > > > drivers/gpu/drm/i915/intel_display.c | 1 + > > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > > drivers/gpu/drm/i915/intel_fbc.c | 33 > > > ++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_frontbuffer.c | 31 > > > ++++++++++++++++++++++++++++++ > > > 6 files changed, 89 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index efca534..45e31d2 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -873,6 +873,13 @@ enum fb_op_origin { > > > ORIGIN_DIRTYFB, > > > }; > > > > > > +enum fb_mmap_wa_flags { > > > + FB_MMAP_WA_CPU = 1 << 0, > > > + FB_MMAP_WA_GTT = 1 << 1, > > > + FB_MMAP_WA_DISABLE = 1 << 2, > > > + FB_MMAP_WA_FLAG_COUNT = 3, > > > +}; > > > + > > I'll do the change to macros you/Jani mentioned in the other emails. > > > > struct intel_fbc { > > > /* This is always the inner lock when overlapping with > > > struct_mutex and > > > * it's the outer lock when overlapping with stolen_lock. > > > */ > > > @@ -910,6 +917,7 @@ struct intel_fbc { > > > unsigned int stride; > > > int fence_reg; > > > unsigned int tiling_mode; > > > + unsigned int mmap_wa_flags; > > > } fb; > > > } state_cache; > > > > > > @@ -2143,6 +2151,7 @@ struct drm_i915_gem_object { > > > unsigned int cache_dirty:1; > > > > > > unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; > > > + unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT; > > > > > > unsigned int pin_display; > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > b/drivers/gpu/drm/i915/i915_gem.c > > > index 8588c83..d44f27e 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct drm_device > > > *dev, void *data, > > > goto unlock; > > > } > > > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > > + > > > /* Pinned buffers may be scanout, so flush the cache */ > > > if (obj->pin_display) > > > i915_gem_object_flush_cpu_write_domain(obj); > > > @@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > > void *data, > > > struct drm_file *file) > > > { > > > struct drm_i915_gem_mmap *args = data; > > > - struct drm_gem_object *obj; > > > + struct drm_i915_gem_object *obj; > > > unsigned long addr; > > > > > > if (args->flags & ~(I915_MMAP_WC)) > > > @@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > > void *data, > > > if (args->flags & I915_MMAP_WC && !cpu_has_pat) > > > return -ENODEV; > > > > > > - obj = drm_gem_object_lookup(dev, file, args->handle); > > > - if (obj == NULL) > > > + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args- > > > >handle)); > > > + if (&obj->base == NULL) > > > return -ENOENT; > > > > > > /* prime objects have no backing filp to GEM mmap > > > * pages from. > > > */ > > > - if (!obj->filp) { > > > - drm_gem_object_unreference_unlocked(obj); > > > + if (!obj->base.filp) { > > > + drm_gem_object_unreference_unlocked(&obj->base); > > > return -EINVAL; > > > } > > > > > > - addr = vm_mmap(obj->filp, 0, args->size, > > > + addr = vm_mmap(obj->base.filp, 0, args->size, > > > PROT_READ | PROT_WRITE, MAP_SHARED, > > > args->offset); > > > if (args->flags & I915_MMAP_WC) { > > > @@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device *dev, > > > void *data, > > > addr = -ENOMEM; > > > up_write(&mm->mmap_sem); > > > } > > > - drm_gem_object_unreference_unlocked(obj); > > > + drm_gem_object_unreference_unlocked(&obj->base); > > > if (IS_ERR((void *)addr)) > > > return addr; > > > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU); > > > + > > > args->addr_ptr = (uint64_t) addr; > > > > > > return 0; > > > @@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file, > > > goto out; > > > > > > *offset = drm_vma_node_offset_addr(&obj->base.vma_node); > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT); > > > > > > out: > > > drm_gem_object_unreference(&obj->base); > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 28ead66..6e7aa9e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14705,6 +14705,7 @@ static int > > > intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > > > struct drm_i915_gem_object *obj = intel_fb->obj; > > > > > > mutex_lock(&dev->struct_mutex); > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > > intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB); > > > mutex_unlock(&dev->struct_mutex); > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index ba45245..bbea7c4 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1082,6 +1082,7 @@ void intel_frontbuffer_flip_complete(struct > > > drm_device *dev, > > > unsigned frontbuffer_bits); > > > void intel_frontbuffer_flip(struct drm_device *dev, > > > unsigned frontbuffer_bits); > > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > > unsigned int flags); > > > unsigned int intel_fb_align_height(struct drm_device *dev, > > > unsigned int height, > > > uint32_t pixel_format, > > > @@ -1371,6 +1372,8 @@ void intel_fbc_invalidate(struct > > > drm_i915_private *dev_priv, > > > enum fb_op_origin origin); > > > void intel_fbc_flush(struct drm_i915_private *dev_priv, > > > unsigned int frontbuffer_bits, enum > > > fb_op_origin origin); > > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > > + unsigned int frontbuffer_bits, unsigned int > > > flags); > > > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > > > > > > /* intel_hdmi.c */ > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index 7101880..718ac38 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -745,6 +745,14 @@ static void > > > intel_fbc_update_state_cache(struct intel_crtc *crtc) > > > cache->fb.stride = fb->pitches[0]; > > > cache->fb.fence_reg = obj->fence_reg; > > > cache->fb.tiling_mode = obj->tiling_mode; > > > + cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags; > > > +} > > > + > > > +static bool need_mmap_disable_workaround(struct intel_fbc *fbc) > > > +{ > > > + unsigned int flags = fbc->state_cache.fb.mmap_wa_flags; > > > + > > > + return (flags & FB_MMAP_WA_CPU) && !(flags & > > > FB_MMAP_WA_DISABLE); > > > } > > > > > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > > @@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct > > > intel_crtc *crtc) > > > return false; > > > } > > > > > > + if (need_mmap_disable_workaround(fbc)) { > > > + fbc->no_fbc_reason = "FB is CPU or WC mmapped"; > > > + return false; > > > + } > > > + > > > return true; > > > } > > > > > > @@ -1008,6 +1021,26 @@ out: > > > mutex_unlock(&fbc->lock); > > > } > > > > > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > > + unsigned int frontbuffer_bits, unsigned int > > > flags) > > > +{ > > > + struct intel_fbc *fbc = &dev_priv->fbc; > > > + > > > + if (!fbc_supported(dev_priv)) > > > + return; > > > + > > > + mutex_lock(&fbc->lock); > > > + > > > + if (fbc->enabled && > > > + (intel_fbc_get_frontbuffer_bit(fbc) & > > > frontbuffer_bits)) { > > > + fbc->state_cache.fb.mmap_wa_flags = flags; > > > + if (need_mmap_disable_workaround(fbc)) > > > + intel_fbc_deactivate(dev_priv); > > > + } > > > + > > > + mutex_unlock(&fbc->lock); > > > +} > > > + > > > /** > > > * intel_fbc_choose_crtc - select a CRTC to enable FBC on > > > * @dev_priv: i915 device instance > > > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c > > > b/drivers/gpu/drm/i915/intel_frontbuffer.c > > > index ac85357..8d9b327 100644 > > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > > > @@ -241,3 +241,34 @@ void intel_frontbuffer_flip(struct drm_device > > > *dev, > > > > > > intel_frontbuffer_flush(dev, frontbuffer_bits, > > > ORIGIN_FLIP); > > > } > > > + > > > +/** > > > + * intel_fb_obj_mmap_wa - notifies about objects being mmapped > > > + * @obj: GEM object being mmapped > > > + * > > > + * This function gets called whenever an object gets mmapped. Not > > > every user > > > + * space application follows the protocol assumed by the > > > frontbuffer tracking > > > + * subsystem when it was created, so this mmap notify callback can > > > be used to > > > + * completely disable frontbuffer features such as FBC and PSR. > > > Even if at some > > > + * point we fix ever user space application, there's still the > > > possibility that > > > + * the user may have a new Kernel with the old user space. > > > + * > > > + * Also notice that there's no munmap API because user space calls > > > munmap() > > > + * directly. Even if we had, it probably wouldn't help since > > > munmap() calls are > > > + * not common. > > > + */ > > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > > unsigned int flags) > > > +{ > > > + struct drm_i915_private *dev_priv = obj->base.dev- > > > >dev_private; > > > + > > > + if (!IS_GEN9(dev_priv)) > > Please only IS_SKYLAKE, I don't want to extend this to either bxt or > > kbl, > > and both are still under prelim_hw_support and not yet shipping, i.e. > > we > > can break them ;-) > > Mmmkay. > > > > > Wrt the implementation: I think it would be great to get PSR on board > > to > > avoid duplicating the logic. > > The goal of this patch going through intel_frontbuffer.c instead of > directly to intel_fbc.c is exactly to allow for PSR to do something > similar. > > > > One option that might would be to create an > > invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect > > (until the first sw_finish or dirty_fb) the frontbuffer tracking code > > itself would filter all flushes. > > That's an non-trivial distortion of the trivial invalidate/flush API. I > prefer having a special codepath for this special case instead of > adding second meanings to flush/invalidate. > > Besides, even with the invalidate/flush API being simple, look at how > many times we already tweaked/broke/fixed those functions, both for PSR > and FBC. Hm good point. It would indeed not neatly fit into the existing invalidate/flush scheme (which assumes that there's only ever one thing that can access a buffer, not a permanent mmap that can always be used). Agreed that having a special case makes sense. > > fbc could then use ORIGIN_MMAP_WA > > invalidate to permanently disable (well, not re-enable after flip) > > until > > it sees a flush. And PSR would Just Work (I hope). > > I think any PSR fixes related to this should be separate patches. I > have to confess I've just been studying the FBC part of the problem, so > I was waiting for Rodrigo's opinions here before any conclusions. > Anyway, we can always change this code (in this patch or later) to > better suit PSR. I'd like to at least have an ack from Rrodigo, even better if he can hack up something quickly. But yeah it's code, we can change it again ;-) -Daniel
On Wed, 2016-03-23 at 09:53 +0100, Daniel Vetter wrote: > On Tue, Mar 22, 2016 at 09:48:00PM +0000, Zanoni, Paulo R wrote: > > Em Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu: > > > On Mon, Mar 21, 2016 at 04:26:57PM -0300, Paulo Zanoni wrote: > > > > > > > > FBC and the frontbuffer tracking infrastructure were designed > > > > assuming > > > > that user space applications would follow a specific set of > > > > rules > > > > regarding frontbuffer management and mmapping. I recently > > > > discovered > > > > that current user space is not exactly following these rules: > > > > my > > > > investigation led me to the conclusion that the generic backend > > > > from > > > > SNA - used by SKL and the other new platforms without a > > > > specific > > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using > > > > the > > > > CPU > > > > and WC mmaps. I discovered this when running lightdm: I would > > > > type > > > > the > > > > password and nothing would appear on the screen unless I moved > > > > the > > > > mouse over the place where the letters were supposed to appear. > > > > > > > > Since we can't detect whether the current DRM master is a well- > > > > behaved > > > > application or not, let's use the sledgehammer approach and > > > > assume > > > > that every user space client on every gen is bad by disabling > > > > FBC > > > > when > > > > the frontbuffer is CPU/WC mmapped. This will allow us to > > > > enable > > > > FBC > > > > on SKL, moving its FBC-related power savings from "always 0%" > > > > to "> > > > > 0% > > > > in some cases". > > > > > > > > There's also some additional code to disable the workaround for > > > > frontbuffers that ever received a sw_finish or dirty_fb IOCTL, > > > > since > > > > the current bad user space never issues those calls. This > > > > should > > > > help > > > > for some specific cases and our IGT tests, but won't be enough > > > > for > > > > a > > > > new xf86-video-intel using TearFree. > > > > > > > > Notice that we may need an equivalent commit for PSR too. We > > > > also > > > > need > > > > to investigate whether PSR needs to be disabled on GTT mmaps: > > > > if > > > > that's the case, we'll have problems since we seem to always > > > > have > > > > GTT > > > > mmaps on our current user space, so we would end up keeping PSR > > > > disabled forever. > > > > > > > > v2: > > > > - Rename some things. > > > > - Disable the WA on sw_finish/dirty_fb (Daniel). > > > > - Fix NULL obj checking. > > > > - Restric to Gen 9. > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > > > > drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++----- > > > > -- > > > > drivers/gpu/drm/i915/intel_display.c | 1 + > > > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > > > drivers/gpu/drm/i915/intel_fbc.c | 33 > > > > ++++++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/i915/intel_frontbuffer.c | 31 > > > > ++++++++++++++++++++++++++++++ > > > > 6 files changed, 89 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index efca534..45e31d2 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -873,6 +873,13 @@ enum fb_op_origin { > > > > ORIGIN_DIRTYFB, > > > > }; > > > > > > > > +enum fb_mmap_wa_flags { > > > > + FB_MMAP_WA_CPU = 1 << 0, > > > > + FB_MMAP_WA_GTT = 1 << 1, > > > > + FB_MMAP_WA_DISABLE = 1 << 2, > > > > + FB_MMAP_WA_FLAG_COUNT = 3, > > > > +}; > > > > + > > > > I'll do the change to macros you/Jani mentioned in the other > > emails. > > > > > > struct intel_fbc { > > > > /* This is always the inner lock when overlapping with > > > > struct_mutex and > > > > * it's the outer lock when overlapping with > > > > stolen_lock. > > > > */ > > > > @@ -910,6 +917,7 @@ struct intel_fbc { > > > > unsigned int stride; > > > > int fence_reg; > > > > unsigned int tiling_mode; > > > > + unsigned int mmap_wa_flags; > > > > } fb; > > > > } state_cache; > > > > > > > > @@ -2143,6 +2151,7 @@ struct drm_i915_gem_object { > > > > unsigned int cache_dirty:1; > > > > > > > > unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; > > > > + unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT; > > > > > > > > unsigned int pin_display; > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > b/drivers/gpu/drm/i915/i915_gem.c > > > > index 8588c83..d44f27e 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > @@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct > > > > drm_device > > > > *dev, void *data, > > > > goto unlock; > > > > } > > > > > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > > > + > > > > /* Pinned buffers may be scanout, so flush the cache > > > > */ > > > > if (obj->pin_display) > > > > i915_gem_object_flush_cpu_write_domain(obj); > > > > @@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device > > > > *dev, > > > > void *data, > > > > struct drm_file *file) > > > > { > > > > struct drm_i915_gem_mmap *args = data; > > > > - struct drm_gem_object *obj; > > > > + struct drm_i915_gem_object *obj; > > > > unsigned long addr; > > > > > > > > if (args->flags & ~(I915_MMAP_WC)) > > > > @@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device > > > > *dev, > > > > void *data, > > > > if (args->flags & I915_MMAP_WC && !cpu_has_pat) > > > > return -ENODEV; > > > > > > > > - obj = drm_gem_object_lookup(dev, file, args->handle); > > > > - if (obj == NULL) > > > > + obj = to_intel_bo(drm_gem_object_lookup(dev, file, > > > > args- > > > > > handle)); > > > > + if (&obj->base == NULL) > > > > return -ENOENT; > > > > > > > > /* prime objects have no backing filp to GEM mmap > > > > * pages from. > > > > */ > > > > - if (!obj->filp) { > > > > - drm_gem_object_unreference_unlocked(obj); > > > > + if (!obj->base.filp) { > > > > + drm_gem_object_unreference_unlocked(&obj > > > > ->base); > > > > return -EINVAL; > > > > } > > > > > > > > - addr = vm_mmap(obj->filp, 0, args->size, > > > > + addr = vm_mmap(obj->base.filp, 0, args->size, > > > > PROT_READ | PROT_WRITE, MAP_SHARED, > > > > args->offset); > > > > if (args->flags & I915_MMAP_WC) { > > > > @@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device > > > > *dev, > > > > void *data, > > > > addr = -ENOMEM; > > > > up_write(&mm->mmap_sem); > > > > } > > > > - drm_gem_object_unreference_unlocked(obj); > > > > + drm_gem_object_unreference_unlocked(&obj->base); > > > > if (IS_ERR((void *)addr)) > > > > return addr; > > > > > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU); > > > > + > > > > args->addr_ptr = (uint64_t) addr; > > > > > > > > return 0; > > > > @@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file, > > > > goto out; > > > > > > > > *offset = drm_vma_node_offset_addr(&obj > > > > ->base.vma_node); > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT); > > > > > > > > out: > > > > drm_gem_object_unreference(&obj->base); > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 28ead66..6e7aa9e 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -14705,6 +14705,7 @@ static int > > > > intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > > > > struct drm_i915_gem_object *obj = intel_fb->obj; > > > > > > > > mutex_lock(&dev->struct_mutex); > > > > + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); > > > > intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB); > > > > mutex_unlock(&dev->struct_mutex); > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index ba45245..bbea7c4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1082,6 +1082,7 @@ void > > > > intel_frontbuffer_flip_complete(struct > > > > drm_device *dev, > > > > unsigned > > > > frontbuffer_bits); > > > > void intel_frontbuffer_flip(struct drm_device *dev, > > > > unsigned frontbuffer_bits); > > > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > > > unsigned int flags); > > > > unsigned int intel_fb_align_height(struct drm_device *dev, > > > > unsigned int height, > > > > uint32_t pixel_format, > > > > @@ -1371,6 +1372,8 @@ void intel_fbc_invalidate(struct > > > > drm_i915_private *dev_priv, > > > > enum fb_op_origin origin); > > > > void intel_fbc_flush(struct drm_i915_private *dev_priv, > > > > unsigned int frontbuffer_bits, enum > > > > fb_op_origin origin); > > > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > > > + unsigned int frontbuffer_bits, unsigned > > > > int > > > > flags); > > > > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > > > > > > > > /* intel_hdmi.c */ > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > > index 7101880..718ac38 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > @@ -745,6 +745,14 @@ static void > > > > intel_fbc_update_state_cache(struct intel_crtc *crtc) > > > > cache->fb.stride = fb->pitches[0]; > > > > cache->fb.fence_reg = obj->fence_reg; > > > > cache->fb.tiling_mode = obj->tiling_mode; > > > > + cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags; > > > > +} > > > > + > > > > +static bool need_mmap_disable_workaround(struct intel_fbc > > > > *fbc) > > > > +{ > > > > + unsigned int flags = fbc > > > > ->state_cache.fb.mmap_wa_flags; > > > > + > > > > + return (flags & FB_MMAP_WA_CPU) && !(flags & > > > > FB_MMAP_WA_DISABLE); > > > > } > > > > > > > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > > > @@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct > > > > intel_crtc *crtc) > > > > return false; > > > > } > > > > > > > > + if (need_mmap_disable_workaround(fbc)) { > > > > + fbc->no_fbc_reason = "FB is CPU or WC > > > > mmapped"; > > > > + return false; > > > > + } > > > > + > > > > return true; > > > > } > > > > > > > > @@ -1008,6 +1021,26 @@ out: > > > > mutex_unlock(&fbc->lock); > > > > } > > > > > > > > +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, > > > > + unsigned int frontbuffer_bits, unsigned > > > > int > > > > flags) > > > > +{ > > > > + struct intel_fbc *fbc = &dev_priv->fbc; > > > > + > > > > + if (!fbc_supported(dev_priv)) > > > > + return; > > > > + > > > > + mutex_lock(&fbc->lock); > > > > + > > > > + if (fbc->enabled && > > > > + (intel_fbc_get_frontbuffer_bit(fbc) & > > > > frontbuffer_bits)) { > > > > + fbc->state_cache.fb.mmap_wa_flags = flags; > > > > + if (need_mmap_disable_workaround(fbc)) > > > > + intel_fbc_deactivate(dev_priv); > > > > + } > > > > + > > > > + mutex_unlock(&fbc->lock); > > > > +} > > > > + > > > > /** > > > > * intel_fbc_choose_crtc - select a CRTC to enable FBC on > > > > * @dev_priv: i915 device instance > > > > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c > > > > b/drivers/gpu/drm/i915/intel_frontbuffer.c > > > > index ac85357..8d9b327 100644 > > > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > > > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > > > > @@ -241,3 +241,34 @@ void intel_frontbuffer_flip(struct > > > > drm_device > > > > *dev, > > > > > > > > intel_frontbuffer_flush(dev, frontbuffer_bits, > > > > ORIGIN_FLIP); > > > > } > > > > + > > > > +/** > > > > + * intel_fb_obj_mmap_wa - notifies about objects being mmapped > > > > + * @obj: GEM object being mmapped > > > > + * > > > > + * This function gets called whenever an object gets mmapped. > > > > Not > > > > every user > > > > + * space application follows the protocol assumed by the > > > > frontbuffer tracking > > > > + * subsystem when it was created, so this mmap notify callback > > > > can > > > > be used to > > > > + * completely disable frontbuffer features such as FBC and > > > > PSR. > > > > Even if at some > > > > + * point we fix ever user space application, there's still the > > > > possibility that > > > > + * the user may have a new Kernel with the old user space. > > > > + * > > > > + * Also notice that there's no munmap API because user space > > > > calls > > > > munmap() > > > > + * directly. Even if we had, it probably wouldn't help since > > > > munmap() calls are > > > > + * not common. > > > > + */ > > > > +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, > > > > unsigned int flags) > > > > +{ > > > > + struct drm_i915_private *dev_priv = obj->base.dev- > > > > > dev_private; > > > > + > > > > + if (!IS_GEN9(dev_priv)) > > > Please only IS_SKYLAKE, I don't want to extend this to either bxt > > > or > > > kbl, > > > and both are still under prelim_hw_support and not yet shipping, > > > i.e. > > > we > > > can break them ;-) > > > > Mmmkay. I vote for INTEL_INFO(dev)->gen < 9... Since we only have the proper api in use for gen8 and older... > > > > > > > > Wrt the implementation: I think it would be great to get PSR on > > > board > > > to > > > avoid duplicating the logic. > > > > The goal of this patch going through intel_frontbuffer.c instead of > > directly to intel_fbc.c is exactly to allow for PSR to do something > > similar. > > > > > > > One option that might would be to create an > > > invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in > > > effect > > > (until the first sw_finish or dirty_fb) the frontbuffer tracking > > > code > > > itself would filter all flushes. > > > > That's an non-trivial distortion of the trivial invalidate/flush > > API. I > > prefer having a special codepath for this special case instead of > > adding second meanings to flush/invalidate. > > > > Besides, even with the invalidate/flush API being simple, look at > > how > > many times we already tweaked/broke/fixed those functions, both for > > PSR > > and FBC. > > Hm good point. It would indeed not neatly fit into the existing > invalidate/flush scheme (which assumes that there's only ever one > thing > that can access a buffer, not a permanent mmap that can always be > used). > Agreed that having a special case makes sense. Hm.... I believe I like the ORIGIN_MMAP_WA idea more. I'm afraid that I'm already changing the original invalidate/flush API here on my side for PSR on VLV. I'm planing to introduce the ORIGIN_VBLANK to be able to trigger psr exit on vblank waits in vlv... and only flush those bits when we have no vblank waiting. Another total different way would be to add a runtime psr domains with vblank domain, but it would be difficult to sync the psr busy bits... so PSR would need to control the origin. So, the same distortion (update?) of the frontbuffer tracking bits could be used for this case here so I believe the amount of period power savings feature keeps disabled would be reduced, improving the power savings. > > > > fbc could then use ORIGIN_MMAP_WA > > > invalidate to permanently disable (well, not re-enable after > > > flip) > > > until > > > it sees a flush. And PSR would Just Work (I hope). > > > > I think any PSR fixes related to this should be separate patches. I > > have to confess I've just been studying the FBC part of the > > problem, so > > I was waiting for Rodrigo's opinions here before any conclusions. > > Anyway, we can always change this code (in this patch or later) to > > better suit PSR. I believe for PSR is exactly the same here... I was waiting your conclusion so I could replicate that for PSR somehow. > > I'd like to at least have an ack from Rrodigo, even better if he can > hack > up something quickly. But yeah it's code, we can change it again ;-) I prefer the solution on the frontbuffer tracking since this is not an fbc issue only, but I don't want to block here. I agree it we can always change it. Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index efca534..45e31d2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -873,6 +873,13 @@ enum fb_op_origin { ORIGIN_DIRTYFB, }; +enum fb_mmap_wa_flags { + FB_MMAP_WA_CPU = 1 << 0, + FB_MMAP_WA_GTT = 1 << 1, + FB_MMAP_WA_DISABLE = 1 << 2, + FB_MMAP_WA_FLAG_COUNT = 3, +}; + struct intel_fbc { /* This is always the inner lock when overlapping with struct_mutex and * it's the outer lock when overlapping with stolen_lock. */ @@ -910,6 +917,7 @@ struct intel_fbc { unsigned int stride; int fence_reg; unsigned int tiling_mode; + unsigned int mmap_wa_flags; } fb; } state_cache; @@ -2143,6 +2151,7 @@ struct drm_i915_gem_object { unsigned int cache_dirty:1; unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; + unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT; unsigned int pin_display; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8588c83..d44f27e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, goto unlock; } + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); + /* Pinned buffers may be scanout, so flush the cache */ if (obj->pin_display) i915_gem_object_flush_cpu_write_domain(obj); @@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { struct drm_i915_gem_mmap *args = data; - struct drm_gem_object *obj; + struct drm_i915_gem_object *obj; unsigned long addr; if (args->flags & ~(I915_MMAP_WC)) @@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, if (args->flags & I915_MMAP_WC && !cpu_has_pat) return -ENODEV; - obj = drm_gem_object_lookup(dev, file, args->handle); - if (obj == NULL) + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); + if (&obj->base == NULL) return -ENOENT; /* prime objects have no backing filp to GEM mmap * pages from. */ - if (!obj->filp) { - drm_gem_object_unreference_unlocked(obj); + if (!obj->base.filp) { + drm_gem_object_unreference_unlocked(&obj->base); return -EINVAL; } - addr = vm_mmap(obj->filp, 0, args->size, + addr = vm_mmap(obj->base.filp, 0, args->size, PROT_READ | PROT_WRITE, MAP_SHARED, args->offset); if (args->flags & I915_MMAP_WC) { @@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, addr = -ENOMEM; up_write(&mm->mmap_sem); } - drm_gem_object_unreference_unlocked(obj); + drm_gem_object_unreference_unlocked(&obj->base); if (IS_ERR((void *)addr)) return addr; + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU); + args->addr_ptr = (uint64_t) addr; return 0; @@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file, goto out; *offset = drm_vma_node_offset_addr(&obj->base.vma_node); + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT); out: drm_gem_object_unreference(&obj->base); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 28ead66..6e7aa9e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14705,6 +14705,7 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, struct drm_i915_gem_object *obj = intel_fb->obj; mutex_lock(&dev->struct_mutex); + intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE); intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ba45245..bbea7c4 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1082,6 +1082,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev, unsigned frontbuffer_bits); void intel_frontbuffer_flip(struct drm_device *dev, unsigned frontbuffer_bits); +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags); unsigned int intel_fb_align_height(struct drm_device *dev, unsigned int height, uint32_t pixel_format, @@ -1371,6 +1372,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, enum fb_op_origin origin); void intel_fbc_flush(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits, enum fb_op_origin origin); +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits, unsigned int flags); void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); /* intel_hdmi.c */ diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 7101880..718ac38 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -745,6 +745,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) cache->fb.stride = fb->pitches[0]; cache->fb.fence_reg = obj->fence_reg; cache->fb.tiling_mode = obj->tiling_mode; + cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags; +} + +static bool need_mmap_disable_workaround(struct intel_fbc *fbc) +{ + unsigned int flags = fbc->state_cache.fb.mmap_wa_flags; + + return (flags & FB_MMAP_WA_CPU) && !(flags & FB_MMAP_WA_DISABLE); } static bool intel_fbc_can_activate(struct intel_crtc *crtc) @@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } + if (need_mmap_disable_workaround(fbc)) { + fbc->no_fbc_reason = "FB is CPU or WC mmapped"; + return false; + } + return true; } @@ -1008,6 +1021,26 @@ out: mutex_unlock(&fbc->lock); } +void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits, unsigned int flags) +{ + struct intel_fbc *fbc = &dev_priv->fbc; + + if (!fbc_supported(dev_priv)) + return; + + mutex_lock(&fbc->lock); + + if (fbc->enabled && + (intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits)) { + fbc->state_cache.fb.mmap_wa_flags = flags; + if (need_mmap_disable_workaround(fbc)) + intel_fbc_deactivate(dev_priv); + } + + mutex_unlock(&fbc->lock); +} + /** * intel_fbc_choose_crtc - select a CRTC to enable FBC on * @dev_priv: i915 device instance diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c index ac85357..8d9b327 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -241,3 +241,34 @@ void intel_frontbuffer_flip(struct drm_device *dev, intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP); } + +/** + * intel_fb_obj_mmap_wa - notifies about objects being mmapped + * @obj: GEM object being mmapped + * + * This function gets called whenever an object gets mmapped. Not every user + * space application follows the protocol assumed by the frontbuffer tracking + * subsystem when it was created, so this mmap notify callback can be used to + * completely disable frontbuffer features such as FBC and PSR. Even if at some + * point we fix ever user space application, there's still the possibility that + * the user may have a new Kernel with the old user space. + * + * Also notice that there's no munmap API because user space calls munmap() + * directly. Even if we had, it probably wouldn't help since munmap() calls are + * not common. + */ +void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags) +{ + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; + + if (!IS_GEN9(dev_priv)) + return; + + obj->fb_mmap_wa_flags |= flags; + + if (!obj->frontbuffer_bits) + return; + + intel_fbc_mmap_wa(dev_priv, obj->frontbuffer_bits, + obj->fb_mmap_wa_flags); +}
FBC and the frontbuffer tracking infrastructure were designed assuming that user space applications would follow a specific set of rules regarding frontbuffer management and mmapping. I recently discovered that current user space is not exactly following these rules: my investigation led me to the conclusion that the generic backend from SNA - used by SKL and the other new platforms without a specific backend - is not issuing sw_finish/dirty_fb IOCTLs when using the CPU and WC mmaps. I discovered this when running lightdm: I would type the password and nothing would appear on the screen unless I moved the mouse over the place where the letters were supposed to appear. Since we can't detect whether the current DRM master is a well-behaved application or not, let's use the sledgehammer approach and assume that every user space client on every gen is bad by disabling FBC when the frontbuffer is CPU/WC mmapped. This will allow us to enable FBC on SKL, moving its FBC-related power savings from "always 0%" to "> 0% in some cases". There's also some additional code to disable the workaround for frontbuffers that ever received a sw_finish or dirty_fb IOCTL, since the current bad user space never issues those calls. This should help for some specific cases and our IGT tests, but won't be enough for a new xf86-video-intel using TearFree. Notice that we may need an equivalent commit for PSR too. We also need to investigate whether PSR needs to be disabled on GTT mmaps: if that's the case, we'll have problems since we seem to always have GTT mmaps on our current user space, so we would end up keeping PSR disabled forever. v2: - Rename some things. - Disable the WA on sw_finish/dirty_fb (Daniel). - Fix NULL obj checking. - Restric to Gen 9. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++------- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_fbc.c | 33 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_frontbuffer.c | 31 ++++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 7 deletions(-)