[igt-dev] [v5 i-g-t 02/14] tests/kms_frontbuffer_tracking: Fix mode selection for 2x tests

[Modem, Bhanuprakash]

> From: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> Sent: Tuesday, May 11, 2021 9:57 AM
> To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>; Daniel Vetter <daniel.vetter at ffwll.ch>
> Subject: Re: [v5 i-g-t 02/14] tests/kms_frontbuffer_tracking: Fix mode
> selection for 2x tests
> 
> 
> On 5/8/2021 9:52 PM, Bhanuprakash Modem wrote:
> > When two monitors connected through MST, the second monitor also
> > tries to use the same mode. So two such modes may not fit into the
> > link bandwidth.
> >
> > This patch will find a combination of modes that fit into the BW.
> >
> > V2:
> > * Remove MST specific logic (Daniel)
> > V3:
> > * Add support for legacy commit (Ankit)
> >
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > ---
> >   tests/kms_frontbuffer_tracking.c | 40 ++++++++++++++++++++++++++++++++
> >   1 file changed, 40 insertions(+)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> > index 2e74bec6f..65c2ddb5c 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1683,14 +1683,54 @@ static void enable_prim_screen_and_wait(const struct
> test_mode *t)
> >   	do_assertions(ASSERT_NO_ACTION_CHANGE);
> >   }
> >
> > +static void update_modeset_cached_params(void)
> > +{
> > +	bool found = false;
> > +
> > +	igt_output_set_pipe(prim_mode_params.output, prim_mode_params.pipe);
> > +	igt_output_set_pipe(scnd_mode_params.output, scnd_mode_params.pipe);
> > +
> > +	found = igt_override_all_active_output_modes_to_fit_bw(&drm.display);
> > +	igt_require_f(found, "No valid mode combo found.\n");
> > +
> > +	prim_mode_params.mode_copy =
> *igt_output_get_mode(prim_mode_params.output);
> > +	prim_mode_params.mode = &prim_mode_params.mode_copy;
> > +	prim_mode_params.primary.w = prim_mode_params.mode->hdisplay;
> > +	prim_mode_params.primary.h = prim_mode_params.mode->vdisplay;
> > +
> > +	scnd_mode_params.mode_copy =
> *igt_output_get_mode(scnd_mode_params.output);
> > +	scnd_mode_params.mode = &scnd_mode_params.mode_copy;
> > +	scnd_mode_params.primary.w = scnd_mode_params.mode->hdisplay;
> > +	scnd_mode_params.primary.h = scnd_mode_params.mode->vdisplay;
> > +
> > +	fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
> > +	fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
> > +
> > +	__set_mode_for_params(&prim_mode_params);
> > +	__set_mode_for_params(&scnd_mode_params);
> > +}
> > +
> >   static void enable_both_screens_and_wait(const struct test_mode *t)
> >   {
> > +	int ret;
> > +
> >   	fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
> >   	fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
> >
> >   	__set_mode_for_params(&prim_mode_params);
> >   	__set_mode_for_params(&scnd_mode_params);
> >
> > +	if (drm.display.is_atomic)
> > +		ret = igt_display_try_commit_atomic(&drm.display,
> > +				DRM_MODE_ATOMIC_TEST_ONLY |
> > +				DRM_MODE_ATOMIC_ALLOW_MODESET,
> > +				NULL);
> > +	else
> > +		ret = igt_display_try_commit2(&drm.display, COMMIT_LEGACY);
> > +
> > +	if (ret)
> > +		update_modeset_cached_params();
> > +
> >   	igt_display_commit2(&drm.display, drm.display.is_atomic ? COMMIT_ATOMIC
> : COMMIT_LEGACY);
> 
> I think this statement should now be the part of the above if-block.

I think still we need this commit, since try_commit_atomic with the flag
DRM_MODE_ATOMIC_TEST_ONLY will do atomic check only and it really won't commit.
Also, we'll miss some display prop clean-up display_commit_changed() with both legacy
and atomic try commit.

This comment is applicable for all patches in this series.
 
> 
> In case either of the try commit passes without need to call
> update_modeset_chached_params(), igt_display_commit again will be redundant.
> 
> With that changed:
> 
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> 
> >
> >   	wanted_crc = &blue_crcs[t->format].crc;