From patchwork Thu Feb 25 16:48:27 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Zanoni, Paulo R" X-Patchwork-Id: 8425051 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 3A8219FC33 for ; Thu, 25 Feb 2016 16:48:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8AF8720377 for ; Thu, 25 Feb 2016 16:48:50 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id AA25E20212 for ; Thu, 25 Feb 2016 16:48:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 577BE6E682; Thu, 25 Feb 2016 16:48:47 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id D973B6E682 for ; Thu, 25 Feb 2016 16:48:45 +0000 (UTC) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 25 Feb 2016 08:48:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,498,1449561600"; d="scan'208";a="894643637" Received: from jholm-mobl.amr.corp.intel.com (HELO panetone.amr.corp.intel.com) ([10.252.138.60]) by orsmga001.jf.intel.com with ESMTP; 25 Feb 2016 08:48:43 -0800 From: Paulo Zanoni To: intel-gfx@lists.freedesktop.org Date: Thu, 25 Feb 2016 13:48:27 -0300 Message-Id: <1456418912-25723-1-git-send-email-paulo.r.zanoni@intel.com> X-Mailer: git-send-email 2.7.0 Cc: Daniel Vetter , Paulo Zanoni , Rodrigo Vivi Subject: [Intel-gfx] [RFC 1/2] drm/i915: opt-out CPU and WC mmaps from FBC X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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/set_domain 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". This also has the potential to hurt FBC on the older platforms a little bit, but my quick check suggests we're fine, so I didn't make this commit SKL-only. 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 may end up keeping PSR disabled forever. And if you're reading this commit message because either igt/kms_frontbuffer_tracking or igt/kms_fbc_crc broke and you decided to bisect, please update your IGT repository. You'll need a series of commits ending with: kms_frontbuffer_tracking: be aware of the new FBC Kernel workaround Cc: Rodrigo Vivi Cc: Daniel Vetter Cc: Chris Wilson Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++------ drivers/gpu/drm/i915/intel_drv.h | 4 ++++ drivers/gpu/drm/i915/intel_fbc.c | 32 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_frontbuffer.c | 25 +++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 6 deletions(-) We've been discussing other possible alternatives, but here's one. Clever alternative ideas are always welcome. diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9e76bfc..fede522 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -904,6 +904,13 @@ enum fb_op_origin { ORIGIN_DIRTYFB, }; +enum mmap_bits { + MMAP_BITS_CPU = 1 << 0, + MMAP_BITS_GTT = 1 << 1, + MMAP_BITS_WC = 1 << 2, + MMAP_BITS_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. */ @@ -941,6 +948,7 @@ struct intel_fbc { unsigned int stride; int fence_reg; unsigned int tiling_mode; + unsigned int mmap_bits; } fb; } state_cache; @@ -2163,6 +2171,7 @@ struct drm_i915_gem_object { unsigned int cache_dirty:1; unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS; + unsigned int mmap_bits:MMAP_BITS_COUNT; unsigned int pin_display; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f68f346..9c0e7b9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1726,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)) @@ -1735,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); + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (obj == 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) { @@ -1763,10 +1763,14 @@ 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; + obj->mmap_bits |= (args->flags & I915_MMAP_WC) ? + MMAP_BITS_WC : MMAP_BITS_CPU; + intel_frontbuffer_obj_mmap(obj); + args->addr_ptr = (uint64_t) addr; return 0; @@ -2101,6 +2105,8 @@ i915_gem_mmap_gtt(struct drm_file *file, goto out; *offset = drm_vma_node_offset_addr(&obj->base.vma_node); + obj->mmap_bits |= MMAP_BITS_GTT; + intel_frontbuffer_obj_mmap(obj); out: drm_gem_object_unreference(&obj->base); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4852049..d02bd7b1 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1042,6 +1042,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_frontbuffer_obj_mmap(struct drm_i915_gem_object *obj); unsigned int intel_fb_align_height(struct drm_device *dev, unsigned int height, uint32_t pixel_format, @@ -1343,6 +1344,9 @@ 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_notify(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits, + unsigned int mmap_bits); 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 0f0492f..a3fd699 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -745,6 +745,12 @@ 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_bits = obj->mmap_bits; +} + +static bool need_mmap_disable_workaround(struct intel_fbc *fbc) +{ + return fbc->state_cache.fb.mmap_bits & (MMAP_BITS_CPU | MMAP_BITS_WC); } static bool intel_fbc_can_activate(struct intel_crtc *crtc) @@ -816,6 +822,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; } @@ -1014,6 +1025,27 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, mutex_unlock(&fbc->lock); } +void intel_fbc_mmap_notify(struct drm_i915_private *dev_priv, + unsigned int frontbuffer_bits, + unsigned int mmap_bits) +{ + 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_bits = mmap_bits; + 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..a4471c3 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -241,3 +241,28 @@ void intel_frontbuffer_flip(struct drm_device *dev, intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP); } + +/** + * intel_frontbuffer_obj_mmap - notifies about frontbuffer objects being mmapped + * @obj: GEM object being mmapped + * + * This function gets called whenever an object gets mmapped. As of February + * 2016, 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. Once every user space is properly behaving - and once we can verify + * this somehow -, we'll be able to remove this function. + * + * 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_frontbuffer_obj_mmap(struct drm_i915_gem_object *obj) +{ + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; + + if (!obj->frontbuffer_bits) + return; + + intel_fbc_mmap_notify(dev_priv, obj->frontbuffer_bits, obj->mmap_bits); +}