diff mbox series

drm/i915/psr: Request modeset on initial commit to compute PSR state

Message ID 20240123071103.2147760-1-jouni.hogander@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/psr: Request modeset on initial commit to compute PSR state | expand

Commit Message

Hogander, Jouni Jan. 23, 2024, 7:11 a.m. UTC
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.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Fixes: a480dd59fe25 ("drm/i915/display: No need for full modeset due to psr")
Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
 drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
 drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
 3 files changed, 11 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä Jan. 23, 2024, 7:41 a.m. UTC | #1
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.

> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> Fixes: a480dd59fe25 ("drm/i915/display: No need for full modeset due to psr")
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
>  drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
>  drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index ab415f41924d..143981b91e8b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3326,6 +3326,14 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
>  		fastset = false;
>  	}
>  
> +	if (CAN_PSR(intel_dp)) {
> +		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute PSR state\
> +n",
> +			    encoder->base.base.id, encoder->base.name);
> +		crtc_state->uapi.mode_changed = true;
> +		fastset = false;
> +	}
> +
>  	return fastset;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 1010b8c405df..b6db7dbfaf1a 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -173,9 +173,6 @@
>   * irrelevant for normal operation.
>   */
>  
> -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> -			   (intel_dp)->psr.source_support)
> -
>  #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)->psr.sink_panel_replay_support && \
>  				    (intel_dp)->psr.source_panel_replay_support)
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index cde781df84d5..3d9920ebafab 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -21,6 +21,9 @@ struct intel_encoder;
>  struct intel_plane;
>  struct intel_plane_state;
>  
> +#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> +			   (intel_dp)->psr.source_support)
> +
>  bool intel_encoder_can_psr(struct intel_encoder *encoder);
>  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
>  void intel_psr_pre_plane_update(struct intel_atomic_state *state,
> -- 
> 2.34.1
Hogander, Jouni Jan. 23, 2024, 7:57 a.m. UTC | #2
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.

BR,

Jouni Högander

> 
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > Fixes: a480dd59fe25 ("drm/i915/display: No need for full modeset
> > due to psr")
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
> >  drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
> >  drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index ab415f41924d..143981b91e8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3326,6 +3326,14 @@ bool intel_dp_initial_fastset_check(struct
> > intel_encoder *encoder,
> >                 fastset = false;
> >         }
> >  
> > +       if (CAN_PSR(intel_dp)) {
> > +               drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing
> > full modeset to compute PSR state\
> > +n",
> > +                           encoder->base.base.id, encoder-
> > >base.name);
> > +               crtc_state->uapi.mode_changed = true;
> > +               fastset = false;
> > +       }
> > +
> >         return fastset;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 1010b8c405df..b6db7dbfaf1a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -173,9 +173,6 @@
> >   * irrelevant for normal operation.
> >   */
> >  
> > -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > -                          (intel_dp)->psr.source_support)
> > -
> >  #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)-
> > >psr.sink_panel_replay_support && \
> >                                     (intel_dp)-
> > >psr.source_panel_replay_support)
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index cde781df84d5..3d9920ebafab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -21,6 +21,9 @@ struct intel_encoder;
> >  struct intel_plane;
> >  struct intel_plane_state;
> >  
> > +#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > +                          (intel_dp)->psr.source_support)
> > +
> >  bool intel_encoder_can_psr(struct intel_encoder *encoder);
> >  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> >  void intel_psr_pre_plane_update(struct intel_atomic_state *state,
> > -- 
> > 2.34.1
>
Ville Syrjälä Jan. 23, 2024, 8:07 a.m. UTC | #3
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.

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > 
> > > Fixes: a480dd59fe25 ("drm/i915/display: No need for full modeset
> > > due to psr")
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
> > >  drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
> > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index ab415f41924d..143981b91e8b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -3326,6 +3326,14 @@ bool intel_dp_initial_fastset_check(struct
> > > intel_encoder *encoder,
> > >                 fastset = false;
> > >         }
> > >  
> > > +       if (CAN_PSR(intel_dp)) {
> > > +               drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing
> > > full modeset to compute PSR state\
> > > +n",
> > > +                           encoder->base.base.id, encoder-
> > > >base.name);
> > > +               crtc_state->uapi.mode_changed = true;
> > > +               fastset = false;
> > > +       }
> > > +
> > >         return fastset;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 1010b8c405df..b6db7dbfaf1a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -173,9 +173,6 @@
> > >   * irrelevant for normal operation.
> > >   */
> > >  
> > > -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > > -                          (intel_dp)->psr.source_support)
> > > -
> > >  #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)-
> > > >psr.sink_panel_replay_support && \
> > >                                     (intel_dp)-
> > > >psr.source_panel_replay_support)
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index cde781df84d5..3d9920ebafab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -21,6 +21,9 @@ struct intel_encoder;
> > >  struct intel_plane;
> > >  struct intel_plane_state;
> > >  
> > > +#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > > +                          (intel_dp)->psr.source_support)
> > > +
> > >  bool intel_encoder_can_psr(struct intel_encoder *encoder);
> > >  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> > >  void intel_psr_pre_plane_update(struct intel_atomic_state *state,
> > > -- 
> > > 2.34.1
> > 
>
Hogander, Jouni Jan. 23, 2024, 8:17 a.m. UTC | #4
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?

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

BR,

Jouni Högander


> 
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > > 
> > > > 
> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > 
> > > > Fixes: a480dd59fe25 ("drm/i915/display: No need for full
> > > > modeset
> > > > due to psr")
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
> > > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index ab415f41924d..143981b91e8b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -3326,6 +3326,14 @@ bool
> > > > intel_dp_initial_fastset_check(struct
> > > > intel_encoder *encoder,
> > > >                 fastset = false;
> > > >         }
> > > >  
> > > > +       if (CAN_PSR(intel_dp)) {
> > > > +               drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s]
> > > > Forcing
> > > > full modeset to compute PSR state\
> > > > +n",
> > > > +                           encoder->base.base.id, encoder-
> > > > > base.name);
> > > > +               crtc_state->uapi.mode_changed = true;
> > > > +               fastset = false;
> > > > +       }
> > > > +
> > > >         return fastset;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 1010b8c405df..b6db7dbfaf1a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -173,9 +173,6 @@
> > > >   * irrelevant for normal operation.
> > > >   */
> > > >  
> > > > -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > > > -                          (intel_dp)->psr.source_support)
> > > > -
> > > >  #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)-
> > > > > psr.sink_panel_replay_support && \
> > > >                                     (intel_dp)-
> > > > > psr.source_panel_replay_support)
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > index cde781df84d5..3d9920ebafab 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > @@ -21,6 +21,9 @@ struct intel_encoder;
> > > >  struct intel_plane;
> > > >  struct intel_plane_state;
> > > >  
> > > > +#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
> > > > +                          (intel_dp)->psr.source_support)
> > > > +
> > > >  bool intel_encoder_can_psr(struct intel_encoder *encoder);
> > > >  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> > > >  void intel_psr_pre_plane_update(struct intel_atomic_state
> > > > *state,
> > > > -- 
> > > > 2.34.1
> > > 
> > 
>
Ville Syrjälä Jan. 23, 2024, 8:38 a.m. UTC | #5
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).
Jani Nikula Jan. 24, 2024, 9:15 a.m. UTC | #6
On Tue, 23 Jan 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> 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.

In general, I'm leaning the same way. Priorities:

1) Avoid full modeset at probe if at all possible.

2) Taking the above into account, enable as many power saving
   etc. features as possible at probe.

3) If a system needs more power savings (or other fancy features
   enabled), a) have them enabled by BIOS/GOP, or b) have the userspace
   do a modeset post-probe according to its policy. Don't force that
   policy on the kernel.

4) *Maybe* provide a way to force full modeset at probe as a policy
   option.


BR,
Jani.




>
>> 
>> BR,
>> 
>> Jouni Högander
>> 
>> > 
>> > > 
>> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> > > 
>> > > Fixes: a480dd59fe25 ("drm/i915/display: No need for full modeset
>> > > due to psr")
>> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/display/intel_dp.c  | 8 ++++++++
>> > >  drivers/gpu/drm/i915/display/intel_psr.c | 3 ---
>> > >  drivers/gpu/drm/i915/display/intel_psr.h | 3 +++
>> > >  3 files changed, 11 insertions(+), 3 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > index ab415f41924d..143981b91e8b 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > @@ -3326,6 +3326,14 @@ bool intel_dp_initial_fastset_check(struct
>> > > intel_encoder *encoder,
>> > >                 fastset = false;
>> > >         }
>> > >  
>> > > +       if (CAN_PSR(intel_dp)) {
>> > > +               drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing
>> > > full modeset to compute PSR state\
>> > > +n",
>> > > +                           encoder->base.base.id, encoder-
>> > > >base.name);
>> > > +               crtc_state->uapi.mode_changed = true;
>> > > +               fastset = false;
>> > > +       }
>> > > +
>> > >         return fastset;
>> > >  }
>> > >  
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > index 1010b8c405df..b6db7dbfaf1a 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > @@ -173,9 +173,6 @@
>> > >   * irrelevant for normal operation.
>> > >   */
>> > >  
>> > > -#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
>> > > -                          (intel_dp)->psr.source_support)
>> > > -
>> > >  #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)-
>> > > >psr.sink_panel_replay_support && \
>> > >                                     (intel_dp)-
>> > > >psr.source_panel_replay_support)
>> > >  
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
>> > > b/drivers/gpu/drm/i915/display/intel_psr.h
>> > > index cde781df84d5..3d9920ebafab 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
>> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
>> > > @@ -21,6 +21,9 @@ struct intel_encoder;
>> > >  struct intel_plane;
>> > >  struct intel_plane_state;
>> > >  
>> > > +#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
>> > > +                          (intel_dp)->psr.source_support)
>> > > +
>> > >  bool intel_encoder_can_psr(struct intel_encoder *encoder);
>> > >  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
>> > >  void intel_psr_pre_plane_update(struct intel_atomic_state *state,
>> > > -- 
>> > > 2.34.1
>> > 
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index ab415f41924d..143981b91e8b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3326,6 +3326,14 @@  bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
 		fastset = false;
 	}
 
+	if (CAN_PSR(intel_dp)) {
+		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute PSR state\
+n",
+			    encoder->base.base.id, encoder->base.name);
+		crtc_state->uapi.mode_changed = true;
+		fastset = false;
+	}
+
 	return fastset;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 1010b8c405df..b6db7dbfaf1a 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -173,9 +173,6 @@ 
  * irrelevant for normal operation.
  */
 
-#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
-			   (intel_dp)->psr.source_support)
-
 #define CAN_PANEL_REPLAY(intel_dp) ((intel_dp)->psr.sink_panel_replay_support && \
 				    (intel_dp)->psr.source_panel_replay_support)
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index cde781df84d5..3d9920ebafab 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -21,6 +21,9 @@  struct intel_encoder;
 struct intel_plane;
 struct intel_plane_state;
 
+#define CAN_PSR(intel_dp) ((intel_dp)->psr.sink_support && \
+			   (intel_dp)->psr.source_support)
+
 bool intel_encoder_can_psr(struct intel_encoder *encoder);
 void intel_psr_init_dpcd(struct intel_dp *intel_dp);
 void intel_psr_pre_plane_update(struct intel_atomic_state *state,