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.6 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,URIBL_BLOCKED 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 40238C10DCE for ; Fri, 13 Mar 2020 08:42: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 099E820724 for ; Fri, 13 Mar 2020 08:42:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 099E820724 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 9DBF66E3AE; Fri, 13 Mar 2020 08:42:28 +0000 (UTC) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0E6D46E3AE for ; Fri, 13 Mar 2020 08:42:26 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Mar 2020 01:42:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,548,1574150400"; d="scan'208,217";a="442348106" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by fmsmga005.fm.intel.com with ESMTP; 13 Mar 2020 01:42:25 -0700 Received: from irsmsx606.ger.corp.intel.com (163.33.146.139) by IRSMSX154.ger.corp.intel.com (163.33.192.96) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 13 Mar 2020 08:42:24 +0000 Received: from irsmsx604.ger.corp.intel.com (163.33.146.137) by IRSMSX606.ger.corp.intel.com (163.33.146.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Fri, 13 Mar 2020 08:42:24 +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; Fri, 13 Mar 2020 08:42:24 +0000 From: "Lisovskiy, Stanislav" To: =?iso-8859-1?Q?Ville_Syrj=E4l=E4?= Thread-Topic: [PATCH v19 2/8] drm/i915: Introduce skl_plane_wm_level accessor. Thread-Index: AQHV9i4Qof7mn5aWakO08NrMTq/LhahDkfWAgAKkIwE= Date: Fri, 13 Mar 2020 08:42:23 +0000 Message-ID: <5172187f1f964161b9dbf483c6163d0b@intel.com> References: <20200309161204.17792-1-stanislav.lisovskiy@intel.com> <20200309161204.17792-3-stanislav.lisovskiy@intel.com>, <20200311160727.GA13686@intel.com> In-Reply-To: <20200311160727.GA13686@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.184.70.1] MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH v19 2/8] drm/i915: Introduce skl_plane_wm_level accessor. 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="===============1066917595==" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" --===============1066917595== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_5172187f1f964161b9dbf483c6163d0bintelcom_" --_000_5172187f1f964161b9dbf483c6163d0bintelcom_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable >> static int >> skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) >> { >> @@ -4606,22 +4618,29 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *c= rtc_state) >> */ >> for (level =3D ilk_wm_max_level(dev_priv); level >=3D 0; level--)= { >> blocks =3D 0; >> + >> for_each_plane_id_on_crtc(crtc, plane_id) { >> - const struct skl_plane_wm *wm =3D >> - &crtc_state->wm.skl.optimal.planes[plane_i= d]; >> + const struct skl_wm_level *wm_level; >> + const struct skl_wm_level *wm_uv_level; >> + int color_plane =3D 0; >These color_plane variables seems kinda pointless. I'd just pass 0/1 direc= tly >(pretty sure that's what we do elsewhere too). Nope. I have a different view - if this is allowed here. 0/1 passed into fu= nction are just a magic numbers with no meaning - that way you see at least what's the param name a= nd it's meaning. Again, _absolutely_ pointless arguing and potentially waste of time instead= of fixing some real thing. Whether those are variables or constants, doesn= 't make this code better or worse. >> /* >> * We only disable the watermarks for each plane = if >> @@ -4732,9 +4765,10 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cr= tc_state) >> * planes must be enabled before the level will = be used." >> * So this is actually safe to do. >> */ >> - if (wm->wm[level].min_ddb_alloc > total[plane_id] = || >> - wm->uv_wm[level].min_ddb_alloc > uv_total[plan= e_id]) >> - memset(&wm->wm[level], 0, sizeof(wm->wm[le= vel])); >> + if (wm_level->min_ddb_alloc > total[plane_id] || >> + wm_uv_level->min_ddb_alloc > uv_total[plane_id= ]) >> + memset(&wm->wm[level], 0, >> + sizeof(struct skl_wm_level)); > memset(wm_level, 0, sizeof(*wm_level)) ? Again - memset(wm_level, 0, sizeof(*wm_level)) and memset(wm_level, 0, size= of(struct skl_wm_level)) are absolutely identical constructs according to C= standard. And I know that you are going to say that sizeof(*wm_level) won't require t= o change type - well if you are changing the code anyway, this is trivial. If I have any freedom to express my point of view at all, I'm not going to = fix that. Why are we even wasting time for this kind of stuff? Aren't there more seri= ous problems no? I myself have already decades of experience in coding in a rather big proje= cts and big companies and can say that those kind of nitpicks are completel= y useless waste of time. Best Regards, Lisovskiy Stanislav ________________________________ From: Ville Syrj=E4l=E4 Sent: Wednesday, March 11, 2020 6:07:27 PM To: Lisovskiy, Stanislav Cc: intel-gfx@lists.freedesktop.org; Ausmus, James; Saarinen, Jani; Roper, = Matthew D Subject: Re: [PATCH v19 2/8] drm/i915: Introduce skl_plane_wm_level accesso= r. On Mon, Mar 09, 2020 at 06:11:58PM +0200, Stanislav Lisovskiy wrote: > For future Gen12 SAGV implementation we need to > seemlessly alter wm levels calculated, depending > on whether we are allowed to enable SAGV or not. > > So this accessor will give additional flexibility > to do that. > > Currently this accessor is still simply working > as "pass-through" function. This will be changed > in next coming patches from this series. > > v2: - plane_id -> plane->id(Ville Syrj=E4l=E4) > - Moved wm_level var to have more local scope > (Ville Syrj=E4l=E4) > - Renamed yuv to color_plane(Ville Syrj=E4l=E4) in > skl_plane_wm_level > > v3: - plane->id -> plane_id(this time for real, Ville Syrj=E4l=E4) > - Changed colorplane id type from boolean to int as index > (Ville Syrj=E4l=E4) > - Moved crtc_state param so that it is first now > (Ville Syrj=E4l=E4) > - Moved wm_level declaration to tigher scope in > skl_write_plane_wm(Ville Syrj=E4l=E4) > > Signed-off-by: Stanislav Lisovskiy > --- > drivers/gpu/drm/i915/intel_pm.c | 85 ++++++++++++++++++++++++++------- > 1 file changed, 67 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index c7928c870b0a..c72fa59a8302 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4547,6 +4547,18 @@ icl_get_total_relative_data_rate(struct intel_crtc= _state *crtc_state, > return total_data_rate; > } > > +static const struct skl_wm_level * > +skl_plane_wm_level(const struct intel_crtc_state *crtc_state, > + enum plane_id plane_id, > + int level, > + int color_plane) > +{ > + const struct skl_plane_wm *wm =3D > + &crtc_state->wm.skl.optimal.planes[plane_id]; > + > + return color_plane =3D=3D 0 ? &wm->wm[level] : &wm->uv_wm[level]; > +} > + > static int > skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > { > @@ -4606,22 +4618,29 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cr= tc_state) > */ > for (level =3D ilk_wm_max_level(dev_priv); level >=3D 0; level--) = { > blocks =3D 0; > + > for_each_plane_id_on_crtc(crtc, plane_id) { > - const struct skl_plane_wm *wm =3D > - &crtc_state->wm.skl.optimal.planes[plane_id= ]; > + const struct skl_wm_level *wm_level; > + const struct skl_wm_level *wm_uv_level; > + int color_plane =3D 0; These color_plane variables seems kinda pointless. I'd just pass 0/1 direct= ly (pretty sure that's what we do elsewhere too). > + > + wm_level =3D skl_plane_wm_level(crtc_state, plane_i= d, > + level, color_plane); > + wm_uv_level =3D skl_plane_wm_level(crtc_state, plan= e_id, > + level, color_plane= + 1); > > if (plane_id =3D=3D PLANE_CURSOR) { > - if (wm->wm[level].min_ddb_alloc > total[PLA= NE_CURSOR]) { > + if (wm_level->min_ddb_alloc > total[PLANE_C= URSOR]) { > drm_WARN_ON(&dev_priv->drm, > - wm->wm[level].min_ddb_a= lloc !=3D U16_MAX); > + wm_level->min_ddb_alloc= !=3D U16_MAX); > blocks =3D U32_MAX; > break; > } > continue; > } > > - blocks +=3D wm->wm[level].min_ddb_alloc; > - blocks +=3D wm->uv_wm[level].min_ddb_alloc; > + blocks +=3D wm_level->min_ddb_alloc; > + blocks +=3D wm_uv_level->min_ddb_alloc; > } > > if (blocks <=3D alloc_size) { > @@ -4644,10 +4663,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cr= tc_state) > * proportional to its relative data rate. > */ > for_each_plane_id_on_crtc(crtc, plane_id) { > - const struct skl_plane_wm *wm =3D > - &crtc_state->wm.skl.optimal.planes[plane_id]; > + const struct skl_wm_level *wm_level; > + const struct skl_wm_level *wm_uv_level; > u64 rate; > u16 extra; > + int color_plane =3D 0; > + > + wm_level =3D skl_plane_wm_level(crtc_state, plane_id, > + level, color_plane); > + wm_uv_level =3D skl_plane_wm_level(crtc_state, plane_id, > + level, color_plane + 1); > > if (plane_id =3D=3D PLANE_CURSOR) > continue; > @@ -4663,7 +4688,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc= _state) > extra =3D min_t(u16, alloc_size, > DIV64_U64_ROUND_UP(alloc_size * rate, > total_data_rate)); > - total[plane_id] =3D wm->wm[level].min_ddb_alloc + extra; > + total[plane_id] =3D wm_level->min_ddb_alloc + extra; > alloc_size -=3D extra; > total_data_rate -=3D rate; > > @@ -4674,7 +4699,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc= _state) > extra =3D min_t(u16, alloc_size, > DIV64_U64_ROUND_UP(alloc_size * rate, > total_data_rate)); > - uv_total[plane_id] =3D wm->uv_wm[level].min_ddb_alloc + ext= ra; > + uv_total[plane_id] =3D wm_uv_level->min_ddb_alloc + extra; > alloc_size -=3D extra; > total_data_rate -=3D rate; > } > @@ -4717,8 +4742,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crt= c_state) > */ > for (level++; level <=3D ilk_wm_max_level(dev_priv); level++) { > for_each_plane_id_on_crtc(crtc, plane_id) { > + const struct skl_wm_level *wm_level; > + const struct skl_wm_level *wm_uv_level; > struct skl_plane_wm *wm =3D > &crtc_state->wm.skl.optimal.planes[plane_i= d]; > + int color_plane =3D 0; > + > + wm_level =3D skl_plane_wm_level(crtc_state, plane_i= d, > + level, color_plane); > + wm_uv_level =3D skl_plane_wm_level(crtc_state, plan= e_id, > + level, color_plane= + 1); > > /* > * We only disable the watermarks for each plane i= f > @@ -4732,9 +4765,10 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crt= c_state) > * planes must be enabled before the level will b= e used." > * So this is actually safe to do. > */ > - if (wm->wm[level].min_ddb_alloc > total[plane_id] |= | > - wm->uv_wm[level].min_ddb_alloc > uv_total[plane= _id]) > - memset(&wm->wm[level], 0, sizeof(wm->wm[lev= el])); > + if (wm_level->min_ddb_alloc > total[plane_id] || > + wm_uv_level->min_ddb_alloc > uv_total[plane_id]= ) > + memset(&wm->wm[level], 0, > + sizeof(struct skl_wm_level)); memset(wm_level, 0, sizeof(*wm_level)) ? Hmm. Also wondering why we're not clearing wm_uv here as well. I suppose it might not mater since the hw doesn't use wm_uv (and I fixed the "did the wms change?" check to ignore it too). Bit might be nice to clear it for consistency. Should be a separate patch though. > > /* > * Wa_1408961008:icl, ehl > @@ -4742,9 +4776,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crt= c_state) > */ > if (IS_GEN(dev_priv, 11) && > level =3D=3D 1 && wm->wm[0].plane_en) { > - wm->wm[level].plane_res_b =3D wm->wm[0].pla= ne_res_b; > - wm->wm[level].plane_res_l =3D wm->wm[0].pla= ne_res_l; > - wm->wm[level].ignore_lines =3D wm->wm[0].ig= nore_lines; > + wm_level =3D skl_plane_wm_level(crtc_state,= plane_id, > + 0, color_plan= e); > + wm->wm[level].plane_res_b =3D > + wm_level->plane_res_b; > + wm->wm[level].plane_res_l =3D > + wm_level->plane_res_l; > + wm->wm[level].ignore_lines =3D > + wm_level->ignore_lines; I would suggest we want this to read something like: const struct skl_wm_level *wm_level0 =3D skl_plane_wm_level(...) wm_level->foo =3D wm_level0->foo; ... And with those we can throw out the 'wm' variable from this loop as well. > } > } > } > @@ -5358,8 +5397,13 @@ void skl_write_plane_wm(struct intel_plane *plane, > &crtc_state->wm.skl.plane_ddb_uv[plane_id]; > > for (level =3D 0; level <=3D max_level; level++) { > + const struct skl_wm_level *wm_level; > + int color_plane =3D 0; > + > + wm_level =3D skl_plane_wm_level(crtc_state, plane_id, level= , color_plane); > + > skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, leve= l), > - &wm->wm[level]); > + wm_level); > } > skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > &wm->trans_wm); > @@ -5392,8 +5436,13 @@ void skl_write_cursor_wm(struct intel_plane *plane= , > &crtc_state->wm.skl.plane_ddb_y[plane_id]; > > for (level =3D 0; level <=3D max_level; level++) { > + const struct skl_wm_level *wm_level; > + int color_plane =3D 0; > + > + wm_level =3D skl_plane_wm_level(crtc_state, plane_id, level= , color_plane); > + > skl_write_wm_level(dev_priv, CUR_WM(pipe, level), > - &wm->wm[level]); > + wm_level); > } > skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm); > > -- > 2.24.1.485.gad05a3d8e5 -- Ville Syrj=E4l=E4 Intel --_000_5172187f1f964161b9dbf483c6163d0bintelcom_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

>>  static int
>>  skl_allocate_pipe_ddb(struct intel_crtc= _state *crtc_state)
>>  {
>> @@ -4606,22 +4618,29 @@ skl_allocate_pipe= _ddb(struct intel_crtc_state *crtc_state)
>>       &nbs= p; */
>>        for= (level =3D ilk_wm_max_level(dev_priv); level >=3D 0; level--) {<= br style=3D"color:rgb(33,33,33); font-family:wf_segoe-ui_normal,"Segoe= UI","Segoe WP",Tahoma,Arial,sans-serif,serif,EmojiFont; fon= t-size:13.3333px"> >>       &nbs= p;        blocks =3D 0;
>> +
>>       &nbs= p;        for_each_plane_id_on_crtc(crtc= , plane_id) {
>> -       &n= bsp;            = ; const struct skl_plane_wm *wm =3D
>> -       &n= bsp;            = ;         &crtc_state->wm.sk= l.optimal.planes[plane_id];
>> +      &nbs= p;            &= nbsp; const struct skl_wm_level *wm_level;
>> +      &nbs= p;            &= nbsp; const struct skl_wm_level *wm_uv_level;
>> +      &nbs= p;            &= nbsp; int color_plane =3D 0;

>These color_plane variables seems kinda pointless.= I'd just pass 0/1 directly
>(pretty sure that's what we do elsewhere too).


Nope. I have a different view - if this is all= owed here. 0/1 passed into function are just a magic

numbers with no meaning - that way you see at least= what's the param name and it's meaning.


Again, _absolutely_ pointless arguing and potential= ly waste of time instead of fixing some real thing. Whether those are variables or constants, doesn't make this code better or worse.<= /span>


>>   &nbs= p;            &= nbsp;       /*
>>       &nbs= p;            &= nbsp;    * We only disable the watermarks for each plane if<= /span>
>> @@ -4732,9 +4765,10 @@ skl_allocate_pipe_= ddb(struct intel_crtc_state *crtc_state)
>>            &nbs= p;            *  planes must be enabled = before the level will be used."
>>            &nbs= p;            * So this is actually safe to d= o.
>>            &nbs= p;            */
>> -       &n= bsp;            = ; if (wm->wm[level].min_ddb_alloc > total[plane_id] ||
>> -       &n= bsp;            = ;     wm->uv_wm[level].min_ddb_alloc > uv_total[p= lane_id])
>> -       &n= bsp;            = ;         memset(&wm->wm[lev= el], 0, sizeof(wm->wm[level]));
>> +      &nbs= p;            &= nbsp; if (wm_level->min_ddb_alloc > total[plane_id] ||
>> +      &nbs= p;            &= nbsp;     wm_uv_level->min_ddb_alloc > uv_total[p= lane_id])
>> +      &nbs= p;            &= nbsp;         memset(&wm->wm= [level], 0,
>> +      &nbs= p;            &= nbsp;           &nbs= p;    sizeof(struct skl_wm_level));

> memset(wm_level, 0, sizeof(*wm_level)) ?


Again - memset(wm_level, 0, sizeof(*wm_level)) and&nbs= p;memset(wm_level, 0, sizeof(struct skl_wm_level)) are absolutely identical constructs accord= ing to C standard.

And I know that you are going to say that sizeof(*wm_level) won't require to c= hange type - 

well if you are changing the code anyway, this is trivial.  

If I have any freedom to express my point of view at all, I'm not going to fi= x that.


Why are we even wasting time for this kind of stuff? Aren't there more serious= problems no?

I myself have already decades of experience in coding in a rather big p= rojects and big companies and can say that those kind of nitpicks are completely useless waste of time.



Best Regards,

Lisovskiy Stanislav
From: Ville Syrj=E4l=E4 &= lt;ville.syrjala@linux.intel.com>
Sent: Wednesday, March 11, 2020 6:07:27 PM
To: Lisovskiy, Stanislav
Cc: intel-gfx@lists.freedesktop.org; Ausmus, James; Saarinen, Jani; = Roper, Matthew D
Subject: Re: [PATCH v19 2/8] drm/i915: Introduce skl_plane_wm_level = accessor.
 
On Mon, Mar 09, 2020 at 06:11:58PM +0200, Stan= islav Lisovskiy wrote:
> For future Gen12 SAGV implementation we need to
> seemlessly alter wm levels calculated, depending
> on whether we are allowed to enable SAGV or not.
>
> So this accessor will give additional flexibility
> to do that.
>
> Currently this accessor is still simply working
> as "pass-through" function. This will be changed
> in next coming patches from this series.
>
> v2: - plane_id -> plane->id(Ville Syrj=E4l=E4)
>     - Moved wm_level var to have more local scope<= br> >       (Ville Syrj=E4l=E4)
>     - Renamed yuv to color_plane(Ville Syrj=E4l=E4= ) in
>       skl_plane_wm_level
>
> v3: - plane->id -> plane_id(this time for real, Ville Syrj=E4l= =E4)
>     - Changed colorplane id type from boolean to i= nt as index
>       (Ville Syrj=E4l=E4)
>     - Moved crtc_state param so that it is first n= ow
>       (Ville Syrj=E4l=E4)
>     - Moved wm_level declaration to tigher scope i= n
>       skl_write_plane_wm(Ville Syrj=E4l= =E4)
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com&g= t;
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 85 +++++&#= 43;++++++++++++++&#= 43;+++++-------
>  1 file changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/in= tel_pm.c
> index c7928c870b0a..c72fa59a8302 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4547,6 +4547,18 @@ icl_get_total_relative_data_rate(struct int= el_crtc_state *crtc_state,
>        return total_data_rate;
>  }
> 
> +static const struct skl_wm_level *
> +skl_plane_wm_level(const struct intel_crtc_state *crtc_state,
> +           = ;     enum plane_id plane_id,
> +           = ;     int level,
> +           = ;     int color_plane)
> +{
> +     const struct skl_plane_wm *wm =3D
> +           = ;  &crtc_state->wm.skl.optimal.planes[plane_id];
> +
> +     return color_plane =3D=3D 0 ? &wm-&g= t;wm[level] : &wm->uv_wm[level];
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  {
> @@ -4606,22 +4618,29 @@ skl_allocate_pipe_ddb(struct intel_crtc_st= ate *crtc_state)
>         */
>        for (level =3D ilk_wm_max_le= vel(dev_priv); level >=3D 0; level--) {
>            = ;    blocks =3D 0;
> +
>            = ;    for_each_plane_id_on_crtc(crtc, plane_id) {
> -           &nb= sp;         const struct skl_plane_= wm *wm =3D
> -           &nb= sp;            =      &crtc_state->wm.skl.optimal.planes[plane_id= ];
> +           = ;          const struct skl_wm= _level *wm_level;
> +           = ;          const struct skl_wm= _level *wm_uv_level;
> +           = ;          int color_plane =3D= 0;

These color_plane variables seems kinda pointless. I'd just pass 0/1 direct= ly
(pretty sure that's what we do elsewhere too).

> +
> +           = ;          wm_level =3D skl_pl= ane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;            &n= bsp;  level, color_plane);
> +           = ;          wm_uv_level =3D skl= _plane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;            &n= bsp;     level, color_plane + 1);
> 
>            = ;            if (pla= ne_id =3D=3D PLANE_CURSOR) {
> -           &nb= sp;            =      if (wm->wm[level].min_ddb_alloc > total[PLAN= E_CURSOR]) {
> +           = ;            &n= bsp;     if (wm_level->min_ddb_alloc > total[PLAN= E_CURSOR]) {
>            = ;            &n= bsp;            = ;   drm_WARN_ON(&dev_priv->drm,
> -           &nb= sp;            =             &nb= sp;            wm-&g= t;wm[level].min_ddb_alloc !=3D U16_MAX);
> +           = ;            &n= bsp;            = ;             w= m_level->min_ddb_alloc !=3D U16_MAX);
>            = ;            &n= bsp;            = ;   blocks =3D U32_MAX;
>            = ;            &n= bsp;            = ;   break;
>            = ;            &n= bsp;       }
>            = ;            &n= bsp;       continue;
>            = ;            }
> 
> -           &nb= sp;         blocks +=3D wm->= wm[level].min_ddb_alloc;
> -           &nb= sp;         blocks +=3D wm->= uv_wm[level].min_ddb_alloc;
> +           = ;          blocks +=3D wm_= level->min_ddb_alloc;
> +           = ;          blocks +=3D wm_= uv_level->min_ddb_alloc;
>            = ;    }
> 
>            = ;    if (blocks <=3D alloc_size) {
> @@ -4644,10 +4663,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_st= ate *crtc_state)
>         * proportional to its = relative data rate.
>         */
>        for_each_plane_id_on_crtc(cr= tc, plane_id) {
> -           &nb= sp; const struct skl_plane_wm *wm =3D
> -           &nb= sp;         &crtc_state->wm.= skl.optimal.planes[plane_id];
> +           = ;  const struct skl_wm_level *wm_level;
> +           = ;  const struct skl_wm_level *wm_uv_level;
>            = ;    u64 rate;
>            = ;    u16 extra;
> +           = ;  int color_plane =3D 0;
> +
> +           = ;  wm_level =3D skl_plane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;       level, color_plane);
> +           = ;  wm_uv_level =3D skl_plane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;          level, color_plane = + 1);
> 
>            = ;    if (plane_id =3D=3D PLANE_CURSOR)
>            = ;            continu= e;
> @@ -4663,7 +4688,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_stat= e *crtc_state)
>            = ;    extra =3D min_t(u16, alloc_size,
>            = ;            &n= bsp;     DIV64_U64_ROUND_UP(alloc_size * rate,
>            = ;            &n= bsp;            = ;            total_d= ata_rate));
> -           &nb= sp; total[plane_id] =3D wm->wm[level].min_ddb_alloc + extra;
> +           = ;  total[plane_id] =3D wm_level->min_ddb_alloc + extra;
>            = ;    alloc_size -=3D extra;
>            = ;    total_data_rate -=3D rate;
> 
> @@ -4674,7 +4699,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_stat= e *crtc_state)
>            = ;    extra =3D min_t(u16, alloc_size,
>            = ;            &n= bsp;     DIV64_U64_ROUND_UP(alloc_size * rate,
>            = ;            &n= bsp;            = ;            total_d= ata_rate));
> -           &nb= sp; uv_total[plane_id] =3D wm->uv_wm[level].min_ddb_alloc + extra; > +           = ;  uv_total[plane_id] =3D wm_uv_level->min_ddb_alloc + extra; >            = ;    alloc_size -=3D extra;
>            = ;    total_data_rate -=3D rate;
>        }
> @@ -4717,8 +4742,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_sta= te *crtc_state)
>         */
>        for (level++; level = <=3D ilk_wm_max_level(dev_priv); level++) {
>            = ;    for_each_plane_id_on_crtc(crtc, plane_id) {
> +           = ;          const struct skl_wm= _level *wm_level;
> +           = ;          const struct skl_wm= _level *wm_uv_level;
>            = ;            struct = skl_plane_wm *wm =3D
>            = ;            &n= bsp;       &crtc_state->wm.skl.optimal= .planes[plane_id];
> +           = ;          int color_plane =3D= 0;
> +
> +           = ;          wm_level =3D skl_pl= ane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;            &n= bsp;  level, color_plane);
> +           = ;          wm_uv_level =3D skl= _plane_wm_level(crtc_state, plane_id,
> +           = ;            &n= bsp;            = ;            &n= bsp;     level, color_plane + 1);
> 
>            = ;            /*
>            = ;             *= We only disable the watermarks for each plane if
> @@ -4732,9 +4765,10 @@ skl_allocate_pipe_ddb(struct intel_crtc_sta= te *crtc_state)
>            = ;             *=   planes must be enabled before the level will be used."
>            = ;             *= So this is actually safe to do.
>            = ;             *= /
> -           &nb= sp;         if (wm->wm[level].mi= n_ddb_alloc > total[plane_id] ||
> -           &nb= sp;            = wm->uv_wm[level].min_ddb_alloc > uv_total[plane_id])
> -           &nb= sp;            =      memset(&wm->wm[level], 0, sizeof(wm->wm[= level]));
> +           = ;          if (wm_level->mi= n_ddb_alloc > total[plane_id] ||
> +           = ;            &n= bsp; wm_uv_level->min_ddb_alloc > uv_total[plane_id])
> +           = ;            &n= bsp;     memset(&wm->wm[level], 0,
> +           = ;            &n= bsp;            size= of(struct skl_wm_level));

memset(wm_level, 0, sizeof(*wm_level)) ?

Hmm. Also wondering why we're not clearing wm_uv here as well. I suppose it might not mater since the hw doesn't use wm_uv (and I fixed the
"did the wms change?" check to ignore it too). Bit might be nice = to clear
it for consistency. Should be a separate patch though.

> 
>            = ;            /*
>            = ;             *= Wa_1408961008:icl, ehl
> @@ -4742,9 +4776,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_sta= te *crtc_state)
>            = ;             *= /
>            = ;            if (IS_= GEN(dev_priv, 11) &&
>            = ;            &n= bsp;   level =3D=3D 1 && wm->wm[0].plane_en) {
> -           &nb= sp;            =      wm->wm[level].plane_res_b =3D wm->wm[0].plan= e_res_b;
> -           &nb= sp;            =      wm->wm[level].plane_res_l =3D wm->wm[0].plan= e_res_l;
> -           &nb= sp;            =      wm->wm[level].ignore_lines =3D wm->wm[0].ign= ore_lines;
> +           = ;            &n= bsp;     wm_level =3D skl_plane_wm_level(crtc_state, pl= ane_id,
> +           = ;            &n= bsp;            = ;            &n= bsp;          0, color_plane);=
> +           = ;            &n= bsp;     wm->wm[level].plane_res_b =3D
> +           = ;            &n= bsp;            = ; wm_level->plane_res_b;
> +           = ;            &n= bsp;     wm->wm[level].plane_res_l =3D
> +           = ;            &n= bsp;            = ; wm_level->plane_res_l;
> +           = ;            &n= bsp;     wm->wm[level].ignore_lines =3D
> +           = ;            &n= bsp;            = ; wm_level->ignore_lines;

I would suggest we want this to read something like:

const struct skl_wm_level *wm_level0 =3D skl_plane_wm_level(...)

wm_level->foo =3D wm_level0->foo;
...

And with those we can throw out the 'wm' variable from this loop as
well.

>            = ;            }
>            = ;    }
>        }
> @@ -5358,8 +5397,13 @@ void skl_write_plane_wm(struct intel_plane = *plane,
>            = ;    &crtc_state->wm.skl.plane_ddb_uv[plane_id];
> 
>        for (level =3D 0; level <= =3D max_level; level++) {
> +           = ;  const struct skl_wm_level *wm_level;
> +           = ;  int color_plane =3D 0;
> +
> +           = ;  wm_level =3D skl_plane_wm_level(crtc_state, plane_id, level, color_= plane);
> +
>            = ;    skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, l= evel),
> -           &nb= sp;            =         &wm->wm[level]);
> +           = ;            &n= bsp;        wm_level);
>        }
>        skl_write_wm_level(dev_priv,= PLANE_WM_TRANS(pipe, plane_id),
>            = ;            &n= bsp;  &wm->trans_wm);
> @@ -5392,8 +5436,13 @@ void skl_write_cursor_wm(struct intel_plane= *plane,
>            = ;    &crtc_state->wm.skl.plane_ddb_y[plane_id];
> 
>        for (level =3D 0; level <= =3D max_level; level++) {
> +           = ;  const struct skl_wm_level *wm_level;
> +           = ;  int color_plane =3D 0;
> +
> +           = ;  wm_level =3D skl_plane_wm_level(crtc_state, plane_id, level, color_= plane);
> +
>            = ;    skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> -           &nb= sp;            =         &wm->wm[level]);
> +           = ;            &n= bsp;        wm_level);
>        }
>        skl_write_wm_level(dev_priv,= CUR_WM_TRANS(pipe), &wm->trans_wm);
> 
> --
> 2.24.1.485.gad05a3d8e5

--
Ville Syrj=E4l=E4
Intel
 --_000_5172187f1f964161b9dbf483c6163d0bintelcom_-- --===============1066917595== 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 --===============1066917595==--