Re: [igt-dev] [PATCH i-g-t] tests/kms_concurrent: Skip the subtest if the resolution is not supported

["Kahola, Mika" <mika.kahola@intel.com>]

> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Gupta,
> Nidhi1
> Sent: Thursday, May 5, 2022 3:30 PM
> To: B S, Karthik <karthik.b.s@intel.com>; igt-dev@lists.freedesktop.org
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_concurrent: Skip the subtest if the
> resolution is not supported
> 
> 
> 
> -----Original Message-----
> From: B S, Karthik <karthik.b.s@intel.com>
> Sent: Thursday, May 5, 2022 11:24 AM
> To: Gupta, Nidhi1 <nidhi1.gupta@intel.com>; igt-dev@lists.freedesktop.org
> Subject: Re: [PATCH i-g-t] tests/kms_concurrent: Skip the subtest if the
> resolution is not supported
> 
> On 5/5/2022 3:30 AM, Nidhi Gupta wrote:
> > The kms_concurrent is about testing mode setting with reducing the
> > resolution and then again increasing it, for this the test will take
> > the high resolution supported by the connector and then calculate the
> > lowest resolution, if the calculated resolution is not supported by
> > the connector it will assign the default resolution of 1024x 768
> > without checking.
> >
> > Adding the check to skip the subtest if the default resolution is not
> > supported by the connector.
> 
> Hi,
> 
> Please update the commit message. We're skipping if , "mode_lo->vdisplay  >
> mode_hi->vdisplay". Mention the same.
> 
> Also, I think its better to move this check inside "get_lowres_mode()". Before
> returning 1024x768 mode, we could use the same check used in the loop,
> "mode->vdisplay < limit", and skip the test if this is not met.
> 
> Thanks,
> Karthik.B.S
> >
> > Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> > ---
> >   tests/kms_concurrent.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/tests/kms_concurrent.c b/tests/kms_concurrent.c index
> > 82b2021e..fdef6b26 100644
> > --- a/tests/kms_concurrent.c
> > +++ b/tests/kms_concurrent.c
> > @@ -272,6 +272,8 @@ test_resolution_with_output(data_t *data, enum pipe
> pipe, int max_planes, igt_ou
> >   		mode_hi = igt_output_get_mode(output);
> >   		mode_lo = get_lowres_mode(data, mode_hi, output);
> >
> > +		igt_skip_on(mode_lo->vdisplay > mode_hi->vdisplay);
> > +
> >   		/* switch to lower resolution */
> >   		igt_output_override_mode(output, mode_lo);
> >   		free(mode_lo);
> Hi Karthik,
> 
> Thanks for your review, but if I use mode->vdisplay < limit, its skipping for eDP
> also, because for eDP mode with resolution less then limit (limit =
> mode_default->vdisplay - SIZE_PLANE) is not available but 1024x768 is available.
> 
> I tried to execute also below are the results:
> gta@sad-gta-dut:~/drivers.gpu.i915.igt-gpu-tools$ sudo
> LD_LIBRARY_PATH=/usr/local/lib:/usr/local/lib/x86_64-linux-gnu/
> build/tests/kms_concurrent
> IGT-Version: 1.26-g6ba88471c25 (x86_64) (Linux: 5.10.54-DII_5677-
> gd13fb75fff37+ x86_64) Opened device: /dev/dri/card0 Starting subtest: pipe-A
> Starting dynamic subtest: eDP-1 Testing resolution with connector eDP-1 using
> pipe A with seed 1651753436 Test requirement not met in function
> get_lowres_mode, file ../tests/kms_concurrent.c:256:
> Test requirement: !(mode->vdisplay > limit) Dynamic subtest eDP-1: SKIP (0.003s)
> Starting dynamic subtest: HDMI-A-1 Testing resolution with connector HDMI-A-1
> using pipe A with seed 1651753436 Test requirement not met in function
> get_lowres_mode, file ../tests/kms_concurrent.c:256:
> Test requirement: !(mode->vdisplay > limit) Dynamic subtest HDMI-A-1: SKIP
> (0.003s)

Hi!

I had as similar idea in mind that Karthik already had. But obviously this is approach is not really working if default mode 1024x768 is supported by the panel.

Originally, this get_lowres_mode() function as a direct copy from the one used by kms_plane_lowres.c. At some point of time, these two get_lowres_mode() functions evolved to be a bit different. Maybe, this would be a time and a place to unify these two different get_lowres_mode() functions or even move this function to be part of a library routines.

Anyway, back with the igt_skip_on(). 

Maybe we could add a comment on igt_skip_on() to give a reason, why we skipped. Maybe something like "No suitable resolution was found". 

Cheers,
Mika