Re: [PATCH] drm/i915/psr: Request modeset on initial commit to compute PSR state

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

On Tue, Jan 23, 2024 at 08:17:36AM +0000, Hogander, Jouni wrote:
> On Tue, 2024-01-23 at 10:07 +0200, Ville Syrjälä wrote:
> > On Tue, Jan 23, 2024 at 07:57:00AM +0000, Hogander, Jouni wrote:
> > > On Tue, 2024-01-23 at 09:41 +0200, Ville Syrjälä wrote:
> > > > On Tue, Jan 23, 2024 at 09:11:03AM +0200, Jouni Högander wrote:
> > > > > We want to request full modeset in initial fast check to force
> > > > > PSR
> > > > > state
> > > > > computation. Otherwise PSR is not enabled on initial commit but
> > > > > on
> > > > > first
> > > > > commit with modeset or fastset. With this change Initial commit
> > > > > will still
> > > > > end up using fastset (unless something else requires full
> > > > > modeset)
> > > > > as PSR
> > > > > parameters are not anymore part of intel_pipe_config_compare.
> > > > 
> > > > I think I'd prefer to go the oppostie direction and try to get
> > > > all
> > > > the full modeset stuff out from the initial commit. The only
> > > > reason
> > > > the initial commit was introduced was to compute the plane states
> > > > due to lack of readout, and then it got extended due to various
> > > > other
> > > > hacks. Our goal is to inherit the state from the BIOS so ideally
> > > > the whole initial_commit thing wouldn't even exist.
> > > 
> > > Bios doesn't enable PSR. Do you think this would be better approach
> > > ?:
> > > 
> > > https://patchwork.freedesktop.org/patch/575368/?series=129023&rev=1
> > > 
> > > What we just need is something triggering intel_psr_compute_config
> > > +
> > > psr enable. Maybe that could be separate function doing both and
> > > call
> > > that from intel_initial_commit. If/when we get rid of that
> > > intel_initial_commit: this function would be called by that new
> > > thing.
> > 
> > I don't think we should do anything at all. PSR will get enabled by
> > the
> > first proper commit, if possible.
> 
> That means PSR is disabled until there is fastset or full modeset. Is
> that ok? I.e. is there some usecase where either of these doesn't
> happen?

Shouldn't happen, unless there is no userspace/fbcon client at all.
But in that case we should just turn off the whole display and let
the device enter runtime suspend. I don't think we are doing that
atm. It should perhaps be done from eg. a work scheduled fairly
far into the future to give userspace/fbcon enough time to
initialize.

> 
> Panel replay is also now coming to picture as it requires sink side
> being enabled before link training. Maybe you have some advice on these
> as well:
> 
> https://patchwork.freedesktop.org/patch/574966/?series=128156&rev=5
> https://patchwork.freedesktop.org/patch/574979/?series=128156&rev=5

I'll have to think a bit about all of it. In general I think the
sink PSR enable/disable should be moved to the full modeset/fastset
sequence properly, same for most of the source side PSR setup. The 
only thing we should be frobbing during any other kind of commit/etc.
is the control register enable bit (in case we need to actually toggle
PSR, as opposed to just forcing a temporary exit with the CURSURFLIVE
trick).

-- 
Ville Syrjälä
Intel