| Message ID | 20241219173636.3377955-1-ravi.kumar.vodapalli@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: Don't program DBUF_CTL tracker state service | expand |
-----Original Message----- From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli Sent: Thursday, December 19, 2024 9:37 AM To: intel-gfx@lists.freedesktop.org Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar <haridhar.kalvala@intel.com>; Chauhan, Shekhar <shekhar.chauhan@intel.com> Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service > > While display initialization along with MBUS credits programming > DBUF_CTL register is also programmed, as a part of it the tracker > state service field is also set to 0x8 value when default value is > other than 0x8 which are for platforms past display version 13. > For remaining platforms the default value is already 0x8 so don't > program them. This could use some rewording. Perhaps: """ For platforms past display version 13, during display initialization along with MBUS credits and DBUF_CTL register programming, the tracker state service field is set to a value of 0x8, which is not the default value for these platforms. All other platforms use 0x8 as the default value and thus do not need to be overwritten. """ Or maybe: """ During display initialization and MBUS credits programming, the DBUF_CTL register is also programmed. Specifically, when programming DBUF_CTL, the tracker state service field is set to the value 0x8. However, this is already the default value for platforms using display versions 13 and prior, so we do not need to program the DBUF_CTL register on those platforms. """ > > Bspec: 49213 > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 34465d56def0..98db721cba33 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) > { > enum dbuf_slice slice; > > - if (display->platform.alderlake_p) > - return; > - I take it we're removing this condition because we no longer expect this code to run on alderlake_p anyways? > for_each_dbuf_slice(display, slice) > intel_de_rmw(display, DBUF_CTL_S(slice), > DBUF_TRACKER_STATE_SERVICE_MASK, > @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, > /* 4. Enable CDCLK. */ > intel_cdclk_init_hw(display); > > - if (DISPLAY_VER(display) >= 12) > + if (DISPLAY_VER(display) == 12) If I'm understanding the purpose of this patch correctly (which I'm probably not), shouldn't this be "if (DISPLAY_VER(display) > 13)"? We only want to overwrite the tracker state service field on platforms after display version 13, and it seems like gen12_dbuf_slices_config overwrites the tracker state service field. -Jonathan Cavitt > gen12_dbuf_slices_config(display); > > /* 5. Enable DBUF. */ > -- > 2.25.1 > >
On Thu, Dec 19, 2024 at 11:39:07AM -0800, Cavitt, Jonathan wrote: > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli > Sent: Thursday, December 19, 2024 9:37 AM > To: intel-gfx@lists.freedesktop.org > Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar <haridhar.kalvala@intel.com>; Chauhan, Shekhar <shekhar.chauhan@intel.com> > Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service > > > > While display initialization along with MBUS credits programming > > DBUF_CTL register is also programmed, as a part of it the tracker > > state service field is also set to 0x8 value when default value is > > other than 0x8 which are for platforms past display version 13. > > For remaining platforms the default value is already 0x8 so don't > > program them. > > This could use some rewording. Perhaps: > """ > For platforms past display version 13, during display initialization along > with MBUS credits and DBUF_CTL register programming, the tracker state > service field is set to a value of 0x8, which is not the default value for > these platforms. All other platforms use 0x8 as the default value and thus > do not need to be overwritten. > """ > > Or maybe: > """ > During display initialization and MBUS credits programming, the > DBUF_CTL register is also programmed. Specifically, when > programming DBUF_CTL, the tracker state service field is set to the > value 0x8. However, this is already the default value for platforms > using display versions 13 and prior, so we do not need to program > the DBUF_CTL register on those platforms. > """ I think these are still missing the point a bit. The key is that the bspec asks us to program non-default values _only_ on certain platforms. For all other platforms (both earlier and later), the expectation is that the hardware's default settings (whatever they happen to be for a given platform) are already correct and should not be adjusted. The code here was originally written back during gen12 development following the standard "assume driver changes will apply to all future platforms too until we know otherwise" design, so the original test was written as ">= 12." It looks like DG2 (one display version 13 platform) still needed the programming, but ADL-P (the other display version 13 platform) did not. From version 14 onward, no platforms need it. So the correct condition to match the hardware/bspec's rules would be: if (DISPLAY_VER(display) == 12 || display->platform.dg2) (I think on an earlier review of this I said it should be just "DISPLAY_VER(display) == 12" but I overlooked that DG2 is indeed included in the list of platforms on bspec page 49213). Once later platforms like ADL-P, MTL, etc. rolled around, it turned out that explicit programming was actually not expected to carry forward, and we should have adjusted the condition to just 12+DG2 at that time, but it was overlooked. That was technically a bug, but it turned out to be harmless because the explicit value we were programming (8) happened to match the new hardware defaults on display version 13. However we shouldn't count on it staying harmless forever --- if the hardware default ever changes to something else in the future, then it can cause problems if we're still explicitly programming it to "8" by accident; this patch is addressing that earlier oversight. > > > > > Bspec: 49213 > > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 34465d56def0..98db721cba33 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) > > { > > enum dbuf_slice slice; > > > > - if (display->platform.alderlake_p) > > - return; > > - > > I take it we're removing this condition because we no longer expect this code to run > on alderlake_p anyways? There's a platform/version check at the callsite and another here inside the function. We only need to do the check at one place or the other; that's somewhat independent of fixing the check(s) themselves to match the right platforms. > > > for_each_dbuf_slice(display, slice) > > intel_de_rmw(display, DBUF_CTL_S(slice), > > DBUF_TRACKER_STATE_SERVICE_MASK, > > @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, > > /* 4. Enable CDCLK. */ > > intel_cdclk_init_hw(display); > > > > - if (DISPLAY_VER(display) >= 12) > > + if (DISPLAY_VER(display) == 12) > > If I'm understanding the purpose of this patch correctly (which I'm probably not), > shouldn't this be "if (DISPLAY_VER(display) > 13)"? > We only want to overwrite the tracker state service field on platforms after display version 13, > and it seems like gen12_dbuf_slices_config overwrites the tracker state service field. No, this change here is [mostly] correct; explicit driver programming at display init time should only happen on 12 and DG2. The DG2 condition is missing (probably because I overlooked it and misspoke in an earlier code review). For all other platforms (both earlier and later versions) we should leave the MBUF_CTL registers at their hardware defaults and not touch them here. Also, don't confuse the code here (which adjusts the register at display init time) with the other (re)programming of these values that happens at runtime during modeset (adjusting based on how many pipes are active). There are separate rules later in the bspec page and separate code that handles that (correctly I believe); the patch here is just addressing the specific stanza of the bspec page that says "The MBus credits should be setup once with the following default values during the display initialization," and that block only applies to the various platforms that user display version 12, and then also to DG2. Matt > > -Jonathan Cavitt > > > gen12_dbuf_slices_config(display); > > > > /* 5. Enable DBUF. */ > > -- > > 2.25.1 > > > >
Quoting Cavitt, Jonathan (2024-12-19 16:39:07-03:00) >-----Original Message----- >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli >Sent: Thursday, December 19, 2024 9:37 AM >To: intel-gfx@lists.freedesktop.org >Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar <haridhar.kalvala@intel.com>; Chauhan, Shekhar <shekhar.chauhan@intel.com> >Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service Ravi, it would be better if new revisions of the patch have version specifiers in the subject (you can use git format-patch -v for that) and the commit message had the change log of what changed across versions of the patch. >> >> While display initialization along with MBUS credits programming >> DBUF_CTL register is also programmed, as a part of it the tracker >> state service field is also set to 0x8 value when default value is >> other than 0x8 which are for platforms past display version 13. >> For remaining platforms the default value is already 0x8 so don't >> program them. > >This could use some rewording. Perhaps: >""" >For platforms past display version 13, during display initialization along >with MBUS credits and DBUF_CTL register programming, the tracker state >service field is set to a value of 0x8, which is not the default value for >these platforms. All other platforms use 0x8 as the default value and thus >do not need to be overwritten. >""" Note that DBUF_TRACKER_STATE_SERVICE is set only during initialization. Looking at the git history, we see that it was done for TGL because the field didn't actually have 0x8 as default value, so the driver had to take care of it. In that regard, a reference to 359d0eff8409 ("drm/i915/display: Program DBUF_CTL tracker state service") could be made in the commit message to provide this historical context. According to the BSpec, programming that field is only applicable to display version 12. The most compeling reason why we should not program that field for other display versions that is not part of the official programming sequence in BSpec, and doing so could affect future platforms if the default is changed for some reason. The changes look good to me. With the commit message updated to reflect the two paragraphs above, Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> > >Or maybe: >""" >During display initialization and MBUS credits programming, the >DBUF_CTL register is also programmed. Specifically, when >programming DBUF_CTL, the tracker state service field is set to the >value 0x8. However, this is already the default value for platforms >using display versions 13 and prior, so we do not need to program >the DBUF_CTL register on those platforms. >""" > >> >> Bspec: 49213 >> Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c >> index 34465d56def0..98db721cba33 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_power.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c >> @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) >> { >> enum dbuf_slice slice; >> >> - if (display->platform.alderlake_p) >> - return; >> - > >I take it we're removing this condition because we no longer expect this code to run >on alderlake_p anyways? > >> for_each_dbuf_slice(display, slice) >> intel_de_rmw(display, DBUF_CTL_S(slice), >> DBUF_TRACKER_STATE_SERVICE_MASK, >> @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, >> /* 4. Enable CDCLK. */ >> intel_cdclk_init_hw(display); >> >> - if (DISPLAY_VER(display) >= 12) >> + if (DISPLAY_VER(display) == 12) > >If I'm understanding the purpose of this patch correctly (which I'm probably not), >shouldn't this be "if (DISPLAY_VER(display) > 13)"? >We only want to overwrite the tracker state service field on platforms after display version 13, >and it seems like gen12_dbuf_slices_config overwrites the tracker state service field. It is the opposite: we only want to program DBUF_TRACKER_STATE_SERVICE for display version 12. -- Gustavo Sousa
-----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Thursday, December 19, 2024 12:50 PM > To: Cavitt, Jonathan <jonathan.cavitt@intel.com> > Cc: Vodapalli, Ravi Kumar <ravi.kumar.vodapalli@intel.com>; intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar <haridhar.kalvala@intel.com>; Chauhan, Shekhar <shekhar.chauhan@intel.com> > Subject: Re: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service > > On Thu, Dec 19, 2024 at 11:39:07AM -0800, Cavitt, Jonathan wrote: > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli > > Sent: Thursday, December 19, 2024 9:37 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar <haridhar.kalvala@intel.com>; Chauhan, Shekhar <shekhar.chauhan@intel.com> > > Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service > > > > > > While display initialization along with MBUS credits programming > > > DBUF_CTL register is also programmed, as a part of it the tracker > > > state service field is also set to 0x8 value when default value is > > > other than 0x8 which are for platforms past display version 13. > > > For remaining platforms the default value is already 0x8 so don't > > > program them. > > > > This could use some rewording. Perhaps: > > """ > > For platforms past display version 13, during display initialization along > > with MBUS credits and DBUF_CTL register programming, the tracker state > > service field is set to a value of 0x8, which is not the default value for > > these platforms. All other platforms use 0x8 as the default value and thus > > do not need to be overwritten. > > """ > > > > Or maybe: > > """ > > During display initialization and MBUS credits programming, the > > DBUF_CTL register is also programmed. Specifically, when > > programming DBUF_CTL, the tracker state service field is set to the > > value 0x8. However, this is already the default value for platforms > > using display versions 13 and prior, so we do not need to program > > the DBUF_CTL register on those platforms. > > """ > > I think these are still missing the point a bit. The key is that the > bspec asks us to program non-default values _only_ on certain platforms. > For all other platforms (both earlier and later), the expectation > is that the hardware's default settings (whatever they happen to be for > a given platform) are already correct and should not be adjusted. > > The code here was originally written back during gen12 development > following the standard "assume driver changes will apply to all future > platforms too until we know otherwise" design, so the original test was > written as ">= 12." It looks like DG2 (one display version 13 platform) > still needed the programming, but ADL-P (the other display version 13 > platform) did not. From version 14 onward, no platforms need it. So > the correct condition to match the hardware/bspec's rules would be: > > if (DISPLAY_VER(display) == 12 || display->platform.dg2) > > (I think on an earlier review of this I said it should be just > "DISPLAY_VER(display) == 12" but I overlooked that DG2 is indeed > included in the list of platforms on bspec page 49213). > > Once later platforms like ADL-P, MTL, etc. rolled around, it turned out > that explicit programming was actually not expected to carry forward, > and we should have adjusted the condition to just 12+DG2 at that time, > but it was overlooked. That was technically a bug, but it turned out to > be harmless because the explicit value we were programming (8) happened > to match the new hardware defaults on display version 13. However we > shouldn't count on it staying harmless forever --- if the hardware > default ever changes to something else in the future, then it can cause > problems if we're still explicitly programming it to "8" by accident; > this patch is addressing that earlier oversight. > > > > > > > > > Bspec: 49213 > > > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 34465d56def0..98db721cba33 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) > > > { > > > enum dbuf_slice slice; > > > > > > - if (display->platform.alderlake_p) > > > - return; > > > - > > > > I take it we're removing this condition because we no longer expect this code to run > > on alderlake_p anyways? > > There's a platform/version check at the callsite and another here inside > the function. We only need to do the check at one place or the other; > that's somewhat independent of fixing the check(s) themselves to match > the right platforms. > > > > > > for_each_dbuf_slice(display, slice) > > > intel_de_rmw(display, DBUF_CTL_S(slice), > > > DBUF_TRACKER_STATE_SERVICE_MASK, > > > @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, > > > /* 4. Enable CDCLK. */ > > > intel_cdclk_init_hw(display); > > > > > > - if (DISPLAY_VER(display) >= 12) > > > + if (DISPLAY_VER(display) == 12) > > > > If I'm understanding the purpose of this patch correctly (which I'm probably not), > > shouldn't this be "if (DISPLAY_VER(display) > 13)"? > > We only want to overwrite the tracker state service field on platforms after display version 13, > > and it seems like gen12_dbuf_slices_config overwrites the tracker state service field. > > No, this change here is [mostly] correct; explicit driver programming at > display init time should only happen on 12 and DG2. The DG2 condition > is missing (probably because I overlooked it and misspoke in an earlier > code review). For all other platforms (both earlier and later versions) > we should leave the MBUF_CTL registers at their hardware defaults and > not touch them here. > > Also, don't confuse the code here (which adjusts the register at display > init time) with the other (re)programming of these values that happens > at runtime during modeset (adjusting based on how many pipes are > active). There are separate rules later in the bspec page and separate > code that handles that (correctly I believe); the patch here is just > addressing the specific stanza of the bspec page that says "The MBus > credits should be setup once with the following default values during > the display initialization," and that block only applies to the various > platforms that user display version 12, and then also to DG2. > Thank you to both @Roper, Matthew D and @Sousa, Gustavo for your input here. Perhaps this would be a better revision to the commit message in that case? """ Only the following platforms mandate updating DBUF_TRACKER_STATE_SERVICE during display initialization: 1. All platforms that use display version 12 2. DG2 All other platforms should use their default values unless stated otherwise, so update the display initialization code to reflect this requirement. """ -Jonathan Cavitt > > Matt > > > > > -Jonathan Cavitt > > > > > gen12_dbuf_slices_config(display); > > > > > > /* 5. Enable DBUF. */ > > > -- > > > 2.25.1 > > > > > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation >
On 12/20/2024 2:49 AM, Cavitt, Jonathan wrote: > -----Original Message----- >> From: Roper, Matthew D<matthew.d.roper@intel.com> >> Sent: Thursday, December 19, 2024 12:50 PM >> To: Cavitt, Jonathan<jonathan.cavitt@intel.com> >> Cc: Vodapalli, Ravi Kumar<ravi.kumar.vodapalli@intel.com>;intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani<balasubramani.vivekanandan@intel.com>; De Marchi, Lucas<lucas.demarchi@intel.com>; Sousa, Gustavo<gustavo.sousa@intel.com>; Taylor, Clinton A<clinton.a.taylor@intel.com>; Atwood, Matthew S<matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar<dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar<haridhar.kalvala@intel.com>; Chauhan, Shekhar<shekhar.chauhan@intel.com> >> Subject: Re: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service >> >> On Thu, Dec 19, 2024 at 11:39:07AM -0800, Cavitt, Jonathan wrote: >>> -----Original Message----- >>> From: Intel-gfx<intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli >>> Sent: Thursday, December 19, 2024 9:37 AM >>> To:intel-gfx@lists.freedesktop.org >>> Cc: Vivekanandan, Balasubramani<balasubramani.vivekanandan@intel.com>; Roper, Matthew D<matthew.d.roper@intel.com>; De Marchi, Lucas<lucas.demarchi@intel.com>; Sousa, Gustavo<gustavo.sousa@intel.com>; Taylor, Clinton A<clinton.a.taylor@intel.com>; Atwood, Matthew S<matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar<dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar<haridhar.kalvala@intel.com>; Chauhan, Shekhar<shekhar.chauhan@intel.com> >>> Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service >>>> While display initialization along with MBUS credits programming >>>> DBUF_CTL register is also programmed, as a part of it the tracker >>>> state service field is also set to 0x8 value when default value is >>>> other than 0x8 which are for platforms past display version 13. >>>> For remaining platforms the default value is already 0x8 so don't >>>> program them. >>> This could use some rewording. Perhaps: >>> """ >>> For platforms past display version 13, during display initialization along >>> with MBUS credits and DBUF_CTL register programming, the tracker state >>> service field is set to a value of 0x8, which is not the default value for >>> these platforms. All other platforms use 0x8 as the default value and thus >>> do not need to be overwritten. >>> """ >>> >>> Or maybe: >>> """ >>> During display initialization and MBUS credits programming, the >>> DBUF_CTL register is also programmed. Specifically, when >>> programming DBUF_CTL, the tracker state service field is set to the >>> value 0x8. However, this is already the default value for platforms >>> using display versions 13 and prior, so we do not need to program >>> the DBUF_CTL register on those platforms. >>> """ >> I think these are still missing the point a bit. The key is that the >> bspec asks us to program non-default values _only_ on certain platforms. >> For all other platforms (both earlier and later), the expectation >> is that the hardware's default settings (whatever they happen to be for >> a given platform) are already correct and should not be adjusted. >> >> The code here was originally written back during gen12 development >> following the standard "assume driver changes will apply to all future >> platforms too until we know otherwise" design, so the original test was >> written as ">= 12." It looks like DG2 (one display version 13 platform) >> still needed the programming, but ADL-P (the other display version 13 >> platform) did not. From version 14 onward, no platforms need it. So >> the correct condition to match the hardware/bspec's rules would be: >> >> if (DISPLAY_VER(display) == 12 || display->platform.dg2) Here since DG2 check is also coming into picture which is display version 13 we have to roll back to previous code, because the higher display versions than 12 should be handled in gen12_dbuf_slices_config() function and implement check for dg2 in that function if (DISPLAY_VER(display) >= 12) gen12_dbuf_slices_config(display); Ravi Kumar V >> (I think on an earlier review of this I said it should be just >> "DISPLAY_VER(display) == 12" but I overlooked that DG2 is indeed >> included in the list of platforms on bspec page 49213). >> >> Once later platforms like ADL-P, MTL, etc. rolled around, it turned out >> that explicit programming was actually not expected to carry forward, >> and we should have adjusted the condition to just 12+DG2 at that time, >> but it was overlooked. That was technically a bug, but it turned out to >> be harmless because the explicit value we were programming (8) happened >> to match the new hardware defaults on display version 13. However we >> shouldn't count on it staying harmless forever --- if the hardware >> default ever changes to something else in the future, then it can cause >> problems if we're still explicitly programming it to "8" by accident; >> this patch is addressing that earlier oversight. >> >>>> Bspec: 49213 >>>> Signed-off-by: Ravi Kumar Vodapalli<ravi.kumar.vodapalli@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- >>>> 1 file changed, 1 insertion(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c >>>> index 34465d56def0..98db721cba33 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c >>>> @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) >>>> { >>>> enum dbuf_slice slice; >>>> >>>> - if (display->platform.alderlake_p) >>>> - return; >>>> - >>> I take it we're removing this condition because we no longer expect this code to run >>> on alderlake_p anyways? >> There's a platform/version check at the callsite and another here inside >> the function. We only need to do the check at one place or the other; >> that's somewhat independent of fixing the check(s) themselves to match >> the right platforms. >> >>>> for_each_dbuf_slice(display, slice) >>>> intel_de_rmw(display, DBUF_CTL_S(slice), >>>> DBUF_TRACKER_STATE_SERVICE_MASK, >>>> @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, >>>> /* 4. Enable CDCLK. */ >>>> intel_cdclk_init_hw(display); >>>> >>>> - if (DISPLAY_VER(display) >= 12) >>>> + if (DISPLAY_VER(display) == 12) >>> If I'm understanding the purpose of this patch correctly (which I'm probably not), >>> shouldn't this be "if (DISPLAY_VER(display) > 13)"? >>> We only want to overwrite the tracker state service field on platforms after display version 13, >>> and it seems like gen12_dbuf_slices_config overwrites the tracker state service field. >> No, this change here is [mostly] correct; explicit driver programming at >> display init time should only happen on 12 and DG2. The DG2 condition >> is missing (probably because I overlooked it and misspoke in an earlier >> code review). For all other platforms (both earlier and later versions) >> we should leave the MBUF_CTL registers at their hardware defaults and >> not touch them here. >> >> Also, don't confuse the code here (which adjusts the register at display >> init time) with the other (re)programming of these values that happens >> at runtime during modeset (adjusting based on how many pipes are >> active). There are separate rules later in the bspec page and separate >> code that handles that (correctly I believe); the patch here is just >> addressing the specific stanza of the bspec page that says "The MBus >> credits should be setup once with the following default values during >> the display initialization," and that block only applies to the various >> platforms that user display version 12, and then also to DG2. >> > Thank you to both @Roper, Matthew D and @Sousa, Gustavo for your input here. > > Perhaps this would be a better revision to the commit message in that case? > """ > Only the following platforms mandate updating > DBUF_TRACKER_STATE_SERVICE during display initialization: > 1. All platforms that use display version 12 > 2. DG2 > > All other platforms should use their default values unless stated > otherwise, so update the display initialization code to reflect this > requirement. > """ > -Jonathan Cavitt > >> Matt >> >>> -Jonathan Cavitt >>> >>>> gen12_dbuf_slices_config(display); >>>> >>>> /* 5. Enable DBUF. */ >>>> -- >>>> 2.25.1 >>>> >>>> >> -- >> Matt Roper >> Graphics Software Engineer >> Linux GPU Platform Enablement >> Intel Corporation >>
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 34465d56def0..98db721cba33 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display) { enum dbuf_slice slice; - if (display->platform.alderlake_p) - return; - for_each_dbuf_slice(display, slice) intel_de_rmw(display, DBUF_CTL_S(slice), DBUF_TRACKER_STATE_SERVICE_MASK, @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display, /* 4. Enable CDCLK. */ intel_cdclk_init_hw(display); - if (DISPLAY_VER(display) >= 12) + if (DISPLAY_VER(display) == 12) gen12_dbuf_slices_config(display); /* 5. Enable DBUF. */
While display initialization along with MBUS credits programming DBUF_CTL register is also programmed, as a part of it the tracker state service field is also set to 0x8 value when default value is other than 0x8 which are for platforms past display version 13. For remaining platforms the default value is already 0x8 so don't program them. Bspec: 49213 Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com> --- drivers/gpu/drm/i915/display/intel_display_power.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)