| Message ID | 20190401171724.215780-3-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: simple: Add mode support to devicetree | expand |
On Mon, 1 Apr 2019 10:17:19 -0700 Douglas Anderson <dianders@chromium.org> wrote: > From: 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) > Changes in v4: > - Don't add mode from timing if override was specified (Thierry) > - Add warning if timing and fixed mode was specified (Thierry) > - Don't add fixed mode if timing was specified (Thierry) > - Refactor/rename a bit to avoid extra indentation from "if" tests > - i should be unsigned (Thierry) > - Add annoying WARN_ONs for some cases (Thierry) > - Simplify 'No display_timing found' handling (Thierry) > - Rename to panel_simple_parse_override_mode() (Thierry) > Changes in v5: > - Added Heiko's Tested-by > > 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> > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Heiko Stuebner <heiko@sntech.de> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > > drivers/gpu/drm/panel/panel-simple.c | 109 +++++++++++++++++++++++++-- > 1 file changed, 104 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 9e8218f6a3f2..ad4f4aac2d44 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 { > @@ -91,6 +92,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) > @@ -98,16 +101,13 @@ static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > return container_of(panel, struct panel_simple, base); > } > > -static int panel_simple_get_fixed_modes(struct panel_simple *panel) > +static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel) > { > struct drm_connector *connector = panel->base.connector; > struct drm_device *drm = panel->base.drm; > struct drm_display_mode *mode; > unsigned int i, num = 0; > > - if (!panel->desc) > - return 0; > - > for (i = 0; i < panel->desc->num_timings; i++) { > const struct display_timing *dt = &panel->desc->timings[i]; > struct videomode vm; > @@ -131,6 +131,16 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > num++; > } > > + return num; > +} > + > +static unsigned 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; > + unsigned int i, num = 0; > + > for (i = 0; i < panel->desc->num_modes; i++) { > const struct drm_display_mode *m = &panel->desc->modes[i]; > > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > num++; > } > > + return num; > +} > + > +static int panel_simple_get_non_edid_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 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 = 1; > + } else { > + dev_err(drm->dev, "failed to add override mode\n"); > + } > + } > + > + /* Only add timings if override was not there or failed to validate */ > + if (num == 0 && panel->desc->num_timings) > + num = panel_simple_get_timings_modes(panel); > + > + /* > + * Only add fixed modes if timings/override added no mode. > + * > + * We should only ever have either the display timings specified > + * or a fixed mode. Anything else is rather bogus. > + */ > + WARN_ON(panel->desc->num_timings && panel->desc->num_modes); > + if (num == 0) > + num = panel_simple_get_fixed_modes(panel); > + > connector->display_info.bpc = panel->desc->bpc; > connector->display_info.width_mm = panel->desc->size.width; > connector->display_info.height_mm = panel->desc->size.height; > @@ -268,7 +316,7 @@ static int panel_simple_get_modes(struct drm_panel *panel) > } > > /* add hard-coded panel modes */ > - num += panel_simple_get_fixed_modes(p); > + num += panel_simple_get_non_edid_modes(p); > > return num; > } > @@ -299,10 +347,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_parse_override_mode(struct device *dev, > + struct panel_simple *panel, > + const struct display_timing *ot) > +{ > + const struct panel_desc *desc = panel->desc; > + struct videomode vm; > + unsigned int i; > + > + if (WARN_ON(desc->num_modes)) { > + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); > + return; > + } > + if (WARN_ON(!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; > + break; > + } > + > + if (WARN_ON(!panel->override_mode.type)) > + dev_err(dev, "Reject override mode: No display_timing found\n"); > +} > + > 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); > @@ -348,6 +444,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_parse_override_mode(dev, panel, &dt); > + > drm_panel_init(&panel->base); > panel->base.dev = dev; > panel->base.funcs = &panel_simple_funcs;
On Mon, Apr 01, 2019 at 10:17:19AM -0700, Douglas Anderson wrote: > From: 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) > Changes in v4: > - Don't add mode from timing if override was specified (Thierry) > - Add warning if timing and fixed mode was specified (Thierry) > - Don't add fixed mode if timing was specified (Thierry) > - Refactor/rename a bit to avoid extra indentation from "if" tests > - i should be unsigned (Thierry) > - Add annoying WARN_ONs for some cases (Thierry) > - Simplify 'No display_timing found' handling (Thierry) > - Rename to panel_simple_parse_override_mode() (Thierry) > Changes in v5: > - Added Heiko's Tested-by > > 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> > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Heiko Stuebner <heiko@sntech.de> > --- > > drivers/gpu/drm/panel/panel-simple.c | 109 +++++++++++++++++++++++++-- > 1 file changed, 104 insertions(+), 5 deletions(-) Acked-by: Thierry Reding <thierry.reding@gmail.com>
Hi Douglas. Again, long overdue. The review triggered several questions that you should have had a long time ago. Hopefully they makes sense to you. Sam On Mon, Apr 01, 2019 at 10:17:19AM -0700, Douglas Anderson wrote: > From: 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) > Changes in v4: > - Don't add mode from timing if override was specified (Thierry) > - Add warning if timing and fixed mode was specified (Thierry) > - Don't add fixed mode if timing was specified (Thierry) > - Refactor/rename a bit to avoid extra indentation from "if" tests > - i should be unsigned (Thierry) > - Add annoying WARN_ONs for some cases (Thierry) > - Simplify 'No display_timing found' handling (Thierry) > - Rename to panel_simple_parse_override_mode() (Thierry) > Changes in v5: > - Added Heiko's Tested-by > > 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> > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Heiko Stuebner <heiko@sntech.de> > --- > > drivers/gpu/drm/panel/panel-simple.c | 109 +++++++++++++++++++++++++-- > 1 file changed, 104 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 9e8218f6a3f2..ad4f4aac2d44 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 { > @@ -91,6 +92,8 @@ struct panel_simple { > struct i2c_adapter *ddc; > > struct gpio_desc *enable_gpio; > + > + struct drm_display_mode override_mode; I fail to see where this poiter is assigned. > }; > > static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > @@ -98,16 +101,13 @@ static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) > return container_of(panel, struct panel_simple, base); > } > > -static int panel_simple_get_fixed_modes(struct panel_simple *panel) > +static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel) > { > struct drm_connector *connector = panel->base.connector; > struct drm_device *drm = panel->base.drm; > struct drm_display_mode *mode; > unsigned int i, num = 0; > > - if (!panel->desc) > - return 0; > - > for (i = 0; i < panel->desc->num_timings; i++) { > const struct display_timing *dt = &panel->desc->timings[i]; > struct videomode vm; > @@ -131,6 +131,16 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > num++; > } > > + return num; > +} > + > +static unsigned 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; > + unsigned int i, num = 0; > + > for (i = 0; i < panel->desc->num_modes; i++) { > const struct drm_display_mode *m = &panel->desc->modes[i]; > > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > num++; > } > > + return num; > +} > + > +static int panel_simple_get_non_edid_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; This looks suspicious. panel->override_mode.type is an unsigned int that may have a number of bits set. So the above code implicitly convert a .type != 0 to a true. This can be expressed in a much more reader friendly way. And on top of this, I cannot see that panel->override_mode points to a valid instance of display_mode, at least not always. > + unsigned int 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 = 1; > + } else { > + dev_err(drm->dev, "failed to add override mode\n"); > + } > + } > + > + /* Only add timings if override was not there or failed to validate */ > + if (num == 0 && panel->desc->num_timings) > + num = panel_simple_get_timings_modes(panel); > + > + /* > + * Only add fixed modes if timings/override added no mode. This part I fail to understand. If we have a panel where we in panel-simple have specified the timings, and done so using display_timing so with proper {min, typ, max} then it should be perfectly legal to specify a more precise variant in the DT file. Or what did I miss here? > + * > + * We should only ever have either the display timings specified > + * or a fixed mode. Anything else is rather bogus. > + */ > + WARN_ON(panel->desc->num_timings && panel->desc->num_modes); > + if (num == 0) > + num = panel_simple_get_fixed_modes(panel); > + > connector->display_info.bpc = panel->desc->bpc; > connector->display_info.width_mm = panel->desc->size.width; > connector->display_info.height_mm = panel->desc->size.height; > @@ -268,7 +316,7 @@ static int panel_simple_get_modes(struct drm_panel *panel) > } > > /* add hard-coded panel modes */ > - num += panel_simple_get_fixed_modes(p); > + num += panel_simple_get_non_edid_modes(p); > > return num; > } > @@ -299,10 +347,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_parse_override_mode(struct device *dev, > + struct panel_simple *panel, > + const struct display_timing *ot) > +{ > + const struct panel_desc *desc = panel->desc; > + struct videomode vm; > + unsigned int i; > + > + if (WARN_ON(desc->num_modes)) { > + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); > + return; > + } > + if (WARN_ON(!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; The binding do not say anything about flags. Is this check really needed? > + > + 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; > + break; > + } > + > + if (WARN_ON(!panel->override_mode.type)) > + dev_err(dev, "Reject override mode: No display_timing found\n"); > +} > + > 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); > @@ -348,6 +444,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_parse_override_mode(dev, panel, &dt); > + Naming bike-shedding. With the new node name, the function name panel_simple_parse_override_mode() could use an update. Maybe: panel_simple_parse_panel_timing_node() > drm_panel_init(&panel->base); > panel->base.dev = dev; > panel->base.funcs = &panel_simple_funcs; > -- > 2.21.0.392.gf8f6787159e-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Douglas. > > + > > + /* Only add timings if override was not there or failed to validate */ > > + if (num == 0 && panel->desc->num_timings) > > + num = panel_simple_get_timings_modes(panel); > > + > > + /* > > + * Only add fixed modes if timings/override added no mode. > > This part I fail to understand. > If we have a panel where we in panel-simple have specified the timings, > and done so using display_timing so with proper {min, typ, max} then it > should be perfectly legal to specify a more precise variant in the DT > file. > Or what did I miss here? Got it now. If display_mode is used for timings this is what you call "fixed mode". Hmm, if I got confused someone else may also be confused by this naming. Sam
Hi, On Sun, Jun 30, 2019 at 1:55 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Douglas. > > > > + > > > + /* Only add timings if override was not there or failed to validate */ > > > + if (num == 0 && panel->desc->num_timings) > > > + num = panel_simple_get_timings_modes(panel); > > > + > > > + /* > > > + * Only add fixed modes if timings/override added no mode. > > > > This part I fail to understand. > > If we have a panel where we in panel-simple have specified the timings, > > and done so using display_timing so with proper {min, typ, max} then it > > should be perfectly legal to specify a more precise variant in the DT > > file. > > Or what did I miss here? > > Got it now. > If display_mode is used for timings this is what you call "fixed mode". > Hmm, if I got confused someone else may also be confused by this naming. The name "fixed mode" comes from the old code, though I guess in the old code it used to refer to a mode that came from either the display_timing or the display_mode. How about if I call it "panel_simple_get_from_fixed_display_mode"? ...or if you have another suggestion feel free to chime in. NOTE: Since this feedback is minor and this patch has been outstanding for a while (and is blocking other work), I am assuming that the best path forward is for Heiko to land this patch with Thierry's Ack and I'll send a follow-up. Please yell if you disagree. I'll respond to each of your emails separately and then wait for your response before sending the follow-up series. -Doug
Hi, On Sun, Jun 30, 2019 at 1:22 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > @@ -91,6 +92,8 @@ struct panel_simple { > > struct i2c_adapter *ddc; > > > > struct gpio_desc *enable_gpio; > > + > > + struct drm_display_mode override_mode; > I fail to see where this poiter is assigned. In panel_simple_parse_override_mode(). Specifically: drm_display_mode_from_videomode(&vm, &panel->override_mode); > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > num++; > > } > > > > + return num; > > +} > > + > > +static int panel_simple_get_non_edid_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; > This looks suspicious. > panel->override_mode.type is an unsigned int that may have a number of > bits set. > So the above code implicitly convert a .type != 0 to a true. > This can be expressed in a much more reader friendly way. You would suggest that I add a boolean field to a structure to indicate whether an override mode is present? I will certainly do that if you're sure that's what you want, but my understanding of the convention for much of the kernel is that you generally rely on structures being zero-initialized and then (assuming that zero isn't a valid value) it's safe to confirm they've been initialized by seeing that they're non-zero. In this case panel_simple_parse_override_mode() will _definitely_ set a non-zero value for this field in the case that it ran to completion. ...and, even further, the end of that function has a WARN_ON() if it doesn't. Any chance you missed reading panel_simple_parse_override_mode() in the original patch? > And on top of this, I cannot see that panel->override_mode points to a > valid instance of display_mode, at least not always. override_mode isn't a pointer, right? It's a structure straight in the panel. So all its fields will be initted to 0 by the kzalloc in panel_simple_probe(). If the type is non-zero then we know panel_simple_parse_override_mode() finished to completion. > > @@ -268,7 +316,7 @@ static int panel_simple_get_modes(struct drm_panel *panel) > > } > > > > /* add hard-coded panel modes */ > > - num += panel_simple_get_fixed_modes(p); > > + num += panel_simple_get_non_edid_modes(p); > > > > return num; > > } > > @@ -299,10 +347,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_parse_override_mode(struct device *dev, > > + struct panel_simple *panel, > > + const struct display_timing *ot) > > +{ > > + const struct panel_desc *desc = panel->desc; > > + struct videomode vm; > > + unsigned int i; > > + > > + if (WARN_ON(desc->num_modes)) { > > + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); > > + return; > > + } > > + if (WARN_ON(!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; > The binding do not say anything about flags. Is this check really > needed? Flags here is an implementation detail of the Linux driver and the bindings are supposed to be Linux-agnostic. The bindings started out talking about lots of stuff that happened in the driver and I was told to take all those out. ;-) Specifically note that flags here holds whether we have specified a positive or negative for hsync or vsync. These are the "hsync-active" and "vsync-active" properties of the panel bindings. Take a look at of_parse_display_timing(). ...so to summarize the flags here is just a different set of properties to check like the checks above. > > + > > + 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; > > + break; > > + } > > + > > + if (WARN_ON(!panel->override_mode.type)) > > + dev_err(dev, "Reject override mode: No display_timing found\n"); > > +} > > + > > 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); > > @@ -348,6 +444,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_parse_override_mode(dev, panel, &dt); > > + > Naming bike-shedding. > With the new node name, the function name > panel_simple_parse_override_mode() could use an update. > Maybe: panel_simple_parse_panel_timing_node() Happy to change the name to panel_simple_parse_panel_timing_node(). --- Summary of the above: * Unless you say otherwise, I will leave the check of "type != 0" and not introduce a new boolean. * Only action request is rename of panel_simple_parse_override_mode() to panel_simple_parse_panel_timing_node() NOTE: Since this feedback is minor and this patch has been outstanding for a while (and is blocking other work), I am assuming that the best path forward is for Heiko to land this patch with Thierry's Ack and I'll send a follow-up. Please yell if you disagree. I'll respond to each of your emails separately and then wait for your response before sending the follow-up series. -Doug
Hi Dough. On Mon, Jul 01, 2019 at 09:39:24AM -0700, Doug Anderson wrote: > Hi, > > On Sun, Jun 30, 2019 at 1:22 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > @@ -91,6 +92,8 @@ struct panel_simple { > > > struct i2c_adapter *ddc; > > > > > > struct gpio_desc *enable_gpio; > > > + > > > + struct drm_display_mode override_mode; > > I fail to see where this poiter is assigned. > > In panel_simple_parse_override_mode(). Specifically: > > drm_display_mode_from_videomode(&vm, &panel->override_mode); The above code-snippet is only called in the panel has specified display timings using display_timings - it is not called when display_mode is used. So override_mode is only assigned in some cases and not all cases. This needs to be fixed so we do not reference override_mode unless it is set. > > > > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > > num++; > > > } > > > > > > + return num; > > > +} > > > + > > > +static int panel_simple_get_non_edid_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; > > This looks suspicious. > > panel->override_mode.type is an unsigned int that may have a number of > > bits set. > > So the above code implicitly convert a .type != 0 to a true. > > This can be expressed in a much more reader friendly way. > > You would suggest that I add a boolean field to a structure to > indicate whether an override mode is present? A simple bool has_override = panel->override_mode.type != 0; would do the trick here. Then there is no hidden conversion from int to a bool. But as override_mode can be NULL something more needs to be done. Sam
On Mon, Jul 01, 2019 at 09:39:06AM -0700, Doug Anderson wrote: > Hi, > > On Sun, Jun 30, 2019 at 1:55 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > Hi Douglas. > > > > > > + > > > > + /* Only add timings if override was not there or failed to validate */ > > > > + if (num == 0 && panel->desc->num_timings) > > > > + num = panel_simple_get_timings_modes(panel); > > > > + > > > > + /* > > > > + * Only add fixed modes if timings/override added no mode. > > > > > > This part I fail to understand. > > > If we have a panel where we in panel-simple have specified the timings, > > > and done so using display_timing so with proper {min, typ, max} then it > > > should be perfectly legal to specify a more precise variant in the DT > > > file. > > > Or what did I miss here? > > > > Got it now. > > If display_mode is used for timings this is what you call "fixed mode". > > Hmm, if I got confused someone else may also be confused by this naming. > > The name "fixed mode" comes from the old code, though I guess in the > old code it used to refer to a mode that came from either the > display_timing or the display_mode. > > How about if I call it "panel_simple_get_from_fixed_display_mode"? > ...or if you have another suggestion feel free to chime in. What we really want to distingush here is the use of display_mode and display_timings (if I got the names right). That display_mode specify a fixed timing and display_timing specify a valid range is something in the semantics of the two types. So naming that refer to display_mode versus display_timing will make the code simpler to understand. and then a nice comment that when display_mode is used one looses the possibility to use override_mode. That would be fine to have in the struct in the driver. > NOTE: Since this feedback is minor and this patch has been outstanding > for a while (and is blocking other work), I am assuming that the best > path forward is for Heiko to land this patch with Thierry's Ack and > I'll send a follow-up. Please yell if you disagree. Let's give the patches a spin more as we have passed the possibility for the current merge window. I am on vacation at the moment and thus slow in responses, but will be back at the home office next week and will be more responsive again. Sam - who is enjoying the alps in Austria
Sam, On Mon, Jul 8, 2019 at 10:50 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Dough. > > On Mon, Jul 01, 2019 at 09:39:24AM -0700, Doug Anderson wrote: > > Hi, > > > > On Sun, Jun 30, 2019 at 1:22 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > > @@ -91,6 +92,8 @@ struct panel_simple { > > > > struct i2c_adapter *ddc; > > > > > > > > struct gpio_desc *enable_gpio; > > > > + > > > > + struct drm_display_mode override_mode; > > > I fail to see where this poiter is assigned. > > > > In panel_simple_parse_override_mode(). Specifically: > > > > drm_display_mode_from_videomode(&vm, &panel->override_mode); > > The above code-snippet is only called in the panel has specified display > timings using display_timings - it is not called when display_mode is > used. > So override_mode is only assigned in some cases and not all cases. > This needs to be fixed so we do not reference override_mode unless > it is set. I'm afraid I'm not following you here. * override_mode is a structure that's directly part of "struct panel_simple". * The panel is allocated in panel_simple_probe() with devm_kzalloc(). * The "z" in kzalloc means that this memory will be zero-initialized. From the points above, "override_mode" will always be set to something. If we didn't run "drm_display_mode_from_videomode(&vm, &panel->override_mode);" then we know the entire override_mode structure will be zero. While it took a while for me to get used to it, the kernel convention is to rely on zero-initialization and not to explicitly init things to zero. As an example of this being codified in the source, you can see that "checkpatch.pl" will yell at you for a similar thing: "do not initialise globals to 0". > > > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > > > num++; > > > > } > > > > > > > > + return num; > > > > +} > > > > + > > > > +static int panel_simple_get_non_edid_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; > > > This looks suspicious. > > > panel->override_mode.type is an unsigned int that may have a number of > > > bits set. > > > So the above code implicitly convert a .type != 0 to a true. > > > This can be expressed in a much more reader friendly way. > > > > You would suggest that I add a boolean field to a structure to > > indicate whether an override mode is present? > A simple bool has_override = panel->override_mode.type != 0; > would do the trick here. > Then there is no hidden conversion from int to a bool. I will change this to "panel->override_mode.type != 0" if you're really sure, but this seems both against the general Linux style feedback I've received over the years (though there is definitely not 100% consistency) and also against the local convention in this file. Examples in this file of treating ints as bools without an explicit "!= 0": * panel_simple_get_fixed_modes checks "if (panel->desc->bus_format)" * panel_simple_disable checks "if (p->desc->delay.disable)" * panel_simple_unprepare checks "if (p->desc->delay.unprepare)" * panel_simple_prepare checks "if (delay)" * panel_simple_enable checks "if (p->desc->delay.enable)" ...and, although slightly different, pointers in this file are checked for NULL vs. non-NULL without an explicit "== NULL". Of course just because all the other examples in the file do it one way doesn't mean that new code has to do it another way, but I wanted to be really sure you wanted me to go against the existing convention before changing this. > But as override_mode can be NULL something more needs to be done. I'm afraid I don't understand how override_mode can be NULL since it's not a pointer. Can you clarify? -Doug
Hi, On Mon, Jul 8, 2019 at 10:56 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > On Mon, Jul 01, 2019 at 09:39:06AM -0700, Doug Anderson wrote: > > Hi, > > > > On Sun, Jun 30, 2019 at 1:55 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > Hi Douglas. > > > > > > > > + > > > > > + /* Only add timings if override was not there or failed to validate */ > > > > > + if (num == 0 && panel->desc->num_timings) > > > > > + num = panel_simple_get_timings_modes(panel); > > > > > + > > > > > + /* > > > > > + * Only add fixed modes if timings/override added no mode. > > > > > > > > This part I fail to understand. > > > > If we have a panel where we in panel-simple have specified the timings, > > > > and done so using display_timing so with proper {min, typ, max} then it > > > > should be perfectly legal to specify a more precise variant in the DT > > > > file. > > > > Or what did I miss here? > > > > > > Got it now. > > > If display_mode is used for timings this is what you call "fixed mode". > > > Hmm, if I got confused someone else may also be confused by this naming. > > > > The name "fixed mode" comes from the old code, though I guess in the > > old code it used to refer to a mode that came from either the > > display_timing or the display_mode. > > > > How about if I call it "panel_simple_get_from_fixed_display_mode"? > > ...or if you have another suggestion feel free to chime in. > What we really want to distingush here is the use of display_mode > and display_timings (if I got the names right). > That display_mode specify a fixed timing and display_timing specify > a valid range is something in the semantics of the two types. > So naming that refer to display_mode versus display_timing will make the > code simpler to understand. and then a nice comment that when > display_mode > is used one looses the possibility to use override_mode. > That would be fine to have in the struct in the driver. OK, I can change the names here and try to find a good place to add a comment. > > NOTE: Since this feedback is minor and this patch has been outstanding > > for a while (and is blocking other work), I am assuming that the best > > path forward is for Heiko to land this patch with Thierry's Ack and > > I'll send a follow-up. Please yell if you disagree. > Let's give the patches a spin more as we have passed the possibility for > the current merge window. Any way I can convince you to change your mind here? There are no functional changes requested so far in your feedback and no bugs--it's just a few variable names and comments. By landing the existing patches as-is: 1. We stop spamming all the people CCed on this whole series (which includes device tree patches) that might be interested in the series as a whole but aren't interested in details. 2. We can debate the bikeshed-type issues on their own merit and I don't have to debate removing existing Acks / Reviewed-by / Tested-by tags as I make changes. 3. Even if it's not a good time to land the patches right now we know that these patches will be ready to land as soon as the window opens. As I mentioned earlier these patches are blocking other work [1] and landing that patch is actually preventing Matthias from submitting another series of patches to add support for rk3288-veyron-tiger and rk3288-veyron-fievel. Certainly I know that upstream doesn't make a policy of landing things just to suit the timelines of a downstream project, but in this case there seems very little downsides to landing the existing patches and taking a later cleanup patch. > I am on vacation at the moment and thus slow in responses, but will be back > at the home office next week and will be more responsive again. > > Sam - who is enjoying the alps in Austria Hope you have had a great vacation! [1] https://lkml.kernel.org/r/20190625222629.154619-1-mka@chromium.org -Doug
On Wed, Jul 10, 2019 at 6:56 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jul 8, 2019 at 10:56 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > On Mon, Jul 01, 2019 at 09:39:06AM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Sun, Jun 30, 2019 at 1:55 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > > > Hi Douglas. > > > > > > > > > > + > > > > > > + /* Only add timings if override was not there or failed to validate */ > > > > > > + if (num == 0 && panel->desc->num_timings) > > > > > > + num = panel_simple_get_timings_modes(panel); > > > > > > + > > > > > > + /* > > > > > > + * Only add fixed modes if timings/override added no mode. > > > > > > > > > > This part I fail to understand. > > > > > If we have a panel where we in panel-simple have specified the timings, > > > > > and done so using display_timing so with proper {min, typ, max} then it > > > > > should be perfectly legal to specify a more precise variant in the DT > > > > > file. > > > > > Or what did I miss here? > > > > > > > > Got it now. > > > > If display_mode is used for timings this is what you call "fixed mode". > > > > Hmm, if I got confused someone else may also be confused by this naming. > > > > > > The name "fixed mode" comes from the old code, though I guess in the > > > old code it used to refer to a mode that came from either the > > > display_timing or the display_mode. > > > > > > How about if I call it "panel_simple_get_from_fixed_display_mode"? > > > ...or if you have another suggestion feel free to chime in. > > What we really want to distingush here is the use of display_mode > > and display_timings (if I got the names right). > > That display_mode specify a fixed timing and display_timing specify > > a valid range is something in the semantics of the two types. > > So naming that refer to display_mode versus display_timing will make the > > code simpler to understand. and then a nice comment that when > > display_mode > > is used one looses the possibility to use override_mode. > > That would be fine to have in the struct in the driver. > > OK, I can change the names here and try to find a good place to add a comment. > > > > > NOTE: Since this feedback is minor and this patch has been outstanding > > > for a while (and is blocking other work), I am assuming that the best > > > path forward is for Heiko to land this patch with Thierry's Ack and > > > I'll send a follow-up. Please yell if you disagree. > > Let's give the patches a spin more as we have passed the possibility for > > the current merge window. > > Any way I can convince you to change your mind here? There are no > functional changes requested so far in your feedback and no bugs--it's > just a few variable names and comments. By landing the existing > patches as-is: > > 1. We stop spamming all the people CCed on this whole series (which > includes device tree patches) that might be interested in the series > as a whole but aren't interested in details. > > 2. We can debate the bikeshed-type issues on their own merit and I > don't have to debate removing existing Acks / Reviewed-by / Tested-by > tags as I make changes. > > 3. Even if it's not a good time to land the patches right now we know > that these patches will be ready to land as soon as the window opens. > As I mentioned earlier these patches are blocking other work [1] and > landing that patch is actually preventing Matthias from submitting > another series of patches to add support for rk3288-veyron-tiger and > rk3288-veyron-fievel. Certainly I know that upstream doesn't make a > policy of landing things just to suit the timelines of a downstream > project, but in this case there seems very little downsides to landing > the existing patches and taking a later cleanup patch. > [sending from my @chromium.org address so any appearance of bias is explicit :) ] Agree with Doug here, the naming and casting discussion is pretty subjective and non-functional. We've got an Ack from Thierry and a Review from Boris (both seasoned drm_panel'ers), this patch has been sitting in review for a while. Let's not let the perfect be the enemy of the good. Sean > > > I am on vacation at the moment and thus slow in responses, but will be back > > at the home office next week and will be more responsive again. > > > > Sam - who is enjoying the alps in Austria > > Hope you have had a great vacation! > > [1] https://lkml.kernel.org/r/20190625222629.154619-1-mka@chromium.org > > -Doug
Hi Doug. > > > > > @@ -91,6 +92,8 @@ struct panel_simple { > > > > > struct i2c_adapter *ddc; > > > > > > > > > > struct gpio_desc *enable_gpio; > > > > > + > > > > > + struct drm_display_mode override_mode; > > > > I fail to see where this poiter is assigned. > > > > > > In panel_simple_parse_override_mode(). Specifically: > > > > > > drm_display_mode_from_videomode(&vm, &panel->override_mode); > > > > The above code-snippet is only called in the panel has specified display > > timings using display_timings - it is not called when display_mode is > > used. > > So override_mode is only assigned in some cases and not all cases. > > This needs to be fixed so we do not reference override_mode unless > > it is set. > > I'm afraid I'm not following you here. > > * override_mode is a structure that's directly part of "struct panel_simple". I had somehow confused myself to think this was a pointer. And you are right that override_mode is properly initialized when the structure is allocated. Sorry for not reading the code and your replies carefully enough. Sam
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 9e8218f6a3f2..ad4f4aac2d44 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 { @@ -91,6 +92,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) @@ -98,16 +101,13 @@ static inline struct panel_simple *to_panel_simple(struct drm_panel *panel) return container_of(panel, struct panel_simple, base); } -static int panel_simple_get_fixed_modes(struct panel_simple *panel) +static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel) { struct drm_connector *connector = panel->base.connector; struct drm_device *drm = panel->base.drm; struct drm_display_mode *mode; unsigned int i, num = 0; - if (!panel->desc) - return 0; - for (i = 0; i < panel->desc->num_timings; i++) { const struct display_timing *dt = &panel->desc->timings[i]; struct videomode vm; @@ -131,6 +131,16 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) num++; } + return num; +} + +static unsigned 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; + unsigned int i, num = 0; + for (i = 0; i < panel->desc->num_modes; i++) { const struct drm_display_mode *m = &panel->desc->modes[i]; @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) num++; } + return num; +} + +static int panel_simple_get_non_edid_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 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 = 1; + } else { + dev_err(drm->dev, "failed to add override mode\n"); + } + } + + /* Only add timings if override was not there or failed to validate */ + if (num == 0 && panel->desc->num_timings) + num = panel_simple_get_timings_modes(panel); + + /* + * Only add fixed modes if timings/override added no mode. + * + * We should only ever have either the display timings specified + * or a fixed mode. Anything else is rather bogus. + */ + WARN_ON(panel->desc->num_timings && panel->desc->num_modes); + if (num == 0) + num = panel_simple_get_fixed_modes(panel); + connector->display_info.bpc = panel->desc->bpc; connector->display_info.width_mm = panel->desc->size.width; connector->display_info.height_mm = panel->desc->size.height; @@ -268,7 +316,7 @@ static int panel_simple_get_modes(struct drm_panel *panel) } /* add hard-coded panel modes */ - num += panel_simple_get_fixed_modes(p); + num += panel_simple_get_non_edid_modes(p); return num; } @@ -299,10 +347,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_parse_override_mode(struct device *dev, + struct panel_simple *panel, + const struct display_timing *ot) +{ + const struct panel_desc *desc = panel->desc; + struct videomode vm; + unsigned int i; + + if (WARN_ON(desc->num_modes)) { + dev_err(dev, "Reject override mode: panel has a fixed mode\n"); + return; + } + if (WARN_ON(!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; + break; + } + + if (WARN_ON(!panel->override_mode.type)) + dev_err(dev, "Reject override mode: No display_timing found\n"); +} + 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); @@ -348,6 +444,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_parse_override_mode(dev, panel, &dt); + drm_panel_init(&panel->base); panel->base.dev = dev; panel->base.funcs = &panel_simple_funcs;