On Thu, Apr 07, 2022 at 08:49:00PM +0300, Jani Nikula wrote:
> On Tue, 05 Apr 2022, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >
> > 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.syrj...@linux.intel.com>
> > ---
> >  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.nik...@intel.com>
> 
> > +
> > +   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

Reply via email to