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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

We shouldn't have code acting differently whether modesets are allowed,
I think I'm missing some context here?

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;
>  	}