Re: [PATCH 1/2] drm/i915/vlv_dsi: Add DMI quirk for wrong panel modeline in BIOS on Asus TF103C

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 21, 2022 at 11:06:07PM +0100, Hans de Goede wrote:
> Vtotal is wrong in the BIOS supplied modeline for the DSI panel on

Please include both the correct and bad modelines in the commit
msg.

> the Asus TF103C leading to the last line of the display being shown
> as the first line.
> 
> The factory installed Android has a hardcoded modeline in its kernel,
> causing it to not suffer from this BIOS bug;
> 
> and the Android boot-splash which uses the EFI FB which does have this bug
> has the last line all black causing the bug to not be visible.
> 
> This commit introduces a generic DMI based mechanism for doing modeline
> fixups, in case we need similar fixups on other models in the future.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/vlv_dsi.c | 36 ++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 06ef822c27bd..66f5cf32bb66 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -23,6 +23,7 @@
>   * Author: Jani Nikula <jani.nikula@intel.com>
>   */
>  
> +#include <linux/dmi.h>
>  #include <linux/slab.h>
>  
>  #include <drm/drm_atomic_helper.h>
> @@ -1831,6 +1832,33 @@ static void vlv_dphy_param_init(struct intel_dsi *intel_dsi)
>  	intel_dsi_log_params(intel_dsi);
>  }
>  
> +typedef void (*vlv_dsi_mode_fixup_func)(struct drm_connector *connector,
> +					struct drm_display_mode *fixed_mode);
> +
> +/*
> + * Vtotal is wrong on the Asus TF103C leading to the last line of the display
> + * being shown as the first line. The factory installed Android has a hardcoded
> + * modeline, causing it to not suffer from this BIOS bug.
> + */
> +static void vlv_dsi_asus_tf103c_mode_fixup(struct drm_connector *connector,
> +					   struct drm_display_mode *fixed_mode)
> +{
> +	fixed_mode->vtotal = 816;

I might prefer a full modeline here. Or maybe just vtotal-- or
something, if it's just an off by one.

> +	fixed_mode->crtc_vtotal = 816;

The crtc timings should all be 0 at this point. So this looks redundant.

> +}
> +
> +static const struct dmi_system_id dmi_mode_fixup_table[] = {
> +	{
> +		/* Asus Transformer Pad TF103C */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "TF103C"),
> +		},
> +		.driver_data = (void *)vlv_dsi_asus_tf103c_mode_fixup,
> +	},
> +	{ }
> +};
> +
>  void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
> @@ -1840,6 +1868,8 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  	struct intel_connector *intel_connector;
>  	struct drm_connector *connector;
>  	struct drm_display_mode *current_mode, *fixed_mode;
> +	const struct dmi_system_id *dmi_id;
> +	vlv_dsi_mode_fixup_func mode_fixup;

The function pointer can go into the if block.

>  	enum port port;
>  	enum pipe pipe;
>  
> @@ -1968,6 +1998,12 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  		goto err_cleanup_connector;
>  	}
>  
> +	dmi_id = dmi_first_match(dmi_mode_fixup_table);
> +	if (dmi_id) {
> +		mode_fixup = (vlv_dsi_mode_fixup_func)dmi_id->driver_data;
> +		mode_fixup(connector, fixed_mode);
> +	}
> +
>  	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>  	intel_backlight_setup(intel_connector, INVALID_PIPE);
>  
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel