[PATCH v2 09/12] drm/i915/display: Factor out intel_display_{min, max}_pipe_bpp()

[Nautiyal, Ankit K]

On 5/7/2025 4:28 PM, Imre Deak wrote:
> On Wed, May 07, 2025 at 09:29:21AM +0530, Nautiyal, Ankit K wrote:
>> On 5/6/2025 8:46 PM, Imre Deak wrote:
>>> On Tue, May 06, 2025 at 06:37:50PM +0530, Nautiyal, Ankit K wrote:
>>>> On 4/28/2025 7:01 PM, Imre Deak wrote:
>>>>> Factor out helpers that can be used in a follow-up change to query the
>>>>> minimum and maximum pipe bpp supported by the HW.
>>>>>
>>>>> Signed-off-by: Imre Deak <imre.deak at intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/display/intel_display.c | 28 +++++++++++++-------
>>>>>     drivers/gpu/drm/i915/display/intel_display.h |  3 +++
>>>>>     drivers/gpu/drm/i915/display/intel_dp.c      |  2 +-
>>>>>     3 files changed, 22 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> index b4ddffe53e23f..cf2c11826ffb3 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>> @@ -4301,6 +4301,22 @@ compute_sink_pipe_bpp(const struct drm_connector_state *conn_state,
>>>>>     	return 0;
>>>>>     }
>>>>> +int intel_display_min_pipe_bpp(void)
>>>>> +{
>>>>> +	return 6 * 3;
>>>> Hmm.. I think this is DP specific, for HDMI min is 8 * 3 = 24 bpp.
>>>>
>>>> I see this is function is used while checking for min bpp while adding
>>>> support for forcing a bpp for different connectors.
>>>>
>>>> Would it make sense to make this connector specific?
>>> I wonder if it could be kept simple for now and use the platform's min
>>> bpp value in the above debugfs entry. IIUC the
>>> platform/connector/output_type specific min pipe bpps are:
>>>
>>> DP/RGB:      18
>>> DP/YCBCR:    24
>>> LVDS:        18
>>> DSI/DDI:     24
>>> DSI/non-DDI: 18
>>> All other:   24
>>>
>>> It would make sense to add a helper and use it everywhere, but it would
>>> be a bigger change. Are you ok to do this as a follow up?
>> Yes we can have a follow up for this, to get this connector specific.
>>
>> Should we make the min as 24 for now, that will work for most things.
>>
>> Or else we can let that be 18 for now, and the tests when forcing a
>> particular bpp, will then use modeset with try commit.
>>
>> If commit fails with -EINVAL, tests will try again with higher bpp.
> Yes, I think it can be left on the platform minimum 18 (for now). The
> valid range of the debugfs entry is anyway the platform's valid range
> not the connected sink's valid range, so a commit can anyway fail if a
> value outside of the sink's range is set. This is so that you can set
> the debugfs entry _before_ connecting a sink.

Hmm alright, lets have platform min 18 for now.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>


>
>> Regards,
>>
>> Ankit
>>
>>
>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +int intel_display_max_pipe_bpp(struct intel_display *display)
>>>>> +{
>>>>> +	if (display->platform.g4x || display->platform.valleyview ||
>>>>> +	    display->platform.cherryview)
>>>>> +		return 10*3;
>>>>> +	else if (DISPLAY_VER(display) >= 5)
>>>>> +		return 12*3;
>>>>> +	else
>>>>> +		return 8*3;
>>>>> +}
>>>>> +
>>>>>     static int
>>>>>     compute_baseline_pipe_bpp(struct intel_atomic_state *state,
>>>>>     			  struct intel_crtc *crtc)
>>>>> @@ -4310,17 +4326,9 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state,
>>>>>     		intel_atomic_get_new_crtc_state(state, crtc);
>>>>>     	struct drm_connector *connector;
>>>>>     	struct drm_connector_state *connector_state;
>>>>> -	int bpp, i;
>>>>> +	int i;
>>>>> -	if (display->platform.g4x || display->platform.valleyview ||
>>>>> -	    display->platform.cherryview)
>>>>> -		bpp = 10*3;
>>>>> -	else if (DISPLAY_VER(display) >= 5)
>>>>> -		bpp = 12*3;
>>>>> -	else
>>>>> -		bpp = 8*3;
>>>>> -
>>>>> -	crtc_state->pipe_bpp = bpp;
>>>>> +	crtc_state->pipe_bpp = intel_display_max_pipe_bpp(display);
>>>>>     	/* Clamp display bpp to connector max bpp */
>>>>>     	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>>>>> index 3b54a62c290af..b6610e9175a7a 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>>>>> @@ -524,6 +524,9 @@ void intel_plane_fixup_bitmasks(struct intel_crtc_state *crtc_state);
>>>>>     bool intel_crtc_vrr_disabling(struct intel_atomic_state *state,
>>>>>     			      struct intel_crtc *crtc);
>>>>> +int intel_display_min_pipe_bpp(void);
>>>>> +int intel_display_max_pipe_bpp(struct intel_display *display);
>>>>> +
>>>>>     /* modesetting */
>>>>>     int intel_modeset_pipes_in_mask_early(struct intel_atomic_state *state,
>>>>>     				      const char *reason, u8 pipe_mask);
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> index 0f89a301e4a0d..73ca9f8efefc5 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> @@ -1198,7 +1198,7 @@ intel_dp_output_format(struct intel_connector *connector,
>>>>>     int intel_dp_min_bpp(enum intel_output_format output_format)
>>>>>     {
>>>>>     	if (output_format == INTEL_OUTPUT_FORMAT_RGB)
>>>>> -		return 6 * 3;
>>>>> +		return intel_display_min_pipe_bpp();
>>>>>     	else
>>>>>     		return 8 * 3;
>>>>>     }