| Message ID | 20250203085613.236340-1-mohammed.thasleem@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dmc: Add debugfs for dc6 counter | expand |
On Mon, 03 Feb 2025, Mohammed Thasleem <mohammed.thasleem@intel.com> wrote: > Starting from MTl we don't have a platform agnostic way to validate DC6 state > due to dc6 counter has been removed to validate DC state. > Adding dc6_entry_counter at display dirver to validate dc6 state. > > Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > 3 files changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > index 554870d2494b..cc244617011f 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > @@ -376,6 +376,7 @@ struct intel_display { > struct { > struct intel_dmc *dmc; > intel_wakeref_t wakeref; > + u32 dc6_entry_counter; > } dmc; > > struct { > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > index f45a4f9ba23c..0eb178aa618d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > + > + display->dmc.dc6_entry_counter++; This file has no business touching the guts of display->dmc. BR, Jani. > } > > void bxt_enable_dc9(struct intel_display *display) > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > index 221d3abda791..f51bd8e6011d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > if (i915_mmio_reg_valid(dc6_reg)) > seq_printf(m, "DC5 -> DC6 count: %d\n", > intel_de_read(display, dc6_reg)); > + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > seq_printf(m, "program base: 0x%08x\n", > intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0)));
On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > Starting from MTl we don't have a platform agnostic way to validate DC6 state > due to dc6 counter has been removed to validate DC state. > Adding dc6_entry_counter at display dirver to validate dc6 state. > > Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > 3 files changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > index 554870d2494b..cc244617011f 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > @@ -376,6 +376,7 @@ struct intel_display { > struct { > struct intel_dmc *dmc; > intel_wakeref_t wakeref; > + u32 dc6_entry_counter; > } dmc; > > struct { > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > index f45a4f9ba23c..0eb178aa618d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > + > + display->dmc.dc6_entry_counter++; AFAIU the goal is to validate that the display HW can reach the DC6 power state. There is no HW DC6 residency counter (and there wasn't such a counter earlier either), so an alternative way is required. According to the HW team the display driver has programmed everything correctly in order to allow the DC6 power state if the DC5 power state is reached (indicated by the HW DC5 residency counter incrementing) and DC6 is enabled by the driver. Based on the above, we'd need a DC6 residency counter maintained by the driver which is incremented if the DC5 residency counter increments while DC6 is enabled. The dc6_entry_counter in this patch is not enough for this, since it doesn't take into account the DC5 residency. While user space could check both dc6_entry_counter and the DC5 residency, that check would be racy wrt. the driver enabling/disabling the DC6 state asynchronously. I suppose the driver could take a snapshot of the DC5 residency counter right after it enables DC6 (dc5_residency_start) and increment the SW DC6 residency counter right before it disables DC6 or when user space reads the DC6 counter. So the driver would update the counter at these two points in the following way: dc6_residency += dc5_residency_current - dc5_residency_start The commit log would need a justification, something along the above lines. > } > > void bxt_enable_dc9(struct intel_display *display) > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > index 221d3abda791..f51bd8e6011d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > if (i915_mmio_reg_valid(dc6_reg)) > seq_printf(m, "DC5 -> DC6 count: %d\n", > intel_de_read(display, dc6_reg)); > + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > seq_printf(m, "program base: 0x%08x\n", > intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > -- > 2.43.0 >
Quoting Imre Deak (2025-02-03 09:43:38-03:00) >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: >> Starting from MTl we don't have a platform agnostic way to validate DC6 state >> due to dc6 counter has been removed to validate DC state. >> Adding dc6_entry_counter at display dirver to validate dc6 state. >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + >> 3 files changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h >> index 554870d2494b..cc244617011f 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h >> @@ -376,6 +376,7 @@ struct intel_display { >> struct { >> struct intel_dmc *dmc; >> intel_wakeref_t wakeref; >> + u32 dc6_entry_counter; >> } dmc; >> >> struct { >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> index f45a4f9ba23c..0eb178aa618d 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); >> + >> + display->dmc.dc6_entry_counter++; > >AFAIU the goal is to validate that the display HW can reach the DC6 >power state. There is no HW DC6 residency counter (and there wasn't such >a counter earlier either), so an alternative way is required. According >to the HW team the display driver has programmed everything correctly in >order to allow the DC6 power state if the DC5 power state is reached >(indicated by the HW DC5 residency counter incrementing) and DC6 is >enabled by the driver. Yep. That's what I learned as well when looking into this stuff a while ago. > >Based on the above, we'd need a DC6 residency counter maintained by the >driver which is incremented if the DC5 residency counter increments >while DC6 is enabled. The dc6_entry_counter in this patch is not enough >for this, since it doesn't take into account the DC5 residency. While >user space could check both dc6_entry_counter and the DC5 residency, >that check would be racy wrt. the driver enabling/disabling the DC6 >state asynchronously. I'm not sure doing a driver-maintained dc6 entry counter would be something worth implementing. Even if we have successfully entered DC5 and, in theory, DC6 would follow if enabled, this would be a synthetic counter and it could be masking some hardware bug that could be preventing DC6. On the IGT side, we could just skip if we are on a platform that does not support DC6 counters, at least while we do not have a reliable alternative way of checking for DC6. It would be good if we could detect that PG0 was in fact disabled, which I believe is a stronger indication of DC6. -- Gustavo Sousa > >I suppose the driver could take a snapshot of the DC5 residency counter >right after it enables DC6 (dc5_residency_start) and increment the SW >DC6 residency counter right before it disables DC6 or when user space >reads the DC6 counter. So the driver would update the counter at these >two points in the following way: >dc6_residency += dc5_residency_current - dc5_residency_start > >The commit log would need a justification, something along the above >lines. > >> } >> >> void bxt_enable_dc9(struct intel_display *display) >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >> index 221d3abda791..f51bd8e6011d 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) >> if (i915_mmio_reg_valid(dc6_reg)) >> seq_printf(m, "DC5 -> DC6 count: %d\n", >> intel_de_read(display, dc6_reg)); >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); >> >> seq_printf(m, "program base: 0x%08x\n", >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); >> -- >> 2.43.0 >>
On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > Quoting Imre Deak (2025-02-03 09:43:38-03:00) > >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > >> due to dc6 counter has been removed to validate DC state. > >> Adding dc6_entry_counter at display dirver to validate dc6 state. > >> > >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > >> 3 files changed, 4 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > >> index 554870d2494b..cc244617011f 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > >> @@ -376,6 +376,7 @@ struct intel_display { > >> struct { > >> struct intel_dmc *dmc; > >> intel_wakeref_t wakeref; > >> + u32 dc6_entry_counter; > >> } dmc; > >> > >> struct { > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> index f45a4f9ba23c..0eb178aa618d 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > >> > >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > >> + > >> + display->dmc.dc6_entry_counter++; > > > >AFAIU the goal is to validate that the display HW can reach the DC6 > >power state. There is no HW DC6 residency counter (and there wasn't such > >a counter earlier either), so an alternative way is required. According > >to the HW team the display driver has programmed everything correctly in > >order to allow the DC6 power state if the DC5 power state is reached > >(indicated by the HW DC5 residency counter incrementing) and DC6 is > >enabled by the driver. > > Yep. That's what I learned as well when looking into this stuff a while > ago. > > >Based on the above, we'd need a DC6 residency counter maintained by the > >driver which is incremented if the DC5 residency counter increments > >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > >for this, since it doesn't take into account the DC5 residency. While > >user space could check both dc6_entry_counter and the DC5 residency, > >that check would be racy wrt. the driver enabling/disabling the DC6 > >state asynchronously. > > I'm not sure doing a driver-maintained dc6 entry counter would be > something worth implementing. Even if we have successfully entered DC5 > and, in theory, DC6 would follow if enabled, this would be a synthetic > counter and it could be masking some hardware bug that could be > preventing DC6. According to the HW team the DC5 residency counter incrementing while DC6 is enabled is a guarantee that the display is configured correctly to allow the HW entering DC6 at all times. IOW this is the HW team's suggestion to validate DC6 at the moment. > On the IGT side, we could just skip if we are on a platform that does > not support DC6 counters, at least while we do not have a reliable > alternative way of checking for DC6. I think IGT would need to validate DC6 in the above way suggested by the HW team. > It would be good if we could detect that PG0 was in fact disabled, which > I believe is a stronger indication of DC6. It would be good to have a HW DC6 residency counter, but there isn't one at the moment. Other ways may have a dependency on other, non-display HW blocks, for instance in case of shared clock/voltage resources, the display functionality validation shouldn't be affected by these HW blocks. > -- > Gustavo Sousa > > > > >I suppose the driver could take a snapshot of the DC5 residency counter > >right after it enables DC6 (dc5_residency_start) and increment the SW > >DC6 residency counter right before it disables DC6 or when user space > >reads the DC6 counter. So the driver would update the counter at these > >two points in the following way: > >dc6_residency += dc5_residency_current - dc5_residency_start > > > >The commit log would need a justification, something along the above > >lines. > > > >> } > >> > >> void bxt_enable_dc9(struct intel_display *display) > >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > >> index 221d3abda791..f51bd8e6011d 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > >> if (i915_mmio_reg_valid(dc6_reg)) > >> seq_printf(m, "DC5 -> DC6 count: %d\n", > >> intel_de_read(display, dc6_reg)); > >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > >> > >> seq_printf(m, "program base: 0x%08x\n", > >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > >> -- > >> 2.43.0 > >>
Quoting Imre Deak (2025-02-03 11:26:19-03:00) >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state >> >> due to dc6 counter has been removed to validate DC state. >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. >> >> >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + >> >> 3 files changed, 4 insertions(+) >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h >> >> index 554870d2494b..cc244617011f 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h >> >> @@ -376,6 +376,7 @@ struct intel_display { >> >> struct { >> >> struct intel_dmc *dmc; >> >> intel_wakeref_t wakeref; >> >> + u32 dc6_entry_counter; >> >> } dmc; >> >> >> >> struct { >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> index f45a4f9ba23c..0eb178aa618d 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); >> >> >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); >> >> + >> >> + display->dmc.dc6_entry_counter++; >> > >> >AFAIU the goal is to validate that the display HW can reach the DC6 >> >power state. There is no HW DC6 residency counter (and there wasn't such >> >a counter earlier either), so an alternative way is required. According >> >to the HW team the display driver has programmed everything correctly in >> >order to allow the DC6 power state if the DC5 power state is reached >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is >> >enabled by the driver. >> >> Yep. That's what I learned as well when looking into this stuff a while >> ago. >> >> >Based on the above, we'd need a DC6 residency counter maintained by the >> >driver which is incremented if the DC5 residency counter increments By the way, the counter that we currently have in our driver is the one incremented by the DMC. I was meaning to send a patch for the residency counter maintained by the hardware, but have not yet... In theory, that one should be more accurate, but would require us to enable and disable that counter as the IGT test starts and finishes. >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough >> >for this, since it doesn't take into account the DC5 residency. While >> >user space could check both dc6_entry_counter and the DC5 residency, >> >that check would be racy wrt. the driver enabling/disabling the DC6 >> >state asynchronously. >> >> I'm not sure doing a driver-maintained dc6 entry counter would be >> something worth implementing. Even if we have successfully entered DC5 >> and, in theory, DC6 would follow if enabled, this would be a synthetic >> counter and it could be masking some hardware bug that could be >> preventing DC6. > >According to the HW team the DC5 residency counter incrementing while >DC6 is enabled is a guarantee that the display is configured correctly >to allow the HW entering DC6 at all times. IOW this is the HW team's >suggestion to validate DC6 at the moment. > >> On the IGT side, we could just skip if we are on a platform that does >> not support DC6 counters, at least while we do not have a reliable >> alternative way of checking for DC6. > >I think IGT would need to validate DC6 in the above way suggested by the >HW team. I'm still inclined to think that we should defer DC6 checking for when we actually have a way to verify it. The way suggested above sounds like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. In that case, just checking for DC5 should be enough for the time being... I won't object further if we do the other way though. > >> It would be good if we could detect that PG0 was in fact disabled, which >> I believe is a stronger indication of DC6. > >It would be good to have a HW DC6 residency counter, but there isn't one >at the moment. Other ways may have a dependency on other, non-display HW >blocks, for instance in case of shared clock/voltage resources, the >display functionality validation shouldn't be affected by these HW >blocks. As far as I understand by reading the docs, DC6 is DC5 with PG0 disabled. That's why my suggestion above. -- Gustavo Sousa > >> -- >> Gustavo Sousa >> >> > >> >I suppose the driver could take a snapshot of the DC5 residency counter >> >right after it enables DC6 (dc5_residency_start) and increment the SW >> >DC6 residency counter right before it disables DC6 or when user space >> >reads the DC6 counter. So the driver would update the counter at these >> >two points in the following way: >> >dc6_residency += dc5_residency_current - dc5_residency_start >> > >> >The commit log would need a justification, something along the above >> >lines. >> > >> >> } >> >> >> >> void bxt_enable_dc9(struct intel_display *display) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> index 221d3abda791..f51bd8e6011d 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) >> >> if (i915_mmio_reg_valid(dc6_reg)) >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", >> >> intel_de_read(display, dc6_reg)); >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); >> >> >> >> seq_printf(m, "program base: 0x%08x\n", >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); >> >> -- >> >> 2.43.0 >> >>
On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > >> >> due to dc6 counter has been removed to validate DC state. > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > >> >> > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > >> >> --- > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > >> >> 3 files changed, 4 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> index 554870d2494b..cc244617011f 100644 > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> @@ -376,6 +376,7 @@ struct intel_display { > >> >> struct { > >> >> struct intel_dmc *dmc; > >> >> intel_wakeref_t wakeref; > >> >> + u32 dc6_entry_counter; > >> >> } dmc; > >> >> > >> >> struct { > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > >> >> > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > >> >> + > >> >> + display->dmc.dc6_entry_counter++; > >> > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > >> >power state. There is no HW DC6 residency counter (and there wasn't such > >> >a counter earlier either), so an alternative way is required. According > >> >to the HW team the display driver has programmed everything correctly in > >> >order to allow the DC6 power state if the DC5 power state is reached > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > >> >enabled by the driver. > >> > >> Yep. That's what I learned as well when looking into this stuff a while > >> ago. > >> > >> >Based on the above, we'd need a DC6 residency counter maintained by the > >> >driver which is incremented if the DC5 residency counter increments > > By the way, the counter that we currently have in our driver is the one > incremented by the DMC. I was meaning to send a patch for the residency > counter maintained by the hardware, but have not yet... In theory, that > one should be more accurate, but would require us to enable and disable > that counter as the IGT test starts and finishes. > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > >> >for this, since it doesn't take into account the DC5 residency. While > >> >user space could check both dc6_entry_counter and the DC5 residency, > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > >> >state asynchronously. > >> > >> I'm not sure doing a driver-maintained dc6 entry counter would be > >> something worth implementing. Even if we have successfully entered DC5 > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > >> counter and it could be masking some hardware bug that could be > >> preventing DC6. > > > >According to the HW team the DC5 residency counter incrementing while > >DC6 is enabled is a guarantee that the display is configured correctly > >to allow the HW entering DC6 at all times. IOW this is the HW team's > >suggestion to validate DC6 at the moment. > > > >> On the IGT side, we could just skip if we are on a platform that does > >> not support DC6 counters, at least while we do not have a reliable > >> alternative way of checking for DC6. > > > >I think IGT would need to validate DC6 in the above way suggested by the > >HW team. > > I'm still inclined to think that we should defer DC6 checking for when > we actually have a way to verify it. The way suggested above sounds > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > In that case, just checking for DC5 should be enough for the time > being... That's not the same as DC5 incrementing while DC6 is enabled. > I won't object further if we do the other way though. > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > >> I believe is a stronger indication of DC6. > > > >It would be good to have a HW DC6 residency counter, but there isn't one > >at the moment. Other ways may have a dependency on other, non-display HW > >blocks, for instance in case of shared clock/voltage resources, the > >display functionality validation shouldn't be affected by these HW > >blocks. > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > disabled. That's why my suggestion above. > > -- > Gustavo Sousa > > > > >> -- > >> Gustavo Sousa > >> > >> > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > >> >DC6 residency counter right before it disables DC6 or when user space > >> >reads the DC6 counter. So the driver would update the counter at these > >> >two points in the following way: > >> >dc6_residency += dc5_residency_current - dc5_residency_start > >> > > >> >The commit log would need a justification, something along the above > >> >lines. > >> > > >> >> } > >> >> > >> >> void bxt_enable_dc9(struct intel_display *display) > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> index 221d3abda791..f51bd8e6011d 100644 > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > >> >> if (i915_mmio_reg_valid(dc6_reg)) > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > >> >> intel_de_read(display, dc6_reg)); > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > >> >> > >> >> seq_printf(m, "program base: 0x%08x\n", > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > >> >> -- > >> >> 2.43.0 > >> >>
On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > >> >> due to dc6 counter has been removed to validate DC state. > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > >> >> > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > >> >> --- > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > >> >> 3 files changed, 4 insertions(+) > > >> >> > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > >> >> index 554870d2494b..cc244617011f 100644 > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > >> >> struct { > > >> >> struct intel_dmc *dmc; > > >> >> intel_wakeref_t wakeref; > > >> >> + u32 dc6_entry_counter; > > >> >> } dmc; > > >> >> > > >> >> struct { > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > >> >> > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > >> >> + > > >> >> + display->dmc.dc6_entry_counter++; > > >> > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > >> >a counter earlier either), so an alternative way is required. According > > >> >to the HW team the display driver has programmed everything correctly in > > >> >order to allow the DC6 power state if the DC5 power state is reached > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > >> >enabled by the driver. > > >> > > >> Yep. That's what I learned as well when looking into this stuff a while > > >> ago. > > >> > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > >> >driver which is incremented if the DC5 residency counter increments > > > > By the way, the counter that we currently have in our driver is the one > > incremented by the DMC. I was meaning to send a patch for the residency > > counter maintained by the hardware, but have not yet... In theory, that > > one should be more accurate, but would require us to enable and disable > > that counter as the IGT test starts and finishes. > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > >> >for this, since it doesn't take into account the DC5 residency. While > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > >> >state asynchronously. > > >> > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > >> something worth implementing. Even if we have successfully entered DC5 > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > >> counter and it could be masking some hardware bug that could be > > >> preventing DC6. > > > > > >According to the HW team the DC5 residency counter incrementing while > > >DC6 is enabled is a guarantee that the display is configured correctly > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > >suggestion to validate DC6 at the moment. > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > >> not support DC6 counters, at least while we do not have a reliable > > >> alternative way of checking for DC6. > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > >HW team. > > > > I'm still inclined to think that we should defer DC6 checking for when > > we actually have a way to verify it. The way suggested above sounds > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > In that case, just checking for DC5 should be enough for the time > > being... > > That's not the same as DC5 incrementing while DC6 is enabled. > > > I won't object further if we do the other way though. > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > >> I believe is a stronger indication of DC6. > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > >at the moment. Other ways may have a dependency on other, non-display HW > > >blocks, for instance in case of shared clock/voltage resources, the > > >display functionality validation shouldn't be affected by these HW > > >blocks. > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > disabled. That's why my suggestion above. > > > > -- > > Gustavo Sousa > > > > > > > >> -- > > >> Gustavo Sousa > > >> > > >> > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > >> >DC6 residency counter right before it disables DC6 or when user space > > >> >reads the DC6 counter. So the driver would update the counter at these > > >> >two points in the following way: > > >> >dc6_residency += dc5_residency_current - dc5_residency_start Hmm I don't have a good feeling about this. I prefer that we are clear to the userspace(IGT) that is an extra flag and not to pretend that we have a residency counter. So, we either are clear that we are counting the entries, or having a flag that tells that we are allowing dc6. Which btw, could be done by IGT checking DC6_EN bit directly, no?! > > >> > > > >> >The commit log would need a justification, something along the above > > >> >lines. > > >> > > > >> >> } > > >> >> > > >> >> void bxt_enable_dc9(struct intel_display *display) > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > >> >> index 221d3abda791..f51bd8e6011d 100644 > > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > >> >> if (i915_mmio_reg_valid(dc6_reg)) > > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > > >> >> intel_de_read(display, dc6_reg)); > > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > >> >> > > >> >> seq_printf(m, "program base: 0x%08x\n", > > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > >> >> -- > > >> >> 2.43.0 > > >> >>
On Mon, Feb 03, 2025 at 11:23:23AM +0200, Jani Nikula wrote: > On Mon, 03 Feb 2025, Mohammed Thasleem <mohammed.thasleem@intel.com> wrote: > > Starting from MTl we don't have a platform agnostic way to validate DC6 state > > due to dc6 counter has been removed to validate DC state. > > Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > 3 files changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > index 554870d2494b..cc244617011f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > @@ -376,6 +376,7 @@ struct intel_display { > > struct { > > struct intel_dmc *dmc; > > intel_wakeref_t wakeref; > > + u32 dc6_entry_counter; > > } dmc; > > > > struct { > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > index f45a4f9ba23c..0eb178aa618d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > + > > + display->dmc.dc6_entry_counter++; > > This file has no business touching the guts of display->dmc. my bad... I might have suggested that because here we are covering something up for DMC :) But yet, maybe better another place of i915_display. > > BR, > Jani. > > > > } > > > > void bxt_enable_dc9(struct intel_display *display) > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > index 221d3abda791..f51bd8e6011d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > if (i915_mmio_reg_valid(dc6_reg)) > > seq_printf(m, "DC5 -> DC6 count: %d\n", > > intel_de_read(display, dc6_reg)); > > + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > > > seq_printf(m, "program base: 0x%08x\n", > > intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > -- > Jani Nikula, Intel
On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > >> >> due to dc6 counter has been removed to validate DC state. > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > >> >> > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > >> >> --- > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > >> >> 3 files changed, 4 insertions(+) > > > >> >> > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > >> >> index 554870d2494b..cc244617011f 100644 > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > >> >> struct { > > > >> >> struct intel_dmc *dmc; > > > >> >> intel_wakeref_t wakeref; > > > >> >> + u32 dc6_entry_counter; > > > >> >> } dmc; > > > >> >> > > > >> >> struct { > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > >> >> > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > >> >> + > > > >> >> + display->dmc.dc6_entry_counter++; > > > >> > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > >> >a counter earlier either), so an alternative way is required. According > > > >> >to the HW team the display driver has programmed everything correctly in > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > >> >enabled by the driver. > > > >> > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > >> ago. > > > >> > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > By the way, the counter that we currently have in our driver is the one > > > incremented by the DMC. I was meaning to send a patch for the residency > > > counter maintained by the hardware, but have not yet... In theory, that > > > one should be more accurate, but would require us to enable and disable > > > that counter as the IGT test starts and finishes. > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > >> >state asynchronously. > > > >> > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > >> something worth implementing. Even if we have successfully entered DC5 > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > >> counter and it could be masking some hardware bug that could be > > > >> preventing DC6. > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > >suggestion to validate DC6 at the moment. > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > >> not support DC6 counters, at least while we do not have a reliable > > > >> alternative way of checking for DC6. > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > >HW team. > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > we actually have a way to verify it. The way suggested above sounds > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > In that case, just checking for DC5 should be enough for the time > > > being... > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > I won't object further if we do the other way though. > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > >> I believe is a stronger indication of DC6. > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > >blocks, for instance in case of shared clock/voltage resources, the > > > >display functionality validation shouldn't be affected by these HW > > > >blocks. > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > disabled. That's why my suggestion above. > > > > > > -- > > > Gustavo Sousa > > > > > > > > > > >> -- > > > >> Gustavo Sousa > > > >> > > > >> > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > >> >two points in the following way: > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > Hmm I don't have a good feeling about this. > > I prefer that we are clear to the userspace(IGT) that is an extra flag > and not to pretend that we have a residency counter. > > So, we either are clear that we are counting the entries, or having > a flag that tells that we are allowing dc6. Which btw, could be done > by IGT checking DC6_EN bit directly, no?! A DC6 enabled check alone would be not enough and checking it from user space along with the DC5 counter would be racy as described above. I see this working by the driver tracking the DC6 enabled flag + the DC5 counter in the above way; it could be exposed to user space with a suitable name, eg. dc6_allowed_time. > > > >> >The commit log would need a justification, something along the above > > > >> >lines. > > > >> > > > > >> >> } > > > >> >> > > > >> >> void bxt_enable_dc9(struct intel_display *display) > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > > >> >> index 221d3abda791..f51bd8e6011d 100644 > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > > >> >> if (i915_mmio_reg_valid(dc6_reg)) > > > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > > > >> >> intel_de_read(display, dc6_reg)); > > > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > > >> >> > > > >> >> seq_printf(m, "program base: 0x%08x\n", > > > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > > >> >> -- > > > >> >> 2.43.0 > > > >> >>
On Mon, Feb 03, 2025 at 06:01:25PM +0200, Imre Deak wrote: > On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > > >> >> due to dc6 counter has been removed to validate DC state. > > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > >> >> > > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > > >> >> --- > > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > > >> >> 3 files changed, 4 insertions(+) > > > > >> >> > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > >> >> index 554870d2494b..cc244617011f 100644 > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > > >> >> struct { > > > > >> >> struct intel_dmc *dmc; > > > > >> >> intel_wakeref_t wakeref; > > > > >> >> + u32 dc6_entry_counter; > > > > >> >> } dmc; > > > > >> >> > > > > >> >> struct { > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > >> >> > > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > > >> >> + > > > > >> >> + display->dmc.dc6_entry_counter++; > > > > >> > > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > > >> >a counter earlier either), so an alternative way is required. According > > > > >> >to the HW team the display driver has programmed everything correctly in > > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > > >> >enabled by the driver. > > > > >> > > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > > >> ago. > > > > >> > > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > > > By the way, the counter that we currently have in our driver is the one > > > > incremented by the DMC. I was meaning to send a patch for the residency > > > > counter maintained by the hardware, but have not yet... In theory, that > > > > one should be more accurate, but would require us to enable and disable > > > > that counter as the IGT test starts and finishes. > > > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > > >> >state asynchronously. > > > > >> > > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > > >> something worth implementing. Even if we have successfully entered DC5 > > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > > >> counter and it could be masking some hardware bug that could be > > > > >> preventing DC6. > > > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > > >suggestion to validate DC6 at the moment. > > > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > > >> not support DC6 counters, at least while we do not have a reliable > > > > >> alternative way of checking for DC6. > > > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > > >HW team. > > > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > > we actually have a way to verify it. The way suggested above sounds > > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > > > In that case, just checking for DC5 should be enough for the time > > > > being... > > > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > > > I won't object further if we do the other way though. > > > > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > > >> I believe is a stronger indication of DC6. > > > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > > >blocks, for instance in case of shared clock/voltage resources, the > > > > >display functionality validation shouldn't be affected by these HW > > > > >blocks. > > > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > > disabled. That's why my suggestion above. > > > > > > > > -- > > > > Gustavo Sousa > > > > > > > > > > > > > >> -- > > > > >> Gustavo Sousa > > > > >> > > > > >> > > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > > >> >two points in the following way: > > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > > > Hmm I don't have a good feeling about this. > > > > I prefer that we are clear to the userspace(IGT) that is an extra flag > > and not to pretend that we have a residency counter. > > > > So, we either are clear that we are counting the entries, or having > > a flag that tells that we are allowing dc6. Which btw, could be done > > by IGT checking DC6_EN bit directly, no?! > > A DC6 enabled check alone would be not enough and checking it from user > space along with the DC5 counter would be racy as described above. I see > this working by the driver tracking the DC6 enabled flag + the DC5 > counter in the above way; it could be exposed to user space with a > suitable name, eg. dc6_allowed_time. Right, the name and new entry in the debugfs file would make this better because we wouldn't be pretending 'residency', specially with no guarantee that it would enter. However I'd like to keep things simple. Stepping back to see what the use case from the test are trying to really accomplish: * SUBTEST: dc6-dpms * Description: Validate display engine entry to DC6 state while all connectors's * DPMS property set to OFF * * SUBTEST: dc6-psr * Description: This test validates display engine entry to DC6 state while PSR is active * Functionality: pm_dc, psr1 Of course, we already know that it is impossible to validate that the display engine itself entered that. But we can at least validate that our driver is allowing that condition. This is with fake residency, with the allowed_time, but also with the simple counter that Mohammed added, or also just by checking the register directly... _MMIO(0x45504) & 0x3 == 2 // in idle scenario described above should be enough imho > > > > > >> >The commit log would need a justification, something along the above > > > > >> >lines. > > > > >> > > > > > >> >> } > > > > >> >> > > > > >> >> void bxt_enable_dc9(struct intel_display *display) > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > >> >> index 221d3abda791..f51bd8e6011d 100644 > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > > > >> >> if (i915_mmio_reg_valid(dc6_reg)) > > > > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > > > > >> >> intel_de_read(display, dc6_reg)); > > > > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > > > >> >> > > > > >> >> seq_printf(m, "program base: 0x%08x\n", > > > > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > > > >> >> -- > > > > >> >> 2.43.0 > > > > >> >>
On Mon, Feb 03, 2025 at 11:12:34AM -0500, Rodrigo Vivi wrote: > On Mon, Feb 03, 2025 at 06:01:25PM +0200, Imre Deak wrote: > > On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > > > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > > > >> >> due to dc6 counter has been removed to validate DC state. > > > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > > >> >> > > > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > > > >> >> --- > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > > > >> >> 3 files changed, 4 insertions(+) > > > > > >> >> > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > >> >> index 554870d2494b..cc244617011f 100644 > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > > > >> >> struct { > > > > > >> >> struct intel_dmc *dmc; > > > > > >> >> intel_wakeref_t wakeref; > > > > > >> >> + u32 dc6_entry_counter; > > > > > >> >> } dmc; > > > > > >> >> > > > > > >> >> struct { > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > > >> >> > > > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > > > >> >> + > > > > > >> >> + display->dmc.dc6_entry_counter++; > > > > > >> > > > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > > > >> >a counter earlier either), so an alternative way is required. According > > > > > >> >to the HW team the display driver has programmed everything correctly in > > > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > > > >> >enabled by the driver. > > > > > >> > > > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > > > >> ago. > > > > > >> > > > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > > > > > By the way, the counter that we currently have in our driver is the one > > > > > incremented by the DMC. I was meaning to send a patch for the residency > > > > > counter maintained by the hardware, but have not yet... In theory, that > > > > > one should be more accurate, but would require us to enable and disable > > > > > that counter as the IGT test starts and finishes. > > > > > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > > > >> >state asynchronously. > > > > > >> > > > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > > > >> something worth implementing. Even if we have successfully entered DC5 > > > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > > > >> counter and it could be masking some hardware bug that could be > > > > > >> preventing DC6. > > > > > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > > > >suggestion to validate DC6 at the moment. > > > > > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > > > >> not support DC6 counters, at least while we do not have a reliable > > > > > >> alternative way of checking for DC6. > > > > > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > > > >HW team. > > > > > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > > > we actually have a way to verify it. The way suggested above sounds > > > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > > > > > In that case, just checking for DC5 should be enough for the time > > > > > being... > > > > > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > > > > > I won't object further if we do the other way though. > > > > > > > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > > > >> I believe is a stronger indication of DC6. > > > > > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > > > >blocks, for instance in case of shared clock/voltage resources, the > > > > > >display functionality validation shouldn't be affected by these HW > > > > > >blocks. > > > > > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > > > disabled. That's why my suggestion above. > > > > > > > > > > -- > > > > > Gustavo Sousa > > > > > > > > > > > > > > > > >> -- > > > > > >> Gustavo Sousa > > > > > >> > > > > > >> > > > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > > > >> >two points in the following way: > > > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > > > > > Hmm I don't have a good feeling about this. > > > > > > I prefer that we are clear to the userspace(IGT) that is an extra flag > > > and not to pretend that we have a residency counter. > > > > > > So, we either are clear that we are counting the entries, or having > > > a flag that tells that we are allowing dc6. Which btw, could be done > > > by IGT checking DC6_EN bit directly, no?! > > > > A DC6 enabled check alone would be not enough and checking it from user > > space along with the DC5 counter would be racy as described above. I see > > this working by the driver tracking the DC6 enabled flag + the DC5 > > counter in the above way; it could be exposed to user space with a > > suitable name, eg. dc6_allowed_time. > > Right, the name and new entry in the debugfs file would make this > better because we wouldn't be pretending 'residency', specially > with no guarantee that it would enter. > > However I'd like to keep things simple. Stepping back to see > what the use case from the test are trying to really > accomplish: > > * SUBTEST: dc6-dpms > * Description: Validate display engine entry to DC6 state while all connectors's > * DPMS property set to OFF > * > * SUBTEST: dc6-psr > * Description: This test validates display engine entry to DC6 state while PSR is active > * Functionality: pm_dc, psr1 > > Of course, we already know that it is impossible to validate > that the display engine itself entered that. But we can > at least validate that our driver is allowing that condition. > > This is with fake residency, with the allowed_time, but also > with the simple counter that Mohammed added, or also just > by checking the register directly... > > _MMIO(0x45504) & 0x3 == 2 // in idle scenario described above should be enough imho 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. > > > > > >> >The commit log would need a justification, something along the above > > > > > >> >lines. > > > > > >> > > > > > > >> >> } > > > > > >> >> > > > > > >> >> void bxt_enable_dc9(struct intel_display *display) > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > >> >> index 221d3abda791..f51bd8e6011d 100644 > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > > > > >> >> if (i915_mmio_reg_valid(dc6_reg)) > > > > > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > > > > > >> >> intel_de_read(display, dc6_reg)); > > > > > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > > > > >> >> > > > > > >> >> seq_printf(m, "program base: 0x%08x\n", > > > > > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > > > > >> >> -- > > > > > >> >> 2.43.0 > > > > > >> >>
Quoting Imre Deak (2025-02-03 12:14:10-03:00) >On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: >> Quoting Imre Deak (2025-02-03 11:26:19-03:00) >> >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: >> >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) >> >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: >> >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state >> >> >> due to dc6 counter has been removed to validate DC state. >> >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. >> >> >> >> >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> >> >> >> --- >> >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + >> >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ >> >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + >> >> >> 3 files changed, 4 insertions(+) >> >> >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h >> >> >> index 554870d2494b..cc244617011f 100644 >> >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h >> >> >> @@ -376,6 +376,7 @@ struct intel_display { >> >> >> struct { >> >> >> struct intel_dmc *dmc; >> >> >> intel_wakeref_t wakeref; >> >> >> + u32 dc6_entry_counter; >> >> >> } dmc; >> >> >> >> >> >> struct { >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> >> index f45a4f9ba23c..0eb178aa618d 100644 >> >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c >> >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) >> >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); >> >> >> >> >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); >> >> >> + >> >> >> + display->dmc.dc6_entry_counter++; >> >> > >> >> >AFAIU the goal is to validate that the display HW can reach the DC6 >> >> >power state. There is no HW DC6 residency counter (and there wasn't such >> >> >a counter earlier either), so an alternative way is required. According >> >> >to the HW team the display driver has programmed everything correctly in >> >> >order to allow the DC6 power state if the DC5 power state is reached >> >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is >> >> >enabled by the driver. >> >> >> >> Yep. That's what I learned as well when looking into this stuff a while >> >> ago. >> >> >> >> >Based on the above, we'd need a DC6 residency counter maintained by the >> >> >driver which is incremented if the DC5 residency counter increments >> >> By the way, the counter that we currently have in our driver is the one >> incremented by the DMC. I was meaning to send a patch for the residency >> counter maintained by the hardware, but have not yet... In theory, that >> one should be more accurate, but would require us to enable and disable >> that counter as the IGT test starts and finishes. >> >> >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough >> >> >for this, since it doesn't take into account the DC5 residency. While >> >> >user space could check both dc6_entry_counter and the DC5 residency, >> >> >that check would be racy wrt. the driver enabling/disabling the DC6 >> >> >state asynchronously. >> >> >> >> I'm not sure doing a driver-maintained dc6 entry counter would be >> >> something worth implementing. Even if we have successfully entered DC5 >> >> and, in theory, DC6 would follow if enabled, this would be a synthetic >> >> counter and it could be masking some hardware bug that could be >> >> preventing DC6. >> > >> >According to the HW team the DC5 residency counter incrementing while >> >DC6 is enabled is a guarantee that the display is configured correctly >> >to allow the HW entering DC6 at all times. IOW this is the HW team's >> >suggestion to validate DC6 at the moment. >> > >> >> On the IGT side, we could just skip if we are on a platform that does >> >> not support DC6 counters, at least while we do not have a reliable >> >> alternative way of checking for DC6. >> > >> >I think IGT would need to validate DC6 in the above way suggested by the >> >HW team. >> >> I'm still inclined to think that we should defer DC6 checking for when >> we actually have a way to verify it. The way suggested above sounds >> like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. >> >> In that case, just checking for DC5 should be enough for the time >> being... > >That's not the same as DC5 incrementing while DC6 is enabled. Ah, I see. You mean that, with DC6 enabled, hardware/firmware flows for DC5 might be different, right? Yeah, that would make sense. In that case, maybe a flag (or counter) from driver that we got into DC5 with DC6 enabled would be helpful indeed. -- Gustavo Sousa > >> I won't object further if we do the other way though. >> >> > >> >> It would be good if we could detect that PG0 was in fact disabled, which >> >> I believe is a stronger indication of DC6. >> > >> >It would be good to have a HW DC6 residency counter, but there isn't one >> >at the moment. Other ways may have a dependency on other, non-display HW >> >blocks, for instance in case of shared clock/voltage resources, the >> >display functionality validation shouldn't be affected by these HW >> >blocks. >> >> As far as I understand by reading the docs, DC6 is DC5 with PG0 >> disabled. That's why my suggestion above. >> >> -- >> Gustavo Sousa >> >> > >> >> -- >> >> Gustavo Sousa >> >> >> >> > >> >> >I suppose the driver could take a snapshot of the DC5 residency counter >> >> >right after it enables DC6 (dc5_residency_start) and increment the SW >> >> >DC6 residency counter right before it disables DC6 or when user space >> >> >reads the DC6 counter. So the driver would update the counter at these >> >> >two points in the following way: >> >> >dc6_residency += dc5_residency_current - dc5_residency_start >> >> > >> >> >The commit log would need a justification, something along the above >> >> >lines. >> >> > >> >> >> } >> >> >> >> >> >> void bxt_enable_dc9(struct intel_display *display) >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >> index 221d3abda791..f51bd8e6011d 100644 >> >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) >> >> >> if (i915_mmio_reg_valid(dc6_reg)) >> >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", >> >> >> intel_de_read(display, dc6_reg)); >> >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); >> >> >> >> >> >> seq_printf(m, "program base: 0x%08x\n", >> >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); >> >> >> -- >> >> >> 2.43.0 >> >> >>
On Mon, Feb 03, 2025 at 06:27:17PM +0200, Imre Deak wrote: > On Mon, Feb 03, 2025 at 11:12:34AM -0500, Rodrigo Vivi wrote: > > On Mon, Feb 03, 2025 at 06:01:25PM +0200, Imre Deak wrote: > > > On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > > > > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > > > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > > > > >> >> due to dc6 counter has been removed to validate DC state. > > > > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > > > >> >> > > > > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > > > > >> >> --- > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > > > > >> >> 3 files changed, 4 insertions(+) > > > > > > >> >> > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > >> >> index 554870d2494b..cc244617011f 100644 > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > > > > >> >> struct { > > > > > > >> >> struct intel_dmc *dmc; > > > > > > >> >> intel_wakeref_t wakeref; > > > > > > >> >> + u32 dc6_entry_counter; > > > > > > >> >> } dmc; > > > > > > >> >> > > > > > > >> >> struct { > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > > > >> >> > > > > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > > > > >> >> + > > > > > > >> >> + display->dmc.dc6_entry_counter++; > > > > > > >> > > > > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > > > > >> >a counter earlier either), so an alternative way is required. According > > > > > > >> >to the HW team the display driver has programmed everything correctly in > > > > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > > > > >> >enabled by the driver. > > > > > > >> > > > > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > > > > >> ago. > > > > > > >> > > > > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > > > > > > > By the way, the counter that we currently have in our driver is the one > > > > > > incremented by the DMC. I was meaning to send a patch for the residency > > > > > > counter maintained by the hardware, but have not yet... In theory, that > > > > > > one should be more accurate, but would require us to enable and disable > > > > > > that counter as the IGT test starts and finishes. > > > > > > > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > > > > >> >state asynchronously. > > > > > > >> > > > > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > > > > >> something worth implementing. Even if we have successfully entered DC5 > > > > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > > > > >> counter and it could be masking some hardware bug that could be > > > > > > >> preventing DC6. > > > > > > > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > > > > >suggestion to validate DC6 at the moment. > > > > > > > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > > > > >> not support DC6 counters, at least while we do not have a reliable > > > > > > >> alternative way of checking for DC6. > > > > > > > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > > > > >HW team. > > > > > > > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > > > > we actually have a way to verify it. The way suggested above sounds > > > > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > > > > > > > In that case, just checking for DC5 should be enough for the time > > > > > > being... > > > > > > > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > > > > > > > I won't object further if we do the other way though. > > > > > > > > > > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > > > > >> I believe is a stronger indication of DC6. > > > > > > > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > > > > >blocks, for instance in case of shared clock/voltage resources, the > > > > > > >display functionality validation shouldn't be affected by these HW > > > > > > >blocks. > > > > > > > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > > > > disabled. That's why my suggestion above. > > > > > > > > > > > > -- > > > > > > Gustavo Sousa > > > > > > > > > > > > > > > > > > > >> -- > > > > > > >> Gustavo Sousa > > > > > > >> > > > > > > >> > > > > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > > > > >> >two points in the following way: > > > > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > > > > > > > Hmm I don't have a good feeling about this. > > > > > > > > I prefer that we are clear to the userspace(IGT) that is an extra flag > > > > and not to pretend that we have a residency counter. > > > > > > > > So, we either are clear that we are counting the entries, or having > > > > a flag that tells that we are allowing dc6. Which btw, could be done > > > > by IGT checking DC6_EN bit directly, no?! > > > > > > A DC6 enabled check alone would be not enough and checking it from user > > > space along with the DC5 counter would be racy as described above. I see > > > this working by the driver tracking the DC6 enabled flag + the DC5 > > > counter in the above way; it could be exposed to user space with a > > > suitable name, eg. dc6_allowed_time. > > > > Right, the name and new entry in the debugfs file would make this > > better because we wouldn't be pretending 'residency', specially > > with no guarantee that it would enter. > > > > However I'd like to keep things simple. Stepping back to see > > what the use case from the test are trying to really > > accomplish: > > > > * SUBTEST: dc6-dpms > > * Description: Validate display engine entry to DC6 state while all connectors's > > * DPMS property set to OFF > > * > > * SUBTEST: dc6-psr > > * Description: This test validates display engine entry to DC6 state while PSR is active > > * Functionality: pm_dc, psr1 > > > > Of course, we already know that it is impossible to validate > > that the display engine itself entered that. But we can > > at least validate that our driver is allowing that condition. > > > > This is with fake residency, with the allowed_time, but also > > with the simple counter that Mohammed added, or also just > > by checking the register directly... > > > > _MMIO(0x45504) & 0x3 == 2 // in idle scenario described above should be enough imho > > 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 commit log would need a justification, something along the above > > > > > > >> >lines. > > > > > > >> > > > > > > > >> >> } > > > > > > >> >> > > > > > > >> >> void bxt_enable_dc9(struct intel_display *display) > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > > >> >> index 221d3abda791..f51bd8e6011d 100644 > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > > > > > >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > > > > > > >> >> if (i915_mmio_reg_valid(dc6_reg)) > > > > > > >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > > > > > > >> >> intel_de_read(display, dc6_reg)); > > > > > > >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > > > > > > >> >> > > > > > > >> >> seq_printf(m, "program base: 0x%08x\n", > > > > > > >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > > > > > > >> >> -- > > > > > > >> >> 2.43.0 > > > > > > >> >>
On Mon, Feb 03, 2025 at 01:37:17PM -0300, Gustavo Sousa wrote: > Quoting Imre Deak (2025-02-03 12:14:10-03:00) > >On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > >> Quoting Imre Deak (2025-02-03 11:26:19-03:00) > >> >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > >> >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > >> >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > >> >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > >> >> >> due to dc6 counter has been removed to validate DC state. > >> >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > >> >> >> > >> >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > >> >> >> --- > >> >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > >> >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > >> >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > >> >> >> 3 files changed, 4 insertions(+) > >> >> >> > >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> >> index 554870d2494b..cc244617011f 100644 > >> >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > >> >> >> @@ -376,6 +376,7 @@ struct intel_display { > >> >> >> struct { > >> >> >> struct intel_dmc *dmc; > >> >> >> intel_wakeref_t wakeref; > >> >> >> + u32 dc6_entry_counter; > >> >> >> } dmc; > >> >> >> > >> >> >> struct { > >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> >> index f45a4f9ba23c..0eb178aa618d 100644 > >> >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > >> >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > >> >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > >> >> >> > >> >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > >> >> >> + > >> >> >> + display->dmc.dc6_entry_counter++; > >> >> > > >> >> >AFAIU the goal is to validate that the display HW can reach the DC6 > >> >> >power state. There is no HW DC6 residency counter (and there wasn't such > >> >> >a counter earlier either), so an alternative way is required. According > >> >> >to the HW team the display driver has programmed everything correctly in > >> >> >order to allow the DC6 power state if the DC5 power state is reached > >> >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > >> >> >enabled by the driver. > >> >> > >> >> Yep. That's what I learned as well when looking into this stuff a while > >> >> ago. > >> >> > >> >> >Based on the above, we'd need a DC6 residency counter maintained by the > >> >> >driver which is incremented if the DC5 residency counter increments > >> > >> By the way, the counter that we currently have in our driver is the one > >> incremented by the DMC. I was meaning to send a patch for the residency > >> counter maintained by the hardware, but have not yet... In theory, that > >> one should be more accurate, but would require us to enable and disable > >> that counter as the IGT test starts and finishes. > >> > >> >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > >> >> >for this, since it doesn't take into account the DC5 residency. While > >> >> >user space could check both dc6_entry_counter and the DC5 residency, > >> >> >that check would be racy wrt. the driver enabling/disabling the DC6 > >> >> >state asynchronously. > >> >> > >> >> I'm not sure doing a driver-maintained dc6 entry counter would be > >> >> something worth implementing. Even if we have successfully entered DC5 > >> >> and, in theory, DC6 would follow if enabled, this would be a synthetic > >> >> counter and it could be masking some hardware bug that could be > >> >> preventing DC6. > >> > > >> >According to the HW team the DC5 residency counter incrementing while > >> >DC6 is enabled is a guarantee that the display is configured correctly > >> >to allow the HW entering DC6 at all times. IOW this is the HW team's > >> >suggestion to validate DC6 at the moment. > >> > > >> >> On the IGT side, we could just skip if we are on a platform that does > >> >> not support DC6 counters, at least while we do not have a reliable > >> >> alternative way of checking for DC6. > >> > > >> >I think IGT would need to validate DC6 in the above way suggested by the > >> >HW team. > >> > >> I'm still inclined to think that we should defer DC6 checking for when > >> we actually have a way to verify it. The way suggested above sounds > >> like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > >> > >> In that case, just checking for DC5 should be enough for the time > >> being... > > > >That's not the same as DC5 incrementing while DC6 is enabled. > > Ah, I see. > > You mean that, with DC6 enabled, hardware/firmware flows for DC5 might > be different, right? Yes, that's possible, or some driver configuration while the driver keeps DC6 enabled would block both DC5 and DC6. > Yeah, that would make sense. In that case, maybe a flag (or counter) > from driver that we got into DC5 with DC6 enabled would be helpful > indeed. > > -- > Gustavo Sousa > > > > >> I won't object further if we do the other way though. > >> > >> > > >> >> It would be good if we could detect that PG0 was in fact disabled, which > >> >> I believe is a stronger indication of DC6. > >> > > >> >It would be good to have a HW DC6 residency counter, but there isn't one > >> >at the moment. Other ways may have a dependency on other, non-display HW > >> >blocks, for instance in case of shared clock/voltage resources, the > >> >display functionality validation shouldn't be affected by these HW > >> >blocks. > >> > >> As far as I understand by reading the docs, DC6 is DC5 with PG0 > >> disabled. That's why my suggestion above. > >> > >> -- > >> Gustavo Sousa > >> > >> > > >> >> -- > >> >> Gustavo Sousa > >> >> > >> >> > > >> >> >I suppose the driver could take a snapshot of the DC5 residency counter > >> >> >right after it enables DC6 (dc5_residency_start) and increment the SW > >> >> >DC6 residency counter right before it disables DC6 or when user space > >> >> >reads the DC6 counter. So the driver would update the counter at these > >> >> >two points in the following way: > >> >> >dc6_residency += dc5_residency_current - dc5_residency_start > >> >> > > >> >> >The commit log would need a justification, something along the above > >> >> >lines. > >> >> > > >> >> >> } > >> >> >> > >> >> >> void bxt_enable_dc9(struct intel_display *display) > >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> >> index 221d3abda791..f51bd8e6011d 100644 > >> >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > >> >> >> @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) > >> >> >> if (i915_mmio_reg_valid(dc6_reg)) > >> >> >> seq_printf(m, "DC5 -> DC6 count: %d\n", > >> >> >> intel_de_read(display, dc6_reg)); > >> >> >> + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); > >> >> >> > >> >> >> seq_printf(m, "program base: 0x%08x\n", > >> >> >> intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0))); > >> >> >> -- > >> >> >> 2.43.0 > >> >> >>
On Mon, Feb 03, 2025 at 11:42:11AM -0500, Rodrigo Vivi wrote: > On Mon, Feb 03, 2025 at 06:27:17PM +0200, Imre Deak wrote: > > On Mon, Feb 03, 2025 at 11:12:34AM -0500, Rodrigo Vivi wrote: > > > On Mon, Feb 03, 2025 at 06:01:25PM +0200, Imre Deak wrote: > > > > On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > > > > > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > > > > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > > > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > > > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > > > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > > > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > > > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > > > > > >> >> due to dc6 counter has been removed to validate DC state. > > > > > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > > > > >> >> > > > > > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > > > > > >> >> --- > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > > > > > >> >> 3 files changed, 4 insertions(+) > > > > > > > >> >> > > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > >> >> index 554870d2494b..cc244617011f 100644 > > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > > > > > >> >> struct { > > > > > > > >> >> struct intel_dmc *dmc; > > > > > > > >> >> intel_wakeref_t wakeref; > > > > > > > >> >> + u32 dc6_entry_counter; > > > > > > > >> >> } dmc; > > > > > > > >> >> > > > > > > > >> >> struct { > > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > > > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > > > > >> >> > > > > > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > > > > > >> >> + > > > > > > > >> >> + display->dmc.dc6_entry_counter++; > > > > > > > >> > > > > > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > > > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > > > > > >> >a counter earlier either), so an alternative way is required. According > > > > > > > >> >to the HW team the display driver has programmed everything correctly in > > > > > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > > > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > > > > > >> >enabled by the driver. > > > > > > > >> > > > > > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > > > > > >> ago. > > > > > > > >> > > > > > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > > > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > > > > > > > > > By the way, the counter that we currently have in our driver is the one > > > > > > > incremented by the DMC. I was meaning to send a patch for the residency > > > > > > > counter maintained by the hardware, but have not yet... In theory, that > > > > > > > one should be more accurate, but would require us to enable and disable > > > > > > > that counter as the IGT test starts and finishes. > > > > > > > > > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > > > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > > > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > > > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > > > > > >> >state asynchronously. > > > > > > > >> > > > > > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > > > > > >> something worth implementing. Even if we have successfully entered DC5 > > > > > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > > > > > >> counter and it could be masking some hardware bug that could be > > > > > > > >> preventing DC6. > > > > > > > > > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > > > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > > > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > > > > > >suggestion to validate DC6 at the moment. > > > > > > > > > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > > > > > >> not support DC6 counters, at least while we do not have a reliable > > > > > > > >> alternative way of checking for DC6. > > > > > > > > > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > > > > > >HW team. > > > > > > > > > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > > > > > we actually have a way to verify it. The way suggested above sounds > > > > > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > > > > > > > > > In that case, just checking for DC5 should be enough for the time > > > > > > > being... > > > > > > > > > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > > > > > > > > > I won't object further if we do the other way though. > > > > > > > > > > > > > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > > > > > >> I believe is a stronger indication of DC6. > > > > > > > > > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > > > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > > > > > >blocks, for instance in case of shared clock/voltage resources, the > > > > > > > >display functionality validation shouldn't be affected by these HW > > > > > > > >blocks. > > > > > > > > > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > > > > > disabled. That's why my suggestion above. > > > > > > > > > > > > > > -- > > > > > > > Gustavo Sousa > > > > > > > > > > > > > > > > > > > > > > >> -- > > > > > > > >> Gustavo Sousa > > > > > > > >> > > > > > > > >> > > > > > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > > > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > > > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > > > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > > > > > >> >two points in the following way: > > > > > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > > > > > > > > > Hmm I don't have a good feeling about this. > > > > > > > > > > I prefer that we are clear to the userspace(IGT) that is an extra flag > > > > > and not to pretend that we have a residency counter. > > > > > > > > > > So, we either are clear that we are counting the entries, or having > > > > > a flag that tells that we are allowing dc6. Which btw, could be done > > > > > by IGT checking DC6_EN bit directly, no?! > > > > > > > > A DC6 enabled check alone would be not enough and checking it from user > > > > space along with the DC5 counter would be racy as described above. I see > > > > this working by the driver tracking the DC6 enabled flag + the DC5 > > > > counter in the above way; it could be exposed to user space with a > > > > suitable name, eg. dc6_allowed_time. > > > > > > Right, the name and new entry in the debugfs file would make this > > > better because we wouldn't be pretending 'residency', specially > > > with no guarantee that it would enter. > > > > > > However I'd like to keep things simple. Stepping back to see > > > what the use case from the test are trying to really > > > accomplish: > > > > > > * SUBTEST: dc6-dpms > > > * Description: Validate display engine entry to DC6 state while all connectors's > > > * DPMS property set to OFF > > > * > > > * SUBTEST: dc6-psr > > > * Description: This test validates display engine entry to DC6 state while PSR is active > > > * Functionality: pm_dc, psr1 > > > > > > Of course, we already know that it is impossible to validate > > > that the display engine itself entered that. But we can > > > at least validate that our driver is allowing that condition. > > > > > > This is with fake residency, with the allowed_time, but also > > > with the simple counter that Mohammed added, or also just > > > by checking the register directly... > > > > > > _MMIO(0x45504) & 0x3 == 2 // in idle scenario described above should be enough imho > > > > 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. --Imre
On Mon, Feb 03, 2025 at 06:51:17PM +0200, Imre Deak wrote: > On Mon, Feb 03, 2025 at 11:42:11AM -0500, Rodrigo Vivi wrote: > > On Mon, Feb 03, 2025 at 06:27:17PM +0200, Imre Deak wrote: > > > On Mon, Feb 03, 2025 at 11:12:34AM -0500, Rodrigo Vivi wrote: > > > > On Mon, Feb 03, 2025 at 06:01:25PM +0200, Imre Deak wrote: > > > > > On Mon, Feb 03, 2025 at 10:45:58AM -0500, Rodrigo Vivi wrote: > > > > > > On Mon, Feb 03, 2025 at 05:14:10PM +0200, Imre Deak wrote: > > > > > > > On Mon, Feb 03, 2025 at 11:59:59AM -0300, Gustavo Sousa wrote: > > > > > > > > Quoting Imre Deak (2025-02-03 11:26:19-03:00) > > > > > > > > >On Mon, Feb 03, 2025 at 10:39:54AM -0300, Gustavo Sousa wrote: > > > > > > > > >> Quoting Imre Deak (2025-02-03 09:43:38-03:00) > > > > > > > > >> >On Mon, Feb 03, 2025 at 02:26:13PM +0530, Mohammed Thasleem wrote: > > > > > > > > >> >> Starting from MTl we don't have a platform agnostic way to validate DC6 state > > > > > > > > >> >> due to dc6 counter has been removed to validate DC state. > > > > > > > > >> >> Adding dc6_entry_counter at display dirver to validate dc6 state. > > > > > > > > >> >> > > > > > > > > >> >> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> > > > > > > > > >> >> --- > > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_core.h | 1 + > > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ > > > > > > > > >> >> drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > > > > > > > > >> >> 3 files changed, 4 insertions(+) > > > > > > > > >> >> > > > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > > >> >> index 554870d2494b..cc244617011f 100644 > > > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > > > > > > > >> >> @@ -376,6 +376,7 @@ struct intel_display { > > > > > > > > >> >> struct { > > > > > > > > >> >> struct intel_dmc *dmc; > > > > > > > > >> >> intel_wakeref_t wakeref; > > > > > > > > >> >> + u32 dc6_entry_counter; > > > > > > > > >> >> } dmc; > > > > > > > > >> >> > > > > > > > > >> >> struct { > > > > > > > > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > > >> >> index f45a4f9ba23c..0eb178aa618d 100644 > > > > > > > > >> >> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > > >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > > > > > > > > >> >> @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) > > > > > > > > >> >> intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); > > > > > > > > >> >> > > > > > > > > >> >> gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); > > > > > > > > >> >> + > > > > > > > > >> >> + display->dmc.dc6_entry_counter++; > > > > > > > > >> > > > > > > > > > >> >AFAIU the goal is to validate that the display HW can reach the DC6 > > > > > > > > >> >power state. There is no HW DC6 residency counter (and there wasn't such > > > > > > > > >> >a counter earlier either), so an alternative way is required. According > > > > > > > > >> >to the HW team the display driver has programmed everything correctly in > > > > > > > > >> >order to allow the DC6 power state if the DC5 power state is reached > > > > > > > > >> >(indicated by the HW DC5 residency counter incrementing) and DC6 is > > > > > > > > >> >enabled by the driver. > > > > > > > > >> > > > > > > > > >> Yep. That's what I learned as well when looking into this stuff a while > > > > > > > > >> ago. > > > > > > > > >> > > > > > > > > >> >Based on the above, we'd need a DC6 residency counter maintained by the > > > > > > > > >> >driver which is incremented if the DC5 residency counter increments > > > > > > > > > > > > > > > > By the way, the counter that we currently have in our driver is the one > > > > > > > > incremented by the DMC. I was meaning to send a patch for the residency > > > > > > > > counter maintained by the hardware, but have not yet... In theory, that > > > > > > > > one should be more accurate, but would require us to enable and disable > > > > > > > > that counter as the IGT test starts and finishes. > > > > > > > > > > > > > > > > >> >while DC6 is enabled. The dc6_entry_counter in this patch is not enough > > > > > > > > >> >for this, since it doesn't take into account the DC5 residency. While > > > > > > > > >> >user space could check both dc6_entry_counter and the DC5 residency, > > > > > > > > >> >that check would be racy wrt. the driver enabling/disabling the DC6 > > > > > > > > >> >state asynchronously. > > > > > > > > >> > > > > > > > > >> I'm not sure doing a driver-maintained dc6 entry counter would be > > > > > > > > >> something worth implementing. Even if we have successfully entered DC5 > > > > > > > > >> and, in theory, DC6 would follow if enabled, this would be a synthetic > > > > > > > > >> counter and it could be masking some hardware bug that could be > > > > > > > > >> preventing DC6. > > > > > > > > > > > > > > > > > >According to the HW team the DC5 residency counter incrementing while > > > > > > > > >DC6 is enabled is a guarantee that the display is configured correctly > > > > > > > > >to allow the HW entering DC6 at all times. IOW this is the HW team's > > > > > > > > >suggestion to validate DC6 at the moment. > > > > > > > > > > > > > > > > > >> On the IGT side, we could just skip if we are on a platform that does > > > > > > > > >> not support DC6 counters, at least while we do not have a reliable > > > > > > > > >> alternative way of checking for DC6. > > > > > > > > > > > > > > > > > >I think IGT would need to validate DC6 in the above way suggested by the > > > > > > > > >HW team. > > > > > > > > > > > > > > > > I'm still inclined to think that we should defer DC6 checking for when > > > > > > > > we actually have a way to verify it. The way suggested above sounds > > > > > > > > like: *trust* that DC6 is reached when DC5 is reached with DC6 enabled. > > > > > > > > > > > > > > > > In that case, just checking for DC5 should be enough for the time > > > > > > > > being... > > > > > > > > > > > > > > That's not the same as DC5 incrementing while DC6 is enabled. > > > > > > > > > > > > > > > I won't object further if we do the other way though. > > > > > > > > > > > > > > > > > > > > > > > > > >> It would be good if we could detect that PG0 was in fact disabled, which > > > > > > > > >> I believe is a stronger indication of DC6. > > > > > > > > > > > > > > > > > >It would be good to have a HW DC6 residency counter, but there isn't one > > > > > > > > >at the moment. Other ways may have a dependency on other, non-display HW > > > > > > > > >blocks, for instance in case of shared clock/voltage resources, the > > > > > > > > >display functionality validation shouldn't be affected by these HW > > > > > > > > >blocks. > > > > > > > > > > > > > > > > As far as I understand by reading the docs, DC6 is DC5 with PG0 > > > > > > > > disabled. That's why my suggestion above. > > > > > > > > > > > > > > > > -- > > > > > > > > Gustavo Sousa > > > > > > > > > > > > > > > > > > > > > > > > > >> -- > > > > > > > > >> Gustavo Sousa > > > > > > > > >> > > > > > > > > >> > > > > > > > > > >> >I suppose the driver could take a snapshot of the DC5 residency counter > > > > > > > > >> >right after it enables DC6 (dc5_residency_start) and increment the SW > > > > > > > > >> >DC6 residency counter right before it disables DC6 or when user space > > > > > > > > >> >reads the DC6 counter. So the driver would update the counter at these > > > > > > > > >> >two points in the following way: > > > > > > > > >> >dc6_residency += dc5_residency_current - dc5_residency_start > > > > > > > > > > > > Hmm I don't have a good feeling about this. > > > > > > > > > > > > I prefer that we are clear to the userspace(IGT) that is an extra flag > > > > > > and not to pretend that we have a residency counter. > > > > > > > > > > > > So, we either are clear that we are counting the entries, or having > > > > > > a flag that tells that we are allowing dc6. Which btw, could be done > > > > > > by IGT checking DC6_EN bit directly, no?! > > > > > > > > > > A DC6 enabled check alone would be not enough and checking it from user > > > > > space along with the DC5 counter would be racy as described above. I see > > > > > this working by the driver tracking the DC6 enabled flag + the DC5 > > > > > counter in the above way; it could be exposed to user space with a > > > > > suitable name, eg. dc6_allowed_time. > > > > > > > > Right, the name and new entry in the debugfs file would make this > > > > better because we wouldn't be pretending 'residency', specially > > > > with no guarantee that it would enter. > > > > > > > > However I'd like to keep things simple. Stepping back to see > > > > what the use case from the test are trying to really > > > > accomplish: > > > > > > > > * SUBTEST: dc6-dpms > > > > * Description: Validate display engine entry to DC6 state while all connectors's > > > > * DPMS property set to OFF > > > > * > > > > * SUBTEST: dc6-psr > > > > * Description: This test validates display engine entry to DC6 state while PSR is active > > > > * Functionality: pm_dc, psr1 > > > > > > > > Of course, we already know that it is impossible to validate > > > > that the display engine itself entered that. But we can > > > > at least validate that our driver is allowing that condition. > > > > > > > > This is with fake residency, with the allowed_time, but also > > > > with the simple counter that Mohammed added, or also just > > > > by checking the register directly... > > > > > > > > _MMIO(0x45504) & 0x3 == 2 // in idle scenario described above should be enough imho > > > > > > 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. 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? 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. > > --Imre >
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. > 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. > 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.
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. > >> 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'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. -- 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.
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 no? not sure why we need to go to the dc9 func... > > -- > 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.
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. >
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. > >
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. >> > >
On Mon, Feb 03, 2025 at 05:19:08PM -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. > > > >> 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'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. Yes, handling 1. and 2. above in gen9_set_dc_state() looks ok to me. > -- > 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.
On Mon, Feb 03, 2025 at 06:18:29PM -0300, Gustavo Sousa wrote: > Quoting Vivi, Rodrigo (2025-02-03 17:59:19-03:00) > [...] > 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. Yes, with the actual delta being: DC5 counter read here - DC5 counter read at (1). > 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). Yes, the orders you described in both (1) and (2) are correct (since a DC5 -> DC6 or DC6 -> DC5 transition is not possible). The dc6_allowed counter should be also updated before returning it to userspace via the debugfs entry, as I mentioned earlier (to account for the case where DC6 is enabled when the read happens). > 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. Yes, that's my understanding. --Imre
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index 554870d2494b..cc244617011f 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -376,6 +376,7 @@ struct intel_display { struct { struct intel_dmc *dmc; intel_wakeref_t wakeref; + u32 dc6_entry_counter; } dmc; struct { diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index f45a4f9ba23c..0eb178aa618d 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -869,6 +869,8 @@ void skl_enable_dc6(struct intel_display *display) intel_dmc_wl_enable(display, DC_STATE_EN_UPTO_DC6); gen9_set_dc_state(display, DC_STATE_EN_UPTO_DC6); + + display->dmc.dc6_entry_counter++; } void bxt_enable_dc9(struct intel_display *display) diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index 221d3abda791..f51bd8e6011d 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -1293,6 +1293,7 @@ static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused) if (i915_mmio_reg_valid(dc6_reg)) seq_printf(m, "DC5 -> DC6 count: %d\n", intel_de_read(display, dc6_reg)); + seq_printf(m, "DC6 entry count: %d\n", display->dmc.dc6_entry_counter); seq_printf(m, "program base: 0x%08x\n", intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0)));
Starting from MTl we don't have a platform agnostic way to validate DC6 state due to dc6 counter has been removed to validate DC state. Adding dc6_entry_counter at display dirver to validate dc6 state. Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com> --- drivers/gpu/drm/i915/display/intel_display_core.h | 1 + drivers/gpu/drm/i915/display/intel_display_power_well.c | 2 ++ drivers/gpu/drm/i915/display/intel_dmc.c | 1 + 3 files changed, 4 insertions(+)