From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 20673C433E0 for ; Mon, 1 Jun 2020 07:53:48 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F167820734 for ; Mon, 1 Jun 2020 07:53:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F167820734 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8A79D89DB7; Mon, 1 Jun 2020 07:53:47 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5CEED89DB7 for ; Mon, 1 Jun 2020 07:53:46 +0000 (UTC) IronPort-SDR: PA6FaBbhy3IqICYMo9oQixdlllHh7UG+m1OSnUJA0ADNc9Jh5KrOYUip7kgpDJ4K9c6NoQLi0v s1zQRZJeDEQQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2020 00:53:45 -0700 IronPort-SDR: 7UpIA9RmzNdjpaRmo4iXwHcbbk/7V8oD1v7IZmzitN9b0EUUEsujK+JXK0Q4oMRPRTzfwNky+F LT+HnMUqMbnQ== X-IronPort-AV: E=Sophos;i="5.73,460,1583222400"; d="scan'208";a="444204741" Received: from unknown (HELO intel.com) ([10.237.72.89]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2020 00:53:43 -0700 Date: Mon, 1 Jun 2020 10:49:54 +0300 From: "Lisovskiy, Stanislav" To: Manasi Navare Message-ID: <20200601074954.GA2239@intel.com> References: <20200526094852.6967-1-stanislav.lisovskiy@intel.com> <20200529235738.GA3731@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200529235738.GA3731@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [Intel-gfx] [PATCH v1] drm/i915: Fix wrong CDCLK adjustment changes X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, chris@chris-wilson.co.uk Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, May 29, 2020 at 04:57:38PM -0700, Manasi Navare wrote: > On Tue, May 26, 2020 at 12:48:52PM +0300, Stanislav Lisovskiy wrote: > > Previous patch didn't take into account all pipes > > but only those in state, which could cause wrong > > CDCLK conclcusions and calculations. > > Also there was a severe issue with min_cdclk being > > assigned to 0 every compare cycle. > > > > Too bad this was found by me only after merge. > > This could be also causing the issues in test, however > > not clear - anyway marking this as fixing the > > "Adjust CDCLK accordingly to our DBuf bw needs". > > > > Signed-off-by: Stanislav Lisovskiy > > Fixes: cd1915460861 ("Adjust CDCLK accordingly to our DBuf bw needs") > > --- > > drivers/gpu/drm/i915/display/intel_bw.c | 51 ++++++++++++-------- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 19 +++++--- > > drivers/gpu/drm/i915/display/intel_display.c | 26 +++++----- > > 3 files changed, 53 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > > index a79bd7aeb03b..8096138abecc 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.c > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > @@ -437,6 +437,7 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state) > > struct intel_crtc *crtc; > > int max_bw = 0; > > int slice_id; > > + enum pipe pipe; > > int i; > > > > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > @@ -447,7 +448,9 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state) > > if (IS_ERR(new_bw_state)) > > return PTR_ERR(new_bw_state); > > > > - crtc_bw = &new_bw_state->dbuf_bw[crtc->pipe]; > > + old_bw_state = intel_atomic_get_old_bw_state(state); > > + > > + crtc_bw = &new_bw_state->dbuf_bw[pipe]; > > > > memset(&crtc_bw->used_bw, 0, sizeof(crtc_bw->used_bw)); > > > > @@ -478,6 +481,15 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state) > > for_each_dbuf_slice_in_mask(slice_id, dbuf_mask) > > crtc_bw->used_bw[slice_id] += data_rate; > > } > > + } > > + > > + if (!old_bw_state) > > + return 0; > > + > > + for_each_pipe(dev_priv, pipe) { > > + struct intel_dbuf_bw *crtc_bw; > > + > > So the condition !old_bw_state() will make sure we loop through > only the active pipes and compute crtc_bw only for those right? > > Manasi Well, in fact this condition just checks if we had any crtcs in state - otherwise there were no changes, so bw_state global object doesn't need to be changed. Whenever something happens to crtc we should have it in the state, so this condition just checks if we need to modify bw_state or not. Regarding active/inactive pipes - currently for inactive pipes, we are going to get 0 dbuf slice mask, so we just won't accumulate any data rate for those. So if the pipe got disabled we will get less required min_cdclk which against old_bw_state, which will mean that we are going to acquire the global state lock for writing. In fact we could optimize the code by skipping inactive pipes completely i.e don't even calculate dbuf slice mask, which will be 0. However the logic and the end result would be the same anyway. Stan > > > + crtc_bw = &new_bw_state->dbuf_bw[pipe]; > > > > for_each_dbuf_slice(slice_id) { > > /* > > @@ -490,14 +502,9 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state) > > */ > > max_bw += crtc_bw->used_bw[slice_id]; > > } > > - > > - new_bw_state->min_cdclk = max_bw / 64; > > - > > - old_bw_state = intel_atomic_get_old_bw_state(state); > > } > > > > - if (!old_bw_state) > > - return 0; > > + new_bw_state->min_cdclk = max_bw / 64; > > > > if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) { > > int ret = intel_atomic_lock_global_state(&new_bw_state->base); > > @@ -511,34 +518,38 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state) > > > > int intel_bw_calc_min_cdclk(struct intel_atomic_state *state) > > { > > - int i; > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct intel_bw_state *new_bw_state = NULL; > > + struct intel_bw_state *old_bw_state = NULL; > > const struct intel_crtc_state *crtc_state; > > struct intel_crtc *crtc; > > int min_cdclk = 0; > > - struct intel_bw_state *new_bw_state = NULL; > > - struct intel_bw_state *old_bw_state = NULL; > > + enum pipe pipe; > > + int i; > > > > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > - struct intel_cdclk_state *cdclk_state; > > - > > new_bw_state = intel_atomic_get_bw_state(state); > > if (IS_ERR(new_bw_state)) > > return PTR_ERR(new_bw_state); > > > > - cdclk_state = intel_atomic_get_cdclk_state(state); > > - if (IS_ERR(cdclk_state)) > > - return PTR_ERR(cdclk_state); > > - > > - min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk); > > - > > - new_bw_state->min_cdclk = min_cdclk; > > - > > old_bw_state = intel_atomic_get_old_bw_state(state); > > } > > > > if (!old_bw_state) > > return 0; > > > > + for_each_pipe(dev_priv, pipe) { > > + struct intel_cdclk_state *cdclk_state; > > + > > + cdclk_state = intel_atomic_get_new_cdclk_state(state); > > + if (!cdclk_state) > > + return 0; > > + > > + min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk); > > + } > > + > > + new_bw_state->min_cdclk = min_cdclk; > > + > > if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) { > > int ret = intel_atomic_lock_global_state(&new_bw_state->base); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index f9b0fc7317de..08468b121d02 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2084,9 +2084,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state) > > { > > struct intel_atomic_state *state = cdclk_state->base.state; > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct intel_bw_state *bw_state = NULL; > > struct intel_crtc *crtc; > > struct intel_crtc_state *crtc_state; > > int min_cdclk, i; > > + enum pipe pipe; > > > > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > int ret; > > @@ -2095,6 +2098,10 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state) > > if (min_cdclk < 0) > > return min_cdclk; > > > > + bw_state = intel_atomic_get_bw_state(state); > > + if (IS_ERR(bw_state)) > > + return PTR_ERR(bw_state); > > + > > if (cdclk_state->min_cdclk[i] == min_cdclk) > > continue; > > > > @@ -2106,15 +2113,11 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state) > > } > > > > min_cdclk = cdclk_state->force_min_cdclk; > > + for_each_pipe(dev_priv, pipe) { > > + min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk); > > > > - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > - struct intel_bw_state *bw_state; > > - > > - min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk); > > - > > - bw_state = intel_atomic_get_bw_state(state); > > - if (IS_ERR(bw_state)) > > - return PTR_ERR(bw_state); > > + if (!bw_state) > > + continue; > > > > min_cdclk = max(bw_state->min_cdclk, min_cdclk); > > } > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index f40b909952cc..66af8f3053ed 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -14708,13 +14708,14 @@ static int intel_atomic_check_cdclk(struct intel_atomic_state *state, > > bool *need_cdclk_calc) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > - int i; > > + struct intel_cdclk_state *new_cdclk_state; > > struct intel_plane_state *plane_state; > > + struct intel_bw_state *new_bw_state; > > struct intel_plane *plane; > > + int min_cdclk = 0; > > + enum pipe pipe; > > int ret; > > - struct intel_cdclk_state *new_cdclk_state; > > - struct intel_crtc_state *new_crtc_state; > > - struct intel_crtc *crtc; > > + int i; > > /* > > * active_planes bitmask has been updated, and potentially > > * affected planes are part of the state. We can now > > @@ -14735,23 +14736,18 @@ static int intel_atomic_check_cdclk(struct intel_atomic_state *state, > > if (ret) > > return ret; > > > > - if (!new_cdclk_state) > > - return 0; > > - > > - for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > - struct intel_bw_state *bw_state; > > - int min_cdclk = 0; > > + new_bw_state = intel_atomic_get_new_bw_state(state); > > > > - min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk); > > + if (!new_cdclk_state || !new_bw_state) > > + return 0; > > > > - bw_state = intel_atomic_get_bw_state(state); > > - if (IS_ERR(bw_state)) > > - return PTR_ERR(bw_state); > > + for_each_pipe(dev_priv, pipe) { > > + min_cdclk = max(new_cdclk_state->min_cdclk[pipe], min_cdclk); > > > > /* > > * Currently do this change only if we need to increase > > */ > > - if (bw_state->min_cdclk > min_cdclk) > > + if (new_bw_state->min_cdclk > min_cdclk) > > *need_cdclk_calc = true; > > } > > > > -- > > 2.24.1.485.gad05a3d8e5 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx