Re: [igt-dev] [PATCH i-g-t v12 4/6] tests/i915/i915_pm_dc: Added test for DC5 during DPMS

Imre Deak
2019-08-27T12:15:22Z

On Tue, Aug 27, 2019 at 05:19:49PM +0530, Gupta, Anshuman wrote:
>
>
> On 8/23/2019 8:00 PM, Imre Deak wrote:
> > On Wed, Aug 14, 2019 at 11:16:21PM +0530, Anshuman Gupta wrote:
> > > From: Jyoti Yadav <jyoti.r.yadav@intel.com>
> > >
> > > Added new subtest for DC5 entry during DPMS on/off cycle.
> > > During DPMS on/off cycle DC5 counter is incremented.
> > >
> > > v2: Rename the subtest with meaningful name.
> > > v3: Rebased.
> > > v4: Addressed review comments by removing leftover code
> > > cleanup().
> > > v5: Addressed the review comment by removing redundant
> > > read_dc_counter() suggested by Imre.
> > > Listing actual change in patch set changelog to make review easier.
> > > v6: Three way patch applied, no functional change.
> > > v7: Disabling runtime suspend for the platform which support, DC9.
> > > rebased due to test name pm_dc changed to i915_pm_dc, aligning to
> > > other PM tests.
> > > v8: Introduced setup_dc_dpms() in order to disable runtime pm, restoring
> > > POWER_DIR values to its original and enabling runtime pm for other
> > > followed sub-tests.
> > > v9: Check DC5 counter value after DPMS off, broke the dpms_on_off
> > > function to dpms_on and dpms_off. [Imre]
> > > v10:Added AT_LEAST_Gen11 condition instead of IS_ICELAKE in order to
> > > disable runtime suspend. [Imre]
> > >
> > > Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > > tests/i915/i915_pm_dc.c | 62 +++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> > > index f261ecbf..f03d30a8 100644
> > > --- a/tests/i915/i915_pm_dc.c
> > > +++ b/tests/i915/i915_pm_dc.c
> > > @@ -46,6 +46,7 @@ typedef struct {
> > > enum psr_mode op_psr_mode;
> > > drmModeModeInfo *mode;
> > > igt_output_t *output;
> > > + bool runtime_suspend_disabled;
> > > } data_t;
> > > bool dc_state_wait_entry(int drm_fd, int dc_flag, int prev_dc_count);
> > > @@ -173,6 +174,62 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
> > > cleanup(data);
> > > }
> > > +static void setup_dc_dpms(data_t *data)
> > > +{
> > > + if (IS_BROXTON(data->devid) || IS_GEMINILAKE(data->devid) ||
> > > + AT_LEAST_GEN(data->devid, 11)) {
> > > + data->runtime_suspend_disabled = igt_disable_runtime_pm();
> > > + igt_require_f(data->runtime_suspend_disabled,
> > > + "unable to disable runtime pm for i915\n");
> > > + } else {
> > > + data->runtime_suspend_disabled = false;
> > > + }
> > > +}
> > > +
> > > +static void dpms_off(data_t *data)
> > > +{
> > > + for (int i = 0; i < data->display.n_outputs; i++) {
> > > + kmstest_set_connector_dpms(data->drm_fd,
> > > + data->display.outputs[i].config.connector,
> > > + DRM_MODE_DPMS_OFF);
> > > + }
> > > +
> > > + if (!data->runtime_suspend_disabled)
> > > + igt_assert(igt_wait_for_pm_status
> > > + (IGT_RUNTIME_PM_STATUS_SUSPENDED));
> > > +}
> > > +
> > > +static void dpms_on(data_t *data)
> > > +{
> > > + for (int i = 0; i < data->display.n_outputs; i++) {
> > > + kmstest_set_connector_dpms(data->drm_fd,
> > > + data->display.outputs[i].config.connector,
> > > + DRM_MODE_DPMS_ON);
> > > + }
> > > +
> > > + if (!data->runtime_suspend_disabled)
> > > + igt_assert(igt_wait_for_pm_status
> > > + (IGT_RUNTIME_PM_STATUS_ACTIVE));
> > > +}
> > > +
> > > +static void test_dc_state_dpms(data_t *data, int dc_flag)
> > > +{
> > > + uint32_t dc_counter;
> > > +
> > > + dc_counter = read_dc_counter(data->drm_fd, dc_flag);
> > > + dpms_off(data);
> > > + check_dc_counter(data->drm_fd, dc_flag, dc_counter);
> > > + dpms_on(data);
> > > +
> > > + /* if runtime PM is disabled for i915 restore it,
> > > + * so any other sub-test can use runtime-PM.
> > > + */
> > > + if (data->runtime_suspend_disabled) {
> > > + igt_restore_runtime_pm();
> > > + igt_setup_runtime_pm();
> > > + }
> >
> > The above restores what setup_dc_dpms() did so could you move it a
> > cleanup_dc_dpms() function for clarity?
> >
> > > +}
> > > +
> > > int main(int argc, char *argv[])
> > > {
> > > bool has_runtime_pm;
> > > @@ -210,6 +267,11 @@ int main(int argc, char *argv[])
> > > test_dc_state_psr(&data, CHECK_DC6);
> > > }
> > > + igt_subtest("dc5-dpms") {
> > > + setup_dc_dpms(&data);
> >
> > Could you move the above call to test_dc_state_dpms() and check for
> I will do this and cleanup_dc_dpms() changes.
> > CHECK_DC5 withing setup_dc_dpms()?
> but i did not understand why do we need to check for CHECK_DC5 (dc flag) in
> setup_dc_dpms(), it is agnostic to DC5 and DC6. dc flag will be require in
> read_dc_counter() and check_dc_counter() function.
> May be it will be clear when i will send entire patch set.

Ah right, missed the dc6 subtest that calls this too. Yes, it's fine to
just do the setup w/o checking for DC5. My point here was just to try to
keep things like the above setup/cleanup functions paired for clarity.

> >
> > I couldn't spot any other issues so with these changes on the patchset:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> >
> > Could you please resend the whole patchset?
> >
> > > + test_dc_state_dpms(&data, CHECK_DC5);
> > > + }
> > > +
> > > igt_fixture {
> > > close(data.debugfs_fd);
> > > display_fini(&data);
> > > --
> > > 2.21.0
> > >