From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 3/6] drm/i915: Add enable_sagv option Date: Wed, 05 Oct 2016 16:32:31 -0300 Message-ID: <1475695951.2381.48.camel@intel.com> References: <1475681598-12081-1-git-send-email-cpaul@redhat.com> <1475681598-12081-4-git-send-email-cpaul@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1475681598-12081-4-git-send-email-cpaul@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Lyude , intel-gfx@lists.freedesktop.org Cc: David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org RW0gUXVhLCAyMDE2LTEwLTA1IMOgcyAxMTozMyAtMDQwMCwgTHl1ZGUgZXNjcmV2ZXU6Cj4gVGhp cyBvcHRpb24gYWxsb3dzIHVzIHRvIG1hbnVhbGx5IGNvbnRyb2wgdGhlIFNBR1YgYXQgbW9kdWxl IGxvYWQKPiB0aW1lLgo+IFRoaXMgY2FuIGJlIHVzZWZ1bCBpbiBzaXR1YXRpb25zIHN1Y2ggYXMg dHJ5aW5nIHRvIGRlYnVnIHdhdGVybWFyawo+IGNoYW5nZXMsIHNpbmNlIGVuYWJsZWQgU0FHViAr IGluY29ycmVjdCB3YXRlcm1hcmtzID0gdG90YWwgR1BVCj4gYW5uaWhpbGF0aW9uLgoKSSdtIG5v dCBhIGh1Z2UgZmFuIG9mIGFkZGluZyBvcHRpb25zIHRoYXQgYXJlIG9ubHkgZm9yIHZlcnkgbGlt aXRlZApkZWJ1Z2dpbmcgc2l0dWF0aW9ucywgZXNwZWNpYWxseSBzaW1wbGUgb25lcyB0aGF0IGNh biBhbHdheXMganVzdCBiZQpyZS1pbXBsZW1lbnRlZCBkdXJpbmcgZGVidWdnaW5nIHNlc3Npb25z IHN1Y2ggYXMgdGhpcyBvbmUuIEFueXdheSwgSSdtCm5vdCBvcHBvc2VkIHRvIGFkZGluZyB0aGUg b3B0aW9uIHNpbmNlIGl0J3MgbWFya2VkIGFzIHVuc2FmZSBhbnl3YXksCkknbSBqdXN0IHN0YXRp bmcgbXkgZ2VuZXJhbCBvcGluaW9uLiBTZWUgbW9yZSBiZWxvdy4KCgo+IAo+IFNpZ25lZC1vZmYt Ynk6IEx5dWRlIDxjcGF1bEByZWRoYXQuY29tPgo+IENjOiBNYWFydGVuIExhbmtob3JzdCA8bWFh cnRlbi5sYW5raG9yc3RAbGludXguaW50ZWwuY29tPgo+IENjOiBWaWxsZSBTeXJqw6Rsw6QgPHZp bGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgo+IENjOiBNYXR0IFJvcGVyIDxtYXR0aGV3LmQu cm9wZXJAaW50ZWwuY29tPgo+IC0tLQo+IMKgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9wYXJh bXMuY8KgwqDCoHzCoMKgNSArKysrKwo+IMKgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9wYXJh bXMuaMKgwqDCoHzCoMKgMSArCj4gwqBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5 LmMgfCAxNiArKysrKysrKysrKysrLS0tCj4gwqAzIGZpbGVzIGNoYW5nZWQsIDE5IGluc2VydGlv bnMoKyksIDMgZGVsZXRpb25zKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9p OTE1L2k5MTVfcGFyYW1zLmMKPiBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfcGFyYW1zLmMK PiBpbmRleCA3NjhhZDg5Li5mNDYyY2Q0IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9p OTE1L2k5MTVfcGFyYW1zLmMKPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3BhcmFt cy5jCj4gQEAgLTYyLDYgKzYyLDcgQEAgc3RydWN0IGk5MTVfcGFyYW1zIGk5MTUgX19yZWFkX21v c3RseSA9IHsKPiDCoAkuaW5qZWN0X2xvYWRfZmFpbHVyZSA9IDAsCj4gwqAJLmVuYWJsZV9kcGNk X2JhY2tsaWdodCA9IGZhbHNlLAo+IMKgCS5lbmFibGVfZ3Z0ID0gZmFsc2UsCj4gKwkuZW5hYmxl X3NhZ3YgPSAtMSwKPiDCoH07Cj4gwqAKPiDCoG1vZHVsZV9wYXJhbV9uYW1lZChtb2Rlc2V0LCBp OTE1Lm1vZGVzZXQsIGludCwgMDQwMCk7Cj4gQEAgLTIzMywzICsyMzQsNyBAQCBNT0RVTEVfUEFS TV9ERVNDKGVuYWJsZV9kcGNkX2JhY2tsaWdodCwKPiDCoG1vZHVsZV9wYXJhbV9uYW1lZChlbmFi bGVfZ3Z0LCBpOTE1LmVuYWJsZV9ndnQsIGJvb2wsIDA0MDApOwo+IMKgTU9EVUxFX1BBUk1fREVT QyhlbmFibGVfZ3Z0LAo+IMKgCSJFbmFibGUgc3VwcG9ydCBmb3IgSW50ZWwgR1ZULWcgZ3JhcGhp Y3MgdmlydHVhbGl6YXRpb24gaG9zdAo+IHN1cHBvcnQoZGVmYXVsdDpmYWxzZSkiKTsKPiArCj4g K21vZHVsZV9wYXJhbV9uYW1lZF91bnNhZmUoZW5hYmxlX3NhZ3YsIGk5MTUuZW5hYmxlX3NhZ3Ys IGludCwgMDQwMCk7Cj4gK01PRFVMRV9QQVJNX0RFU0MoZW5hYmxlX3NhZ3YsCj4gKwkiRW5hYmxl IHRoZSBTQUdWIChnZW45KyBvbmx5KSgxPWVuYWJsZWQsIDA9ZGlzYWJsZWQsCj4gLTE9ZHJpdmVy IGRpc2NyZXRpb24gW2RlZmF1bHRdKSIpOwo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pOTE1X3BhcmFtcy5oCj4gYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3BhcmFtcy5o Cj4gaW5kZXggM2EwZGQ3OC4uYTdkYjEyNSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pOTE1X3BhcmFtcy5oCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9wYXJh bXMuaAo+IEBAIC02NSw2ICs2NSw3IEBAIHN0cnVjdCBpOTE1X3BhcmFtcyB7Cj4gwqAJYm9vbCBl bmFibGVfZHBfbXN0Owo+IMKgCWJvb2wgZW5hYmxlX2RwY2RfYmFja2xpZ2h0Owo+IMKgCWJvb2wg ZW5hYmxlX2d2dDsKPiArCWludCBlbmFibGVfc2FndjsKPiDCoH07Cj4gwqAKPiDCoGV4dGVybiBz dHJ1Y3QgaTkxNV9wYXJhbXMgaTkxNSBfX3JlYWRfbW9zdGx5Owo+IGRpZmYgLS1naXQgYS9kcml2 ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmMKPiBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1 L2ludGVsX2Rpc3BsYXkuYwo+IGluZGV4IGE3MWQwNWEuLmRkMTVhZTIgMTAwNjQ0Cj4gLS0tIGEv ZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5jCj4gKysrIGIvZHJpdmVycy9ncHUv ZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5jCj4gQEAgLTE2OTA0LDEyICsxNjkwNCwyMiBAQCBpbnRl bF9tb2Rlc2V0X3NldHVwX2h3X3N0YXRlKHN0cnVjdAo+IGRybV9kZXZpY2UgKmRldikKPiDCoAkJ cGxsLT5vbiA9IGZhbHNlOwo+IMKgCX0KPiDCoAo+IC0JaWYgKElTX1ZBTExFWVZJRVcoZGV2KSB8 fCBJU19DSEVSUllWSUVXKGRldikpCj4gKwlpZiAoSVNfVkFMTEVZVklFVyhkZXYpIHx8IElTX0NI RVJSWVZJRVcoZGV2KSkgewo+IMKgCQl2bHZfd21fZ2V0X2h3X3N0YXRlKGRldik7Cj4gLQllbHNl IGlmIChJU19HRU45KGRldikpCj4gKwl9IGVsc2UgaWYgKElTX0dFTjkoZGV2KSkgewo+IMKgCQlz a2xfd21fZ2V0X2h3X3N0YXRlKGRldik7Cj4gLQllbHNlIGlmIChIQVNfUENIX1NQTElUKGRldikp Cj4gKwo+ICsJCWlmIChpOTE1LmVuYWJsZV9zYWd2ICE9IC0xKSB7Cj4gKwkJCWlmIChpOTE1LmVu YWJsZV9zYWd2KQo+ICsJCQkJaW50ZWxfZW5hYmxlX3NhZ3YoZGV2X3ByaXYpOwo+ICsJCQllbHNl Cj4gKwkJCQlpbnRlbF9kaXNhYmxlX3NhZ3YoZGV2X3ByaXYpOwo+ICsKPiArCQkJZGV2X3ByaXYt PnNhZ3Zfc3RhdHVzID0KPiBJOTE1X1NBR1ZfTk9UX0NPTlRST0xMRUQ7CgpBZGRpbmcgdGhpcyBj b2RlIHRvIHRoZSBtaWRkbGUgb2YgYSBnZXRfaHdfc3RhdGUoKSBpZi1sYWRkZXIgZG9lc24ndApz ZWVtIHRvIGJlIHRoZSBiZXN0IGFwcHJvYWNoLiBNeSBzdWdnZXN0aW9uIHdvdWxkIGJlIHRvIGNy ZWF0ZQppbnRlbF9zZXR1cF9zYWd2KCkgKG9yIGludGVsX2luaXRfc2FndikgYW5kIHRoZW4gY2Fs bCBpdCBmcm9tIHNvbWV3aGVyZQoobWF5YmUgdGhlIGVuZCBvZiB0aGlzIGZ1bmN0aW9uPykuCgpB bHNvLCBJOTE1X1NBR1ZfTk9UX0NPTlRST0xMRUQgaXMgb25seSB1c2VkIG9uIFNreWxha2UuIEJ5 IHNldHRpbmcKc2Fndl9zdGF0dXMgdG8gdG8geW91J3JlIG1ha2luZyBpOTE1LmVuYWJsZV9zYWd2 IGJlaGF2ZSBkaWZmZXJlbnRseSBvbgpTS0wgY29tcGFyZWQgdG8gImFsbCB0aGUgb3RoZXIiIChh a2Egb25seSBLQkwgbm93KSBwbGF0Zm9ybXMuIEl0IHdvdWxkCnByb2JhYmx5IGJlIGJldHRlciB0 byBoYXZlIHVuaWZpZWQgYmVoYXZpb3IsIG1heWJlIGJ5IHJld29ya2luZyB0aGUKSTkxNV9TQUdW X05PVF9DT05UUk9MTEVEIGhhbmRsaW5nIG9yIGp1c3QgYWRkaW5nIGEgbmV3IGZsYWcgb3IKc29t ZXRoaW5nIGVsc2UuCgoKPiArCQl9Cj4gKwl9IGVsc2UgaWYgKEhBU19QQ0hfU1BMSVQoZGV2KSkg ewo+IMKgCQlpbGtfd21fZ2V0X2h3X3N0YXRlKGRldik7Cj4gKwl9Cj4gwqAKPiDCoAlmb3JfZWFj aF9pbnRlbF9jcnRjKGRldiwgY3J0Yykgewo+IMKgCQl1bnNpZ25lZCBsb25nIHB1dF9kb21haW5z OwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1n ZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9s aXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932230AbcJETdX (ORCPT ); Wed, 5 Oct 2016 15:33:23 -0400 Received: from mga05.intel.com ([192.55.52.43]:61577 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753624AbcJETdU (ORCPT ); Wed, 5 Oct 2016 15:33:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,302,1473145200"; d="scan'208";a="16648286" Message-ID: <1475695951.2381.48.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add enable_sagv option From: Paulo Zanoni To: Lyude , intel-gfx@lists.freedesktop.org Cc: David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter Date: Wed, 05 Oct 2016 16:32:31 -0300 In-Reply-To: <1475681598-12081-4-git-send-email-cpaul@redhat.com> References: <1475681598-12081-1-git-send-email-cpaul@redhat.com> <1475681598-12081-4-git-send-email-cpaul@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu: > This option allows us to manually control the SAGV at module load > time. > This can be useful in situations such as trying to debug watermark > changes, since enabled SAGV + incorrect watermarks = total GPU > annihilation. I'm not a huge fan of adding options that are only for very limited debugging situations, especially simple ones that can always just be re-implemented during debugging sessions such as this one. Anyway, I'm not opposed to adding the option since it's marked as unsafe anyway, I'm just stating my general opinion. See more below. > > Signed-off-by: Lyude > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Matt Roper > --- >  drivers/gpu/drm/i915/i915_params.c   |  5 +++++ >  drivers/gpu/drm/i915/i915_params.h   |  1 + >  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++--- >  3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 768ad89..f462cd4 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = { >   .inject_load_failure = 0, >   .enable_dpcd_backlight = false, >   .enable_gvt = false, > + .enable_sagv = -1, >  }; >   >  module_param_named(modeset, i915.modeset, int, 0400); > @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, >  module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); >  MODULE_PARM_DESC(enable_gvt, >   "Enable support for Intel GVT-g graphics virtualization host > support(default:false)"); > + > +module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400); > +MODULE_PARM_DESC(enable_sagv, > + "Enable the SAGV (gen9+ only)(1=enabled, 0=disabled, > -1=driver discretion [default])"); > diff --git a/drivers/gpu/drm/i915/i915_params.h > b/drivers/gpu/drm/i915/i915_params.h > index 3a0dd78..a7db125 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -65,6 +65,7 @@ struct i915_params { >   bool enable_dp_mst; >   bool enable_dpcd_backlight; >   bool enable_gvt; > + int enable_sagv; >  }; >   >  extern struct i915_params i915 __read_mostly; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a71d05a..dd15ae2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct > drm_device *dev) >   pll->on = false; >   } >   > - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) > + if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { >   vlv_wm_get_hw_state(dev); > - else if (IS_GEN9(dev)) > + } else if (IS_GEN9(dev)) { >   skl_wm_get_hw_state(dev); > - else if (HAS_PCH_SPLIT(dev)) > + > + if (i915.enable_sagv != -1) { > + if (i915.enable_sagv) > + intel_enable_sagv(dev_priv); > + else > + intel_disable_sagv(dev_priv); > + > + dev_priv->sagv_status = > I915_SAGV_NOT_CONTROLLED; Adding this code to the middle of a get_hw_state() if-ladder doesn't seem to be the best approach. My suggestion would be to create intel_setup_sagv() (or intel_init_sagv) and then call it from somewhere (maybe the end of this function?). Also, I915_SAGV_NOT_CONTROLLED is only used on Skylake. By setting sagv_status to to you're making i915.enable_sagv behave differently on SKL compared to "all the other" (aka only KBL now) platforms. It would probably be better to have unified behavior, maybe by reworking the I915_SAGV_NOT_CONTROLLED handling or just adding a new flag or something else. > + } > + } else if (HAS_PCH_SPLIT(dev)) { >   ilk_wm_get_hw_state(dev); > + } >   >   for_each_intel_crtc(dev, crtc) { >   unsigned long put_domains;