Quoting Imre Deak (2025-02-03 11:26:19-03:00)
>On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote:
>> Quoting Imre Deak (2025-02-03 09:43:38-03:00)
>> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote:
>> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state
>> >> due to dc6 counter has been removed to validate DC state.
>> >> Adding dc6_entry_counter at display dirver to validate dc6 state.
>> >>
>> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@xxxxxxxxx>
>> >> ---
>> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 +
>> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++
>> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 +
>> >> 3 files changed, 4 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> index 554870d2494b..cc244617011f 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> @@ -376,6 +376,7 @@ struct intel_display {
>> >> struct {
>> >> struct intel_dmc *dmc;
>> >> intel_wakeref_t wakeref;
>> >> + u32 dc6_entry_counter;
>> >> } dmc;
>> >>
>> >> struct {
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> index f45a4f9ba23c..0eb178aa618d 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display)
>> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6);
>> >>
>> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6);
>> >> +
>> >> + display->dmc.dc6_entry_counter++;
>> >
>> >AFAIU the goal is to validate that the display HW can reach the DC6
>> >power state. There is no HW DC6 residency counter (and there wasn't such
>> >a counter earlier either), so an alternative way is required. According
>> >to the HW team the display driver has programmed everything correctly in
>> >order to allow the DC6 power state if the DC5 power state is reached
>> >(indicated by the HW DC5 residency counter incrementing) and DC6 is
>> >enabled by the driver.
>>
>> Yep. That's what I learned as well when looking into this stuff a while
>> ago.
>>
>> >Based on the above, we'd need a DC6 residency counter maintained by the
>> >driver which is incremented if the DC5 residency counter increments
By the way, the counter that we currently have in our driver is the one
incremented by the DMC. I was meaning to send a patch for the residency
counter maintained by the hardware, but have not yet... In theory, that
one should be more accurate, but would require us to enable and disable
that counter as the IGT test starts and finishes.
>> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough
>> >for this, since it doesn't take into account the DC5 residency. While
>> >user space could check both dc6_entry_counter and the DC5 residency,
>> >that check would be racy wrt. the driver enabling/disabling the DC6
>> >state asynchronously.
>>
>> I'm not sure doing a driver-maintained dc6 entry counter would be
>> something worth implementing. Even if we have successfully entered DC5
>> and, in theory, DC6 would follow if enabled, this would be a synthetic
>> counter and it could be masking some hardware bug that could be
>> preventing DC6.
>
>According to the HW team the DC5 residency counter incrementing while
>DC6 is enabled is a guarantee that the display is configured correctly
>to allow the HW entering DC6 at all times. IOW this is the HW team's
>suggestion to validate DC6 at the moment.
>
>> On the IGT side, we could just skip if we are on a platform that does
>> not support DC6 counters, at least while we do not have a reliable
>> alternative way of checking for DC6.
>
>I think IGT would need to validate DC6 in the above way suggested by the
>HW team.
I'm still inclined to think that we should defer DC6 checking for when
we actually have a way to verify it. The way suggested above sounds
like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled.
In that case, just checking for DC5 should be enough for the time
being...
I won't object further if we do the other way though.
>
>> It would be good if we could detect that PG0 was in fact disabled, which
>> I believe is a stronger indication of DC6.
>
>It would be good to have a HW DC6 residency counter, but there isn't one
>at the moment. Other ways may have a dependency on other, non-display HW
>blocks, for instance in case of shared clock/voltage resources, the
>display functionality validation shouldn't be affected by these HW
>blocks.
As far as I understand by reading the docs, DC6 is DC5 with PG0
disabled. That's why my suggestion above.
--
Gustavo Sousa
>
>> --
>> Gustavo Sousa
>>
>> >
>> >I suppose the driver could take a snapshot of the DC5 residency counter
>> >right after it enables DC6 (dc5_residency_start) and increment the SW
>> >DC6 residency counter right before it disables DC6 or when user space
>> >reads the DC6 counter. So the driver would update the counter at these
>> >two points in the following way:
>> >dc6_residency += dc5_residency_current - dc5_residency_start
>> >
>> >The commit log would need a justification, something along the above
>> >lines.
>> >
>> >> }
>> >>
>> >> void bxt_enable_dc9(struct intel_display *display)
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> index 221d3abda791..f51bd8e6011d 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused)
>> >> if (i915_mmio_reg_valid(dc6_reg))
>> >> seq_printf(m, "DC5 -> DC6 count: %d\n",
>> >> intel_de_read(display, dc6_reg));
>> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter);
>> >>
>> >> seq_printf(m, "program base: 0x%08x\n",
>> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0)));
>> >> --
>> >> 2.43.0
>> >>
