Re: [PATCH i-g-t 1/4] lib/igt_kms: move bigjoiner_mode_found to lib

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

On 3/18/2024 11:08 AM, Joshi, Kunal1 wrote:
> Hello Ankit,
>
> On 3/15/2024 9:40 AM, Nautiyal, Ankit K wrote:
>>
>> On 3/10/2024 7:57 PM, Kunal Joshi wrote:
>>> move bigjoiner_mode_found to lib
>>>
>>> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> 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                | 25 +++++++++++++++++++++++++
>>>   lib/igt_kms.h                |  2 ++
>>>   tests/intel/kms_big_joiner.c | 14 +-------------
>>>   3 files changed, 28 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index e18f6fe59..63c8045c7 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -6143,6 +6143,31 @@ bool igt_bigjoiner_possible(drmModeModeInfo 
>>> *mode, int max_dotclock)
>>>           mode->clock > max_dotclock);
>>>   }
>>>   +/**
>>> + * bigjoiner_mode_found:
>>> + * @connector: libdrm connector
>>> + * @sort_method: comparator method
>>> + * @mode: libdrm mode
>>> + *
>>
>> This is incorrect.
>>
>>
>>> + * Bigjoiner will come in to the picture when the
>>> + * resolution > 5K or clock > max-dot-clock.
>>> + *
>>> + * Returns: True if big joiner found in connector modes
>>> + */
>>> +bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>>> +              int max_dotclock)
>>> +{
>>> +    bool found = false;
>>> +
>>> +    igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc);
>>> +    found = igt_bigjoiner_possible(&connector->modes[0], 
>>> max_dotclock);
>>> +    if (!found) {
>>> +        igt_sort_connector_modes(connector, 
>>> sort_drm_modes_by_clk_dsc);
>>> +        found = igt_bigjoiner_possible(&connector->modes[0], 
>>> max_dotclock);
>>> +    }
>>> +    return found;
>>> +}
>>> +
>>>   /**
>>>    * igt_check_bigjoiner_support:
>>>    * @display: a pointer to an #igt_display_t structure
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index b3882808b..bab8487d3 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -1212,6 +1212,8 @@ bool igt_max_bpc_constraint(igt_display_t 
>>> *display, enum pipe pipe,
>>>           igt_output_t *output, int bpc);
>>>   int igt_get_max_dotclock(int fd);
>>>   bool igt_bigjoiner_possible(drmModeModeInfo *mode, int max_dotclock);
>>> +bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>>> +              int max_dotclock);
>>>   bool igt_check_bigjoiner_support(igt_display_t *display);
>>>   bool igt_parse_mode_string(const char *mode_string, 
>>> drmModeModeInfo *mode);
>>>   bool intel_pipe_output_combo_valid(igt_display_t *display);
>>> diff --git a/tests/intel/kms_big_joiner.c 
>>> b/tests/intel/kms_big_joiner.c
>>> index aba2adfbe..28678b958 100644
>>> --- a/tests/intel/kms_big_joiner.c
>>> +++ b/tests/intel/kms_big_joiner.c
>>> @@ -199,16 +199,6 @@ static void test_dual_display(data_t *data)
>>>       igt_display_commit2(display, COMMIT_ATOMIC);
>>>   }
>>>   -static bool bigjoiner_mode_found(drmModeConnector *connector,
>>> -                 int (*sort_method)(const void *, const void*),
>>> -                 drmModeModeInfo *mode)
>>> -{
>>> -    igt_sort_connector_modes(connector, sort_method);
>>> -    *mode = connector->modes[0];
>>> -
>>> -    return igt_bigjoiner_possible(mode, max_dotclock);
>>> -}
>>> -
>>>   igt_main
>>>   {
>>>       data_t data;
>>> @@ -235,9 +225,7 @@ igt_main
>>>                * Bigjoiner will come in to the picture when the
>>>                * resolution > 5K or clock > max-dot-clock.
>>>                */
>>> -            found = (bigjoiner_mode_found(connector, 
>>> sort_drm_modes_by_res_dsc, &mode) ||
>>> -                 bigjoiner_mode_found(connector, 
>>> sort_drm_modes_by_clk_dsc, &mode)) ?
>>> -                    true : false;
>>> +            found = bigjoiner_mode_found(data.drm_fd, connector, 
>>> max_dotclock);
>>
>>
>> How does this work? Earlier mode was filled by the 
>> bigjoiner_mode_found().
> Currently it works as we will have the connector modes sorted.
> But as you state for better readability will have a mode parameter.

No, I meant that with only this patch we have a variable mode which we 
are not filling now.

Later we are doing  `memcpy(&data.output[count].mode, &mode, 
sizeof(drmModeModeInfo));`  inside the if (found) block.

I think we need to remove the drmModeModeInfo mode and memcpy line.

Regards,

Ankit


>>
>> Now mode is not getting filled at all.
>>
>> Regards,
>>
>> Ankit
>>
>>>                 if (found) {
>>>                   data.output[count].output_id = output->id;
> Thanks and Regards
> Kunal Joshi