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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

> +}
> +
>  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.

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