Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

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

Quoting Jani Nikula (2024-10-22 05:03:21-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> 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[] = {
>>          {},
>>  };
>>  
>> +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 },
>> +
>> +        { .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 },
>> +
>> +        {},
>> +};
>> +
>>  static void __intel_dmc_wl_release(struct intel_display *display)
>>  {
>>          struct drm_i915_private *i915 = to_i915(display->drm);
>> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>>          return false;
>>  }
>>  
>> -static bool intel_dmc_wl_check_range(u32 address)
>> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>  {
>> -        return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
>> +        const struct intel_dmc_wl_range *ranges;
>> +
>> +        ranges = lnl_wl_range;
>> +
>> +        if (intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        switch (display->power.domains.dc_state) {
>
>This file has no business looking at power domain guts. Use or add
>interfaces instead of poking at data directly.

I started adding a function intel_display_power_get_current_dc_state()
here, but then realized that display->power.domains is protected by a
mutex and we do not want to use it in an atomic context.

So, in v2, to avoid rewriting the whole power domains code to use
spinlocks, I decided to go with having a copy of dc_state struct
intel_dmc_wl, which is set by intel_dmc_wl_enable().

--
Gustavo Sousa

>
>> +        case DC_STATE_EN_DC3CO:
>> +                ranges = xe3lpd_dc3co_wl_ranges;
>> +                break;
>> +        case DC_STATE_EN_UPTO_DC5:
>> +        case DC_STATE_EN_UPTO_DC6:
>> +                ranges = xe3lpd_dc5_dc6_wl_ranges;
>> +                break;
>> +        default:
>> +                ranges = NULL;
>> +        }
>> +
>> +        if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        return false;
>>  }
>>  
>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>
>Side note, unrelated to this patch, i915_reg_t is supposed to be opaque,
>nobody should look at reg.reg directly, there's i915_mmio_reg_offset()
>for it.
>
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>
>-- 
>Jani Nikula, Intel