Re: [PATCH i-g-t] lib/igt_kms: Use get_max_pipe_hdisplay in joiner_possible fucntions

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

Hi Jeevan,

On 3/19/2025 11:29 PM, B, Jeevan wrote:
>> -----Original Message-----
>> From: B S, Karthik <karthik.b.s@intel.com>
>> Sent: Wednesday, March 19, 2025 11:43 AM
>> To: igt-dev@lists.freedesktop.org
>> Cc: Reddy Guddati, Santhosh <santhosh.reddy.guddati@intel.com>; B, Jeevan
>> <jeevan.b@intel.com>; B S, Karthik <karthik.b.s@intel.com>
>> Subject: [PATCH i-g-t] lib/igt_kms: Use get_max_pipe_hdisplay in
>> joiner_possible fucntions
>>
>> Update 'joiner_possible' helper functions to use the existing
>> get_max_pipe_hdisplay helper.
>>
>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>> ---
>>   lib/igt_kms.c | 45 ++++++++++++++++++++-------------------------
>>   lib/igt_kms.h |  2 +-
>>   2 files changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index cc3bb3ae7..99c8707c7 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -6428,6 +6428,20 @@ int igt_get_current_cdclk(int fd)
>>   	return read_and_parse_cdclk_debugfs(fd, "Current CD clock
>> frequency:");  }
>>
>> +/**
>> + * get_max_hdisplay:
>> + * @drm_fd: drm file descriptor
>> + *
>> + * Returns: The maximum hdisplay supported per pipe.
>> + */
>> +static int get_max_pipe_hdisplay(int drm_fd) {
>> +	int dev_id = intel_get_drm_devid(drm_fd);
>> +
>> +	return (intel_display_ver(dev_id) >= 30) ? HDISPLAY_6K_PER_PIPE :
>> +						   HDISPLAY_5K_PER_PIPE;
>> +}
>> +
>>   /**
>>    * igt_bigjoiner_possible:
>>    * @drm_fd: drm file descriptor
>> @@ -6441,13 +6455,8 @@ int igt_get_current_cdclk(int fd)
>>    */
>>   bool igt_bigjoiner_possible(int drm_fd, drmModeModeInfo *mode, int
>> max_dotclock)  {
>> -	int max_hdisplay, dev_id;
>> -
>> -	dev_id = intel_get_drm_devid(drm_fd);
>> -	max_hdisplay = (intel_display_ver(dev_id) >= 30) ?
>> HDISPLAY_6K_PER_PIPE :
>> -			HDISPLAY_5K_PER_PIPE;
>> -
>> -	return (mode->hdisplay > max_hdisplay || mode->clock >
>> max_dotclock);
>> +	return (mode->hdisplay > get_max_pipe_hdisplay(drm_fd) ||
>> +		mode->clock > max_dotclock);
>>   }
>>
>>   /**
>> @@ -6469,7 +6478,7 @@ bool bigjoiner_mode_found(int drm_fd,
>> drmModeConnector *connector,
>>
>>   	for (int i=0; i< connector->count_modes; i++) {
>>   		if (igt_bigjoiner_possible(drm_fd, &connector->modes[i],
>> max_dotclock) &&
>> -		    !igt_ultrajoiner_possible(&connector->modes[i],
>> max_dotclock)) {
>> +		    !igt_ultrajoiner_possible(drm_fd, &connector->modes[i],
>> +max_dotclock)) {
>>   			*mode = connector->modes[i];
>>   			found = true;
>>   			break;
>> @@ -6478,20 +6487,6 @@ bool bigjoiner_mode_found(int drm_fd,
>> drmModeConnector *connector,
>>   	return found;
>>   }
>>
>> -/**
>> - * get_max_hdisplay:
>> - * @drm_fd: drm file descriptor
>> - *
>> - * Returns: The maximum hdisplay supported per pipe.
>> - */
>> -static int get_max_pipe_hdisplay(int drm_fd) -{
>> -	int dev_id = intel_get_drm_devid(drm_fd);
>> -
>> -	return (intel_display_ver(dev_id) >= 30) ? HDISPLAY_6K_PER_PIPE :
>> -						   HDISPLAY_5K_PER_PIPE;
>> -}
>> -
>>   /**
>>    * max_non_joiner_mode_found:
>>    * @drm_fd: drm file descriptor
>> @@ -6567,9 +6562,9 @@ bool igt_is_joiner_enabled_for_pipe(int drmfd,
>> enum pipe pipe)
>>    *
>>    * Returns: True if mode requires Ultrajoiner, else False.
>>    */
>> -bool igt_ultrajoiner_possible(drmModeModeInfo *mode, int max_dotclock)
>> +bool igt_ultrajoiner_possible(int drm_fd, drmModeModeInfo *mode, int
>> +max_dotclock)
>>   {
>> -	return (mode->hdisplay > 2 * HDISPLAY_5K_PER_PIPE ||
>> +	return (mode->hdisplay > 2 * get_max_pipe_hdisplay(drm_fd) ||
>>   		mode->clock > 2 * max_dotclock);
>>   }
>>
> For ultrajoiner hdisplay logic feels off, we need to correct this.
> Need to check spec and update. Correct me if I am wrong
>
> Apart from these code LGTM.

Thank you for the review.

The logic for ultrajoiner is similar to what we've in bigjoiner and also 
we've the same logic in kernel as well.

https://gitlab.freedesktop.org/drm/tip/-/blob/drm-tip/drivers/gpu/drm/i915/display/intel_dp.c?ref_type=heads#L1333

Thanks,
Karthik.B.S
>> @@ -6591,7 +6586,7 @@ bool ultrajoiner_mode_found(int drm_fd,
>> drmModeConnector *connector,
>>   	bool found = false;
>>
>>   	for (int i = 0; i < connector->count_modes; i++) {
>> -		if (igt_ultrajoiner_possible(&connector->modes[i],
>> max_dotclock)) {
>> +		if (igt_ultrajoiner_possible(drm_fd, &connector->modes[i],
>> +max_dotclock)) {
>>   			*mode = connector->modes[i];
>>   			found = true;
>>   			break;
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 27b545f52..0381c82ad
>> 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -1252,7 +1252,7 @@ bool bigjoiner_mode_found(int drm_fd,
>> drmModeConnector *connector,  bool max_non_joiner_mode_found(int
>> drm_fd, drmModeConnector *connector,
>>   			       int max_dotclock, drmModeModeInfo *mode);
>> bool igt_is_joiner_enabled_for_pipe(int drmfd, enum pipe pipe); -bool
>> igt_ultrajoiner_possible(drmModeModeInfo *mode, int max_dotclock);
>> +bool igt_ultrajoiner_possible(int drmfd, drmModeModeInfo *mode, int
>> +max_dotclock);
>>   bool ultrajoiner_mode_found(int drm_fd, drmModeConnector *connector,
>>   			  int max_dotclock, drmModeModeInfo *mode);  bool
>> igt_has_force_joiner_debugfs(int drmfd, char *conn_name);
>> --
>> 2.43.0