Re: [Intel-gfx] [PATCH 1/3] drm: Add infrastructure to allow seamless mode switches

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 26, 2022 at 06:32:01PM +0000, Souza, Jose wrote:
> On Tue, 2022-04-26 at 21:08 +0300, Ville Syrjälä wrote:
> > On Thu, Apr 21, 2022 at 12:22:03PM -0700, José Roberto de Souza wrote:
> > > Intel hardware supports change between modes with different refresh
> > > rates without any glitches or visual artifacts, that feature is called
> > > seamless DRRS.
> > > 
> > > So far i915 driver was automatically changing between preferred panel
> > > mode and lower refresh rate mode based on idleness but ChromeOS
> > > compositor team is requesting to be in control of the mode switch.
> > > So for a certain types of content it can switch to mode with a lower
> > > refresh rate without user noticing a thing and saving power.
> > > 
> > > This seamless mode switch will be triggered when user-space dispatch
> > > a atomic commit with the new mode and clears the
> > > DRM_MODE_ATOMIC_ALLOW_MODESET flag.
> > > 
> > > A driver that don't implement atomic_seamless_mode_switch_check
> > > function will continue to fail in the atomic check phase with
> > > "[CRTC:%d:%s] requires full modeset" debug message.
> > > While a driver that implements it and support the seamless change
> > > between old and new mode will return 0 otherwise it should return the
> > > appropried errno.
> > > 
> > > So here adding basic drm infrastructure to that be supported by i915
> > > and other drivers.
> > 
> > I don't see the need for any extra infrastructure for this.
> > 
> > I think we just need:
> > - fix the fastset code to not suck
> 
> How would it know that only mode changed and not all other things that causes mode_changed to be set?
> For example: intel_digital_connector_atomic_check()

That's what the fastset stuff does. It checks if anything changes
that needs a full modeset or not.

> 
> > - reprogram M/N during fastset
> > - calculate eDP link params using panel's highest refresh rate mode
> >   to make sure we get the same link params for all refresh rates
> > 
> > > 
> > > Cc: Vidya Srinivas <vidya.srinivas@intel.com>
> > > Cc: Sean Paul <seanpaul@chromium.org>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c              |  1 +
> > >  drivers/gpu/drm/drm_atomic_helper.c       | 16 +++++++++++++++
> > >  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> > >  include/drm/drm_crtc.h                    | 25 +++++++++++++++++++++++
> > >  4 files changed, 43 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 58c0283fb6b0c..21525f9f4b4c1 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -437,6 +437,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > >  	drm_printf(p, "\tself_refresh_active=%d\n", state->self_refresh_active);
> > >  	drm_printf(p, "\tplanes_changed=%d\n", state->planes_changed);
> > >  	drm_printf(p, "\tmode_changed=%d\n", state->mode_changed);
> > > +	drm_printf(p, "\tseamless_mode_changed=%d\n", state->seamless_mode_changed);
> > >  	drm_printf(p, "\tactive_changed=%d\n", state->active_changed);
> > >  	drm_printf(p, "\tconnectors_changed=%d\n", state->connectors_changed);
> > >  	drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 9603193d2fa13..e6f3a966f7b86 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -631,6 +631,22 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > >  			drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> > >  				       crtc->base.id, crtc->name);
> > >  			new_crtc_state->mode_changed = true;
> > > +
> > > +			if (!state->allow_modeset &&
> > > +			    crtc->funcs->atomic_seamless_mode_switch_check) {
> > > +				ret = crtc->funcs->atomic_seamless_mode_switch_check(state, crtc);
> > > +				if (ret == -EOPNOTSUPP) {
> > > +					drm_dbg_atomic(dev, "[CRTC:%d:%s] Seamless mode switch not supported\n",
> > > +						       crtc->base.id, crtc->name);
> > > +					return ret;
> > > +				}
> > > +
> > > +				if (ret < 0)
> > > +					return ret;
> > > +
> > > +				new_crtc_state->seamless_mode_changed = true;
> > > +				new_crtc_state->mode_changed = false;
> > > +			}
> > >  		}
> > >  
> > >  		if (old_crtc_state->enable != new_crtc_state->enable) {
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 3b6d3bdbd0996..c093073ea6e11 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -142,6 +142,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > >  	if (state->gamma_lut)
> > >  		drm_property_blob_get(state->gamma_lut);
> > >  	state->mode_changed = false;
> > > +	state->seamless_mode_changed = false;
> > >  	state->active_changed = false;
> > >  	state->planes_changed = false;
> > >  	state->connectors_changed = false;
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index a70baea0636ca..b7ce378d679d3 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -140,6 +140,16 @@ struct drm_crtc_state {
> > >  	 */
> > >  	bool mode_changed : 1;
> > >  
> > > +	/**
> > > +	 * @seamless_mode_changed: @mode has been changed but user-space
> > > +	 * is requesting to change to the new mode with a fastset and driver
> > > +	 * supports this request.
> > > +	 * To be used by drivers to steer the atomic commit control flow to
> > > +	 * appropriate paths to change mode without any visual corruption.
> > > +	 * Never set together with @mode_changed.
> > > +	 */
> > > +	bool seamless_mode_changed : 1;
> > > +
> > >  	/**
> > >  	 * @active_changed: @active has been toggled. Used by the atomic
> > >  	 * helpers and drivers to steer the atomic commit control flow. See also
> > > @@ -939,6 +949,21 @@ struct drm_crtc_funcs {
> > >  				     int *max_error,
> > >  				     ktime_t *vblank_time,
> > >  				     bool in_vblank_irq);
> > > +
> > > +	/**
> > > +	 * @atomic_seamless_mode_switch_check
> > > +	 *
> > > +	 * Called when user-space wants to change mode without do a modeset.
> > > +	 * Drivers can optionally support do a mode switch without any visual
> > > +	 * corruption when changing between certain modes.
> > > +	 *
> > > +	 * Returns:
> > > +	 * Zero if possible to seamless switch mode, -EOPNOTSUPP if not
> > > +	 * supported seamless mode change or appropriate errno if an error
> > > +	 * happened.
> > > +	 */
> > > +	int (*atomic_seamless_mode_switch_check)(struct drm_atomic_state *state,
> > > +						 struct drm_crtc *crtc);
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.36.0
> > 
> 

-- 
Ville Syrjälä
Intel