On Mon, 2024-02-05 at 04:50 +0000, Manna, Animesh wrote:
>
>
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx>
> > Sent: Friday, February 2, 2024 1:17 PM
> > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> > gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v3 04/21] drm/i915/psr: Rename
> > intel_psr_enabled
> >
> > On Fri, 2024-02-02 at 07:34 +0000, Manna, Animesh wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx>
> > > > Sent: Friday, January 19, 2024 3:40 PM
> > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Manna, Animesh <animesh.manna@xxxxxxxxx>; Hogander, Jouni
> > > > <jouni.hogander@xxxxxxxxx>
> > > > Subject: [PATCH v3 04/21] drm/i915/psr: Rename
> > > > intel_psr_enabled
> > > >
> > > > Intel_psr_enabled is now misleading name as we are using main
> > > > link
> > > > on with panel replay. I.e. link retraining is not controlled by
> > > > hardware.
> > > > Rename
> > > > intel_psr_enabled as intel_psr_hw_controls_link_retrain.
> > > >
> > > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> > > > drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
> > > > drivers/gpu/drm/i915/display/intel_psr.h | 2 +-
> > > > 3 files changed, 3 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..e7cda3162ea2 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -4951,7 +4951,7 @@ intel_dp_needs_link_retrain(struct
> > > > intel_dp
> > > > *intel_dp)
> > > > * Also when exiting PSR, HW will retrain the link
> > > > anyways
> > > > fixing
> > > > * any link status error.
> > > > */
> > > > - if (intel_psr_enabled(intel_dp))
> > > > + if (intel_psr_hw_controls_link_retrain(intel_dp))
> > > > return false;
> > > >
> > > > if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux,
> > > > DP_PHY_DPRX, diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index d11f8ea6e0a9..7b3290f4e0b4 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -3011,7 +3011,7 @@ void intel_psr_short_pulse(struct
> > > > intel_dp
> > > > *intel_dp)
> > > > mutex_unlock(&psr->lock);
> > > > }
> > > >
> > > > -bool intel_psr_enabled(struct intel_dp *intel_dp)
> > > > +bool intel_psr_hw_controls_link_retrain(struct intel_dp
> > > > *intel_dp)
> > >
> > > Based on CAN_PSR() check the function will return early and only
> > > get
> > > executed for psr. No sure still do we need to rename it?
> >
> > Ok. For me it was just surprice what it does and why this function
> > exists, thus
> > renaming. Much more descriptive. Also we will soon have main link
> > off with
> > Panel Replay as well then this is not about having PSR or Panel
> > Replay
> > enabled, but HW controlling link retraining.
> >
> > I'm fine with dropping the patch if you have strong opinion on
> > this.
>
> Do not see any value addition, though no strong objection.
I sent updated version of this set. I have addressed some of your
comments there including this on. Can you please recheck my patches.
BR,
Jouni Högander
>
> Regards,
> Animesh
> >
> > BR,
> >
> > Jouni Högander
> >
> > >
> > > Regards,
> > > Animesh
> > > > {
> > > > bool ret;
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > index cde781df84d5..f7c5cc07864f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > > @@ -45,7 +45,7 @@ void intel_psr_get_config(struct
> > > > intel_encoder
> > > > *encoder, void intel_psr_irq_handler(struct intel_dp
> > > > *intel_dp,
> > > > u32 psr_iir);
> > > > void intel_psr_short_pulse(struct intel_dp *intel_dp); void
> > > > intel_psr_wait_for_idle_locked(const struct intel_crtc_state
> > > > *new_crtc_state); -bool intel_psr_enabled(struct intel_dp
> > > > *intel_dp);
> > > > +bool intel_psr_hw_controls_link_retrain(struct intel_dp
> > > > *intel_dp);
> > > > int intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > > *state,
> > > > struct intel_crtc *crtc);
> > > > void intel_psr2_program_trans_man_trk_ctl(const struct
> > > > intel_crtc_state *crtc_state);
> > > > --
> > > > 2.34.1
> > >
>
