Re: [Intel-gfx] [PATCH v2 17/22] drm/i915/bios: Refactor panel_type code

[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>
>
> 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@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.

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

Reviewed-by: Jani Nikula <jani.nikula@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