On Thu, Apr 07, 2022 at 07:21:44PM +0300, Jani Nikula wrote:
> On Tue, 05 Apr 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
>
> > + 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
