Re: [PATCH i-g-t] tests/kms_bw: Allow custom modes for external panels

[Karthik B S <karthik.b.s@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8167 bytes --]

Hi Kunal,

On 4/14/2026 6:14 PM, Joshi, Kunal1 wrote:
>
> Hello Karthik,
>
> On 07-04-2026 15:22, Karthik B S wrote:
>> Currently the test is rejecting all the outputs where the mode is not
>> matching. This is leading to excessive skips on configs wherer the tests
>> were passing previouly.
>>
>> Instead of this, use default mode for fixed mode panels if the required
>> custom mode is not found. And use custom mode for external panels as
>> these are allowed and have also been passing previously.
>>
>> Cc: Jason-JH Lin<jason-jh.lin@mediatek.com>
>> Cc: Kunal Joshi<kunal1.joshi@intel.com>
>> Signed-off-by: Karthik B S<karthik.b.s@intel.com>
>> ---
>>   tests/kms_bw.c | 57 ++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/tests/kms_bw.c b/tests/kms_bw.c
>> index 4df5c2cee..4a32ec811 100644
>> --- a/tests/kms_bw.c
>> +++ b/tests/kms_bw.c
>> @@ -186,14 +186,16 @@ static void force_output_mode(data_t *d, igt_output_t *output,
>>   	igt_output_override_mode(output, mode);
>>   }
>>   
>> -static bool output_mode_supported(igt_output_t *output, const drmModeModeInfo *mode)
>> +/* Check if output has a matching mode and call it out if mode is being forced */
>> +static void output_mode_supported(igt_output_t *output, const drmModeModeInfo *mode)
> Since the function is now void and purely informational, the name
> output_mode_supported no longer reflects what it does. Consider renaming

Thank you for the review.

Sure will update this.

>>   {
>>   	drmModeConnector *connector = output->config.connector;
>> +	drmModeModeInfo *default_mode;
>>   	int i;
>>   
>>   	/* Virtual/forced sinks support all modes */
>>   	if (!igt_output_is_connected(output))
>> -		return true;
>> +		return;
>>   
>>   	for (i = 0; i < connector->count_modes; i++) {
>>   		drmModeModeInfo *conn_mode = &connector->modes[i];
>> @@ -204,26 +206,32 @@ static bool output_mode_supported(igt_output_t *output, const drmModeModeInfo *m
>>   			igt_debug("Found matching mode for %dx%d@%dHz on %s\n",
>>   				  mode->hdisplay, mode->vdisplay, mode->vrefresh,
>>   				  igt_output_name(output));
>> -			return true;
>>   		}
>>   	}
>>   
>> -	igt_info("Mode %dx%d@%dHz not supported by %s (has %d modes)\n",
>> -		 mode->hdisplay, mode->vdisplay, mode->vrefresh,
>> -		 igt_output_name(output), connector->count_modes);
>> -
>> -	return false;
>> +	if (output_is_internal_panel(output)) {
>> +		default_mode = igt_output_get_mode(output);
>> +		igt_info("Mode %dx%d@%dHz not supported by %s (has %d modes).\n"
>> +			 "%s Default mode: %dx%d@%dHz\n",
>> +			 mode->hdisplay, mode->vdisplay, mode->vrefresh,
>> +			 igt_output_name(output), connector->count_modes, igt_output_name(output),
>> +			 default_mode->hdisplay, default_mode->vdisplay, default_mode->vrefresh);
>> +	} else {
>> +		igt_info("Mode %dx%d@%dHz not supported by %s (has %d modes). Forcing mode.\n",
>> +			 mode->hdisplay, mode->vdisplay, mode->vrefresh,
>> +			 igt_output_name(output), connector->count_modes);
>> +	}
> Previously the matching case returned immediately from inside the loop.
> If that early return is removed without adding equivalent control flow,
> then when a matching mode /is/found the function will still fall through
> and print the "not supported" message afterward. In that case we would end
> up logging both "Found matching mode" and "not supported" for the same
> connector/mode pair.
Will fix this.
>>   }
>>   
>>   static void run_test_linear_tiling(data_t *data, int n_crtcs, const drmModeModeInfo *mode, bool physical) {
>>   	igt_display_t *display = &data->display;
>>   	igt_output_t *output;
>> +	drmModeModeInfo fb_mode;
>>   	struct igt_fb buffer[IGT_MAX_PIPES];
>>   	igt_crc_t zero, captured[IGT_MAX_PIPES];
>>   	int i = 0, num_pipes = 0;
>>   	igt_crtc_t *crtc;
>>   	int ret;
>> -	bool has_supported_mode = false;
>>   
>>   	/* Cannot use igt_display_n_crtcs() due to fused pipes on i915 where they do
>>   	 * not give the numver of valid crtcs and always return IGT_MAX_PIPES */
>> @@ -242,14 +250,26 @@ static void run_test_linear_tiling(data_t *data, int n_crtcs, const drmModeModeI
>>   		crtc = data->crtc[i];
>>   
>>   		output = physical ? data->connected_output[i] : data->output[i];
>> -		if (!output || !output_mode_supported(output, mode)) {
>> +		if (!output)
>>   			continue;
>> -		}
>>   
>> -		force_output_mode(data, output, mode);
>> +		output_mode_supported(output, mode);
>> +
>> +		/*
>> +		 * On fixed mode panels trying to force a custom mode can lead to failures or
>> +		 * implicit handling where the default mode is used even though custom mode is requested.
>> +		 * To avoid this use default mode on fixed mode panels and
>> +		 * use custom modes only on external panels.
>> +		 */
>> +		if (output_is_internal_panel(output)) {
>> +			fb_mode = *igt_output_get_mode(output);
>> +		} else {
>> +			force_output_mode(data, output, mode);
>> +			fb_mode = *mode;
>> +		}
> For internal panels, the subtest will always run at the panel's native 
> mode
> regardless of what mode the subtest requested. So a subtest meant to 
> stress
> e.g. 4K@60 bandwidth will actually run at whatever the native eDP 
> resolution
> is. The test will pass but it won't be validating the bandwidth 
> scenario the
> subtest name implies.
>
> For internal panels where the requested custom mode is not natively 
> supported,
> instead of silently falling back to the default mode, skip that output 
> for that
> specific subtest (also prefer external panel first to reduce skip's),
> since the bandwidth scenario cannot actually be exercised on that panel.
>
> I know we are doing this so we eventually have less skip's, but 
> someone who won't
> know this fact may have false sense of coverage (i.e we tested 4x4k).
>
> Other alternative would be changing test description to state test 
> exercise's standard
> modes on external panel with default mode internal if present. This 
> scenario is more likely as user will
> have internal panel running along with some external display's connected.

This is something even I was contemplating on which direction to take. 
But I leaned on using the fixed mode panel finally, as without that 
we'll completely leave out one of the displays.

I will update the test description and also see if the test name can 
also be updated to call this out more clearly.

Thanks and Regards,
Karthik.B.S
>
> Thanks and Regards
> Kunal Joshi
>>   
>> -		igt_create_color_fb(display->drm_fd, mode->hdisplay,
>> -				    mode->vdisplay, DRM_FORMAT_XRGB8888,
>> +		igt_create_color_fb(display->drm_fd, fb_mode.hdisplay,
>> +				    fb_mode.vdisplay, DRM_FORMAT_XRGB8888,
>>   				    DRM_FORMAT_MOD_LINEAR, 1.f, 0.f, 0.f,
>>   				    &buffer[i]);
>>   
>> @@ -258,10 +278,8 @@ static void run_test_linear_tiling(data_t *data, int n_crtcs, const drmModeModeI
>>   		igt_plane_set_fb(data->primary[i], &buffer[i]);
>>   		igt_info("Assigning pipe %s to output %s with mode %s\n",
>>   			 igt_crtc_name(crtc), igt_output_name(output),
>> -			 mode->name);
>> -		has_supported_mode = true;
>> +			 fb_mode.name);
>>   	}
>> -	igt_skip_on_f(!has_supported_mode, "Unsupported mode for all pipes\n");
>>   
>>   	ret = igt_display_try_commit_atomic(display,
>>   					    DRM_MODE_ATOMIC_ALLOW_MODESET |
>> @@ -273,9 +291,8 @@ static void run_test_linear_tiling(data_t *data, int n_crtcs, const drmModeModeI
>>   
>>   	for (i = 0; i < n_crtcs; i++) {
>>   		output = physical ? data->connected_output[i] : data->output[i];
>> -		if (!output || !output_mode_supported(output, mode)) {
>> +		if (!output)
>>   			continue;
>> -		}
>>   
>>   		igt_pipe_crc_collect_crc(data->pipe_crc[i], &captured[i]);
>>   		igt_assert_f(!igt_check_crc_equal(&zero, &captured[i]),
>> @@ -284,7 +301,7 @@ static void run_test_linear_tiling(data_t *data, int n_crtcs, const drmModeModeI
>>   
>>   	for (i = n_crtcs - 1; i >= 0; i--) {
>>   		output = physical ? data->connected_output[i] : data->output[i];
>> -		if (!output || !output_mode_supported(output, mode))
>> +		if (!output)
>>   			continue;
>>   
>>   		igt_remove_fb(display->drm_fd, &buffer[i]);

[-- Attachment #2: Type: text/html, Size: 10269 bytes --]