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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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

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