| Message ID | 20200711094726.GA940164@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: panel-simple: validate panel description | expand |
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,
Hi Laurent. On Sun, Jul 12, 2020 at 01:56:16AM +0300, Laurent Pinchart wrote: > 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 ? We should be noisy, but not too noisy. dev_warn() is fine, maybe only dev_info()? Unless I get other feedback I will convert to dev_info() in the next version of the patch. Fixing all panels before we add the checks, is not feasible. Access to datasheet is not easy/possible for many panels. Sam > > [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
Hi Sam, On Sun, Jul 12, 2020 at 12:58:19PM +0200, Sam Ravnborg wrote: > On Sun, Jul 12, 2020 at 01:56:16AM +0300, Laurent Pinchart wrote: > > 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 ? > > We should be noisy, but not too noisy. > dev_warn() is fine, maybe only dev_info()? > > Unless I get other feedback I will convert to dev_info() in the > next version of the patch. > Fixing all panels before we add the checks, is not feasible. > Access to datasheet is not easy/possible for many panels. And that's the whole point of adding the warning, trying to get the people who have access to the panels to help fixing the problem. I think a dev_warn() would be a good compromise, the missing info can have adverse consequences, so a warning seems appropriate. > > [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,
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; } drm_panel_init(&panel->base, dev, &panel_simple_funcs,
Warn is we detect a panel with missing descriptions. This is inpsired by a similar patch by Laurent that introduced checks for LVDS panels - this extends the checks to the reminaing type of 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(-)