diff mbox series

[2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario

Message ID 20220916083102.18559-2-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/pps: Add get_pps_idx() hook as part of pps_get_register() cleanup | expand

Commit Message

Manna, Animesh Sept. 16, 2022, 8:31 a.m. UTC
From display gen12 onwards to support dual EDP two instances of pps added.
Currently backlight controller and pps instance can be mapped together
for a specific panel. Extended support for gen12 for dual EDP usage.

v1: Iniital revision
v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init. [Jani]

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
 drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
 drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
 drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
 4 files changed, 14 insertions(+), 11 deletions(-)

Comments

Ville Syrjälä Sept. 16, 2022, 8:58 a.m. UTC | #1
On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
> >From display gen12 onwards to support dual EDP two instances of pps added.
> Currently backlight controller and pps instance can be mapped together
> for a specific panel. Extended support for gen12 for dual EDP usage.
> 
> v1: Iniital revision
> v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init. [Jani]
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
>  drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
>  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
>  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
>  4 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 28bdb936cd1f..5fd4c09dfa96 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct drm_i915_private *i915,
>  	return 0;
>  }
>  
> -enum panel_type {
> -	PANEL_TYPE_OPREGION,
> -	PANEL_TYPE_VBT,
> -	PANEL_TYPE_PNPID,
> -	PANEL_TYPE_FALLBACK,
> -};
> -
>  static int 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 e375405a7828..da01b13260ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -231,6 +231,13 @@ struct mipi_pps_data {
>  	u16 panel_power_cycle_delay;
>  } __packed;
>  
> +enum panel_type {
> +	PANEL_TYPE_OPREGION,
> +	PANEL_TYPE_VBT,
> +	PANEL_TYPE_PNPID,
> +	PANEL_TYPE_FALLBACK,
> +};
> +
>  void intel_bios_init(struct drm_i915_private *dev_priv);
>  void intel_bios_init_panel(struct drm_i915_private *dev_priv,
>  			   struct intel_panel *panel,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c19e99ee06b6..6f7afa75ec4d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5222,6 +5222,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	intel_bios_init_panel(dev_priv, &intel_connector->panel,
> +			      encoder->devdata, NULL);

We don't want to go for the fallback type here if we
the VBT wants us to use pnpid. Maybe we should just
remove the fallback type entirely? Or perhaps only
use it if the VBT panel type is entirely invalid?

> +
>  	intel_pps_init(intel_dp);
>  
>  	/* Cache DPCD and EDID for edp. */
> @@ -5255,9 +5258,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		edid = ERR_PTR(-ENOENT);
>  	}
>  	intel_connector->edid = edid;
> -
> -	intel_bios_init_panel(dev_priv, &intel_connector->panel,
> -			      encoder->devdata, IS_ERR(edid) ? NULL : edid);
> +	if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
> +		intel_bios_init_panel(dev_priv, &intel_connector->panel,
> +				      encoder->devdata, IS_ERR(edid) ? NULL : edid);
>  
>  	intel_panel_add_edid_fixed_modes(intel_connector,
>  					 intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE,
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index b972fa6ec00d..4b8413382c5d 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1430,7 +1430,7 @@ 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))
> +	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) || DISPLAY_VER(i915) >= 12)
>  		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;
> -- 
> 2.29.0
Manna, Animesh Sept. 16, 2022, 11:02 a.m. UTC | #2
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, September 16, 2022 2:28 PM
> To: Manna, Animesh <animesh.manna@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario
> 
> On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
> > >From display gen12 onwards to support dual EDP two instances of pps added.
> > Currently backlight controller and pps instance can be mapped together
> > for a specific panel. Extended support for gen12 for dual EDP usage.
> >
> > v1: Iniital revision
> > v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init.
> > [Jani]
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
> > drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
> >  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
> >  4 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> > b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 28bdb936cd1f..5fd4c09dfa96 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct
> drm_i915_private *i915,
> >  	return 0;
> >  }
> >
> > -enum panel_type {
> > -	PANEL_TYPE_OPREGION,
> > -	PANEL_TYPE_VBT,
> > -	PANEL_TYPE_PNPID,
> > -	PANEL_TYPE_FALLBACK,
> > -};
> > -
> >  static int 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 e375405a7828..da01b13260ae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -231,6 +231,13 @@ struct mipi_pps_data {
> >  	u16 panel_power_cycle_delay;
> >  } __packed;
> >
> > +enum panel_type {
> > +	PANEL_TYPE_OPREGION,
> > +	PANEL_TYPE_VBT,
> > +	PANEL_TYPE_PNPID,
> > +	PANEL_TYPE_FALLBACK,
> > +};
> > +
> >  void intel_bios_init(struct drm_i915_private *dev_priv);  void
> > intel_bios_init_panel(struct drm_i915_private *dev_priv,
> >  			   struct intel_panel *panel,
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index c19e99ee06b6..6f7afa75ec4d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5222,6 +5222,9 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
> >  		return false;
> >  	}
> >
> > +	intel_bios_init_panel(dev_priv, &intel_connector->panel,
> > +			      encoder->devdata, NULL);
> 
> We don't want to go for the fallback type here if we the VBT wants us to use
> pnpid. Maybe we should just remove the fallback type entirely? Or perhaps only
> use it if the VBT panel type is entirely invalid?

Ok, Can I add like below?
If (!PANEL_TYPE_FALLBACK)
	intel_pps_init(intel_dp);

But to read EDID pps_init should be called before it. Or else I can enable both the pps and later disable the unused one.

Regards,
Animesh
 
> > +
> >  	intel_pps_init(intel_dp);
> >
> >  	/* Cache DPCD and EDID for edp. */
> > @@ -5255,9 +5258,9 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
> >  		edid = ERR_PTR(-ENOENT);
> >  	}
> >  	intel_connector->edid = edid;
> > -
> > -	intel_bios_init_panel(dev_priv, &intel_connector->panel,
> > -			      encoder->devdata, IS_ERR(edid) ? NULL : edid);
> > +	if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
> > +		intel_bios_init_panel(dev_priv, &intel_connector->panel,
> > +				      encoder->devdata, IS_ERR(edid) ? NULL :
> edid);
> >
> >  	intel_panel_add_edid_fixed_modes(intel_connector,
> >  					 intel_connector->panel.vbt.drrs_type
> != DRRS_TYPE_NONE, diff
> > --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index b972fa6ec00d..4b8413382c5d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -1430,7 +1430,7 @@ 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))
> > +	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) || DISPLAY_VER(i915) >=
> > +12)
> >  		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;
> > --
> > 2.29.0
> 
> --
> Ville Syrjälä
> Intel
Jani Nikula Sept. 16, 2022, 11:29 a.m. UTC | #3
On Fri, 16 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
>> -----Original Message-----
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Sent: Friday, September 16, 2022 2:28 PM
>> To: Manna, Animesh <animesh.manna@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
>> Shankar, Uma <uma.shankar@intel.com>
>> Subject: Re: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario
>>
>> On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
>> > >From display gen12 onwards to support dual EDP two instances of pps added.
>> > Currently backlight controller and pps instance can be mapped together
>> > for a specific panel. Extended support for gen12 for dual EDP usage.
>> >
>> > v1: Iniital revision
>> > v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init.
>> > [Jani]
>> >
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Cc: Uma Shankar <uma.shankar@intel.com>
>> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
>> > drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
>> >  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
>> >  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
>> >  4 files changed, 14 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>> > b/drivers/gpu/drm/i915/display/intel_bios.c
>> > index 28bdb936cd1f..5fd4c09dfa96 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> > @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct
>> drm_i915_private *i915,
>> >     return 0;
>> >  }
>> >
>> > -enum panel_type {
>> > -   PANEL_TYPE_OPREGION,
>> > -   PANEL_TYPE_VBT,
>> > -   PANEL_TYPE_PNPID,
>> > -   PANEL_TYPE_FALLBACK,
>> > -};
>> > -
>> >  static int 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 e375405a7828..da01b13260ae 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
>> > @@ -231,6 +231,13 @@ struct mipi_pps_data {
>> >     u16 panel_power_cycle_delay;
>> >  } __packed;
>> >
>> > +enum panel_type {
>> > +   PANEL_TYPE_OPREGION,
>> > +   PANEL_TYPE_VBT,
>> > +   PANEL_TYPE_PNPID,
>> > +   PANEL_TYPE_FALLBACK,
>> > +};
>> > +

I don't want enum panel_type exposed from intel_bios.c at all, there's
no reason for that.

>> >  void intel_bios_init(struct drm_i915_private *dev_priv);  void
>> > intel_bios_init_panel(struct drm_i915_private *dev_priv,
>> >                        struct intel_panel *panel,
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index c19e99ee06b6..6f7afa75ec4d 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -5222,6 +5222,9 @@ static bool intel_edp_init_connector(struct intel_dp
>> *intel_dp,
>> >             return false;
>> >     }
>> >
>> > +   intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> > +                         encoder->devdata, NULL);
>>
>> We don't want to go for the fallback type here if we the VBT wants us to use
>> pnpid. Maybe we should just remove the fallback type entirely? Or perhaps only
>> use it if the VBT panel type is entirely invalid?
>
> Ok, Can I add like below?
> If (!PANEL_TYPE_FALLBACK)
>         intel_pps_init(intel_dp);
>
> But to read EDID pps_init should be called before it. Or else I can enable both the pps and later disable the unused one.

The first call should init everything if it can (panel type is *not*
pnpid). Fallback is fine in that case too.

If panel type indicates pnpid, intel_bios_init_panel() should set the
pps id to, say, -1, so intel_pps_init() or more specifically
bxt_power_sequencer_idx() can use some default or look at the hardware
or whatever.

intel_bios_init_panel() should probably also return a value on pnpid
indicating intel_edp_init_connector() should call
intel_bios_init_panel() again, this time with EDID. (Note: I kind of
like separating returning the value and setting the pps id to -1. I
don't want intel_edp_init_connector() to look at pps id, that's for pps,
and I don't want to pass flags all the way to bxt_power_sequencer_idx()
either.)


BR,
Jani.


>
> Regards,
> Animesh
>
>> > +
>> >     intel_pps_init(intel_dp);
>> >
>> >     /* Cache DPCD and EDID for edp. */
>> > @@ -5255,9 +5258,9 @@ static bool intel_edp_init_connector(struct intel_dp
>> *intel_dp,
>> >             edid = ERR_PTR(-ENOENT);
>> >     }
>> >     intel_connector->edid = edid;
>> > -
>> > -   intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> > -                         encoder->devdata, IS_ERR(edid) ? NULL : edid);
>> > +   if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
>> > +           intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> > +                                 encoder->devdata, IS_ERR(edid) ? NULL :
>> edid);
>> >
>> >     intel_panel_add_edid_fixed_modes(intel_connector,
>> >                                      intel_connector->panel.vbt.drrs_type
>> != DRRS_TYPE_NONE, diff
>> > --git a/drivers/gpu/drm/i915/display/intel_pps.c
>> > b/drivers/gpu/drm/i915/display/intel_pps.c
>> > index b972fa6ec00d..4b8413382c5d 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> > @@ -1430,7 +1430,7 @@ 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))
>> > +   if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) || DISPLAY_VER(i915) >=
>> > +12)
>> >             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;
>> > --
>> > 2.29.0
>>
>> --
>> Ville Syrjälä
>> Intel
Manna, Animesh Sept. 20, 2022, 1:49 p.m. UTC | #4
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Friday, September 16, 2022 5:00 PM
> To: Manna, Animesh <animesh.manna@intel.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
> Subject: RE: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario
> 
> On Fri, 16 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
> >> -----Original Message-----
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Sent: Friday, September 16, 2022 2:28 PM
> >> To: Manna, Animesh <animesh.manna@intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> >> <jani.nikula@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> >> Subject: Re: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP
> >> scenario
> >>
> >> On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
> >> > >From display gen12 onwards to support dual EDP two instances of pps
> added.
> >> > Currently backlight controller and pps instance can be mapped
> >> > together for a specific panel. Extended support for gen12 for dual EDP
> usage.
> >> >
> >> > v1: Iniital revision
> >> > v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init.
> >> > [Jani]
> >> >
> >> > Cc: Jani Nikula <jani.nikula@intel.com>
> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Cc: Uma Shankar <uma.shankar@intel.com>
> >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
> >> > drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
> >> >  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
> >> >  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
> >> >  4 files changed, 14 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > index 28bdb936cd1f..5fd4c09dfa96 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct
> >> drm_i915_private *i915,
> >> >     return 0;
> >> >  }
> >> >
> >> > -enum panel_type {
> >> > -   PANEL_TYPE_OPREGION,
> >> > -   PANEL_TYPE_VBT,
> >> > -   PANEL_TYPE_PNPID,
> >> > -   PANEL_TYPE_FALLBACK,
> >> > -};
> >> > -
> >> >  static int 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 e375405a7828..da01b13260ae 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> >> > @@ -231,6 +231,13 @@ struct mipi_pps_data {
> >> >     u16 panel_power_cycle_delay;
> >> >  } __packed;
> >> >
> >> > +enum panel_type {
> >> > +   PANEL_TYPE_OPREGION,
> >> > +   PANEL_TYPE_VBT,
> >> > +   PANEL_TYPE_PNPID,
> >> > +   PANEL_TYPE_FALLBACK,
> >> > +};
> >> > +
> 
> I don't want enum panel_type exposed from intel_bios.c at all, there's no reason
> for that.

Hi Jani,

Thanks for your inputs. I have made the changes as per your suggestion.
I have a doubt which is mentioned below, currently have sent to trybot for quick feedback.
https://patchwork.freedesktop.org/patch/503759/?series=108786&rev=1

> 
> >> >  void intel_bios_init(struct drm_i915_private *dev_priv);  void
> >> > intel_bios_init_panel(struct drm_i915_private *dev_priv,
> >> >                        struct intel_panel *panel, diff --git
> >> > a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > index c19e99ee06b6..6f7afa75ec4d 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > @@ -5222,6 +5222,9 @@ static bool intel_edp_init_connector(struct
> >> > intel_dp
> >> *intel_dp,
> >> >             return false;
> >> >     }
> >> >
> >> > +   intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> > +                         encoder->devdata, NULL);
> >>
> >> We don't want to go for the fallback type here if we the VBT wants us
> >> to use pnpid. Maybe we should just remove the fallback type entirely?
> >> Or perhaps only use it if the VBT panel type is entirely invalid?
> >
> > Ok, Can I add like below?
> > If (!PANEL_TYPE_FALLBACK)
> >         intel_pps_init(intel_dp);
> >
> > But to read EDID pps_init should be called before it. Or else I can enable both
> the pps and later disable the unused one.
> 
> The first call should init everything if it can (panel type is *not* pnpid). Fallback is
> fine in that case too.

First time intel_bios_init_panel() will be called with null edid and panel type will be set to PANEL_TYPE_FALLBACK.
  
> 
> If panel type indicates pnpid, intel_bios_init_panel() should set the pps id to, say,
> -1, so intel_pps_init() or more specifically
> bxt_power_sequencer_idx() can use some default or look at the hardware or
> whatever.

Currently checking for PANEL_TYPE_PNPID in intel_bios_init_panel() and setting pps id to -1.
But I am not sure why special handling is needed in bxt_power_sequencer_idx().
Is backlight_controller value can be used to derive pps instance for PANEL_TYPE_PNPID.
 
> 
> intel_bios_init_panel() should probably also return a value on pnpid indicating
> intel_edp_init_connector() should call
> intel_bios_init_panel() again, this time with EDID. (Note: I kind of like separating
> returning the value and setting the pps id to -1. I don't want
> intel_edp_init_connector() to look at pps id, that's for pps, and I don't want to
> pass flags all the way to bxt_power_sequencer_idx()
> either.)

For PANEL_TYPE_PNPID am returning boolean retry, with that don’t need to move panet type enum to intel_bios.h.
If the retry is true will call intel_bios_init_panel() with edid to check for PANEL_TYPE_PNPID. 

Regards,
Animesh

> 
> 
> BR,
> Jani.
> 
> 
> >
> > Regards,
> > Animesh
> >
> >> > +
> >> >     intel_pps_init(intel_dp);
> >> >
> >> >     /* Cache DPCD and EDID for edp. */ @@ -5255,9 +5258,9 @@ static
> >> > bool intel_edp_init_connector(struct intel_dp
> >> *intel_dp,
> >> >             edid = ERR_PTR(-ENOENT);
> >> >     }
> >> >     intel_connector->edid = edid;
> >> > -
> >> > -   intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> > -                         encoder->devdata, IS_ERR(edid) ? NULL : edid);
> >> > +   if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
> >> > +           intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> > +                                 encoder->devdata, IS_ERR(edid) ? NULL :
> >> edid);
> >> >
> >> >     intel_panel_add_edid_fixed_modes(intel_connector,
> >> >
> >> > intel_connector->panel.vbt.drrs_type
> >> != DRRS_TYPE_NONE, diff
> >> > --git a/drivers/gpu/drm/i915/display/intel_pps.c
> >> > b/drivers/gpu/drm/i915/display/intel_pps.c
> >> > index b972fa6ec00d..4b8413382c5d 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> >> > @@ -1430,7 +1430,7 @@ 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))
> >> > +   if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) ||
> >> > +DISPLAY_VER(i915) >=
> >> > +12)
> >> >             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;
> >> > --
> >> > 2.29.0
> >>
> >> --
> >> Ville Syrjälä
> >> Intel
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Sept. 26, 2022, 10:35 a.m. UTC | #5
On Tue, 20 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Friday, September 16, 2022 5:00 PM
>> To: Manna, Animesh <animesh.manna@intel.com>; Ville Syrjälä
>> <ville.syrjala@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
>> Subject: RE: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario
>>
>> On Fri, 16 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
>> >> -----Original Message-----
>> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> Sent: Friday, September 16, 2022 2:28 PM
>> >> To: Manna, Animesh <animesh.manna@intel.com>
>> >> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
>> >> <jani.nikula@intel.com>; Shankar, Uma <uma.shankar@intel.com>
>> >> Subject: Re: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP
>> >> scenario
>> >>
>> >> On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
>> >> > >From display gen12 onwards to support dual EDP two instances of pps
>> added.
>> >> > Currently backlight controller and pps instance can be mapped
>> >> > together for a specific panel. Extended support for gen12 for dual EDP
>> usage.
>> >> >
>> >> > v1: Iniital revision
>> >> > v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init.
>> >> > [Jani]
>> >> >
>> >> > Cc: Jani Nikula <jani.nikula@intel.com>
>> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > Cc: Uma Shankar <uma.shankar@intel.com>
>> >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
>> >> > drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
>> >> >  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
>> >> >  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
>> >> >  4 files changed, 14 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > index 28bdb936cd1f..5fd4c09dfa96 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct
>> >> drm_i915_private *i915,
>> >> >     return 0;
>> >> >  }
>> >> >
>> >> > -enum panel_type {
>> >> > -   PANEL_TYPE_OPREGION,
>> >> > -   PANEL_TYPE_VBT,
>> >> > -   PANEL_TYPE_PNPID,
>> >> > -   PANEL_TYPE_FALLBACK,
>> >> > -};
>> >> > -
>> >> >  static int 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 e375405a7828..da01b13260ae 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
>> >> > @@ -231,6 +231,13 @@ struct mipi_pps_data {
>> >> >     u16 panel_power_cycle_delay;
>> >> >  } __packed;
>> >> >
>> >> > +enum panel_type {
>> >> > +   PANEL_TYPE_OPREGION,
>> >> > +   PANEL_TYPE_VBT,
>> >> > +   PANEL_TYPE_PNPID,
>> >> > +   PANEL_TYPE_FALLBACK,
>> >> > +};
>> >> > +
>>
>> I don't want enum panel_type exposed from intel_bios.c at all, there's no reason
>> for that.
>
> Hi Jani,
>
> Thanks for your inputs. I have made the changes as per your suggestion.
> I have a doubt which is mentioned below, currently have sent to trybot for quick feedback.
> https://patchwork.freedesktop.org/patch/503759/?series=108786&rev=1

I'm not subscribed to the trybot list, so I can't easily reply with
comments.

Use trybot only to *test* stuff, not if you want human feedback.

>
>>
>> >> >  void intel_bios_init(struct drm_i915_private *dev_priv);  void
>> >> > intel_bios_init_panel(struct drm_i915_private *dev_priv,
>> >> >                        struct intel_panel *panel, diff --git
>> >> > a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > index c19e99ee06b6..6f7afa75ec4d 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > @@ -5222,6 +5222,9 @@ static bool intel_edp_init_connector(struct
>> >> > intel_dp
>> >> *intel_dp,
>> >> >             return false;
>> >> >     }
>> >> >
>> >> > +   intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> >> > +                         encoder->devdata, NULL);
>> >>
>> >> We don't want to go for the fallback type here if we the VBT wants us
>> >> to use pnpid. Maybe we should just remove the fallback type entirely?
>> >> Or perhaps only use it if the VBT panel type is entirely invalid?
>> >
>> > Ok, Can I add like below?
>> > If (!PANEL_TYPE_FALLBACK)
>> >         intel_pps_init(intel_dp);
>> >
>> > But to read EDID pps_init should be called before it. Or else I can enable both
>> the pps and later disable the unused one.
>>
>> The first call should init everything if it can (panel type is *not* pnpid). Fallback is
>> fine in that case too.
>
> First time intel_bios_init_panel() will be called with null edid and panel type will be set to PANEL_TYPE_FALLBACK.
>
>>
>> If panel type indicates pnpid, intel_bios_init_panel() should set the pps id to, say,
>> -1, so intel_pps_init() or more specifically
>> bxt_power_sequencer_idx() can use some default or look at the hardware or
>> whatever.
>
> Currently checking for PANEL_TYPE_PNPID in intel_bios_init_panel() and setting pps id to -1.
> But I am not sure why special handling is needed in bxt_power_sequencer_idx().
> Is backlight_controller value can be used to derive pps instance for PANEL_TYPE_PNPID.

Backlight controller and pps index are basically the *same* thing. If
PNPID is used, the backlight controller is uninitialized.

This is actually a bug in the current code, even for single panel
configurations, if someone has chosen to use the 2nd pps/backlight.

BR,
Jani.

>
>>
>> intel_bios_init_panel() should probably also return a value on pnpid indicating
>> intel_edp_init_connector() should call
>> intel_bios_init_panel() again, this time with EDID. (Note: I kind of like separating
>> returning the value and setting the pps id to -1. I don't want
>> intel_edp_init_connector() to look at pps id, that's for pps, and I don't want to
>> pass flags all the way to bxt_power_sequencer_idx()
>> either.)
>
> For PANEL_TYPE_PNPID am returning boolean retry, with that don’t need to move panet type enum to intel_bios.h.
> If the retry is true will call intel_bios_init_panel() with edid to check for PANEL_TYPE_PNPID.
>
> Regards,
> Animesh
>
>>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > Regards,
>> > Animesh
>> >
>> >> > +
>> >> >     intel_pps_init(intel_dp);
>> >> >
>> >> >     /* Cache DPCD and EDID for edp. */ @@ -5255,9 +5258,9 @@ static
>> >> > bool intel_edp_init_connector(struct intel_dp
>> >> *intel_dp,
>> >> >             edid = ERR_PTR(-ENOENT);
>> >> >     }
>> >> >     intel_connector->edid = edid;
>> >> > -
>> >> > -   intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> >> > -                         encoder->devdata, IS_ERR(edid) ? NULL : edid);
>> >> > +   if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
>> >> > +           intel_bios_init_panel(dev_priv, &intel_connector->panel,
>> >> > +                                 encoder->devdata, IS_ERR(edid) ? NULL :
>> >> edid);
>> >> >
>> >> >     intel_panel_add_edid_fixed_modes(intel_connector,
>> >> >
>> >> > intel_connector->panel.vbt.drrs_type
>> >> != DRRS_TYPE_NONE, diff
>> >> > --git a/drivers/gpu/drm/i915/display/intel_pps.c
>> >> > b/drivers/gpu/drm/i915/display/intel_pps.c
>> >> > index b972fa6ec00d..4b8413382c5d 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> >> > @@ -1430,7 +1430,7 @@ 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))
>> >> > +   if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) ||
>> >> > +DISPLAY_VER(i915) >=
>> >> > +12)
>> >> >             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;
>> >> > --
>> >> > 2.29.0
>> >>
>> >> --
>> >> Ville Syrjälä
>> >> Intel
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
Manna, Animesh Sept. 27, 2022, 5:53 p.m. UTC | #6
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Monday, September 26, 2022 4:06 PM
> To: Manna, Animesh <animesh.manna@intel.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
> Subject: RE: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP scenario
> 
> On Tue, 20 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> Sent: Friday, September 16, 2022 5:00 PM
> >> To: Manna, Animesh <animesh.manna@intel.com>; Ville Syrjälä
> >> <ville.syrjala@linux.intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma
> >> <uma.shankar@intel.com>
> >> Subject: RE: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP
> >> scenario
> >>
> >> On Fri, 16 Sep 2022, "Manna, Animesh" <animesh.manna@intel.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >> Sent: Friday, September 16, 2022 2:28 PM
> >> >> To: Manna, Animesh <animesh.manna@intel.com>
> >> >> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> >> >> <jani.nikula@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> >> >> Subject: Re: [PATCH 2/2] drm/i915/pps: Enable 2nd pps for dual EDP
> >> >> scenario
> >> >>
> >> >> On Fri, Sep 16, 2022 at 02:01:02PM +0530, Animesh Manna wrote:
> >> >> > >From display gen12 onwards to support dual EDP two instances of
> >> >> > >pps
> >> added.
> >> >> > Currently backlight controller and pps instance can be mapped
> >> >> > together for a specific panel. Extended support for gen12 for
> >> >> > dual EDP
> >> usage.
> >> >> >
> >> >> > v1: Iniital revision
> >> >> > v2: Called intel_bios_panel_init w/o PNPID before intel_pps_init.
> >> >> > [Jani]
> >> >> >
> >> >> > Cc: Jani Nikula <jani.nikula@intel.com>
> >> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >> > Cc: Uma Shankar <uma.shankar@intel.com>
> >> >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 7 -------
> >> >> > drivers/gpu/drm/i915/display/intel_bios.h | 7 +++++++
> >> >> >  drivers/gpu/drm/i915/display/intel_dp.c   | 9 ++++++---
> >> >> >  drivers/gpu/drm/i915/display/intel_pps.c  | 2 +-
> >> >> >  4 files changed, 14 insertions(+), 11 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> > b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> > index 28bdb936cd1f..5fd4c09dfa96 100644
> >> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> > @@ -706,13 +706,6 @@ static int fallback_get_panel_type(struct
> >> >> drm_i915_private *i915,
> >> >> >     return 0;
> >> >> >  }
> >> >> >
> >> >> > -enum panel_type {
> >> >> > -   PANEL_TYPE_OPREGION,
> >> >> > -   PANEL_TYPE_VBT,
> >> >> > -   PANEL_TYPE_PNPID,
> >> >> > -   PANEL_TYPE_FALLBACK,
> >> >> > -};
> >> >> > -
> >> >> >  static int 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 e375405a7828..da01b13260ae 100644
> >> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> >> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> >> >> > @@ -231,6 +231,13 @@ struct mipi_pps_data {
> >> >> >     u16 panel_power_cycle_delay;  } __packed;
> >> >> >
> >> >> > +enum panel_type {
> >> >> > +   PANEL_TYPE_OPREGION,
> >> >> > +   PANEL_TYPE_VBT,
> >> >> > +   PANEL_TYPE_PNPID,
> >> >> > +   PANEL_TYPE_FALLBACK,
> >> >> > +};
> >> >> > +
> >>
> >> I don't want enum panel_type exposed from intel_bios.c at all,
> >> there's no reason for that.
> >
> > Hi Jani,
> >
> > Thanks for your inputs. I have made the changes as per your suggestion.
> > I have a doubt which is mentioned below, currently have sent to trybot for
> quick feedback.
> > https://patchwork.freedesktop.org/patch/503759/?series=108786&rev=1
> 
> I'm not subscribed to the trybot list, so I can't easily reply with comments.
> 
> Use trybot only to *test* stuff, not if you want human feedback.
> 
> >
> >>
> >> >> >  void intel_bios_init(struct drm_i915_private *dev_priv);  void
> >> >> > intel_bios_init_panel(struct drm_i915_private *dev_priv,
> >> >> >                        struct intel_panel *panel, diff --git
> >> >> > a/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > index c19e99ee06b6..6f7afa75ec4d 100644
> >> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > @@ -5222,6 +5222,9 @@ static bool
> >> >> > intel_edp_init_connector(struct intel_dp
> >> >> *intel_dp,
> >> >> >             return false;
> >> >> >     }
> >> >> >
> >> >> > +   intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> >> > +                         encoder->devdata, NULL);
> >> >>
> >> >> We don't want to go for the fallback type here if we the VBT wants
> >> >> us to use pnpid. Maybe we should just remove the fallback type entirely?
> >> >> Or perhaps only use it if the VBT panel type is entirely invalid?
> >> >
> >> > Ok, Can I add like below?
> >> > If (!PANEL_TYPE_FALLBACK)
> >> >         intel_pps_init(intel_dp);
> >> >
> >> > But to read EDID pps_init should be called before it. Or else I can
> >> > enable both
> >> the pps and later disable the unused one.
> >>
> >> The first call should init everything if it can (panel type is *not*
> >> pnpid). Fallback is fine in that case too.
> >
> > First time intel_bios_init_panel() will be called with null edid and panel type will
> be set to PANEL_TYPE_FALLBACK.
> >
> >>
> >> If panel type indicates pnpid, intel_bios_init_panel() should set the
> >> pps id to, say, -1, so intel_pps_init() or more specifically
> >> bxt_power_sequencer_idx() can use some default or look at the
> >> hardware or whatever.
> >
> > Currently checking for PANEL_TYPE_PNPID in intel_bios_init_panel() and
> setting pps id to -1.
> > But I am not sure why special handling is needed in
> bxt_power_sequencer_idx().
> > Is backlight_controller value can be used to derive pps instance for
> PANEL_TYPE_PNPID.
> 
> Backlight controller and pps index are basically the *same* thing. If PNPID is
> used, the backlight controller is uninitialized.
> 
> This is actually a bug in the current code, even for single panel configurations, if
> someone has chosen to use the 2nd pps/backlight.

I have sent the next version for review.
https://patchwork.freedesktop.org/series/109135/
Can you please have a look.

Regards,
Animesh

> 
> BR,
> Jani.
> 
> >
> >>
> >> intel_bios_init_panel() should probably also return a value on pnpid
> >> indicating
> >> intel_edp_init_connector() should call
> >> intel_bios_init_panel() again, this time with EDID. (Note: I kind of
> >> like separating returning the value and setting the pps id to -1. I
> >> don't want
> >> intel_edp_init_connector() to look at pps id, that's for pps, and I
> >> don't want to pass flags all the way to bxt_power_sequencer_idx()
> >> either.)
> >
> > For PANEL_TYPE_PNPID am returning boolean retry, with that don’t need to
> move panet type enum to intel_bios.h.
> > If the retry is true will call intel_bios_init_panel() with edid to check for
> PANEL_TYPE_PNPID.
> >
> > Regards,
> > Animesh
> >
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > Regards,
> >> > Animesh
> >> >
> >> >> > +
> >> >> >     intel_pps_init(intel_dp);
> >> >> >
> >> >> >     /* Cache DPCD and EDID for edp. */ @@ -5255,9 +5258,9 @@
> >> >> > static bool intel_edp_init_connector(struct intel_dp
> >> >> *intel_dp,
> >> >> >             edid = ERR_PTR(-ENOENT);
> >> >> >     }
> >> >> >     intel_connector->edid = edid;
> >> >> > -
> >> >> > -   intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> >> > -                         encoder->devdata, IS_ERR(edid) ? NULL : edid);
> >> >> > +   if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
> >> >> > +           intel_bios_init_panel(dev_priv, &intel_connector->panel,
> >> >> > +                                 encoder->devdata, IS_ERR(edid) ? NULL :
> >> >> edid);
> >> >> >
> >> >> >     intel_panel_add_edid_fixed_modes(intel_connector,
> >> >> >
> >> >> > intel_connector->panel.vbt.drrs_type
> >> >> != DRRS_TYPE_NONE, diff
> >> >> > --git a/drivers/gpu/drm/i915/display/intel_pps.c
> >> >> > b/drivers/gpu/drm/i915/display/intel_pps.c
> >> >> > index b972fa6ec00d..4b8413382c5d 100644
> >> >> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> >> >> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> >> >> > @@ -1430,7 +1430,7 @@ 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))
> >> >> > +   if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) ||
> >> >> > +DISPLAY_VER(i915) >=
> >> >> > +12)
> >> >> >             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;
> >> >> > --
> >> >> > 2.29.0
> >> >>
> >> >> --
> >> >> Ville Syrjälä
> >> >> Intel
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 28bdb936cd1f..5fd4c09dfa96 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -706,13 +706,6 @@  static int fallback_get_panel_type(struct drm_i915_private *i915,
 	return 0;
 }
 
-enum panel_type {
-	PANEL_TYPE_OPREGION,
-	PANEL_TYPE_VBT,
-	PANEL_TYPE_PNPID,
-	PANEL_TYPE_FALLBACK,
-};
-
 static int 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 e375405a7828..da01b13260ae 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -231,6 +231,13 @@  struct mipi_pps_data {
 	u16 panel_power_cycle_delay;
 } __packed;
 
+enum panel_type {
+	PANEL_TYPE_OPREGION,
+	PANEL_TYPE_VBT,
+	PANEL_TYPE_PNPID,
+	PANEL_TYPE_FALLBACK,
+};
+
 void intel_bios_init(struct drm_i915_private *dev_priv);
 void intel_bios_init_panel(struct drm_i915_private *dev_priv,
 			   struct intel_panel *panel,
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index c19e99ee06b6..6f7afa75ec4d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5222,6 +5222,9 @@  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	intel_bios_init_panel(dev_priv, &intel_connector->panel,
+			      encoder->devdata, NULL);
+
 	intel_pps_init(intel_dp);
 
 	/* Cache DPCD and EDID for edp. */
@@ -5255,9 +5258,9 @@  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		edid = ERR_PTR(-ENOENT);
 	}
 	intel_connector->edid = edid;
-
-	intel_bios_init_panel(dev_priv, &intel_connector->panel,
-			      encoder->devdata, IS_ERR(edid) ? NULL : edid);
+	if (intel_connector->panel.vbt.panel_type == PANEL_TYPE_FALLBACK)
+		intel_bios_init_panel(dev_priv, &intel_connector->panel,
+				      encoder->devdata, IS_ERR(edid) ? NULL : edid);
 
 	intel_panel_add_edid_fixed_modes(intel_connector,
 					 intel_connector->panel.vbt.drrs_type != DRRS_TYPE_NONE,
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index b972fa6ec00d..4b8413382c5d 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1430,7 +1430,7 @@  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))
+	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915) || DISPLAY_VER(i915) >= 12)
 		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;