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