Re: [PATCH i-g-t 2/2] lib/igt_kms: add support for choosing big joiner mode

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 1/18/2024 2:44 PM, Joshi, Kunal1 wrote:
> Hello Bhanu,
>
> On 1/18/2024 2:40 PM, Modem, Bhanuprakash wrote:
>>
>> On 17-01-2024 02:44 pm, Joshi, Kunal1 wrote:
>>> Hello Swati and Bhanu,
>>>
>>> On 1/17/2024 2:25 PM, Sharma, Swati2 wrote:
>>>> Hi Kunal,
>>>>
>>>> On 17-Jan-24 11:14 AM, Joshi, Kunal1 wrote:
>>>>> Hello Swati,
>>>>>
>>>>> On 1/16/2024 5:33 PM, Sharma, Swati2 wrote:
>>>>>> Hi Kunal,
>>>>>>
>>>>>> On 16-Jan-24 5:29 PM, Joshi, Kunal1 wrote:
>>>>>>> Hello Swati,
>>>>>>>
>>>>>>> On 1/16/2024 4:34 PM, Sharma, Swati2 wrote:
>>>>>>>> Hi Kunal,
>>>>>>>>
>>>>>>>> On 15-Jan-24 4:28 PM, Kunal Joshi wrote:
>>>>>>>>> add support to choose big joiner mode with environment
>>>>>>>>> variable, use mode with highest clock if no mode with big joiner
>>>>>>>>> found.
>>>>>>>>>
>>>>>>>>> v2: reuse bigjoiner_mode_found (Bhanu)
>>>>>>>>> v3: avoid returning from multiple places (Bhanu)
>>>>>>>>>      avoid frequent debugfs reads (Bhanu)
>>>>>>>>>
>>>>>>>>> Cc: Karthik B S <karthik.b.s@intel.com>
>>>>>>>>> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>>>>>>>>> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
>>>>>>>>> ---
>>>>>>>>>   lib/igt_kms.c | 24 +++++++++++++++++++-----
>>>>>>>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>>>>>>>> index cb6d57c2d..2c4210d4b 100644
>>>>>>>>> --- a/lib/igt_kms.c
>>>>>>>>> +++ b/lib/igt_kms.c
>>>>>>>>> @@ -1771,8 +1771,9 @@ void 
>>>>>>>>> igt_sort_connector_modes(drmModeConnector *connector,
>>>>>>>>>   bool kmstest_get_connector_default_mode(int drm_fd, 
>>>>>>>>> drmModeConnector *connector,
>>>>>>>>>                       drmModeModeInfo *mode)
>>>>>>>>>   {
>>>>>>>>> +    bool found;
>>>>>>>>>       char *env;
>>>>>>>>> -    int i;
>>>>>>>>> +    int i, max_dotclock;
>>>>>>>>>         if (!connector->count_modes) {
>>>>>>>>>           igt_warn("no modes for connector %d\n",
>>>>>>>>> @@ -1781,21 +1782,34 @@ bool 
>>>>>>>>> kmstest_get_connector_default_mode(int drm_fd, 
>>>>>>>>> drmModeConnector *connector,
>>>>>>>>>       }
>>>>>>>>>         env = getenv("IGT_KMS_RESOLUTION");
>>>>>>>>> +    max_dotclock = igt_get_max_dotclock(drm_fd);
>>>>>>>>>       if (env) {
>>>>>>>>>           /*
>>>>>>>>> -         * Only (0 or 1) and (lowest or highest) are allowed.
>>>>>>>>> +         * Only (0 or 1 or 2) and (lowest or highest or 
>>>>>>>>> joiner) are allowed.
>>>>>>>>>            *
>>>>>>>>>            * 0/lowest: Choose connector mode with lowest 
>>>>>>>>> possible resolution.
>>>>>>>>>            * 1/highest: Choose connector mode with highest 
>>>>>>>>> possible resolution.
>>>>>>>>> +         * 2/joiner: Choose connector mode with bigjoiner 
>>>>>>>>> support or with
>>>>>>>>> +                 highest clock if can't support big joiner
>>>>>>>>
>>>>>>>> Shouldn't this be only joiner mode?
>>>>>>>>
>>>>>>>>
>>>>>>> You mean can be named as joiner mode because same can be used 
>>>>>>> for big joiner / ultra joiner?
>>>>>>
>>>>>> I guess this should be bigjoiner mode only. For ultra there might 
>>>>>> be other restrictions which we need to look at.
>>>>>>
>>>>>>> or
>>>>>>> We are returning the highest clock mode if no big joiner mode 
>>>>>>> found?
>>>>>>
>>>>>> Yes, this is my query here. Why are we returning highest clock 
>>>>>> mode if no big joiner mode found?
>>>>>>
>>>>> We can return either with highest clock or highest resolution or 
>>>>> default mode,
>>>>> Since we have the other two already covered used highest clock.
>>>>
>>>> But why we need to return something if bigjoiner mode not found?
>>>> If its not found, then we won't get to know only bigjoiner mode not 
>>>> found and we are falling back to default mode.
>>>> @bhanu what you suggest here?
>>>>
>>> Consider a setup with two display's with one having mode with big 
>>> joiner support and other doesn't have any mode
>>> for enabling big joiner.
>>>
>>> In such case we expect the tests to run.
>>> We can discuss on what mode to use on the display (highest_clock, 
>>> highest_res, default) which doesn't support big joiner, but we need 
>>> to have a mode there is
>>> what i understand.
>>
>> IMHO, We should fall back to default mode if there is no bigjoiner 
>> supported mode available.
>>
>> As per my understanding, Pick the mode that exercises the max 
>> bandwidth if there is no bigjoiner mode available is the expectation 
>> from Kunal.
>>
>> - Bhanu
> Will select default_mode in case of no big joiner mode found.
> Will have separate parameter for the bandwidth.

'Highest' and 'lowest' make sense for any driver, but IMHO bigjoiner is 
Intel-specific terminology, do we really need to have an environment 
variable for that.

For most purpose, highest itself should be sufficient, if the test-setup 
has a panel that needs bigjoiner.


Regards,

Ankit


>
>>
>>
>>>>>>>>>            */
>>>>>>>>> -        if (!strcmp(env, "highest") || !strcmp(env, "1"))
>>>>>>>>> +        if (!strcmp(env, "joiner") || !strcmp(env, "2"))
>>>>>>>>> +            found = bigjoiner_mode_found(drm_fd, connector,
>>>>>>>>> + sort_drm_modes_by_clk_dsc,mode,
>>>>>>>>> +                             max_dotclock) ||
>>>>>>>>> +                bigjoiner_mode_found(drm_fd, connector,
>>>>>>>>> + sort_drm_modes_by_res_dsc, mode,
>>>>>>>>> +                             max_dotclock) ||
>>>>>>>>> +                bigjoiner_mode_found(drm_fd, connector,
>>>>>>>>> + sort_drm_modes_by_clk_dsc, mode,
>>>>>>>>> +                             max_dotclock);
>>>>>>>>> +        else if (!strcmp(env, "highest") || !strcmp(env, "1"))
>>>>>>>>>               igt_sort_connector_modes(connector, 
>>>>>>>>> sort_drm_modes_by_res_dsc);
>>>>>>>>>           else if (!strcmp(env, "lowest") || !strcmp(env, "0"))
>>>>>>>>>               igt_sort_connector_modes(connector, 
>>>>>>>>> sort_drm_modes_by_res_asc);
>>>>>>>>>           else
>>>>>>>>>               goto default_mode;
>>>>>>>>> -
>>>>>>>>> -        *mode = connector->modes[0];
>>>>>>>>> +        if (!found)
>>>>>>>>> +            *mode = connector->modes[0];
>>>>>>>>>           return true;
>>>>>>>>>       }
>>> Thanks and Regards
>>> Kunal Joshi