[PATCH i-g-t] tests/intel/kms_cdclk: Refactor mode setting and CD clock verification
Reddy Guddati, Santhosh
santhosh.reddy.guddati at intel.com
Tue Dec 24 05:47:53 UTC 2024
On 23-12-2024 12:20, Nautiyal, Ankit K wrote:
>
> On 12/13/2024 11:32 AM, Swati Sharma wrote:
>> Add conditions to filter valid outputs. Encapsulate the
>> mode-setting functionality in the set_mode function to reduce
>> redundancy. Remove redundant framebuffer and size setting
>> operations for the plane. Simplify the CD clock frequency
>> checks to ensure it increases after the mode transition. Clean
>> up unnecessary plane operations and redundant framebuffer
>> handling. Enhance the readability and modularity of the mode
>> transition logic to handle multiple outputs more effectively.
>
> This patch is doing more things along with refactoring like skipping if
> there is only one mode, and checking if there are minimum 2 displays.
>
> So lets refactor be one patch, with no functional change.
>
> Then add other enhancements etc.
>
>
>>
>> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
>> ---
>> tests/intel/kms_cdclk.c | 112 +++++++++++++++++++---------------------
>> 1 file changed, 54 insertions(+), 58 deletions(-)
>>
>> diff --git a/tests/intel/kms_cdclk.c b/tests/intel/kms_cdclk.c
>> index 382b3e9d1..93276f45e 100644
>> --- a/tests/intel/kms_cdclk.c
>> +++ b/tests/intel/kms_cdclk.c
>> @@ -54,6 +54,7 @@ IGT_TEST_DESCRIPTION("Test cdclk features : crawling
>> and squashing");
>> #define VDISPLAY_4K 2160
>> #define VREFRESH 60
>> #define MAX_CDCLK_4K 307200
>> +#define MIN_DISPLAYS 2
>> /* Test flags */
>> enum {
>> @@ -269,90 +270,86 @@ static void test_mode_transition(data_t *data,
>> enum pipe pipe, igt_output_t *out
>> igt_remove_fb(display->drm_fd, &fb);
>> }
>> +
>> +static void set_mode(data_t *data, drmModeModeInfo *mode, int count,
>> + struct igt_fb fb, igt_output_t **valid_outputs)
>
> let count be after data, as this is count of modes and valid outputs.
> Perhaps a better order will be:
>
> set_mode(data_t *data, int count, drmModeModeInfo *mode, igt_output_t
> **valid_outputs, struct igt_fb fb)
>
>> +{
>> + igt_display_t *display = &data->display;
>> + igt_pipe_t *pipe;
>> + igt_plane_t *plane;
>> +
>> + for (int k = 0; k < count; k++) {
>
> lets use i for iterator.
>
>
>> + pipe = &display->pipes[k];
>> + plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>> +
>> + igt_output_override_mode(valid_outputs[k], &mode[k]);
>> +
>> + igt_plane_set_fb(plane, &fb);
>> + igt_fb_set_size(&fb, plane, mode[k].hdisplay, mode[k].vdisplay);
>> + igt_plane_set_size(plane, mode[k].hdisplay, mode[k].vdisplay);
>> + }
>> +}
>> +
>> static void test_mode_transition_on_all_outputs(data_t *data)
>> {
>> igt_display_t *display = &data->display;
>> int debugfs_fd = data->debugfs_fd;
>> - drmModeModeInfo *mode, *mode_hi, *mode_lo;
>> + drmModeModeInfo *mode, mode_hi[IGT_MAX_PIPES] = {0},
>> mode_lo[IGT_MAX_PIPES] = {0};
>> + igt_output_t *valid_outputs[IGT_MAX_PIPES] = {NULL};
>> igt_output_t *output;
>> - int valid_outputs = 0;
>> + int count = 0;
>> int cdclk_ref, cdclk_new;
>> uint16_t width = 0, height = 0;
>> struct igt_fb fb;
>> - igt_pipe_t *pipe;
>> - igt_plane_t *plane;
>> - int i = 0, j = 0;
>> do_cleanup_display(display);
>> igt_display_reset(display);
>> - for_each_connected_output(&data->display, output)
>> - valid_outputs++;
>> -
>> - i = 0;
>> for_each_connected_output(display, output) {
>> - mode = igt_output_get_mode(output);
>> - igt_assert(mode);
>> + const drmModeModeInfo *highres_mode;
>> + const drmModeModeInfo *lowres_mode;
>> - width = max(width, mode->hdisplay);
>> - height = max(height, mode->vdisplay);
>> + highres_mode = get_highres_mode(output);
>> Can we move the get_highres_mode from test functions to lib and re
use in other tests kms_dsc, kms_display_modes, in fact here we are
restricting highest modes to 4k, imho it would be good to sort the modes
to get highest mode.
>> + igt_require(highres_mode != NULL);
>> + mode_hi[count] = *highres_mode;
>> - mode_hi = get_highres_mode(output);
>> - igt_require(mode_hi != NULL);
>> + lowres_mode = get_lowres_mode(output);
>> + mode_lo[count] = *lowres_mode;
>
> I think it will be better to set the mode_hi and mode_lo later after we
> set valid_output.
>
> If both are equal we are overwriting mode_hi and mode_lo.
>
>
>> - igt_output_set_pipe(output, i);
>> - igt_output_override_mode(output, mode_hi);
>> - i++;
>> - }
>> - igt_require(intel_pipe_output_combo_valid(display));
>> - igt_display_reset(display);
>> + if (mode_hi[count].hdisplay == mode_lo[count].hdisplay &&
>> + mode_hi[count].vdisplay == mode_lo[count].vdisplay) {
> In line with above comment, use highres_mode and lowres_mode to compare.
>> + igt_debug("Highest and lowest mode resolutions are same;
>> no transition\n");
>> + continue;
>> + }
>> - igt_create_pattern_fb(data->drm_fd, width, height,
>> DRM_FORMAT_XRGB8888,
>> - DRM_FORMAT_MOD_LINEAR, &fb);
>> - i = 0;
>> - for_each_connected_output(display, output) {
>> - pipe = &display->pipes[i];
>> - plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>> + valid_outputs[count++] = output;
>> + }
>> - mode = NULL;
>> + igt_skip_on_f(count < MIN_DISPLAYS,
>> + "Number of valid outputs (%d) must be greater than or
>> equal to the "
>> + "minimum required displays (%d)\n", count, MIN_DISPLAYS);
>
> I think what we mean here is that the count should be more than 1,
> meaning there should be more than 1 output.
>
> We dont need to have a macro MIN_DISPLAYS as count >= 1 is self
> explanatory.
>
> Regards,
>
> Ankit
>
>> - igt_output_set_pipe(output, i);
>> - mode = igt_output_get_mode(output);
>> + for (int i = 0; i < count; i++) {
>> + mode = igt_output_get_mode(valid_outputs[i]);
>> igt_assert(mode);
>> - mode_lo = get_lowres_mode(output);
>> + width = max(width, mode->hdisplay);
>> + height = max(height, mode->vdisplay);
>> - igt_output_override_mode(output, mode_lo);
>> - igt_plane_set_fb(plane, &fb);
>> - igt_fb_set_size(&fb, plane, mode_lo->hdisplay, mode_lo-
>> >vdisplay);
>> - igt_plane_set_size(plane, mode_lo->hdisplay, mode_lo->vdisplay);
>> - i++;
>> + igt_output_set_pipe(valid_outputs[i], i);
>> + igt_output_override_mode(valid_outputs[i], &mode_hi[i]);
>> }
>> - igt_display_commit2(display, COMMIT_ATOMIC);
>> - cdclk_ref = get_current_cdclk_freq(debugfs_fd);
>> -
>> - j = 0;
>> - for_each_connected_output(display, output) {
>> - pipe = &display->pipes[j];
>> - plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>> -
>> - mode = NULL;
>> -
>> - igt_output_set_pipe(output, j);
>> - mode = igt_output_get_mode(output);
>> - igt_assert(mode);
>> + igt_require(intel_pipe_output_combo_valid(display));
>> - mode_hi = get_highres_mode(output);
>> - igt_require(mode_hi != NULL);
>> + igt_create_pattern_fb(data->drm_fd, width, height,
>> DRM_FORMAT_XRGB8888,
>> + DRM_FORMAT_MOD_LINEAR, &fb);
>> - igt_output_override_mode(output, mode_hi);
>> - igt_plane_set_fb(plane, &fb);
>> - igt_fb_set_size(&fb, plane, mode_hi->hdisplay, mode_hi-
>> >vdisplay);
>> - igt_plane_set_size(plane, mode_hi->hdisplay, mode_hi->vdisplay);
>> - j++;
>> - }
>> + set_mode(data, mode_lo, count, fb, valid_outputs);
>> + igt_display_commit2(display, COMMIT_ATOMIC);
>> + cdclk_ref = get_current_cdclk_freq(debugfs_fd);
>> + set_mode(data, mode_hi, count, fb, valid_outputs);
>> igt_display_commit2(display, COMMIT_ATOMIC);
>> cdclk_new = get_current_cdclk_freq(debugfs_fd);
>> igt_info("CD clock frequency %d -> %d\n", cdclk_ref, cdclk_new);
>> @@ -360,7 +357,6 @@ static void
>> test_mode_transition_on_all_outputs(data_t *data)
>> /* cdclk should bump */
>> igt_assert_lt(cdclk_ref, cdclk_new);
>> - igt_plane_set_fb(plane, NULL);
>> do_cleanup_display(display);
>> igt_remove_fb(data->drm_fd, &fb);
>> }
More information about the igt-dev
mailing list