On Tue, 27 Aug 2024, Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> 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@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;
>> }
>
--
Jani Nikula, Intel
