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

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

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@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.

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.

--
Gustavo Sousa

>
>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.