On Wed, 2024-11-06 at 09:29 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-11-06 09:23:32-03:00)
> > On Tue, 2024-11-05 at 10:42 -0300, Gustavo Sousa wrote:
> > > Quoting Luca Coelho (2024-11-01 09:58:33-03:00)
> > > > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > > > Allow simpler syntax for defining entries for single registers in range
> > > > > tables. That makes them easier to type as well as to read, allowing one
> > > > > to quickly tell whether a range actually refers to a single register or
> > > > > a "true range".
> > > > >
> > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
> > > > > 1 file changed, 60 insertions(+), 58 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > index 8bf2f32be859..6992ce654e75 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
> > > > > };
> > > > >
> > > > > static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> > > > > - { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> > > > > + { .start = 0x45500 }, /* DC_STATE_SEL */
> > > > > { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> > > > > - { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> > > > > + { .start = 0x45504 }, /* DC_STATE_EN */
> > > > > { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> > > > > - { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> > > > > + { .start = 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 = 0x44300 },
> > > > > + { .start = 0x44304 },
> > > > > + { .start = 0x44f00 },
> > > > > + { .start = 0x44f04 },
> > > > > + { .start = 0x44fe8 },
> > > > > + { .start = 0x45008 },
> > > > >
> > > > > - { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > > > - { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> > > > > - { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > > > + { .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > > > + { .start = 0x46000 }, /* CDCLK_CTL */
> > > > > + { .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > >
> > > > Many of these are probably actually ranges. I'm not a HW guy, but
> > > > these are probably blocks that need the wakelock and it just happens
> > > > that many of those addresses are actually not used, but would need a
> > > > wakelock if they were used?
> > > >
> > > > IOW, e.g. all these DBUF_CTL registers are probably in the same range
> > > > that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
> > > > define many of these individually?
> > > >
> > > > This is related to the previous patch as well, but I decided to comment
> > > > it here because it becomes clearer.
> > >
> > > Maybe my reply on the previous patch clarifies this? I.e., these
> > > offset or offset ranges represent offsets that the DMC touches when on
> > > specific DC states.
> >
> > Yeah, but I think this idea of blocks is still valid. I think it's
> > very unlikely that only certain _addresses_ and not full blocks of
> > addresses are affected in the HW.
>
> Except that this is not about the hardware per se, this is about
> registers that are touched by the *DMC* during DC states and that need
> DC exit for properly accessing them from the driver. So, I think blocks
> are not applicable here.
Ah, okay, makes sense now. This could be explained in the awesome
comment you're planning to add, as discussed in the previous patch.
--
Cheers,
Luca.
