[PATCH v3] tests/kms_vrr: Add a subtest for seamless modeset to a virtual LRR mode
Manasi Navare
navaremanasi at chromium.org
Mon Mar 25 22:51:40 UTC 2024
On Thu, Mar 21, 2024 at 10:20 PM Modem, Bhanuprakash
<bhanuprakash.modem at intel.com> wrote:
>
> Hi Manasi,
>
> Thanks for the patch, unfortunately it is not compiling cleanly in my WS
> and getting below warnings:
>
> [2/11] Compiling C object tests/kms_vrr.p/kms_vrr.c.o
> ../tests/kms_vrr.c: In function ‘virtual_rr_vrr_range_mode’:
> ../tests/kms_vrr.c:229:22: warning: unused variable ‘connector’
> [-Wunused-variable]
> 229 | drmModeConnectorPtr connector = output->config.connector;
> | ^~~~~~~~~
> ../tests/kms_vrr.c:227:6: warning: unused variable ‘i’ [-Wunused-variable]
> 227 | int i;
> | ^
> ../tests/kms_vrr.c: In function ‘test_seamless_virtual_rr_basic’:
> ../tests/kms_vrr.c:685:48: warning: passing argument 4 of
> ‘flip_and_measure’ makes pointer from integer without a cast
> [-Wint-conversion]
> 685 | result = flip_and_measure(data, output, pipe, rate, 1,
> TEST_DURATION_NS);
> | ^~~~
> | |
> | uint64_t {aka
> long unsigned int}
> ../tests/kms_vrr.c:413:14: note: expected ‘uint64_t *’ {aka ‘long
> unsigned int *’} but argument is of type ‘uint64_t’ {aka ‘long unsigned
> int’}
> 413 | uint64_t *rates_ns, int num_rates, uint64_t duration_ns)
> | ~~~~~~~~~~^~~~~~~~
> ../tests/kms_vrr.c:700:49: warning: passing argument 4 of
> ‘flip_and_measure’ makes pointer from integer without a cast
> [-Wint-conversion]
> 700 | result = flip_and_measure(data, output, pipe, rate, 1,
> TEST_DURATION_NS);
> | ^~~~
> | |
> | uint64_t {aka
> long unsigned int}
> ../tests/kms_vrr.c:413:14: note: expected ‘uint64_t *’ {aka ‘long
> unsigned int *’} but argument is of type ‘uint64_t’ {aka ‘long unsigned
> int’}
> 413 | uint64_t *rates_ns, int num_rates, uint64_t duration_ns)
> | ~~~~~~~~~~^~~~~~~~
>
Thanks Bhanu, I have fixed these which were new because of rebase over
upstream version.
>
> On 19-03-2024 10:03 pm, Manasi Navare wrote:
> > This subtest validates LRR fastset functionality by testing seamless switching
> > without full modeset to any of the virtual LRR mode within VRR range.
> > Here we start from highest refresh rate mode and then switch to virtual LRR
> > modes in the steps of 10Hz within the VRR range.
> >
> > This is used to test the LRR fastset functionality of the driver.
> >
> > v3:
> > - Fix build error due to rebase (Manasi)
> >
> > Cc: Drew Davenport <ddavenport at chromium.org>
> > Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Sean Paul <seanpaul at chromium.org>
> > Signed-off-by: Manasi Navare <navaremanasi at chromium.org>
> > ---
> > tests/kms_vrr.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> > index 15c62b34b..316b626c3 100644
> > --- a/tests/kms_vrr.c
> > +++ b/tests/kms_vrr.c
> > @@ -70,6 +70,11 @@
> > * Description: Oscillates between highest and lowest refresh each frame for
> > * manual flicker profiling
> > *
> > + * SUBTEST: seamless-virtual-rr-switch
>
> IMHO, we need to update the subtest name as 'seamless-rr-switch-virtual'
> to match with other subtests 'seamless-rr-switch-(drrs | vrr)'.
Sure will update this.
>
> > + * Description: Test to create a Virtual Mode in VRR range and switch to it
> > + * without a full modeset.
> > + * Functionality: LRR
> > + *
> > * SUBTEST: negative-basic
> > * Description: Make sure that VRR should not be enabled on the Non-VRR panel.
> > */
> > @@ -92,11 +97,13 @@ enum {
> > TEST_FASTSET = 1 << 6,
> > TEST_MAXMIN = 1 << 7,
> > TEST_NEGATIVE = 1 << 8,
> > + TEST_SEAMLESS_VIRTUAL_RR = 1 << 9,
>
> IMHO, we could move this enum just after the TEST_SEAMLESS_DRRS or just
> before the TEST_NEGATIVE.
Yes I agree will change this position in the enum
>
> > };
> >
> > enum {
> > HIGH_RR_MODE,
> > LOW_RR_MODE,
> > + VIRTUAL_RR_MODE,
> > RR_MODES_COUNT,
> > };
> >
> > @@ -214,6 +221,19 @@ low_rr_mode_with_same_res(igt_output_t *output, unsigned int vrr_min)
> > return mode;
> > }
> >
> > +static drmModeModeInfo
> > +virtual_rr_vrr_range_mode(igt_output_t *output, unsigned int virtual_refresh_rate)
> > +{
> > + int i;
> > + drmModeConnectorPtr connector = output->config.connector;
>
> Please drop these unused variables 'i' & 'connector'.
Yup done
>
> > + drmModeModeInfo mode = *igt_output_get_mode(output);
> > + uint64_t clock_hz = mode.clock * 1000;
> > +
> > + mode.vtotal = clock_hz / (mode.htotal * virtual_refresh_rate);
> > + mode.vrefresh = virtual_refresh_rate;
>
> Please add a new line here.
Yup done
>
> > + return mode;
> > +}
> > +
> > /* Read min and max vrr range from the connector debugfs. */
> > static range_t
> > get_vrr_range(data_t *data, igt_output_t *output)
> > @@ -641,6 +661,48 @@ test_seamless_rr_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint3
> > vrr ? "on" : "off", vrr ? "not reached" : "exceeded", result);
> > }
> >
> > +static void
> > +test_seamless_virtual_rr_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
> > +{
> > + uint32_t result;
> > + uint64_t rate;
>
> Please declare the variable 'rate' as array or pass it's reference to
> flip_and_measure().
Yup done, changed to fix the warning
>
> > + unsigned int vrefresh;
> > +
> > + igt_info("Use HIGH_RR Mode as default\n");
> > + kmstest_dump_mode(&data->switch_modes[HIGH_RR_MODE]);
> > +
> > + prepare_test(data, output, pipe);
> > + rate = rate_from_refresh(data->switch_modes[HIGH_RR_MODE].vrefresh);
> > +
> > + /*
> > + * Sink with DRR and VRR can be in downclock mode so
> > + * switch to highest refresh rate mode.
> > + */
> > + igt_output_override_mode(output, &data->switch_modes[HIGH_RR_MODE]);
> > + igt_assert(igt_display_try_commit_atomic(&data->display, DRM_MODE_PAGE_FLIP_EVENT, NULL) == 0);
> > +
> > + result = flip_and_measure(data, output, pipe, rate, 1, TEST_DURATION_NS);
> > + igt_assert_f(result > 75,
> > + "Refresh rate (%u Hz) %"PRIu64"ns: Target threshold not reached, result was %u%%\n",
> > + data->switch_modes[HIGH_RR_MODE].vrefresh, rate, result);
> > +
> > + /* Switch to Virtual RR */
> > + for (vrefresh = data->range.min + 10; vrefresh < data->range.max; vrefresh += 10) {
> > +
> Please drop this new line.
Sure
>
> > + data->switch_modes[VIRTUAL_RR_MODE] = virtual_rr_vrr_range_mode(output, vrefresh);
>
> Do we really need this 'VIRTUAL_RR_MODE' enum? Instead we can use a
> local variable.
>
> drmModeModeInfo mode = virtual_rr_vrr_range_mode(output, vrefresh);
Yea I think this new logic now doesnt require VIRTUAL_RR_MODE, earlier
as I only had one virtual mode I needed it.
Will change this to use a local variable.
Thanks for catching this.
>
> > + igt_info("Requesting Virtual Mode with Refresh Rate (%u Hz): \n", vrefresh);
> > + kmstest_dump_mode(&data->switch_modes[VIRTUAL_RR_MODE]);
>
> Please add a new line here.
Sure
Regards
Manasi
>
> > + igt_output_override_mode(output, &data->switch_modes[VIRTUAL_RR_MODE]);
> > + igt_assert(igt_display_try_commit_atomic(&data->display, 0, NULL) == 0);
> > +
> > + rate = rate_from_refresh(vrefresh);
> > + result = flip_and_measure(data, output, pipe, rate, 1, TEST_DURATION_NS);
> > + igt_assert_f(result > 75,
> > + "Refresh rate (%u Hz) %"PRIu64"ns: Target threshold not reached, result was %u%%\n",
> > + vrefresh, rate, result);
> > + }
> > +}
> > +
> > static void test_cleanup(data_t *data, enum pipe pipe, igt_output_t *output)
> > {
> > if (vrr_capable(output))
> > @@ -686,7 +748,7 @@ static bool output_constraint(data_t *data, igt_output_t *output, uint32_t flags
> > igt_output_override_mode(output, &data->switch_modes[HIGH_RR_MODE]);
> >
> > /* Search for a low refresh rate mode. */
> > - if (!(flags & (TEST_SEAMLESS_VRR | TEST_SEAMLESS_DRRS)))
> > + if (!(flags & (TEST_SEAMLESS_VRR | TEST_SEAMLESS_DRRS | TEST_SEAMLESS_VIRTUAL_RR)))
> > return true;
> >
> > data->switch_modes[LOW_RR_MODE] = low_rr_mode_with_same_res(output, data->range.min);
> > @@ -841,6 +903,10 @@ igt_main_args("drs:", long_opts, help_str, opt_handler, &data)
> > "between flip timestamps converges to the requested rate");
> > igt_subtest_with_dynamic("flip-basic-fastset")
> > run_vrr_test(&data, test_basic, TEST_FASTSET);
> > +
> > + igt_describe("Test to switch to any custom virtual mode in VRR range without modeset.");
> > + igt_subtest_with_dynamic("seamless-virtual-rr-switch")
> > + run_vrr_test(&data, test_seamless_virtual_rr_basic, TEST_SEAMLESS_VIRTUAL_RR);
> > }
> >
> > igt_fixture {
More information about the igt-dev
mailing list