Re: [Intel-gfx] [PATCH v2] drm/i915/bios: calculate panel type as per child device index in VBT

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jun 10, 2022 at 01:54:12PM +0300, Jani Nikula wrote:
> On Thu, 09 Jun 2022, Animesh Manna <animesh.manna@intel.com> wrote:
> > Each LFP may have different panel type which is stored in LFP data
> > data block. Based on the child device index respective panel-type/
> > panel-type2 field will be used.
> >
> > v1: Initial rfc verion.
> > v2: Based on review comments from Jani,
> > - Used panel-type instead addition panel-index variable.
> > - DEVICE_HANDLE_* name changed and placed before DEVICE_TYPE_*
> > macro.
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c        |  2 +-
> >  drivers/gpu/drm/i915/display/intel_bios.c     | 40 +++++++++++++------
> >  drivers/gpu/drm/i915/display/intel_bios.h     |  3 +-
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +-
> >  drivers/gpu/drm/i915/display/intel_lvds.c     |  3 +-
> >  drivers/gpu/drm/i915/display/intel_sdvo.c     |  2 +-
> >  drivers/gpu/drm/i915/display/intel_vbt_defs.h |  4 ++
> >  drivers/gpu/drm/i915/display/vlv_dsi.c        |  2 +-
> >  8 files changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 3b5305c219ba..b3aa430abd03 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -2050,7 +2050,7 @@ void icl_dsi_init(struct drm_i915_private *dev_priv)
> >  	/* attach connector to encoder */
> >  	intel_connector_attach_encoder(intel_connector, encoder);
> >  
> > -	intel_bios_init_panel(dev_priv, &intel_connector->panel, NULL);
> > +	intel_bios_init_panel(dev_priv, intel_connector, NULL);
> >  
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	intel_panel_add_vbt_lfp_fixed_mode(intel_connector);
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index aaea27fe5d16..f74e63823c08 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -604,13 +604,15 @@ get_lfp_data_tail(const struct bdb_lvds_lfp_data *data,
> >  }
> >  
> >  static int opregion_get_panel_type(struct drm_i915_private *i915,
> > -				   const struct edid *edid)
> > +				   const struct edid *edid,
> > +				   const struct intel_bios_encoder_data *devdata)
> >  {
> >  	return intel_opregion_get_panel_type(i915);
> >  }
> >  
> >  static int vbt_get_panel_type(struct drm_i915_private *i915,
> > -			      const struct edid *edid)
> > +			      const struct edid *edid,
> > +			      const struct intel_bios_encoder_data *devdata)
> 
> This is nitpicking, but semantically feels like the devdata parameter
> should be before edid.
> 
> >  {
> >  	const struct bdb_lvds_options *lvds_options;
> >  
> > @@ -625,11 +627,17 @@ static int vbt_get_panel_type(struct drm_i915_private *i915,
> >  		return -1;
> >  	}
> >  
> > -	return lvds_options->panel_type;
> > +	if (devdata->child.handle == DEVICE_HANDLE_LFP1)
> > +		return lvds_options->panel_type;
> > +	else if (devdata->child.handle == DEVICE_HANDLE_LFP2)
> > +		return lvds_options->panel_type2;
> > +	else
> > +		return -1;
> 
> Not all legacy panels have encoder data (i.e. VBT child device
> config). I'd go for something like this:
> 
> 	if (devdata && devdata->child.handle == DEVICE_HANDLE_LFP2)
> 		return lvds_options->panel_type2;
> 
> 	drm_WARN_ON(&i915->drm, devdata && devdata->child.handle != DEVICE_HANDLE_LFP1)
> 
> 	return lvds_options->panel_type;
> 
> I don't know if that's going to lead to a bunch of warnings, but I'd
> want to know. Or we can demote it to drm_dbg_kms(), now or later.

I went through my VBT stash and looks like handle==LFP1 should
hold for everything (even my ancient i830 has that). So I'd go
with a WARN.

> >  }
> >  
> >  static int pnpid_get_panel_type(struct drm_i915_private *i915,
> > -				const struct edid *edid)
> > +				const struct edid *edid,
> > +				const struct intel_bios_encoder_data *devdata)
> >  {
> >  	const struct bdb_lvds_lfp_data *data;
> >  	const struct bdb_lvds_lfp_data_ptrs *ptrs;
> > @@ -675,7 +683,8 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
> >  }
> >  
> >  static int fallback_get_panel_type(struct drm_i915_private *i915,
> > -				   const struct edid *edid)
> > +				   const struct edid *edid,
> > +				   const struct intel_bios_encoder_data *devdata)
> >  {
> >  	return 0;
> >  }
> > @@ -688,12 +697,14 @@ enum panel_type {
> >  };
> >  
> >  static int get_panel_type(struct drm_i915_private *i915,
> > -			  const struct edid *edid)
> > +			  const struct edid *edid,
> > +			  const struct intel_bios_encoder_data *devdata)
> >  {
> >  	struct {
> >  		const char *name;
> >  		int (*get_panel_type)(struct drm_i915_private *i915,
> > -				      const struct edid *edid);
> > +				      const struct edid *edid,
> > +				      const struct intel_bios_encoder_data *devdata);
> >  		int panel_type;
> >  	} panel_types[] = {
> >  		[PANEL_TYPE_OPREGION] = {
> > @@ -716,7 +727,7 @@ static int get_panel_type(struct drm_i915_private *i915,
> >  	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(panel_types); i++) {
> > -		panel_types[i].panel_type = panel_types[i].get_panel_type(i915, edid);
> > +		panel_types[i].panel_type = panel_types[i].get_panel_type(i915, edid, devdata);
> >  
> >  		drm_WARN_ON(&i915->drm, panel_types[i].panel_type > 0xf &&
> >  			    panel_types[i].panel_type != 0xff);
> > @@ -747,7 +758,8 @@ static int get_panel_type(struct drm_i915_private *i915,
> >  static void
> >  parse_panel_options(struct drm_i915_private *i915,
> >  		    struct intel_panel *panel,
> > -		    const struct edid *edid)
> > +		    const struct edid *edid,
> > +		    const struct intel_bios_encoder_data *devdata)
> >  {
> >  	const struct bdb_lvds_options *lvds_options;
> >  	int panel_type;
> > @@ -759,7 +771,7 @@ parse_panel_options(struct drm_i915_private *i915,
> >  
> >  	panel->vbt.lvds_dither = lvds_options->pixel_dither;
> >  
> > -	panel_type = get_panel_type(i915, edid);
> > +	panel_type = get_panel_type(i915, edid, devdata);
> >  
> >  	panel->vbt.panel_type = panel_type;
> >  
> > @@ -3103,12 +3115,16 @@ void intel_bios_init(struct drm_i915_private *i915)
> >  }
> >  
> >  void intel_bios_init_panel(struct drm_i915_private *i915,
> > -			   struct intel_panel *panel,
> > +			   struct intel_connector *intel_connector,
> >  			   const struct edid *edid)
> >  {
> > +	struct intel_panel *panel = &intel_connector->panel;
> > +	struct intel_encoder *encoder = intel_connector->encoder;
> 
> At least vlv_dsi_init() calls intel_bios_init_panel() before setting
> intel_connector->encoder, which would oops on the next line.

The different order of stuff for vlv_dsi_init() vs. icl_dsi_init()
is rather annoying. Some kind of unification effort might be nice.

> 
> > +	const struct intel_bios_encoder_data *devdata = i915->vbt.ports[encoder->port];
> > +
> 
> intel_bios_init_panel() gets called:
> 
> * On platforms/outputs where encoder->port does not make sense,
>   e.g. intel_lvds_init() sets it to PORT_NONE.
> 
> * On platforms where i915->vbt.ports[] is not initialized at all. See
>   has_ddi_port_info().
> 
> * On platforms/outputs where i915->vbt.ports[] is not
>   initialized. Specifically, DSI is not handled by parse_ddi_port()
>   because on VLV, at least in theory, the DSI ports can coexist and
>   collide with other ports.
> 
> I'm wondering if maybe it's best to have the caller figure out const
> struct intel_bios_encoder_data *, and pass that along. If it's not
> possible, just pass NULL. For DP on DDI platforms it's already set in
> encoder->devdata. (We should basically set that on all platforms where
> it's available, but we're not there yet.)
> 
> This should work trivially for the immediate goal of enabling multiple
> eDP panels, and be compatible with enabling multiple DSI or combo
> eDP/DSI panels in the future once we've figured out how to fix devdata
> for DSI.
> 
> Ville, thoughts?

Sounds reasonable enough to me.

-- 
Ville Syrjälä
Intel