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

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

Hello Ankit,

On 1/18/2024 4:08 PM, Nautiyal, Ankit K wrote:
>
> 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"))
How about
if (is_intel_device(drm_fd) && !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
Thanks and Regards
Kunal Joshi