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