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

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

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.

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