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@xxxxxxxxx>
> > > ---
> > > 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).
> > > +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.
