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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Cc: Eric Anholt <eric@xxxxxxxxxx>
> Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> Cc: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Stéphane Marchesin <marcheu@xxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
