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

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

--
Gustavo Sousa

>
>
>> +
>> +        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>> +        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>> +        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>> +        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        {},
>> +};
>> +
>> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +
>> +        /* 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 */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        /* Scanline registers */
>> +        { .start = 0x70000, .end = 0x70000 },
>> +        { .start = 0x70004, .end = 0x70004 },
>> +        { .start = 0x70014, .end = 0x70014 },
>> +        { .start = 0x70018, .end = 0x70018 },
>> +        { .start = 0x71000, .end = 0x71000 },
>> +        { .start = 0x71004, .end = 0x71004 },
>> +        { .start = 0x71014, .end = 0x71014 },
>> +        { .start = 0x71018, .end = 0x71018 },
>> +        { .start = 0x72000, .end = 0x72000 },
>> +        { .start = 0x72004, .end = 0x72004 },
>> +        { .start = 0x72014, .end = 0x72014 },
>> +        { .start = 0x72018, .end = 0x72018 },
>> +        { .start = 0x73000, .end = 0x73000 },
>> +        { .start = 0x73004, .end = 0x73004 },
>> +        { .start = 0x73014, .end = 0x73014 },
>> +        { .start = 0x73018, .end = 0x73018 },
>> +        { .start = 0x7b000, .end = 0x7b000 },
>> +        { .start = 0x7b004, .end = 0x7b004 },
>> +        { .start = 0x7b014, .end = 0x7b014 },
>> +        { .start = 0x7b018, .end = 0x7b018 },
>> +        { .start = 0x7c000, .end = 0x7c000 },
>> +        { .start = 0x7c004, .end = 0x7c004 },
>> +        { .start = 0x7c014, .end = 0x7c014 },
>> +        { .start = 0x7c018, .end = 0x7c018 },
>
>And so are all these.
>
>
>--
>Cheers,
>Luca.