Re: [PATCH i-g-t v4 5/5] lib/igt_kms: Add function to get valid pipe for specific output

[Karthik B S <karthik.b.s@intel.com>]

On 1/27/2025 8:14 PM, Louis Chauvet wrote:
>
>
> Le 24/01/2025 à 05:26, Karthik B S a écrit :
>
>>> +/**
>>> + * igt_get_pipe_for_output:
>>> + * @display: display to fetch the pipes
>>> + * @output: output to use
>>> + *
>>> + * Get a valid pipe for a specific output. The return value is the 
>>> pipe first valid pipe for a
>>> + * specific output.
>>> + */
>>> +enum pipe igt_get_pipe_for_output(igt_display_t *display,
>>> +                  igt_output_t *output)
>>> +{
>>> +    enum pipe pipe;
>>> +
>>> +    for_each_pipe(display, pipe) {
>>> +        if (igt_output_is_connected(output) &&
>>> +             (output->config.valid_crtc_idx_mask & (1 << (pipe))))
>>
>> Please use 'igt_pipe_connector_valid'.
>
> Sorry, I probably extended it to debug something and forgot to 
> unexpand. It will be fixed for the next iteration.
>
>> Also, this function is mostly a duplicate of the existing
>> 'chamelium_get_pipe_for_output'. Could we reuse the existing function
>> here instead of adding a new one?
>
> I know, but:
> 1 - chamelium_get_pipe_for_output is only enabled with the chamelium 
> build option;
> 2 - chamelium_get_pipe_for_output also enables the pipe during the 
> process.
>
> For 1, I can simply replace all calls to use the new helper.

Hi,

Agreed. I don't see any added value keeping them chamelium specific.

>
> For 2, I need to understand why they choose to call 
> igt_output_set_pipe to get a pipe to see if I can simply replace the 
> chamelium helper by mine or if I need to change my helper to do the 
> same thing.

'igt_output_set_pipe' is used for the 'igt_check_bigjoiner_support' 
nested within 'intel_pipe_output_combo_valid' as there are a few 
restrictions in big joiner usage. Although most of them are applicable 
while using multidisplay combinations(which shouldn't apply in this 
case), the restriction on last pipe would still be applicable IMHO, if 
we've an output with a default mode which would required big joiner.

Keeping this in mind, I would actually propose renaming the original 
function and moving it to a common lib to make it generic rather then 
having it chamelium specific. With this we will have a generic function 
to get_pipe_for_output and also have the intel_pipe_output_combo_valid 
check.

Thanks,
Karthik.B.S
>
> Thanks,
> Louis Chauvet
>
>> Thanks,
>> Karthik.B.S
>>> +            return pipe;
>>> +    }
>>> +
>>> +    return PIPE_NONE;
>>> +}
>
>