[Intel-gfx] [PATCH] drm/i915/pps: added get_pps_idx() hook as part of pps_get_register() cleanup

Fri Aug 19 09:46:15 UTC 2022

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Wednesday, August 3, 2022 1:44 PM
> To: Manna, Animesh <animesh.manna at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com; Shankar, Uma <uma.shankar at intel.com>;
> Manna, Animesh <animesh.manna at intel.com>
> 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 at intel.com> 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 at intel.com>
> > ---
> >  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