Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Luca Coelho (2024-11-06 08:47:07-03:00)
>On Tue, 2024-11-05 at 10:00 -0300, Gustavo Sousa wrote:
>> Quoting Luca Coelho (2024-11-01 09:51:48-03:00)
>> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> > > There are extra registers that require the DMC wakelock when specific
>> > > dynamic DC states are in place. Add the table ranges for them and use
>> > > the correct table depending on the allowed DC states.
>> > > 
>> > > Bspec: 71583
>> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>> > >  1 file changed, 108 insertions(+), 4 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > index d597cc825f64..8bf2f32be859 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > @@ -5,6 +5,7 @@
>> > >  
>> > >  #include <linux/kernel.h>
>> > >  
>> > > +#include "i915_reg.h"
>> > >  #include "intel_de.h"
>> > >  #include "intel_dmc.h"
>> > >  #include "intel_dmc_regs.h"
>> > > @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>> > >          {},
>> > >  };
>> > 
>> > Do we still need the lnl_wl_range[]? This was sort of a place-holder
>> > with a very large range just for testing.  I can see that there are at
>> > least some ranges in common between lnl_wl_range[] and the new range
>> > tables defined below.
>> 
>> Yes, although we could do some homework to get a more accurate set of
>> ranges.
>> 
>> Now, about the different tables:
>> 
>>  - lnl_wl_range should be about ranges of registers that are powered
>>    down during DC states and that the HW requires DC exit for proper
>>    access.
>>  - xe3lpd_{dc5_dc6,dc3co}_wl_ranges are registers that are touched by
>>    the DMC and need the wakelock for properly restoring the correct
>>    value before accessing them.
>> 
>> Maybe a comment in the code explaining the above is warranted?
>
>I think a better naming for the arrays is warranted. :) Wouldn't
>changing lnl_wl_range to base_wl_range or so be better? My point is
>that LNL is not related at all here (anymore).

Yep, we could come up with better names for those variables. I went
with:

    s/lnl_wl_range/powered_off_ranges/
    s/xe3lpd_dc3co_wl_ranges/xe3lpd_dc3co_dmc_ranges/
    s/xe3lpd_dc5_dc6_wl_ranges/xe3lpd_dc5_dc6_dmc_ranges/

And also added comments to differentiate their purposes.

--
Gustavo Sousa

>
>
>> > > +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> > > +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> > > +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> > > +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> > > +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> > > +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> > > +
>> > > +        /* DBUF_CTL_* */
>> > > +        { .start = 0x44300, .end = 0x44300 },
>> > > +        { .start = 0x44304, .end = 0x44304 },
>> > > +        { .start = 0x44f00, .end = 0x44f00 },
>> > > +        { .start = 0x44f04, .end = 0x44f04 },
>> > > +        { .start = 0x44fe8, .end = 0x44fe8 },
>> > > +        { .start = 0x45008, .end = 0x45008 },
>> > > +
>> > > +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> > > +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> > > +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> > > +
>> > > +        /* TRANS_CMTG_CTL_* */
>> > > +        { .start = 0x6fa88, .end = 0x6fa88 },
>> > > +        { .start = 0x6fb88, .end = 0x6fb88 },
>> > 
>> > These, for instance, are part of lnl_wl_range[].
>> 
>> Given my clarification above about the different purposes of the ranges,
>> I think we should stick to keeping the same values from the (soon to
>> be?) documented tables, even if there is some small redundancy.
>> Otherwise we would require the programmer to remember to check ranges in
>> the "more general" table every time a DC state-specific one needs to be
>> added or updated.
>
>Makes sense, I guess it's okay that the base table and the specialized
>tables are slightly redundant then.
>
>--
>Cheers,
>Luca.