Re: [PATCH] drm/i915/dmc: Add debugfs for dc6 counter

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

On Mon, Feb 03, 2025 at 05:19:08PM -0300, Gustavo Sousa wrote:
> Quoting Imre Deak (2025-02-03 16:22:44-03:00)
> >On Mon, Feb 03, 2025 at 12:15:26PM -0500, Rodrigo Vivi wrote:
> >> > > > [...]
> >> > > >
> >> > > > The driver enabling DC6 is not an enough condition for DC6 being allowed
> >> > > > from the display side. Some display clock gating etc. configuration by
> >> > > > the driver could be blocking it. According to the HW team, DC5 being
> >> > > > entered while DC6 is enabled is a guarantee that DC6 is allowed from the
> >> > > > display side - i.e. the driver has configured everything correctly for
> >> > > > that.
> >> > > 
> >> > > Fair enough. So IGT test case would check directly if DC5 counter is
> >> > > increasing and DC6 is allowed.
> >> > > 
> >> > > Something as simple as this in the kernel code would tell that
> >> > > DC6 is enabled:
> >> > > 
> >> > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> >> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> >> > > @@ -1294,6 +1294,10 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused)
> >> > >                 seq_printf(m, "DC5 -> DC6 count: %d\n",
> >> > >                            intel_de_read(display, dc6_reg));
> >> > >  
> >> > > +       seq_printf(m, "DC6 allowed: %s\n", str_yes_no((intel_de_read(display,
> >> > > +                                                                   DC_STATE_EN)
> >> > > +                                                     & 0x3) == 2));
> >> > > +
> >> > > 
> >> > > and
> >> > > 
> >> > > $ cat i915_dmc_info
> >> > > [snip]
> >> > > DC3 -> DC5 count: 286
> >> > > DC5 -> DC6 count: 0
> >> > > DC6 allowed: yes
> >> > > [snip]
> >> > > 
> >> > > $ cat i915_dmc_info
> >> > > [snip]
> >> > > DC3 -> DC5 count: 292
> >> > > DC5 -> DC6 count: 0
> >> > > DC6 allowed: yes
> >> > > [snip]
> >> > > 
> >> > > Thoughts?
> >> > 
> >> > The DC5 increment could've happened while DC6 was disabled by the driver.
> >> 
> >> Yes, it could... in general the dc6 bit would be zero, but right, there's
> >> a possible race.
> >> 
> >> But should we complicate things when we know that on the test case itself
> >> we are in full control of the modeset?! From the test perspective I believe
> >> this would be more than enough without complicating things.
> >
> >Imo having a counter which works based on the condition that guarantees the
> >display to allow DC6, would help later in debugging.
> >
> >> But well, if you really believe that we need to go further in the driver
> >> to cover that it is fine by me.
> >> 
> >> But just to ensure that we are in the same page, could you please open
> >> up a bit more of your idea on when to increase the dc5 sw counters
> >> in a way that we would ensure that we don't have race and that we
> >> get the dc6 allowed counter correctly?
> >
> >Something like the following:
> >
> >1. Right after the driver sets DC6_EN, it stores the DC5 HW counter's
> >   current value:
> >   dc5_start = dc5_current
> >2. Right before the driver clears DC6_EN, it updates the DC6 allowed
> >   counter:
> >   dc6_allowed += dc5_current - dc5_start
> >   dc5_start = dc5_current
> >3. When userspace reads the counters via debugfs the driver first
> >   updates dc6_allowed the same way as 2. did if DC6_EN is set.
> 
> This sounds good to me.
> 
> I'd like to add that we should ensure that DC6 is really being allowed,
> so that might need to be handled inside gen9_set_dc_state(), where
> allowed_dc_mask is applied.

Yes, handling 1. and 2. above in gen9_set_dc_state() looks ok to me.

> --
> Gustavo Sousa
> 
> >
> >> Btw, while doing this experiment I noticed that the debugfs and test
> >> also doesn't call that residency anyway and it is just count. So
> >> perhaps with your idea we don't need to change the debugfs interface.