On Tue, 2022-01-04 at 21:21 +0530, Manna, Animesh wrote:
> Hi,
>
> > -----Original Message-----
> > From: Souza, Jose <jose.souza@xxxxxxxxx>
> > Sent: Wednesday, November 24, 2021 1:19 AM
> > To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Manna, Animesh
> > <animesh.manna@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Mun, Gwan-gyeong <gwan-gyeong.mun@xxxxxxxxx>; Nikula, Jani
> > <jani.nikula@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>; Navare,
> > Manasi D <manasi.d.navare@xxxxxxxxx>
> > Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> > config for panel replay
> >
> > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > > As panel replay feature similar to PSR feature of EDP panel, so
> > > currently utilized existing psr framework for panel replay.
> > >
> > > v1: RFC version.
> > > v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> > > v3:
> > > - code comments improved. [Jani]
> > > - dpcd_readb used instead of dpcd_read. [Jani]
> > > - panel-repaplay init/compute functions moved inside respective psr
> > > function. [Jani]
> > >
> > > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > > ---
> > > .../drm/i915/display/intel_display_types.h | 2 +
> > > drivers/gpu/drm/i915/display/intel_dp.c | 43 +++++++++++++----
> > > drivers/gpu/drm/i915/display/intel_psr.c | 48 +++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_psr.h | 3 ++
> > > 4 files changed, 87 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 39e11eaec1a3..48f7d676ed2c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > > bool req_psr2_sdp_prior_scanline;
> > > u32 dc3co_exitline;
> > > u16 su_y_granularity;
> > > + bool has_panel_replay;
> >
> > We can drop this and reuse current ones ones, see bellow.
> >
> > > struct drm_dp_vsc_sdp psr_vsc;
> > >
> > > /*
> > > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > > bool irq_aux_error;
> > > u16 su_w_granularity;
> > > u16 su_y_granularity;
> > > + bool sink_panel_replay_support;
> >
> > move this closer to has_psr and set both when it is panel replay.
> > otherwise psr functions will not be executed for panel replay, see CAN_PSR().
>
> AFIU Panel replay do not have any dependency with PSR.
> So that’s why I have created separate function for panel replay which is doing similar thing whatever needed for panel replay.
> For example intel_panel_replay_compute_config() can be independent of intel_psr_compute_config().
> Do you see any dependency with PSR for panel replay?
There is no dependency but panel replay is PSR for DP so we should re-use PSR code as much as possible.
eDP + sink_support = PSR
DP + sink_support = panel replay
So we can reuse has_psr and all other stuff.
>
> >
> > > u32 dc3co_exitline;
> > > u32 dc3co_exit_delay;
> > > struct delayed_work dc3co_work;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 10fda20a5bd8..f58a7b72be14 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1587,12 +1587,22 @@ static void
> > intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >
> > > - /*
> > > - * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > - * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > - * Colorimetry Format indication.
> > > - */
> > > - vsc->revision = 0x5;
> > > + if (crtc_state->has_panel_replay) {
> > > + /*
> > > + * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > > + * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > > + * Encoding/Colorimetry Format indication.
> > > + */
> > > + vsc->revision = 0x7;
> > > + } else {
> > > + /*
> > > + * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > + * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > + * Colorimetry Format indication.
> > > + */
> > > + vsc->revision = 0x5;
> > > + }
> > > +
> > > vsc->length = 0x13;
> > >
> > > /* DP 1.4a spec, Table 2-120 */
> > > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> > intel_dp *intel_dp,
> > > vsc->revision = 0x4;
> > > vsc->length = 0xe;
> > > }
> > > + } else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > > + if (intel_dp->psr.colorimetry_support &&
> > > + intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > > + /* [Panel Replay with colorimetry info] */
> > > + intel_dp_compute_vsc_colorimetry(crtc_state,
> > conn_state,
> > > + vsc);
> > > + } else {
> > > + /*
> > > + * [Panel Replay without colorimetry info]
> > > + * Prepare VSC Header for SU as per DP 2.0 spec, Table
> > 2-223
> > > + * VSC SDP supporting 3D stereo + Panel Replay.
> > > + */
> > > + vsc->revision = 0x6;
> > > + vsc->length = 0x10;
> > > + }
> > > } else {
> > > /*
> > > * [PSR1]
> > > @@ -2749,10 +2774,10 @@ static ssize_t intel_dp_vsc_sdp_pack(const struct
> > drm_dp_vsc_sdp *vsc,
> > > sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> > >
> > > /*
> > > - * Only revision 0x5 supports Pixel Encoding/Colorimetry Format as
> > > - * per DP 1.4a spec.
> > > + * Revision 0x5 and 0x7 supports Pixel Encoding/Colorimetry Format as
> > > + * per DP 1.4a spec and DP 2.0 spec respectively.
> > > */
> > > - if (vsc->revision != 0x5)
> > > + if (vsc->revision != 0x5 || vsc->revision != 0x7)
> > > goto out;
> > >
> > > /* VSC SDP Payload for DB16 through DB18 */ diff --git
> > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 7a205fd5023b..91c2efe2f3ad 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -933,6 +933,21 @@ static bool intel_psr2_config_valid(struct intel_dp
> > *intel_dp,
> > > return true;
> > > }
> > >
> > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > + struct intel_crtc_state *crtc_state) {
> > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +
> > > + if (!intel_dp->psr.sink_panel_replay_support)
> > > + return;
> > > +
> > > + crtc_state->has_panel_replay = true;
> > > + crtc_state->infoframes.enable |=
> > > +intel_hdmi_infoframe_enable(DP_SDP_VSC);
> > > +
> > > + if (HAS_PSR2_SEL_FETCH(i915))
> > > + intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state); }
> > > +
> > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > struct intel_crtc_state *crtc_state,
> > > struct drm_connector_state *conn_state) @@ -
> > 942,6 +957,8 @@
> > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > &crtc_state->hw.adjusted_mode;
> > > int psr_setup_time;
> > >
> > > + intel_panel_replay_compute_config(intel_dp, crtc_state);
> >
> > have you checked if the other PSR are needed for panel replay? what about the
> > psr2 checks? when using panel replay selective update some additional tests will
> > be needed regarding granularity...
>
> As mentioned above I understood panel replay do not have any dependency with PSR.
> Will not target panel replay selective update for dg2.
>
> >
> > > +
> > > /*
> > > * Current PSR panels dont work reliably with VRR enabled
> > > * So if VRR is enabled, do not enable PSR.
> > > @@ -2170,6 +2187,35 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> > > }
> > > }
> > >
> > > +/**
> > > + * intel_panel_replay_init - Check for sink and source capability.
> > > + * @intel_dp: Intel DP
> > > + *
> > > + * This function is called after the initializing connector.
> > > + * (the initializing of connector treats the handling of connector
> > > +capabilities)
> > > + * And it initializes basic panel replay stuff for each DP Encoder.
> > > + */
> > > +void intel_panel_replay_init(struct intel_dp *intel_dp) {
> > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > + u8 pr_dpcd = 0;
> > > +
> > > + if (!(HAS_DP20(dev_priv) && HAS_PANEL_REPLAY(dev_priv)))
> > > + return;
> > > +
> > > + drm_dp_dpcd_readb(&intel_dp->aux, DP_PANEL_REPLAY_CAP,
> > &pr_dpcd);
> > > +
> > > + if (!(pr_dpcd & PANEL_REPLAY_SUPPORT)) {
> > > + drm_dbg_kms(&dev_priv->drm,
> > > + "Panel replay is not supported by panel\n");
> > > + return;
> > > + }
> > > +
> > > + drm_dbg_kms(&dev_priv->drm,
> > > + "Panel replay is supported by panel\n");
> > > + intel_dp->psr.sink_panel_replay_support = true; }
> > > +
> > > /**
> > > * intel_psr_init - Init basic PSR work and mutex.
> > > * @intel_dp: Intel DP
> > > @@ -2183,6 +2229,8 @@ void intel_psr_init(struct intel_dp *intel_dp)
> > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > >
> > > + intel_panel_replay_init(intel_dp);
> > > +
> > > if (!HAS_PSR(dev_priv))
> > > return;
> >
> > mutex initialization is not executed, workers not initialized...
> > please go more carefully trough every PSR function and check what are the
> > panel replay implications
>
> Tried to double check once more.
> Currently as per bspec and dp 2.0 spec did not see any dependency with PSR.
> Not sure if we really need worker thread to enter in panel replay mode unlike psr where we need to wait for few idle frames.
> Any suggestion/input will be helpful here.
>
> Regards,
> Animesh
>
> >
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index facffbacd357..c9d1c1f0b470 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -32,6 +32,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> > > unsigned frontbuffer_bits,
> > > enum fb_op_origin origin);
> > > void intel_psr_init(struct intel_dp *intel_dp);
> > > +void intel_panel_replay_init(struct intel_dp *intel_dp);
> > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > struct intel_crtc_state *crtc_state,
> > > struct drm_connector_state *conn_state); @@ -
> > 52,5 +53,7 @@
> > > void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > const struct intel_crtc_state
> > *crtc_state); void
> > > intel_psr_pause(struct intel_dp *intel_dp); void
> > > intel_psr_resume(struct intel_dp *intel_dp);
> > > +void intel_panel_replay_compute_config(struct intel_dp *intel_dp,
> > > + struct intel_crtc_state *crtc_state);
> > >
> > > #endif /* __INTEL_PSR_H__ */
>
