Re: [PATCH v3] drm/i915/dmc: Create debugfs entry for dc6 counter

[Imre Deak <imre.deak@intel.com>]

On Tue, Mar 04, 2025 at 02:16:15PM +0200, Jani Nikula wrote:
> On Tue, 04 Mar 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On Tue, Mar 04, 2025 at 12:53:19AM +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.
> >> 
> >> 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.
> >> 
> >> 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_counter += dc5_current_count - dc5_start_count
> >> 
> >> v2: Update the discription. (Imre)
> >>     Read dc5 count during dc6 enable and disable then and update
> >>     dc6 residency counter. (Imre)
> >>     Remove variable from dmc structure. (Jani)
> >>     Updated the subject title.
> >> v3: Add i915_power_domains lock to updated dc6 count in debugfs. (Imre)
> >>     Use flags to check dc6 enable/disable states. (Imre)
> >>     Move the display version check and counter read/update to
> >>     a helper. (Imre)
> >>     Resize the variable length. (Rodrigo)
> >>     Use old dc6 debugfs entry for every platform. (Rodrigo)
> >> 
> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
> >> ---
> >>  .../gpu/drm/i915/display/intel_display_core.h |  2 ++
> >>  .../i915/display/intel_display_power_well.c   | 26 +++++++++++++++++++
> >>  .../i915/display/intel_display_power_well.h   |  1 +
> >>  drivers/gpu/drm/i915/display/intel_dmc.c      | 21 ++++++++++++---
> >>  4 files changed, 47 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> index 554870d2494b..1608268bd9e2 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> @@ -490,6 +490,8 @@ struct intel_display {
> >>  
> >>  		/* perform PHY state sanity checks? */
> >>  		bool chv_phy_assert[2];
> >> +		unsigned int dc6_count;
> >
> > I think it's better to use dc6_allowed_count as Rodrigo suggested, not
> > regarding it as a counter for actual DC6 transitions.
> >
> >> +		unsigned int dc5_start_count;
> >>  	} power;
> >>  
> >>  	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 5b60db597329..8478e687abb7 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> >> @@ -17,6 +17,7 @@
> >>  #include "intel_dkl_phy.h"
> >>  #include "intel_dkl_phy_regs.h"
> >>  #include "intel_dmc.h"
> >> +#include "intel_dmc_regs.h"
> >>  #include "intel_dmc_wl.h"
> >>  #include "intel_dp_aux_regs.h"
> >>  #include "intel_dpio_phy.h"
> >> @@ -728,6 +729,22 @@ void gen9_sanitize_dc_state(struct intel_display *display)
> >>  	power_domains->dc_state = val;
> >>  }
> >>  
> >> +void update_dc6_count(struct intel_display *display, bool dc6_en_dis)
> >
> > Maybe rename dc6_en_dis to start_tracking?
> >
> > As Jani suggested the function should be in intel_dmc.c
> 
> Well, maybe.
> 
> I think the DMC register read should be in intel_dmc.c.
> 
> But maybe the display->power.* handling should be in
> intel_display_power(_well).c?

I think the counters should be stored in the intel_dmc struct and the
dc6_allowed_count should be returned via an interface (for instance just
make the above function return it).

> IOW, make the functions and interfaces make sense for both. Now there's
> no logic in the division.
> 
> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel