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

[Hogander, Jouni]

On Tue, 2024-08-27 at 13:29 +0300, Jani Nikula wrote:
> On Tue, 27 Aug 2024, Maarten Lankhorst
> <maarten.lankhorst at 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?

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