| Message ID | 20180208174855.55620-5-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, 2018-02-08 18:48 GMT+01:00 Sean Paul <seanpaul@chromium.org>: > This patch adds the ability to override the typical display timing for a > given panel. This is useful for devices which have timing constraints > that do not apply across the entire display driver (eg: to avoid > crosstalk between panel and digitizer on certain laptops). The rules are > as follows: > > - panel must not specify fixed mode (since the override mode will > either be the same as the fixed mode, or we'll be unable to > check the bounds of the overried) > - panel must specify at least one display_timing range which will be > used to ensure the override mode fits within its bounds > > Changes in v2: > - Parse the full display-timings node (using the native-mode) (Rob) > Changes in v3: > - No longer parse display-timings subnode, use panel-timing (Rob) > > Cc: Doug Anderson <dianders@chromium.org> > Cc: Eric Anholt <eric@anholt.net> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jeffy Chen <jeffy.chen@rock-chips.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Stéphane Marchesin <marcheu@chromium.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: devicetree@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 5591984a392b..87488392bca1 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -34,6 +34,7 @@ > #include <drm/drm_panel.h> > > #include <video/display_timing.h> > +#include <video/of_display_timing.h> > #include <video/videomode.h> > > struct panel_desc { > @@ -87,6 +88,8 @@ struct panel_simple { > struct i2c_adapter *ddc; > > struct gpio_desc *enable_gpio; > + > + struct drm_display_mode override_mode; > }; > > static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > @@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > struct drm_connector *connector = panel->base.connector; > struct drm_device *drm = panel->base.drm; > struct drm_display_mode *mode; > + bool has_override = panel->override_mode.type; > unsigned int i, num = 0; > > if (!panel->desc) > return 0; > > + if (has_override) { > + mode = drm_mode_duplicate(drm, &panel->override_mode); > + if (mode) { > + drm_mode_probed_add(connector, mode); > + num++; > + } else { > + dev_err(drm->dev, "failed to add override mode\n"); > + } > + } > + > for (i = 0; i < panel->desc->num_timings; i++) { > const struct display_timing *dt = &panel->desc->timings[i]; > struct videomode vm; > @@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > mode->type |= DRM_MODE_TYPE_DRIVER; > > - if (panel->desc->num_timings == 1) > + if (panel->desc->num_timings == 1 && !has_override) > mode->type |= DRM_MODE_TYPE_PREFERRED; > > drm_mode_probed_add(connector, mode); > @@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = { > .get_timings = panel_simple_get_timings, > }; > > +#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ > + (to_check->field.typ >= bounds->field.min && \ > + to_check->field.typ <= bounds->field.max) > +static void panel_simple_add_override_mode(struct device *dev, > + struct panel_simple *panel, > + const struct display_timing *ot) > +{ > + const struct panel_desc *desc = panel->desc; > + struct videomode vm; > + int i; > + > + if (desc->num_modes) { > + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); > + return; > + } > + if (!desc->num_timings) { > + dev_err(dev, "Reject override mode: no timings specified\n"); > + return; > + } > + > + for (i = 0; i < panel->desc->num_timings; i++) { > + const struct display_timing *dt = &panel->desc->timings[i]; > + > + if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len)) > + continue; > + > + if (ot->flags != dt->flags) > + continue; > + > + videomode_from_timing(ot, &vm); > + drm_display_mode_from_videomode(&vm, &panel->override_mode); > + panel->override_mode.type |= DRM_MODE_TYPE_DRIVER | > + DRM_MODE_TYPE_PREFERRED; > + return; > + } > + > + dev_err(dev, "Reject override mode: No display_timing found\n"); > + return; > +} > + > static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > { > struct device_node *backlight, *ddc; > struct panel_simple *panel; > + struct display_timing dt; > int err; > > panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); > @@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > } > } > > + if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) > + panel_simple_add_override_mode(dev, panel, &dt); > + > drm_panel_init(&panel->base); > panel->base.dev = dev; > panel->base.funcs = &panel_simple_funcs; > -- Tested on top of linux-next on a Samsung Chromebook Plus. Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > 2.16.0.rc1.238.g530d649a79-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Feb 08, 2018 at 12:48:51PM -0500, Sean Paul wrote: > This patch adds the ability to override the typical display timing for a > given panel. This is useful for devices which have timing constraints > that do not apply across the entire display driver (eg: to avoid > crosstalk between panel and digitizer on certain laptops). The rules are > as follows: > > - panel must not specify fixed mode (since the override mode will > either be the same as the fixed mode, or we'll be unable to > check the bounds of the overried) > - panel must specify at least one display_timing range which will be > used to ensure the override mode fits within its bounds > > Changes in v2: > - Parse the full display-timings node (using the native-mode) (Rob) > Changes in v3: > - No longer parse display-timings subnode, use panel-timing (Rob) > > Cc: Doug Anderson <dianders@chromium.org> > Cc: Eric Anholt <eric@anholt.net> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jeffy Chen <jeffy.chen@rock-chips.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Stéphane Marchesin <marcheu@chromium.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: devicetree@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 5591984a392b..87488392bca1 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -34,6 +34,7 @@ > #include <drm/drm_panel.h> > > #include <video/display_timing.h> > +#include <video/of_display_timing.h> > #include <video/videomode.h> > > struct panel_desc { > @@ -87,6 +88,8 @@ struct panel_simple { > struct i2c_adapter *ddc; > > struct gpio_desc *enable_gpio; > + > + struct drm_display_mode override_mode; > }; > > static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > @@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > struct drm_connector *connector = panel->base.connector; > struct drm_device *drm = panel->base.drm; > struct drm_display_mode *mode; > + bool has_override = panel->override_mode.type; > unsigned int i, num = 0; > > if (!panel->desc) > return 0; > > + if (has_override) { > + mode = drm_mode_duplicate(drm, &panel->override_mode); > + if (mode) { > + drm_mode_probed_add(connector, mode); > + num++; > + } else { > + dev_err(drm->dev, "failed to add override mode\n"); > + } > + } Do we really want to continue after this? Shouldn't the override mode simply override everything else? That is, if users do override the mode, do we want to give them additional modes to potentially choose from? Presumably the reason why the user had to override is because the fixed one didn't work. Actually, we should only ever have either the display timings specified or a fixed mode. Anything else is rather bogus. > + > for (i = 0; i < panel->desc->num_timings; i++) { > const struct display_timing *dt = &panel->desc->timings[i]; > struct videomode vm; > @@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > mode->type |= DRM_MODE_TYPE_DRIVER; > > - if (panel->desc->num_timings == 1) > + if (panel->desc->num_timings == 1 && !has_override) > mode->type |= DRM_MODE_TYPE_PREFERRED; > > drm_mode_probed_add(connector, mode); > @@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = { > .get_timings = panel_simple_get_timings, > }; > > +#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ > + (to_check->field.typ >= bounds->field.min && \ > + to_check->field.typ <= bounds->field.max) > +static void panel_simple_add_override_mode(struct device *dev, > + struct panel_simple *panel, > + const struct display_timing *ot) > +{ > + const struct panel_desc *desc = panel->desc; > + struct videomode vm; > + int i; unsigned int, please. > + > + if (desc->num_modes) { > + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); > + return; > + } > + if (!desc->num_timings) { > + dev_err(dev, "Reject override mode: no timings specified\n"); > + return; > + } Perhaps these should be WARN_ON() to be annoying, but let the driver continue with the override mode? Again, this is something that we should catch during review (when a new compatible is added, or a new driver, we should make sure that it always comes with a timing). WARN_ON() is probably enough to let people know when they test that they forgot something. > + > + for (i = 0; i < panel->desc->num_timings; i++) { > + const struct display_timing *dt = &panel->desc->timings[i]; > + > + if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) || > + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len)) > + continue; > + > + if (ot->flags != dt->flags) > + continue; > + > + videomode_from_timing(ot, &vm); > + drm_display_mode_from_videomode(&vm, &panel->override_mode); > + panel->override_mode.type |= DRM_MODE_TYPE_DRIVER | > + DRM_MODE_TYPE_PREFERRED; > + return; > + } > + > + dev_err(dev, "Reject override mode: No display_timing found\n"); Perhaps something like this, to simplify the code flow: if (!panel->override_mode.type) dev_err(dev, ...); Then turn the above return into a break and leave out the below return. > + return; > +} > + > static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > { > struct device_node *backlight, *ddc; > struct panel_simple *panel; > + struct display_timing dt; > int err; > > panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); > @@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > } > } > > + if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) > + panel_simple_add_override_mode(dev, panel, &dt); Perahps "panel_simple_parse_override_mode()"? To clarify what exactly this does. This doesn't actually "add" the mode yet, that's only done in panel_simple_get_fixed_modes(). Thierry
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 5591984a392b..87488392bca1 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -34,6 +34,7 @@ #include <drm/drm_panel.h> #include <video/display_timing.h> +#include <video/of_display_timing.h> #include <video/videomode.h> struct panel_desc { @@ -87,6 +88,8 @@ struct panel_simple { struct i2c_adapter *ddc; struct gpio_desc *enable_gpio; + + struct drm_display_mode override_mode; }; static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) @@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) struct drm_connector *connector = panel->base.connector; struct drm_device *drm = panel->base.drm; struct drm_display_mode *mode; + bool has_override = panel->override_mode.type; unsigned int i, num = 0; if (!panel->desc) return 0; + if (has_override) { + mode = drm_mode_duplicate(drm, &panel->override_mode); + if (mode) { + drm_mode_probed_add(connector, mode); + num++; + } else { + dev_err(drm->dev, "failed to add override mode\n"); + } + } + for (i = 0; i < panel->desc->num_timings; i++) { const struct display_timing *dt = &panel->desc->timings[i]; struct videomode vm; @@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) mode->type |= DRM_MODE_TYPE_DRIVER; - if (panel->desc->num_timings == 1) + if (panel->desc->num_timings == 1 && !has_override) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); @@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = { .get_timings = panel_simple_get_timings, }; +#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ + (to_check->field.typ >= bounds->field.min && \ + to_check->field.typ <= bounds->field.max) +static void panel_simple_add_override_mode(struct device *dev, + struct panel_simple *panel, + const struct display_timing *ot) +{ + const struct panel_desc *desc = panel->desc; + struct videomode vm; + int i; + + if (desc->num_modes) { + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); + return; + } + if (!desc->num_timings) { + dev_err(dev, "Reject override mode: no timings specified\n"); + return; + } + + for (i = 0; i < panel->desc->num_timings; i++) { + const struct display_timing *dt = &panel->desc->timings[i]; + + if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) || + !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len)) + continue; + + if (ot->flags != dt->flags) + continue; + + videomode_from_timing(ot, &vm); + drm_display_mode_from_videomode(&vm, &panel->override_mode); + panel->override_mode.type |= DRM_MODE_TYPE_DRIVER | + DRM_MODE_TYPE_PREFERRED; + return; + } + + dev_err(dev, "Reject override mode: No display_timing found\n"); + return; +} + static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) { struct device_node *backlight, *ddc; struct panel_simple *panel; + struct display_timing dt; int err; panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); @@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) } } + if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) + panel_simple_add_override_mode(dev, panel, &dt); + drm_panel_init(&panel->base); panel->base.dev = dev; panel->base.funcs = &panel_simple_funcs;
This patch adds the ability to override the typical display timing for a given panel. This is useful for devices which have timing constraints that do not apply across the entire display driver (eg: to avoid crosstalk between panel and digitizer on certain laptops). The rules are as follows: - panel must not specify fixed mode (since the override mode will either be the same as the fixed mode, or we'll be unable to check the bounds of the overried) - panel must specify at least one display_timing range which will be used to ensure the override mode fits within its bounds Changes in v2: - Parse the full display-timings node (using the native-mode) (Rob) Changes in v3: - No longer parse display-timings subnode, use panel-timing (Rob) Cc: Doug Anderson <dianders@chromium.org> Cc: Eric Anholt <eric@anholt.net> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Jeffy Chen <jeffy.chen@rock-chips.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Stéphane Marchesin <marcheu@chromium.org> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: devicetree@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-)