Re: [PATCH v6 4/5] drm/panel-edp: Add override_edid_mode quirk for generic edp

[Maxime Ripard]

On Tue, Nov 07, 2023 at 12:41:54PM -0800, Hsin-Yi Wang wrote:
> Generic edp gets mode from edid. However, some panels report incorrect
> mode in this way, resulting in glitches on panel. Introduce a new quirk
> additional_mode to the generic edid to pick a correct hardcoded mode.
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> no change.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 67 ++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 599a949d74d1..c0c24d94c3a0 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -203,6 +203,9 @@ struct edp_panel_entry {
>  
>  	/** @name: Name of this panel (for printing to logs). */
>  	const char *name;
> +
> +	/** @override_edid_mode: Override the mode obtained by edid. */
> +	const struct drm_display_mode *override_edid_mode;
>  };
>  
>  struct panel_edp {
> @@ -301,6 +304,24 @@ static unsigned int panel_edp_get_display_modes(struct panel_edp *panel,
>  	return num;
>  }
>  
> +static int panel_edp_override_edid_mode(struct panel_edp *panel,
> +					struct drm_connector *connector,
> +					const struct drm_display_mode *override_mode)
> +{
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, override_mode);
> +	if (!mode) {
> +		dev_err(panel->base.dev, "failed to add additional mode\n");
> +		return 0;
> +	}
> +
> +	mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(connector, mode);
> +	return 1;
> +}
> +
>  static int panel_edp_get_non_edid_modes(struct panel_edp *panel,
>  					struct drm_connector *connector)
>  {
> @@ -568,6 +589,9 @@ static int panel_edp_get_modes(struct drm_panel *panel,
>  {
>  	struct panel_edp *p = to_panel_edp(panel);
>  	int num = 0;
> +	bool has_override_edid_mode = p->detected_panel &&
> +				      p->detected_panel != ERR_PTR(-EINVAL) &&
> +				      p->detected_panel->override_edid_mode;
>  
>  	/* probe EDID if a DDC bus is available */
>  	if (p->ddc) {
> @@ -575,9 +599,18 @@ static int panel_edp_get_modes(struct drm_panel *panel,
>  
>  		if (!p->edid)
>  			p->edid = drm_get_edid(connector, p->ddc);
> -
> -		if (p->edid)
> -			num += drm_add_edid_modes(connector, p->edid);
> +		if (p->edid) {
> +			if (has_override_edid_mode) {

It's not clear to me why the override mechanism is only there when
there's a ddc bus?

You mentioned before that you were following panel-simple, but that's a
clear deviation from what I can see. If there's a reason for that
deviation, that's fine by me, but it should at least be documented in
the commit log.

> +				/*
> +				 * override_edid_mode is specified. Use
> +				 * override_edid_mode instead of from edid.
> +				 */
> +				num += panel_edp_override_edid_mode(p, connector,
> +						p->detected_panel->override_edid_mode);
> +			} else {
> +				num += drm_add_edid_modes(connector, p->edid);
> +			}
> +		}
>  
>  		pm_runtime_mark_last_busy(panel->dev);
>  		pm_runtime_put_autosuspend(panel->dev);
> @@ -950,6 +983,19 @@ static const struct panel_desc auo_b101ean01 = {
>  	},
>  };
>  
> +static const struct drm_display_mode auo_b116xa3_mode = {
> +	.clock = 70589,
> +	.hdisplay = 1366,
> +	.hsync_start = 1366 + 40,
> +	.hsync_end = 1366 + 40 + 40,
> +	.htotal = 1366 + 40 + 40 + 32,
> +	.vdisplay = 768,
> +	.vsync_start = 768 + 10,
> +	.vsync_end = 768 + 10 + 12,
> +	.vtotal = 768 + 10 + 12 + 6,
> +	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};

That should be a separate patch

Maxime

Attachment: signature.asc
Description: PGP signature