[PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Luca Coelho]

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 at intel.com>
> > > ---
> > >  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.

For instance:

         /* DBUF_CTL_* */
-        { .start = 0x44300, .end = 0x44300 },
-        { .start = 0x44304, .end = 0x44304 },
-        { .start = 0x44f00, .end = 0x44f00 },
-        { .start = 0x44f04, .end = 0x44f04 },
-        { .start = 0x44fe8, .end = 0x44fe8 },

This probably means that _all_ the block, from at least 0x44300 till
0x44fff, needs to be protected.  What I'm trying to say is that even
though we don't access e.g. 0x44400, if we did, it would most likely
also have to be protected, because it's in the same block of addresses.

I guess this doesn't matter _that_ much, but it would be just cleaner
to know the actual ranges where the wakelocks are _potentially_ needed.

--
Cheers,
Luca.