Re: [PATCH i-g-t v4 1/2] lib/igt_vrr:Add VRR helper library for display refresh rate testing

["Naladala, Ramanaidu" <Ramanaidu.naladala@intel.com>]

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

Hi Chaitanya,

On 8/20/2026 4:38 PM, Borah, Chaitanya Kumar wrote:
>
> Hello Rama,
>
> On 8/12/2026 6:37 PM, Naladala Ramanaidu wrote:
>> Introduce a new helper library for Variable Refresh Rate (VRR).
>>
>> Add helpers to validate targeted refresh-rate testing.
>>
>> v2: Modify debugfs with helpers.
>> v3: Add helper to check cmrr support.
>>      Address review comments. (Mitul)
>>
>> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala@intel.com>
>> ---
>>   lib/igt_vrr.c   | 154 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_vrr.h   |  44 ++++++++++++++
>>   lib/meson.build |   1 +
>>   3 files changed, 199 insertions(+)
>>   create mode 100644 lib/igt_vrr.c
>>   create mode 100644 lib/igt_vrr.h
>>
>> diff --git a/lib/igt_vrr.c b/lib/igt_vrr.c
>> new file mode 100644
>> index 000000000..704eaa3a3
>> --- /dev/null
>> +++ b/lib/igt_vrr.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#include <inttypes.h>
>> +
>> +#include "igt_vrr.h"
>> +#include "igt_sysfs.h"
>> +
>> +const uint32_t igt_vrr_standard_video_timing_fps[] = {
>> +    24, 25, 30, 48, 50, 60, 75, 90, 96, 100, 120, 144, 165, 180, 
>> 200, 240,
>> +};
>> +
>
> Please cite the source/rationale for these RRs
Sure. I will address this in next revision.
>
>> +const size_t igt_vrr_standard_video_timing_fps_count =
>> +    ARRAY_SIZE(igt_vrr_standard_video_timing_fps);
>> +
>> +/**
>> + * igt_target_rr_debugfs_write:
>> + * @fd: DRM file descriptor.
>> + * @crtc_index: Index of the CRTC.
>> + * @vrefresh: Target refresh rate to program.
>> + * @numerator: Numerator component of the target refresh rate fraction.
>> + * @denominator: Denominator component of the target refresh rate 
>> fraction.
>> + *
>> + * Write the target refresh rate configuration to the per-CRTC
>> + * VRR debugfs interface.
>> + *
>> + * Returns: None.
>> + */
>> +void
>> +igt_target_rr_debugfs_write(int fd, int crtc_index,
>> +                uint32_t vrefresh,
>> +                uint32_t numerator,
>> +                uint32_t denominator)
>
> You can simplify the arguments to rr_numerator and rr_denominator. 
> Skip vrefresh.
Sure. I will address this in next revision.
>
>> +{
>> +    char buf[32];
>> +    int ret, dir;
>> +    uint64_t val;
>> +
>> +    val = vrefresh * numerator;
>> +
>> +    snprintf(buf, sizeof(buf), "%" PRIu64 "/%u",
>> +         val, denominator);
>> +
>> +    dir = igt_debugfs_crtc_dir(fd, crtc_index);
>> +    igt_require_fd(dir);
>> +
>> +    ret = igt_sysfs_write(dir, "intel_vrr_target_refresh_rate",
>> +                  buf, sizeof(buf) - 1);
>
> Better to use strlen so that we don't send out any garbage to kernel.
Sure. i will fix this in next revision.
>
>
> Also what is the policy regarding using intel specific debugfs in lib?
> Should there be a wrapper to abstract it?
I will update the debugfs operations to be Intel-specific.
>
>> +     close(dir);
>> +     igt_assert_f(ret == (sizeof(buf) - 1), "debugfs_write failed");
>> +}
>> +
>> +/**
>> + * igt_cmrr_debugfs_read:
>
> needs update.
sure.
>
>> + * @fd: DRM file descriptor.
>> + * @crtc_index: Index of the CRTC.
>> + *
>> + * Read the configured refresh rate from the per-CRTC VRR debugfs node.
>> + *
>> + * Return: None.
>> + */
>> +void
>> +igt_target_rr_debugfs_read(int fd, int crtc_index)
>> +{
>> +    char buf[32];
>> +    int ret, dir;
>> +
>> +    dir = igt_debugfs_crtc_dir(fd, crtc_index);
>> +    igt_require_fd(dir);
>> +
>> +    ret = igt_sysfs_read(dir, "intel_vrr_target_refresh_rate",
>> +                 buf,  sizeof(buf) - 1);
>> +    close(dir);
>> +    igt_assert_f(ret >= 0,
>> +             "Failed to read intel_vrr_target_refresh_rate.\n");
>> +
>> +    buf[ret] = '\0';
>> +
>> +    igt_info("vrr target RR: %s\n", buf);
>> +}
>> +
>> +/**
>> + * igt_vrr_mode_line_refresh_hz:
>> + * @mode: DRM display mode used for the calculation
>> + *
>> + * Compute the refresh rate directly from the mode timing parameters.
>> + *
>> + * Returns: Refresh rate in Hz as a floating-point value.
>> + */
>> +double igt_vrr_mode_line_refresh_hz(const drmModeModeInfo *mode)
>> +{
>> +    return (double)mode->clock * 1000.0 / ((double)mode->htotal * 
>> (double)mode->vtotal);
>> +}
>> +
>> +/**
>> + * igt_vrr_get_mode_with_video_timing:
>> + * @output: Display output containing connector mode list
>> + * @fps: Requested integer refresh rate in Hz
>> + * @matched_mode: Returned mode that matches @fps
>> + *
>> + * Find and return a connector mode that matches the requested
>> + * video timing refresh rate in Hz.
>> + *
>> + * Returns: true when a mode is found, false otherwise
>> + */
>> +
>> +bool igt_vrr_get_mode_with_video_timing(igt_output_t *output,
>> +                    uint32_t fps,
>> +                    drmModeModeInfo *matched_mode)
>
> This helper seems to be only relevant to CMRR for now. So better to 
> move it to the test file? Do you see it being used elsewhere?

Currently CMRR is the only user, but the helper itself is generic and 
not CMRR-specific. I expect it to be useful for other VRR/video timing 
tests that need to look up a mode by FPS,

so I'd prefer to keep it in the library. If we want to keep the API 
surface minimal, I can move it to the test for now and promote it later 
when another user appears.

>
>> +{
>> +    drmModeConnectorPtr connector;
>> +
>> +    connector = output->config.connector;
>> +    if (!connector)
>> +        return false;
>> +
>> +    for (int i = 0; i < connector->count_modes; i++) {
>> +        if (connector->modes[i].vrefresh == fps) {
>> +            *matched_mode = connector->modes[i];
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +/**
>> + * cmrr_supported:
>> + * @fd: DRM device file descriptor.
>> + * @crtc_index: Index of the CRTC.
>> + *
>> + * Checks whether the intel_vrr_target_refresh_rate debugfs node is 
>> present
>> + * for the specified CRTC, indicating CMRR support.
>> + *
>> + * Returns: true if CMRR is supported, false otherwise.
>> + */
>> +bool cmrr_supported(int fd, int crtc_index)
>
> You can rename it to target_refresh_rate_supported.
sure. i will fix this in next revision.
>
>> +{
>> +    int dir;
>> +
>> +    dir = igt_debugfs_crtc_dir(fd, crtc_index);
>> +
>> +    if (dir < 0)
>> +        return false;
>> +
>> +    if (faccessat(dir, "intel_vrr_target_refresh_rate", F_OK, 0) == 
>> 0) {
>> +        close(dir);
>> +        return true;
>> +    }
>> +
>> +    close(dir);
>> +    return false;
>> +}
>> diff --git a/lib/igt_vrr.h b/lib/igt_vrr.h
>> new file mode 100644
>> index 000000000..a317d4684
>> --- /dev/null
>> +++ b/lib/igt_vrr.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#ifndef IGT_VRR_H
>> +#define IGT_VRR_H
>> +
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include "igt.h"
>> +#include "igt_kms.h"
>> +
>> +#define CMRR_NUMERATOR 1000ULL
>> +#define CMRR_DENOMINATOR 1000ULL
>> +#define CMRR_VIDEO_MODE_DENOMINATOR 1001ULL
>> +#define TARGET_RR_SAMP_COUNT 100
>> +
>> +enum {
>> +    CMRR_VIDEO_MODE,
>> +    CMRR_NON_VIDEO_MODE,
>> +    CMRR_DISABLE,
>> +};
>> +
>
> move these macros and enums out to the test file.
sure. i will fix this in next revision.
>
> ==
> Chaitanya
>
>> +extern const uint32_t igt_vrr_standard_video_timing_fps[];
>> +extern const size_t igt_vrr_standard_video_timing_fps_count;
>> +
>> +void
>> +igt_target_rr_debugfs_write(int fd, int crtc_index,
>> +                uint32_t vrefresh,
>> +                uint32_t numerator,
>> +                uint32_t denominator);
>> +void
>> +igt_target_rr_debugfs_read(int fd, int crtc_index);
>> +
>> +double igt_vrr_mode_line_refresh_hz(const drmModeModeInfo *mode);
>> +
>> +bool igt_vrr_get_mode_with_video_timing(igt_output_t *output,
>> +                    uint32_t fps,
>> +                    drmModeModeInfo *matched_mode);
>> +
>> +bool cmrr_supported(int fd, int crtc_index);
>> +
>> +#endif
>> diff --git a/lib/meson.build b/lib/meson.build
>> index 3001b473e..8675bd4a6 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -22,6 +22,7 @@ lib_sources = [
>>       'igt_configfs.c',
>>       'igt_facts.c',
>>       'igt_crc.c',
>> +        'igt_vrr.c',
>>       'igt_debugfs.c',
>>       'igt_device.c',
>>       'igt_device_scan.c',
>

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