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=-3.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI,MIME_HTML_MOSTLY,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=no 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 9985EC3F2CE for ; Sat, 29 Feb 2020 09:34:29 +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 77C902072A for ; Sat, 29 Feb 2020 09:34:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 77C902072A 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 132446E196; Sat, 29 Feb 2020 09:34:28 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 086866E196 for ; Sat, 29 Feb 2020 09:34:26 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Feb 2020 01:34:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,499,1574150400"; d="scan'208,217";a="227803346" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga007.jf.intel.com with ESMTP; 29 Feb 2020 01:34:24 -0800 Received: from irsmsx603.ger.corp.intel.com (163.33.146.9) by IRSMSX107.ger.corp.intel.com (163.33.3.99) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sat, 29 Feb 2020 09:34:23 +0000 Received: from irsmsx604.ger.corp.intel.com (163.33.146.137) by irsmsx603.ger.corp.intel.com (163.33.146.9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Sat, 29 Feb 2020 09:34:23 +0000 Received: from irsmsx604.ger.corp.intel.com ([163.33.146.137]) by IRSMSX604.ger.corp.intel.com ([163.33.146.137]) with mapi id 15.01.1713.004; Sat, 29 Feb 2020 09:34:23 +0000 From: "Lisovskiy, Stanislav" To: =?iso-8859-1?Q?Ville_Syrj=E4l=E4?= Thread-Topic: [PATCH v18 4/8] drm/i915: Introduce more *_state_changed indicators Thread-Index: AQHV6ygcvVP4sP4SqEO3nuWy072ucqgsAYGAgAM5qoCAARfBAIAAeosAgAEdFX4= Date: Sat, 29 Feb 2020 09:34:23 +0000 Message-ID: References: <20200224153240.9047-5-stanislav.lisovskiy@intel.com> <20200225145733.32043-1-stanislav.lisovskiy@intel.com> <20200227161243.GR13686@intel.com> , <20200228161236.GH13686@intel.com> In-Reply-To: <20200228161236.GH13686@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.253.164] MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH v18 4/8] drm/i915: Introduce more *_state_changed indicators 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" Content-Type: multipart/mixed; boundary="===============0476079982==" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" --===============0476079982== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_ceb355b86ad84eeba8817b53713fe27aintelcom_" --_000_ceb355b86ad84eeba8817b53713fe27aintelcom_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable >> But this patch is not about that - it is about how we signal/determine >> that some change has to be written at commit stage. >>As you remember when we were discussed offline, I just wanted to have >> some expicit way to mark if some global state subsystem had changed, >>without having to do any additional checks, because imho all the checks >> we should do during atomic check, while commit simply applies what has >> to be applied. >> >>If you are really against having those boolean or any other way to be >>able so simply mark some stage object "dirty" (just like mem pages >>analogy) then would vote at least to have some helper functions to do >>that. >>i.e smth like: >> >>bool pipe_sagv_mask_changed(..) >This is just a !=3D, no? Don't see a function really making it any more cl= ear. it is more compact at least(imho) and also it makes it more clear why we do this "!=3D". >> >> bool ddb_state_changed(...) >So far I don't see any real need to check for that. The idea behind this is that we need to recompute SAGV only when either active pipes had changed or wm/ddb allocations had changed(lets say we now use a different mode or less planes and so on). Currently we have _no_ flag that indicates if ddb/wm had changed and recomputing SAGV everytime "just in case" looks redundant. To me it looks easier rather than having comparisons with implicit meaning. Would be even nicer to unify that idea in a sense that any global object like bw_state, dbuf_state can be checked if it's state had changed and to have it as some helper functions for that in intel_global_state.c or someth= ing like that. If you can propose a better way, please do: I think you got the idea, what I mean. ________________________________ From: Ville Syrj=E4l=E4 Sent: Friday, February 28, 2020 6:12:36 PM To: Lisovskiy, Stanislav Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D Subject: Re: [PATCH v18 4/8] drm/i915: Introduce more *_state_changed indic= ators On Fri, Feb 28, 2020 at 08:56:58AM +0000, Lisovskiy, Stanislav wrote: > On Thu, 2020-02-27 at 18:12 +0200, Ville Syrj=E4l=E4 wrote: > > On Tue, Feb 25, 2020 at 04:57:33PM +0200, Stanislav Lisovskiy wrote: > > > The reasoning behind this is such that current dependencies > > > in the code are rather implicit in a sense, we have to constantly > > > check a bunch of different bits like state->modeset, > > > state->active_pipe_changes, which sometimes can indicate counter > > > intuitive changes. > > > > > > By introducing more fine grained state change tracking we achieve > > > better readability and dependency maintenance for the code. > > > > > > For example it is no longer needed to evaluate active_pipe_changes > > > to understand if there were changes for wm/ddb - lets just have > > > a correspondent bit in a state, called ddb_state_changed. > > > > > > active_pipe_changes just indicate whether there was some pipe added > > > or removed. Then we evaluate if wm/ddb had been changed. > > > Same for sagv/bw state. ddb changes may or may not affect if out > > > bandwidth constraints have been changed. > > > > > > v2: Add support for older Gens in order not to introduce > > > regressions > > > > > > Signed-off-by: Stanislav Lisovskiy > > > --- > > > drivers/gpu/drm/i915/display/intel_atomic.c | 2 ++ > > > drivers/gpu/drm/i915/display/intel_bw.c | 28 ++++++++++++++- > > > - > > > drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++---- > > > .../drm/i915/display/intel_display_types.h | 32 ++++++++++++--- > > > ---- > > > drivers/gpu/drm/i915/intel_pm.c | 5 ++- > > > 5 files changed, 62 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > > > b/drivers/gpu/drm/i915/display/intel_atomic.c > > > index d043057d2fa0..0db9c66d3c0f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > > > @@ -525,6 +525,8 @@ void intel_atomic_state_clear(struct > > > drm_atomic_state *s) > > > state->dpll_set =3D state->modeset =3D false; > > > state->global_state_changed =3D false; > > > state->active_pipes =3D 0; > > > + state->ddb_state_changed =3D false; > > > + state->bw_state_changed =3D false; > > > > Not really liking these. > > > > After some pondering I was thinking along the lines of something > > simple > > like this: > > > > struct bw_state { > > u8 sagv_reject; > > }; > > > > bw_check() > > { > > for_each_crtc_in_state() { > > if (sagv_possible(crtc_state)) > > new->sagv_reject &=3D ~BIT(pipe); > > else > > new->sagv_reject |=3D BIT(pipe); > > } > > > > calculate new->qgv_mask > > } > > This is exactly what's done in the next patch, except > that I store pipe, which are allowed to have SAGV, i.e: I think inverted mask idea leads to neater code because then we don't have to care which pipes are actually present in the hw and which are fused off/not present: sagv_reject =3D=3D 0 -> SAGV possible sagv_reject !=3D 0 -> SAGV not possible > > struct intel_bw_state { > struct intel_global_state base; > > + /* > + * Contains a bit mask, used to determine, whether > correspondent > + * pipe allows SAGV or not. > + */ > + u8 pipe_sagv_mask; > + > + /* > + * Used to determine if we already had calculated > + * SAGV mask for this state once. > + */ > + bool sagv_calculated; > + > + /* > + * Contains final SAGV decision based on current mask, > + * to prevent doing the same job over and over again. > + */ > + bool can_sagv; > + > > Also the mask is calculated almost exactly same way: > > static void icl_compute_sagv_mask(struct intel_atomic_state *state) > +{ > + struct intel_crtc *crtc; > + struct intel_crtc_state *new_crtc_state; > + int i; > + struct intel_bw_state *new_bw_state =3D > intel_bw_get_state(state); > + > + if (IS_ERR(new_bw_state)) { > + WARN(1, "Could not get bw_state\n"); > + return; > + } > + > + for_each_new_intel_crtc_in_state(state, crtc, > + new_crtc_state, i) { > + if (skl_can_enable_sagv_on_pipe(state, crtc->pipe)) > + new_bw_state->pipe_sagv_mask |=3D BIT(crtc- > >pipe); > + else > + new_bw_state->pipe_sagv_mask &=3D ~BIT(crtc- > >pipe); > + } > +} > > But this patch is not about that - it is about how we signal/determine > that some change has to be written at commit stage. > As you remember when we were discussed offline, I just wanted to have > some expicit way to mark if some global state subsystem had changed, > without having to do any additional checks, because imho all the checks > we should do during atomic check, while commit simply applies what has > to be applied. > > If you are really against having those boolean or any other way to be > able so simply mark some stage object "dirty" (just like mem pages > analogy) then would vote at least to have some helper functions to do > that. > i.e smth like: > > bool pipe_sagv_mask_changed(..) This is just a !=3D, no? Don't see a function really making it any more cle= ar. > > bool ddb_state_changed(...) So far I don't see any real need to check for that. > > Stan > > > > > > } > > > > > > struct intel_crtc_state * > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c > > > b/drivers/gpu/drm/i915/display/intel_bw.c > > > index bdad7476dc7b..d5be603b8b03 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > > @@ -424,9 +424,27 @@ int intel_bw_atomic_check(struct > > > intel_atomic_state *state) > > > struct intel_crtc *crtc; > > > int i, ret; > > > > > > - /* FIXME earlier gens need some checks too */ > > > - if (INTEL_GEN(dev_priv) < 11) > > > + /* > > > + * For earlier Gens let's consider bandwidth changed if ddb > > > requirements, > > > + * has been changed. > > > + */ > > > + if (INTEL_GEN(dev_priv) < 11) { > > > + if (state->ddb_state_changed) { > > > + bw_state =3D intel_bw_get_state(state); > > > + if (IS_ERR(bw_state)) > > > + return PTR_ERR(bw_state); > > > + > > > + ret =3D intel_atomic_lock_global_state(&bw_state- > > > >base); > > > + if (ret) > > > + return ret; > > > + > > > + DRM_DEBUG_KMS("Marking bw state changed for > > > atomic state %p\n", > > > + state); > > > + > > > + state->bw_state_changed =3D true; > > > + } > > > return 0; > > > + } > > > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > > old_crtc_state, > > > new_crtc_state, i) { > > > @@ -447,7 +465,7 @@ int intel_bw_atomic_check(struct > > > intel_atomic_state *state) > > > old_active_planes =3D=3D new_active_planes) > > > continue; > > > > > > - bw_state =3D intel_bw_get_state(state); > > > + bw_state =3D intel_bw_get_state(state); > > > if (IS_ERR(bw_state)) > > > return PTR_ERR(bw_state); > > > > > > @@ -468,6 +486,10 @@ int intel_bw_atomic_check(struct > > > intel_atomic_state *state) > > > if (ret) > > > return ret; > > > > > > + DRM_DEBUG_KMS("Marking bw state changed for atomic state %p\n", > > > state); > > > + > > > + state->bw_state_changed =3D true; > > > + > > > data_rate =3D intel_bw_data_rate(dev_priv, bw_state); > > > num_active_planes =3D intel_bw_num_active_planes(dev_priv, > > > bw_state); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 3031e64ee518..137fb645097a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -15540,8 +15540,10 @@ static void > > > intel_atomic_commit_tail(struct intel_atomic_state *state) > > > * SKL workaround: bspec recommends we disable the SAGV > > > when we > > > * have more then one pipe enabled > > > */ > > > - if (!intel_can_enable_sagv(state)) > > > - intel_disable_sagv(dev_priv); > > > + if (state->bw_state_changed) { > > > + if (!intel_can_enable_sagv(state)) > > > + intel_disable_sagv(dev_priv); > > > + } > > > > > > intel_modeset_verify_disabled(dev_priv, state); > > > } > > > @@ -15565,7 +15567,7 @@ static void intel_atomic_commit_tail(struct > > > intel_atomic_state *state) > > > intel_encoders_update_prepare(state); > > > > > > /* Enable all new slices, we might need */ > > > - if (state->modeset) > > > + if (state->ddb_state_changed) > > > icl_dbuf_slice_pre_update(state); > > > > > > /* Now enable the clocks, plane, pipe, and connectors that we > > > set up. */ > > > @@ -15622,7 +15624,7 @@ static void intel_atomic_commit_tail(struct > > > intel_atomic_state *state) > > > } > > > > > > /* Disable all slices, we don't need */ > > > - if (state->modeset) > > > + if (state->ddb_state_changed) > > > icl_dbuf_slice_post_update(state); > > > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > > old_crtc_state, new_crtc_state, i) { > > > @@ -15641,8 +15643,10 @@ static void > > > intel_atomic_commit_tail(struct intel_atomic_state *state) > > > if (state->modeset) > > > intel_verify_planes(state); > > > > > > - if (state->modeset && intel_can_enable_sagv(state)) > > > - intel_enable_sagv(dev_priv); > > > + if (state->bw_state_changed) { > > > + if (intel_can_enable_sagv(state) > > > + intel_enable_sagv(dev_priv); > > > + } > > > > > > drm_atomic_helper_commit_hw_done(&state->base); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 0d8a64305464..12b47ba3c68d 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -471,16 +471,6 @@ struct intel_atomic_state { > > > > > > bool dpll_set, modeset; > > > > > > - /* > > > - * Does this transaction change the pipes that are > > > active? This mask > > > - * tracks which CRTC's have changed their active state at the > > > end of > > > - * the transaction (not counting the temporary disable during > > > modesets). > > > - * This mask should only be non-zero when intel_state->modeset > > > is true, > > > - * but the converse is not necessarily true; simply changing a > > > mode may > > > - * not flip the final active status of any CRTC's > > > - */ > > > - u8 active_pipe_changes; > > > - > > > u8 active_pipes; > > > > > > struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS]; > > > @@ -494,10 +484,30 @@ struct intel_atomic_state { > > > bool rps_interactive; > > > > > > /* > > > - * active_pipes > > > + * active pipes > > > */ > > > bool global_state_changed; > > > > > > + /* > > > + * Does this transaction change the pipes that are > > > active? This mask > > > + * tracks which CRTC's have changed their active state at the > > > end of > > > + * the transaction (not counting the temporary disable during > > > modesets). > > > + * This mask should only be non-zero when intel_state->modeset > > > is true, > > > + * but the converse is not necessarily true; simply changing a > > > mode may > > > + * not flip the final active status of any CRTC's > > > + */ > > > + u8 active_pipe_changes; > > > + > > > + /* > > > + * More granular change indicator for ddb changes > > > + */ > > > + bool ddb_state_changed; > > > + > > > + /* > > > + * More granular change indicator for bandwidth state changes > > > + */ > > > + bool bw_state_changed; > > > + > > > /* Number of enabled DBuf slices */ > > > u8 enabled_dbuf_slices_mask; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 409b91c17a7f..ac4b317ea1bf 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3894,7 +3894,7 @@ skl_ddb_get_pipe_allocation_limits(struct > > > drm_i915_private *dev_priv, > > > * that changes the active CRTC list or do modeset would need > > > to > > > * grab _all_ crtc locks, including the one we currently hold. > > > */ > > > - if (!intel_state->active_pipe_changes && !intel_state->modeset) > > > { > > > + if (!intel_state->ddb_state_changed) { > > > /* > > > * alloc may be cleared by clear_intel_crtc_state, > > > * copy from old state to be sure > > > @@ -5787,6 +5787,9 @@ static int skl_wm_add_affected_planes(struct > > > intel_atomic_state *state, > > > return PTR_ERR(plane_state); > > > > > > new_crtc_state->update_planes |=3D BIT(plane_id); > > > + > > > + DRM_DEBUG_KMS("Marking ddb state changed for atomic > > > state %p\n", state); > > > + state->ddb_state_changed =3D true; > > > } > > > > > > return 0; > > > -- > > > 2.24.1.485.gad05a3d8e5 > > > > -- Ville Syrj=E4l=E4 Intel --_000_ceb355b86ad84eeba8817b53713fe27aintelcom_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

>> But this patch is not about that - it is a= bout how we signal/determine
>> that some change has to be written at commit = stage.
>>As you remember when we were discussed offline= , I just wanted to have
>> some expicit way to mark if some global state= subsystem had changed,
>>without having to do any additional checks, be= cause imho all the checks
>> we should do during atomic check, while commi= t simply applies what has
>> to be applied.
>>
>>If you are really against having those boolean= or any other way to be
>>able so simply mark some stage object "di= rty" (just like mem pages
>>analogy) then would vote at least to have some= helper functions to do
>>that.
>>i.e smth like:
>>
>>bool pipe_sagv_mask_changed(..)

>This is just a !=3D, no? Don't see a function r= eally making it any more clear.


it is more compact at least(imho) and als= o it makes it more clear why

we do this "!=3D".  

>>
>> bool ddb_state_changed(...)

>So far I don't see any real need to check for that= .


The idea behind this is that we need to recompute SAGV only

when either active pipes had changed or wm/ddb allocations had changed(l= ets

say we now use a different mode or less planes and so on).


Currently we have _no_ flag that indicates if ddb/wm had changed and&nbs= p;

recomputing SAGV everytime "just in case" looks redundant.


To me it looks easier rather than having comparisons with implicit

meaning.  

Would be even nicer to unify that idea in= a sense that any global object

like bw_state, dbuf_state can be checked = if it's state had changed and to

have it as some helper functions for that= in intel_global_state.c or something like that.

 

If you can propose a better way, please d= o: I think you got the idea,

what I mean.


From: Ville Syrj=E4l=E4 &= lt;ville.syrjala@linux.intel.com>
Sent: Friday, February 28, 2020 6:12:36 PM
To: Lisovskiy, Stanislav
Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D
Subject: Re: [PATCH v18 4/8] drm/i915: Introduce more *_state_change= d indicators
 
On Fri, Feb 28, 2020 at 08:56:58AM +0000, Liso= vskiy, Stanislav wrote:
> On Thu, 2020-02-27 at 18:12 +0200, Ville Syrj=E4l=E4 wrote:
> > On Tue, Feb 25, 2020 at 04:57:33PM +0200, Stanislav Lisovskiy= wrote:
> > > The reasoning behind this is such that current dependencies<= br> > > > in the code are rather implicit in a sense, we have to const= antly
> > > check a bunch of different bits like state->modeset,
> > > state->active_pipe_changes, which sometimes can indicate = counter
> > > intuitive changes.
> > >
> > > By introducing more fine grained state change tracking we ac= hieve
> > > better readability and dependency maintenance for the code.<= br> > > >
> > > For example it is no longer needed to evaluate active_pipe_c= hanges
> > > to understand if there were changes for wm/ddb - lets just h= ave
> > > a correspondent bit in a state, called ddb_state_changed. > > >
> > > active_pipe_changes just indicate whether there was some pip= e added
> > > or removed. Then we evaluate if wm/ddb had been changed.
> > > Same for sagv/bw state. ddb changes may or may not affect if= out
> > > bandwidth constraints have been changed.
> > >
> > > v2: Add support for older Gens in order not to introduce
> > > regressions
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@i= ntel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_atomic.c  = ; |  2 ++
> > >  drivers/gpu/drm/i915/display/intel_bw.c  &nb= sp;    | 28 +++++++++= 3;++++-
> > > -
> > >  drivers/gpu/drm/i915/display/intel_display.c  | 1= 6 ++++++----
> > >  .../drm/i915/display/intel_display_types.h  =   | 32 ++++++++++++---=
> > > ----
> > >  drivers/gpu/drm/i915/intel_pm.c    = ;           |  5 = 3;+-
> > >  5 files changed, 62 insertions(+), 21 deletions(-)=
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index d043057d2fa0..0db9c66d3c0f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.= c
> > > @@ -525,6 +525,8 @@ void intel_atomic_state_clear(struct=
> > > drm_atomic_state *s)
> > >    state->dpll_set =3D state->modeset = =3D false;
> > >    state->global_state_changed =3D false;<= br> > > >    state->active_pipes =3D 0;
> > > + state->ddb_state_changed =3D false;
> > > + state->bw_state_changed =3D false;
> >
> > Not really liking these.
> >
> > After some pondering I was thinking along the lines of something<= br> > > simple
> > like this:
> >
> > struct bw_state {
> >      u8 sagv_reject;
> > };
> >
> > bw_check()
> > {
> >      for_each_crtc_in_state() {
> >           =    if (sagv_possible(crtc_state))
> >           =            new->sagv_r= eject &=3D ~BIT(pipe);
> >           =    else
> >           =            new->sagv_r= eject |=3D BIT(pipe);
> >      }
> >
> >      calculate new->qgv_mask
> > }
>
> This is exactly what's done in the next patch, except
> that I store pipe, which are allowed to have SAGV, i.e:

I think inverted mask idea leads to neater code because then we
don't have to care which pipes are actually present in the hw
and which are fused off/not present:

sagv_reject =3D=3D 0 -> SAGV possible
sagv_reject !=3D 0 -> SAGV not possible

>
>  struct intel_bw_state {
>        struct intel_global_state ba= se;
> 
> +     /*
> +      * Contains a bit mask, used to det= ermine, whether
> correspondent
> +      * pipe allows SAGV or not.
> +      */
> +     u8 pipe_sagv_mask;
> +
> +     /*
> +      * Used to determine if we already = had calculated
> +      * SAGV mask for this state once. > +      */
> +     bool sagv_calculated;
> +
> +     /*
> +      * Contains final SAGV decision bas= ed on current mask,
> +      * to prevent doing the same job ov= er and over again.
> +      */
> +     bool can_sagv;
> +
>
> Also the mask is calculated almost exactly same way:
>
> static void icl_compute_sagv_mask(struct intel_atomic_state *state) > +{
> +     struct intel_crtc *crtc;
> +     struct intel_crtc_state *new_crtc_state;=
> +     int i;
> +     struct intel_bw_state *new_bw_state =3D<= br> > intel_bw_get_state(state);
> +
> +     if (IS_ERR(new_bw_state)) {
> +           = ;  WARN(1, "Could not get bw_state\n");
> +           = ;  return;
> +     }
> +
> +     for_each_new_intel_crtc_in_state(state, = crtc,
> +           = ;            &n= bsp;            = ;  new_crtc_state, i) {
> +           = ;  if (skl_can_enable_sagv_on_pipe(state, crtc->pipe))
> +           = ;          new_bw_state->pi= pe_sagv_mask |=3D BIT(crtc-
> >pipe);
> +           = ;  else
> +           = ;          new_bw_state->pi= pe_sagv_mask &=3D ~BIT(crtc-
> >pipe);
> +     }
> +}
>
> But this patch is not about that - it is about how we signal/determine=
> that some change has to be written at commit stage.
> As you remember when we were discussed offline, I just wanted to have<= br> > some expicit way to mark if some global state subsystem had changed, > without having to do any additional checks, because imho all the check= s
> we should do during atomic check, while commit simply applies what has=
> to be applied.
>
> If you are really against having those boolean or any other way to be<= br> > able so simply mark some stage object "dirty" (just like mem= pages
> analogy) then would vote at least to have some helper functions to do<= br> > that.
> i.e smth like:
>
> bool pipe_sagv_mask_changed(..)

This is just a !=3D, no? Don't see a function really making it any more cle= ar.

>
> bool ddb_state_changed(...)

So far I don't see any real need to check for that.

> 
> Stan
>
> >
> > >  }
> > > 
> > >  struct intel_crtc_state *
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c
> > > b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index bdad7476dc7b..d5be603b8b03 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > > @@ -424,9 +424,27 @@ int intel_bw_atomic_check(struct > > > intel_atomic_state *state)
> > >    struct intel_crtc *crtc;
> > >    int i, ret;
> > > 
> > > - /* FIXME earlier gens need some checks too */
> > > - if (INTEL_GEN(dev_priv) < 11)
> > > + /*
> > > +  * For earlier Gens let's consider bandwidth chan= ged if ddb
> > > requirements,
> > > +  * has been changed.
> > > +  */
> > > + if (INTEL_GEN(dev_priv) < 11) {
> > > +         if (st= ate->ddb_state_changed) {
> > > +         &= nbsp;       bw_state =3D intel_bw_get_state(s= tate);
> > > +         &= nbsp;       if (IS_ERR(bw_state))
> > > +         &= nbsp;           &nbs= p;   return PTR_ERR(bw_state);
> > > +
> > > +         &= nbsp;       ret =3D intel_atomic_lock_global_= state(&bw_state-
> > > >base);
> > > +         &= nbsp;       if (ret)
> > > +         &= nbsp;           &nbs= p;   return ret;
> > > +
> > > +         &= nbsp;       DRM_DEBUG_KMS("Marking bw st= ate changed for
> > > atomic state %p\n",
> > > +         &= nbsp;           &nbs= p;         state);
> > > +
> > > +         &= nbsp;       state->bw_state_changed =3D tr= ue;
> > > +         }
> > >          &= nbsp; return 0;
> > > + }
> > > 
> > >    for_each_oldnew_intel_crtc_in_state(state,= crtc,
> > > old_crtc_state,
> > >          &= nbsp;           &nbs= p;            &= nbsp;    new_crtc_state, i) {
> > > @@ -447,7 +465,7 @@ int intel_bw_atomic_check(struct
> > > intel_atomic_state *state)
> > >          &= nbsp;     old_active_planes =3D=3D new_active_planes) > > >          &= nbsp;         continue;
> > > 
> > > -         bw_state&n= bsp; =3D intel_bw_get_state(state);
> > > +         bw_sta= te =3D intel_bw_get_state(state);
> > >          &= nbsp; if (IS_ERR(bw_state))
> > >          &= nbsp;         return PTR_ERR(bw_sta= te);
> > > 
> > > @@ -468,6 +486,10 @@ int intel_bw_atomic_check(struct > > > intel_atomic_state *state)
> > >    if (ret)
> > >          &= nbsp; return ret;
> > > 
> > > + DRM_DEBUG_KMS("Marking bw state changed for atomi= c state %p\n",
> > > state);
> > > +
> > > + state->bw_state_changed =3D true;
> > > +
> > >    data_rate =3D intel_bw_data_rate(dev_priv,= bw_state);
> > >    num_active_planes =3D intel_bw_num_active_= planes(dev_priv,
> > > bw_state);
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 3031e64ee518..137fb645097a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display= .c
> > > @@ -15540,8 +15540,10 @@ static void
> > > intel_atomic_commit_tail(struct intel_atomic_state *state) > > >          &= nbsp;  * SKL workaround: bspec recommends we disable the SAGV
> > > when we
> > >          &= nbsp;  * have more then one pipe enabled
> > >          &= nbsp;  */
> > > -         if (!intel= _can_enable_sagv(state))
> > > -          = ;       intel_disable_sagv(dev_priv);
> > > +         if (st= ate->bw_state_changed) {
> > > +         &= nbsp;       if (!intel_can_enable_sagv(state)= )
> > > +         &= nbsp;           &nbs= p;   intel_disable_sagv(dev_priv);
> > > +         }
> > > 
> > >          &= nbsp; intel_modeset_verify_disabled(dev_priv, state);
> > >    }
> > > @@ -15565,7 +15567,7 @@ static void intel_atomic_commit_= tail(struct
> > > intel_atomic_state *state)
> > >          &= nbsp; intel_encoders_update_prepare(state);
> > > 
> > >    /* Enable all new slices, we might need */=
> > > - if (state->modeset)
> > > + if (state->ddb_state_changed)
> > >          &= nbsp; icl_dbuf_slice_pre_update(state);
> > > 
> > >    /* Now enable the clocks, plane, pipe, and= connectors that we
> > > set up. */
> > > @@ -15622,7 +15624,7 @@ static void intel_atomic_commit_= tail(struct
> > > intel_atomic_state *state)
> > >    }
> > > 
> > >    /* Disable all slices, we don't need */ > > > - if (state->modeset)
> > > + if (state->ddb_state_changed)
> > >          &= nbsp; icl_dbuf_slice_post_update(state);
> > > 
> > >    for_each_oldnew_intel_crtc_in_state(state,= crtc,
> > > old_crtc_state, new_crtc_state, i) {
> > > @@ -15641,8 +15643,10 @@ static void
> > > intel_atomic_commit_tail(struct intel_atomic_state *state) > > >    if (state->modeset)
> > >          &= nbsp; intel_verify_planes(state);
> > > 
> > > - if (state->modeset && intel_can_enable_sagv(sta= te))
> > > -         intel_enab= le_sagv(dev_priv);
> > > + if (state->bw_state_changed) {
> > > +         if (in= tel_can_enable_sagv(state)
> > > +         &= nbsp;       intel_enable_sagv(dev_priv);
> > > + }
> > > 
> > >    drm_atomic_helper_commit_hw_done(&stat= e->base);
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_type= s.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 0d8a64305464..12b47ba3c68d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display= _types.h
> > > @@ -471,16 +471,6 @@ struct intel_atomic_state {
> > > 
> > >    bool dpll_set, modeset;
> > > 
> > > - /*
> > > -  * Does this transaction change the pipes that are > > > active?  This mask
> > > -  * tracks which CRTC's have changed their active stat= e at the
> > > end of
> > > -  * the transaction (not counting the temporary disabl= e during
> > > modesets).
> > > -  * This mask should only be non-zero when intel_state= ->modeset
> > > is true,
> > > -  * but the converse is not necessarily true; simply c= hanging a
> > > mode may
> > > -  * not flip the final active status of any CRTC's
> > > -  */
> > > - u8 active_pipe_changes;
> > > -
> > >    u8 active_pipes;
> > > 
> > >    struct intel_shared_dpll_state shared_dpll= [I915_NUM_PLLS];
> > > @@ -494,10 +484,30 @@ struct intel_atomic_state {
> > >    bool rps_interactive;
> > > 
> > >    /*
> > > -  * active_pipes
> > > +  * active pipes
> > >     */
> > >    bool global_state_changed;
> > > 
> > > + /*
> > > +  * Does this transaction change the pipes that ar= e
> > > active?  This mask
> > > +  * tracks which CRTC's have changed their active = state at the
> > > end of
> > > +  * the transaction (not counting the temporary di= sable during
> > > modesets).
> > > +  * This mask should only be non-zero when intel_s= tate->modeset
> > > is true,
> > > +  * but the converse is not necessarily true; simp= ly changing a
> > > mode may
> > > +  * not flip the final active status of any CRTC's=
> > > +  */
> > > + u8 active_pipe_changes;
> > > +
> > > + /*
> > > +  * More granular change indicator for ddb changes=
> > > +  */
> > > + bool ddb_state_changed;
> > > +
> > > + /*
> > > +  * More granular change indicator for bandwidth s= tate changes
> > > +  */
> > > + bool bw_state_changed;
> > > +
> > >    /* Number of enabled DBuf slices */
> > >    u8 enabled_dbuf_slices_mask;
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 409b91c17a7f..ac4b317ea1bf 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3894,7 +3894,7 @@ skl_ddb_get_pipe_allocation_limits= (struct
> > > drm_i915_private *dev_priv,
> > >     * that changes the active CRTC list = or do modeset would need
> > > to
> > >     * grab _all_ crtc locks, including t= he one we currently hold.
> > >     */
> > > - if (!intel_state->active_pipe_changes && !intel= _state->modeset)
> > > {
> > > + if (!intel_state->ddb_state_changed) {
> > >          &= nbsp; /*
> > >          &= nbsp;  * alloc may be cleared by clear_intel_crtc_state,
> > >          &= nbsp;  * copy from old state to be sure
> > > @@ -5787,6 +5787,9 @@ static int skl_wm_add_affected_pla= nes(struct
> > > intel_atomic_state *state,
> > >          &= nbsp;         return PTR_ERR(plane_= state);
> > > 
> > >          &= nbsp; new_crtc_state->update_planes |=3D BIT(plane_id);
> > > +
> > > +         DRM_DE= BUG_KMS("Marking ddb state changed for atomic
> > > state %p\n", state);
> > > +         state-= >ddb_state_changed =3D true;
> > >    }
> > > 
> > >    return 0;
> > > --
> > > 2.24.1.485.gad05a3d8e5
> >
> >

--
Ville Syrj=E4l=E4
Intel
 --_000_ceb355b86ad84eeba8817b53713fe27aintelcom_-- --===============0476079982== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0476079982==--