On Sun, Jun 28, 2020 at 02:48:20PM +0300, Laurent Pinchart wrote:
> Hi Sam,
>
> On Sun, Jun 28, 2020 at 11:13:47AM +0200, Sam Ravnborg wrote:
> > On Sun, Jun 28, 2020 at 10:54:01AM +0300, Laurent Pinchart wrote:
> > > The DRM bus flags reporting on which clock edge the pixel data and sync
> > > signals are sampled or driven don't make sense for LVDS panels, as the
> > > bus then uses sub-clock timings to send data. Drop those flags and add a
> > > warning in the probe function to make sure the mistake won't be
> > > repeated.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/panel/panel-simple.c | 30 +++++++++++++++++-----------
> > > 1 file changed, 18 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index bff1cab529d2..537d75ef7fc2 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -549,6 +549,14 @@ 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. */
> > > + WARN_ON(desc->bus_flags &
> > > + (DRM_BUS_FLAG_PIXDATA_POSEDGE |
> > > + DRM_BUS_FLAG_PIXDATA_NEGEDGE |
> > > + DRM_BUS_FLAG_SYNC_POSEDGE |
> > > + DRM_BUS_FLAG_SYNC_NEGEDGE));
> >
> > The above are only the legacy flags.
>
> We currently have
>
> DRM_BUS_FLAG_PIXDATA_POSEDGE = BIT(2),
> DRM_BUS_FLAG_PIXDATA_NEGEDGE = BIT(3),
> DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
> DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
> DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE = DRM_BUS_FLAG_PIXDATA_POSEDGE,
>
> Only the first two are legacy, but the last four have the same values. I
> could pick DRIVE or SAMPLE (the latter seems to be a better match here)
> instead.
We want to get rid of the legacy values, so yes, no more new users.
Sam
>
> > For LVDS panels I think we could check if either
> > DRM_BUS_FLAG_DE_LOW or DRM_BUS_FLAG_DE_HIGH is set and nothing else.
>
> A whitelist instead of a blacklist is a good idea.
>
> > And then as an additional check that the .bus_format is relevant.
>
> I have a patch for that, but we first need to fix two occurrences of
> incorrect formats. As I couldn't find documentation for these two
> panels, I've replied to the patches that introduced them, and am waiting
> for answers.
>
> > Then we have managed to catch most of the mistakes for LVDS panels.
> >
> > We could add checks for DPI panels too, but that would be later.
> >
> > > +
> > > drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> > > desc->connector_type);
> > >
> > > @@ -688,7 +696,7 @@ static const struct panel_desc auo_b101aw03 = {
> > > .height = 125,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -1398,7 +1406,7 @@ static const struct panel_desc chunghwa_claa070wp03xg = {
> > > .height = 150,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -1423,7 +1431,7 @@ static const struct panel_desc chunghwa_claa101wa01a = {
> > > .height = 120,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -1448,7 +1456,7 @@ static const struct panel_desc chunghwa_claa101wb01 = {
> > > .height = 125,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -2123,7 +2131,7 @@ static const struct panel_desc innolux_n156bge_l21 = {
> > > .height = 193,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -2256,7 +2264,7 @@ static const struct panel_desc koe_tx26d202vm0bwa = {
> > > .disable = 1000,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -2497,9 +2505,7 @@ static const struct panel_desc logictechno_lt170410_2whc = {
> > > .height = 136,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH |
> > > - DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> > > - DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -3072,7 +3078,7 @@ static const struct panel_desc samsung_ltn101nt05 = {
> > > .height = 125,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -3319,7 +3325,7 @@ static const struct panel_desc starry_kr070pe2t = {
> > > .height = 86,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
> > > @@ -3512,7 +3518,7 @@ static const struct panel_desc toshiba_lt089ac29000 = {
> > > .height = 116,
> > > },
> > > .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> > > - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> > > .connector_type = DRM_MODE_CONNECTOR_LVDS,
> > > };
> > >
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
