> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Wednesday, August 3, 2022 1:44 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: ville.syrjala@xxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> Manna, Animesh <animesh.manna@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915/pps: added get_pps_idx() hook as part of
> pps_get_register() cleanup
>
> On Wed, 03 Aug 2022, Animesh Manna <animesh.manna@xxxxxxxxx> wrote:
> > To support dual LFP two instances of pps added from display gen12 onwards.
> > Few older platform like VLV also has dual pps support but handling is
> > different. So added separate hook get_pps_idx() to formulate which pps
> > instance to used for a soecific LFP on a specific platform.
> >
> > Simplified pps_get_register() which use get_pps_idx() hook to derive
> > the pps instance and get_pps_idx() will be initialized at pps_init().
> >
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 5 ++++
> > drivers/gpu/drm/i915/display/intel_bios.h | 1 +
> > .../drm/i915/display/intel_display_types.h | 2 ++
> > drivers/gpu/drm/i915/display/intel_pps.c | 25 ++++++++++++++-----
> > 4 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> > b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 51dde5bfd956..42315615a728 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -611,6 +611,11 @@ static int opregion_get_panel_type(struct
> drm_i915_private *i915,
> > return intel_opregion_get_panel_type(i915);
> > }
> >
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder) {
> > + return encoder->devdata && encoder->devdata->child.handle ==
> > +DEVICE_HANDLE_LFP2; }
>
> AFAICS the encoder never really needs to know whether it's "lfp1" or "lfp2". It
> needs to know the pps controller number.
>
> > +
> > static int vbt_get_panel_type(struct drm_i915_private *i915,
> > const struct intel_bios_encoder_data *devdata,
> > const struct edid *edid)
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> > b/drivers/gpu/drm/i915/display/intel_bios.h
> > index e47582b0de0a..aea72a87ea2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -251,6 +251,7 @@ bool intel_bios_is_lspcon_present(const struct
> drm_i915_private *i915,
> > enum port port);
> > bool intel_bios_is_lane_reversal_needed(const struct drm_i915_private *i915,
> > enum port port);
> > +bool intel_bios_is_lfp2(struct intel_encoder *encoder);
> > enum aux_ch intel_bios_port_aux_ch(struct drm_i915_private *dev_priv,
> > enum port port); bool intel_bios_get_dsc_params(struct intel_encoder
> *encoder,
> > struct intel_crtc_state *crtc_state, diff --git
> > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 0da9b208d56e..95f71a572b07 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1723,6 +1723,8 @@ struct intel_dp {
> >
> > /* When we last wrote the OUI for eDP */
> > unsigned long last_oui_write;
> > +
> > + int (*get_pps_idx)(struct intel_dp *intel_dp);
>
> Making this a function pointer should be a separate step. Please don't try to do
> too many things at once. Rule of thumb, one change per patch.
>
> Also, this should be placed near the other function pointer members in struct
> intel_dp.
>
> > };
> >
> > enum lspcon_vendor {
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 1b21a341962f..c9cdb302d318 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -231,6 +231,17 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
> > return backlight_controller;
> > }
> >
> > +static int
> > +gen12_power_sequencer_idx(struct intel_dp *intel_dp) {
> > + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > + if (intel_bios_is_lfp2(encoder))
> > + return 1;
>
> This is actually not how this works. The bxt_power_sequencer_idx() matches
> bspec 20149: "PWM and PPS are assumed to be aligned to be from same block
> and not mismatch" i.e. the backlight controller id and the pps id are the same.
> There are no requirements to map lfp# and pps controller like this, even if it
> might be true in the common case.
>
> The problem is we need the information *before* we call
> intel_bios_init_panel().
>
> It's a catch-22. We need the pps id to read the panel EDID, and we need the
> panel EDID to choose the correct panel type in VBT, which we need to get the
> pps id from the VBT.
>
> This is highlighted in [1]. The 2nd eDP (which is not even physically present, only
> in the VBT, *sigh*) screws up the PPS for the 1st during init.
>
> I think Ville had some ideas here.
Hi Ville,
Can you please share your thoughts on the above?
Regards,
Animesh
>
>
> BR,
> Jani.
>
>
> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/5531
>
>
> > +
> > + return 0;
> > +}
> > +
> > typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
> > enum pipe pipe);
> >
> > @@ -361,15 +372,10 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
> > struct pps_registers *regs)
> > {
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > - int pps_idx = 0;
> > + int pps_idx = intel_dp->get_pps_idx(intel_dp);
> >
> > memset(regs, 0, sizeof(*regs));
> >
> > - if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> > - pps_idx = bxt_power_sequencer_idx(intel_dp);
> > - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - pps_idx = vlv_power_sequencer_pipe(intel_dp);
> > -
> > regs->pp_ctrl = PP_CONTROL(pps_idx);
> > regs->pp_stat = PP_STATUS(pps_idx);
> > regs->pp_on = PP_ON_DELAYS(pps_idx); @@ -1431,6 +1437,13 @@
> void
> > intel_pps_init(struct intel_dp *intel_dp)
> > intel_dp->pps.initializing = true;
> > INIT_DELAYED_WORK(&intel_dp->pps.panel_vdd_work,
> > edp_panel_vdd_work);
> >
> > + if (IS_GEMINILAKE(i915) || IS_BROXTON(i915))
> > + intel_dp->get_pps_idx = bxt_power_sequencer_idx;
> > + else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > + intel_dp->get_pps_idx = vlv_power_sequencer_pipe;
> > + else if (DISPLAY_VER(i915) >= 12)
> > + intel_dp->get_pps_idx = gen12_power_sequencer_idx;
> > +
> > pps_init_timestamps(intel_dp);
> >
> > with_intel_pps_lock(intel_dp, wakeref) {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
