On Thu, Apr 07, 2022 at 08:49:00PM +0300, Jani Nikula wrote: > On Tue, 05 Apr 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > Make the panel type code a bit more abstract along the > > lines of the source of the panel type. For the moment > > we have three classes: OpRegion, VBT, fallback. > > Well introduce another one shortly. > > > > We can now also print out all the different panel types, > > and indicate which one we ultimately selected. Could help > > with debugging. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_bios.c | 47 ++++++++++++++++------- > > 1 file changed, 34 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > > index 26839263abf0..feef5aa97398 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bios.c > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > > @@ -606,25 +606,46 @@ static int vbt_panel_type(struct drm_i915_private *i915) > > return lvds_options->panel_type; > > } > > > > +enum panel_type { > > + PANEL_TYPE_OPREGION, > > + PANEL_TYPE_VBT, > > + PANEL_TYPE_FALLBACK, > > +}; > > + > > static int get_panel_type(struct drm_i915_private *i915) > > { > > - int ret; > > + struct { > > + const char *name; > > + int panel_type; > > + } panel_types[] = { > > + [PANEL_TYPE_OPREGION] = { .name = "OpRegion", .panel_type = -1, }, > > + [PANEL_TYPE_VBT] = { .name = "VBT", .panel_type = -1, }, > > + [PANEL_TYPE_FALLBACK] = { .name = "fallback", .panel_type = 0, }, > > + }; > > + int i; > > > > - ret = intel_opregion_get_panel_type(i915); > > - if (ret >= 0) { > > - drm_WARN_ON(&i915->drm, ret > 0xf); > > - drm_dbg_kms(&i915->drm, "Panel type: %d (OpRegion)\n", ret); > > - return ret; > > - } > > + panel_types[PANEL_TYPE_OPREGION].panel_type = intel_opregion_get_panel_type(i915); > > + panel_types[PANEL_TYPE_VBT].panel_type = vbt_panel_type(i915); > > I'd probably add a function pointer in the array, to be able to call > these nicely in the loop. Needs a static wrapper function for > intel_opregion_get_panel_type() in the follow-up to eat the edid > parameter, but I think it's worth it. Sure. > > > + > > + for (i = 0; i < ARRAY_SIZE(panel_types); i++) { > > + drm_WARN_ON(&i915->drm, panel_types[i].panel_type > 0xf); > > I know that's loud, but it's kind of annoying that the out of bounds > value still goes through here. > > > > > - ret = vbt_panel_type(i915); > > - if (ret >= 0) { > > - drm_WARN_ON(&i915->drm, ret > 0xf); > > - drm_dbg_kms(&i915->drm, "Panel type: %d (VBT)\n", ret); > > - return ret; > > + if (panel_types[i].panel_type >= 0) > > + drm_dbg_kms(&i915->drm, "Panel type (%s): %d\n", > > + panel_types[i].name, panel_types[i].panel_type); > > } > > > > - return 0; /* fallback */ > > + if (panel_types[PANEL_TYPE_OPREGION].panel_type >= 0) > > + i = PANEL_TYPE_OPREGION; > > + else if (panel_types[PANEL_TYPE_VBT].panel_type >= 0) > > + i = PANEL_TYPE_VBT; > > + else > > + i = PANEL_TYPE_FALLBACK; > > At this stage, we could set this in the loop too, but dunno how > complicated that gets with the pnpid match rules. I don't think the PNPID stuff really lends itself well to a loop since it needs to consider both the VBT and PNPID pane types at the same time. > > Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> > > > + > > + drm_dbg_kms(&i915->drm, "Selected panel type (%s): %d\n", > > + panel_types[i].name, panel_types[i].panel_type); > > + > > + return panel_types[i].panel_type; > > } > > > > /* Parse general panel options */ > > -- > Jani Nikula, Intel Open Source Graphics Center -- Ville Syrjälä Intel