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

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

Quoting Vivi, Rodrigo (2025-02-03 17:59:19-03:00)
>On Mon, 2025-02-03 at 17:40 -0300, Gustavo Sousa wrote:
>> 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()?
>
>doh! I really need to stop trying work without glasses :)
>
>> 
>> 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).
>
>but this function is the only one that really writes the right values
>to the registers, so if we need something here, why not just reading
>the register directly?
>
>so perhaps I really missed your point on why we would need this...

Perhaps Imre can explain this better, but I believe the point is that we
want to track increments to DC5 counter when we have DC6 enabled. That
driver-managed counter would be in dc6_allowed.

Repeating Imre's suggestions with a minor tweak:

1. Before we tell the hardware that we are allowing DC6 (disable ->
   DC6), we store the value of the current DC5 counter.
2. After we disable DC states from DC6 (DC6 -> disable), we read the DC5
   counter again and subtract the value from (1). The result would then
   be added to the current value of dc6_allowed.

In (1) I think we should read the DC5 counter before we update
DC_STATE_EN, just to be sure we avoid some sort of race (although that
appears to be unlikely to happen).

During DC6 validation, if the test sees that dc6_allowed was
incremented, that means that the display engine reached a state where
the SOC would be able to put the display in DC6.

--
Gustavo Sousa

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