On Tue, 2024-08-27 at 13:29 +0300, Jani Nikula wrote:
> 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?
We already have bpp configured by GOP correctly read. I don't think we
want to stick into that always?
The patch is targeted for case where userspace thinks changing refresh
rate without modeset is possible as panel supports VRR. Our
compute_config ends up to full modeset due to differing bpp and
preventing changing refresh rate unless full mode set is allowed.
BR,
Jouni Högander
>
> 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;
> > > }
> >
>
