Re: [Intel-gfx] [PATCH v2 07/22] drm/i915/bios: Reorder panel DTD parsing

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

On Thu, Apr 07, 2022 at 07:21:44PM +0300, Jani Nikula wrote:
> On Tue, 05 Apr 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Reorder things so that we can parse the entier LFP data block
> 
> *entire
> 
> > in one go. For now we just stick to parsing the DTD from it.
> >
> > Also fix the misleading comment about block 42 being deprecated.
> > Only the DTD part is deprecated, the rest is still very much needed.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 62 ++++++++++++-----------
> >  1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 799c1fe36b23..f90991cac438 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -488,25 +488,16 @@ parse_panel_options(struct drm_i915_private *i915)
> >  	}
> >  }
> >  
> > -/* Try to find integrated panel timing data */
> >  static void
> > -parse_lfp_panel_dtd(struct drm_i915_private *i915)
> > +parse_lfp_panel_dtd(struct drm_i915_private *i915,
> > +		    const struct bdb_lvds_lfp_data *lvds_lfp_data,
> > +		    const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs)
> >  {
> > -	const struct bdb_lvds_lfp_data *lvds_lfp_data;
> > -	const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs;
> >  	const struct lvds_dvo_timing *panel_dvo_timing;
> >  	const struct lvds_fp_timing *fp_timing;
> >  	struct drm_display_mode *panel_fixed_mode;
> >  	int panel_type = i915->vbt.panel_type;
> >  
> > -	lvds_lfp_data = find_section(i915, BDB_LVDS_LFP_DATA);
> > -	if (!lvds_lfp_data)
> > -		return;
> > -
> > -	lvds_lfp_data_ptrs = find_section(i915, BDB_LVDS_LFP_DATA_PTRS);
> > -	if (!lvds_lfp_data_ptrs)
> > -		return;
> > -
> >  	panel_dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
> >  					       lvds_lfp_data_ptrs,
> >  					       panel_type);
> > @@ -537,6 +528,24 @@ parse_lfp_panel_dtd(struct drm_i915_private *i915)
> >  	}
> >  }
> >  
> > +static void
> > +parse_lfp_data(struct drm_i915_private *i915)
> > +{
> > +	const struct bdb_lvds_lfp_data *data;
> > +	const struct bdb_lvds_lfp_data_ptrs *ptrs;
> > +
> > +	ptrs = find_section(i915, BDB_LVDS_LFP_DATA_PTRS);
> > +	if (!ptrs)
> > +		return;
> > +
> > +	data = find_section(i915, BDB_LVDS_LFP_DATA);
> > +	if (!data)
> > +		return;
> > +
> > +	if (!i915->vbt.lfp_lvds_vbt_mode)
> > +		parse_lfp_panel_dtd(i915, data, ptrs);
> 
> Could do an early return on i915->vbt.lfp_lvds_vbt_mode.

I'm adding more stuff that we don't want to skip to the end of
the function in later patches.

> 
> > +}
> > +
> >  static void
> >  parse_generic_dtd(struct drm_i915_private *i915)
> >  {
> > @@ -615,23 +624,6 @@ parse_generic_dtd(struct drm_i915_private *i915)
> >  	i915->vbt.lfp_lvds_vbt_mode = panel_fixed_mode;
> >  }
> >  
> > -static void
> > -parse_panel_dtd(struct drm_i915_private *i915)
> > -{
> > -	/*
> > -	 * Older VBTs provided provided DTD information for internal displays
> > -	 * through the "LFP panel DTD" block (42).  As of VBT revision 229,
> > -	 * that block is now deprecated and DTD information should be provided
> > -	 * via a newer "generic DTD" block (58).  Just to be safe, we'll
> > -	 * try the new generic DTD block first on VBT >= 229, but still fall
> > -	 * back to trying the old LFP block if that fails.
> > -	 */
> > -	if (i915->vbt.version >= 229)
> > -		parse_generic_dtd(i915);
> > -	if (!i915->vbt.lfp_lvds_vbt_mode)
> > -		parse_lfp_panel_dtd(i915);
> > -}
> > -
> >  static void
> >  parse_lfp_backlight(struct drm_i915_private *i915)
> >  {
> > @@ -2708,7 +2700,17 @@ void intel_bios_init(struct drm_i915_private *i915)
> >  	parse_general_features(i915);
> >  	parse_general_definitions(i915);
> >  	parse_panel_options(i915);
> > -	parse_panel_dtd(i915);
> > +	/*
> > +	 * Older VBTs provided DTD information for internal displays through
> > +	 * the "LFP panel tables" block (42).  As of VBT revision 229 the
> > +	 * DTD information should be provided via a newer "generic DTD"
> > +	 * block (58).  Just to be safe, we'll try the new generic DTD block
> > +	 * first on VBT >= 229, but still fall back to trying the old LFP
> > +	 * block if that fails.
> > +	 */
> > +	if (i915->vbt.version >= 229)
> 
> I'd probably stick the vbt version check and the comment in
> parse_generic_dtd() instead of polluting the top level.

I suppose. Although that does make the ordering requirement between
parse_generic_dtd() vs. parse_lfp_data() a bit less clear. But maybe
this will all be rather temporary and we'll start transitioning
to a more on-demand based parsing of each thing.

> 
> Up to you if you want to do anything about the nitpicks,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> > +		parse_generic_dtd(i915);
> > +	parse_lfp_data(i915);
> >  	parse_lfp_backlight(i915);
> >  	parse_sdvo_panel_data(i915);
> >  	parse_driver_features(i915);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel