Re: [PATCH] drm/i915/display: use old bpp as a base when modeset is not allowed

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 27 Aug 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Hey,
>
> We shouldn't have code acting differently whether modesets are allowed,
> I think I'm missing some context here?

Yeah. Since GOP is mentioned, is this really about state readout
instead?

BR,
Jani.


>
> Cheers,
> ~Marten
>
> Den 2024-08-26 kl. 12:41, skrev Jouni Högander:
>> We are currently observing failure on refresh rate change on VRR setup if
>> full modeset is not allowed. This is caused by the mismatch in bpp
>> configured by GOP and bpp value calculated by our driver. Changing bpp to
>> value calculated by our driver would require full mode set.
>> 
>> We don't have mechanism to communicate current bpp to userspace ->
>> Userspace can't request to use current bpp. Changing bpp means full
>> modeset. This becomes a problem when userspace haven't allowed full mode
>> set.
>> 
>> Complete solution here would mean adding mechanism to communicate current
>> bpp to userspace. User space should use this bpp to avoid changing bpp if
>> it wants to avoid full mode set.
>> 
>> Tackle this for now in our driver by using existing bpp if full modeset is
>> not allowed.
>> 
>> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++------
>>  1 file changed, 23 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 9049b9a1209d8..7b805998b280a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -4385,21 +4385,34 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state,
>>  			  struct intel_crtc *crtc)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> -	struct intel_crtc_state *crtc_state =
>> +	struct intel_crtc_state *new_crtc_state =
>>  		intel_atomic_get_new_crtc_state(state, crtc);
>> +	struct intel_crtc_state *old_crtc_state =
>> +		intel_atomic_get_old_crtc_state(state, crtc);
>>  	struct drm_connector *connector;
>>  	struct drm_connector_state *connector_state;
>>  	int bpp, i;
>>  
>> -	if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
>> -	    IS_CHERRYVIEW(dev_priv)))
>> -		bpp = 10*3;
>> -	else if (DISPLAY_VER(dev_priv) >= 5)
>> -		bpp = 12*3;
>> -	else
>> -		bpp = 8*3;
>> +	/*
>> +	 * TODO: We don't have mechanism to communicate current bpp to
>> +	 * userspace -> Userspace can't request to use current bpp. Changing bpp
>> +	 * means full modeset. This becomes a problem when userspace wants to
>> +	 * avoid full modeset. Tackle this on our driver by using existing bpp
>> +	 * if full modeset is not allowed.
>> +	 */
>> +	if (!state->base.allow_modeset) {
>> +		bpp = old_crtc_state->pipe_bpp;
>> +	} else {
>> +		if ((IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
>> +		     IS_CHERRYVIEW(dev_priv)))
>> +			bpp = 10 * 3;
>> +		else if (DISPLAY_VER(dev_priv) >= 5)
>> +			bpp = 12 * 3;
>> +		else
>> +			bpp = 8 * 3;
>> +	}
>>  
>> -	crtc_state->pipe_bpp = bpp;
>> +	new_crtc_state->pipe_bpp = bpp;
>>  
>>  	/* Clamp display bpp to connector max bpp */
>>  	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
>> @@ -4408,7 +4421,7 @@ compute_baseline_pipe_bpp(struct intel_atomic_state *state,
>>  		if (connector_state->crtc != &crtc->base)
>>  			continue;
>>  
>> -		ret = compute_sink_pipe_bpp(connector_state, crtc_state);
>> +		ret = compute_sink_pipe_bpp(connector_state, new_crtc_state);
>>  		if (ret)
>>  			return ret;
>>  	}
>

-- 
Jani Nikula, Intel