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