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

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

Quoting Vivi, Rodrigo (2025-02-03 17:23:53-03:00)
>On Mon, 2025-02-03 at 17:19 -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.
>
>yeap, it makes sense
>
>> > 
>> > > 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 like that as well.
>
>> 
>> 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.
>
>well, for that we can also have the 
>
>--- 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));
>
>on top of what Imre suggested right?
>so the dc6 count is updated and also we have the live view of the
>register set

Hm... Not sure if that would be required to validate that the display
engine was ready for DC6. I guess the dc6_allowed counter would be
enough.

>
>no?
>
>not sure why we need to go to the dc9 func...

Hm... dc9? Did you mean gen9_set_dc_state()?

Function sanitizes the target value for DC_STATE_EN so that we do not
use a value that is not allowed (e.g. when the driver was loaded with
enable_dc=0).

--
Gustavo Sousa

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