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

["Joshi, Kunal1" <kunal1.joshi@intel.com>]

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.

>
>
>>>>>>>>            */
>>>>>>>> -        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