Re: [PATCH] drm/panel: panel-simple: validate panel description

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sam,

(CC'ing Daniel)

Thank you for the patch.

On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
> Warn is we detect a panel with missing descriptions.

s/is/if/

> This is inpsired by a similar patch by Laurent that introduced checks
> for LVDS panels - this extends the checks to the reminaing type of

s/reminaing type/remaining types/

> connectors.
> 
> This is known to fail for some of the existing panels but added
> despite this as we need help from people using the panels to
> add the missing info.
> The checks are not complete but will catch the most common mistakes.
> The checks at the same time serves as documentation for the minimum
> required description for a panel.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> This is my attempt on the validation described in the previous mail.
> The assignment of default connector_type will then be a follow-up patch
> to this.
> 
> 	Sam
> 
>  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 2aff93accad5..025a7ccdfcb3 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
>  	}
>  
> -	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> -		/* Catch common mistakes for LVDS panels. */
> +	/* Catch common mistakes for panels. */
> +	switch (desc->connector_type) {
> +	case 0:
> +		WARN(desc->connector_type == 0, "specify missing connector_type\n");
> +		break;
> +	case DRM_MODE_CONNECTOR_LVDS:
>  		WARN_ON(desc->bus_flags &
>  			~(DRM_BUS_FLAG_DE_LOW |
>  			  DRM_BUS_FLAG_DE_HIGH |
> @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
>  			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
>  			desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		WARN_ON(desc->bus_format == 0);
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_DPI:
> +		WARN_ON(desc->bus_flags &
> +			~(DRM_BUS_FLAG_DE_LOW |
> +			  DRM_BUS_FLAG_DE_HIGH |
> +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +			  DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> +			  DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> +			  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> +			  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
> +		WARN_ON(desc->bus_format == 0);
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	default:
> +		WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type);
> +		break;
>  	}

The checks look sane to me. For LVDS we've added the WARN_ON after
checking all LVDS panels [1], so the warning will only get displayed for
new panel drivers. For other types of panel, this will cause lots of
WARN_ON to trigger. On one hand it gets the issues noticed, which should
help fixing them, but on the other hand it will also scare lots of users
and developers. I'm not sure if we should downgrade that to a dev_warn()
for some time until we get at least the majority of the issues fixed.
Daniel, any opinion ?

[1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix
bpc for LG LB070WV8 panel" to fix one bpc issue.

>  	drm_panel_init(&panel->base, dev, &panel_simple_funcs,

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel